Repository navigation
fix(deploy): a reader of a resource replaced in the deploy is sent, not skipped, when a secret rewrote its values (#4838) - #4851
Merged
Conversation
…ot skipped, when a secret rewrote its values (#4838)
…cement; fixture tag from 5f on; leak gates (#4838 review)
Owner
Author
|
Merged — thanks. Step 5g made this one easy to trust: the pre-fix arm went red exactly where predicted (the reader kept the deleted queue's ARN), and the broad set passed on 0c5911c. |
This was referenced Oct 13, 2026
fix(deploy): keep a NoEcho-named resource's ARN in its attributes, outputs and exports (#4849)
#4850
Merged
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.
Closes #4838
Problem
Take an SQS queue named from a secret. The secret rotates, and a create-only change then REPLACES the queue under the new name (
sdin-q-1->sdin-q-2). A reader of the queue that also spells the same{{resolve:...}}reference itself, for example in a tag besideValue: Fn::GetAtt Queue.Arn, was not updated:sdin-q-2 -> {{resolve:...}}.arn:...:{{resolve:...}}that the reader had stored for the old queue.recordMatchesDesiredinsrc/deployment/deploy-engine/update.ts) compared equal, and the reader was skipped.Fix
recordMatchesDesiredis now false, so the resource is SENT rather than skipped, when both of these hold:redactionRewroteDesired): the redacted desired bag differs from the resolved one after the sameNoEchocoordinate masking; andreadsProducerReplacedThisRun): anyRef/Fn::GetAtt/DependsOntarget whose id differs from the one the deploy LOADED (healBaseline).A reference does not say which value it stood for. After a replacement that changed the id, an equal reference form is therefore no evidence that AWS holds the value.
Still skipped:
--replace).Cost: a resource that spells a secret and reads a resource replaced with a new id gets one update it may not need. Replacing with a new id is already a deploy that rewrites most readers.
The issue's other option (re-resolve the stored reference under the loaded physical id) would need the resolver at the compare site. The change here stays local to the skip decision. Both no-change skips (the plain one and the
NoEcho-confirmed one) read the same predicate.Create-only paths. A top-level property the redaction rewrote also counts as MOVED in the two places that decide whether a propagated replacement stands. Without that, such a property's reference forms compared equal, the ceiling was lowered, and a reader like an
AWS::Lambda::EventSourceMapping(create-onlyEventSourceArn) was updated in place. Cloud Control fails such an update; an SDK provider would leave the mapping on the deleted queue. With the fix it is replaced, as CloudFormation would do.Not covered (#4852): the check is scoped to this run, so these shapes still keep a reader on a deleted resource:
The general fix needs a persisted generation witness, which is a state-schema decision.
Tests
tests/unit/deployment/deploy-engine-reader-replaced-producer-4838.test.tsruns the realDeployEngine/DiffCalculator/ resolver, using the #4802 harness. Cases:FifoQueuereplacement, with the reader spelling the secret in a tag: the reader is sent the new ARN. On main it is skipped (sent = []).GetAtt Queue.Kind) is still skipped.--replace) leaves the reader that spells the secret skipped.EventSourceArnreads the replaced queue is REPLACED, not updated in place.Mutation probes (each restored from a byte-exact copy), all red:
The same term in the
NoEcho-parameter create-only arm (movedAt) has no dedicated case.Integ
tests/integration/secret-derived-immutable-names, new step 5g. It builds on #4805'sAssembledNameQueue/AssembledQueueReader, which, from step 5f on (SDIN_READER_EDIT=true), also spells the queue name's original token in a tag (AqName). Before 5f it does not, so the step-2 create still stores the plaintext ARN and step 4 still exercises #4805.aqueuefield tosdin-aq2-<suffix>.SDIN_AQ_RESPELL=true,...:aqueue:AWSCURRENT}}). That is a create-only change, so the queue is REPLACED under the rotated name.AssembledNameQueue ... replaced, the new queue's ARN ends with the rotated name, and the old queue is gone.expect_eq: the reader keeps the deleted queue's ARN.AqSubscription, a new SNS subscription, subscribes the queue to the newAqTopic. Its create-onlyEndpointreads the queue's ARN and its filter policy spells the same token. The log must showAqSubscription ... replaced, and the topic's only subscription endpoint must be the new queue's ARN. Pre-fix the subscription is skipped and stays subscribed to the deleted queue.The leak gates cover both queue names, in 5g's deploy log and in the destroy log. Cleanup and the destroy gone-checks cover the replacement queue too. I synthesized both arms in a scratch copy. This lane has not run it against AWS.
Review round 1
state.jsonafter 5g. Persistence did not change.extractAllDependenciesreports.DependsOn-only reader and aNoEchocoordinate.Review round 2
Both re-reviews are CLEAN. Taken from their notes:
AqTopic/AqSubscriptionin step 5g. Cleanup deletes the topic by its per-run name, the destroy checks it is gone, and the fixture'scovers:list gains the two SNS types.rewrittenPathsis decided per resource, not per producer. A create-only path that spells a secret and reads a producer replaced under the SAME id, in a resource that also reads another producer replaced under a NEW id, is replaced although its value is equal. Narrowing it means intersecting with the paths that read a new-id producer. No real CDK app is known to produce the shape, and a stateful reader still meets the stateful guard and approval.