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

Add support for snowflake passwords stored in AWS SSM parameter store #20

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

thomas-brx
Copy link

@thomas-brx thomas-brx commented Aug 2, 2023

This PR fixes #18 by adding support for a new variable: snowflake_password_from_parameter_store_name.

The variable is validated according to the AWS documentation, but ignoring some rules like A parameter name can't be prefixed with "aws" or "ssm".

Caveat: Does add a policy statement to allow access to the parameter, but does not add any policy to decrypt the parameter if it's stored as SecureString.

@thomas-brx thomas-brx force-pushed the support-for-parameter-store-passwords branch from a37cd9f to 47c6cd2 Compare August 8, 2023 07:19
@jbeemster jbeemster self-requested a review August 11, 2023 01:45
Copy link
Member

@jbeemster jbeemster left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution here! Would be great if you can add a new variable for the parameter store ARN instead of overloading the existing password that would make it much clearer how to leverage this option.

I would also have assumed that the IAM Policy needs to be allowed Get access to the supplied parameter? Can you add the relevant change to the IAM Policy object in main.tf to manage that allowance conditionally?

README.md Outdated
@@ -167,7 +169,7 @@ module "sf_loader" {
| <a name="input_snowflake_aws_s3_bucket_name"></a> [snowflake\_aws\_s3\_bucket\_name](#input\_snowflake\_aws\_s3\_bucket\_name) | AWS bucket name where data to load is stored | `string` | n/a | yes |
| <a name="input_snowflake_database"></a> [snowflake\_database](#input\_snowflake\_database) | Snowflake database name | `string` | n/a | yes |
| <a name="input_snowflake_loader_user"></a> [snowflake\_loader\_user](#input\_snowflake\_loader\_user) | Snowflake username used by loader to perform loading | `string` | n/a | yes |
| <a name="input_snowflake_password"></a> [snowflake\_password](#input\_snowflake\_password) | Password for snowflake\_loader\_user used by loader to perform loading | `string` | n/a | yes |
| <a name="input_snowflake_password"></a> [snowflake\_password](#input\_snowflake\_password) | Snowflake password as a string or an object `{ ec2ParameterStore: { parameterName: "<parameter name>" }}`, for snowflake_loader_user used by loader to perform loading | `string\|map` | n/a | yes |
Copy link
Member

Choose a reason for hiding this comment

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

Rather than overloading this parameter can you instead just add a new variable of snowflake_password_from_parameter_store_arn - if this is populated it can favor using this as the option and the config can be updated conditionally?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback. That's what I initially did before 47c6cd2, I will revert back.
It's not an ARN, so will use snowflake_password_from_parameter_store_name, if that's ok?

Copy link
Author

Choose a reason for hiding this comment

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

Dropped that commit, and renamed the parameter to match the suggestion.

@thomas-brx
Copy link
Author

I would also have assumed that the IAM Policy needs to be allowed Get access to the supplied parameter? Can you add the relevant change to the IAM Policy object in main.tf to manage that allowance conditionally?

Ah, good catch. I had attached AmazonSSMManagedInstanceCore to the IAM role, so was just working for me. Will update main.tf 👍

@thomas-brx thomas-brx force-pushed the support-for-parameter-store-passwords branch from 47c6cd2 to d0edf95 Compare August 11, 2023 06:28
@thomas-brx
Copy link
Author

I would also have assumed that the IAM Policy needs to be allowed Get access to the supplied parameter? Can you add the relevant change to the IAM Policy object in main.tf to manage that allowance conditionally?

Ah, good catch. I had attached AmazonSSMManagedInstanceCore to the IAM role, so was just working for me. Will update main.tf 👍

Statement to allow ssm:GetParameter for the configured parameter has been added.

Caveat - does not add statements to allow decryption of SecureString parameters - this would require knowing which KMS key was used. Let me know if I you think I should add support for that as well.

@thomas-brx thomas-brx requested a review from jbeemster August 21, 2023 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for passwords using ec2ParameterStore
3 participants