feat: Move an environment between SDK keys without rebuilding its client - #896
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
a96adce to
199ebb2
Compare
199ebb2 to
8b21c9f
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8b21c9f. Configure here.
8244281 to
564e023
Compare
Phase 3 of forward-porting concurrent multi-key support from v8 (#817). An environment now serves its whole accepted credential set, and moves its upstream connection between SDK keys by re-keying the one client it holds. ReconcileCredentials replaces UpdateCredential: it adds the incoming keys' mappings, re-anchors while the outgoing key still authenticates downstream traffic, then takes revoked mappings down. The re-anchor is LDClient.SetSDKKey plus the two components relay owns and the SDK cannot reach: the big segment synchronizer, which takes its key at construction and so is rebuilt, and the event publishers, which are repointed. There is no client to build, so there is nothing to roll back on a transient failure. SetSDKKey fails only on a key that is not valid in an HTTP header, which no retry fixes, so the environment parks on its current anchor and says so. Stream handlers are built per request behind a re-check of the accepted set. The map they used to live in was what made revocation racy: a handler stayed in it until the removal was processed, so a request that authenticated before a revocation still found a working handler. The REPORT stream endpoints let a client pace the body read that precedes the lookup, so that window is as long as the client wants. Removing a credential no longer closes the SDK client. The client is not tied to the key it was built with any more, so closing it there would tear down an environment's only upstream connection whenever any SDK key was revoked. The status resource's credential fields now come from one accepted-set snapshot, so they cannot disagree with each other, and neither they nor the metrics derived from them depend on map iteration order. Also pays off the deletions the previous phases deferred, because their callers live here: the single-key rotation shims, UpdateCredential and CredentialUpdate, and EnvironmentParams.ExpiringSDKKey with ExpiringKeyRep.ToParams. go.mod temporarily pins go-server-sdk#457 and go-sdk-events#63, which must become released versions before this merges.
It was written to check an assumption about the cost of building a handler per request rather than caching it, which it confirmed. The numbers are recorded in the comment on acceptsForStream; the benchmark itself is not worth carrying.
Offline mode validated a credential payload after creating the environment. NewEnvContext seeds the rotator from the same params, so a payload declared malformed still had its anchor and primary mobile key admitted, while every key its arrays listed was refused. A relay serving that environment authenticated the one credential the payload's authoritative arrays omitted, and 401'd the SDKs holding the keys those arrays named. Validate first and abandon the environment on a malformed payload, which is where the auto-config stream already validates relative to applying a put. An update validates before writing any of the identifiers, TTL or secure mode, so a refused environment is left exactly as it was rather than half updated. UpdateHandler now reports a refusal, because ArchiveManager recorded the archive's version as applied either way. A corrected archive can carry the same version and data ID as the one just refused, which the unchanged-archive check would then skip, so the correction never landed until the process restarted. The version advances only once the handler accepts the environment.
…e key A payload with no mobKey and an empty mobileKeys[] is valid: it makes the environment server-side only. Reconcile reported a mobile repoint only when a replacement key existed, so this case produced no signal at all and the event dispatcher kept forwarding on the key the payload had just revoked. Report the revocation separately from a repoint, and stop the mobile analytics endpoint when it fires. This does not rescue the events already queued: closing runs a final flush, which goes out on the revoked key and is refused. Nothing can deliver them, because LaunchDarkly revoked the key before relay was told. What it does is stop relay holding a dead credential and its goroutines for the life of the environment, and say plainly in the log that those events are lost.
…ch-up Two paths in the re-anchor had no test. The first is the invariant that makes a failed re-anchor safe. A brand-new anchor's connection mappings go up before the client is re-keyed, because a key cannot serve before its mappings exist, so a failure has to take them back down again. The test drives a refusal through the fake client and asserts the whole sequence, not just the end state: the key is registered, then unregistered, and the previous anchor is left accepted and still serving. The second is the catch-up for an anchor that moves while the initial client build is in flight. Holding the fake factory inside its unbuffered send keeps the build running for as long as the test needs, so the reconcile lands with no client present and the newly built client has to be caught up to the anchor the rotator already holds.
Stopping an endpoint closed its relays but left them attached. A later payload that gave the environment a mobile key again only replaced the credential, which updates relays whose goroutines have already exited, so mobile posts were accepted with a 202 and nothing reached LaunchDarkly. That is worse than what it replaced: before, those events at least failed loudly against a revoked key. Release the relays as well as closing them. The endpoint keeps its configuration and its credential, and the getters that built the relays in the first place are lazy, so the next event after a credential is restored builds live ones. The rest of the behaviour is unchanged: the events already queued when the key was revoked are still lost, because nothing can deliver them.
Releasing the relays left the endpoint able to build new ones. A post that got past the middleware before the revoked credential's connection mapping came down, or one whose body read was still in progress, reached the lazy getter and built live relays on the credential that had just been revoked. Those goroutines then lived for the rest of the environment's life, which is the leak that stopping the endpoint exists to close. Mark the endpoint stopped and discard events while it is, so nothing rebuilds on a credential the environment no longer accepts. Replacing the credential clears the mark, so a restored key still resumes forwarding.
…eans Three review comments asked what the code meant rather than what it did. Both comments on the refusal paths in updatedArchive leaned on "the check above" without saying which check or why it matters. lastKnownEnvs is the only thing that decides whether a reload counts as a change, so say that, and say why each branch records nothing: a hand-corrected environment need not change its version or its data, so recording the refused metadata would make the correction compare equal and be skipped. The ReconcileCredentials contract used "re-anchor" without defining it, and the word predates the current design, so a reader who knew the old one reasonably read it as building a replacement client. State what the anchor is, that re-anchoring moves that designation, that the client is re-keyed in place and keeps its store, and which components still have to be moved by hand.
… addCredential Two fixes that came from review of this branch rather than from me, kept and verified here. The stopped check belonged in the relay getters, not in dispatch. Reading the flag and then looking up the relay were two separate acquisitions of the endpoint's lock, so a revocation landing between them still built a relay on the credential that had just been revoked -- the same defect the flag was added to prevent, with a narrower window. The getters now decline while stopped, which makes the decision and the lookup one operation, and the callers note the drop. addCredential had no closed check. Close takes none of the reconcile locks, so a reconcile applying its additions, or the cleanup ticker draining them, can finish after Close has. A mapping registered then would let a request select an environment that has no validator or stream left to serve it. Also correct the per-connect measurements comment to say that the benchmark it quotes was removed, rather than implying a reader can still run it.
ea8a1eb to
0b32737
Compare
…ccur Every LaunchDarkly environment has at least one global key of each kind, so the auto-configuration stream never sends a payload that leaves an environment without a mobile key. The signal for that transition, and everything built to act on it, was guarding a state that does not arise. This removes the Reconcile result field, the branch that consumed it, and the endpoint machinery it drove: stopping an endpoint, releasing its relays, the stopped flag that kept the lazy getters from rebuilding on a revoked credential, and the drop path for a post that raced the revocation. Their tests go with them. Worth recording what is being given up, because it was not free. The original finding was real for the shape it described, and two follow-ups fixed genuine defects in the fix: releasing the relays rather than only closing them, so a restored credential could forward again, and moving the decision into the getters so the check and the relay lookup could not interleave. All of that only mattered for a payload the service does not produce.
Two comments described the same invariant and neither drew the line a reader needs. The auto-configuration stream always names a server key, a mobile key and an environment ID, because every LaunchDarkly environment has at least one global key of each kind. Manual configuration is the case that does not, and it reaches the rotator by a different route. BuildAcceptedSet attributed an environment with no mobile key to a hand-assembled offline archive. An archive is generated from the same LaunchDarkly data and carries the same guarantee, so that pointed at the wrong source and implied this function has to tolerate something that arrives somewhere else. It says instead why the shape is accepted rather than refused -- an accepted set needs only an SDK key and an anchor -- and that manual configuration, which never comes through here, is where it would come from. Initialize stated the permissive half without saying why, so a reader could not tell whether it was a manual-configuration allowance or a gap in what the stream sends. It now says which, and that those two checks are load-bearing for that reason. It also records that manually configured environments are never reconciled, so for them this is the whole credential set rather than a starting point.
|
In the description of the PR at the time of writing this there is: Is this still accurate? |
They are still using those PRs. I'm not planning on merging those until I have this work mostly finished relay side. That's why this is all being merged into a feature branch instead of directly into v9. |
| // ReconcileCredentials updates the environment's accepted credentials to match newSet. Calls are | ||
| // serialized. The method owns the order of operations: add, re-anchor, remove. Adding first | ||
| // registers the new keys' mappings, the re-anchor then moves the upstream connection while the | ||
| // outgoing key still authenticates downstream traffic, and revoked mappings come down last. |
There was a problem hiding this comment.
Usually in systems migrating credentials, revocation happens first to ensure least access. This comment says "revoked mappings come down last". Is there any risk that a revoked key gets used successfully by downstream traffic after the upstream connection moved?
There was a problem hiding this comment.
Yes, deliberately — and your instinct is right that it's worth pinning down, because the real exposure isn't this ordering.
Relay's keys do two jobs. The anchor is what relay authenticates with upstream; every accepted key is what SDKs authenticate with downstream. Tearing a downstream mapping down early doesn't improve least access, it just 401s SDKs that are still holding a key LaunchDarkly said was valid until some expiry. So within one reconcile the order is add, re-anchor, remove, which guarantees there's no instant where neither the outgoing nor the incoming key can serve. That window is microseconds to milliseconds, inside a single reconcile.
How long a revoked key actually keeps working is decided by its expiry, not by this ordering, and the two cases differ:
- Revoked outright (the payload stops listing it, no expiry):
reconcileAcceptedKeysdeletes it and queues its expiration in the same reconcile, so it comes down at the end of that same pass. - Deprecated with a grace period: it keeps authenticating until its expiry, which is the intended behaviour — that's what the grace period is for.
Where least access genuinely leaks is the thing you found further down: IsAccepted is a bare map lookup and never consults Expiry, so a deprecated key stays valid up to one ExpiredCredentialCleanupInterval past its stated expiry, and that interval has a validated minimum and no maximum. That's the real answer to "can a revoked key still be used successfully" — not the within-reconcile ordering, but the slop after expiry. Enforcing expiry in IsAccepted closes it.
Your removal-ordering point compounds the same thing from the other side: the comment justifies deferring the outgoing anchor's removal, but the code defers every revocation, so the window scales with the number of additions in the payload. Revoking non-anchor keys first and keeping only the outgoing anchor for last is the right shape, and I'll reword this comment to say what it actually covers rather than implying all removals need to be last.
| // change it. Its HTTP requests already set Authorization per request rather than relying on baked | ||
| // in headers, so re-keying it in place is within reach; what stands in the way is coordinating a | ||
| // backoff reset and a reconnect with the goroutine that owns its retry strategy. Refer to | ||
| // SDK-3200. It is nil when big segments are not configured. |
There was a problem hiding this comment.
Do you actually want this ticket number, SDK-3200, here?
There was a problem hiding this comment.
No, good catch — it should come out. This repository is public and SDK-3200 is an internal ticket, so to anyone reading ld-relay outside LaunchDarkly that reference is a dead end. It's also the same class of thing the repo already avoids in comments, since the reference rots independently of the code.
The explanation around it stands on its own: the synchronizer takes its key at construction and exposes no way to change it, its requests already set Authorization per request rather than relying on baked-in headers, so re-keying it in place is within reach, and what stands in the way is coordinating a backoff reset and a reconnect with the goroutine that owns its retry strategy. That tells the next reader what the obstacle is and roughly what it would take, without needing a ticket they can't open. I'll drop the sentence pointing at SDK-3200 and keep the rest.
|
I ran a multi-agent review against this PR (three parallel reviewers — general, security-focused, and adversarial — at head Blocking1. Critical — a key rotation can no longer heal a dead environmentTwo halves, same root: the environment now gets exactly one client-build attempt for its whole lifetime (
No race is required: relay restarts after a rotation happened while it was down → the auto-config cache replays the stale key → 401 kills the data system → the live payload's re-anchor calls Suggested direction: after a successful Failing proof test:
|
|
Thanks — this is a good catch list, and the three blocking items are all real. The important context is that all three are already fixed in PRs stacked above this one, which a review scoped to #896's head can't see. This is the third of five: #897, #898 and #899 sit on top of it and are pushed. Blocking1. Critical, rotation can't heal a dead environment — fixed in #899. Your suggested direction is exactly what's there: after a successful 2. High, refused payload acknowledged and never retried — fixed in #897. The doc/code mismatches are the more useful half, because one is live in this PR. 3. High, Didn't reproduceThe flaky filedata test. I can't get it to fail here: 0/10 whole-package under Your mechanism is sound either way and worth acting on independently of the test:
Agreed, acting on hereThe handler-build figures cite commit Want to talk about these rather than just patch them
Replying to your two inline comments separately — the revocation-ordering question is the interesting one and connects directly to the removal-ordering finding above. |
Review found each of these; none changes behaviour. reconcileCredentials claimed the stream manager validates a credential payload before recording its version. That is true in a later PR and false here, so this PR on its own told a reader something untrue. It now describes only what this function does, and why keeping the previous set is the safe response, which holds either way. The handler-build measurements named the commit that removed the benchmark they came from. Rebasing has since orphaned that commit, so the reference sent a reader looking for something no longer reachable. The numbers stay, with the benchmark described as throwaway rather than identified by hash. The add, re-anchor, remove rationale justified deferring the outgoing anchor's removal and then implied every removal has to be last. It now says which key the deferral is for, and that the others are removed later than they need to be, with the expiry rather than this ordering being what decides how long a revoked key authenticates. The big-segment synchronizer comment pointed at an internal ticket. This repository is public, so that is a dead end for most readers; the explanation of what blocks re-keying it in place stands without it.
Three places described the same operation and only sdks.LDClientContext.SetSDKKey was accurate. The two in relayenv said the re-anchor "moves the upstream connection", which reads as though the connection relocates when reanchor returns. It does not: SetSDKKey changes the credential future requests carry, and a stream that is already open keeps the previous key until LaunchDarkly or the network ends it. Both now say what happens instead, and ReconcileCredentials records the consequence that was written down nowhere. A graceful rotation keeps the outgoing key alive for its grace period on purpose, and a healthy stream can outlast that window, so Relay may go on consuming flag data on the deprecated key well after it reports the rotation applied. reanchor also notes that a nil return from SetSDKKey means only that the value is a legal HTTP header. No request is made there, so it is not evidence that LaunchDarkly accepts the key -- which is how a successful-looking re-anchor onto a dead data system went unnoticed for as long as it did. The success log said the environment had moved to a new key. It now says the key was changed and that an open connection keeps the previous one until it reconnects, which is what actually happened. Nothing asserts on that text.
|
Worked through the four remaining items. Two landed, two we're deliberately leaving — reasoning for each below so the decisions are on the record rather than just the outcomes. LandedRe-anchor docs overstating what moves (
The four smaller corrections ( Leaving, with reasons
One refinement on the framing, because it matters if anyone picks this up later: Removal ordering broader than its rationale. Agreed on the substance, declining on magnitude. The window is bounded by one reconcile: a couple of map writes and an eventsource registration per addition, plus a re-anchor measured in single-digit microseconds. Set that beside the expiry decision directly above — a key past its stated expiry keeps working for up to a minute, by design, with no configured maximum on the interval. Reordering inside the reconcile would shave microseconds off one path while that minute stands, so it would be odd to take one and not the other. Also worth flagging for whoever revisits it: "remove non-anchor revocations first" taken literally creates a gap, since removals have to follow additions or a payload replacing key A with key B leaves a window where neither authenticates. The defensible version is narrower — non-anchor removals between the additions and the re-anchor. I have reworded the comment so it no longer implies every removal needs to be last, which was the part that was actually wrong. No ownership check on credential mappings. Leaving this one too, though I'll note it's the one where I think the convention cuts your way: the payload is a genuine system boundary, so defending there is consistent rather than speculative. Both functions are byte-identical at Still curious about the two that wouldn't reproduce here, whenever you get a chance — the filedata flake and the closed-environment test — since a difference in machine or head would be useful to know about. |
2 participants
Summary
Phase 3 of forward-porting concurrent multi-key support from v8 (#817) to v9. Stacked on #894, so this diff shows only phase 3; GitHub retargets it to
feat/concurrent-keys-v9as the stack merges.An environment now serves its whole accepted credential set, and moves its upstream connection between SDK keys by re-keying the one client it holds.
ReconcileCredentialsreplacesUpdateCredential: add the incoming keys' mappings, re-anchor while the outgoing key still authenticates downstream traffic, then take revoked mappings down.What a re-anchor is now
LDClient.SetSDKKey, plus the two components relay owns that the SDK cannot reach: the big segment synchronizer, which takes its key at construction and so is rebuilt, and the event publishers, which are repointed through the existingReplaceCredential.There is no client to build, so there is nothing to roll back on a transient failure.
SetSDKKeyfails only on a key that is not valid in an HTTP header, which no retry fixes, so the environment parks on its current anchor and logs loudly; the next auto-configuration payload supplies a new key. A reconcile can also move the anchor while the first client build is still in flight, sostartSDKClientre-keys the client on install if the anchor moved underneath it.The line worth reviewing closely
removeCredentialno longer closes the SDK client. The client is not tied to the key it was built with any more, so closing it there would tear down an environment's only upstream connection whenever any SDK key was revoked, including the environment's original key once it has been rotated away from.TestRotatingTheSDKKeyRekeysTheSameClientasserts the client survives both the rotation and the outgoing key's expiry.Stream handlers
Built per request, behind a re-check of the accepted set. The map they used to live in was what made revocation racy: a handler stayed in it until the removal was processed, so a request that authenticated before a revocation still found a working handler, and the REPORT stream endpoints let a client pace the body read that precedes the lookup.
The build is cheap enough to do per connect, and this is measured rather than assumed —
env_context_stream_handler_bench_test.goputs the client-side path at 13ns with no allocations, which is faster than the two-level map lookup it replaced, and the heaviest provider (server-side V2, wrapping an init deadline and a basis-header closure) at 105ns and 96 bytes. Both are invisible next to the SSE handshake that follows.Behavior changes visible in tests
GetCredentials()reports the whole accepted set, so a key inside its grace period appears there as well as inGetDeprecatedCredentials(). The shared assertions inrelay/testutils_test.gonow compare against the designated credentials via adesignatedCredentialshelper, which is what they were really checking.The accepted set is also declarative where the old model was incremental: an old-format archive payload listing one SDK key revokes a key that a previous payload had granted a grace period. That is called out in the offline-mode test.
Deletions this phase owed
The previous phases retained superseded API because its callers live here: the single-key rotation shims,
UpdateCredentialandCredentialUpdate, andEnvironmentParams.ExpiringSDKKeywithExpiringKeyRep.ToParams. Also fixes the missingc.closedguard in the reconcile path, and the missingreturninAddEnvironmentthat calledUpdateCredentialon a nilEnvContextafter an initialization error.Follow-ups filed
Authorizationper request, so the plumbing needs no change; what needs designing is resetting its retry backoff from another goroutine./statuskey arrays (sdkKeys[],mobileKeys[]) remain phase 5, which is where the singularexpiringSdkKeyfield and the metrics derived from it get re-based onto them.Part of SDK-3199, under epic SDK-3188.
Note
Overview
Replaces incremental credential rotation with a declarative accepted-set model.
EnvContext.ReconcileCredentials(add → re-anchor → remove) supersedesUpdateCredential, and the legacy single-key rotator shims plusEnvironmentParams.ExpiringSDKKeyare removed.SDK key rotation no longer spins up a second client. Each environment keeps one upstream
LDClient, moved viaSetSDKKey(temporarygo.modpins unreleased go-server-sdk support). Re-anchor also repoints metrics/events dispatchers and rebuilds the big-segment synchronizer on the new anchor; revoking a key no longer closes the client. Stream handlers are built per connect with an accepted-set re-check instead of a cached handler map.Auto-config and offline archives validate with
envfactory.BuildAcceptedSetbefore applying changes; malformed payloads are refused without partial updates.ArchiveManageronly records environment versions after add/update succeed so hand-corrected archives retry without a version bump.Status reporting uses
GetAcceptedKeysfor anchor/mobile keys and picks a deterministicexpiringSdkKeyfrom the accepted set.Reviewed by Cursor Bugbot for commit a268af8. Bugbot is set up for automated code reviews on this repo. Configure here.