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.

Sign up for GitHub

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 #31097

Closed
wants to merge 9 commits into from
Closed

fix(dynamodb): replication regions are incompatible with resource policies in TableV2 #31097

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
da7c863
Fix - Resource base policy for replica tables
Aug 13, 2024
7c74c78
Fix - Added integ tests
Aug 13, 2024
35db782
Merge branch 'main' into rbp-fix-lhnng
LeeroyHannigan Aug 13, 2024
4accd0f
Removing comments from table-v2
Aug 21, 2024
ce9f776
Added feature flag for resource policy
Aug 28, 2024
5721dd6
Adding feature flag to cx-api
Aug 28, 2024
8cfe236
Update integ test
Aug 28, 2024
da181c8
Merge branch 'main' into rbp-fix-lhnng
LeeroyHannigan Aug 28, 2024
973a379
Update features.ts to remove blank space
LeeroyHannigan Aug 28, 2024
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
4 changes: 2 additions & 2 deletions .../aws-dynamodb/test/integ.dynamodb-v2.policy.js.snapshot/ResourcePolicyTest-v2.assets.json
View file
Open in desktop

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

8 changes: 7 additions & 1 deletion ...ws-dynamodb/test/integ.dynamodb-v2.policy.js.snapshot/ResourcePolicyTest-v2.template.json
View file
Open in desktop
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"
}
Comment on lines +52 to +55
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this change from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The stream came from adding a replica.

    table.addReplica({
      region: 'eu-west-2',
    });

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, that's good to know.

},
"UpdateReplacePolicy": "Delete",
"DeletionPolicy": "Delete"
Expand Down
2 changes: 1 addition & 1 deletion ...framework-integ/test/aws-dynamodb/test/integ.dynamodb-v2.policy.js.snapshot/manifest.json
View file
Open in desktop

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

8 changes: 7 additions & 1 deletion ...ing/framework-integ/test/aws-dynamodb/test/integ.dynamodb-v2.policy.js.snapshot/tree.json
View file
Open in desktop

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

3 changes: 3 additions & 0 deletions packages/@aws-cdk-testing/framework-integ/test/aws-dynamodb/test/integ.dynamodb-v2.policy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ class TestStack extends Stack {
});

table.grantReadData(new iam.AccountPrincipal('123456789012'));
table.addReplica({
region: 'eu-west-2',
});
Comment on lines +34 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

This test didn't even set the feature flag, so my understanding is that it should use the table's resource policy but doesn't seem to be the case in the output template.json. May I ask how you ran the tests locally and update the snapshot?

}
}

Expand Down
5 changes: 3 additions & 2 deletions packages/aws-cdk-lib/aws-dynamodb/lib/table-v2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,8 +664,9 @@ 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;

//const resourcePolicy = props.resourcePolicy ?? this.tableOptions.resourcePolicy;
//const resourcePolicy = props.resourcePolicy;
LeeroyHannigan marked this conversation as resolved.
Show resolved Hide resolved
const 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.

This is a breaking change for existing users. If existing users manage to bypass the CFN error by using escape hatch. They then remove the escape hatch and upgrade to a version that includes this change, their replication region will now have the resource policies removed, thus this is not an acceptable change. I would recommend you to do it following the suggested proposal in the original issue.

Ideally, the construct could allow null and when null is specified in a specific replica, no resourcePolicy is added to that replica even when one is defined in the TableV2 itself

new TableV2(this, `MyTable-${stage}`, {
      pointInTimeRecovery: true,
      partitionKey: {
        name: 'key',
        type: AttributeType.STRING,
      },
      tableName: 'MyTable',
      // us-east-2 should not have tablePolicyDocument added in the template
      replicas: [{ region: 'us-east-1' }, { region: 'us-east-2', resourcePolicy: null }], 
      resourcePolicy: tablePolicyDocument,
    });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is forcing a user to add null for when they don't want a resource policy. The default implementation should only require them to add a policy to a replica should the want it. It shouldn't default to adding it to all replicas.

return {
region: props.region,
globalSecondaryIndexes: this.configureReplicaGlobalSecondaryIndexes(props.globalSecondaryIndexOptions),
Expand Down
178 changes: 178 additions & 0 deletions packages/aws-cdk-lib/aws-dynamodb/test/table-v2.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3007,4 +3007,182 @@ test('Resource policy test', () => {
},
],
});
});

test('Add resource policy to local table only', () => {
// GIVEN
const stack = new Stack(undefined, 'Stack', { env: { region: 'eu-west-1' } });

const doc = new PolicyDocument({
statements: [
new PolicyStatement({
actions: ['dynamodb:GetItem'],
principals: [new ArnPrincipal('arn:aws:iam::111122223333:user/foobar')],
resources: ['*'],
}),
],
});

// WHEN
const table = new TableV2(stack, 'Table', {
partitionKey: { name: 'metric', type: AttributeType.STRING },
resourcePolicy: doc,
replicas: [{
region: 'eu-west-2',
}],
});

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::DynamoDB::GlobalTable', {
Replicas: [
{
Region: 'eu-west-2',
},
{
Region: 'eu-west-1',
ResourcePolicy: {
PolicyDocument: {
Statement: [
{
Action: 'dynamodb:GetItem',
Effect: 'Allow',
Principal: {
AWS: 'arn:aws:iam::111122223333:user/foobar',
},
Resource: '*',
},
],
Version: '2012-10-17',
},
},
},
],
});
});

test('Add resource policy to replica table only', () => {
// GIVEN
const stack = new Stack(undefined, 'Stack', { env: { region: 'eu-west-1' } });

const doc = new PolicyDocument({
statements: [
new PolicyStatement({
actions: ['dynamodb:GetItem'],
principals: [new ArnPrincipal('arn:aws:iam::111122223333:user/foobar')],
resources: ['*'],
}),
],
});

// WHEN
const table = new TableV2(stack, 'Table', {
partitionKey: { name: 'metric', type: AttributeType.STRING },
replicas: [{
region: 'eu-west-2',
resourcePolicy: doc,
}],
});

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::DynamoDB::GlobalTable', {
Replicas: [
{
Region: 'eu-west-2',
ResourcePolicy: {
PolicyDocument: {
Statement: [
{
Action: 'dynamodb:GetItem',
Effect: 'Allow',
Principal: {
AWS: 'arn:aws:iam::111122223333:user/foobar',
},
Resource: '*',
},
],
Version: '2012-10-17',
},
},
},
{
Region: 'eu-west-1',
},
],
});
});

test('Add two different resource policies to replicas', () => {
// GIVEN
const stack = new Stack(undefined, 'Stack', { env: { region: 'eu-west-1' } });

const doc1 = new PolicyDocument({
statements: [
new PolicyStatement({
actions: ['dynamodb:GetItem'],
principals: [new ArnPrincipal('arn:aws:iam::111122223333:user/foobar')],
resources: ['*'],
}),
],
});
const doc2 = new PolicyDocument({
statements: [
new PolicyStatement({
actions: ['dynamodb:DeleteItem'],
principals: [new ArnPrincipal('arn:aws:iam::111122223333:user/barfoo')],
resources: ['*'],
}),
],
});

// WHEN
const table = new TableV2(stack, 'Table', {
partitionKey: { name: 'metric', type: AttributeType.STRING },
resourcePolicy: doc1,
replicas: [{
region: 'eu-west-2',
resourcePolicy: doc2,
}],
});

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::DynamoDB::GlobalTable', {
Replicas: [
{
Region: 'eu-west-2',
ResourcePolicy: {
PolicyDocument: {
Statement: [
{
Action: 'dynamodb:DeleteItem',
Effect: 'Allow',
Principal: {
AWS: 'arn:aws:iam::111122223333:user/barfoo',
},
Resource: '*',
},
],
Version: '2012-10-17',
},
},
},
{
Region: 'eu-west-1',
ResourcePolicy: {
PolicyDocument: {
Statement: [
{
Action: 'dynamodb:GetItem',
Effect: 'Allow',
Principal: {
AWS: 'arn:aws:iam::111122223333:user/foobar',
},
Resource: '*',
},
],
Version: '2012-10-17',
},
},
},
],
});
});