Fix deferrable Kubernetes 401s with a default exec-based kubeconfig - #72300
Conversation
7c23e9d to
f4b894b
Compare
Exec credential plugins such as "aws eks get-token" issue tokens that expire after roughly fifteen minutes, so a triggerer holding a cached kubeconfig starts getting 401 Unauthorized part-way through a long pod or job. Caching was already skipped for exec-based auth reached through config_dict, kube_config and kube_config_path, but a kubeconfig picked up from the default location was still cached unconditionally and kept the stale token for the lifetime of the hook. When KUBECONFIG genuinely names several files the merge rules that decide the active user belong to kubernetes_asyncio, so the config is left uncached rather than duplicating them here.
The helpers return True both when a kubeconfig genuinely uses an exec credential plugin and when the active user could not be read at all, so a reader had no way to tell the two apart. The comment was narrower still: it explained only the several-files case, while the same branch also covers the default location resolving to no readable file.
_resolve_default_kubeconfig_path returns a path and loads nothing. Generated-by: Claude Opus 5
f4b894b to
02e2880
Compare
potiuk
left a comment
There was a problem hiding this comment.
The default-kubeconfig fall-through is the one branch #65212 missed, and this closes it the same way the other three already do.
I verified the new path resolution against kubernetes_asyncio's own semantics rather than taking it on trust: load_kube_config(config_file=None) falls back to KUBE_CONFIG_DEFAULT_LOCATION, which KubeConfigMerger then splits on the path separator, runs through expanduser, and filters by existence — the same three steps, in the same order, as _resolve_default_kubeconfig_path(). ENV_KUBECONFIG_PATH_SEPARATOR is os.pathsep on both platforms. So the exec detection reads exactly the file the loader used.
Also checked: the extracted _kubeconfig_file_uses_exec_auth() is behaviour-identical to the block it replaces (same read, same parse, same warning, same conservative True fallback), nothing else in the hunk was absorbed, no existing test was modified, and the two expected_cached=False cases cannot pass pre-fix, since the default branch set _config_loaded = True unconditionally. Treating an ambiguous default location as exec, rather than reimplementing the merge rules, is the right direction to be conservative in.
I rebased the branch on main — it was 457 commits behind — and pushed one wording fixup: _resolve_default_kubeconfig_path() returns a path and loads nothing, so its docstring now says so.
Smaller observations
- Three of the five new cases (
default_kubeconfig_no_exec,..._ignores_paths_that_do_not_exist,..._expands_home_directory) assert_config_loaded is True, which is also the pre-fix result, so they pass on unmodifiedmain. Worth keeping regardless — each flips if theexpanduseror existence-filter logic regresses — just noting the revert-detectable signal lives in the twoexpected_cached=Falsecases. - The tests patch
async_config.KUBE_CONFIG_DEFAULT_LOCATION, which is the name_resolve_default_kubeconfig_path()itself reads, so this is right for what is being tested. Only worth knowing that the realload_kube_config()reads its own module-level copy, so the patch would stop steering anything if someone later un-mocks the loader in these tests. - Optional follow-up, not for this PR: a multi-file
$KUBECONFIGwith purely static credentials is now never cached, so the kubeconfig is re-merged on every trigger poll.KubeConfigMerger(paths).configwould give the merged view without reimplementing the merge rules, if that cost ever shows up.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Airflow maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.More on how Apache Airflow handles maintainer review:
contributing-docs/05_pull_requests.rst.
Summary
AsyncKubernetesHookcaches the kubeconfig it loads, and both the pod and job triggers keep that hook as acached_property. This causes a problem with exec-based credentials such asaws eks get-token, which mint tokens that expire after about fifteen minutes. The cached token can therefore expire during a long-running deferrable task, causing subsequent Kubernetes API requests to fail with401 Unauthorized.#65212 fixed this for exec auth loaded through
config_dict,kube_config, andkube_config_path, but missed the fall-through branch wherekubernetes_asyncioloads the kubeconfig from~/.kube/configor$KUBECONFIG.This branch is used when:
config_file,KUBECONFIG, andThis is also the typical setup when an EKS kubeconfig is mounted at the default location.
This PR applies the same caching decision to the default kubeconfig path: exec-based credentials are not cached, while static credentials remain cached. This allows the exec plugin to be invoked again and obtain a fresh token when the trigger polls later.
When
$KUBECONFIGcontains multiple existing files, the config is left uncached rather than reimplementingkubernetes_asyncio's merge rules. This adds one kubeconfig read per poll in that case.In-cluster configuration continues to be cached because
load_incluster_config()installs arefresh_api_key_hookthat re-reads the rotated service account token.Closes #61737
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 5) following the guidelines