You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/11/25 18:34:06 UTC
[GitHub] [airflow] dwreeves opened a new pull request, #27920: 26571 aws secrets manager update
dwreeves opened a new pull request, #27920:
URL: https://github.com/apache/airflow/pull/27920
Closes: #26571
This PR contains the following:
* Implemented the following bullet points outlined in #26571:
* Removed `ast.literal_eval` for deserializing JSON secrets.
* Removed `full_url_mode` kwarg entirely. JSONs are now automatically detected.
* Removed requirement that `get_conn_value()` must return a URI.
* **Note that this means that the AWS provider package will no longer work with Airflow 2.2.x and requires 2.3.0 or greater.**
* Removed requirement that secrets must be URL-encoded when stored as JSONs.
* **Note that this change will be breaking for some users, although the current AWS provider package should be raising `DeprecationWarning`s for users who will be affected.**
* Set `login` as the default name for the login with the JSON standardization thing in `_standardize_secret_keys`.
* Updated docs to reflect these changes.
* Removed all references to full URL mode.
* Provided what I believe to be much clearer examples of AWS Secrets Manager setup.
* Moved the docs to being more consistent with Airflow's base secrets API, e.g. `login` instead of `user`, `password` instead of `pass`, etc. + I wrote a note saying that using these names is encouraged.
* I also raised the header levels a little bit across the doc; they were very low!
As stated in #26571, the intent with this PR (as well as the one leading up to it that added a bunch of deprecation warnings) is to make it so the secret values accepted by the `SecretsManagerBackend` are a strict superset of what is allowed by the `BaseSecretsBackend`, and behave in the same way, so as to adhere to the principle of behavioral subtyping.
For both the reasons stated above in bold-- this is incompatible with Airflow 2.2.x, and a small number of users will be hit by backwards incompatibility after implementing this change-- merging this PR would require a major version bump of the Amazon provider package.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on a diff in pull request #27920: AWS Secrets Manager Backend - major update
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on code in PR #27920:
URL: https://github.com/apache/airflow/pull/27920#discussion_r1039910932
##########
tests/providers/amazon/aws/secrets/test_secrets_manager.py:
##########
@@ -52,14 +52,14 @@ def test_get_conn_value_full_url_mode(self):
assert "postgresql://airflow:airflow@host:5432/airflow" == returned_uri
@pytest.mark.parametrize(
- "full_url_mode, login, host",
+ "are_secret_values_urlencoded, login, host",
[
- (False, "is url encoded", "not%20idempotent"),
- (True, "is%20url%20encoded", "not%2520idempotent"),
+ (True, "is url encoded", "not%20idempotent"),
+ (False, "is%20url%20encoded", "not%2520idempotent"),
Review Comment:
Got it, thanks for the explanation @dwreeves!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dwreeves commented on pull request #27920: AWS Secrets Manager Backend - major update
Posted by GitBox <gi...@apache.org>.
dwreeves commented on PR #27920:
URL: https://github.com/apache/airflow/pull/27920#issuecomment-1338239720
I'm sorry this PR opened such a can of worms regarding major version releases. 😅 This all just started as a passion project to fix something I considered counterintuitive that I thought many others may also find counterintuitive. I use the word "fix" here because I do believe there were a lot of genuine problems with the initial handling of JSON secrets that look especially odd in a 2.3 world where JSON secrets are first-class citizens of the base Airflow API.
I tried my best to smooth out the transition to a more sensible system across my two PRs that address the problem. I also believe the approaches I took were the best way to do that, both in terms of the path going forward as well as the intermediate steps I took with my first PR to the `SecretsManagerBackend`. I know future Airflow users will find the SecretsManager much easier to work with, and I hope current users do not find these changes too disruptive.
Thank you all once again for reviewing my PRs, I'll probably keep contributing to Airflow in the future although I sure hope it is in a less "breaking changes" capacity.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #27920: AWS Secrets Manager Backend - major update
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #27920:
URL: https://github.com/apache/airflow/pull/27920
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on pull request #27920: AWS Secrets Manager Backend - major update
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #27920:
URL: https://github.com/apache/airflow/pull/27920#issuecomment-1481638387
Cherry picked in 2.5.3 to resolve conflicts
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #27920: AWS Secrets Manager Backend - major update
Posted by GitBox <gi...@apache.org>.
kaxil commented on PR #27920:
URL: https://github.com/apache/airflow/pull/27920#issuecomment-1327942456
cc @ferruzzi @o-nikolas @vincbeck
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on a diff in pull request #27920: AWS Secrets Manager Backend - major update
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on code in PR #27920:
URL: https://github.com/apache/airflow/pull/27920#discussion_r1033839864
##########
tests/providers/amazon/aws/secrets/test_secrets_manager.py:
##########
@@ -52,14 +52,14 @@ def test_get_conn_value_full_url_mode(self):
assert "postgresql://airflow:airflow@host:5432/airflow" == returned_uri
@pytest.mark.parametrize(
- "full_url_mode, login, host",
+ "are_secret_values_urlencoded, login, host",
[
- (False, "is url encoded", "not%20idempotent"),
- (True, "is%20url%20encoded", "not%2520idempotent"),
+ (True, "is url encoded", "not%20idempotent"),
+ (False, "is%20url%20encoded", "not%2520idempotent"),
Review Comment:
This seems backwards?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on pull request #27920: AWS Secrets Manager Backend - major update
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on PR #27920:
URL: https://github.com/apache/airflow/pull/27920#issuecomment-1338277364
> I'm sorry this PR opened such a can of worms regarding major version releases. sweat_smile
Absolutely no worries Daniel! These were great changes that were made very carefully with fantastic communication. The discussions on when and how often to release code is a discussion as old as time and was going on before your changes. I hope to see you around as a contributor in Airflow for years to come :smiley:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #27920: AWS Secrets Manager Backend - major update
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27920:
URL: https://github.com/apache/airflow/pull/27920#issuecomment-1338244358
Well. Discussion is not opening can of worms. It's often a good thing to have, as long as it results in an action (merging it is an action, so all good)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on pull request #27920: AWS Secrets Manager Backend - major update
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on PR #27920:
URL: https://github.com/apache/airflow/pull/27920#issuecomment-1338347160
> I sure hope it is in a less "breaking changes" capacity.
Sometimes it's simply the best way forward. Thanks for the contribution!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #27920: AWS Secrets Manager Backend - major update
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #27920:
URL: https://github.com/apache/airflow/pull/27920#discussion_r1039048143
##########
tests/providers/amazon/aws/secrets/test_secrets_manager.py:
##########
@@ -52,14 +52,14 @@ def test_get_conn_value_full_url_mode(self):
assert "postgresql://airflow:airflow@host:5432/airflow" == returned_uri
@pytest.mark.parametrize(
- "full_url_mode, login, host",
+ "are_secret_values_urlencoded, login, host",
[
- (False, "is url encoded", "not%20idempotent"),
- (True, "is%20url%20encoded", "not%2520idempotent"),
+ (True, "is url encoded", "not%20idempotent"),
+ (False, "is%20url%20encoded", "not%2520idempotent"),
Review Comment:
This one is nice - @o-nikolas - what's your take ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #27920: AWS Secrets Manager Backend - major update
Posted by GitBox <gi...@apache.org>.
eladkal commented on PR #27920:
URL: https://github.com/apache/airflow/pull/27920#issuecomment-1337992194
> We've had a lot of major version bumps lately on the Amazon Provider Package. Should we wait until there are few more breaking changes and batch them together? What do others think?
I don't see reason to wait.
Providers suppose to move fast.
Noting that if AWS see the need to backport features/bug fixes to earlier provider version that is possible (And even simple). The procedure is documented in https://github.com/apache/airflow#release-process-for-providers (last paragraph of that section - cherry picking)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on pull request #27920: AWS Secrets Manager Backend - major update
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on PR #27920:
URL: https://github.com/apache/airflow/pull/27920#issuecomment-1338226112
> > We've had a lot of major version bumps lately on the Amazon Provider Package. Should we wait until there are few more breaking changes and batch them together? What do others think?
>
> I don't see reason to wait (?) if AWS see the need to backport features/bug fixes to earlier provider version that is possible (And even simple). The procedure is documented in https://github.com/apache/airflow#release-process-for-providers (last paragraph of that section - cherry picking)
It's not the end of the world of course, but it does add overhead for contributors maintaining the code (backporting, etc) as well as users who have to migrate very frequently. The same reasons apply for why we don't release a major version of Airflow weekly (albeit at a much smaller scale).
But I can't seem to get on the same page with release schedule and semver around here, so I should probably stop trying :sweat_smile:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dwreeves commented on a diff in pull request #27920: AWS Secrets Manager Backend - major update
Posted by GitBox <gi...@apache.org>.
dwreeves commented on code in PR #27920:
URL: https://github.com/apache/airflow/pull/27920#discussion_r1035442271
##########
tests/providers/amazon/aws/secrets/test_secrets_manager.py:
##########
@@ -52,14 +52,14 @@ def test_get_conn_value_full_url_mode(self):
assert "postgresql://airflow:airflow@host:5432/airflow" == returned_uri
@pytest.mark.parametrize(
- "full_url_mode, login, host",
+ "are_secret_values_urlencoded, login, host",
[
- (False, "is url encoded", "not%20idempotent"),
- (True, "is%20url%20encoded", "not%2520idempotent"),
+ (True, "is url encoded", "not%20idempotent"),
+ (False, "is%20url%20encoded", "not%2520idempotent"),
Review Comment:
The test is basically saying:
- If the values are URL-encoded (True), then the expected value will have the escaping removed `"is%20url%20encoded" -> "is url encoded"`
- If the values are _not_ URL-encoded (False), then the expected value will not have the escaping removed `"is%20url%20encoded" -> "is%20url%20encoded"`
Admittedly this is very confusing, not least of all because I am flipping the kwarg `full_url_mode` to `are_secret_values_urlencoded`.
This was a really weirdly specified test, looking back at it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org