Skip to content
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

io.fabric8.kubernetes.client.Config should expose all and the current context defined in kubeconfig #2215

Closed
adietish opened this issue May 12, 2020 · 11 comments · Fixed by #2290

Comments

@adietish
Copy link
Contributor

adietish commented May 12, 2020

Ex. If you need to list all clusters that are present in your kubeconfig there's no other way than to (re-) read and (re-) parse ~/.kube/config. io.fabric8.kubernetes.client.Config should therefore expose this info that is present in io.fabric8.kubernetes.api.model.Config.
There are also more values in the kubeconfig that a 3rd party programmer might want/need (ex. current cluster, etc.).

@adietish
Copy link
Contributor Author

adietish commented May 12, 2020

I can contribute a PR, all I need is to know if you're ok with exposing io.fabric8.kubernetes.api.model.Config or if you'd prefer to hide it behind io.fabric8.kubernetes.client.Config and expose specific values.

@adietish adietish changed the title io.fabric8.kubernetes.client.Config should expose all clusters defined in kubeconfig io.fabric8.kubernetes.client.Config should expose all clusters (and other values) defined in kubeconfig May 12, 2020
@rohanKanojia
Copy link
Member

@adietish : @manusa @oscerd thoughts?? Hiding io.fabric8.kubernetes.client.Config and exposing specific values sound good to me. Maybe it would be less confusing.

@oscerd
Copy link
Member

oscerd commented May 12, 2020

+1

@adietish
Copy link
Contributor Author

@rohanKanojia you mean hiding io.fabric8.kubernetes.api.model.Config (the resource class which is deserialized) which is currently hidden behind the facade io.fabric8.kubernetes.client.Config?
If this is what you're after, works perfectly for me.

@manusa
Copy link
Member

manusa commented May 13, 2020

Hiding io.fabric8.kubernetes.client.Config and exposing specific values sound good to me. Maybe it would be less confusing.

I have the same confusion as @adietish. Do you mean exposing certain fields (supposedly parsed from io.fabric8...model.Config) in io.fabric8...client.Config? Yes, I think it's safe enough (and probably quite useful) to expose the current configuration values.

Back to the original issue, I'm not sure that what you can find in client.Config matches what you find in your deserialized local ~/.kube/config into model.Config. In case of a completely autoconfigured KuberenetesClient, this should of course be the same. But in case the client was configured manually, the configuration settings won't match with those in your local ~/.kube/config (maybe the file was never parsed > kubernetes.auth.tryKubeConfig=false).

I see there is helper a method in KubeConfigUtils to deserialize configurations. Even after exposing the active configuration values/settings in client.Config, I'm not sure, but maybe it's safer to deserialize again the file to be sure you're retrieving the right information.

@adietish
Copy link
Contributor Author

Hiding io.fabric8.kubernetes.client.Config and exposing specific values sound good to me. Maybe it would be less confusing.

Yes, I think it's safe enough (and probably quite useful) to expose the current configuration values.

ok, great.

But in case the client was configured manually, the configuration settings won't match with those in your local ~/.kube/config (maybe the file was never parsed > kubernetes.auth.tryKubeConfig=false).

Thx for the insight, wasn't aware, that's a very helpful pointer. What are the consequences exactly?

I see there is helper a method in KubeConfigUtils to deserialize configurations. Even after exposing the active configuration values/settings in client.Config, I'm not sure, but maybe it's safer to deserialize again the file to be sure you're retrieving the right information.

I'd think that one would be interested in the overriding values, not the overridden ones. And for the ones that might not be available (kubernetes.auth.tryKubeConfig=false) load&parse as you pointed out?

@manusa
Copy link
Member

manusa commented May 13, 2020

Sorry, I think I misunderstood the question/issue.

I'm understanding 2 different things here:

  1. "client.Config should expose all clusters defined in ~/.kube/config" For this case what I'm saying is that maybe in the first place that config was never parsed to start with. So if you want to read a value that's there, the safest option is to parse the file again.

  2. "client.Config should expose all other values": I would rephrase that to should expose all values active in the current client configuration. e.g. I want to know which is the current masterUrl in use.

I don't know if this is what you're actually asking/proposing.

Anyway, client.Config should definitely expose current configuration values (if it doesn't already).

@adietish
Copy link
Contributor Author

adietish commented Jun 12, 2020

Hi @manusa

  1. "client.Config should expose all clusters defined in ~/.kube/config" For this case what I'm saying is that maybe in the first place that config was never parsed to start with. So if you want to read a value that's there, the safest option is to parse the file again.

Would you agree that the kubernetes-client should do this (if requested) so that user programmers would not have to replicate the code?

@adietish
Copy link
Contributor Author

adietish commented Jun 12, 2020

Hi @manusa

  1. "client.Config should expose all other values": I would rephrase that to should expose all values active in the current client configuration. e.g. I want to know which is the current masterUrl in use.

"all active values" is too wide in the context of my requirements while it's of course completely valid on its own.
I have the following requirements:

  • I need to list all contexts
  • I need to know which one of these is the current one
  • I need to display the contexts by their name

Here's the background to it:
We list all contexts and allow the user to browse the current one:

image

KubeConfigUtils.getCurrentContext(config) currently allows me to extract the current io.fabric8.kubernetes.api.model.Context. Context unfortunately isn't good enough for me, I need to display the name of it and thus need io.fabric8.kubernetes.api.model.NamedContext instead. Backing up this requirement is that we freely allow the user to use a context and this is currently allowed by instantiating DefaultKubernetesClient with a given config. The config then may be set to the current context via a named context (Kotlin):

val config = Config.autoConfigure(context.name)
val k8Client = DefaultKubernetesClient(config)

@adietish adietish changed the title io.fabric8.kubernetes.client.Config should expose all clusters (and other values) defined in kubeconfig io.fabric8.kubernetes.client.Config should expose all and the current context defined in kubeconfig Jun 12, 2020
@manusa
Copy link
Member

manusa commented Jun 16, 2020

Hi @adietish
Sorry for the delay, just saw your updates.

Would you agree that the kubernetes-client should do this (if requested) so that user programmers would not have to replicate the code?

Yes, sure. Whatever improves developer's experience is welcome ;) Especially if it reuses internal methods, so having a clean public API to expose that seems pretty good (IMHO).

In relation to #2215 (comment)
I think changing the method return type (as you did in #2290) is the right approach to fulfill your requirements.

@adietish
Copy link
Contributor Author

adietish commented Jun 16, 2020

@manusa here's a naive implementation: #2290. Please review.

adietish added a commit to adietish/kubernetes-client that referenced this issue Jun 17, 2020
adietish added a commit to adietish/kubernetes-client that referenced this issue Jun 17, 2020
adietish added a commit to adietish/kubernetes-client that referenced this issue Jun 17, 2020
adietish added a commit to adietish/kubernetes-client that referenced this issue Jun 17, 2020
adietish added a commit to adietish/kubernetes-client that referenced this issue Jun 17, 2020
adietish added a commit to adietish/kubernetes-client that referenced this issue Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants