fix(ssm): lazily create the backing CfnParameter for imported string parameters - #38405
fix(ssm): lazily create the backing CfnParameter for imported string parameters#38405kawaaaas wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
The pull request linter fails with the following errors:
❌ Fixes must contain a change to an integration test file and the resulting snapshot.
If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.
✅ A exemption request has been requested. Please wait for a maintainer's review.
|
Exemption Request This PR does not add an integ test because the change has nothing for an integ test to exercise. The fix only changes when a The only observable effect of this change is a difference in synthesized template shape (whether an unreferenced Happy to add one if a maintainer sees a code path this reasoning misses. |
Issue (if applicable)
Closes #38396
Reason for this change
ssm.StringParameter.fromStringParameterName()(andfromStringParameterAttributes()/fromStringParameterArn()/StringListParameter.fromListParameterAttributes()) eagerly create aCfnParameter(AWS::SSM::Parameter::Value<...>) to back.stringValue/.stringListValueas soon as the factory method is called, even when the caller never reads that property.ecs.Secret.fromSsmParameter()only ever reads.parameterArn, which needs no backing CloudFormation resource since it is a plainStack.of(scope).formatArn(...)string. So importing a parameter by name and using it only for its ARN, the pattern in the issue, leaves aCfnParameterin the template that nothing references. This has always been synthesized. It only became visible once the new defaultCloudFormation-Validaterule set (#38135, 2.262.0) started flagging it asW2001.Description of changes
Changed
stringValue/stringListValuefrom an eagerly-assigned field to a memoizedgetaccessor in the following methods.StringParameter.fromStringParameterArn()StringParameter.fromStringParameterAttributes()(also coversfromStringParameterName())StringListParameter.fromListParameterAttributes()The backing
CfnParameter(orCfnDynamicReference, selection logic unchanged) is now only created the first time the property is read, using the samescope/idas before, so the logical ID and template output are unchanged for any code path that already reads it.Alternative considered. Gating this behind a feature flag. Went without one since nothing is being rejected and no template changes occur for any consumer that already reads
.stringValue/.stringListValue. The only difference is that aParametersentry that was never functional in the first place stops being emitted for ARN-only consumers. Flagging this explicitly for maintainer input, should this go behind a feature flag instead?Describe any new or updated permissions being added
None.
Description of how you validated changes
Unit tests only, in
aws-ssm/test/parameter.test.tsandaws-ecs/test/container-definition.test.ts.Parameterssection without ever reading.stringValue/.stringListValuewere relying on the old eager-creation behavior. Updated them to read the property first.Parameterssection is empty when only.parameterArnis used, for each of the three factory methods. This is the direct regression case..stringValue's first read happening inside aLazy.string()producer, asserting the template still comes out correct (see the design decision above).ecs.Secret.fromSsmParameter(ssm.StringParameter.fromStringParameterName(...)), asserting no unreferencedParametersentry and a correctSecrets[].ValueFrom. Confirmed this test actually catches the regression by running it against the pre-fix code and watching it fail.No integ test. The only thing to demonstrate here, that there is no unreferenced
Parametersentry, is a synth time template concern that the unit tests already cover deterministically. An integ test would add AWS account dependent CI cost without checking anything new.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license