search

aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.7k stars 3.93k forks source link

SSM - parameter path and value changes are not updated in the stack. #7722

Open brettswift opened 4 years ago

brettswift commented 4 years ago

There are two ways of resolving ssm parameters (as long as it isn't a secure parameter):

  1. ssm.StringParameter.valueForStringParameter
  2. ssm.StringParameter.fromStringParameterAttributes

They behave differently, but according to the documentation I would expect them to behave the same.

  1. Test by changing paths ie: change the SSM path from one deployment to the next. Retrieving by value does what I expect - it updates values when the path changes.
    Retrieving by attributes does not. The value in the stack will not change.

  2. Test by changing values it doesn't matter what we change, the values in the stack do not change.

Reproduction Steps

https://github.com/brettswift/cdk-ssm-test

follow the README.md

shell scripts are there to demonstrate everything.

Environment

This is :bug: Bug Report

brettswift commented 4 years ago

I'm seeing differing behaviour with the valueForStringParameter function, that is working in the sample provided vs my actual stack.

I've changed the value in SSM, but do not see any change when I deploy.

joraycorn commented 4 years ago

The same happened to me. I'm trying to save a new version of my layerArn into SSM. While my layer updates, SSM valueForStringParameter doesn't appear to change at all for my other stacks.

CDK diff is telling me that my layer gets replaced, but is telling me that none of my other stacks is being updated.

Any idea of what's going on here ?

cynicaljoy commented 4 years ago

This is because the values are being stored in your cdk.context.json -- if you clear the context, which you can either clear the entire context cdk context --clear or you can reset by key cdk context --reset <key> then the latest value should be loaded the next time you cdk synth

Seamus1989 commented 2 years ago

doesnt work for me

corymhall commented 2 years ago

After testing this the only scenario that I could reproduce was the first one - changing the parameterName. It looks like this is due to the way CloudFormation (and CDK) handles parameters, i.e. by default cdk deploy uses --previous-parameters which tells CloudFormation to use the previous parameter value (the parameter name in this case).

When you use valueForStringParameter CDK generates a logicalId that includes the parameterName. So when you change the parameter name the logicalId changes which causes CloudFormation to see it is a different (new) resource and it fetches the new value. https://github.com/aws/aws-cdk/blob/main/packages/@aws-cdk/aws-ssm/lib/parameter.ts#L441-L441

When you use fromStringParameterAttributes the logicalId is set to whatever you provide as the construct id. When the Default for a parameter of type AWS::SSM::Parameter::Value<String> changes, it does not cause CloudFormation to fetch the new value, because of the --previous-parameters options. If you instead run cdk deploy --no-previous-parameters you should see the new value being used.

There are a couple of workarounds that you can use.

  1. Run cdk deploy with --no-previous-parameters when you update the parameter name.
  2. When you change the parameter name, also change the construct id to have CloudFormation treat it as a different resource.

A permanent solution to this may be to update the fromStringParameterAttributes to generate a logicalId the say way that valueForStringParameter does.

ayozemr commented 2 years ago

Run cdk deploy with --no-previous-parameters when you update the parameter name.

OMG didnt know about this and saved my day... Thanks!

0xdevalias commented 1 year ago

A permanent solution to this may be to update the fromStringParameterAttributes to generate a logicalId the say way that valueForStringParameter does.

Would be awesome if CDK could be updated with better default values here.. as currently this is an issue I have run into multiple times, and it's always one that I forget about until I have to deep dive into why things aren't working as expected. It's definitely not obvious/expected behaviour IMO.

Edit: Also.. where are those values that using --no-previous-parameters resets cached; as they don't appear to be in my cdk.context.json file at all?

Edit 2: Haven't checked this out fully yet, but sounds promising:

Looking at cdk context --help, I have the values I expect from cdk.context.json, and the explicit values I set in cdk.json, but none of them seem to correlate to the paths/values I'm using with StringParameter.fromStringParameterAttributes.. so still not sure where those values are being cached/read from/etc.

Edit 3: Looking closer at the CDK docs for StringParameter.fromStringParameterAttributes(scope, id, attrs), and particularly StringParameterAttributes, there appears to be a forceDynamicReference option:

Looking for more information about dynamic references:

0xdevalias commented 1 year ago

Edit 3: Looking closer at the CDK docs for StringParameter.fromStringParameterAttributes(scope, id, attrs), and particularly StringParameterAttributes, there appears to be a forceDynamicReference option:

Ha.. so apparently forceDynamicReference and the underlying functionality related to it is brand new as of CDK 2.87.0 (released ~2 weeks ago):

Previously, when we import a SSM parameter by ssm.StringParameter.fromStringParameterAttributes, we use CfnParameter to get the value.

  "Parameters": {
    "importsqsstringparamParameter": {
      "Type": "AWS::SSM::Parameter::Value<String>",
      "Default": {
        "Fn::ImportValue": "some-exported-value-holding-the-param-name"
      }
    },

However, Parameters.<Name>.Default only allows a concrete string value. If it contains e.g. intrinsic functions, we get an error like this from CFn: Template format error: Every Default member must be a string.

This PR changes the behavior of fromStringParameterAttributes method. Now it uses CfnDynamicReference instead of CfnParameter if a parameter name contains unresolved tokens.

Originally posted by @tmokmss in https://github.com/aws/aws-cdk/pull/25749

Another thing we can say about ssm parameters is that it doesn't differ much between CfnParameters and dynamic references. Reading through the document, it seems that most of the characteristics are the same, such as when it's resolved and updated, where it can be used in a template, etc. There are of course some differences e.g. max num of references (200 vs 60), but they seems trivial.

_Originally posted by @tmokmss in https://github.com/aws/aws-cdk/pull/25749#discussion_r1213201913_

I'm now wondering whether switching a parameter to a dynamic reference should really be considered as a breaking change. As far as I read the docs, there seems to be no remarkable difference between them. Given it's also very rare to use a lazy token for parameter names, we can tolerate the change, maybe under a feature flag.

_Originally posted by @tmokmss in https://github.com/aws/aws-cdk/pull/25749#discussion_r1229498664_

If only we could stop using CfnParameter and use CfnDynamicReference instead for all cases... I see no point to use CfnParameter here. (Actually, isn't that the purpose of feature flags?)

_Originally posted by @tmokmss in https://github.com/aws/aws-cdk/pull/25749#discussion_r1229662611_

There are some limitations https://github.com/aws/aws-cdk/pull/22239#issuecomment-1262069499

_Originally posted by @corymhall in https://github.com/aws/aws-cdk/pull/25749#discussion_r1229669719_

So how about letting users choose which they use, parameter or dynamic reference? We'll add a property like forceDynamicReference?: boolean (default to false) to CommonStringParameterAttributes. This is kind of a leaky abstraction, but it should at least solve all the problem above. Plus we can easily ensure there is no breaking change, without adding any feature flag.

_Originally posted by @tmokmss in https://github.com/aws/aws-cdk/pull/25749#discussion_r1229717891_

Playing with this new forceDynamicReference option, making this change:

  const stripeSecretApiKey = StringParameter.fromStringParameterAttributes(
    this,
    'StripeSecretApiKey',
    {
      parameterName: `${parameterStoreNamespace}/stripe/secret_api_key`,
+     forceDynamicReference: true,
    }
  ).stringValue

Resulted in this cdk diff output:

  Stack REDACTED

  Parameters
- [-] Parameter StripeSecretApiKeyParameter: {"Type":"AWS::SSM::Parameter::Value<String>","Default":"/REDACTED/development/stripe/secret_api_key"}

  Resources
  [~] AWS::Lambda::Function FnStripeCustomerSubscriptionEventsHandler/Handler FnStripeCustomerSubscriptionEventsHandlerB91CA9D9
   └─ [~] Environment
       └─ [~] .Variables:
           └─ [~] .stripeApiKey:
               └─ @@ -1,3 +1,1 @@
-                 [-] {
-                 [-]   "Ref": "StripeSecretApiKeyParameter"
-                 [-] }
+                 [+] "{{resolve:ssm:/REDACTED/development/stripe/secret_api_key}}"

For reference/comparison, changing the above StringParameter.fromStringParameterAttributes to use StringParameter.valueForStringParameter as follows:

const stripeSecretApiKey = StringParameter.valueForStringParameter(
  this,
  `${parameterStoreNamespace}/stripe/secret_api_key`
)

Resulted in this cdk diff output:

  Stack REDACTED

  Parameters
- [-] Parameter StripeSecretApiKeyParameter: {"Type":"AWS::SSM::Parameter::Value<String>","Default":"/REDACTED/development/stripe/secret_api_key"}
+ [+] Parameter SsmParameterValue:--REDACTED--development--stripe--secret_api_key:REDACTED.Parameter SsmParameterValueREDACTEDdevelopmentstripesecretapikeyREDACTEDParameter: {"Type":"AWS::SSM::Parameter::Value<String>","Default":"/REDACTED/development/stripe/secret_api_key"}

  Resources
  [~] AWS::Lambda::Function FnStripeCustomerSubscriptionEventsHandler/Handler FnStripeCustomerSubscriptionEventsHandlerB91CA9D9
   └─ [~] Environment
       └─ [~] .Variables:
           └─ [~] .stripeApiKey:
               └─ [~] .Ref:
-                  ├─ [-] StripeSecretApiKeyParameter
+                  └─ [+] SsmParameterValueREDACTEDdevelopmentstripesecretapikeyREDACTEDParameter

Exploring the CDK code to see exactly how --no-previous-parameters works:

We can see the option being parsed from the CLI as usePreviousParameters here:

https://github.com/aws/aws-cdk/blob/6c75581ae2b9537fa9d1d724b837fe81ae22d345/packages/aws-cdk/lib/cli.ts#L510C11-L510C32

We can see lib/api/deploy-stack.ts using the same usePreviousParameters option:

https://github.com/aws/aws-cdk/blob/6c75581ae2b9537fa9d1d724b837fe81ae22d345/packages/aws-cdk/lib/api/deploy-stack.ts#L136-L143

Which is used later on in that same file. When usePreviousParameters is true then templateParams.updateExisting is called, otherwise templateParams.supplyAll is called.

https://github.com/aws/aws-cdk/blob/6c75581ae2b9537fa9d1d724b837fe81ae22d345/packages/aws-cdk/lib/api/deploy-stack.ts#L265-L272

We can see the definitions of both of these functions here:

https://github.com/aws/aws-cdk/blob/6c75581ae2b9537fa9d1d724b837fe81ae22d345/packages/aws-cdk/lib/api/util/cloudformation.ts#L399-L419

Which seems to describe how it tells CloudFormation to use the old values as:

Will take into account parameters already set on the template (will emit UsePreviousValue: true for those unless the value is changed), and will throw if parameters without a Default value or a Previous value are not supplied.

We can see the definition of the ParameterValues class here:

https://github.com/aws/aws-cdk/blob/6c75581ae2b9537fa9d1d724b837fe81ae22d345/packages/aws-cdk/lib/api/util/cloudformation.ts#L422-L504

Of particular note/interest is the hasChanges function in that class:

https://github.com/aws/aws-cdk/blob/6c75581ae2b9537fa9d1d724b837fe81ae22d345/packages/aws-cdk/lib/api/util/cloudformation.ts#L478-L501

Which suggests that:

If any of the parameters are SSM parameters, deploying must always happen because we can't predict what the values will be. We will allow some parameters to opt out of this check by having a magic string in their description.

The magic string is denoted by SSMPARAM_NO_INVALIDATE, which is defined in aws-cdk-lib/cx-api:

https://github.com/aws/aws-cdk/blob/6c75581ae2b9537fa9d1d724b837fe81ae22d345/packages/aws-cdk-lib/cx-api/lib/cxapi.ts#L41-L47

The hasChanges function is called in lib/api/deploy-stack.ts:

https://github.com/aws/aws-cdk/blob/6c75581ae2b9537fa9d1d724b837fe81ae22d345/packages/aws-cdk/lib/api/deploy-stack.ts#L274

And is passed to canSkipDeploy as the parameterChanges param:

https://github.com/aws/aws-cdk/blob/6c75581ae2b9537fa9d1d724b837fe81ae22d345/packages/aws-cdk/lib/api/deploy-stack.ts#L682-L694

Which will force a deploy when ssm parameters are used:

https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk/lib/api/deploy-stack.ts#L735-L743

Which unfortunately seems to suggest that the behaviour for how this is actually resolved at deploy time looks like it is defined somewhere else.. either in CloudFormation itself, or perhaps somewhere in the CDK bootstrapper/related code/similar; not too sure.

Looking deeper into how CDK template diffing works:

The main diffTemplate function is defined here:

https://github.com/aws/aws-cdk/blob/6c75581ae2b9537fa9d1d724b837fe81ae22d345/packages/%40aws-cdk/cloudformation-diff/lib/diff-template.ts#L31-L78

Which then seems to call calculateTemplateDiff(currentTemplate, newTemplate), which is defined here:

https://github.com/aws/aws-cdk/blob/6c75581ae2b9537fa9d1d724b837fe81ae22d345/packages/%40aws-cdk/cloudformation-diff/lib/diff-template.ts#L96-L115

That seems to use one of various different DIFF_HANDLERS depending on the key being compared, falling back to a default diffUnknown if there is no more specific handler.

DIFF_HANDLERS is defined here:

https://github.com/aws/aws-cdk/blob/6c75581ae2b9537fa9d1d724b837fe81ae22d345/packages/%40aws-cdk/cloudformation-diff/lib/diff-template.ts#L10-L29

It seems to use impl.diffParameter for the parameters, which is defined here:

https://github.com/aws/aws-cdk/blob/6c75581ae2b9537fa9d1d724b837fe81ae22d345/packages/%40aws-cdk/cloudformation-diff/lib/diff/index.ts#L25-L27

And then calls types.ParameterDifference(oldValue, newValue), which is defined here:

https://github.com/aws/aws-cdk/blob/6c75581ae2b9537fa9d1d724b837fe81ae22d345/packages/%40aws-cdk/cloudformation-diff/lib/diff/types.ts#L436-L438

And seems to just extend from Difference<Parameter>, which is defined here:

https://github.com/aws/aws-cdk/blob/6c75581ae2b9537fa9d1d724b837fe81ae22d345/packages/%40aws-cdk/cloudformation-diff/lib/diff/types.ts#L273-L310

Which then uses deepEqual, which is defined here:

https://github.com/aws/aws-cdk/blob/6c75581ae2b9537fa9d1d724b837fe81ae22d345/packages/%40aws-cdk/cloudformation-diff/lib/diff/util.ts#L1-L60

Going back to calculateTemplateDiff, once the raw differences are identified, it then passes them into types.TemplateDiff(differences), which is defined here:

https://github.com/aws/aws-cdk/blob/6c75581ae2b9537fa9d1d724b837fe81ae22d345/packages/%40aws-cdk/cloudformation-diff/lib/diff/types.ts#L9-L21

We can see that parameters ends up as DifferenceCollection<Parameter, ParameterDifference>, and is initialised in the constructor here:

https://github.com/aws/aws-cdk/blob/6c75581ae2b9537fa9d1d724b837fe81ae22d345/packages/%40aws-cdk/cloudformation-diff/lib/diff/types.ts#L48

0xdevalias commented 1 year ago

Opened a tangentially related issue to allow --no-previous-parameters to be configured in cdk.json:

WtfJoke commented 1 year ago

forceDynamicReference didnt helped in my case.

I used following workaround:

 const fetchAlwaysNewVersionId = `ImportedVersion-${Date.now()}}`;
    const codeObjectVersion = StringParameter.fromStringParameterAttributes(
      this,
      fetchAlwaysNewVersionId,
      {
        parameterName,
      }
    );

Since each synth the id is changed (due to usage of Date.now())), it will be refetched.

anentropic commented 2 weeks ago

This is a terrible default behaviour!

If I'm making a deployment I never want to risk deploying with stale parameter values! I nearly messed up my production environment with this unexpected stupidity.

Is doing cdk deploy --no-previous-parameters still the correct answer? If I do cdk deploy --help that arg is not listed.

There is this one instead:

      --previous-parameters  Use previous values for existing parameters (you
                             must specify all parameters on every deployment if
                             this is disabled)         [boolean] [default: true]