Re: [PR] Add config option [secrets]backends_order [airflow]
eladkal commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3023344132 > 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 -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add config option [secrets]backends_order [airflow]
potiuk commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3022748078 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. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add config option [secrets]backends_order [airflow]
VladaZakharova commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r2174662212 ## airflow-core/src/airflow/config_templates/config.yml: ## @@ -1265,6 +1265,20 @@ secrets: sensitive: true example: ~ default: "" +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" Review Comment: Hi there! I see that we are close to release Af3.1, so maybe we can merge this change? Thanks :) -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add config option [secrets]backends_order [airflow]
eladkal commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r2023517222 ## airflow-core/src/airflow/config_templates/config.yml: ## @@ -1265,6 +1265,20 @@ secrets: sensitive: true example: ~ default: "" +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" Review 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( -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r2022943159 ## airflow-core/src/airflow/config_templates/config.yml: ## @@ -1265,6 +1265,20 @@ secrets: sensitive: true example: ~ default: "" +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" Review 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? -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add config option [secrets]backends_order [airflow]
eladkal commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r2021018760 ## airflow-core/src/airflow/config_templates/config.yml: ## @@ -1265,6 +1265,20 @@ secrets: sensitive: true example: ~ default: "" +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" Review 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?` -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add config option [secrets]backends_order [airflow]
potiuk commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-2661094214 Yeah. I think that might be 3.1 -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add config option [secrets]backends_order [airflow]
eladkal commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-2656887152 We are on feature freeze for Airflow 3. https://lists.apache.org/thread/r26htzl0w3th7pw0l1y31g6s14qbtwwt -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add config option [secrets]backends_order [airflow]
VladaZakharova commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-2656190070 Hi @potiuk @eladkal ! Are there some other changes we need to make here? Or we can merge this one? -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add config option [secrets]backends_order [airflow]
VladaZakharova commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-2639426046 hi there! @potiuk Can we merge this one please? -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add config option [secrets]backends_order [airflow]
potiuk commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-2614079232 @eladkal ? -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add config option [secrets]backends_order [airflow]
potiuk commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-2614079156 I was initially against making it configurable, but seeing the simplicity and flexibility, I am in. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add config option [secrets]backends_order [airflow]
moiseenkov commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-2609151622 @eladkal , please take a look at the updates. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add config option [secrets]backends_order [airflow]
moiseenkov commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r1925348243 ## airflow/config_templates/config.yml: ## @@ -1296,6 +1296,15 @@ secrets: sensitive: true example: ~ default: "" +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`, `environment_variable`, `metastore`. + version_added: 2.10.5 + type: string + example: ~ + default: "custom,environment_variable,metastore" Review Comment: Thanks for noticing that - I updated the documentation and the new option description. Speaking of breaking changes, I'm pretty sure that the default behavior is identical the current one, because the existing implementation used to check any custom backend first, and then load `EnvironmentVariablesBackend` and `MetastoreBackend`. ```python DEFAULT_SECRETS_SEARCH_PATH = [ "airflow.secrets.environment_variables.EnvironmentVariablesBackend", "airflow.secrets.metastore.MetastoreBackend", ] ... def initialize_secrets_backends() -> list[BaseSecretsBackend]: ... backend_list = [] custom_secret_backend = get_custom_secret_backend() if custom_secret_backend is not None: backend_list.append(custom_secret_backend) for class_name in DEFAULT_SECRETS_SEARCH_PATH: secrets_backend_cls = import_string(class_name) backend_list.append(secrets_backend_cls()) return backend_list ``` -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add config option [secrets]backends_order [airflow]
moiseenkov commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r1925339026 ## airflow/config_templates/config.yml: ## @@ -1296,6 +1296,15 @@ secrets: sensitive: true example: ~ default: "" +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`, `environment_variable`, `metastore`. + version_added: 2.10.5 Review Comment: Thanks for pointing that out. Changed to the version 3.0.0. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add config option [secrets]backends_order [airflow]
eladkal commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r1925299794 ## airflow/config_templates/config.yml: ## @@ -1296,6 +1296,15 @@ secrets: sensitive: true example: ~ default: "" +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`, `environment_variable`, `metastore`. + version_added: 2.10.5 + type: string + example: ~ + default: "custom,environment_variable,metastore" Review Comment: isn't this breaking change? What is custom? Is it for rolling secret backend? The current order is not the same as you listed: > If you enable an alternative secrets backend, it will be searched first, followed by environment variables, then metastore. This search ordering is not configurable. Also you need to make doc changes to explain about this new functionality https://airflow.apache.org/docs/apache-airflow/stable/security/secrets/secrets-backend/index.html#search-path ## airflow/config_templates/config.yml: ## @@ -1296,6 +1296,15 @@ secrets: sensitive: true example: ~ default: "" +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`, `environment_variable`, `metastore`. + version_added: 2.10.5 Review Comment: This can not be in 2.10.5 nor in 2.11 it's a new feature. Will have to wait for Airflow 3. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
