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

feat(codepipeline): change default value for crossAccountKeys to false (under feature flag) #28556

Merged
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ const prodStage = {
};

new codepipeline.Pipeline(stack, 'Pipeline', {
crossAccountKeys: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to explicitly set it to true? The feature flag is not enabled by default and shouldn't change existing behaviour right?
Same questions for all other places where you added this prop.

Copy link
Contributor Author

@go-to-k go-to-k Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that the feature flag is enabled by default for integ tests in CodeBuild CI. I put it all back and got an error in CodeBuild CI.

Changes

CodeBuild CI Build log (AccessDenied by Request has expired)

Copy link
Contributor Author

@go-to-k go-to-k Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still tried to go back to the original code to test it out, and the CI was an error.

fd0ee48

ci1

ci2

However, there is also an integ test where the feature flag remains off by default.

https://github.com/go-to-k/aws-cdk/blob/cba7c75fdd055b1cd48f21fbaa3ab29cf5944693/packages/@aws-cdk-testing/framework-integ/test/aws-cloudwatch-actions/test/integ.lambda-alarm-action.ts#L60-L74

I thought it might be due to a difference in integ.json (this file and the another file) by using new integ.IntegTest() or not, but since I still don't understand how this works, I think I'll leave this PR as is for now. (Because fixing this could be another potentially destructive change.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran the other test (integ.lambda-alarm-action.ts) as a new integ file (with no existing snapshot) to try it out, and it seems that the feature flag is enabled by default.
(Does using new integ.IntegTest() turn off the feature flag with an existing snapshot?)

Either way, I think we're good to go for now.

stages: [
sourceStage,
prodStage,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ new lambda.Function(lambdaStack, 'Lambda', {
// other resources that your Lambda needs, added to the lambdaStack...

const pipelineStack = new cdk.Stack(app, 'PipelineStack');
const pipeline = new codepipeline.Pipeline(pipelineStack, 'Pipeline');
const pipeline = new codepipeline.Pipeline(pipelineStack, 'Pipeline', {
crossAccountKeys: true,
});

// add the source code repository containing this code to your Pipeline,
// and the source code of the Lambda Function, if they're separate
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ const app = new cdk.App();

const stack = new cdk.Stack(app, 'aws-cdk-codepipeline-lambda');

const pipeline = new codepipeline.Pipeline(stack, 'Pipeline');
const pipeline = new codepipeline.Pipeline(stack, 'Pipeline', {
crossAccountKeys: true,
});

const sourceStage = pipeline.addStage({ stageName: 'Source' });
const bucket = new s3.Bucket(stack, 'PipelineBucket', {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ const deployStage = {
};

new codepipeline.Pipeline(stack, 'Pipeline', {
crossAccountKeys: true,
stages: [
sourceStage,
deployStage,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ const app = new cdk.App();

const stack = new cdk.Stack(app, 'aws-cdk-codepipeline-cloudformation');

const pipeline = new codepipeline.Pipeline(stack, 'Pipeline');
const pipeline = new codepipeline.Pipeline(stack, 'Pipeline', {
crossAccountKeys: true,
});

const bucket = new s3.Bucket(stack, 'PipelineBucket', {
versioned: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ const testAction = new cpactions.CodeBuildAction({
});

new codepipeline.Pipeline(stack, 'Pipeline', {
crossAccountKeys: true,
stages: [
{
stageName: 'source',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ const eventPattern

const pipeline = new codepipeline.Pipeline(stack, 'Pipeline', {
pipelineName: 'IntegCustomEventPipeline',
crossAccountKeys: true,
stages: [
{
stageName: 'source',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ const repo = new codecommit.Repository(stack, 'MyRepo', {
});

new codepipeline.Pipeline(stack, 'Pipeline', {
crossAccountKeys: true,
stages: [
{
stageName: 'source',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const repo = new codecommit.Repository(stack, 'MyRepo', {
});

new codepipeline.Pipeline(stack, 'Pipeline', {
crossAccountKeys: true,
stages: [
{
stageName: 'source',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ const app = new cdk.App();

const stack = new cdk.Stack(app, 'aws-cdk-pipeline-event-target');

const pipeline = new codepipeline.Pipeline(stack, 'MyPipeline');
const pipeline = new codepipeline.Pipeline(stack, 'MyPipeline', {
crossAccountKeys: true,
});

const repository = new codecommit.Repository(stack, 'CodeCommitRepo', {
repositoryName: 'foo',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ const simpleStateMachine = new stepfunctions.StateMachine(stack, 'SimpleStateMac
definition: startState,
});

const pipeline = new codepipeline.Pipeline(stack, 'MyPipeline');
const pipeline = new codepipeline.Pipeline(stack, 'MyPipeline', {
crossAccountKeys: true,
});
pipeline.addStage({
stageName: 'Source',
actions: [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ const bucket = new Bucket(stack1, 'ReplicationBucket', {

const artifact = new Artifact();
const pipeline = new Pipeline(stack2, 'Pipeline', {
crossAccountKeys: true,
crossRegionReplicationBuckets: {
'us-east-1': bucket,
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const bucket = new s3.Bucket(bucketStack, 'Bucket', {
const pipelineStack = new Stack(app, 'PipelineStack');
const sourceOutput = new codepipeline.Artifact();
new codepipeline.Pipeline(pipelineStack, 'Pipeline', {
crossAccountKeys: true,
stages: [
{
stageName: 'Source',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@ const repo = new codecommit.Repository(stack, 'Repo', {
repositoryName: 'TestRepository',
});

const pipeline = new codepipeline.Pipeline(stack, 'pipelinePipeline22F2A91D');
const pipeline = new codepipeline.Pipeline(stack, 'pipelinePipeline22F2A91D', {
crossAccountKeys: true,
});

const srcArtifact = new codepipeline.Artifact('Src');
pipeline.addStage({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export class TestCdkStack extends Stack {
});

const pipeline = new cdkp.CdkPipeline(this, 'TestPipeline', {
crossAccountKeys: true,
selfMutating: false,
pipelineName: 'TestPipeline',
cloudAssemblyArtifact,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ class CdkpipelinesDemoPipelineStack extends Stack {
autoDeleteObjects: true,
});
const pipeline = new cdkp.CdkPipeline(this, 'Pipeline', {
crossAccountKeys: true,
cloudAssemblyArtifact,
singlePublisherPerType: true,

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ class CdkpipelinesDemoPipelineStack extends Stack {
autoDeleteObjects: true,
});
const pipeline = new cdkp.CdkPipeline(this, 'Pipeline', {
crossAccountKeys: true,
cloudAssemblyArtifact,

// Where the source can be found
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ class CdkpipelinesDemoPipelineStack extends Stack {
autoDeleteObjects: true,
});
const pipeline = new cdkp.CdkPipeline(this, 'Pipeline', {
crossAccountKeys: true,
cloudAssemblyArtifact,

// Where the source can be found
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ const deployAction = new S3DeployAction({
extract: true,
});
const pipeline = new Pipeline(stack, 'MyPipeline', {
crossAccountKeys: true,
stages: [
{
stageName: 'beta',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ const deployment = new BucketDeployment(stack, 'BucketDeployment', {
],
});
const pipeline = new Pipeline(stack, 'Pipeline', {
crossAccountKeys: true,
stages: [
{
stageName: 'source',
Expand Down
20 changes: 19 additions & 1 deletion packages/@aws-cdk/cx-api/FEATURE_FLAGS.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ Flags come in three types:
| [@aws-cdk/aws-rds:preventRenderingDeprecatedCredentials](#aws-cdkaws-rdspreventrenderingdeprecatedcredentials) | When enabled, creating an RDS database cluster from a snapshot will only render credentials for snapshot credentials. | 2.98.0 | (fix) |
| [@aws-cdk/aws-codepipeline-actions:useNewDefaultBranchForCodeCommitSource](#aws-cdkaws-codepipeline-actionsusenewdefaultbranchforcodecommitsource) | When enabled, the CodeCommit source action is using the default branch name 'main'. | 2.103.1 | (fix) |
| [@aws-cdk/aws-cloudwatch-actions:changeLambdaPermissionLogicalIdForLambdaAction](#aws-cdkaws-cloudwatch-actionschangelambdapermissionlogicalidforlambdaaction) | When enabled, the logical ID of a Lambda permission for a Lambda action includes an alarm ID. | V2NEXT | (fix) |
| [@aws-cdk/aws-codepipeline:crossAccountKeysDefaultValueToFalse](#aws-cdkaws-codepipelinecrossaccountkeysdefaultvaluetofalse) | Enables Pipeline to set the default value for crossAccountKeys to false. | V2NEXT | (default) |

<!-- END table -->

Expand Down Expand Up @@ -118,7 +119,8 @@ The following json shows the current recommended set of flags, as `cdk init` wou
"@aws-cdk/aws-appsync:useArnForSourceApiAssociationIdentifier": true,
"@aws-cdk/aws-rds:preventRenderingDeprecatedCredentials": true,
"@aws-cdk/aws-codepipeline-actions:useNewDefaultBranchForCodeCommitSource": true,
"@aws-cdk/aws-cloudwatch-actions:changeLambdaPermissionLogicalIdForLambdaAction": true
"@aws-cdk/aws-cloudwatch-actions:changeLambdaPermissionLogicalIdForLambdaAction": true,
"@aws-cdk/aws-codepipeline:crossAccountKeysDefaultValueToFalse": true
paulhcsun marked this conversation as resolved.
Show resolved Hide resolved
}
}
```
Expand Down Expand Up @@ -1213,4 +1215,20 @@ If the flag is set to false then it can only make one alarm for the Lambda with
| V2NEXT | `false` | `true` |


### @aws-cdk/aws-codepipeline:crossAccountKeysDefaultValueToFalse

*Enables Pipeline to set the default value for crossAccountKeys to false.* (default)

If this is set, and a `crossAccountKeys` prop in a `Pipeline` construct is not passed to,
the construct will set the default value of the prop to false.
go-to-k marked this conversation as resolved.
Show resolved Hide resolved


| Since | Default | Recommended |
| ----- | ----- | ----- |
| (not in v1) | | |
| V2NEXT | `false` | `true` |

**Compatibility with old behavior:** Pass `crossAccountKeys: true` to `Pipeline` construct to restore the previous behavior.


<!-- END details -->
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ const prodStage = {
};

new codepipeline.Pipeline(stack, 'Pipeline', {
crossAccountKeys: true,
stages: [
sourceStage,
prodStage,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ new lambda.Function(lambdaStack, 'Lambda', {
// other resources that your Lambda needs, added to the lambdaStack...

const pipelineStack = new cdk.Stack(app, 'PipelineStack');
const pipeline = new codepipeline.Pipeline(pipelineStack, 'Pipeline');
const pipeline = new codepipeline.Pipeline(pipelineStack, 'Pipeline', {
crossAccountKeys: true,
});

// add the source code repository containing this code to your Pipeline,
// and the source code of the Lambda Function, if they're separate
Expand Down
7 changes: 4 additions & 3 deletions packages/aws-cdk-lib/aws-codepipeline/lib/pipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,8 @@ export interface PipelineProps {
* encrypted with an AWS-managed key). However, cross-account deployments will
* no longer be possible.
*
* @default true
* @default false - false if the feature flag `CODEPIPELINE_CROSS_ACCOUNT_KEYS_DEFAULT_VALUE_TO_FALSE`
* is true, true otherwise
*/
readonly crossAccountKeys?: boolean;

Expand Down Expand Up @@ -386,8 +387,8 @@ export class Pipeline extends PipelineBase {
throw new Error('Only one of artifactBucket and crossRegionReplicationBuckets can be specified!');
}

// @deprecated(v2): switch to default false
this.crossAccountKeys = props.crossAccountKeys ?? true;
this.crossAccountKeys = props.crossAccountKeys
?? (FeatureFlags.of(this).isEnabled(cxapi.CODEPIPELINE_CROSS_ACCOUNT_KEYS_DEFAULT_VALUE_TO_FALSE) ? false : true);
go-to-k marked this conversation as resolved.
Show resolved Hide resolved
this.enableKeyRotation = props.enableKeyRotation;

// Cross account keys must be set for key rotation to be enabled
Expand Down
43 changes: 43 additions & 0 deletions packages/aws-cdk-lib/aws-codepipeline/test/pipeline.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -495,6 +495,49 @@ describe('', () => {
'EnableKeyRotation': true,
});
});

test('crossAccountKeys as default value is set to false when feature flag is set', () => {
go-to-k marked this conversation as resolved.
Show resolved Hide resolved
const app = new cdk.App();
app.node.setContext(cxapi.CODEPIPELINE_CROSS_ACCOUNT_KEYS_DEFAULT_VALUE_TO_FALSE, true);

const stack = new cdk.Stack(app, 'PipelineStack');
const sourceOutput = new codepipeline.Artifact();
new codepipeline.Pipeline(stack, 'Pipeline', {
stages: [
{
stageName: 'Source',
actions: [new FakeSourceAction({ actionName: 'Source', output: sourceOutput })],
},
{
stageName: 'Build',
actions: [new FakeBuildAction({ actionName: 'Build', input: sourceOutput })],
},
],
});

Template.fromStack(stack).resourceCountIs('AWS::KMS::Key', 0);
});

test('crossAccountKeys as default value is set to true when feature flag is not set', () => {
const app = new cdk.App();

const stack = new cdk.Stack(app, 'PipelineStack');
const sourceOutput = new codepipeline.Artifact();
new codepipeline.Pipeline(stack, 'Pipeline', {
stages: [
{
stageName: 'Source',
actions: [new FakeSourceAction({ actionName: 'Source', output: sourceOutput })],
},
{
stageName: 'Build',
actions: [new FakeBuildAction({ actionName: 'Build', input: sourceOutput })],
},
],
});

Template.fromStack(stack).resourceCountIs('AWS::KMS::Key', 1);
});
});
});

Expand Down
20 changes: 19 additions & 1 deletion packages/aws-cdk-lib/cx-api/FEATURE_FLAGS.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ Flags come in three types:
| [@aws-cdk/aws-rds:preventRenderingDeprecatedCredentials](#aws-cdkaws-rdspreventrenderingdeprecatedcredentials) | When enabled, creating an RDS database cluster from a snapshot will only render credentials for snapshot credentials. | 2.98.0 | (fix) |
| [@aws-cdk/aws-codepipeline-actions:useNewDefaultBranchForCodeCommitSource](#aws-cdkaws-codepipeline-actionsusenewdefaultbranchforcodecommitsource) | When enabled, the CodeCommit source action is using the default branch name 'main'. | 2.103.1 | (fix) |
| [@aws-cdk/aws-cloudwatch-actions:changeLambdaPermissionLogicalIdForLambdaAction](#aws-cdkaws-cloudwatch-actionschangelambdapermissionlogicalidforlambdaaction) | When enabled, the logical ID of a Lambda permission for a Lambda action includes an alarm ID. | 2.124.0 | (fix) |
| [@aws-cdk/aws-codepipeline:crossAccountKeysDefaultValueToFalse](#aws-cdkaws-codepipelinecrossaccountkeysdefaultvaluetofalse) | Enables Pipeline to set the default value for crossAccountKeys to false. | V2NEXT | (default) |

<!-- END table -->

Expand Down Expand Up @@ -118,7 +119,8 @@ The following json shows the current recommended set of flags, as `cdk init` wou
"@aws-cdk/aws-appsync:useArnForSourceApiAssociationIdentifier": true,
"@aws-cdk/aws-rds:preventRenderingDeprecatedCredentials": true,
"@aws-cdk/aws-codepipeline-actions:useNewDefaultBranchForCodeCommitSource": true,
"@aws-cdk/aws-cloudwatch-actions:changeLambdaPermissionLogicalIdForLambdaAction": true
"@aws-cdk/aws-cloudwatch-actions:changeLambdaPermissionLogicalIdForLambdaAction": true,
"@aws-cdk/aws-codepipeline:crossAccountKeysDefaultValueToFalse": true
paulhcsun marked this conversation as resolved.
Show resolved Hide resolved
}
}
```
Expand Down Expand Up @@ -1213,4 +1215,20 @@ If the flag is set to false then it can only make one alarm for the Lambda with
| 2.124.0 | `false` | `true` |


### @aws-cdk/aws-codepipeline:crossAccountKeysDefaultValueToFalse

*Enables Pipeline to set the default value for crossAccountKeys to false.* (default)

If this is set, and a `crossAccountKeys` prop in a `Pipeline` construct is not passed to,
the construct will set the default value of the prop to false.
go-to-k marked this conversation as resolved.
Show resolved Hide resolved


| Since | Default | Recommended |
| ----- | ----- | ----- |
| (not in v1) | | |
| V2NEXT | `false` | `true` |

**Compatibility with old behavior:** Pass `crossAccountKeys: true` to `Pipeline` construct to restore the previous behavior.


<!-- END details -->
17 changes: 17 additions & 0 deletions packages/aws-cdk-lib/cx-api/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -258,3 +258,20 @@ _cdk.json_
}
}
```

* `@aws-cdk/aws-codepipeline:crossAccountKeysDefaultValueToFalse`

Enables Pipeline to set the default value for `crossAccountKeys` to false.

If this is set, and a `crossAccountKeys` prop in a `Pipeline` construct is not passed to,
the construct will set the default value of the prop to false.
go-to-k marked this conversation as resolved.
Show resolved Hide resolved

_cdk.json_

```json
{
"context": {
"@aws-cdk/aws-codepipeline:crossAccountKeysDefaultValueToFalse": true
}
}
```
14 changes: 14 additions & 0 deletions packages/aws-cdk-lib/cx-api/lib/features.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ export const AURORA_CLUSTER_CHANGE_SCOPE_OF_INSTANCE_PARAMETER_GROUP_WITH_EACH_P
export const APPSYNC_ENABLE_USE_ARN_IDENTIFIER_SOURCE_API_ASSOCIATION = '@aws-cdk/aws-appsync:useArnForSourceApiAssociationIdentifier';
export const CODECOMMIT_SOURCE_ACTION_DEFAULT_BRANCH_NAME = '@aws-cdk/aws-codepipeline-actions:useNewDefaultBranchForCodeCommitSource';
export const LAMBDA_PERMISSION_LOGICAL_ID_FOR_LAMBDA_ACTION = '@aws-cdk/aws-cloudwatch-actions:changeLambdaPermissionLogicalIdForLambdaAction';
export const CODEPIPELINE_CROSS_ACCOUNT_KEYS_DEFAULT_VALUE_TO_FALSE = '@aws-cdk/aws-codepipeline:crossAccountKeysDefaultValueToFalse';

export const FLAGS: Record<string, FlagInfo> = {
//////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -993,6 +994,19 @@ export const FLAGS: Record<string, FlagInfo> = {
introducedIn: { v2: '2.124.0' },
recommendedValue: true,
},

//////////////////////////////////////////////////////////////////////
[CODEPIPELINE_CROSS_ACCOUNT_KEYS_DEFAULT_VALUE_TO_FALSE]: {
type: FlagType.ApiDefault,
summary: 'Enables Pipeline to set the default value for crossAccountKeys to false.',
detailsMd: `
If this is set, and a \`crossAccountKeys\` prop in a \`Pipeline\` construct is not passed to,
the construct will set the default value of the prop to false.
go-to-k marked this conversation as resolved.
Show resolved Hide resolved
`,
introducedIn: { v2: 'V2NEXT' },
GavinZZ marked this conversation as resolved.
Show resolved Hide resolved
recommendedValue: true,
compatibilityWithOldBehaviorMd: 'Pass `crossAccountKeys: true` to `Pipeline` construct to restore the previous behavior.',
},
};

const CURRENT_MV = 'v2';
Expand Down
Loading