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

Do not rely on whatever kubernetes context is current at the client (allow specifying context) #453

Merged
merged 2 commits into from Jul 16, 2021

Conversation

skarsaune
Copy link
Contributor

Not specifying context can be highly confusing for users switching between different clusters. Allow connections to specify what context to use.

@skarsaune
Copy link
Contributor Author

Note: Everything points to the YAML parsing (jackson + snakeyaml) being extremely slow. For the fabric8 kubernetes client, updating jackson could be a thing. FasterXML/jackson-dataformats-text#67

@skarsaune
Copy link
Contributor Author

@rhuss : I'm wondering whether it should also be possible to specify context in a query param in the url, as it may be quite confusing in situations where the env map is not easily specified (such as in jconsole/jvisualvm/jmc). e.g.
"service:jmx:jolokia:///mynamespace/mypod:myport/mypath?context=my-kubernetes-context"

@rhuss
Copy link
Member

rhuss commented Jun 8, 2021

Hi @skarsaune sorry for being again late and I'm a bit out-of-context again.

I'm a bit worried that the connect URL is not self-contained as it still relies on an external context (~/.kube/config). But it's probably to cumbersome to specify the full configuration as query parameters, so I think using an environment variable is fine.

However, I would not make the context configurable but the location of the config file. This is how it is down typically, I.e. to set the KUBECONFIG environment variable and evaluate this. For managing multiple configurations its also easier to use multiple configurations files (which typically contain one context), instead of managing multiple contexts in the same configuration file.

@skarsaune
Copy link
Contributor Author

skarsaune commented Jun 9, 2021

Hi @skarsaune sorry for being again late and I'm a bit out-of-context again.

I'm a bit worried that the connect URL is not self-contained as it still relies on an external context (~/.kube/config). But it's probably to cumbersome to specify the full configuration as query parameters, so I think using an environment variable is fine.

However, I would not make the context configurable but the location of the config file. This is how it is down typically, I.e. to set the KUBECONFIG environment variable and evaluate this. For managing multiple configurations its also easier to use multiple configurations files (which typically contain one context), instead of managing multiple contexts in the same configuration file.

Hmm, that may be a good pattern for advanced users, but I thing multiple contexts in the config is a quite common use case. So for instance AKS, EKS and gke will store config to the default config, and to use config get-contexts and config use-context in your routine. I can test, but I believe the client will honour the conventions (env var etc) on locating the config to use (?), so in that use case it should work out of the box (as it takes the one context there is). I can verify that it works with env var. The env map could of course support a "top override" for config path, that takes precedence over the current resolution order. (As there may be difficult or risky to control environment variables of your JVM)
For instance if you have Java mission control open and wish to connect to pods in different clusters, environment variables are not that flexible.

@skarsaune
Copy link
Contributor Author

I have tried to debug a little. There is no direct support for query parameters in the JMX service url API.
We could split path and query using using java.net.URI
and then ok http HttpUrl which is already on classpath to parse the query parameters.
Not the most elegant perhaps, but it would support the self-contained-ness of the URL. We could support this along the JMX env map (which is used by tools already)

@rhuss
Copy link
Member

rhuss commented Jul 5, 2021

Ok, let's stick with the context file then, but KUBECONFIG should be supported, too. But as you say this should work out of the box anyway (because the kubernetes client evaluates that env variable)

wrt/ self contained-ness of the connect uri: I guess this would be helpful, but on the other side, this also exposes credential information in the URL (so that it can appear in logs). If so we might want to provide both options. Regardless of what, it's up to you and let the PR merge asap. Just tell me in which direction you want to go. I plan a release also soonish when #464 has landed.

@skarsaune
Copy link
Contributor Author

I'm happy with the current state of the PR. It will allow supporting this in JMC:
image
A release would be great. Should I also sumbit some documentation on connecting with jolokia or kubernetes proxy? (I guess that can also be fixed post release)

@rhuss
Copy link
Member

rhuss commented Jul 16, 2021

A release would be great. Should I also sumbit some documentation on connecting with jolokia or kubernetes proxy? (I guess that can also be fixed post release)

Documentation is always great ;-) I think it makes totally sense to add this to the Jolokia manual, but as you said, we can do this post-release, but it would be cool if we would have some basic docs that can be referenced from the release notes.

Let's merge the PR now, the release will probably happen either over the weekend or at least next week (hopefully)

@rhuss rhuss merged commit 72f8ca0 into jolokia:master Jul 16, 2021
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 this pull request may close these issues.

None yet

2 participants