Secrets backend failover

[fhoda]

1. Currently Airflow does not check the default secrets backends (env and metastore db) if there is any sort of connection related error to an Alternative Backend, causing related tasks to fail. The change proposed here allows it to fail over to checking the default backends when this happens.

2. Additionally GCP Secrets Manager causes the Airflow Webserver to crash at startup if credentials for the backend cannot be found. This behavior seems to be unique to GCP Secret Manager and this PR addresses that for parity in behavior regarding missing credentials across all backends.

closes: #14592

@kaxil

[fhoda]

Submitting for initial review. Assuming I will need to make changes based on feedback :)

[potiuk]

I believe the conclusion of our discussion was that in case of get_config use where Secrets Backend is missing-in-action we should raise an Exception. I gues It should be added here (including tests?)

[fhoda]

I believe the conclusion of our discussion was that in case of get_config use where Secrets Backend is missing-in-action we should raise an Exception. I gues It should be added here (including tests?)

@potiuk For get_config currently it causes airflow to crash if it cannot reach the Secrets Backend. This happens when get_config is called. If I understood properly we considered this behavior as expected so I left it alone.

[potiuk]

I believe the conclusion of our discussion was that in case of get_config use where Secrets Backend is missing-in-action we should raise an Exception. I gues It should be added here (including tests?)

@potiuk For get_config currently it causes airflow to crash if it cannot reach the Secrets Backend. This happens when get_config is called. If I understood properly we considered this behavior as expected so I left it alone.

But I think we should add a better message in this case and a test to cover this scenario explicitly. Then no-one will ever "fix it" thinking it is a bug.

[fhoda]

But I think we should add a better message in this case and a test to cover this scenario explicitly. Then no-one will ever "fix it" thinking it is a bug.

Ah yes, I understand now. Agree, was planning to add tests for all changes once I got feedback on the approach. I will implement your suggestion for get_config as well and then add tests.

[kaxil]

Thanks for the PR, can you fix the static checks and rebase on latest main @fhoda

[ashb]

Currently Airflow does not check the default secrets backends (env and metastore db) if there is any sort of connection related error to an Alternative Backend, causing related tasks to fail. The change proposed here allows it to fail over to checking the default backends when this happens.

This leads to some other possible bugs -- if you have different connection info in the DB to the external secrets store then sometimes/somewhat randomly you would end up with different crednetials returned.

I'm not sure that is a good behaviour...

[ashb]

If the goal is to protect against a misconfigured secrets backend only, then we should change it to test the settings somehow in get_custom_secret_backend, and if it's not working there, to not use it, rather than to put the try/except around every use of the secrets backends.

[potiuk]

I'm not sure that is a good behaviour...
If the goal is to protect against a misconfigured secrets backend only, then we should change it to test the settings somehow in get_custom_secret_backend, and if it's not working there, to not use it, rather than to put the try/except around every use of the secrets backends.

I quite agree. Just a bit of context @ashb

We had big discussion about it in #14592 and my original "extreme" statement was that we should crash Airflow if secret backend is unreachable (I had same concern).

Finally we reached a bit "softer" approach (which might allow the tasks to survive secret manager temporary unavailability if the user decides that this is the case). We decideed the crash should only happen when you read configuration (because there are default fallback values which might change airflow behaviour), but there might be some valid cases of fallback for connections or variables.

The idea here is that the user might then implement scenarios where the fallback values will be OK. It requires a deliberate action from the user - of defining both variable and connection fallback. There are no "defaults" for neither connections nor variables, so unless the users explicitly defines them, the task will not proceed/fail in case secret backend is unreachable (there will be no connection nor variable to use). While this is not as "clean" as ('crash whenever the backend is not available'), it makes sense to be implemented this way.

I'd love to hear your thoughts about it.

[kaxil]

This will raise the same error if there is any error getting variable from default secret backends (env var and db).

Let's separate this PR into 2:

1. GCP Change in airflow/providers/google/cloud/secrets/secret_manager.py
2. Broader Secrets Failover Change (with optional retries controlled via airflow.cfg 🤷 - that can even be a 3rd PR )

[fhoda]

I will split this PR up and reference accordingly as @kaxil suggested.