feat: make imported resources account/region-aware #8280

skinny85 · 2020-05-29T22:52:30Z

Add the account and region properties to the IResource interface and Resource class.
By default, these are equal to the account and region of the Stack the resource belongs to;
however, they can be set to different values in resources that are imported.

Use those new properties in two places:

In CodePipeline, to determine whether a given action is cross-account
(with support for specifying the account and region in S3's BucketAttributes,
as a first use case).
IAM's addToPrincipalOrResource(), to correctly know when to modify the receiver's resource policy.
This is aided by adding an optional principalAccount property to IPrincipal,
as a way to compare to the account present in the passed IResource instance.

Fixes #2807
Fixes #5740
Fixes #7012

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

eladb

Initial round.

There are quite a few unrelated changes such as spacing and stuff. Ideally those should not be included.

@rix0rrr please review as well.

packages/@aws-cdk/aws-ecs-patterns/test/fargate/test.load-balanced-fargate-service.ts

packages/@aws-cdk/aws-iam/lib/grant.ts

eladb · 2020-06-02T06:27:55Z

packages/@aws-cdk/aws-iam/lib/account-utils.ts

+  const secondIsUnresolved = cdk.Token.isUnresolved(account2);
+
+  if (firstIsUnresolved && secondIsUnresolved) {
+    return AccountCompare.SAME;


This assumes that if account is unresolved it will definitely resolve to the pseudo reference AWS::Account but it could also be a lazy value for example.

Maybe we should be a bit smarter about this and allow you to “peek” into the token and only consider them “same” if they both resolve to the pseudo.

You're correct, but do you have an idea how we can do this "peeking"?

packages/@aws-cdk/aws-route53/test/test.util.ts

packages/@aws-cdk/core/lib/arn.ts

eladb · 2020-06-02T06:38:44Z

packages/@aws-cdk/core/lib/arn.ts

@@ -135,12 +135,16 @@ export class Arn {
   *      components of the ARN.
   */
  public static parse(arn: string, sepIfToken: string = '/', hasName: boolean = true): ArnComponents {
-    if (Token.isUnresolved(arn)) {
+    const components = arn.split(':') as Array<string | undefined>;


I would only do that if the string starts with arn:

Why? It's not likely to throw.

eladb · 2020-06-02T06:41:03Z

packages/@aws-cdk/core/lib/resource.ts

+   * The AWS account ID that this resource belongs to.
+   * For resources that are created and managed by the CDK
+   * (generally, those created by creating new class instances like Role, Bucket, etc.)
+   * this is always the same as the account of the stack they belong to;


So it’s not “always the same”. Phrasing is a bit awkward

For the created ones, it is always the same.

But of course, if you have a better wording, I'm all ears!

rix0rrr

I like it! Thanks for improving my half-baked work :)

packages/@aws-cdk/aws-certificatemanager/test/test.util.ts

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts

rix0rrr · 2020-06-03T13:58:35Z

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts

        // if the resource is from a different stack in another region but the same account,
        // use that stack as home for the cross-region support resources
-        if (pipelineStack.account === actionResourceStack.account) {
+        if (pipelineStack.account === actionResourceStack.account &&


I believe Aws.REGION does always resolve to the same object, so I think we're good here. Let me know if you disagree.

I don't necessarily disagree, I'm just scared.

packages/@aws-cdk/aws-ecs-patterns/test/fargate/integ.load-balanced-fargate-service.ts

packages/@aws-cdk/aws-ecs-patterns/test/fargate/test.load-balanced-fargate-service.ts

packages/@aws-cdk/aws-iam/lib/role.ts

packages/@aws-cdk/aws-s3/lib/bucket.ts

rix0rrr · 2020-06-03T14:12:15Z

packages/@aws-cdk/aws-s3/lib/bucket.ts

-    const crossAccountAccess = this.isGranteeFromAnotherAccount(grantee);
-    let ret: iam.Grant;
-    if (crossAccountAccess) {
-      // if the access is cross-account, we need to trust the accessing principal in the bucket's policy
-      ret = iam.Grant.addToPrincipalAndResource({
-        grantee,
-        actions: bucketActions,
-        resourceArns: resources,
-        resource: this,
-      });
-    } else {
-      // if not, we don't need to modify the resource policy if the grantee is an identity principal
-      ret = iam.Grant.addToPrincipalOrResource({
-        grantee,
-        actions: bucketActions,
-        resourceArns: resources,
-        resource: this,
-      });
-    }


Woohoo!! \o/

rix0rrr · 2020-06-03T14:14:30Z

packages/@aws-cdk/core/lib/arn.ts

@@ -135,12 +135,16 @@ export class Arn {
   *      components of the ARN.
   */
  public static parse(arn: string, sepIfToken: string = '/', hasName: boolean = true): ArnComponents {
-    if (Token.isUnresolved(arn)) {
+    const components = arn.split(':') as Array<string | undefined>;


Why? It's not likely to throw.

packages/@aws-cdk/core/lib/cfn-pseudo.ts

rix0rrr · 2020-06-05T11:07:23Z

As discussed face-to-face, I think .account and .region members on IResource take up valuable autocomplete real estate for very rare use cases (<1% of CDK users would need this, and I'm guessing even <10% of construct library authors).

Maybe move to a static method instead?

(The comparison logic that takes unresolveds into account can go there as well)

eladb · 2020-06-07T06:00:38Z

As discussed face-to-face, I think .account and .region members on IResource take up valuable autocomplete real estate for very rare use cases (<1% of CDK users would need this, and I'm guessing even <10% of construct library authors).

Maybe move to a static method instead?

(The comparison logic that takes unresolveds into account can go there as well)

I personally don’t have much of an issue with adding those to resource directly. Those are a natural attribute of a resource so I don’t think a “mixin” is required. Let’s keep things simple and discoverable.

rix0rrr · 2020-06-08T16:07:15Z

It's not a "mixin", and why should we optimize for the 0.01% use case and pessimize the rest?

I'm arguing it doesn't need to be discoverable, because hardly anyone will use it.

eladb · 2020-06-08T20:00:14Z

It's not a "mixin", and why should we optimize for the 0.01% use case and pessimize the rest?

I'm arguing it doesn't need to be discoverable, because hardly anyone will use it.

You can make a similar argument for many APIs we expose on many classes. I disagree that this pessimizes the common use case. It adds an api to Resource which makes sense in the context of every AWS resource. A good way to “measure” this is to ask whether these properties might conflict or contradict with something that comes from a subclass. I think the answer is no. I can’t think of an AWS resource that will have other meanings for account or region because those concepts come from the same domain (unlike, for example, the APIs we have under ConstructNode, which come from a different domain).

skinny85 · 2020-06-08T20:46:43Z

@eladb what is your opinion on account and region in (I)Resource returning an instance of a new class, instead of just simple Strings, that has some logic for intelligent comparisons of the values, including tokens, AWS::Region/AWS::AccountId, etc.? So we don't have to repeat the logic that's currently in IAM, in the accountsAreSameOrUnresolved function?

eladb · 2020-06-09T05:10:18Z

@eladb what is your opinion on account and region in (I)Resource returning an instance of a new class, instead of just simple Strings, that has some logic for intelligent comparisons of the values, including tokens, AWS::Region/AWS::AccountId, etc.? So we don't have to repeat the logic that's currently in IAM, in the accountsAreSameOrUnresolved function?

I like it. Then maybe we should encapsulate those into Env and also expose a similar object from Stack. I would also add a partition there.

skinny85 · 2020-07-01T01:54:45Z

@eladb @rix0rrr I've included your comments, and added the classes we talked about in #8280 (comment) , so this is ready for another round of reviews!

packages/@aws-cdk/core/lib/resource.ts

packages/@aws-cdk/core/lib/environment-component.ts

packages/@aws-cdk/aws-iam/lib/grant.ts

rix0rrr · 2020-07-07T07:49:42Z

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts

        // if the resource is from a different stack in another region but the same account,
        // use that stack as home for the cross-region support resources
-        if (pipelineStack.account === actionResourceStack.account) {
+        if (pipelineStack.account === actionResourceStack.account &&


I don't necessarily disagree, I'm just scared.

mergify · 2020-08-08T03:19:23Z

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

skinny85 · 2020-08-08T03:19:46Z

@rix0rrr included your comments. This is ready for another round of reviews!

rix0rrr

I think we might have a different bug.

This concerns me: #8280 (comment)

skinny85 · 2020-08-13T00:58:54Z

@rix0rrr I need your help with @aws-cdk/pipelines changes. I think the problem is here:

aws-cdk/packages/@aws-cdk/pipelines/lib/actions/deploy-cdk-stack-action.ts

Lines 312 to 316 in 0ea4ea3

    
           function cfnExpressionFromManifestString(s: string) { 
        
             // This implementation relies on the fact that the manifest placeholders are 
        
             // '${AWS::Partition}' etc., and so are the same values as those that are 
        
             // trivially substituable using a `Fn.sub`. 
        
             return Fn.sub(s);

. Because you're wrapping an entire ARN we use to import the role with into an Fn::Sub, our logic cannot take out the account from the ARN (as it's a Token), and thus see that they're in fact in the same account. Hence, there is a diff in the integ tests (we modify the resource policy of the artifact bucket).

Would appreciate any ideas on how to tackle that.

skinny85 · 2020-08-13T01:17:16Z

Weird failure:

@aws-cdk/aws-apigateway: error: [awslint:attribute-tag:@aws-cdk/aws-apigateway.ProxyResource.resourceEnv] attribute properties must have an "@attribute" doctag on:  @aws-cdk/core.IResource.resourceEnv
@aws-cdk/aws-apigateway: error: [awslint:attribute-tag:@aws-cdk/aws-apigateway.Resource.resourceEnv] attribute properties must have an "@attribute" doctag on:  @aws-cdk/core.IResource.resourceEnv

I didn't even touch anything in api-gateway...

skinny85 · 2020-08-13T01:18:15Z

Oh, I know what this is. The name resource was picked up the linter, as it's the name of the API Gateway Resource resource.

rix0rrr · 2020-08-13T16:14:03Z

Instead of wrapping in Fn.sub the only solution then is to use cxapi.EnvironmentalPlaceholders.sub (or whatever it's called) to replace the placeholder strings with CDK tokens, so that we end up with a :-delimited string.

rix0rrr · 2020-08-14T07:40:16Z

packages/@aws-cdk/pipelines/test/integ.pipeline.ts

@@ -55,7 +55,9 @@ class CdkpipelinesDemoPipelineStack extends Stack {

    // This is where we add the application stages
    // ...
-    const stage = pipeline.addApplicationStage(new MyStage(this, 'PreProd'));
+    const stage = pipeline.addApplicationStage(new MyStage(this, 'PreProd', {
+      env: { account: process.env.CDK_DEFAULT_ACCOUNT, region: process.env.CDK_DEFAULT_REGION },


Hm. Why did this work in the first place?

skinny85 · 2020-08-14T18:12:02Z

@eladb this is ready to be merged. Let me know if you have any comments on the current state of the code!

packages/@aws-cdk/aws-codepipeline-actions/README.md

packages/@aws-cdk/aws-ec2/lib/bastion-host.ts

packages/@aws-cdk/core/lib/resource.ts

eladb · 2020-08-17T17:49:37Z

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

+ * An enum-like class that represents the result of comparing two Tokens.
+ * The return type of {@link Token.compareStrings}.
+ */
+export class TokenComparison {


Why isn't this just a simple enum?

Because it allows us to add helper methods to it later, like isEqualOrUnresolved() that were here before, which I still think is the correct way to model this.

If we make this an enum, we will lose that flexibility.

Feels like "backwards-future-compatibility". Not sure I understand the use case of isEqualOfUnresolved(), can you elaborate or show an example?

I mean in code like this:

const thisAndPolicyAccountComparison = Token.compareStrings(this.resourceEnv.account, policy.resourceEnv.account); const equalOrAnyUnresolved = thisAndPolicyAccountComparison === TokenComparison.SAME || thisAndPolicyAccountComparison === TokenComparison.BOTH_UNRESOLVED || thisAndPolicyAccountComparison === TokenComparison.ONE_UNRESOLVED; if (equalOrAnyUnresolved) { // ...

I think a much more natural way to write that is:

if (Token.compareStrings(this.resourceEnv.account, policy.resourceEnv.account) .isEqualOrAnyUnresolved()) { // ...

I don't want us to block ourselves from achieving the latter by using an enum.

Feels a bit non idiomatic (fluent and such).

I would have done this:

Token.isEqualOrAnyUnresolved(this.resourceEnv.account, policy.resourceEnv.account)

But if you feel strongly, I am fine...

aws-cdk-automation · 2020-08-19T21:45:26Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 041b808
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-08-19T21:45:49Z

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

#8280 enabled imported resources to be account & region aware. However, while this set the region on the object itself, it didn't adjust the various region-aware properties of imported buckets (e.g., regional domain names). This change makes the regional properties of the imported bucket use the correct region. fixes #9556 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

In #8280 we made a resource's account/region distinct from the stack in which the construct was defined, to account for accounts and regions from imported resources. The pipelines module used to define imported roles in a separate in-memory Stack so that the old, broken "cross-environment" logic would do the right thing. That crutch was removed as part of #8280. The new logic hasn't been carried through everywhere though. For example, the logic in the grants of KMS keys had not been updated to match, leading to cross-account/cross-region deployments being broken (as reported in #10166) because the cross-region support stack's KMS key had the wrong permissions. In fact, it switched from: ``` { "Action": [ "kms:Decrypt", "kms:DescribeKey" ], "Principal": { "AWS": { "Fn::Sub": "arn:${AWS::Partition}:iam::561462023695:role/cdk-hnb659fds-deploy-role-561462023695-us-east-2" } }, "Resource": "*" } ``` to ``` { "Action": [ "kms:Decrypt", "kms:DescribeKey" ], "Principal": { "AWS": { "Fn::Join": [ "", [ "arn:", { "Ref": "AWS::Partition" }, ":iam::355421412380:root" ] ] } }, "Resource": "*" } ``` Ignoring the switch from `Fn::Sub` to `Fn::Join`, it switched from the `deploy-role` in a DIFFERENT account to the root principal of the SAME account.

skinny85 requested review from rix0rrr and eladb May 29, 2020 22:52

mergify bot added the contribution/core This is a PR that came from AWS. label May 29, 2020

skinny85 mentioned this pull request May 29, 2020

fix(iam): no trust policy added for cross-account roles #5812

Closed

SomayaB assigned skinny85 Jun 1, 2020

skinny85 force-pushed the feat/region-account-in-resources branch from ce52185 to 40f16e9 Compare June 1, 2020 18:24

eladb reviewed Jun 2, 2020

View reviewed changes

rix0rrr reviewed Jun 3, 2020

View reviewed changes

skinny85 mentioned this pull request Jun 23, 2020

Cross Account resource import and source account tracking aws/aws-cdk-rfcs#85

Closed

2 tasks

skinny85 added the pr-linter/exempt-readme The PR linter will not require README changes label Jun 29, 2020

skinny85 force-pushed the feat/region-account-in-resources branch from 40f16e9 to 226111d Compare July 1, 2020 01:53

skinny85 requested review from rix0rrr and eladb July 1, 2020 01:54

skinny85 removed the pr-linter/exempt-readme The PR linter will not require README changes label Jul 3, 2020

rix0rrr requested changes Jul 7, 2020

View reviewed changes

skinny85 requested a review from rix0rrr August 8, 2020 03:18

skinny85 force-pushed the feat/region-account-in-resources branch from 226111d to e44dc18 Compare August 8, 2020 03:18

skinny85 added pr-linter/exempt-readme The PR linter will not require README changes and removed pr-linter/exempt-readme The PR linter will not require README changes labels Aug 8, 2020

rix0rrr requested changes Aug 10, 2020

View reviewed changes

rix0rrr mentioned this pull request Aug 10, 2020

feat(ec2): L2 Construct - VpcPeeringConnection #9339

Closed

skinny85 requested a review from rix0rrr August 13, 2020 00:56

skinny85 force-pushed the feat/region-account-in-resources branch 3 times, most recently from cd20abb to 4846304 Compare August 14, 2020 04:22

rix0rrr reviewed Aug 14, 2020

View reviewed changes

rix0rrr changed the title ~~feat: add account and region to (I)Resource~~ feat: make imported resources account/region-aware Aug 14, 2020

rix0rrr approved these changes Aug 14, 2020

View reviewed changes

skinny85 force-pushed the feat/region-account-in-resources branch from 4846304 to b876163 Compare August 14, 2020 18:04

skinny85 mentioned this pull request Aug 17, 2020

Region config for S3DeployAction #7012

Closed

2 tasks

eladb reviewed Aug 17, 2020

View reviewed changes

eladb approved these changes Aug 17, 2020

View reviewed changes

skinny85 force-pushed the feat/region-account-in-resources branch from b876163 to b95828f Compare August 19, 2020 21:03

skinny85 removed the pr/do-not-merge This PR should not be merged at this time. label Aug 19, 2020

feat: add account and region to (I)Resource

041b808

skinny85 force-pushed the feat/region-account-in-resources branch from b95828f to 041b808 Compare August 19, 2020 21:21

mergify bot merged commit d6278b3 into aws:master Aug 19, 2020

skinny85 deleted the feat/region-account-in-resources branch August 19, 2020 21:45

njlynch mentioned this pull request Aug 24, 2020

feat(s3): imported buckets can have an explicit region #9936

Merged

ddneilson mentioned this pull request Aug 28, 2020

chore(deps): Bulk update of all dependencies aws/aws-rfdk#104

Merged

rix0rrr mentioned this pull request Sep 7, 2020

fix(pipelines): cross-region/cross-account key permissions are wrong #10222

Closed

idm-ryou mentioned this pull request Sep 8, 2020

[aws-ecs] TaskDefinition that uses cross account ECR repository image throws an error #10233

Closed

feat: make imported resources account/region-aware #8280

feat: make imported resources account/region-aware #8280

Conversation

skinny85 commented May 29, 2020 • edited

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr commented Jun 5, 2020

eladb commented Jun 7, 2020

rix0rrr commented Jun 8, 2020

eladb commented Jun 8, 2020

skinny85 commented Jun 8, 2020

eladb commented Jun 9, 2020

skinny85 commented Jul 1, 2020

Choose a reason for hiding this comment

mergify bot commented Aug 8, 2020

skinny85 commented Aug 8, 2020

rix0rrr left a comment • edited

Choose a reason for hiding this comment

skinny85 commented Aug 13, 2020

skinny85 commented Aug 13, 2020

skinny85 commented Aug 13, 2020

rix0rrr commented Aug 13, 2020

Choose a reason for hiding this comment

skinny85 commented Aug 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb Aug 17, 2020 • edited

Choose a reason for hiding this comment

aws-cdk-automation commented Aug 19, 2020

AWS CodeBuild CI Report

mergify bot commented Aug 19, 2020

skinny85 commented May 29, 2020 •

edited

rix0rrr left a comment •

edited

eladb Aug 17, 2020 •

edited