-
Notifications
You must be signed in to change notification settings - Fork 4
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
base: master
Are you sure you want to change the base?
Add support for snowflake passwords stored in AWS SSM parameter store #20
Conversation
a37cd9f
to
47c6cd2
Compare
There was a problem hiding this 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 | |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Ah, good catch. I had attached |
47c6cd2
to
d0edf95
Compare
Statement to allow 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. |
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.