Add support for managed master credentials #123
Conversation
Added one additional nullable variable that will allow users to manage their master credentials using AWS Secrets Manager. This is a native feature of the core resource.
Updated the documentation to be a bit more clear and aligned with the actual documentation.
Was originally set to check whether `manage_master_user_password` is null, which is not the correct test.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for managing master credentials using AWS Secrets Manager within the DocumentDB module.
- Added a new variable "manage_master_user_password" to allow toggling credential management.
- Updated outputs and main resource logic to account for this new feature.
- Modified module enablement conditions for DNS and SSM modules accordingly.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| variables.tf | New variable introduced to allow managed master user password handling. |
| outputs.tf | Updated master_password output to return null when managed externally. |
| main.tf | Added a parameter to the DNS and SSM modules and updated password creation logic. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
/terratest |
Co-authored-by: Ben <ben.smith.developer@gmail.com>
Head branch was pushed to by a user without write access
|
/terratest |
|
/terratest |
|
/terratest |
- Simplify logic for managing master password and Secrets Manager - Set default for manage_master_user_password to false - Clarify variable description and output behavior
|
/terratest |
| # If `master_password` or `manage_master_user_password` is set, the other MUST be set to null, otherwise it will cause an error. | ||
| master_password = var.manage_master_user_password ? null : local.master_password | ||
| manage_master_user_password = var.manage_master_user_password ? true : null |
There was a problem hiding this comment.
@Benbentwo I've altered the approach here a bit.
manage_master_user_password is now a bool with a default value of false from the get-go. This simplifies the logic that needs to be used throughout the module.
The major change here, is that one of manage_password and manage_master_user_password will be null.
The sacrifice that I needed to make for this to work, however, is that one of the two parameters will need to take priority over the other.
There was a problem hiding this comment.
Hmm, I'm thinking over if this is the right approach.
My concern with this pattern is as you described - the manage_master_user_password variable takes priority which can make it confusing why master_password isn't being used. The hard part here is the resource essentially wants a boolean value that is either true or null. I'm wondering if it's better to leave both as null, or alternatively, error out as terraform does if both are provided rather than switching it for the user so that it's less confusing why something isn't working.
Can I open a PR into your PR and see if I can get something to work, then perhaps you can test on your side if it works for you?
what
why
references