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(rds): RDS Parameter Group doesn't support custom removal policy #28660

Merged
merged 3 commits into from
Jan 11, 2024

Conversation

GavinZZ
Copy link
Contributor

@GavinZZ GavinZZ commented Jan 10, 2024

Can't destroy a stack that includes a rds database and rds parameter group where the database has removalPolicy property set to RemovalPolicy.RETAIN

The following is the current behaviour:

const parameterGroup = new ParameterGroup(this, 'ParameterGroup', {
    ...
}

const database = new DatabaseInstance(this, 'DatabaseInstance', {
    parameterGroup: parameterGroup,
    removalPolicy: RemovalPolicy.RETAIN,
    ...
})

When destroying the stack

When I destroy this stack I see the following errors:

2:04:24 PM | DELETE_FAILED        | AWS::RDS::DBParameterGroup                  | ParameterGroup5E32DECB
One or more database instances are still members of this parameter group xxx-database-parametergroup5e32decb-daetrwpaqpgw, so the group cannot be deleted (Service: Rd
s, Status Code: 400, Request ID: 389b18db-ea82-482b-a0e6-f64887da6f82)

2:19:21 PM | DELETE_FAILED        | AWS::EC2::SecurityGroup                     | DatabaseInstanceSecurityGroup8BDF0112
resource sg-0bfc8aacb3d3e3d4a has a dependent object (Service: AmazonEC2; Status Code: 400; Error Code: DependencyViolation; Request ID: 1eac5393-83df-48cf-bd75-41f25abb04
7a; Proxy: null)

As pointed out in the issue linked below, we cannot simply use the clusterRds' or instanceRds' removal policy because the parameter group can be simultaneously binded to a cluster and an instance.

New behaviour:

Add an optional property removalPolicy to the L2 Parameter Group resource and set the deletion policy to the generated L1 Parameter Group (Either cluster or instance) depending on the usage.

Added unit test and integration test to verify that it works as expected.

Closes #22141


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

@github-actions github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Jan 10, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team January 10, 2024 23:36
@GavinZZ GavinZZ force-pushed the yuanhaoz/rds_pg_removal_policy branch from 8ff160a to bd6254a Compare January 11, 2024 01:02
@GavinZZ GavinZZ added contribution/core This is a PR that came from AWS. and removed beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Jan 11, 2024
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 11, 2024
Copy link
Contributor

@paulhcsun paulhcsun left a comment

Choose a reason for hiding this comment

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

Overall code looks good to me, just a couple questions about the integ test and policy description.

publiclyAccessible: true,
iamAuthentication: true,
parameterGroup,
removalPolicy: cdk.RemovalPolicy.DESTROY,
Copy link
Contributor

Choose a reason for hiding this comment

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

The original issue was not working when removalPolicy: RemovalPolicy.RETAIN was set. I know for an integ test we can't leave behind the database but I just want to confirm that either:

  1. The issue wasn't specific to the RETAIN removal policy but rather any removal policy set for the databaseInstance or
  2. If the issue was specific to the RETAIN removal policy that you were able to verify locally that the stack was sucessfully destroyed when the removal policy was RETAIN.

If the second case is true maybe we could update this integ test to use RemovalPolicy.RETAIN and verify that the stack was destroyed before cleaning up the databaseInstance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this issue is not specific to RETAIN rather setting any removal policy. I confirmed <2> by deploying a test stack with setting DBInstance and ParameterGroup to RETAIN and deleting the stack. I can confirm that I'm no longer encountering the issue described in the issue #22141.

We can't update the integ test to use RETAIN as we won't be able to cleanup the retained resources once the stack is deleted, which will cause leftover resources.

Comment on lines +88 to +89
* The CloudFormation policy to apply when the instance is removed from the
* stack or replaced during an update.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this policy always needed or can we update the description to be more specific that it's only needed when the stack also has a DatabaseInstance?

Copy link
Contributor Author

@GavinZZ GavinZZ Jan 11, 2024

Choose a reason for hiding this comment

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

This doesn't have to be used with DBInstance. It's technically a standalone removal policy that applies to ParameterGroup.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 9e7689e
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

Copy link
Contributor

mergify bot commented Jan 11, 2024

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. contribution/core This is a PR that came from AWS. effort/small Small work item – less than a day of effort p2 pr/needs-maintainer-review This PR needs a review from a Core Team Member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(aws-rds): Can't destroy a stack with a database that has removalPolicy property set to RemovalPolicy.RETAIN
3 participants