Reconcile stale configured OAuth clients

[jhrozek]

Summary

Removing a delegate or SPIFFE client-auth association from an operator's config never cleaned up the corresponding durable storage record (Redis or in-memory). The stale row stayed forever, indistinguishable from a live, currently-configured client — a gap deliberately deferred out of #6474 pending this reconciliation machinery.

• Track configured-client ownership (a configured marker, separate from the client value) through both memory and Redis storage, surviving eviction.
• Reconcile the full desired set of operator-declared clients against durable storage at embedded auth-server startup, pruning rows that carry the configured marker but are no longer in the desired set. Legacy (unmarked) and DCR-issued rows are never touched.
• For Redis, coordinate multiple replicas with a short-lived lease per replica: publish the desired client IDs/fingerprints, renew in the background on a ticker, and reject a fingerprint conflict for the same client ID instead of silently letting one replica overwrite another's configuration.
• SPIFFE static clients are deliberately excluded from this reconciliation — they're already durably claimed as inert placeholders by the separate decorateStorageForSPIFFE/preflightDurableCollisions path, and feeding the real client in here would fight that placeholder for the same row.
• Fix a related bug found while implementing this: releasing a lease on shutdown used to sweep with an empty desired set, which treated the last live replica going down as "nothing is desired" and pruned every durably reconciled client on a routine restart. Release now only relinquishes the lease; cleanup happens on the next real reconciliation call.

Fixes #6477

Type of change

• Bug fix

Test plan

• Unit tests (task test)
• Linting (task lint-fix)

API Compatibility

No operator API surface touched — this is internal auth-server storage behavior only.

Does this introduce a user-facing change?

Yes: removing a delegate or SPIFFE client-auth entry from config now actually removes its durable storage row (on the next server startup) instead of leaving it orphaned indefinitely.

Special notes for reviewers

Stale SPIFFE static-client placeholder cleanup is explicitly left for a follow-up once SPIFFE client-auth enforcement lands (see the code comment on configuredClients in pkg/authserver/server_impl.go) — this PR only reconciles delegate clients, since SPIFFE clients aren't reachable end-to-end yet (#6199).

[codecov]

Codecov Report

❌ Patch coverage is 85.84071% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.52%. Comparing base (720209d) to head (4030642).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/authserver/storage/redis.go	83.92%	18 Missing ⚠️
pkg/authserver/server_impl.go	82.69%	9 Missing ⚠️
pkg/authserver/storage/memory.go	91.37%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6527      +/-   ##
==========================================
+ Coverage   78.50%   78.52%   +0.01%     
==========================================
  Files         776      776              
  Lines       76494    76726     +232     
==========================================
+ Hits        60052    60249     +197     
- Misses      16437    16472      +35     
  Partials        5        5              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[JAORMX]

Blocking — lease publication is not atomic with pruning

ReconcileConfiguredClients first writes configured client rows (pkg/authserver/storage/redis.go:695), then separately publishes the lease (:716) and adds its index membership (:719). A concurrent replica whose desired set does not contain that ID can run sweepConfiguredClients in this interval. Its liveConfiguredClaims sees no claim, and deleteConfiguredClientIfUnchanged only WATCHes the client key and the lease keys from that stale snapshot (:769-862); it does not watch/re-read the lease index. It can therefore delete the newly written row before the lease becomes visible. The owner does not restore it until the 10-second renewal (pkg/authserver/server_impl.go:312-323), leaving an active replica unable to resolve its configured OAuth client.

The distributed coordination is needed for rolling replicas; neither the Go stdlib nor the existing dependencies replaces that requirement. Please make claiming visible before client writes and make claim publication plus the prune decision atomic/serialized (for example, use the installed go-redis/v9 transaction/Lua support with an in-transaction claim check), and add an interleaving test for the write/publish window.

Non-blocking — lease fingerprint duplicates different identity semantics

configuredClientFingerprint hashes the raw slice order and duplicate count (pkg/authserver/storage/redis.go:654-669), while the existing configured-client identity intentionally treats scopes, audiences, grant types, and response types as sets (pkg/authserver/storage/types.go:600-654). Thus two otherwise identical rolling replicas which merely reorder (or repeat) a scope can successfully reconcile the durable row but reject each other’s live leases at redis.go:704-707. Canonicalize the lease input using the existing set semantics before hashing, so the lease protocol has one definition of “same logical client.”

[JAORMX]

Blocking — periodic reconciliation can reactivate rotated OAuth secrets

The heartbeat calls the full ReconcileConfiguredClients every 10 seconds (pkg/authserver/server_impl.go:312-323), which rewrites each configured client through ReconcileConfiguredClient (pkg/authserver/storage/redis.go:695). The live-lease fingerprint deliberately omits the secret (:654-669), so replicas with the same client ID and authorization metadata but an old versus rotated secret are considered compatible. During a rolling secret rotation, each replica can therefore alternately restore its own secret hash; a revoked credential becomes valid again whenever the old replica renews, and the new credential intermittently fails.

Please separate lease renewal from client material reconciliation, so the periodic worker extends only its current claim and cannot write client rows. Then define/implement a controlled handoff for secret rotation and cover two replicas with different secrets across multiple heartbeat intervals.

Blocking — a changed configured client cannot roll out under the same ID

ReconcileConfiguredClients writes each desired client before it publishes or compares its lease (pkg/authserver/storage/redis.go:695-706). ReconcileConfiguredClient rejects an existing client with a different configured fingerprint (:629-632), so changing a delegate client's scopes, audiences, grants, or response types causes the new replica to fail startup while the old durable authorization remains. Because the ID remains in the desired set, the subsequent sweep would retain it even after the old replica exits. This prevents a configuration tightening from converging and can leave a broader client authorization live.

Please make rollout of a changed desired client an explicit, safe ownership/handoff path rather than an indefinite ErrAlreadyExists, with a test for tightening a scope/audience across a rolling deployment.

Also please correct docs/arch/11-auth-server-storage.md:256: SPIFFE placeholder clients are explicitly excluded from this reconciliation, so they are not reconciled as part of this desired set.

[JAORMX]

Blocking — removed configured clients still survive normal rolling updates

The startup sweep correctly leaves rows claimed by old replicas alone, but the replacement then renews only its empty/new lease. When the old pod exits, ReleaseConfiguredClientLease deliberately performs no sweep, so the removed row remains until another auth-server startup. A normal Kubernetes rolling update therefore does not converge to the desired set and does not fix #6477 without an unrelated later restart.

Please trigger a sweep-only reconciliation after lease topology changes (without rewriting client material), or otherwise guarantee deletion once the final old claim disappears. Add coverage for: old replica claims a client → new config omits it → old replica releases → stale client is removed while the new replica remains running.

Blocking — existing Redis-backed configured clients cannot upgrade

The reconciliation Lua script rejects any existing row without configured (pkg/authserver/storage/redis.go:819-827). Pre-PR delegate rows lack that new marker, so an ordinary upgrade fails newServer at pkg/authserver/server_impl.go:356-359 with ErrAlreadyExists; rolling upgrades stall and a simultaneous restart produces avoidable downtime. The reconciliation intended to clean up these rows must safely adopt matching legacy configured-delegate rows, while preserving DCR and reserved collision protections.

Blocking — same-ID material changes still deadlock the normal rollout

The new drain-first behaviour explicitly rejects a replacement when an old-material lease exists (pkg/authserver/storage/redis.go:802-839), but operator Deployments use the default RollingUpdate strategy. The replacement cannot become ready until the old pod has released its lease; Kubernetes keeps the ready old pod while the replacement cannot become ready. Secret rotation or authorization tightening under the same client ID cannot converge, leaving the old credential and broader authorization live. The documented manual drain-first procedure does not meet the previously requested safe ownership/handoff under the normal rollout mechanism.

The lease publication/pruning race, periodic material rewrites, fingerprint set canonicalization, and SPIFFE documentation are resolved. The SPIFFE placeholder cleanup remains an explicitly documented and safe deferred item.

[JAORMX]

Fresh review of the fb6b5d6e… → bdd8ddb0… delta: the three blocking findings in my current-head changes-requested review remain unchanged. One non-blocking regression: the 10,000-client guard now runs after allocations and serialization of the full input in pkg/authserver/storage/redis.go:709-751, defeating its stated role as a bound for untrusted-size work. Restore the check before allocating/iterating the input.

[jhrozek]

Replaced the lease-based approach with a much simpler one after stepping back on all three findings above.

The lease existed to stop two replicas from alternately resurrecting old-vs-new client material during a rollout, but it never actually delivered that guarantee: every replica shares one Redis row regardless of leasing, so only one version is ever live at a time with or without a lease. What the lease did add was the deadlock you found — a new pod's own startup runs this same reconcile before any health listener opens, so rejecting it means the new pod can never become Ready, and the old pod is never killed because the new one isn't Ready. That's a real trade of a brief, self-healing inconsistency for a hard deadlock, for no actual benefit.

So it's gone. ReconcileConfiguredClients now just writes the desired set and prunes anything configured-but-no-longer-desired, last-write-wins, no per-replica coordination. It also runs on a 10s heartbeat now, not just at startup, so a removal converges during a normal rolling update instead of needing a second, unrelated restart (finding #1). An unmarked legacy row is adopted if its shape matches what's being reconciled, otherwise it's rejected as a genuine collision — so an existing Redis-backed deployment upgrading past this PR can adopt its pre-existing rows instead of failing startup (finding #2). And since reconcile never rejects a same-ID change anymore, there's nothing left to deadlock a rollout on (finding #3).

This is closer to what #6477 itself originally proposed (persist a marker, periodically diff against config, prune) — the lease/lock machinery was scope creep past that, and every fix for it kept opening a new correctness gap. Also fixed while in there: sweep now skips and warns on a single corrupt/unmarshalable row instead of failing the whole reconcile, and the docs section describing this design is rewritten to match.

[JAORMX]

Blocking — a replacement replica can delete a still-live old client

ReconcileConfiguredClients sweeps every configured row that is absent from the caller’s desired set. During a normal rolling update that removes delegate X, the new replica starts first and its startup reconciliation deletes X immediately (pkg/authserver/storage/redis.go:741-813); the old replica can still be ready and serving requests that require X. There is no live-claim/ownership check, nor a drain step. This turns the requested stale-row cleanup into an avoidable availability regression during the rollout.

Please retain a removed client until no replica can still depend on it, or otherwise coordinate the handoff. Add a rolling-update test covering old config with X → new config without X, proving X remains usable until the old replica is gone and is subsequently pruned.

Blocking — same-ID rotations still reactivate retired credentials and permissions

Every replica runs the full write-and-sweep reconcile on a ten-second ticker (pkg/authserver/server_impl.go:301-327). writeConfiguredClient intentionally overwrites an already-configured row without checking configuration material (pkg/authserver/storage/redis.go:656-730). Therefore, during a RollingUpdate that changes a delegate secret, scopes, or audiences while retaining its ID, an old replica can overwrite the new row on its next tick. A revoked secret or removed authorization is then valid again until the next competing write.

This resolves the former readiness deadlock only by allowing last-write-wins credential/authorization rollback. Please add a versioned/authoritative handoff in which stale replicas cannot overwrite a newer configuration, plus a two-replica rotation test that exercises further old-replica heartbeats after the new configuration is written.

Blocking — in-memory “atomic” reconcile can leave partial state on capacity failure

MemoryStorage.ReconcileConfiguredClients promises an atomic desired-set application (pkg/authserver/storage/memory.go:518), but prunes stale configured rows before inserting new rows (:548-569). insertClientLocked can then return ErrClientCapacity; earlier pruning and any preceding desired-row writes stay committed. A rejected configuration can therefore delete existing configured clients or leave a partial set.

Preflight capacity/evictability before changing the maps, or stage/rollback the full mutation, and add a failing-capacity test that asserts the pre-reconcile set is unchanged.

Resolved from my prior review: matching legacy rows are adopted, and the previous lease-related startup/removal problems are no longer present because leases were removed. The explicit SPIFFE-placeholder cleanup deferral remains acceptable. Non-blocking: the new storage architecture section contradicts itself by first calling SPIFFE clients part of the periodic desired set and then excluding them (docs/arch/11-auth-server-storage.md:254-261).

[jhrozek]

Thanks — two of these are fixed and pushed, one needs a bit more design work before I implement it.

Fixed: in-memory partial state on capacity failure. MemoryStorage.ReconcileConfiguredClients now stages the full desired-set mutation against an isolated copy of the client maps/order and only commits it back if every step succeeds; a capacity failure partway through now leaves the live state completely untouched instead of partially pruned. Added a test that forces ErrClientCapacity mid-reconcile and asserts the prior state (including the client that would have been pruned) is unchanged.

Fixed: a replacement replica deleting a still-live old client. Redis now marks a client absent from a reconcile's desired set (StaleSinceUnix) instead of deleting it immediately. Any replica that still writes the row (because it's still in that replica's desired set) resets the mark to unset — no cross-replica coordination needed. Only once a row has sat marked-stale and unrefreshed for a bounded grace period (30s, several heartbeats) does a later sweep actually delete it. So a client removed from config now survives until every replica that still wants it has stopped reconciling, not until the first replica that doesn't want it happens to run a sweep.

Not yet fixed: same-ID material rotation (secret/scope/audience change) still last-write-wins during a rollout. This one I want to get right rather than patch quickly, since it's the same territory the earlier lease design got wrong. Current plan: gate writeConfiguredClient's write on a monotonic generation number sourced from the owning CR's metadata.generation (not a per-pod ID or wall-clock time) — a write carrying an older generation than what's already stored is silently dropped, no error, no blocking, so it can't reintroduce the readiness deadlock the lease caused. Two independent reviews (OAuth/credential-security and Redis/concurrency) confirmed this is sound and fits the existing WATCH/MULTI pattern already in this file with no new locking primitive. Implementation is in progress; will push once it's done and tested.

[JAORMX]

Blocking — a live old replica can restore retired delegate credentials and authorization

writeConfiguredClient unconditionally replaces every already-configured row (pkg/authserver/storage/redis.go:695-700, :730-739), and every replica invokes that full writer every ten seconds (pkg/authserver/server_impl.go:301-324). There is no CR generation, version, or other monotonic authority stored or compared. The included test intentionally locks in this behavior (pkg/authserver/storage/redis_test.go:803-811).

Rollout sequence:

1. The ready old replica has delegate C with secret S-old, scope broad, and audience A-old in its immutable configuredClients slice.
2. The CR changes C in place to S-new, a narrowed scope, and/or A-new; the new replica starts and its startup reconcile writes that new material.
3. Kubernetes retains the ready old replica during the RollingUpdate. On its next heartbeat it calls ReconcileConfiguredClients with the old slice, and writeConfiguredClient overwrites the shared Redis row with S-old/broad/A-old again.
4. The old credential and removed permissions are accepted until a later new-replica write, and can be repeatedly reactivated while the old replica remains live.

The stale grace correctly preserves a removed client while an old replica still runs, but it does not protect same-ID material. Please make writes monotonic from the owning CR generation (or an equivalent authoritative, durable version) with Redis CAS/WATCH so a lower-generation replica is ignored; add a two-replica rolling-update test that verifies further old heartbeats cannot restore the old secret, scope, or audience.

[JAORMX]

Blocking — retired auth-server instances keep reconciling obsolete configuration

The new ticker worker stops only in (*server).Close (pkg/authserver/server_impl.go:301-330, :642-645). But the established shared-storage replacement path deliberately retires the old auth server with CloseIdleConnections, not Close (pkg/authserver/server.go:85-103; pkg/authserver/server_impl.go:627-631). Consequently the replaced instance keeps its ticker and desired set indefinitely. It continues restoring removed rows and reasserting old configuration, so the 30-second stale grace cannot converge; it also amplifies the material-rollback issue below. This leaks a goroutine/ticker per replacement and violates the documented retirement contract.

Please bind the reconciliation worker to the same lifecycle that retires/replaces a server (or otherwise ensure it cannot survive configuration replacement), and add a shared-storage replacement test using CloseIdleConnections that proves the old desired set no longer reconciles.

Blocking — old replicas still reactivate rotated credentials and permissions

Each replica performs a full write-and-sweep every ten seconds (pkg/authserver/server_impl.go:301-327). writeConfiguredClient overwrites an already-configured Redis row (pkg/authserver/storage/redis.go:695-750), so an old replica with the same client ID can restore its old secret, scopes, or audiences after the replacement has written tightened/rotated material. The current Redis test intentionally encodes this last-write-wins behavior (pkg/authserver/storage/redis_test.go:803-811). During a normal RollingUpdate, that makes revoked credentials and removed authorization intermittently valid until every old replica exits.

Please fence writes using an authoritative monotonic configuration revision/generation so stale replicas cannot overwrite newer material without blocking replacement readiness. Add a two-replica test that reconciles old material after newer material and proves the newer secret/scopes/audiences remain stored.

Blocking — Redis failure can expose a partial rejected desired set

ReconcileConfiguredClients applies Redis client writes one-by-one (pkg/authserver/storage/redis.go:665-692, :717-739). If an earlier write succeeds and a later client collides with a protected DCR/reserved row, it returns an error after leaving the earlier change live. Other replicas can then observe a partially applied configuration that the server rejected. The new in-memory implementation correctly stages and commits atomically (pkg/authserver/storage/memory.go:548-569); Redis needs equivalent preflight/atomic desired-set handling.

Please preflight or atomically apply the entire desired set and add a test that a later collision leaves earlier rows unchanged.

Resolved from my prior review: the in-memory capacity failure is now atomic; stale removal during a normal rolling replacement has a grace window; matching legacy Redis rows are adopted; SPIFFE placeholder cleanup remains an explicitly documented acceptable deferral. Non-blocking: configuredClients and the legacy fallback duplicate delegate-client construction in pkg/authserver/server_impl.go:379-402.

[JAORMX]

Fresh review at 4030642e1ebd23044a66f1c21b8a56af56b57fee:

Blocking — bulk memory reconciliation deletes SPIFFE reservations

The PR says SPIFFE placeholders remain outside this reconciler, but the in-memory path does the opposite. preflightDurableCollisions creates the inert placeholder through ReconcileConfiguredClient (pkg/authserver/storage/spiffe_decorator.go:76-84); MemoryStorage.ReconcileConfiguredClient records every such row in configuredClients (pkg/authserver/storage/memory.go:613-619, :630-635). Startup then invokes bulk reconciliation on the unwrapped base store with its delegate-only desired set (pkg/authserver/server_impl.go:347-391). The memory sweep deletes every marked ID absent from that set, with no reserved-placeholder exemption (pkg/authserver/storage/memory.go:575-580).

So a configured SPIFFE client ID's durable reservation is removed in the same startup path. This violates ConfiguredClientReconciler's explicit guarantee never to prune SPIFFE reserved placeholders (pkg/authserver/storage/types.go:739-757) and permits a later non-SPIFFE user of shared storage to claim that ID. Redis correctly exempts reserved rows (pkg/authserver/storage/redis.go:779-780) and has coverage; memory needs the same protection and a combined SPIFFE-plus-bulk-reconcile regression test.

The three blockers in my prior current-head review remain unchanged: retirement via CloseIdleConnections does not stop the reconciliation worker; an old rolling replica can restore rotated/tightened same-ID material on its heartbeat; and Redis can expose writes from an ultimately rejected multi-client desired set. The documented SPIFFE placeholder cleanup deferral remains acceptable, but it cannot delete active placeholders as above.

CI is green at this head; that status is separate from these review blockers.