Skip to content
This repository was archived by the owner on Oct 27, 2025. It is now read-only.

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
Contributor

@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
| <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
Contributor

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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for passwords using ec2ParameterStore

3 participants