AWS Secrets Manager Backend - major update

[dwreeves]

Closes: #26571

This PR contains the following:

• Implemented the following bullet points outlined in Migrate Amazon Provider Package's SecretsManagerBackend's full_url_mode=False implementation. #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 DeprecationWarnings 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.

[kaxil]

cc @ferruzzi @o-nikolas @vincbeck

[vincbeck]

Love to see this simplification! LGTM!

[o-nikolas]

I remember this, nice PR!

We'll need to be cognizant of when we merge it since it will cause a major version bump, as you say.

[o-nikolas]

This seems backwards?

[dwreeves]

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.

[potiuk]

This one is nice - @o-nikolas - what's your take ?

[o-nikolas]

Got it, thanks for the explanation @dwreeves!

[potiuk]

Looks cool, but I think I need few more eyes (@o-nikolas mainly) to take a look

[o-nikolas]

Approving!
I still think we could be strategic about when we merge this though. 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?

[o-nikolas]

Got it, thanks for the explanation @dwreeves!

[eladkal]

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)

[o-nikolas]

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 😅

[dwreeves]

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.

[potiuk]

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)

[o-nikolas]

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 😃

[ferruzzi]

I sure hope it is in a less "breaking changes" capacity.

Sometimes it's simply the best way forward. Thanks for the contribution!

[pierrejeambrun]

Cherry picked in 2.5.3 to resolve conflicts for #29580.

Cf #release-management, #29580 were dropped from 2.5.3