Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(aws-iam): StringParameter.value_from_lookup's dummy value did not suffice #8699

Closed
ThomasSteinbach opened this issue Jun 23, 2020 · 22 comments · Fixed by #21520
Closed

(aws-iam): StringParameter.value_from_lookup's dummy value did not suffice #8699

ThomasSteinbach opened this issue Jun 23, 2020 · 22 comments · Fixed by #21520
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management bug This issue is a bug. effort/small Small work item – less than a day of effort good first issue Related to contributions. See CONTRIBUTING.md p1

Comments

@ThomasSteinbach
Copy link
Contributor

ThomasSteinbach commented Jun 23, 2020

aws_iam.StringParameter.value_from_lookup(...)

returns a dummy-value-for-${parameterName} during synthesis (from #3654). This value did not suffice for use as ARN. The dummy value itself should represent a dummy ARN pattern to avoid errors.

Reproduction Steps

Here is a short (and stripped) example, which currently leads to an error:

aws_kms.Key.from_key_arn(
    self,
    id,
    key_arn=aws_ssm.StringParameter.value_from_lookup(
        self,
        parameter_name="/example/param",
    ),
)

Error Log

During synthesis this leads to an error:

jsii.errors.JSIIError: ARNs must have at least 6 components: dummy-value-for-/example/param

Workaround

_param = aws_ssm.StringParameter.value_from_lookup(self, parameter_name="/example/param")

if "dummy-value" in _param:
    _param = "arn:aws:service:eu-central-1:123456789012:entity/dummy-value"

aws_kms.Key.from_key_arn(
    self,
    id,
    key_arn=_param,
)

Solution Proposal

Instead of dummy-value-for-${parameterName} the method should return something like arn:aws:service:eu-central-1:123456789012:entity/dummy-value

This solution would also address/solve #7051


This is 🐛 Bug Report

@ThomasSteinbach ThomasSteinbach added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 23, 2020
@github-actions github-actions bot added the @aws-cdk/aws-iam Related to AWS Identity and Access Management label Jun 23, 2020
ThomasSteinbach added a commit to ThomasSteinbach/aws-cdk that referenced this issue Jun 23, 2020
This is my proposed solution for aws#8699 which also works with resources requiring an ARN. The solution works independently of the parameter value.
@ThomasSteinbach
Copy link
Contributor Author

ThomasSteinbach commented Jun 24, 2020

Hi @eladb , is looks like you have created the original implementation of the "dummy-value" for ssm-StringParameters. If so, could you please have a look on my proposed solution code (#8700 ) for this bug report and "do it right", as I am no TypeScript programmer? Thank you :)

@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Jun 25, 2020
@MrArnoldPalmer
Copy link
Contributor

@ThomasSteinbach I'm curious to know about the use case that is breaking with this right now. Can you provide details about what constructs/processes you're using that aren't supported currently?

@SomayaB SomayaB added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed needs-triage This issue or PR still needs to be triaged. labels Jun 29, 2020
@github-actions
Copy link

github-actions bot commented Jul 7, 2020

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jul 7, 2020
@rix0rrr rix0rrr added p1 good first issue Related to contributions. See CONTRIBUTING.md and removed closing-soon This issue will automatically close in 4 days unless further comments are made. in-progress This issue is being actively worked on. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Jul 8, 2020
@rix0rrr
Copy link
Contributor

rix0rrr commented Jul 8, 2020

The dummy return value should probably be a Token, this should suppress literal string validation in most of the CDK.

@ThomasSteinbach
Copy link
Contributor Author

Sorry @MrArnoldPalmer for the late response. (I spent my last 14d in holidays). Regarding your questions:

I'm curious to know about the use case that is breaking with this right now.

The Reproduction Steps in the issues description will give you one use case ( 'aws_kms.Key` can't handle the current dummy values).

Without testing it, in my description I also statet that the solution, provided with this issue ( #8700 ), may resolve #7051 , which then is also a use case, as aws_s3.Bucket.from_bucket_arn will raise the exact same error on the current dummy value.

Can you provide details about what constructs/processes you're using that aren't supported currently?

As shown in the descriptions example, aws_kms.Key.from_key_arn() would not support the dummy-value-for-${parameterName} returned by aws_ssm.StringParameter.value_from_lookup() but would support arn:aws:service:eu-central-1:123456789012:entity/dummy-value as proposed in my solution https://github.com/aws/aws-cdk/pull/8700/files.

The proposed solution should work for all other resource methods requiring an ARN, like aws_s3.Bucket.from_bucket_arn.

We've tested workaround (for python) given in the description, and it worked well for us under every circumstances. The proposed solution would fix the issue natively in TypeScript. However a professional TypeScript programmer should review and "cleanup" the solution.

@MrArnoldPalmer
Copy link
Contributor

@ThomasSteinbach thanks, I didn't derive all those details from the original post.

Since ssm params can obviously contain stuff other than Arns, and validations may be anything really, a more generic solution is required I believe. As @rix0rrr mentioned, we can possibly change the dummy value return type to be a Token, which makes sense since Tokens are stand ins for values that will resolve at some point in the future.

@ThomasSteinbach
Copy link
Contributor Author

As long as it works, I am totally fine with any solution 😃

As Python-CDK user I feel not able to provide a clean solution for replacing the current dummy values by tokens. Could you please provide one or delegate it to a CDK TypeScript programmer? I am confident, that it should be no extravagant expense but would solve this as well as this #7051 issue.

Thanks in advance

@zxkane
Copy link
Contributor

zxkane commented Sep 18, 2020

Another use case,

If putting the json string into SSM parameter store for externalizing the parameters of CDK app, the valueFromLookup always immediately returns the default value that breaks the JSON parsing.

@jishi
Copy link

jishi commented Nov 4, 2020

I'm getting this problem when trying to "import" an SNS topic via ARN that I get from parameter store:

const cachePurgeTopicArn = StringParameter.valueFromLookup(
      this, '/infrastructure/topic/cache-purge');
const cachePurgeTopic = Topic.fromTopicArn(this, 'CachePurgeTopic', cachePurgeTopicArn);

Meaning, I can't synthesize this to validate it. This works fine with importing VPCs, ALB listeners, ECS cluster ecs and actually synthesize to the actual parameter store value, so I'm not sure why this becomes a dummy value in this example?

EDIT: OH wait, it only fails on first run? If I run the lookup first, then add the .fromTopicArn, it works? Because of resource caching I guess, that's even weirder...

@jishi
Copy link

jishi commented Nov 26, 2020

This actually fails for deploy runs as well, not only synthesizing. Making it somewhat cumbersome to store ARNs on SSM without having to jump through hoops to create the cdk context first without actually using the values.

@ilko-rbi
Copy link

and another one:

asm.Secret.from_secret_attributes(
        scope=scope, id=secret_arn, secret_complete_arn=secret_arn, encryption_key=key
    )

leads to

 `secretCompleteArn` does not appear to be complete; missing 6-character suffix

CDK: 1.85.0

@infa-rasrinivasan
Copy link

With the following lines of code I am getting a similar error with 1.85.0. Is this a know bug ? If yes will be fixed in the version.
iam.Role.from_role_arn(self, id="SourceCodeRole", role_arn = source_role_arn, mutable = False)

Error: jsii.errors.JSIIError: ARNs must start with "arn:" and have at least 6 components: ""

@mspolitaev
Copy link

Hi team! Still planned to be fixed or waiting for better times?)

@iwt-kschoenrock
Copy link

I'm trying to load a JSON string from ParameterStore (StringParameter.valueFromLookup) and getting bit by this (I'm using CDK 1.128.0). cdk synth fails without updating the context if I try to parse the JSON, but works if I skip the parsing.

@adam-phillipps
Copy link

This is still happening to me using 1.132.0. I'm trying to do a Function.from_function_arn using an SSM lookup.
Basically hard coding ARNs is not my favorite way to go. Any idea if we can expect a solution or non-hard code solution to this?

@gshpychka
Copy link
Contributor

The workaround is to check whether the value contains "dummy_value_for*", and replacing it with your own dummy value that adheres to the required pattern.

After the first synth, the looked-up value will be written to cdk.context.json and subsequent synths will work as expected.

@adam-phillipps
Copy link

That's essentially a hard coding, imho.
We can do string interpolation for most parts of the ARN but the name of a function is still needed so we have to create contracts in our code and/or manually encode at least some part of the ARN e.g.

f"arn:aws:lambda:{env.region}:{env.account}:function:{function_name}"

After that, we still have to add in a ternary or something to account for the problematic dummy-value-for-... format.

The workaround does work but I'm looking forward to that feature that "fixes" this one 👍
Any idea how far down the priorities list this is or an idea if/when it might see some love?

@sdhuang32
Copy link

sdhuang32 commented Nov 23, 2021

I ran into this too, and found a bit more elegant solution recently, which was to use Lazy values.
To be specific, the following example I use the Lazy.string() of the CDK core module, which encodes your variable as a token and defer the calculation of the string value to synthesis time.

import * as cdk from '@aws-cdk/core';
import * as ssm from '@aws-cdk/aws-ssm';
import * as iam from '@aws-cdk/aws-iam';

const roleArn = ssm.StringParameter.valueFromLookup(this, "/param/testRoleArn");
const role = iam.Role.fromRoleArn(this, "role", cdk.Lazy.string({ produce: () => roleArn }));

It's more general because it can apply to many other use cases besides fromXXXArn() methods, and is in fact used in many places of internal CDK source to get values rendered at synthesis time, as long as the method you throw a lazy value to can handle tokens.

The following is an example that shows why I don't think making ssm.StringParameter.valueFromLookup() return a token is a good idea.

When importing a VPC using ec2.Vpc.fromLookup(), just give the return value of the
ssm.StringParameter.valueFromLookup() and it works.

import * as cdk from '@aws-cdk/core';
import * as ssm from '@aws-cdk/aws-ssm';
import * as ec2 from '@aws-cdk/aws-ec2';

const vpcId = ssm.StringParameter.valueFromLookup(this, "/param/vpcId");
const vpc = ec2.Vpc.fromLookup(this, "vpc", {
  vpcId: vpcId
});

This is because ec2.Vpc.fromLookup() does not check the format of vpcId input parameter,
so it won't cause the construction phase to fail.

However, this method demands your input vpcId to be a concrete string and not a token, so the statement
vpcId: cdk.Lazy.string({produce: () => vpcId }) will result in the error
All arguments to Vpc.fromLookup() must be concrete (no Tokens).
You never know how the callee will deal with the input parameter.

You can store any kind of strings, for different purposes in a SSM parameter,
so it's hard to demand a universal fix for every use cases from inside the valueFromLookup() method,
unless (the ideal fix in my mind) the CDK team does a big re-design and make it fetch the parameters at construction time.

Seems to me at the moment the best way is to understand your use case and have a look at the source code of the method
you're gonna pass your parameter to, and decide what approach you need to pre-process your parameter.

Anyone interested can have a look at my complete analysis.

@gshpychka
Copy link
Contributor

@sdhuang32 thanks for this approach, this is very helpful. Although I often use valueFromLookup precisely in places that don't accept a token, like you mentioned, so this wouldn't work in those cases (and neither would having the lookup return a token).

@goosefraba
Copy link

any update on this issue?
Still getting the error when trying to load a SNS topic ARN from SSM parameter store and then do a Topic.fromTopicArn(...)

@zxkane
Copy link
Contributor

zxkane commented Mar 8, 2022

I suffered this issue when synthesizing cdk application in ec2 with IAM role(using AWS_DEFAULT_PROFILE to another aws account).

Synthesizing same app in local env using AK/SK works, then updated the context.json in ec2 env as workaround.

@mergify mergify bot closed this as completed in #21520 Aug 9, 2022
mergify bot pushed a commit that referenced this issue Aug 9, 2022
…mats (#21520)

`StringParameter.valueFromLookup` can be used to retrieve a string
value, but that value could be in any format and there are times where
it is provided as input to properties that require a specific format
(i.e. arn format). Because of the way that lookups are resolved, it is
possible for the initial value to be the dummy value
`dummy-value-for-${parameterName}` which might cause synth errors.

Since there is no way for the CDK to know _how_ you will use the
returned value, we can't really add logic to specifically handle edge
cases. For example, we could have `valueFromLookup` always return a
token, but then it would no longer be able to be used in cases where a
`string` is required. See #8699 (comment)
for a good analysis.

This PR adds documentation instructing users how to handle these use
cases.

closes #8699, #9138


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

github-actions bot commented Aug 9, 2022

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

josephedward pushed a commit to josephedward/aws-cdk that referenced this issue Aug 30, 2022
…mats (aws#21520)

`StringParameter.valueFromLookup` can be used to retrieve a string
value, but that value could be in any format and there are times where
it is provided as input to properties that require a specific format
(i.e. arn format). Because of the way that lookups are resolved, it is
possible for the initial value to be the dummy value
`dummy-value-for-${parameterName}` which might cause synth errors.

Since there is no way for the CDK to know _how_ you will use the
returned value, we can't really add logic to specifically handle edge
cases. For example, we could have `valueFromLookup` always return a
token, but then it would no longer be able to be used in cases where a
`string` is required. See aws#8699 (comment)
for a good analysis.

This PR adds documentation instructing users how to handle these use
cases.

closes aws#8699, aws#9138


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management bug This issue is a bug. effort/small Small work item – less than a day of effort good first issue Related to contributions. See CONTRIBUTING.md p1
Projects
None yet