-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
Decrypt secrets from SystemsManagerParameterStoreBackend #9214
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
|
How is this change backwards compatible? Does AWS automatically "fallback" in case of a non-secure string? |
Per the boto docs, it's a no-op on non-secure strings: https://docs.aws.amazon.com/goto/WebAPI/ssm-2014-11-06/GetParameter
|
@BasPH yes - if you set from the reference docs:
|
Nice, looks good. Could you add a test for |
Awesome work, congrats on your first merged pull request! |
(cherry picked from commit ffb8574)
(cherry picked from commit ffb8574)
(cherry picked from commit ffb8574)
(cherry picked from commit ffb8574)
This is my first PR, so pardon me if I missed any steps, but this proposed change is also very simple.
The current implementation of
SystemsManagerParameterStoreBackend
_get_secret
usesbut this only allows for secrets to be stored in clear text.
This PR changes the value to
WithDecryption=True
, which is backwards compatible with clear text values, but also supports KMS decryption in the API call.for reference in the API docs:
https://docs.aws.amazon.com/systems-manager/latest/APIReference/API_GetParameter.html
After reviewing the test coverage and the documentation, I don't think there are any changes that need to be made. The are no behavior changes for users other than this will support a storage option and will natively support tools like chamber cli tooling
I guess there could be an argument made to update documentation stating this feature is now supported?
Make sure to mark the boxes below before creating PR: [x]
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.