Add config option [secrets]backends_order #45931
Conversation
61b5ab6 to
4489808
Compare
4489808 to
58d80f3
Compare
|
@eladkal , please take a look at the updates. |
|
I was initially against making it configurable, but seeing the simplicity and flexibility, I am in. |
|
@eladkal ? |
|
hi there! |
|
We are on feature freeze for Airflow 3. |
|
Yeah. I think that might be 3.1 |
8c516f8 to
347e2f1
Compare
| 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" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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(
There was a problem hiding this comment.
Hi there!
I see that we are close to release Af3.1, so maybe we can merge this change?
Thanks :)
347e2f1 to
2b750de
Compare
2b750de to
4c0d958
Compare
|
This looks good to me - but I think it might be worth to raise a devlist discussion for it @VladaZakharova -> there were past discussions about changing the sequence of resolving configurations, and I know people have strong opinion about "fixed" vs. "confifurable" sequence - and there are arguments pro / against each of those options. I think it would be good to raise a discussion asking what peopel think about it and try to reach consensus. |
I agree. I am not comfortable with making this change without the UI indication / other mechanisem that allows dag authors to see the cluster admin setup for backend order |
Introduce a new configuration option for specifying secret backends load order:
The default value represents current behavior, thus nothing will change for existing users.