fix(ssm): cannot import a ssm parameter with a name containing unresolved token

[tmokmss]

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.

Since previously the case when Token.isUnresolved(attrs.parameterName) == true just resulted in a deployment error, this is not a breaking change.

Closes #17094

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[gitpod-io]

[corymhall]

I think there are cases where this could be a breaking change. There could be cases where Token.isUnresolved returns true, but the value in the template is still a resolved string.

For example

const parameterName = Lazy.string({ produce: () => 'myParameterName' });

Token.isUnresolved(parameterName) will return true, but the value that will be output will be

{
  "Type": "AWS::SSM::Parameter::Value<STRING>",
  "Default": "myParameterName",
}

We might need to use Tokenization.isResolvable instead.

[tmokmss]

Hi @corymhall thanks! I confirmed you are right (I added a test case and it failed.)

Now I tried Tokenization.isResolvable but for some reason it was not evaluated as true for a cross stack reference. I'll dive deep the reason later.

Perhaps we need to resolve the token first, and check if it contains CFn intrinsics, as you've done on cfn-resources.ts.

[tmokmss]

Tokenization.isResolvable(attrs.parameterName) won't work as expected because typeof(attrs.parameterName) is always 'string', which fails this evaluation.

aws-cdk/packages/aws-cdk-lib/core/lib/token.ts

Lines 319 to 321 in 1d7a9a8

	export function isResolvableObject(x: any): x is IResolvable {
	return typeof(x) === 'object' && x !== null && typeof x.resolve === 'function';
	}

So to determine whether a parameter name can be resolved to a concrete string, we can actually resolve it. Like this:

if (typeof Stack.of(scope).resolve(attrs.parameterName) != 'string') 

However, if we resolve the token here, it means a lazy string isn't actually evaluated lazily enough. Instead it's evaluated immediately to determine the condition. I'm not sure if it will break something though. A similar approach is also used in s3-deployment module, so maybe it's just fine.

aws-cdk/packages/aws-cdk-lib/aws-s3-deployment/lib/render-data.ts

Lines 18 to 19 in 1d7a9a8

	export function renderData(scope: Construct, data: string): Content {
	const obj = Stack.of(scope).resolve(data);

Alternatively, we can create a method like isLazyString to determine if a string is not a lazy value without resolving it. But then we cannot cover the case when Lazy.string produces cfn intrinsics like this:

Lazy.string({ produce: () => someResource.ref });

Another consideration is if we should really care this edge case. I think almost all of the devs don't use a Lazy string as a ssm parameter name to import it. At least we can fix it if someone would report an issue.

Hope you can suggest which way we should go, thanks! @corymhall

[corymhall]

It seems like what we are really trying to do is determine if the value is an intrinsic or not. In that case I think it might be best to just add a isIntrinsic method to Intrinsic

[tmokmss]

Thanks for the insight! I agree with that the only case we have to handle is when parameterName contains intrinsics.

But attrs.parameterName is just a string (like ${Token[TOKEN.274]}) before resolution. Is it possible to determine if the string is actually an instrinsic without resolving it? (And I realized my isLazyString solution also falls in this question...)

[tmokmss]

@corymhall I see, but isn't it too complicated from users' perspective? We cannot tell which case works and which case doesn't.

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?)

[corymhall]

There are some limitations #22239 (comment)

[tmokmss]

@corymhall Thanks, I didn't know that!

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.

[corymhall]

@tmokmss lets do both. Lets cover what we can (my comment above) and then allow the user to force the dynamic reference. Neither should need a feature flag.

[tmokmss]

@corymhall Now dynamic reference is used only when cdk.Fn is explicitly used. It can address the original use case reported on the issue.

As for other parameter name containing a token, we cannot decide if it contains CFn intrinsics or not without resolving the token, which is not ideal for lazy tokens. In that case, we just use CfnParameter, and letting a user to force dynamic reference by passing a flag.

I made the change to only fromStringParameterAttributes, not fromListParameterAttributes. I think we can do this in another PR.

Pull request has been modified.

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Pull request has been modified.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: a479f2f
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).