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

feat!: Terraform MSV increased to 1.0, consolidate security group rules under one generic resource, add support for managed master password #335

Merged
merged 10 commits into from
Apr 10, 2023

Conversation

bryantbiggs
Copy link
Member

@bryantbiggs bryantbiggs commented Nov 7, 2022

Description

  • With RDS now supporting the integration with SecretsManager to manage the master user password, the ability to generate a random password has been removed from this module. This is the preferred and strongly recommended route to mange the password since it keeps the password out of the Terraform state and allows for the password to be rotated automatically.
  • Support for generating a random snapshot identifier has been removed. The AWS provider has been updated to enforce a conflict between snapshot_identifier and global_cluster_identifier which makes this feature challenging to support; which it has already been challenging to support in the past and often catching users off guard. Instead, the module now defaults to null for both of these values and puts the control back in user's hands if they wish to set a value for one of these arguments.
  • The default value for create_db_subnet_group has changed from true to false - typically, a common/shared DB subnet group is utilized since there are no real tangible benefits to creating a new one for each DB cluster
  • allowed_security_groups, allowed_cidr_blocks, and security_group_egress_rules have been removed and replaced with a more generic security_group_rules variable which supports both ingress and egress rules to/from all supported resources/destinations (e.g. security groups, CIDR blocks, prefix lists, etc.)
  • Minimum supported Terraform version is now 1.0
  • Update GitHub action versions to use latest. This remove warnings related to https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/
  • Ensure pre-commit config is aligned with latest
  • Add lock.yml workflow to automatically lock issues and PRs after 30 days. Theres a lot "Me too" or "I have this issue, when will this get fixed" on really old/stale issues and in order to properly triage, users need to supply their configurations. This workflow is pulled from the AWS provider's repo to force users to fill out a proper issue ticket and keep chatter out of merged PRs or old issues
  • Update examples to:
    • Use data source for availability zones and compute subnet CIDR blocks using the cidrsubnet() function
    • Use latest supported engine versions
    • Remove standalone resources for parameter groups since module now supports this resource management

See UPGRADE-8.0.md for full details

Motivation and Context

Breaking Changes

How Has This Been Tested?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using one or more of the provided examples/* projects
  • I have executed pre-commit run -a on my pull request

@bryantbiggs bryantbiggs marked this pull request as draft November 7, 2022 19:09
@bryantbiggs
Copy link
Member Author

The fix here is a version bump so I will catch that up with other TODOs slated for a breaking change to roll it up into one

bryantbiggs added a commit to bryantbiggs/terraform-aws-rds-aurora that referenced this pull request Jan 1, 2023
bryantbiggs added a commit to magreenbaum/terraform-aws-rds-aurora that referenced this pull request Jan 1, 2023
@bryantbiggs
Copy link
Member Author

we should wait for hashicorp/terraform-provider-aws#28538 to see what that implementation is before merging this, but in the meantime I'll prep the other changes

@bryantbiggs bryantbiggs changed the title fix: Update CI configuration files to use latest version feat!: Bump minimum supported Terraform version to 1.0, consolidate security group rules under one generic resource, update CI workflows Jan 2, 2023
@mad-it
Copy link

mad-it commented Feb 13, 2023

Since this is a breaking change anyways, maybe also good to set the default password length to something like 20 characters. 10 characters is not that long and since not using special characters it makes it fairly weak.

@bryantbiggs
Copy link
Member Author

Since this is a breaking change anyways, maybe also good to set the default password length to something like 20 characters. 10 characters is not that long and since not using special characters it makes it fairly weak.

I am waiting for hashicorp/terraform-provider-aws#28538 which deals with the password management so we'll see what that API looks like and make changes as necessary here. Ideally, there are no more passwords generated/managed by this module and therefore outside of the Terraform state

@bryantbiggs bryantbiggs force-pushed the chore/update-ci branch 2 times, most recently from 81a0ba4 to ed1083a Compare April 6, 2023 17:27
@bryantbiggs bryantbiggs changed the title feat!: Bump minimum supported Terraform version to 1.0, consolidate security group rules under one generic resource, update CI workflows feat!: Terraform MSV increased to 1.0, consolidate security group rules under one generic resource, add support for managed master password Apr 6, 2023
@bryantbiggs bryantbiggs marked this pull request as ready for review April 6, 2023 19:56
@bryantbiggs bryantbiggs requested a review from antonbabenko April 6, 2023 19:56
@bryantbiggs bryantbiggs removed the wip label Apr 6, 2023
Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

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

Zero comments! Looks perfect right away :)


on:
schedule:
- cron: '50 1 * * *'
Copy link
Member

Choose a reason for hiding this comment

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

Not related to the PR, but I would like to know how to avoid GitHub token rate limit throttling since we have the same cron expression in all repositories, and every night I receive a lot of emails when this workflow fails.

There is an open issue already, but it doesn't seem to be moving anywhere - dessant/lock-threads#35

@bryantbiggs bryantbiggs merged commit e054f77 into terraform-aws-modules:master Apr 10, 2023
@bryantbiggs bryantbiggs deleted the chore/update-ci branch April 10, 2023 11:15
antonbabenko pushed a commit that referenced this pull request Apr 10, 2023
## [8.0.0](v7.7.1...v8.0.0) (2023-04-10)

### ⚠ BREAKING CHANGES

* Terraform MSV increased to 1.0, consolidate security group rules under one generic resource, add support for managed master password (#335)

### Features

* Terraform MSV increased to 1.0, consolidate security group rules under one generic resource, add support for managed master password ([#335](#335)) ([e054f77](e054f77))
@antonbabenko
Copy link
Member

This PR is included in version 8.0.0 🎉

@nikolay
Copy link

nikolay commented Apr 11, 2023

@antonbabenko What is the proper way to migrate to v8 if you had create_random_password?

@antonbabenko
Copy link
Member

@nikolay Probably remove that random password resource from Terraform state (terraform state rm ...) and then set it as master_password value, and later migrate to use manage_master_user_password = true as the recommended approach for handling passwords.

@bryantbiggs Should we upgrade this process in the Upgrade Guide for version 8.0? I suspect this will be a frequent question.

@bryantbiggs
Copy link
Member Author

Just simply changing to use manage_master_user_password = true worked as expected so I didn't see any need to document anything special

@nwalters512
Copy link

nwalters512 commented Apr 14, 2023

Will manage_master_user_password = true opt us in to automatic password rotation? I can't tell from the docs, and I'm pretty scared to test that out. If the database password changes on a regular basis, that's going to cause lots of problems for applications that aren't pulling credentials from Secrets Manager. Even for apps that do pull from Secrets Manager, I can't see how to avoid downtime when the password is rotated. I do think this needs to be documented.

@bryantbiggs
Copy link
Member Author

bryantbiggs commented Apr 14, 2023

It is documented in the AWS documentation - Aurora does automatically rotate the password when it manages the master password

The master password is not intended for applications - the master password should be treated like the root user of an AWS account. You use it for only certain scenarios, but you should create custom users and groups with the appropriate access permissions which are used by users and/or applications. You can also look at providing access using IAM auth - either directly through Aurora or through something like RDS Proxy

@nwalters512
Copy link

That's very helpful advice - thanks @bryantbiggs!

@nikolay
Copy link

nikolay commented May 3, 2023

@antonbabenko and/or @bryantbiggs When I removed create_random_password and added manage_master_user_password = true on a live cluster, the output with the secret is now an empty array ([]). Any idea why?

@bryantbiggs
Copy link
Member Author

thats the point of the feature and why it was designed - when using manage_master_user_password the password is stored in a SecretsManager secret in your account; no secret is leaked into your Terraform state and no conflicts with Terraform if you rotate that secret

@nikolay
Copy link

nikolay commented May 3, 2023

@bryantbiggs I was expecting the ARN to the secret. I use the PostgreSQL provider to create app-specific users, i.e. we still need a way to authenticate as root in order to create the limited users.

@bryantbiggs
Copy link
Member Author

that link didn't seem to work as expected, but its on main

output "cluster_master_user_secret" {
description = "The generated database master user secret when `manage_master_user_password` is set to `true`"
value = try(aws_rds_cluster.this[0].master_user_secret, null)
}

@github-actions
Copy link

github-actions bot commented Jun 3, 2023

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants