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

fix(dynamodb): replication regions are incompatible with resource policies in TableV2 and feature flag #31513

Merged
merged 10 commits into from
Oct 23, 2024

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
}
],
"Replicas": [
{
"Region": "eu-west-2"
},
{
"Region": "eu-west-1",
"ResourcePolicy": {
Expand Down Expand Up @@ -46,7 +49,10 @@
}
}
}
]
],
"StreamSpecification": {
"StreamViewType": "NEW_AND_OLD_IMAGES"
}
},
"UpdateReplacePolicy": "Delete",
"DeletionPolicy": "Delete"
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@ import * as dynamodb from 'aws-cdk-lib/aws-dynamodb';
import * as iam from 'aws-cdk-lib/aws-iam';
import { IntegTest } from '@aws-cdk/integ-tests-alpha';

const app = new App();
const app = new App({
postCliContext: {
'@aws-cdk/aws-dynamodb:resourcePolicyPerReplica': false,
},
});
Comment on lines +7 to +11
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a test for true case?


class TestStack extends Stack {
constructor(scope: Construct, id: string, props?: StackProps) {
Expand All @@ -21,16 +25,18 @@ class TestStack extends Stack {
});

// table with resource policy
const table = new dynamodb.TableV2(this, 'TableTestV2-1', {
new dynamodb.TableV2(this, 'TableTestV2-1', {
partitionKey: {
name: 'id',
type: dynamodb.AttributeType.STRING,
},
removalPolicy: RemovalPolicy.DESTROY,
resourcePolicy: docu,
replicas: [{
region: 'eu-west-2',
}],
});

table.grantReadData(new iam.AccountPrincipal('123456789012'));
}
}

Expand Down
12 changes: 10 additions & 2 deletions packages/aws-cdk-lib/aws-dynamodb/lib/table-v2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ import { TableBaseV2, ITableV2 } from './table-v2-base';
import { PolicyDocument } from '../../aws-iam';
import { IStream } from '../../aws-kinesis';
import { IKey, Key } from '../../aws-kms';
import { ArnFormat, CfnTag, Lazy, PhysicalName, RemovalPolicy, Stack, Token } from '../../core';
import { ArnFormat, CfnTag, FeatureFlags, Lazy, PhysicalName, RemovalPolicy, Stack, Token } from '../../core';
import * as cxapi from '../../cx-api';

const HASH_KEY_TYPE = 'HASH';
const RANGE_KEY_TYPE = 'RANGE';
Expand Down Expand Up @@ -664,7 +665,14 @@ export class TableV2 extends TableBaseV2 {
private configureReplicaTable(props: ReplicaTableProps): CfnGlobalTable.ReplicaSpecificationProperty {
const pointInTimeRecovery = props.pointInTimeRecovery ?? this.tableOptions.pointInTimeRecovery;
const contributorInsights = props.contributorInsights ?? this.tableOptions.contributorInsights;
const resourcePolicy = props.resourcePolicy ?? this.tableOptions.resourcePolicy;
/*
* Feature flag set as the following may be a breaking change.
* @see https://github.com/aws/aws-cdk/pull/31097
* @see https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/cx-api/FEATURE_FLAGS.md
*/
const resourcePolicy = FeatureFlags.of(this).isEnabled(cxapi.DYNAMODB_TABLEV2_RESOURCE_POLICY_PER_REPLICA)
? (this.region ? this.tableOptions.resourcePolicy : props.resourcePolicy)
: (props.region === this.region ? this.tableOptions.resourcePolicy : props.resourcePolicy) || undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

On feature flag false case, can we stick to the original behaviour?


return {
region: props.region,
Expand Down
16 changes: 16 additions & 0 deletions packages/aws-cdk-lib/cx-api/lib/features.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ export const LOG_API_RESPONSE_DATA_PROPERTY_TRUE_DEFAULT = '@aws-cdk/custom-reso
export const S3_KEEP_NOTIFICATION_IN_IMPORTED_BUCKET = '@aws-cdk/aws-s3:keepNotificationInImportedBucket';
export const USE_NEW_S3URI_PARAMETERS_FOR_BEDROCK_INVOKE_MODEL_TASK = '@aws-cdk/aws-stepfunctions-tasks:useNewS3UriParametersForBedrockInvokeModelTask';
export const REDUCE_EC2_FARGATE_CLOUDWATCH_PERMISSIONS = '@aws-cdk/aws-ecs:reduceEc2FargateCloudWatchPermissions';
export const DYNAMODB_TABLEV2_RESOURCE_POLICY_PER_REPLICA = '@aws-cdk/aws-dynamodb:resourcePolicyPerReplica';
Copy link
Contributor

Choose a reason for hiding this comment

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


export const FLAGS: Record<string, FlagInfo> = {
//////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -1142,6 +1143,21 @@ export const FLAGS: Record<string, FlagInfo> = {
recommendedValue: true,
compatibilityWithOldBehaviorMd: 'Disable the feature flag to continue grant permissions to log group when no log group is specified',
},

//////////////////////////////////////////////////////////////////////
[DYNAMODB_TABLEV2_RESOURCE_POLICY_PER_REPLICA]: {
type: FlagType.BugFix,
summary: 'When enabled will allow you to specify a resource policy per replica, and not copy the source table policy to all replicas',
detailsMd: `
If this flag is not set, the default behavior for \`TableV2\` is to use
the use a different \`resourcePolicy\` for all replicas.

If this flag is set to false, the behavior is that each replica shares the same \`resourcePolicy\` as the source table.

This is a feature flag as the old behavior was technically incorrect but users may have come to depend on it.`,
introducedIn: { v2: 'V2NEXT' },
recommendedValue: true,
},
};

const CURRENT_MV = 'v2';
Expand Down