Fix 1592 empty source on error #1792
Conversation
Signed-off-by: wind57 <[email protected]>
| * | ||
| * @author wind57 | ||
| */ | ||
| public class VisibleFabric8SecretsPropertySourceLocator extends Fabric8SecretsPropertySourceLocator { |
There was a problem hiding this comment.
this one is in src/test/java and needed only for tests
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.springframework.cloud.kubernetes.fabric8.config; |
There was a problem hiding this comment.
its only in /src/test/java
| @@ -162,6 +163,12 @@ else if (propertySource instanceof CompositePropertySource source) { | |||
| } | |||
|
|
|||
| static boolean changed(List<? extends MapPropertySource> k8sSources, List<? extends MapPropertySource> appSources) { | |||
|
|
|||
| if (k8sSources.stream().anyMatch(source -> source.getName().equals(SourceData.EMPTY_SOURCE_NAME_ON_ERROR))) { | |||
There was a problem hiding this comment.
this is actually the fix, we check to see if the source name that was generated is a well known one, that we create only when there was an error reading the configmaps/secrets from k8s
| /** | ||
| * source name that is generated when there is an error reading the underlying | ||
| * configmap(s) or secret(s). | ||
| */ |
There was a problem hiding this comment.
name to for the source when we failed reading from k8s api
There was a problem hiding this comment.
We could have a source with this name. but I realize that might never happen. I wonder if another approach would be to subclass this class and call it ErrorSourceData and then just check if the source is an instance of that class. Thoughts?
There was a problem hiding this comment.
this is exactly what I wanted to do initially, the issue is that at the time when we run a cycle, for example via polling reload, we do not deal with SourceData anymore, we deal with PropertySource(s).
There was a problem hiding this comment.
Where does that translation from SourceData to PropertySource happen? Could we then create a ErrorPropertySource then?
There was a problem hiding this comment.
hmm, overall a very good idea. the issue is that SourceData is a record and that can not be extended, but I can try to add an enum to differentiate between them. I'll try to see if that is possible tomorrow and update you
There was a problem hiding this comment.
so instead of having a source with a pre-defined name in case of error, add a property that would indicate that. Did I understand you correctly?
There was a problem hiding this comment.
sorry for bugging you, but in this case we still need a name for it. So we have this code:
catch (Exception e) {
LOG.warn("failure in reading named sources");
onException(failFast, e);
return SourceData.emptyRecord(EMPTY_SOURCE_NAME_ON_ERROR);
}
if we want to flip a property, we would still need to generate a name for this source data. We could have it exactly like the bug has it : configmap..default (yes, two dots there, since we do not know the name, there was an error after all).
In the end this name would be irrelevant, since we are not going to store such a property source in the environment, plus we are going to check for a property, not the name.
I just want to make sure we both are on par here
There was a problem hiding this comment.
So really the only problem is that we log a message with funky looking name?
There was a problem hiding this comment.
Me confused :) the problem is that in case of an error, we are going to put in our environment a property source with indeed a funky name. Putting it there means we break reload, forever from that point in time. And while writing this comment, I realized that your previous comment makes total sense and I can definitely flip a property...
I'll do that tomorrow
| @@ -69,7 +70,9 @@ public final SourceData compute(String sourceName, ConfigUtils.Prefix prefix, St | |||
|
|
|||
| } | |||
| catch (Exception e) { | |||
| LOG.warn("failure in reading named sources"); | |||
| onException(failFast, e); | |||
There was a problem hiding this comment.
in case of Exception, return a source with a known name that later can be checked for
| @@ -54,28 +54,54 @@ class KubernetesClientSecretsPropertySourceLocatorTests { | |||
|
|
|||
| private static final String LIST_API = "/api/v1/namespaces/default/secrets"; | |||
|
|
|||
| private static final String LIST_BODY = "{\n" + "\t\"kind\": \"SecretList\",\n" + "\t\"apiVersion\": \"v1\",\n" | |||
There was a problem hiding this comment.
just formatting, nothing else changed here
| @@ -80,28 +80,54 @@ class KubernetesClientSecretsPropertySourceTests { | |||
|
|
|||
| private static final String LIST_API_WITH_LABEL = "/api/v1/namespaces/default/secrets"; | |||
|
|
|||
| private static final String LIST_BODY = "{\n" + "\t\"kind\": \"SecretList\",\n" + "\t\"apiVersion\": \"v1\",\n" | |||
|
@ryanjbaxter this is ready to be looked at. The fix itself is not complicated, but testing was, since I did not want to add integration tests. Thank you |
|
I LIKE IT! |
|
@ryanjbaxter this one is ready, but please do not close the bug just yet, because I need to look at something more, another minor PR might be needed. I will let you know |
No description provided.