search

spring-cloud / spring-cloud-kubernetes

Kubernetes integration with Spring Cloud Discovery Client, Configuration, etc...
Apache License 2.0
3.43k stars 1.03k forks source link

Fabric8 LeaderProperties.getNamespace(defaultValue) no longer falls back to given default when namespace is undefined #1661

Closed kzander91 closed 3 weeks ago

kzander91 commented 1 month ago

Describe the bug Starting with Spring Cloud 2023.0.2/Spring Cloud Kubernetes 3.1.2, LeaderProperties.getNamespace(String default) is broken. As of commit 53fa5555, the method no longer falls back to the given defaultValue when namespace is not defined.

With 3.1.1, we return the defaultValue if the namespace is undefined (as expected): https://github.com/spring-cloud/spring-cloud-kubernetes/blob/2c9e4f07691c94960aa929f653b3705d91a930d9/spring-cloud-kubernetes-commons/src/main/java/org/springframework/cloud/kubernetes/commons/leader/LeaderProperties.java#L121-L127 With 3.1.2, we return the namespace if the defaultValue is defined (unexpected): https://github.com/spring-cloud/spring-cloud-kubernetes/blob/979bc62c6f0c57f1cba8771a7946add79057e980/spring-cloud-kubernetes-commons/src/main/java/org/springframework/cloud/kubernetes/commons/leader/LeaderProperties.java#L108-L114

This was likely the intention:

public String getNamespace(String defaultValue) {
    if (!StringUtils.hasText(namespace)) {
        return defaultValue;
    }

    return namespace;
}

Applications that are not configuring spring.cloud.kubernetes.leader.namespace explicitly, relying on the auto-detected current namespace as the default will no longer work.

This is was accidentally introduced by the Fabric8 Leader refactoring made with #1648.

Impact My application fails to start with this error:

io.fabric8.kubernetes.client.KubernetesClientException: namespace cannot be null
    at io.fabric8.kubernetes.client.dsl.internal.BaseOperation.inNamespace(BaseOperation.java:252)
    at io.fabric8.kubernetes.client.dsl.internal.BaseOperation.inNamespace(BaseOperation.java:97)
    at org.springframework.cloud.kubernetes.fabric8.leader.Fabric8LeaderRecordWatcher.lambda$start$1(Fabric8LeaderRecordWatcher.java:63)
    at org.springframework.cloud.kubernetes.commons.leader.LeaderUtils.guarded(LeaderUtils.java:51)
    at org.springframework.cloud.kubernetes.fabric8.leader.Fabric8LeaderRecordWatcher.start(Fabric8LeaderRecordWatcher.java:59)
    at org.springframework.cloud.kubernetes.commons.leader.LeaderInitiator.start(LeaderInitiator.java:62)
    at org.springframework.context.support.DefaultLifecycleProcessor.doStart(DefaultLifecycleProcessor.java:288)
wind57 commented 1 month ago

oh snap! you're right, I introduced this one.

I'll fix it and add tests for it, apparently we had none covering this case...

Thank you for raising this!

@ryanjbaxter can you add the bug label here please?

wind57 commented 3 weeks ago

I have a fix ready here

Also, I am trying to get a different (native) approach on the leader election process here