Merge pull request #289 from voxpupuli/perf/batch-supersession-crl-resign

Batch the supersession sweep's CRL re-sign

Author: bootc

docs/configuration.md

@@ -838,16 +838,18 @@ Some things worth knowing before you rely on it:
   operator set the window to 0. On a CA that has never recorded a supersession
   each pass is a single absent-key read taking no cluster lock: the sweep rules
   the work out before acquiring one.
-- **Each due entry costs one CRL re-sign, under the shared CRL lock.** The sweep
-  revokes entries one at a time, and every revocation is a full read, re-sign
-  and write of the CRL. A large backlog coming due at once — after a fleet-wide
-  outage, say — therefore drains at a rate set by CRL re-sign cost rather than
-  by the sweep interval, and it holds the lock that every revocation on every
-  replica needs while it does. A pass stops before its budget is spent and logs
-  what it deferred to the next one, so a backlog that is not draining is visible
-  rather than silent: a deferred pass raises `puppetca_supersede_failures_total`
-  and logs `ran out of budget; deferring the rest`, and the entries it defers
-  stay on the list. Batching those re-signs into one is a separate change.
+- **A pass costs one CRL re-sign, whatever the backlog.** The sweep collects
+  every due entry and amends the CRL once — one read, one signature, one write —
+  however many certificates come due together, and holds the shared CRL lock
+  that every revocation on every replica needs for that single amendment rather
+  than for one per entry. Under
+  [`ca_key_provider: openbao`](openbao-transit.md) it is likewise one remote
+  Transit round trip rather than one per entry. That matters most in the case
+  the sweep used to handle worst: a large backlog coming due at once, after a
+  fleet-wide outage or a passphrase rotation. A pass that cannot amend the CRL
+  fails as a whole — it leaves every entry it attempted on the list, raises
+  `puppetca_supersede_failures_total`, and the next pass retries them
+  together.
 - **Revoking a subject retires its pending predecessor too.** `revoke --certname`
   and `DELETE /certificate_status` retire the subject's current certificate
   *and* anything of that subject's still inside its window, in the same call —

docs/metrics.md

@@ -311,13 +311,13 @@ sweep is not completing — check the failure counter.
 > that renewal but is left for the sweep to count, so one corrupt blob cannot
 > become a counter storm on a busy CA; a sweep pass that could not take the CRL
 > lock or write the list back; a predecessor a subject revocation could not
-> retire; and each pass that left an entry unrevoked, deferred one for want of
-> budget, or discarded one whose serial it could never revoke. A pass counts
+> retire; and each pass that left an entry unrevoked or discarded one whose
+> serial it could never revoke. A pass counts
 > once however many entries it failed on, so this is a count of bad passes
 > rather than of lost certificates.
 >
 > The cases differ in what you have to do about them, and the log line is what
-> tells them apart. `Could not revoke superseded certificate` retries on the
+> tells them apart. `Could not revoke superseded certificates` retries on the
 > next pass by itself. `failed to retire replaced certificate` and `Discarding`
 > are both gone for good — nothing will rediscover them, and the certificate
 > stays valid for its full remaining life. Retire those by serial with
@@ -330,9 +330,12 @@ sweep is not completing — check the failure counter.
 > A sweep that cannot read the list at all counts once per pass and omits
 > `puppetca_supersede_pending` rather than reporting zero, so the two signals
 > cannot both read clean while the list stops draining. A sweep that *can* read
-> it but runs out of budget counts too, and logs `ran out of budget; deferring
-> the rest to the next pass` — that is the shape a backlog draining slower than
-> it accrues takes, and the entries it defers stay on the list.
+> it but cannot amend the CRL counts once per pass too, not once per entry: the
+> re-sign covers the whole pass, so every certificate that pass attempted stays
+> on the list and is retried together. A pending gauge that does not fall while
+> this counter rises is a sweep failing outright rather than one falling behind
+> — the sweep no longer paces itself, so there is no longer a backlog it
+> declines to attempt.
 
 ### Client trust domains
 

internal/ca/supersede.go

@@ -516,21 +516,26 @@ var _ = Describe("Delayed supersession", func() {
 
 	Describe("ReconcileSuperseded when a revocation fails", func() {
 		// The retry half of the sweep's failure handling. The alert text tells
-		// operators to distinguish "Could not revoke superseded certificate"
+		// operators to distinguish "Could not revoke superseded certificates"
 		// (recorded, will retry) from "Discarding" (gone for good), and only the
 		// second was pinned. A change that dropped failed entries instead of
 		// carrying them forward would turn a retryable failure into a
 		// permanently valid credential and move no assertion.
+		//
+		// This is also the carry-forward case #176 had to preserve when it
+		// batched the re-sign. Batched, the failure is all-or-nothing rather
+		// than per entry — but where the entries end up is unchanged, which is
+		// the part that decides whether a certificate stays tracked.
 		It("carries the entry forward rather than dropping it, and counts the pass once", func() {
 			first := issue("node-r")
 			second := issue("node-s")
 			writePending([]pendingEntry{
 				{Serial: hexSerial(first.SerialNumber), Subject: "node-r", RevokeAt: time.Now().UTC().Add(-time.Minute)},
 				{Serial: hexSerial(second.SerialNumber), Subject: "node-s", RevokeAt: time.Now().UTC().Add(-time.Minute)},
 			})
-			// An unparseable CRL makes revokeSerialLocked fail for every entry
-			// without making any of them unrevocable — the distinction the two
-			// arms turn on.
+			// An unparseable CRL fails the batch's own read without making any
+			// of its entries unrevocable — the distinction the two arms turn
+			// on.
 			Expect(store.UpdateCRL(ctx, []byte("not a valid CRL"))).To(Succeed())
 			before := myCA.SupersedeFailures()
 
@@ -720,13 +725,18 @@ var _ = Describe("Delayed supersession", func() {
 		})
 	})
 
-	// The deferral arm. Every other sweep spec runs on context.Background(), so
-	// the reserve is never reached and this path shipped untested in round 1 —
-	// which is how it shipped erasing the entries it claimed to defer.
-	// ReconcileSuperseded's own WithTimeout keeps an earlier parent deadline, so
-	// a caller deadline below the reserve reaches the branch with no sleeping.
-	Describe("ReconcileSuperseded when the budget runs low", func() {
-		It("leaves the entries it defers on the list rather than erasing them", func() {
+	// This used to be the deferral arm: below the reserve the per-entry loop
+	// retired one entry and carried the rest to the next pass, because N
+	// entries cost N CRL re-signs and a backlog could not fit in one lock hold.
+	// #176 collapsed those into one re-sign, so the reserve is gone and a pass
+	// on the same deadline clears the whole backlog. The spec is kept, aimed at
+	// the property that replaced it — every other sweep spec runs on
+	// context.Background(), so without this one nothing exercises the sweep
+	// with a deadline near enough to matter at all. ReconcileSuperseded's own
+	// WithTimeout keeps an earlier parent deadline, so the short context here
+	// really is the one the pass runs under.
+	Describe("ReconcileSuperseded on a deadline that used to ration the pass", func() {
+		It("drains the whole backlog rather than deferring part of it", func() {
 			first := issue("node-y")
 			second := issue("node-z")
 			writePending([]pendingEntry{
@@ -735,31 +745,27 @@ var _ = Describe("Delayed supersession", func() {
 			})
 			before := myCA.SupersedeFailures()
 
-			// Below the reserve, so the loop breaks after the first entry.
+			// Below what the old reserve check (LockTimeout/2) allowed, so the
+			// pre-batch sweep revoked node-y here and deferred node-z.
 			shortCtx, cancel := context.WithTimeout(ctx, 5*time.Second)
 			defer cancel()
 			count, err := myCA.ReconcileSuperseded(shortCtx)
 			Expect(err).NotTo(HaveOccurred())
 
-			// Always at least one: a pass that revoked nothing while entries
-			// were due would make no progress, every time, forever.
-			Expect(count).To(Equal(1), "a pass must attempt at least one entry even with no budget left")
-			Expect(revoked(first.SerialNumber)).To(BeTrue(),
-				"oldest-first: the entry superseded longest ago is the one retired")
+			Expect(count).To(Equal(2),
+				"one re-sign covers both entries, so a deadline that used to stop the pass "+
+					"after the first no longer rations it")
+			// Anchored on each serial, so a batch that lost one fails here
+			// naming it rather than passing on a count that happens to match.
+			Expect(revoked(first.SerialNumber)).To(BeTrue(), "node-y must be on the CRL")
+			Expect(revoked(second.SerialNumber)).To(BeTrue(), "node-z must be on the CRL")
 
-			// The deferred entry is the whole point. Anchored on its serial, so
-			// this fails for the deferral and not for some other survivor.
-			entries := pending()
-			Expect(entries).To(HaveLen(1),
-				"an entry the budget deferred must still be on the list for the next pass; "+
-					"nothing else records that it is owed a revocation")
-			Expect(entries[0].Serial).To(Equal(hexSerial(second.SerialNumber)),
-				"and it must be the deferred entry, not some other survivor")
-			Expect(revoked(second.SerialNumber)).To(BeFalse())
-
-			Expect(myCA.SupersedeFailures()).To(Equal(before+1),
-				"a deferred backlog must be counted, or a sweep that cannot keep up looks "+
-					"identical to one that is keeping up")
+			Expect(pending()).To(BeEmpty(),
+				"an entry that is on the CRL is no longer owed a revocation, so the pass "+
+					"must prune it")
+			Expect(myCA.SupersedeFailures()).To(Equal(before),
+				"a pass that left nothing unrevoked is not a failure, and counting it as one "+
+					"would fire PuppetCASupersedeFailing on every healthy sweep")
 		})
 	})