fix(certificatee): stop the legacy-duplicate delete/recreate thrash loop - #43
Merged
Conversation
v0.5.5 (#42) deleted a legacy duplicate's storage file every cycle, but HAProxy's runtime cert listing never drops it without a reload, so ListCertificateRefs kept rediscovering the same stale ref and the ordinary per-ref sync recreated it via EnsureStorageCertificate's 404-fallback-to-create. Confirmed directly against a real Data Plane API (shared_lb_fra_staging / haproxy-fra1): storage delete succeeds and the file is gone from disk, but the runtime listing keeps reporting the old content until an unrelated reload happens; a runtime-level delete is not an option either, since HAProxy refuses to remove a certificate still referenced by a bind (500 "in use"). Restructure processHAProxyEndpoint into three phases: fetch every ref's live metadata first, classify legacy duplicates from that complete picture, then sync everything that isn't a confirmed duplicate. Confirmed duplicates are deleted once (never recreated, since they no longer go through the normal per-ref sync at all) as long as their keeper stays stable; if the keeper isn't stable yet, the duplicate falls back to being synced normally so nothing goes unrenewed while cleanup waits for a calmer cycle.
Code Coverage ReportTotal Coverage: Coverage by function |
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
1 participant
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.
AI description, click for details
Summary
v0.5.5 (#42) shipped, deployed to staging + canary, and ran for ~18h - but the cleanup never actually converged. It logged
Removed legacy duplicate certificate Xevery ~10-minute cycle, for the same certs, forever, and the duplicates were still present in HAProxy's live cert store the whole time.Root cause, confirmed directly against a real Data Plane API (
haproxy-fra1, part of theshared_lb_fra_stagingfleet - samestaging_mainnetpool certificatee-staging manages):ListCertificateRefs/GetCertificateDetail/UpdateCertificateall read HAProxy's runtime in-memory cert table, populated only at the last actual reload.DeleteCertificateonly hits the storage (on-disk) delete endpoint withskip_reload=true- confirmed via directcurl: the delete succeeds (204) and the file is genuinely gone from disk (404 on a follow-upGET), but the runtime listing keeps reporting the exact same stale content afterward, completely unaffected.DELETE /v3/services/haproxy/runtime/ssl_certs/{name}(the runtime-socket delete) - and it 500s:certificate ... in use, can't be deleted!. Every file loaded from a directory crt-store (crt certs/) counts as "in use" regardless of whether it's the one actually selected for a live SNI match, so this isn't a viable path either.ListCertificateRefs()rediscovers the same stale runtime-listed ref, and the ordinary per-ref sync loop callsEnsureStorageCertificateon it - which 404s against the now-missing file and recreates it. Confirmed this exact 404-then-recreate step with a directPUTagainst the deleted file. Cleanup deletes it again at the end of that cycle. Repeat forever.Fix
Restructure
processHAProxyEndpointinto three phases:syncOneCertificate, extracted from the old per-ref loop body - identical logic). Confirmed duplicates are excluded from the normal sync path entirely once their keeper is confirmed stable this cycle, so there's nothing left to recreate them. If the keeper isn't stable yet, the duplicate falls back to being synced normally (old behavior) so nothing goes unrenewed while cleanup waits.Removal is still delete-only,
skip_reload=true, never forces a reload. The duplicate's storage file gets deleted once and stays deleted; HAProxy's live listing will keep reporting it until some other, unrelated reload happens on that node - at which point it drops out for good on its own. Repeating the delete attempt every cycle after that is now a harmless no-op (404), not a recreate loop.Test plan
TestCleanupLegacyDuplicateCertificatesDoesNotThrashAcrossCycles: runsprocessHAProxyEndpointtwice against a mock that keeps listing both duplicate names every cycle (matching real HAProxy behavior) and asserts the legacy name is never written to storage again after being deletedclassifyLegacyDuplicatesambiguous-group cases now unit-tested directly (no HTTP mocking needed)go test ./pkg/haproxy/... ./cmd/certificatee/... ./pkg/certmetrics/...,go vet,gofmtall cleanNo Linear ticket - follow-up fix from watching #42 run live on staging/canary.
🤖 Generated with Claude Code