fix(rds): subnet selection not respected for multi user secret rotation by jogold · Pull Request #19237 · aws/aws-cdk

jogold · 2022-03-04T10:52:50Z

The subnet selection was always overriden by the subnet selection of the
instance/cluster.

Avoid these kinds of errors by explicitely defining rotation options and
their defaults.

Closes #19233

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

The subnet selection was always overriden by the subnet selection of the instance/cluster. Avoid these kinds of errors by explicitely defining rotation options and their defaults. Closes aws#19233

gitpod-io · 2022-03-04T10:52:52Z

skinny85

Thank for the fix @jogold! A few minor comments.

skinny85 · 2022-03-05T00:58:37Z

packages/@aws-cdk/aws-rds/lib/instance.ts

+    const rotationOptions: Complete<RotationMultiUserOptions> = {
+      automaticallyAfter: options.automaticallyAfter,
+      endpoint: options.endpoint,
      excludeCharacters: options.excludeCharacters ?? DEFAULT_PASSWORD_EXCLUDE_CHARS,
+      secret: options.secret,
+      vpcSubnets: options.vpcSubnets ?? this.vpcPlacement,
+    };


This block is repeated 4 times (yes, I know secret is not always there 😛). Can we get rid of this duplication?

skinny85 · 2022-03-05T01:00:11Z

packages/@aws-cdk/aws-rds/lib/serverless-cluster.ts

+    const rotationOptions: Complete<RotationMultiUserOptions> = {
+      automaticallyAfter: options.automaticallyAfter,
+      endpoint: options.endpoint,
      excludeCharacters: options.excludeCharacters ?? DEFAULT_PASSWORD_EXCLUDE_CHARS,
+      secret: options.secret,
+      vpcSubnets: options.vpcSubnets ?? this.vpcSubnets,
+    };


This block is repeated 6 times (yes, I know secret is not always there 😛). Can we get rid of this duplication?

skinny85 · 2022-03-05T01:12:45Z

packages/@aws-cdk/aws-rds/test/cluster.test.ts

    });
  });

+  test('addRotationMultiUser() with options', () => {


Can we write a better description of what this test is checking?

We are testing here that those rotation options are correctly passed: automaticallyAfter, excludeCharacters and vpcSubnets.

Same as here:

aws-cdk/packages/@aws-cdk/aws-rds/test/instance.test.ts

Line 800 in e0f863a

test('addRotationSingleUser() with options', () => {

OK, can we write something like that? 🙂

skinny85 · 2022-03-05T01:18:51Z

packages/@aws-cdk/aws-rds/test/cluster.test.ts

+        masterSecretArn: {
+          Ref: 'DatabaseSecretAttachmentE5D1B020',
+        },
+      },


I have to say - I don't understand this assertion...

Do we need all of these properties here? Are we not worried only about the vpcSubnetIds?

'Fn::Join': ['', [ { Ref: 'VpcprivateSubnet1SubnetCEAD3716' }, ',', { Ref: 'VpcprivateSubnet2Subnet2DE7549C' }, ]],

Is there a clearer way we can formulate this assertion? Maybe imported subnets? It is not clear to me that these are the PRIVATE_WITH_NAT that we provided when creating the rotation.

Do we need all of these properties here? Are we not worried only about the vpcSubnetIds?

We can remove functionName and vpcSecurityGroupIds. We need the other ones because we are checking that options are correctly passed (and endpoint is also an option).

Is there a clearer way we can formulate this assertion?

See the propostion with stack.resolve(). Do we want to update the same test for single user rotation?

Instead of weird resolve() tricks, can't we do this?

// GIVEN vpc = ec2.Vpc.fromVpcAttributes(stack, 'Vpc', { vpcId: 'vpc-id', availabilityZones: ['az1'], publicSubnetIds: ['public-subnet-id-1', 'public-subnet-id-2'], publicSubnetNames: ['public-subnet-name-1', 'public-subnet-name-2'], privateSubnetIds: ['private-subnet-id-1', 'private-subnet-id-2'], privateSubnetNames: ['private-subnet-name-1', 'private-subnet-name-2'], isolatedSubnetIds: ['isolated-subnet-id-1', 'isolated-subnet-id-2'], isolatedSubnetNames: ['isolated-subnet-name-1', 'isolated-subnet-name-2'], }); const instance = new rds.DatabaseInstance(stack, 'Instance', { engine: rds.DatabaseInstanceEngine.MYSQL, vpc: vpc, vpcSubnets: { subnetType: ec2.SubnetType.PUBLIC }, }); // WHEN instance.addRotationMultiUser('user', { secret: new rds.DatabaseSecret(stack, 'DatabaseSecret', { username: 'user', }), vpcSubnets: { subnetType: ec2.SubnetType.PRIVATE_WITH_NAT }, }); // THEN Template.fromStack(stack).hasResourceProperties('AWS::Serverless::Application', { Parameters: { vpcSubnetIds: 'private-subnet-id-1,private-subnet-id-2', }, });

?

skinny85 · 2022-03-05T01:18:56Z

packages/@aws-cdk/aws-rds/test/instance.test.ts

+        },
+      },
+    });
+  });


Same comment here as for the test above.

Pull request has been modified.

skinny85

Almost there! 🙂

skinny85 · 2022-03-07T21:29:20Z

packages/@aws-cdk/aws-rds/lib/props.ts

 * Properties common to single-user and multi-user rotation options.
 */
-interface CommonRotationUserOptions {
+export interface CommonRotationUserOptions {


Why this change...? I guess because you need this type in applyDefaultRotationOptions()...?

How about creating a new file in lib/private, something like rotation-utils.ts, that contains this type, and the applyDefaultRotationOptions() function?

Any chance we can move this to a private file

Unfortunately not possible to have a public interface that extends an interface from a non exported file with jsii...

error JSII9002: Unable to resolve type "@aws-cdk/aws-rds.CommonRotationUserOptions". It may be @internal or not exported from the module's entry point (as configured in "package.json" as "main").

That's weird, because this interface used to be not exported, but JSII is gonna JSII...😉. Let's leave it as-is.

skinny85 · 2022-03-07T21:34:39Z

packages/@aws-cdk/aws-rds/lib/private/util.ts

+ * Transforms optional properties to required properties that may be undefined
+ */
+type Complete<T> = {
+  [P in keyof Required<T>]: Pick<T, P> extends Required<Pick<T, P>> ? T[P] : (T[P] | undefined);


I won't even pretend I understand what this is doing 😃.

Do we really need this?

skinny85 · 2022-03-07T22:13:40Z

packages/@aws-cdk/aws-rds/test/cluster.test.ts

    });
  });

+  test('addRotationMultiUser() with options', () => {


OK, can we write something like that? 🙂

skinny85 · 2022-03-07T22:15:35Z

packages/@aws-cdk/aws-rds/test/cluster.test.ts

+        masterSecretArn: {
+          Ref: 'DatabaseSecretAttachmentE5D1B020',
+        },
+      },


Instead of weird resolve() tricks, can't we do this?

// GIVEN vpc = ec2.Vpc.fromVpcAttributes(stack, 'Vpc', { vpcId: 'vpc-id', availabilityZones: ['az1'], publicSubnetIds: ['public-subnet-id-1', 'public-subnet-id-2'], publicSubnetNames: ['public-subnet-name-1', 'public-subnet-name-2'], privateSubnetIds: ['private-subnet-id-1', 'private-subnet-id-2'], privateSubnetNames: ['private-subnet-name-1', 'private-subnet-name-2'], isolatedSubnetIds: ['isolated-subnet-id-1', 'isolated-subnet-id-2'], isolatedSubnetNames: ['isolated-subnet-name-1', 'isolated-subnet-name-2'], }); const instance = new rds.DatabaseInstance(stack, 'Instance', { engine: rds.DatabaseInstanceEngine.MYSQL, vpc: vpc, vpcSubnets: { subnetType: ec2.SubnetType.PUBLIC }, }); // WHEN instance.addRotationMultiUser('user', { secret: new rds.DatabaseSecret(stack, 'DatabaseSecret', { username: 'user', }), vpcSubnets: { subnetType: ec2.SubnetType.PRIVATE_WITH_NAT }, }); // THEN Template.fromStack(stack).hasResourceProperties('AWS::Serverless::Application', { Parameters: { vpcSubnetIds: 'private-subnet-id-1,private-subnet-id-2', }, });

?

Pull request has been modified.

skinny85

Love it, thanks @jogold!

skinny85 · 2022-03-08T18:36:53Z

packages/@aws-cdk/aws-rds/lib/private/util.ts

+/**
+ * Applies defaults for rotation options
+ */
+export function applyDefaultRotationOptions(options: CommonRotationUserOptions, defaultvpcSubnets?: ec2.SubnetSelection): CommonRotationUserOptions {


So much simpler, love it!

skinny85 · 2022-03-08T18:43:11Z

packages/@aws-cdk/aws-rds/lib/props.ts

 * Properties common to single-user and multi-user rotation options.
 */
-interface CommonRotationUserOptions {
+export interface CommonRotationUserOptions {


That's weird, because this interface used to be not exported, but JSII is gonna JSII...😉. Let's leave it as-is.

skinny85 · 2022-03-08T18:44:39Z

packages/@aws-cdk/aws-rds/test/cluster.test.ts

+        masterSecretArn: {
+          Ref: 'DatabaseSecretAttachmentE5D1B020',
+        },
+      },


mergify · 2022-03-08T18:47:21Z

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

skinny85 · 2022-03-08T18:47:23Z

packages/@aws-cdk/aws-rds/test/instance.test.ts

+    vpcWithIsolated.selectSubnets({
+      subnetType: ec2.SubnetType.PRIVATE_WITH_NAT,
+    }).subnetIds;


I think we can kill this safely:

Suggested change

vpcWithIsolated.selectSubnets({

subnetType: ec2.SubnetType.PRIVATE_WITH_NAT,

}).subnetIds;

aws-cdk-automation · 2022-03-08T19:29:10Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 7a447a6
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2022-03-08T19:29:32Z

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

Forgot to clean this up in aws#19237

Forgot to clean this up in #19237 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…on (aws#19237) The subnet selection was always overriden by the subnet selection of the instance/cluster. Avoid these kinds of errors by explicitely defining rotation options and their defaults. Closes aws#19233 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Forgot to clean this up in aws#19237 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

fix(rds): subnet selection not respected for multi user secret rotation

6716199

The subnet selection was always overriden by the subnet selection of the instance/cluster. Avoid these kinds of errors by explicitely defining rotation options and their defaults. Closes aws#19233

github-actions bot added the @aws-cdk/aws-rds Related to Amazon Relational Database label Mar 4, 2022

github-actions bot assigned skinny85 Mar 4, 2022

rix0rrr added bug This issue is a bug. p1 and removed bug This issue is a bug. p1 @aws-cdk/aws-rds Related to Amazon Relational Database labels Mar 4, 2022

skinny85 previously requested changes Mar 5, 2022

View reviewed changes

update tests

0232551

less duplication

ce55a96

jogold requested a review from skinny85 March 5, 2022 10:35

jogold added 2 commits March 7, 2022 09:59

defaultVpcSubnets

2b840ad

Merge branch 'master' into rds-multi-rotation-subnet

27fdd26

skinny85 previously requested changes Mar 7, 2022

View reviewed changes

PR feedback

fe2e546

move up

97ba6dc

jogold requested a review from skinny85 March 8, 2022 09:07

skinny85 approved these changes Mar 8, 2022

View reviewed changes

skinny85 reviewed Mar 8, 2022

View reviewed changes

Merge branch 'master' into rds-multi-rotation-subnet

7a447a6

mergify bot merged commit dc7a17c into aws:master Mar 8, 2022

jogold added a commit to jogold/aws-cdk that referenced this pull request Mar 8, 2022

chore(rds): remove unused code from test

cb8790b

Forgot to clean this up in aws#19237

jogold mentioned this pull request Mar 8, 2022

chore(rds): remove unused code from test #19294

Merged

mergify bot pushed a commit that referenced this pull request Mar 8, 2022

chore(rds): remove unused code from test (#19294)

0e514de

Forgot to clean this up in #19237 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

jogold deleted the rds-multi-rotation-subnet branch March 12, 2022 13:51

	vpcWithIsolated.selectSubnets({
	subnetType: ec2.SubnetType.PRIVATE_WITH_NAT,
	}).subnetIds;

Conversation

jogold commented Mar 4, 2022

Uh oh!

gitpod-io bot commented Mar 4, 2022

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Mar 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Mar 8, 2022

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Mar 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

skinny85 Mar 7, 2022 •

edited

Loading

skinny85 Mar 7, 2022 •

edited

Loading