feat(pipelines): allow disabling of KMS keys #10396

rix0rrr · 2020-09-16T15:01:41Z

KMS keys for cross-account actions used to be created automatically,
but incur a $1/month charge for every region, adding a charge you
don't need if you don't plan to deploy in to cross-account destinations.

Add the option crossAccountKeys: false to allow users to switch off
the KMS keys and avoid the charge if they don't need it.

Relates to #10115.

Must not be merged before #10474.

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

KMS keys for cross-account actions used to be created automatically, but incur a $1/month charge for every region, adding a charge you don't need if you don't plan to deploy in to cross-account destinations. Add the option `crossAccountKeys: false` to allow users to switch off the KMS keys and avoid the charge if they don't need it. Relates to #10115.

skinny85

I'm a little concerned with the CodeBuild changes. How about moving them out to a separate PR?

skinny85 · 2020-09-18T00:21:58Z

packages/@aws-cdk/aws-codebuild/lib/project.ts

@@ -753,7 +753,9 @@ export class Project extends ProjectBase {
      environment: this.renderEnvironment(props.environment, environmentVariables),
      fileSystemLocations: this.renderFileSystemLocations(),
      // lazy, because we have a setter for it in setEncryptionKey
-      encryptionKey: Lazy.stringValue({ produce: () => this._encryptionKey && this._encryptionKey.keyArn }),
+      // The 'alias/aws/s3' default is necessary because leaving the `encryptionKey` field
+      // empty will not remove existing encryptionKeys during an update (ref. t/D17810523)


I'm a little worried about this change. Any we 100% certain an undefined key is the same as alias/aws/s3? Should we maybe wait for the service to fix it in their API?

How about extracting this CodeBuild change to a separate PR? It seems like we can merge the rest without this, no?

I don't think we can merge without this.

If you have an existing pipeline for which you want to switch off the keys, all codebuild projects in them break (because they'll keep on referring to the old keys).

That seems like an extremely sharp edge I'm not too keen on rolling out because people WILL break their pipelines this way.

Any we 100% certain an undefined key is the same as alias/aws/s3?

Like 90% :).

Should we maybe wait for the service to fix it in their API?

We can but that will realistically take a good while before it's addressed and rolled out everywhere.

Would be nice to put the explanation briefly inline here. No reason this should not be public.

I'm confused. I added a comment right there?

Yes, but it doesn't describe the part about CloudFormation Update behaviour causing this as nicely as it does in that ticket.

nija-at · 2020-09-21T13:01:21Z

packages/@aws-cdk/aws-codebuild/lib/project.ts

@@ -753,7 +753,9 @@ export class Project extends ProjectBase {
      environment: this.renderEnvironment(props.environment, environmentVariables),
      fileSystemLocations: this.renderFileSystemLocations(),
      // lazy, because we have a setter for it in setEncryptionKey
-      encryptionKey: Lazy.stringValue({ produce: () => this._encryptionKey && this._encryptionKey.keyArn }),
+      // The 'alias/aws/s3' default is necessary because leaving the `encryptionKey` field
+      // empty will not remove existing encryptionKeys during an update (ref. t/D17810523)


Would be nice to put the explanation briefly inline here. No reason this should not be public.

packages/@aws-cdk/aws-codebuild/lib/project.ts

nija-at · 2020-09-21T13:06:25Z

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

+To perform actions in a different account than your Pipeline is in, most
+actions accept a resource that is in a different account (either created


Sentence not clear, specifically what you mean by "accept a resource". What kind of resource?

Rephrase.

Please suggest something better. It can be a Bucket, a CodeDeploy DeploymentGroup, an ECS Cluster. What is a good enough generic term that is not too generic in your opinion?

How about something like -

"Pipelines can execute Actions in a different account from where the pipeline is deployed. In some cases, such as s3 bucket, the account id can be specified on the destination resource itself. In other cases, the action takes an optional account property where this can be specified."

nija-at · 2020-09-21T13:11:43Z

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

+CodePipeline requires that an IAM Role exists in the target account with a
+well-known name. The `Pipeline` construct automatically defines a **support
+stack** for you, named `<PipelineStackName>-support-<account>`, that will
+provision a role that the pipeline will assume in the given account before
+executing this action. This support stack will automatically be deployed
+before the stack containing the pipeline.


It would be nice to start the paragraph with something like this. And move what's above this paragraph after.
I don't mean exactly as is (of course), but content.

More generically - start by stating the feature, then how to use it and then how it's implemented. The second and third can be slightly interleaved if it makes for easier reading.

I'm having a hard time describing what the feature is without describing the context in which the feature operates (which is that we need a role and we either create one for you or you specify one).

"Pipelines can define Actions that are performed in a different account. In order to execute this action in the target account, the Pipeline construct automatically defines an IAM Role for you in the target account which the pipeline will assume to perform that action...."

Why not start like this? Then say,

"Actions that can be deployed in a different account take the accountId as an option. S3DeployAction and CodeBuildAction are two such examples:
"

nija-at · 2020-09-21T13:12:10Z

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

+You can also explicitly pass a `role` when creating the action. In that case,
+the `account` property is ignored, and the action will operate in the same


Should error instead.

I'm scared to break people

nija-at · 2020-09-21T13:13:17Z

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

 ### Cross-region CodePipelines

-You can also use the cross-region feature to deploy resources
-into a different region than your Pipeline is in.
+To perform actions in a different region than your Pipeline is in, most
+actions accept a resource that is in a different region (either created
+or imported):
+
+```typescript
+stage.addAction(new codepipeline_actions.S3DeployAction({
+  bucket: s3.Bucket.fromBucketAttributes(this, 'Bucket', {
+    region: 'us-west-1',
+    // ...
+  }),


Repeating content.

? Isn't the difference cross-account and cross-region?

Oops, missed that. Feels like this can be merged into the same section. And change the phrasing to 'account or region or both'.

Up to you.

nija-at · 2020-09-21T13:16:19Z

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

+ * Can't put these on Action themselves since we only have an interface
+ * and every library would need to reimplement everything (there is no
+ * `ActionBase`).


Can we introduce it now and add env() as its first member?
How many such instances are there?

Could have, but since we have a ticket for properly refactoring aws-codepipeline anyway to support CDKP and other extensions better in the future, I'd rather tackle it as part of that work.

nija-at · 2020-09-21T13:18:55Z

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

@@ -0,0 +1,36 @@
+import { Aws, ResourceEnvironment } from '@aws-cdk/core';


Isn't our standard to use lib/private/ for private classes. What's the _ prefix in the file name?

We have both (I started the lib/private convention, Elad prefers the _prefix.ts convention). At the moment my opinion is that one or a few files can have a _ prefix. If there grow to be too many, move to a /private/ folder.

Feels weird. We should standardize to one of the two.

+1 on moving this to private/ (there are a few different private files in this module already, maybe it makes sense to move those there as well).

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

…Build issue" These changes have been moved into a separate PR.

nija-at

LGTM overall. Would like the remaining comments addressed but not blocking.

Might be good for @skinny85 to take another look at the codepipeline related changes.

skinny85

Looks good, some minor comments.

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

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

packages/@aws-cdk/aws-codepipeline/lib/cross-region-support-stack.ts

skinny85 · 2020-09-23T23:13:17Z

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

+  /**
+   * The environment this action runs in
+   */
+  public get env(): ResourceEnvironment {


Have to say, I don't love this class. I feel like this would be better served as a helper function, or private method in Pipeline.

It is effectively a helper function, except modeled as a richer class instead of a static method. The following:

new EnhancedAction(action).env ActionHelpers.env(action)

Are effectively the same thing. Except the first feels "better" to me. It's more OO. It's like Rich Wrappers in Scala (except explicit instead of implicit).

I don't agree that a class whose only possible usage is new EnhancedAction(action).env is more OO. Classes need to have behavior, state, methods, to be considered good OO. This reminds me of the argument that instead of this:

interface Whatevs { value: string; }

, you should do this:

class Whatevs { private _value: string; public get value(): string { return this._value; } public set value(value: string): void { this._value = value; } }

because it's "more OO".

ActionHelpers.env(action)

I don't actually think this should be a static method. This should be a private method of the Pipeline class, which is the entity that is actually concerned with the algorithm used to determine this (as it's what determines the support Stack algorithm).

skinny85 · 2020-09-23T23:14:41Z

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

@@ -0,0 +1,36 @@
+import { Aws, ResourceEnvironment } from '@aws-cdk/core';


+1 on moving this to private/ (there are a few different private files in this module already, maybe it makes sense to move those there as well).

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

packages/@aws-cdk/aws-codepipeline/test/cross-env.test.ts

…o huijbers/cheap-pipelines

skinny85

A few more comments, I hope you'll humor me a little while longer 😉.

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

skinny85 · 2020-09-24T22:35:33Z

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

+    // `artifactBucket.grantRead(action.role)` (which is going to occur down the
+    // line) can even work.
+    const hasAccount = !Token.isUnresolved(actionEnv.account);
+    if (hasAccount && !sameEnvDimension(this.env.account, actionEnv.account) && !this.crossAccountKeys) {


One thing: I see we already do a check for this here, and it's more general, because it checks for the case when a customer provides their own Bucket (which this does not).

Do we really need this extra check here? Is there something missing from the existing check that we should change?

You're right, this is way better.

Aha it's actually not. If I just rely on that check, the tests for "cross-account by role" and "cross-account by resource" stop throwing the errror.

For the first case because if we use a Role we never get there: https://github.com/aws/aws-cdk/blob/huijbers/cheap-pipelines/packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts/#L642

The second case I haven't figured out yet.

But in fact this is also wrong, if we were to be COMPLETELY correct we should not be checking the this.artifactBucket, we should be checking the bucket in the same region -- which can be the artifactBucket but it can also be one of the replicationBuckets.

Which in fact suggests that the method that deals with the region is the better place to put this check?

That's all fine by me, let's correct this check instead of adding a new one - we have to tackle the case of a customer passing a Bucket without a Key anyway, might as well do it immediately. I can't imagine getting the correct Bucket (replication or main) for an action's region in that method that deals with accounts will be difficult, and I think having a nice separation between cross-region setup done in ensureReplicationResourcesExistFor and cross-account setup done in getRoleForAction is important (the code here is already complicated and difficult to follow).

No but it's silly, because the only function that properly returns the bucket is ensureReplicationResourcesExistFor, so that's the one we're going to have to use.

So now this is the call tree:

_attachActionToPipeline -> ensureReplicationResourcesExistFor (to get cross-region info) -> getRoleForAction (to get the role) -> getRoleFromActionPropsOrGenerateIfCrossAccount -> ensureReplicationResourcesExistFor (to get the cross-region bucket and check if it has a key)

I'll do it, but to me checking in ensureReplicationResourcesExistFor makes more sense. Mentally saying:

* ensureReplicationResourcesExistFor == only about region * getRoleForAction == only about account

Is wrong, especially since the method names indicate nothing about either region or account, they only talk about resources (and apparently both region and account have an impact on the replication bucket).

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

skinny85 · 2020-09-24T22:38:52Z

packages/@aws-cdk/aws-codepipeline/lib/private/rich-action.ts

+      account: (this.action.actionProperties.role?.env.account
+        ?? this.action.actionProperties?.resource?.env.account
+        ?? this.action.actionProperties.account
+        ?? Aws.ACCOUNT_ID),


Hmmm... is this actually a sane default? If the account is not know, I feel like it should be undefined here, which means "this is the same as the Pipeline account" (doesn't matter if the Pipeline account is AWS::AccountId, or a specific account ID).

It's a good point and I was in doubt about this initially, but I settled on this eventually.

The motivation here is that the cases aren't actually different, if you think about the end result: what ends up in the pipeline structure?

Even we defaulted to undefined here, the end result could still be Aws.ACCOUNT_ID (it could come from the Role, the Resource or the account property). So now we have two cases to deal with: undefined | Aws.ACCOUNT_ID.

Both of those cases render the same: they both end up rendering undefined into the template structure.

So why would we have the calling side do case analysis to fold those cases into each other? It feels like you're retaining fidelity and retaining information (from this side of the implementation), but I would argue it's fake fidelity. It's more cases that the caller has to deal with, that he's not actually interested in (i.e., more chances for bugs to sneak in).

In fact, I just feel that in principle there's no value to, when you ask an Action: "what is your account", for it to say "I don't know". Especially since the answer would be treated as "oh well then it's the same as my account" anyway. The only correct answer is "any account".

In my opinion, there is an edge case here, and it is a Pipeline in a Stack with a specific account/region, but an Action with AWS::AccountId / AWS::Region set as its account/region. In my opinion, that is incorrect, and we should error out in that case.

I'm not saying this validation needs to be done in this PR, but if we know there is a case that calls for it, this API should be capable of expressing it.

skinny85 · 2020-09-24T22:39:14Z

packages/@aws-cdk/aws-codepipeline/lib/private/rich-action.ts

+        ?? Aws.ACCOUNT_ID),
+      region: (this.action.actionProperties.resource?.env.region
+        ?? this.action.actionProperties.region
+        ?? Aws.REGION),


Same comment here as above for account (I don't think Aws.REGION is a good default; I think undefined should actually be the default here)

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

…sOrGenerateIfCrossAccount'

…lines

skinny85

@rix0rrr I've pushed a small refactoring of your code that makes the the RichAction class quite different. If you agree with this direction, feel free to merge this, if not, we can iterate further.

I have one last worry about this change, and that's these lines:

        encryption: encryptionKey ? s3.BucketEncryption.KMS : s3.BucketEncryption.KMS_MANAGED,

I just want to make 100% sure it's in line with the CodeBuild change for disabling Keys (I think it is, but just to double-check).

If that's correct, than LGTM!

aoropeza · 2020-09-28T19:06:44Z

Hi guys! I created multiple stacks and each one created one KMS, not to much money, but my client would like to save that dollars for something else. Do you have some timeline to merge this ?

skinny85 · 2020-09-28T19:12:11Z

Hi guys! I created multiple stacks and each one created one KMS, not to much money, but my client would like to save that dollars for something else. Do you have some timeline to merge this ?

Yes, it should be in the next release, which will be out this week 🙂.

mergify · 2020-09-29T12:17:58Z

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

aws-cdk-automation · 2020-09-29T12:18:12Z

AWS CodeBuild CI Report

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

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

rix0rrr requested review from skinny85, njlynch and a team September 16, 2020 15:01

rix0rrr self-assigned this Sep 16, 2020

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

Need to use alias/aws/s3 as default key to work around CodeBuild issue

84a3e31

skinny85 suggested changes Sep 18, 2020

View reviewed changes

rix0rrr requested a review from skinny85 September 21, 2020 08:13

nija-at suggested changes Sep 21, 2020

View reviewed changes

rix0rrr added 2 commits September 22, 2020 11:33

Revert "Need to use alias/aws/s3 as default key to work around Code…

845291f

…Build issue" These changes have been moved into a separate PR.

Update README

ef81751

rix0rrr requested a review from nija-at September 22, 2020 09:53

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Sep 22, 2020

nija-at approved these changes Sep 22, 2020

View reviewed changes

Add v2 annotation

c28e0f8

skinny85 suggested changes Sep 23, 2020

View reviewed changes

rix0rrr added 2 commits September 24, 2020 16:09

Review comments, move a bunch of libraries to private/

9cb3671

Merge branch 'huijbers/cheap-pipelines' of github.com:aws/aws-cdk int…

df3ef4c

…o huijbers/cheap-pipelines

skinny85 reviewed Sep 24, 2020

View reviewed changes

Review comments

b66bf0e

skinny85 reviewed Sep 25, 2020

View reviewed changes

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts Outdated Show resolved Hide resolved

Call 'ensureReplicationResourcesExistFor' from 'getRoleFromActionProp…

6ff7653

…sOrGenerateIfCrossAccount'

rix0rrr requested a review from skinny85 September 28, 2020 08:28

skinny85 added 2 commits September 28, 2020 09:27

Merge remote-tracking branch 'origin/master' into huijbers/cheap-pipe…

98c67d7

…lines

Small refactoring of the RichAction class.

9d58892

skinny85 approved these changes Sep 28, 2020

View reviewed changes

Merge branch 'master' into huijbers/cheap-pipelines

bd3bdbb

rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label Sep 29, 2020

mergify bot merged commit 1f7311f into master Sep 29, 2020

mergify bot deleted the huijbers/cheap-pipelines branch September 29, 2020 12:18

skinny85 mentioned this pull request Oct 19, 2020

[codepipeline] Error : Artifact Bucket must have a KMS Key to add cross-account action #10846

Closed

		To perform actions in a different account than your Pipeline is in, most
		actions accept a resource that is in a different account (either created

		You can also explicitly pass a `role` when creating the action. In that case,
		the `account` property is ignored, and the action will operate in the same

		@@ -0,0 +1,36 @@
		import { Aws, ResourceEnvironment } from '@aws-cdk/core';

feat(pipelines): allow disabling of KMS keys #10396

feat(pipelines): allow disabling of KMS keys #10396

Conversation

rix0rrr commented Sep 16, 2020 • edited Loading

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr Sep 21, 2020 • edited Loading

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

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

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 Sep 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nija-at left a comment

Choose a reason for hiding this comment

skinny85 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

skinny85 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

rix0rrr Sep 28, 2020 • edited Loading

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

skinny85 Sep 24, 2020 • edited Loading

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

aoropeza commented Sep 28, 2020 • edited Loading

skinny85 commented Sep 28, 2020

mergify bot commented Sep 29, 2020

aws-cdk-automation commented Sep 29, 2020

AWS CodeBuild CI Report

rix0rrr commented Sep 16, 2020 •

edited

Loading

rix0rrr Sep 21, 2020 •

edited

Loading

rix0rrr Sep 21, 2020 •

edited

Loading

rix0rrr Sep 28, 2020 •

edited

Loading

skinny85 Sep 24, 2020 •

edited

Loading

aoropeza commented Sep 28, 2020 •

edited

Loading