Skip to content

Add config option [secrets]backends_order #45931

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

moiseenkov
Copy link
Contributor

Introduce a new configuration option for specifying secret backends load order:

[secrets]backends_order = custom,environment_variable,metastore

The default value represents current behavior, thus nothing will change for existing users.

@moiseenkov moiseenkov force-pushed the secrets-backends-order branch from 61b5ab6 to 4489808 Compare January 22, 2025 13:38
@moiseenkov moiseenkov requested a review from eladkal January 22, 2025 13:45
@moiseenkov moiseenkov force-pushed the secrets-backends-order branch from 4489808 to 58d80f3 Compare January 22, 2025 14:35
@moiseenkov
Copy link
Contributor Author

@eladkal , please take a look at the updates.

@potiuk
Copy link
Member

potiuk commented Jan 25, 2025

I was initially against making it configurable, but seeing the simplicity and flexibility, I am in.

@potiuk
Copy link
Member

potiuk commented Jan 25, 2025

@eladkal ?

@VladaZakharova
Copy link
Contributor

hi there!
@potiuk
Can we merge this one please?

@VladaZakharova
Copy link
Contributor

Hi @potiuk @eladkal ! Are there some other changes we need to make here? Or we can merge this one?

@eladkal
Copy link
Contributor

eladkal commented Feb 13, 2025

We are on feature freeze for Airflow 3.
https://lists.apache.org/thread/r26htzl0w3th7pw0l1y31g6s14qbtwwt

@potiuk
Copy link
Member

potiuk commented Feb 15, 2025

Yeah. I think that might be 3.1

@Crowiant Crowiant force-pushed the secrets-backends-order branch 3 times, most recently from 8c516f8 to 347e2f1 Compare March 31, 2025 13:00
Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prevent accidental merge. We are on feature freeze for Airflow 3.
PR can not be merged till main branch is for 3.1

@eladkal eladkal added this to the Arflow 3.1+ milestone Mar 31, 2025
Comment on lines +1268 to +1272
backends_order:
description: |
Comma-separated list of secret backends. These backends will be used in the order they are specified.
Please note that the `environment_variable` and `metastore` are required values and cannot be removed
from the list. Supported values are:

* ``custom``: Custom secret backend specified in the ``secrets[backend]`` configuration option.
* ``environment_variable``: Standard environment variable backend
``airflow.secrets.environment_variables.EnvironmentVariablesBackend``.
* ``metastore``: Standard metastore backend ``airflow.secrets.metastore.MetastoreBackend``.
version_added: 3.0.0
type: string
example: ~
default: "custom,environment_variable,metastore"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am a bit concerned making it just a "hidden" setting.
I think we better to come up with a way to expose the chosen order in the UI. that way DAG authors can verify and use it for debug for questions like (why I see wrong value in variable).

Also, correct me if I am wrong the order affect only read, not write. maybe the setting should be backends_read_order?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @eladkal ! I don't see any info regarding the secrets section the UI is using right now. So maybe we could skip it for now? What do you think?
Regarding the naming. As stated in the doc it is responsible for secrets backends search ordering. So maybe backends_search_order is more convenient?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right now it's not configurable so the Airflow docs is good enough but after this PR is accepted the Airflow docs can't help you... You'll need to check the deployment to understand the priority order.
In many deployments dag authors don't have access to the deployment code.

My thoughts on the UI is an improvement (I think crucial but that is just my own perspective) I welcome others to share their thoughts. This feature is already targeting 3.1+ so regardless if we do the UI part or not we can't merge it now. We need to wait for main branch to become 3.1 (probably only after we cut RC1 for Airflow 3(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi there!
I see that we are close to release Af3.1, so maybe we can merge this change?
Thanks :)

@eladkal eladkal modified the milestones: Airflow 3.1+, Airflow 3.1.0 Apr 21, 2025
@Crowiant Crowiant force-pushed the secrets-backends-order branch from 347e2f1 to 2b750de Compare April 24, 2025 12:21
@Crowiant Crowiant force-pushed the secrets-backends-order branch from 2b750de to 4c0d958 Compare May 23, 2025 14:52
@potiuk potiuk requested a review from eladkal June 30, 2025 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants