-
Notifications
You must be signed in to change notification settings - Fork 4.4k
fix(core): arnForXxxx() helpers ignore environments from referenced resources
#36599
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
Conversation
All `CfnXxxx` classes have a helper to return the ARN for a given
resource, given an `IXxxRef`. For example:
```ts
declare const bucket: IBucketRef;
console.log(CfnBucket.arnForBucket(bucket));
```
This will use the already-existing ARN if available (as a CloudFormation
attribute), but may also construct a fresh ARN from the ARN components
if they are not available.
In the second case, this would always use the Stack's account and
region, which might be incorrect in case of a resource referenced
by ARN. The following can happen:
```ts
const stack = new Stack(..., { account: '11111' });
const resource = Resource.fromResourceArn(stack, ..., '...:222222:...');
console.log(CfnResource.arnForResource(resource)); // Should return 22222 not 11111
```
This PR fixes that.
arnForXxxx() helpers always use Stack environmentarnForXxxx() helpers ignore environments from referenced resources
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This review is outdated)
| Region: stackOfResource.prop('region'), | ||
| Account: stackOfResource.prop('account'), | ||
| ...mapValues(this.decider.resourceReference.arnVariables!, (propName) => $E(resourceIdentifier)[refAttributeName][propName]), | ||
| Partition: CDK_CORE.Aws.PARTITION, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Stack has some fancy logic around the partition. Is there any particular reason we need to bypass this logic now?
aws-cdk/packages/aws-cdk-lib/core/lib/stack.ts
Lines 727 to 737 in d43951a
| public get partition(): string { | |
| // Return a non-scoped partition intrinsic when the stack's region is | |
| // unresolved or unknown. Otherwise we will return the partition name as | |
| // a literal string. | |
| if (!FeatureFlags.of(this).isEnabled(cxapi.ENABLE_PARTITION_LITERALS) || Token.isUnresolved(this.region)) { | |
| return Aws.PARTITION; | |
| } else { | |
| const partition = RegionInfo.get(this.region).partition; | |
| return partition ?? Aws.PARTITION; | |
| } | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could use the same logic with resourceIdentifier.env.region instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an idea. I wasn't aware Stack had this prettification logic.
I guess we could rely on the Stack to fill in the partition, and it should be 99% equivalent or better.
Only additional requirement then is that the (potentially referenced) resource is not scoped underneath a Stack. Stack.of() would throw an exception in that case.
I'm not sure if there's an absolute need for it to be in a Stack, and I'd like to avoid constraining unnecessarily. Let's see what I can do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha -- you know what; I was about to write why the same logic isn't necessary for region -- and the reason is because we already require that referenced resources are imported in the context of a Stack. We can loosen that restriction in the future, maybe, but for now relying on Stacks is not making the situation worse. So we can do the partition thing as well.
As for why it's not necessary for region: the resource's region will be default-filled from the containing Stack's region already, so whatever logic applies to Stack applies to region as well.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
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). |
Merge Queue Status✅ The pull request has been merged at 67ffcef This pull request spent 39 minutes 46 seconds in the queue, including 39 minutes 36 seconds running CI. Required conditions to merge
|
|
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). |
|
Comments on closed issues and PRs are hard for our team to see. |
All
CfnXxxxclasses have a helper to return the ARN for a given resource, given anIXxxRef. For example:This will use the already-existing ARN if available (as a CloudFormation attribute), but may also construct a fresh ARN from the ARN components if they are not available.
In the second case, this would always use the Stack's account and region, which might be incorrect in case of a resource referenced by ARN. The following can happen:
This PR fixes that.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license