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

Fix regression in cos-mysql kms_data variable introduced in #31 #46

Merged
merged 4 commits into from
Oct 13, 2019

Conversation

ludoo
Copy link
Contributor

@ludoo ludoo commented Oct 12, 2019

Setting types on the cos_mysql submodule variables was done incorrectly, with kms_data missing a member and KMS becoming unusable in the module. This fixes it by only setting the type declaration to a generic string map, so that all members can be passed in, and the variable is optional (as it should be when no password decryption is required).

@ludoo ludoo requested a review from averbuks October 12, 2019 17:00
Copy link
Member

@averbuks averbuks left a comment

Choose a reason for hiding this comment

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

Hi,

  • I was not able to find any place where some other keys of kms_data are used (except project_id, key, keyring, location). You mentioned members, but I could not find that. The question why should we do 50ef682 (sorry if I'm missing something).
  • I think the generated docs should be updated.

@ludoo
Copy link
Contributor Author

ludoo commented Oct 13, 2019

Members is a generic term for "keys and values in the map". :)

The main point is that KMS is only used if this is true (in locals):

use_kms        = var.password != "" && length(var.kms_data) == 4

So, three things this fixes:

  • make it so that the kms_data variable can have four key/value pairs instead of being forced to three by the type, which force disabled KMS decryption whatever the input variable
  • allow passing in the project_id key/value pair that is needed by the script doing KMS decryption on the instance (type excluded it, so even if passed in it was discarded during type conversion)
  • remove the type so kms_data can be left out, if KMS decryption is not needed

The previous commit broke this module's use of KMS decryption, password was considered plaintext regardless of what was passed in kms_data, and it also made it a required variable, which is incorrect.

@ludoo ludoo requested a review from averbuks October 13, 2019 07:30
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.

2 participants