[core][kuberay] Support configurable Kubernetes API authentication in the autoscaler - #65827
[core][kuberay] Support configurable Kubernetes API authentication in the autoscaler#65827ankushbbbr wants to merge 2 commits into
Conversation
… the autoscaler The KubeRay autoscaler's Kubernetes API client hardcodes in-cluster ServiceAccount authentication: it reads a bearer token from the projected token file and verifies the API server with the projected CA certificate. Some Kubernetes distributions front the API server with an authenticating proxy that identifies callers by a client TLS certificate instead, and `requests` has no environment variable for client certificates, so there is no configuration-only way to run the autoscaler there today. Add three optional environment variables, following the existing KUBERAY_REQUEST_TIMEOUT_S convention in the same module: RAY_KUBERAY_CLIENT_CERT_PATH RAY_KUBERAY_CLIENT_KEY_PATH RAY_KUBERAY_CA_CERT_PATH When the cert/key pair is set it is passed to `requests` as `cert=`, and when the CA path is set it is used for `verify`. When none are set the behavior is unchanged: the same bearer token header, the same `verify` path, and `cert=None`, which is the `requests` default. The ServiceAccount token becomes optional only when a client certificate is configured, since such environments often do not project one. The API server host is already overridable via KUBERNETES_SERVICE_HOST / KUBERNETES_SERVICE_PORT_HTTPS, so url_from_resource is untouched. IKubernetesHttpApiClient and all existing KubernetesHttpApiClient construction sites are unchanged. Signed-off-by: Ankush Babbar <ankushbbbr@gmail.com> Committed-By-Agent: claude
There was a problem hiding this comment.
Code Review
This pull request introduces support for optional client TLS credentials (client certificate, client key, and CA certificate) in the KubeRay node provider, allowing authentication with the Kubernetes API server via a client certificate instead of just the in-cluster ServiceAccount token. The changes update load_k8s_secrets and KubernetesHttpApiClient to handle these new credentials and include comprehensive unit tests. The review feedback suggests stripping trailing whitespace from the ServiceAccount token to avoid malformed headers and simplifying the _get_refreshed_credentials method by removing a redundant else block.
| if cert is None or os.path.exists(IN_CLUSTER_TOKEN_PATH): | ||
| with open(IN_CLUSTER_TOKEN_PATH) as secret: | ||
| token = secret.read() | ||
| headers["Authorization"] = "Bearer " + token |
There was a problem hiding this comment.
Kubernetes ServiceAccount tokens read from disk can sometimes contain trailing newlines or whitespace, which can lead to malformed HTTP headers or authentication failures with strict API servers or proxies. It is safer to strip any leading/trailing whitespace from the token before adding it to the headers.
| if cert is None or os.path.exists(IN_CLUSTER_TOKEN_PATH): | |
| with open(IN_CLUSTER_TOKEN_PATH) as secret: | |
| token = secret.read() | |
| headers["Authorization"] = "Bearer " + token | |
| if cert is None or os.path.exists(IN_CLUSTER_TOKEN_PATH): | |
| with open(IN_CLUSTER_TOKEN_PATH) as secret: | |
| token = secret.read().strip() | |
| headers["Authorization"] = "Bearer " + token |
There was a problem hiding this comment.
I'd like to leave it out of this PR, because it is the one change here that would alter the default code path.
The core property this PR is trying to preserve is that with none of the new environment variables set, the outgoing request is identical to today's. secret.read() without .strip() is exactly what load_k8s_secrets() does on master, and test_client_get_defaults_unchanged / test_client_patch_defaults_unchanged assert the complete kwargs dict handed to requests to lock that down.
fwiw, I don't think the failure mode is reachable in practice today: Kubernetes projects the ServiceAccount token without a trailing newline.
…hing Address review feedback: collapse the redundant else branch in _get_refreshed_credentials so the cached credentials are returned from a single exit point. Behavior is unchanged. Add a test that pins the caching contract the simplification touches: credentials are loaded once and reused across requests, and reloaded once the refresh period has elapsed. Signed-off-by: Ankush Babbar <ankushbbbr@gmail.com> Committed-By-Agent: claude
Description
The KubeRay autoscaler's Kubernetes API client hardcodes in-cluster ServiceAccount authentication:
load_k8s_secrets()reads a bearer token from/var/run/secrets/kubernetes.io/serviceaccount/tokenand verifies the API server with/var/run/secrets/kubernetes.io/serviceaccount/ca.crt, andKubernetesHttpApiClient.get()/.patch()pass nocert=torequests.Many organizations run customized Kubernetes distributions where API authentication differs from the out-of-the-box in-cluster mechanism — for example, requests must go through an authenticating proxy that expects a client TLS certificate rather than a ServiceAccount bearer token. Today the autoscaler cannot run in those environments, and it cannot be worked around by configuration:
requestshas no environment variable for client certificates, andREQUESTS_CA_BUNDLEonly affectsverify, which is inert here because Ray passesverify=explicitly.This PR makes the authentication mechanism configurable via three optional environment variables, named after the existing
KUBERAY_REQUEST_TIMEOUT_S/KUBERAY_CRD_VERconvention in the same module:RAY_KUBERAY_CLIENT_CERT_PATHrequestsas part ofcert=RAY_KUBERAY_CLIENT_KEY_PATHrequestsas part ofcert=RAY_KUBERAY_CA_CERT_PATHverify=, instead of the ServiceAccount CAWhen none of these are set, behavior is unchanged. The client sends the same
Authorization: Bearer <token>header, the sameverifypath, andcert=None— which isrequests' own default forcert, so the outgoing request is identical to today's.test_client_get_defaults_unchangedandtest_client_patch_defaults_unchangedassert the full kwargs dict passed torequests.get/requests.patchto lock this in.Other properties, all deliberate:
test_load_k8s_secrets_token_required_without_client_cert).ValueErrornaming both variables, rather than silently ignoring the configuration.url_from_resourceis untouched. The API server host is already overridable throughKUBERNETES_SERVICE_HOST/KUBERNETES_SERVICE_PORT_HTTPS, and it already accepts a host carrying thehttps://scheme verbatim, so only authentication needed to change. That keeps this diff small.IKubernetesHttpApiClientis unchanged, and all existing construction sites keep working unchanged:node_provider.py,autoscaling_config.py, andautoscaler/v2/instance_manager/cloud_providers/kuberay/cloud_provider.py._privatemodule:load_k8s_secrets()now returns a third element (cert), and_get_refreshed_headers_and_verifyis renamed to_get_refreshed_credentialsto match. Neither is referenced outsidenode_provider.py.An alternative design would be to make the client fully pluggable, e.g. a config field or entry point naming a custom
IKubernetesHttpApiClientimplementation. That is more general but a much larger surface to support, and it isn't needed for this case. Happy to change direction if maintainers prefer that; discussed in the linked issue.I'd like this to land in Ray 2.59.
Related issues
Closes #65826
Additional information
Example usage
Tests run
Ten new test functions (13 cases with parametrization) in
python/ray/tests/kuberay/test_kuberay_node_provider.py, covering: unchanged defaults asserted at therequestskwargs level for bothgetandpatch;cert=pass-through; CA override with and without a client cert; optional token with a client cert; token still required without one; cert/key required together; and the credential caching/refresh contract.The last two cover the other
KubernetesHttpApiClient/IKubernetesHttpApiClientconsumers, to confirm nothing downstream broke.Not tested locally: the client-certificate path against a real API server behind an authenticating proxy. The unit tests assert what is handed to
requests; therequestsside ofcert=is library behavior.Lint
Clean on both changed files:
black --check,ci/lint/check-docstyle.sh,ci/lint/check_import_order.py,pyflakes,pycodestyle --max-line-length=88, andisort --check-only.I could not run the full
pre-commitsuite in my environment: the pinnedruff==0.8.4andpydoclint==0.8.3were unavailable there, and theruffbinary I did install would not execute at all (killed on start). Soruffandpydoclintwere not actually run locally —pyflakes/pycodestyle/isortabove are stand-ins for theruffandruff --select Ihooks, and nothing substitutes forpydoclint. Please treat CI as authoritative for those two hooks; happy to fix whatever they flag.DCO
The commit is signed off (
git commit -s).Duplicate-work check
Per
AGENTS.md, I checked for existing work before opening this:No open PR or issue covers configurable Kubernetes API authentication for the autoscaler. The nearest hits are unrelated (in-process TLS credential reloading for RayCluster components, RBAC questions).
AI assistance
AI assistance was used to write this change and its tests. I have reviewed every changed line, run the tests above locally, and can defend the design and the backwards-compatibility argument.