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

Configuration should use proxy-url if provided in .kube/config #6150

Closed
hudac opened this issue Jul 18, 2024 · 7 comments · Fixed by #6289
Closed

Configuration should use proxy-url if provided in .kube/config #6150

hudac opened this issue Jul 18, 2024 · 7 comments · Fixed by #6289
Assignees
Labels
Milestone

Comments

@hudac
Copy link

hudac commented Jul 18, 2024

Is your enhancement related to a problem? Please describe

The ~/.kube/config file supports a proxy-url attribute per cluster entry, like:

- cluster:
  name: cluster-eks-42
    server: https://xxx.eks.amazonaws.com
    certificate-authority-data: LS0tLS1...
    proxy-url: socks5://localhost:1080

This is currently not taken into account, only env/system properties (i.e. all.proxy) apply.

Having this config per cluster is ideal when working with multiple clusters, with different (or for some none) proxy settings.

Describe the solution you'd like

Getting this per cluster proxy setting in ~/.kube/config in sync with env/system properties driven proxy settings, I'd suggest that the per cluster setting in ~/.kube/config should prevail.

For example:
~/.kube/config

- cluster:
  name: cluster1
    server: https://...
    certificate-authority-data: ...
    proxy-url: socks5://localhost:1080
- cluster:
  name: cluster2
    server: https://...
    certificate-authority-data: ...
    proxy-url: socks5://another-proxy:1080
- cluster:
  name: cluster3
    server: https://...
    certificate-authority-data: ...

The expected behaviour should be:

$ ALL_PROXY=http://default-proxy.local:8080 ./start.sh

> cluster1 -> socks5://localhost:1080
> cluster2 -> socks5://another-proxy:1080
> cluster3 -> http://default-proxy.local:8080
$ unset ALL_PROXY; ./start.sh

> cluster1 -> socks5://localhost:1080
> cluster2 -> socks5://another-proxy:1080
> cluster3 -> * no proxy *

Describe alternatives you've considered

Current workaround is either having different environment variables (when connecting to one cluster per process), or programatically (a must when connecting to more than one cluster within same process).

Additional context

Tested with io.fabric8:kubernetes-client:6.13.1

@rohanKanojia
Copy link
Member

I'd suggest that the per cluster setting in ~/.kube/config should prevail.

@hudac : Hello, I'm a little unsure about this expectation. Do you mean in presence of ALL_PROXY environment variable and proxy-url set in current cluster in kubeconfig, proxy-url would be given precedence ? From what I see in the link you've shared : socks5-proxy-access-api/#client-configuration . It says environment variables will be given most precedence:

When using proxy-url, the proxy is used only for the relevant kubectl context, whereas the environment variable will affect all contexts.

Do we want to give more precedence to what's configured in the kubeconfig's proxy-url? Currently KubernetesClient has this order of loading Config (listed in increasing order of precedence)

  • kubeconfig (unsupported, this would be fixed soon)
  • all.proxy System Property
  • http.proxy System Property
  • ConfigBuilder's .withHttpsProxy("socks5://localhost:1080")

@rohanKanojia
Copy link
Member

I've raised #6289 to fix the issue. Current changes would read proxy-url from kubeconfig during autoconfiguration and set the http(s)Proxy in Config.
However if the user has also System Properties configured, They will be given precedence. This is the current behavior of KubernetesClient. Should proxy-url from kubeconfig override proxy configured via System Property / Environment Variables http.proxy / https.proxy ?

@shawkins
Copy link
Contributor

Seems like system / env should override anything loaded from the config.

@manusa
Copy link
Member

manusa commented Aug 26, 2024

I'd suggest that the per cluster setting in ~/.kube/config should prevail.

Seems like system / env should override anything loaded from the config.

In general, we want that system properties (and environment variables) override whatever is defined in a config file so that users are able to provide one-off overrides when executing their code.

(Think of hot fixes in production environments where you can't change configuration files but you can change the environment)

@hudac
Copy link
Author

hudac commented Aug 26, 2024

@rohanKanojia, thanks for tackling this issue so quickly!

From what I see in the link you've shared : socks5-proxy-access-api/#client-configuration . It says environment variables will be given most precedence.

Tbh, I understood it the other way around 😀 But I agree, the documentation is somewhat vague on this.

Seems like system / env should override anything loaded from the config.

In general, we want that system properties (and environment variables) override whatever is defined in a config file so that users are able to provide one-off overrides when executing their code.

I understand that this is the general behaviour, makes sense to me. I've double-checked how kubectl behaves:

  • 🔴 HTTPS_PROXY=wrong_host:port and ⚪ no proxy-url setting in kubeconfig: 🟥 fails as expected, as configured proxy host in HTTPS_PROXY is unknown (by purpose)
  • 🔴 HTTPS_PROXY=wrong_host:port and 🟢 proxy-url: valid_host:port: 🟩 succeeds, the valid host in proxy-url prevails
  • 🟢 HTTPS_PROXY=valid_host:port and 🔴 proxy-url: wrong_host:port: 🟥 fails, the unknown host in proxy-url prevails
  • 🟢 HTTPS_PROXY=valid_host:port and ⚪ no proxy-url: 🟩 succeeds

A proxy-url entry within kubeconfig seems to overrule any environment variable (HTTP[S]_PROXY), at least in kubectl.
Sounds reasonable to me, as the proxy-url is applied only to the relevant (specific) context, whereas environment variables affect all contexts.

I'm happy about the proxy-url in kubeconfig. The config priority might be a detail, and not set in stone, at least as per existing docs.

@rohanKanojia
Copy link
Member

@hudac : Thanks for verifying kubectl behavior with proxies. I tested kubectl behavior in presence of HTTPS_PROXY environment variable and proxy-url in kubeconfig, I think you're right about the precedence. In presence of proxy-url, kubectl is not checking for HTTPS_PROXY environment variables.

When checking godocs for the proxy-url field, it also states the precedence order similar to what you described:
https://github.com/kubernetes/client-go/blob/46965213e4561ad1b9c585d1c3551a0cc8d3fcd6/tools/clientcmd/api/v1/types.go#L88

// If this configuration is not provided or the empty string, the client
// attempts to construct a proxy configuration from http_proxy and
// https_proxy environment variables.
// If these environment variables are not set, the client does not attempt to proxy requests.

I can see in client-go code that the ProxyURL field is first read into client Config
https://github.com/kubernetes/client-go/blob/46965213e4561ad1b9c585d1c3551a0cc8d3fcd6/tools/clientcmd/client_config.go#L200

and then while building transport Config, we first initialize proxy to be read from environment variables. However, it is overridden by proxy func from Config if not nil:

https://github.com/kubernetes/client-go/blob/46965213e4561ad1b9c585d1c3551a0cc8d3fcd6/transport/cache.go#L125

@manusa
Copy link
Member

manusa commented Aug 27, 2024

// If this configuration is not provided or the empty string, the client
// attempts to construct a proxy configuration from http_proxy and
// https_proxy environment variables.
// If these environment variables are not set, the client does not attempt to proxy requests.

If this is the case, then HTTP_PROXY and HTTPS_PROXY have special consideration (we would need to see what happens with ALL_PROXY, NO_PROXY, and other PROXY related environment variables).

For the implementation we should update the test, implement correctly, and document the variables in the README.md configuration section.

@manusa manusa added this to the 7.0.0 milestone Aug 28, 2024 — with automated-tasks
@github-project-automation github-project-automation bot moved this from Review to Done in Eclipse JKube Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants