-
Notifications
You must be signed in to change notification settings - Fork 215
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Try to get lease namespace if unspecified #1450
Conversation
@@ -59,8 +68,8 @@ public LeaderElectionConfiguration( | |||
this.identity = identity; | |||
} | |||
|
|||
public String getLeaseNamespace() { | |||
return leaseNamespace; | |||
public Optional<String> getLeaseNamespace() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this is a breaking change. I can mark it as deprecated and provide another method getOptionalLeaseNamespace
, if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh normally I would prefer backwards compatible with deprecated. getOptionalLeaseNamespace
does not sound too good, maybe just stick with this this time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably would be better, yes. Then again, I don't know if anyone is using Leader Election already…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for users, direct usage of this API will be rare even they do use leader election feature. Probably only in unit test.
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java
Show resolved
Hide resolved
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java
Show resolved
Hide resolved
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java
Show resolved
Hide resolved
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java
Outdated
Show resolved
Hide resolved
if (id == null || id.isBlank()) { | ||
id = UUID.randomUUID().toString(); | ||
} | ||
return id; | ||
} | ||
|
||
private static Optional<String> tryNamespaceFromPath() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is needed, could you try instead just use the client's facilities, like:
Config.autoConfigure(null).getNamespace()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Would this lead to the same NS? And actually, we should probably get it from ConfigurationServiceProvider.instance().getClientConfiguration()
instead of creating a new Config
object that might not match the configured one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default Config getClientConfiguration() {
return Config.autoConfigure(null);
}
This seems create a new config as well.
I can give it a try and see whether those tests still work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could get tests pass by setting System.setProperty(KUBERNETES_AUTH_TRYKUBECONFIG_SYSTEM_PROPERTY, "false");
, but I worry that loading a full config here might not be a correct approach because it confuses users if they do not expect .kube/config
to be read at all. The intention of this change is to only make it work in-cluster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, maybe we can then just stick with the current impl for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems create a new config as well.
That's just the default implementation, though. The Quarkus extension, for example, returns the configuration associated with the client (i.e. the one the client uses to connect to the cluster).
Are there use cases where the config would be configured to use a different namespace than the one associated with the service account? Do we specifically only want the SA namespace regardless of how the Kubernetes config is set up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@metacosm OK I see what you mean now. Let me try something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So something like 0c12947 ? If the config was loaded from .kube/config
, there is for sure no namespace; if it is in-cluster config, the namespace will be read from /var/run/secrets/kubernetes.io/serviceaccount/namespace
if no overridden.
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java
Show resolved
Hide resolved
@@ -59,8 +68,8 @@ public LeaderElectionConfiguration( | |||
this.identity = identity; | |||
} | |||
|
|||
public String getLeaseNamespace() { | |||
return leaseNamespace; | |||
public Optional<String> getLeaseNamespace() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh normally I would prefer backwards compatible with deprecated. getOptionalLeaseNamespace
does not sound too good, maybe just stick with this this time.
if (id == null || id.isBlank()) { | ||
id = UUID.randomUUID().toString(); | ||
} | ||
return id; | ||
} | ||
|
||
private static Optional<String> tryNamespaceFromPath() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could reasonably ask for the related method to become public in the Fabric8 client so that we don't have to maintain it here…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that would be better. Do you think I could give it a shot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will give it a try. On the other hand, if we want to get this shipped, we don't need to get blocked because the roundtrip might take some time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, we can adjust it here after, and ship this as it is. Maybe just create an issue so we don't forget it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the fence regarding the change in API, LGTM otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@csviri I guess we can. :) Strictly speaking it is still more like a feature, but given this small breaking change, shipping it sooner seems to be better. |
I'm making a small tweak. |
var namespacePath = tempDir.resolve("namespace"); | ||
Files.writeString(namespacePath, namespace); | ||
|
||
System.setProperty(KUBERNETES_AUTH_TRYKUBECONFIG_SYSTEM_PROPERTY, "false"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why it's needed to set this property to false
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is because of https://github.com/fabric8io/kubernetes-client/blob/0742e942eb09f8ed2672f7f82e176b76e595ad50/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/Config.java#L600. Otherwise it will try to read .kube/config
. By setting this the test case makes sure it tries to read the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm however still not sure this gets the correct client config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order also seems to be wrong:
configurationService.getLeaderElectionConfiguration()
.ifPresent(c -> leaderElectionManager.init(c, this.kubernetesClient));
ConfigurationServiceProvider.set(configurationService);
Now the PR as it is, tries to access ConfigurationServiceProvider.instance()
before ConfigurationServiceProvider.set(configurationService)
is called.
For the use case creating k8s client manually, this seems to be required but maybe it is supposed to be so?
var operator =
new Operator(
kubernetesClient,
configurationServiceOverrider ->
configurationServiceOverrider
.withCloseClientOnStop(false)
.withClientConfiguration(kubernetesClient.getConfiguration()));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, the instance is already set in ConfigurationServiceProvider::overrideCurrent
, so ConfigurationServiceProvider.set(configurationService);
doesn't do anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean manually created? In the background it always created, if the namespace is not set it should use always one where the pod is running.
For example:
var kubernetesClient = ...; # build the client; may not be from .kube/config
var operator =
new Operator(
kubernetesClient,
configurationServiceOverrider ->
configurationServiceOverrider
.withCloseClientOnStop(false)
.withClientConfiguration(kubernetesClient.getConfiguration()));
If here withClientConfiguration
is not called, I think a default configuration will be created, which might be different than the configuration used to build kubernetesClient
. Then we have an inconsistency. I'm not sure of the consequence though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is absolutely true. The thing is that on the other hand on quarkus extension it would make issues when the config from config service not used. Note however that if the namespace is configured for a client, it's not our api.
So we have api for namespace config on controller level. And we have api for leader election to config namespace. So I would expect that the leader election is either uses the pod namespace or a setting explicitly for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I understand that. I think as you said this is a separated issue to track, not related to leader election. I can create a new issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modified the existing e2e test a bit. PTAL, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably need to clean things up or, at the very least, document them better. The intention is to create client instance(s) based on the configuration retrieved from the ConfigurationService
. I'm not sure that we do follow that ourselves in our code but we should review it. You should only create client instances with a different configuration only for very specific purposes.
536f3ed
to
87315bb
Compare
var namespacePath = tempDir.resolve("namespace"); | ||
Files.writeString(namespacePath, namespace); | ||
|
||
System.setProperty(KUBERNETES_AUTH_TRYKUBECONFIG_SYSTEM_PROPERTY, "false"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably need to clean things up or, at the very least, document them better. The intention is to create client instance(s) based on the configuration retrieved from the ConfigurationService
. I'm not sure that we do follow that ourselves in our code but we should review it. You should only create client instances with a different configuration only for very specific purposes.
Could you please help merge this so it doesn't stay too long and get conflict? Thank you. |
yep, fine by me, @metacosm could you take a look pls? |
I already approved it… 😁 |
The only question that remains is whether we should merge it in |
If going to |
I think this is kinda gray area, it's not a bug fix, rather an improvement IMHO. But fine with merging it to the main, and have a release. |
Yeah agreed. I don't have a strong opinion. Either way is ok for me. |
merged to main, thank you @honnix !! |
Thank you all! |
Thank you! |
This PR makes providing lease namespace optional. If unspecified, it will try to read from a file pointed by system property or env var
kubenamespace
with default value as/var/run/secrets/kubernetes.io/serviceaccount/namespace
which is injected by k8s. The code is inspired by https://github.com/fabric8io/kubernetes-client/blob/master/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/Config.java.The idea is to simplify use code because in most cases users will need to do the same. Please let me know if this makes sense. Thanks.
@csviri @metacosm