fix(core): enforce chart uniqueness via lease claim#54
Open
drey wants to merge 3 commits into
Open
Conversation
The validating webhook could not reliably prevent two HelmClusterAddon objects from using the same repository/chart pair: admission requests are served concurrently and before the object is persisted, and the check read from the controller cache, so bursts of creates raced past it. Move the guarantee into the reconciler and delegate it to the API server's only atomic cross-object primitive, object-name uniqueness: each repo/chart pair maps to a single Lease name, claimed via ChartClaimService before any downstream resource is created. The first creator wins; a duplicate that loses surfaces a ChartClaimConflict condition and requeues, recovering once the owner releases the pair. Stale claims (deleted or repointed owner) are taken over. The claim is acquired before the finalizer so a loser accrues no finalizer, and released only after all downstream resources are gone. Drop uniqueness validation from the webhook's UPDATE path (and UPDATE from the manifest): with duplicates transiently present, validating updates would reject the very finalizer the reconciler needs to resolve the conflict. The webhook now only fast-rejects the obvious duplicate on CREATE. Signed-off-by: Ilya Drey <ilya.drey@flant.com>
drey
force-pushed
the
fix/webhhook-race
branch
3 times, most recently
from
July 22, 2026 08:16
b3f69d9 to
ef34f8a
Compare
Signed-off-by: Ilya Drey <ilya.drey@flant.com>
Signed-off-by: Ilya Drey <ilya.drey@flant.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes the "at most one
HelmClusterAddonper repository/chart pair" rule actually hold under load. The rule was enforced only by a validating webhook, which let duplicates slip through when many addons were created in a burst. Uniqueness is now guaranteed in the reconciler via an atomicLease-based claim, and the webhook is demoted to a best-effort fast-reject onCREATE.Why
The validating webhook cannot reliably enforce cross-object uniqueness:
CREATEs for the same chart both list the current state, neither sees the other, and both are admitted.The result: creating many
HelmClusterAddons quickly produced several objects using the same chart/repo — exactly what the webhook was meant to prevent.The only atomic cross-object primitive Kubernetes offers is object-name uniqueness in etcd. This PR maps each repo/chart pair to a single
Leasename and lets the API server serialize the winners.Key changes
New
ChartClaimService—images/operator-helm-controller/internal/services/chart_claim_service.goAcquirecreates aLeasenamed deterministically from the repo/chart pair (utils.GetChartClaimLeaseName, a pure hash → always a valid DNS name) withholderIdentity = addon.Name. First creator wins; losers observe a foreign holder.mgr.GetAPIReader()(uncached) so an acquisition decision is never made against stale cache data.Update.Releasedeletes theLeaseonly if this addon still owns it, guarded by aResourceVersionprecondition so a taken-over claim is never clobbered.Reconciler wiring —
internal/reconcile/helmclusteraddon/reconciler.goChartClaimConflictcondition and requeues (every 30s) instead of accruing a finalizer it would have to clean up. It recovers on its own once the owner releases the pair.reconcileDelete, only after every downstream resource is gone, so no other addon can start reconciling the same chart mid-teardown.Webhook demoted to CREATE-only —
internal/webhook/helmclusteraddon/webhook.go,templates/operator-helm-controller/validation-webhook.yamlValidateUpdateno longer checks uniqueness. With duplicates transiently present, validating updates would reject the very finalizer the reconciler needs to resolve the conflict.UPDATEdropped from the webhook manifest'soperations(now["CREATE", "DELETE"]), since both validators are now no-ops on update — removing needless webhook round-trips and a failure point underfailurePolicy: Fail.Supporting bits
api/v1alpha1/conditions.go: newReasonChartClaimConflictreason.internal/utils/name.go:GetChartClaimLeaseNamehelper.leasesverbs ind8-operator-helm(from leader election).Review focus / risks
Acquire(chart_claim_service.go) — the race-safety hinges on:CreatereturningAlreadyExists→ re-read → ownership check, and the takeover path using optimisticUpdate(conflict → report as claimed + requeue). Verify no branch can wrongly returnacquired = truefor a foreign-held claim.ValidateUpdateis a no-op), and that a loser correctly gets no finalizer.Leaselingers. It self-heals via takeover (next contender sees the holder gone). Confirm this is acceptable; it is the one known non-cleaned edge.isHolderStale) — a holder that changed its chart leaves its oldLeaseorphaned until another addon contends for that chart. Verify the "holder's current spec maps to a different lease name ⇒ stale" logic.GetChartClaimLeaseNameuses the shared 12-char hash (utils.GetHash); a collision would be a false conflict. Consistent with existing name helpers, but worth a sanity check for expected addon counts.ChartClaimConflictrather than synchronously atkubectl apply. Confirm this UX trade-off is intended.