fix: incluster config handling #2540
Conversation
Walkthrough
Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ocp_resources/resource.py (1)
292-292: Prefer imported name over fully qualified reference.Use the imported
DynamicClientname instead ofkubernetes.dynamic.DynamicClientfor consistency with the import at line 22.- return kubernetes.dynamic.DynamicClient(client=_client) + return DynamicClient(client=_client)
dtrifiro
force-pushed
the
fix-incluster-config-handling
branch
from
36adcfe to
8cc94e9
Compare
dtrifiro
force-pushed
the
fix-incluster-config-handling
branch
from
8cc94e9 to
f99c070
Compare
ocp_resources/resource.py
Outdated
| Pass either config_file or config_dict. | ||
| If none of them are passed, client will be created from default OS kubeconfig | ||
| If none of them are passed, client will be created from the incluster config or from default OS kubeconfig |
There was a problem hiding this comment.
incluster should be called in the user didn't pass also username,password,host or token. Please update the docstring.
There was a problem hiding this comment.
I refactored the logic to work in all broken cases, I also updated the docstring to reflect what's going on in the code
|
/retest all |
dtrifiro
force-pushed
the
fix-incluster-config-handling
branch
from
f99c070 to
f2125fd
Compare
dtrifiro
force-pushed
the
fix-incluster-config-handling
branch
from
f2125fd to
2670708
Compare
The current implementation is broken if run in a cluster. It should be possible to load an in-cluster configuration via `kubernetes.config.load_incluster_config()`, which populates host/ports/certs from environment variables and secrets from `/var/run/secrets/kubernetes.io`. There's a few issues in the current implementation: - Running `get_client()` falls through `new_client_from_config` which raises `ConfigException: Invalid kube-config file. No configuration found.`, - The final return is broken, since the `DynamicClient` is instantiated with `client=kubernetes.config.incluster_config.load_incluster_config`, which is always None - Trying to bypass the above problems by creating a custom `client_config` and passing it to `get_client` also fails: it falls through to `new_client_from_config` and raises `ConfigException`, because `load_incluster_config` sets `api_key="bearer <token>"` which is ignored.
dtrifiro
force-pushed
the
fix-incluster-config-handling
branch
from
2670708 to
ff7f058
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ocp_resources/resource.py (2)
241-260: Fix fall-through, honor client_configuration, and use try_refresh_token for in-cluster
- _client can be undefined when not in-cluster and no kubeconfig is present.
- Provided client_configuration isn’t used to build the client.
- try_refresh_token parameter is unused; pass it to load_incluster_config.
Apply this patch:
@@ - client_configuration = client_configuration or kubernetes.client.Configuration() + client_configuration = client_configuration or kubernetes.client.Configuration() + _client: kubernetes.client.ApiClient | None = None @@ - elif config_file or ("KUBECONFIG" in os.environ) or os.path.exists(os.path.expanduser("~/.kube/config")): + elif config_file or ("KUBECONFIG" in os.environ) or os.path.exists(os.path.expanduser("~/.kube/config")): @@ - if not config_file: - config_file = os.environ.get("KUBECONFIG", "~/.kube/config") + if not config_file: + config_file = os.environ.get("KUBECONFIG", "~/.kube/config") + # Ensure user home shortcut is expanded + config_file = os.path.expanduser(config_file) LOGGER.info("Trying to get client via new_client_from_config config_file=%s", config_file) @@ - elif all( + elif (client_configuration.api_key or client_configuration.host): + LOGGER.info("Using provided client_configuration to create ApiClient") + _client = kubernetes.client.ApiClient(client_configuration) + elif all( var in os.environ for var in ( kubernetes.config.incluster_config.SERVICE_HOST_ENV_NAME, kubernetes.config.incluster_config.SERVICE_PORT_ENV_NAME, ) ): LOGGER.info("Trying to get client via incluster_config") - # Ref: https://github.com/kubernetes-client/python/blob/v26.1.0/kubernetes/base/config/incluster_config.py - kubernetes.config.load_incluster_config(client_configuration) + # Ref: https://github.com/kubernetes-client/python/blob/v26.1.0/kubernetes/base/config/incluster_config.py + kubernetes.config.load_incluster_config(config=client_configuration, try_refresh_token=try_refresh_token) _client = kubernetes.client.ApiClient(client_configuration) @@ - return kubernetes.dynamic.DynamicClient(client=_client) + if _client is None: + raise kubernetes.config.config_exception.ConfigException( + "No Kubernetes configuration found. Provide credentials, config_dict/file, " + "a prepared client_configuration, or run in-cluster." + ) + return kubernetes.dynamic.DynamicClient(client=_client)This preserves intended precedence, prevents UnboundLocalError, and uses try_refresh_token. Based on learnings.
Also applies to: 270-285, 286-301
247-250: Don’t log proxy URL values (may leak credentials)Logging the full proxy can expose user:pass in logs. Log the presence, not the value.
Apply this:
- LOGGER.info(f"Setting proxy from environment variable: {proxy}") + LOGGER.info("Setting proxy from environment variable.")
🧹 Nitpick comments (1)
ocp_resources/resource.py (1)
206-236: Docstring: clarify precedence and document client_configuration usage
- Precedence should be explicit and match code.
- Document how a provided client_configuration is used.
Apply this docstring tweak:
@@ - Valid combinations for arguments include: - - config file (kubeconfig path) - - config_dict - - username, password and host (uses basic auth) - - host and token (bearer token) - - client_configuration + Valid combinations for arguments include (first match wins): + 1) username, password and host (basic auth) + 2) host and token (bearer token) + 3) config_dict + 4) config file (kubeconfig path) + 5) client_configuration (pre-configured kubernetes.client.Configuration with host/api_key) @@ - If none of the above arguments are provided: - - if the `KUBECONFIG` env var is set: it will be used to configure the client - - if not set and `~/.kube/config` exists, it will be used to configure the client - - If no configuration files are available, incluster config loading will be attempted + If none of the above are provided: + - If the `KUBECONFIG` env var is set, it will be used. + - Else, if `~/.kube/config` exists, it will be used. + - Else, if running in a cluster (`KUBERNETES_SERVICE_HOST` and `KUBERNETES_SERVICE_PORT`), in‑cluster config is loaded. + - Otherwise, an explicit configuration is required. @@ - persist_config (bool): whether to persist config file. + persist_config (bool): whether to persist config file. @@ - token (str): Use token to login + token (str): Use token to login + client_configuration (kubernetes.client.Configuration | None): Pre-configured client configuration + (used directly if it contains required auth/host).Based on learnings.
| # Ref: https://github.com/kubernetes-client/python/blob/v26.1.0/kubernetes/base/config/__init__.py | ||
| LOGGER.info("Trying to get client via new_client_from_config") | ||
|
|
||
| elif config_file or ("KUBECONFIG" in os.environ) or os.path.exists(os.path.expanduser("~/.kube/config")): |
There was a problem hiding this comment.
("KUBECONFIG" in os.environ) this is not a good check, this will be true for TEST_KUBECONFIG in os.environ.
Use os.environ.get("KUBECONFIG") and save it since you need to use it later.
There was a problem hiding this comment.
TEST_KUBECONFIG as an environment variable? In that case "KUBECONFIG" in os.environ will not be true, os.environ is a dict and will not have the "KUBECONFIG" key.
The idea here is to go through this path only if:
config_fileis passed by the user explicitly- user explicitly set a
KUBECONFIGenvironment variable (so the key will be inos.environ) - a kubeconfig is available at the default location
| # Ref: https://github.com/kubernetes-client/python/blob/v26.1.0/kubernetes/base/config/__init__.py | ||
| if not config_file: | ||
| config_file = os.environ.get("KUBECONFIG", "~/.kube/config") | ||
| LOGGER.info("Trying to get client via new_client_from_config config_file=%s", config_file) |
There was a problem hiding this comment.
Please use f-string and not %s
There was a problem hiding this comment.
This is the correct way to pass argument to logging statements: formatting of message arguments is deferred until it cannot be avoided.
https://docs.python.org/3/howto/logging.html#optimization
https://docs.python.org/3/howto/logging.html#logging-variable-data
| client_configuration=client_configuration, | ||
| persist_config=persist_config, | ||
| ) | ||
| elif all( |
There was a problem hiding this comment.
Any reason why not query os.environ with the needed keys?
There was a problem hiding this comment.
This is supposed to be a minimal check to assess whether this is running in a k8s environment, where KUBERNETES_SERVICE_PORT and KUBERNETES_SERVICE_HOST are set.
If these are set, there's a fairly good chance an incluster configuration is available, so we call load_incluster_config which will take care of reading/validating the configuration from env vars and files, see https://github.com/kubernetes-client/python/blob/a486091a91fdd0d0d389bc9538533118ee6fa3c8/kubernetes/base/config/incluster_config.py#L59-L85
6 participants
The current implementation is broken if run in a cluster.
It should be possible to load an in-cluster configuration via
kubernetes.config.load_incluster_config(), which populateshost/ports/certs from environment variables and secrets from
/var/run/secrets/kubernetes.io.There's a few issues in the current implementation:
get_client()falls throughnew_client_from_configwhich raisesConfigException: Invalid kube-config file. No configuration found.,DynamicClientis instantiated withclient=kubernetes.config.incluster_config.load_incluster_config, which is always Noneclient_configand passing it toget_clientalso fails: it falls through tonew_client_from_configand raisesConfigException, becauseload_incluster_configsetsclient_config.api_key="bearer <token>"which is ignored.This PR fixes the issues by:
KUBECONFIGenv var or if~/.kube.configexists.KUBERNETES_SERVICE_PORTandKUBERNETES_SERVICE_HOSTare set)client_configurationas is, fixing the last issue.Summary by CodeRabbit
New Features
Bug Fixes
Chores