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!: Aligned the behaviour of additional_users resource in all 3 Cloud SQL instance modules. #398

Merged
merged 31 commits into from
Jan 13, 2023

Conversation

ravisiddhu
Copy link
Contributor

@ravisiddhu ravisiddhu commented Dec 22, 2022

This PR is in response to #222

Buganizer bug : http://b/259914601

In mssql module, there was no way to generate random password for the user. the password was a required field, hence couldn't omit it.
If password is set as null, then no password is created for the user, and if the password is set as "", then the password is also literally "" (empty password).

I am aligning the behaviour of additonal_users resource in all 3 cloud sql modules.

This PR include BREAKING CHANGES :-

  1. Addition of new field called random_password in additional_users resource.
  2. Added support for creation of IAM users in MySQL.
  3. The same modified additional_users resource is used in all the 3 modules.

Please merge this PR with breaking changes only when we are releasing the next major version.

@ravisiddhu ravisiddhu requested a review from a team as a code owner December 22, 2022 13:03
ravisiddhu and others added 7 commits December 28, 2022 16:16
…e there are some sensitive fields in google_sql_database_instance resource.
…le, since there are some sensitive fields in google_sql_database_instance resource."

This reverts commit 6c3aeef.
@ravisiddhu ravisiddhu changed the title Made the behaviour of additional_users resource in mssql module identical to mysql module fix: made the behaviour of the 'additional_users' resource in mssql module identical to mysql module Dec 28, 2022
Comment on lines 240 to 241
type = list(object({
name = string
password = string
}))
default = []
type = list(map(any))
default = []
Copy link
Member

Choose a reason for hiding this comment

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

We had received some feedback that not having vars typed hurt UX as users were unable to determine what values to pass in. WDYT about switching to use optionals instead? This will be breaking change requiring 1.3.0 but we expect most modules to switch to use optionals next year.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we should force all our customers to upgrade terraform to 1.3.0+, for a small fearure fix. I think we should wait and only cause this breaking change when it is absolutely necessary, wdyt?

As an alternate solution we can have one more resource called "additional_users_with_random_password", where users can set null or "" to the password field to generate random password (replicating the behavior of postgres module) in mssql module. In this way we are not breaking anything for the exisiting mssql customers, what do you think? Should I go forward with this?

Copy link
Member

Choose a reason for hiding this comment

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

We will likely switch to 1.3 in the next breaking release as we had multiple breaking major releases (12.0,11.0,10.0,8.0) due to changes in object types which 1.3 will avoid. Agreed we don't need to force the switch for this feature, I was thinking of bundling this with next major if we decided to use optionals.

With the alternate approach using additional_users_with_random_password, would we need a new input to target this? Also re reading the description, I suspect most users may not want to use a literal "" since no password is supported by null. It might just be cleaner to not support literal "".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting password as "", creates an user with an empty password, then that user can login without providing password.

  1. Should I use Optionals and send the PR, where you can merge this PR once we decide to create a breaking release. or
  2. With additional_users_with_random_password, yes, it would be a new input field for the customer. Now mssql customers could either use additonal_users or additonal_users_with_random_password input fields for creating users. The mssql customers can use the new input if they require their users to be set with an random password (and they still would have the option to change the random password if needed. This new field would replicate the behaviour of additional_users field in postgres module).

Should I go ahead with (1) or (2) ?

Copy link
Member

Choose a reason for hiding this comment

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

If password is set as null, then no password is created for the user, and if the password is set as "", then the password is also literally "" (empty password).

Setting password as "", creates an user with an empty password, then that user can login without providing password.

In that case, what is the difference between empty string "" as password vs no password at all via null?

I am wondering even with optionals we will need to provide a default value signifying when to generate the random password - would we use empty string or null?

Copy link
Contributor Author

@ravisiddhu ravisiddhu Jan 5, 2023

Choose a reason for hiding this comment

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

In postgres module, random password is generated during empty string and null values for password.

In MySQL, random password is generated only when password field is not set in the additional_users resource, since additional_users is of type list(map(any), it is possible to not set the pwd field.

In mssql, random password is never generated, and a login user without pwd is created if the customer set the pwd as null or "".

I tried using "Optionals", but optionals also just force set the value of unset optional fields as null. So again if the customer does not set the password, it would only create a login user without pwd.

If we change the existing behaviour in mssql, that is to produce a random password for the user if the customer sets the password as null or "", then it would be a breaking change if the customers were in fact actually setting null or "" to pwd field to create a password-less login users.

I think creating a new resource called "additional_users_with_random_password" in mssql module would help to create users with random pwd without any breaking change. what do you think?

ravisiddhu and others added 2 commits January 2, 2023 18:28
… for creating users with random password if password is set as null or an empty string.
@ravisiddhu ravisiddhu changed the title fix: made the behaviour of the 'additional_users' resource in mssql module identical to mysql module feat: Added new resource called additional_users_with_random_password in mssql module Jan 5, 2023
@ravisiddhu ravisiddhu changed the title feat: Added new resource called additional_users_with_random_password in mssql module eat: Aligned the behaviour of additional_users resource in all 3 Cloud SQL instance modules. Jan 9, 2023
@ravisiddhu ravisiddhu changed the title eat: Aligned the behaviour of additional_users resource in all 3 Cloud SQL instance modules. feat: Aligned the behaviour of additional_users resource in all 3 Cloud SQL instance modules. Jan 9, 2023
@comment-bot-dev
Copy link

@ravisiddhu
Thanks for the PR! 🚀
✅ Lint checks have passed.

@bharathkkb bharathkkb changed the title feat: Aligned the behaviour of additional_users resource in all 3 Cloud SQL instance modules. feat!: Aligned the behaviour of additional_users resource in all 3 Cloud SQL instance modules. Jan 13, 2023
@bharathkkb bharathkkb merged commit 7d6b209 into terraform-google-modules:master Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants