feat: Automatically disable `manage_master_user_password` when `password` is provided

[SoulMind01]

Description

This pull request adds functionality to automatically set manage_master_user_password to false when the user provides a manual password. This ensures that the manually provided password is respected and not overridden by Terraform when manage_master_user_password is set to true by default.

Motivation and Context

This change resolves an issue where users provide a password, but it is ignored because manage_master_user_password is set to true by default. By automatically setting manage_master_user_password to false when a password is provided, the module behaves more intuitively, using the password provided by the user.

This change addresses an issue discussed in #510, where the unexpected behavior caused confusion.

Breaking Changes

This change is backward-compatible. If no password is provided, the module will behave as it currently does, with manage_master_user_password defaulting to true. This ensures that the new behavior only affects users who explicitly provide a password.

How Has This Been Tested?

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

[bryantbiggs]

I'm not sure why there is confusion - there is an explicit variable manage_master_user_password which determines whether or not RDS will manage the password.

[MuhammedHMahmood]

The confusion comes when someone looks up the terraform module and sees that there is an input for the password without seeing that they need to set manage_master_user_password to false. I am currently teaching a class on cloud computing and instructed my students to use this module and set the master password through terraform. Many if not most failed to immediately catch that the variable manage_master_user_password must be set, despite terraform allowing the input of the password to succeed. This resulted in many students having to manually update their passwords in secrets manager or recreate their instances with manage_master_user_password = false. One of my students opened this PR and I think there IS a case to be made that this variable must be explicitly set for the module to apply a provided password. My own suggestion regarding this issue would be for the plan to error if a password is provided while manage_master_user_password is set to true. I can see how complications might arise if there is a manual input for manage_master_user_password = true and inputting password treats it as set to false.

[bryantbiggs]

so where is the confusion? in reading the input parameters or understanding what they do? I see you are a security devops engineer - isn't the recommended practice to let RDS manage this password so you can enable auto rotation and keep the password out of plain text in Terraform?

[MuhammedHMahmood]

The confusion would be in misreading the input parameters but also in the module for allowing someone to input a password, only to disregard it because manage_master_user_password is set to true by default. Why allow them to provide a password at all if it will not be used? It would just be better if the plan fails when a password is provided and manage_master_user_password isn't set to false. This makes the feedback loop much smaller for people who have a valid use case for keeping the password in terraform. Testing with RDS is a cumbersome process as it takes 20 minutes to create and up to 60 to destroy and recreate a testing environment if you provided it a password but didn't setmanage_master_user_password to false.

And on your other point, I agree. The recommended practice is to let RDS manage this password and keep passwords out of plain text in terraform. However, that's beside the point. The objective of a module shouldn't be to secretly enforce the recommended practices, but to allow its users to create what they expect efficiently.

I am not the only person who believes this is bad design, this issue was created but never addressed. https://github.com/terraform-aws-modules/terraform-aws-rds/issues/510

[bryantbiggs]

The objective of a module shouldn't be to secretly enforce the recommended practices, but to allow its users to create what they expect efficiently.

It is not doing anything secretly - it has an explicit variable.

[MuhammedHMahmood]

The variable is explicit, its definition is implicit. You can write a module which explicitly inputs a password, and the module will disregard it because the value of manage_master_user_password was implicitly set to true by default.

[bryantbiggs]

this is not implicit - its clearly defined as a default value https://github.com/terraform-aws-modules/terraform-aws-rds/blob/a76a3cd92220b91eaa467a5328db6f2c21e1fdee/variables.tf#L181

if it were implicit, it would be null and the implied value is that of API's default value

I'm sorry if you don't know how Terraform and Terraform modules work, but this is a common pattern repeated across all popular modules. Users need to look at the variables and their default values, and even when using a module for the first time, read the Terraform plan thoroughly to understand what is being configured. This is simply a mis-understanding on your part by thinking you can set a value and the module would accept it as is without understanding the context of the module's default values

[MuhammedHMahmood]

I'm not arguing in favor of the module accepting it without the user understanding the context. I'm arguing in favor of preventing a time-expensive mistake from occurring by throwing an error or warning during the plan to make it more obvious that the password is being disregarded. It's true that it falls on the user to thoroughly understand the output of their terraform plan and what configuration is being deployed. But these mistakes are easy to make in modules with hundreds of inputs and time-expensive to fix, so I don't see how preventing them from being made is a bad pattern for modules.

[MuhammedHMahmood]

After looking into it further, this is the functionality for the underlying call to aws_db_instance if manage_master_user_password is set to true and a password is also provided.

I respect that you are the maintainer of this module and have been very active, so I defer to your knowledge. I still am curious though as to why you believe it is better for the module to make a decision to disregard the password instead of allowing it to fail at the level where the aws_db_instance resource is being defined. I'd like to see myself and my students contribute to these projects in the future and I would benefit greatly from an experienced perspective.

[github-actions[bot]]

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.