-
Notifications
You must be signed in to change notification settings - Fork 4k
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(rds): subnet selection not respected for multi user secret rotation #19237
Conversation
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank for the fix @jogold! A few minor comments.
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, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block is repeated 4 times (yes, I know secret
is not always there 😛). Can we get rid of this duplication?
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, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block is repeated 6 times (yes, I know secret
is not always there 😛). Can we get rid of this duplication?
@@ -954,6 +954,76 @@ describe('cluster', () => { | |||
}); | |||
}); | |||
|
|||
test('addRotationMultiUser() with options', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we write a better description of what this test is checking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are testing here that those rotation options are correctly passed: automaticallyAfter
, excludeCharacters
and vpcSubnets
.
Same as here:
test('addRotationSingleUser() with options', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, can we write something like that? 🙂
masterSecretArn: { | ||
Ref: 'DatabaseSecretAttachmentE5D1B020', | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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',
},
});
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it!
}, | ||
}, | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here as for the test above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there! 🙂
@@ -456,7 +456,7 @@ export abstract class SnapshotCredentials { | |||
/** | |||
* Properties common to single-user and multi-user rotation options. | |||
*/ | |||
interface CommonRotationUserOptions { | |||
export interface CommonRotationUserOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's weird, because this interface used to be not exported, but JSII is gonna JSII...😉. Let's leave it as-is.
* 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I won't even pretend I understand what this is doing 😃.
Do we really need this?
@@ -954,6 +954,76 @@ describe('cluster', () => { | |||
}); | |||
}); | |||
|
|||
test('addRotationMultiUser() with options', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, can we write something like that? 🙂
masterSecretArn: { | ||
Ref: 'DatabaseSecretAttachmentE5D1B020', | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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',
},
});
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it, thanks @jogold!
/** | ||
* Applies defaults for rotation options | ||
*/ | ||
export function applyDefaultRotationOptions(options: CommonRotationUserOptions, defaultvpcSubnets?: ec2.SubnetSelection): CommonRotationUserOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So much simpler, love it!
@@ -456,7 +456,7 @@ export abstract class SnapshotCredentials { | |||
/** | |||
* Properties common to single-user and multi-user rotation options. | |||
*/ | |||
interface CommonRotationUserOptions { | |||
export interface CommonRotationUserOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's weird, because this interface used to be not exported, but JSII is gonna JSII...😉. Let's leave it as-is.
masterSecretArn: { | ||
Ref: 'DatabaseSecretAttachmentE5D1B020', | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it!
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). |
vpcWithIsolated.selectSubnets({ | ||
subnetType: ec2.SubnetType.PRIVATE_WITH_NAT, | ||
}).subnetIds; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can kill this safely:
vpcWithIsolated.selectSubnets({ | |
subnetType: ec2.SubnetType.PRIVATE_WITH_NAT, | |
}).subnetIds; |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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*
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