fix: refresh Helm client credentials to prevent silent reconcile failures - #24
Conversation
… reconcile failures The Helm client built in clientForNamespace() copied the static BearerToken captured once at process startup (via ctrl.GetConfigOrDie()) instead of following BearerTokenFile like the controller-runtime client does. Once the projected service-account token rotates, every Helm uninstall in helmReleaseFinalizer starts failing with "Kubernetes cluster unreachable: the server has asked for the client to provide credentials" for the rest of the pod's lifetime, with no crash/restart to surface it. clientForNamespace now re-reads the token from BearerTokenFile on each call, mirroring client-go's own refresh behavior, falling back to the static token when BearerTokenFile is unset (e.g. out-of-cluster kubeconfigs). Observed in production on 3 separate EKS clusters within the same week; only fixed by a manual pod restart each time.
🛡️ SDD Check — action requiredI couldn't detect an SDD in this PR. Please check one option below (requires write access to the repo):
|
Covers: BearerTokenFile takes precedence over the static startup token; the file is re-read on every call (the core regression for the original bug — a rotated token on disk is picked up without a restart); fallback to the static token when BearerTokenFile is unset (out-of-cluster kubeconfig); and an unreadable token file surfaces as an error instead of silently falling back.
AdrianWR
left a comment
AdrianWR
left a comment
There was a problem hiding this comment.
Status: Low risk, well-tested, ready for review and merge pending approvals.
Observations:
Code quality: Clean, defensive, well-commented. The extended comment block (lines 378–391) clearly documents the root cause and rationale.
Test coverage: Excellent — four unit tests plus the existing envtest suite all pass. The suite confirms no regression, though it doesn't directly exercise the changed code path (by design, since it pre-sets HelmConfig to bypass file I/O).
Error handling: Properly propagated up the call stack using existing Recorder.Eventf(...) pattern.
Production impact: Fixes a known, reproducible production issue on three EKS clusters. No breaking changes to public APIs.
CI/Merge status: Code passes go build, go vet, and go test. PR is mergeable but blocked by status checks and review approvals.
|
3 participants
Summary
ConditionalTTLReconciler.clientForNamespacebuilt the Helmgenericclioptions.ConfigFlagsusingr.Config.BearerToken— a static string captured once at process startup byctrl.GetConfigOrDie()inmain.go. It never followedr.Config.BearerTokenFile, unlike the controller-runtime client (mgr.GetClient()), whose transport re-reads the projected service-account token file on every request.helm uninstallinsidehelmReleaseFinalizerfails with:ConditionalTTLfinalizers stop running and Helm releases are never cleaned up, causing unbounded resource/backlog growth.kubectl rollout restartof the deployment (which re-runsctrl.GetConfigOrDie()and gets a fresh token) resolved it — until it recurred after enough uptime.Fix
clientForNamespacenow re-reads the current token fromr.Config.BearerTokenFileon each call (mirroring client-go's own bearer-token-refresh transport), falling back to the original staticr.Config.BearerTokenwhenBearerTokenFileis unset (e.g. out-of-cluster kubeconfigs). The function signature now returns an error for the file-read case, handled at the one call site inhelmReleaseFinalizervia the existingRecorder.Eventf(..., "HelmSetupFailed", ...)pattern.Verification
go build ./...— passesgo vet ./...— passesgo test ./...— passes (existingcontrollersenvtest suite andcustom_celpackage). Note: the existing suite pre-setsHelmConfigon the reconciler to bypassclientForNamespaceentirely (a documented test-only shortcut), so it confirms no regression but does not directly exercise the changed code path — that's inherently hard to unit test since it depends onBearerTokenFilepointing at a real file, which differs between envtest and real in-cluster environments.Opening for team review, not intended for auto-merge.