-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(rds): support instance and iam-db-auth-error CloudWatch log exports #35058
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
base: main
Are you sure you want to change the base?
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
kumvprat
left a comment
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.
Thanks for raising the PR.
I do see that the RDS apis support this but I cant seem to find it in the Cloudformation template reference : https://docs.aws.amazon.com/AWSCloudFormation/latest/TemplateReference/aws-resource-rds-dbcluster.html#cfn-rds-dbcluster-enablecloudwatchlogsexports
I think the integration test checks can be expanded to check that the DB -> CW integration works as expected post stack deployment. (assertions guide might be helpful)
|
CloudFormation documentation for AWS::RDS::DBCluster.EnableCloudwatchLogsExports shows only postgresql as valid for Aurora PostgreSQL, but RDS API supports instance | postgresql | iam-db-auth-error. Evidence: I am reaching out internally for clarifying. For this PR, we need to make sure CFN won't throw errors with all 3 types. |
|
@sjakthol We are working with internal teams to clarify the documentation : #35058 (comment) |
kumvprat
left a comment
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 the integration test checks can be expanded to check that the DB -> CW integration works as expected post stack deployment. (assertions guide might be helpful)
Can you add these extra assertion checks to make the tests complete ?
Comment
|
Thanks for the input and apologies for the silence. I was waiting for that clarification from the service team on if this change can actually be made or not before making additional changes. I think it would be possible to update the integration test to check the log group contents after the database clusters are deployed. However, I'm not sure how robust such test would be as the database is not really guaranteed to log anything or the messages it logs might change between runs and versions etc. Would it be sufficient to have the integration test check the cluster configuration via the DescribeDbClusters API [1] and assert that the set of EnabledCloudwatchLogsExports on the cluster matches the set configured via CDK / CloudFormation? Personally I think that if the logs the cluster claims to be exporting to CloudWatch end up to CloudWatch is more of an RDS / Aurora service matter. It does not really sound like something CDK integration tests should be responsible checking. But checking that the log types specified by CDK / CloudFormation were accepted and reflected in the cluster configuration could provide some coverage against configuration mismatches. Thanks again for the review. I'll try to finalize this as soon as possible once the open questions have been clarified. [1] https://docs.aws.amazon.com/cli/latest/reference/rds/describe-db-clusters.html |
Agreed, like you said I was mainly expecting that the configuration of log type to properly be reflected in cluster configuration. Based on the PR I am assuming these tests were not present and this is the first time these integration tests are being added. In that case configuration check would be the aim since matching the values in the log will be to brittle and un-maintainable tests that won't be robust to future changes Will keep you informed on our discussion with internal teams so that you can make the requested changes when we have the confirmation |
|
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. Note that PRs with failing linting check or builds are not reviewed, please ensure your build is passing To prevent automatic closure:
This PR will automatically close in 14 days if no action is taken. |
Pull request has been modified.
b9c1011 to
34a6d17
Compare
…ly applied to the clusters
34a6d17 to
cf23356
Compare
|
I've update the integration tests to call rds.DescribeDBCluster and assert that the set of enabled log exports match those set in CDK / CloudFormation. Also rebased on top of latest main and fixed few incompatibilities this caused. This should be ready to go now. |
Issue # (if applicable)
Closes #35018.
Reason for this change
Add support for newly launched Aurora CloudWatch log exports.
Description of changes
This commit updates the list of allowed CloudWatch log exports to match the values accepted by the RDS API:
https://docs.aws.amazon.com/AmazonRDS/latest/APIReference/API_CreateDBCluster.html
Tests and documentation updated to consider this change as well.
Describe any new or updated permissions being added
N/A
Description of how you validated changes
Unit tests have been updated to check that the new log export names are accepted. Integration tests added to verify that both CDK and RDS accept all the supported log exports.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license