Re: [PR] Add config option [secrets]backends_order [airflow]
potiuk commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-4658517979 @moiseenkov — There are 16 unresolved review thread(s) on this PR, and you have engaged with each one (post-review commits and/or in-thread replies). Could you confirm whether you believe the feedback is fully addressed and the PR is ready for maintainer review confirmation? If yes, reply here (a short "yes / ready" is fine) and an Apache Airflow maintainer will pick the PR up from the review queue on the next sweep. If you are still working on a thread, please reply with what is outstanding so the threads stay unresolved on purpose. _Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this [two-stage triage process](https://github.com/apache/airflow/blob/main/contributing-docs/25_maintainer_pr_triage.md#why-the-first-pass-is-automated) so that our maintainers' limited time is spent where it matters most: the conversation with you._ -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-4542617086 Hello @potiuk @vatsrahul1001 could you please help with converting this PR back to open? CI seems to be ok. Also I responded to all comments from AI. But I can't resolve that threads due to permission issue. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
potiuk commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-4476281592 @moiseenkov This PR has been converted to **draft** because it has **merge conflicts with `main`** that prevent CI from running cleanly — see our [Pull Request quality criteria](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-quality-criteria). **Issues found:** - :x: **Merge conflicts**: this PR cannot be cleanly merged into `main`. Rebase your branch onto the latest `main`, resolve any conflicts locally, then force-push. See the [working-with-git docs](https://github.com/apache/airflow/blob/main/contributing-docs/10_working_with_git.rst). **What to do next:** - `git fetch upstream main && git rebase upstream/main`, resolve conflicts, then `git push --force-with-lease`. - Once the rebase is in and CI re-triggers cleanly, mark the PR as "Ready for review". Converting a PR to draft is **not** a rejection — it is an invitation to bring the PR up to the project's standards. No rush — take your time. If you have questions, feel free to ask on the [Airflow Slack](https://s.apache.org/airflow-slack). --- _Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this [two-stage triage process](https://github.com/apache/airflow/blob/main/contributing-docs/25_maintainer_pr_triage.md#why-the-first-pass-is-automated) so that our maintainers' limited time is spent where it matters most: the conversation with you._ -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-4449724358 > @moiseenkov Can you address all review comments ? Hello @vatsrahul1001 I’m currently working on this PR. I’ve responded to all the comments but don't have the permissions to resolve them. Could you please help me with that? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
vatsrahul1001 commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-4439493006 @moiseenkov Can you address all review comments ? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
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_r3225246844 ## airflow-core/src/airflow/secrets/__init__.py: ## @@ -29,12 +29,11 @@ from airflow.utils.deprecation_tools import add_deprecated_classes -__all__ = ["BaseSecretsBackend", "DEFAULT_SECRETS_SEARCH_PATH"] +__all__ = [ +"BaseSecretsBackend", +] -from airflow.secrets.base_secrets import ( -DEFAULT_SECRETS_SEARCH_PATH as DEFAULT_SECRETS_SEARCH_PATH, -BaseSecretsBackend, -) +from airflow.secrets.base_secrets import BaseSecretsBackend Review Comment: Earlier, reviewers didn't request this, so I think it is not strictly necessary -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
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_r3225238862
##
airflow-core/src/airflow/ui/src/pages/Variables/BackendsOrderCard.tsx:
##
@@ -0,0 +1,42 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Box, useDisclosure } from "@chakra-ui/react";
+import { useTranslation } from "react-i18next";
+import { LuSettings } from "react-icons/lu";
+
+import { BackendsOrderButton } from "src/pages/Variables/BackendsOrderButton";
+import { BackendsOrderModal } from "src/pages/Variables/BackendsOrderModal";
Review Comment:
fixed
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
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_r3225195023
##
airflow-core/src/airflow/configuration.py:
##
@@ -741,35 +740,107 @@ def get_custom_secret_backend(worker_mode: bool = False)
-> BaseSecretsBackend |
return conf._get_custom_secret_backend(worker_mode=worker_mode)
+def get_importable_secret_backend(class_name: str | None) ->
BaseSecretsBackend | None:
+"""Get secret backend defined in the given class name."""
+if class_name is not None:
+secrets_backend_cls = import_string(class_name)
+return secrets_backend_cls()
+return None
+
+
+class Backends(Enum):
+"""Type of the secrets backend."""
+
+ENVIRONMENT_VARIABLE = "environment_variable"
+EXECUTION_API = "execution_api"
+CUSTOM = "custom"
+METASTORE = "metastore"
+
+
def initialize_secrets_backends(
-default_backends: list[str] = DEFAULT_SECRETS_SEARCH_PATH,
+default_backends: list[str] | None = None,
) -> list[BaseSecretsBackend]:
"""
Initialize secrets backend.
* import secrets backend classes
* instantiate them and return them in a list
"""
-backend_list = []
worker_mode = False
-if default_backends != DEFAULT_SECRETS_SEARCH_PATH:
+search_section = "secrets"
+environment_variable_args: str | None = (
+"airflow.secrets.environment_variables.EnvironmentVariablesBackend"
+)
+metastore_args: str | None = "airflow.secrets.metastore.MetastoreBackend"
+execution_args: str | None = None
+
+if default_backends is not None:
worker_mode = True
+search_section = "workers"
+environment_variable_args = (
+environment_variable_args if environment_variable_args in
default_backends else None
+)
+metastore_args = metastore_args if metastore_args in default_backends
else None
+execution_args = (
+
"airflow.sdk.execution_time.secrets.execution_api.ExecutionAPISecretsBackend"
+if
"airflow.sdk.execution_time.secrets.execution_api.ExecutionAPISecretsBackend"
+in default_backends
+else None
+)
+
+backends_map: dict[str, dict[str, Any]] = {
+"environment_variable": {
+"callback": get_importable_secret_backend,
+"args": (environment_variable_args,),
+},
+"metastore": {
+"callback": get_importable_secret_backend,
+"args": (metastore_args,),
+},
+"custom": {
+"callback": get_custom_secret_backend,
+"args": (worker_mode,),
+},
+"execution_api": {
+"callback": get_importable_secret_backend,
+"args": (execution_args,),
+},
+}
-custom_secret_backend = get_custom_secret_backend(worker_mode)
+backends_order = conf.getlist(search_section, "backends_order",
delimiter=",")
-if custom_secret_backend is not None:
-from airflow.models import Connection
+required_backends = (
+[Backends.ENVIRONMENT_VARIABLE, Backends.EXECUTION_API]
+if worker_mode
+else [Backends.METASTORE, Backends.ENVIRONMENT_VARIABLE]
+)
-custom_secret_backend._set_connection_class(Connection)
-backend_list.append(custom_secret_backend)
+if missing_backends := [b.value for b in required_backends if b.value not
in backends_order]:
+raise AirflowConfigException(
+f"The configuration option [{search_section}]backends_order is
misconfigured. "
+f"The following backend types are missing: {missing_backends}",
+search_section,
Review Comment:
I don't think that it is needed as there were no such check previously
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
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_r3225189568
##
task-sdk/src/airflow/sdk/configuration.py:
##
@@ -232,36 +252,92 @@ def initialize_secrets_backends(
Uses SDK's conf instead of Core's conf.
"""
-from airflow.sdk._shared.module_loading import import_string
-
-backend_list = []
-worker_mode = False
# Determine worker mode - if default_backends is not the server default,
it's worker mode
# This is a simplified check; in practice, worker mode is determined by
the caller
-if default_backends != _SERVER_DEFAULT_SECRETS_SEARCH_PATH:
+from airflow.sdk.configuration import conf
+
+worker_mode = False
+search_section = "secrets"
+environment_variable_args: str | None = (
+"airflow.secrets.environment_variables.EnvironmentVariablesBackend"
+)
+metastore_args: str | None = "airflow.secrets.metastore.MetastoreBackend"
+execution_args: str | None = None
+
+if default_backends is not None:
worker_mode = True
+search_section = "workers"
+environment_variable_args = (
+environment_variable_args if environment_variable_args in
default_backends else None
+)
+metastore_args = metastore_args if metastore_args in default_backends
else None
+execution_args = (
+
"airflow.sdk.execution_time.secrets.execution_api.ExecutionAPISecretsBackend"
+if
"airflow.sdk.execution_time.secrets.execution_api.ExecutionAPISecretsBackend"
+in default_backends
+else None
+)
+
+backends_map: dict[str, dict[str, Any]] = {
+"environment_variable": {
+"callback": get_importable_secret_backend,
+"args": (environment_variable_args,),
+},
+"metastore": {
+"callback": get_importable_secret_backend,
+"args": (metastore_args,),
+},
+"custom": {
+"callback": get_custom_secret_backend,
+"args": (worker_mode,),
+},
+"execution_api": {
+"callback": get_importable_secret_backend,
+"args": (execution_args,),
+},
+}
-custom_secret_backend = get_custom_secret_backend(worker_mode)
+backends_order = conf.getlist(search_section, "backends_order",
delimiter=",")
-if custom_secret_backend is not None:
-from airflow.sdk.definitions.connection import Connection
+required_backends = (
+[Backends.ENVIRONMENT_VARIABLE, Backends.EXECUTION_API]
+if worker_mode
+else [Backends.METASTORE, Backends.ENVIRONMENT_VARIABLE]
+)
-custom_secret_backend._set_connection_class(Connection)
-backend_list.append(custom_secret_backend)
+if missing_backends := [b.value for b in required_backends if b.value not
in backends_order]:
+raise AirflowConfigException(
+f"The configuration option [{search_section}]backends_order is
misconfigured. "
+f"The following backend types are missing: {missing_backends}",
+search_section,
Review Comment:
I don't think that it is needed as there were no such check previously
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
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_r3225065256 ## task-sdk/src/airflow/sdk/configuration.py: ## @@ -232,36 +252,92 @@ def initialize_secrets_backends( Uses SDK's conf instead of Core's conf. """ -from airflow.sdk._shared.module_loading import import_string - -backend_list = [] -worker_mode = False # Determine worker mode - if default_backends is not the server default, it's worker mode # This is a simplified check; in practice, worker mode is determined by the caller -if default_backends != _SERVER_DEFAULT_SECRETS_SEARCH_PATH: +from airflow.sdk.configuration import conf + Review Comment: It is not needed. Other functions in this file use such import. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
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_r3225057479 ## task-sdk/src/airflow/sdk/configuration.py: ## @@ -221,8 +222,27 @@ def get_custom_secret_backend(worker_mode: bool = False): return conf._get_custom_secret_backend(worker_mode=worker_mode) +class Backends(Enum): +"""Type of the secrets backend.""" + +ENVIRONMENT_VARIABLE = "environment_variable" +EXECUTION_API = "execution_api" +CUSTOM = "custom" +METASTORE = "metastore" + + +def get_importable_secret_backend(class_name: str | None) -> Any | None: +"""Get secret backend defined in the given class name.""" +from airflow.sdk._shared.module_loading import import_string + +if class_name is not None: Review Comment: Moved import_string at module scope -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
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_r3225037717 ## task-sdk/src/airflow/sdk/configuration.py: ## @@ -270,10 +346,9 @@ def ensure_secrets_loaded( """ # Check if the secrets_backend_list contains only 2 default backends. -# Check if we are loading the backends for worker too by checking if the default_backends is equal -# to _SERVER_DEFAULT_SECRETS_SEARCH_PATH. +# Check if we are loading the backends for worker too by checking if the default_backends is not None secrets_backend_list = initialize_secrets_backends() -if len(secrets_backend_list) == 2 or default_backends != _SERVER_DEFAULT_SECRETS_SEARCH_PATH: +if len(secrets_backend_list) == 2 or default_backends is not None: return initialize_secrets_backends(default_backends=default_backends) return secrets_backend_list Review Comment: Moved secrets_backend_list as it is in airflow-core -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
potiuk commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-4323637242 @moiseenkov — There are 7 unresolved review threads on this PR from @amoghrajesh and @uranusjr. Could you either push a fix or reply in each thread explaining why the feedback doesn't apply? Once you believe the feedback is addressed, mark the thread as resolved so the reviewer isn't re-pinged needlessly. Thanks! --- _Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this [two-stage triage process](https://github.com/apache/airflow/blob/main/contributing-docs/25_maintainer_pr_triage.md#why-the-first-pass-is-automated) so that our maintainers' limited time is spent where it matters most: the conversation with you._ -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
ay-azara commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-4296531237 @potiuk No offense intended as I'm sure it's no small effort to run an open source project this large, but it seems poor taste to throw this back on @moiseenkov when @amoghrajesh and @eladkal have completely ghosted the thread. The PR has been updated to take care of existing issues multiple times while waiting for them to provide feedback. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
potiuk commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-4292801152 @moiseenkov This PR has a few issues that need to be addressed before it can be reviewed — please see our [Pull Request quality criteria](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-quality-criteria). **Issues found:** - :warning: **Unresolved review comments**: This PR has 2 unresolved review thread(s) from maintainers: **@amoghrajesh** (MEMBER): 2 unresolved threads. Please review and resolve all inline review comments before requesting another review. You can resolve a conversation by clicking 'Resolve conversation' on each thread after addressing the feedback. See [pull request guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst). **What to do next:** - Fix each issue listed above. - Make sure static checks pass locally (`prek run --from-ref main --stage pre-commit`). - Mark the PR as "Ready for review" when you're done. There is no rush — take your time and work at your own pace. We appreciate your contribution and are happy to wait for updates. If you have questions, feel free to ask on the [Airflow Slack](https://s.apache.org/airflow-slack). -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
Copilot commented on code in PR #45931:
URL: https://github.com/apache/airflow/pull/45931#discussion_r3066497474
##
airflow-core/src/airflow/configuration.py:
##
@@ -741,35 +740,107 @@ def get_custom_secret_backend(worker_mode: bool = False)
-> BaseSecretsBackend |
return conf._get_custom_secret_backend(worker_mode=worker_mode)
+def get_importable_secret_backend(class_name: str | None) ->
BaseSecretsBackend | None:
+"""Get secret backend defined in the given class name."""
+if class_name is not None:
+secrets_backend_cls = import_string(class_name)
+return secrets_backend_cls()
+return None
+
+
+class Backends(Enum):
+"""Type of the secrets backend."""
+
+ENVIRONMENT_VARIABLE = "environment_variable"
+EXECUTION_API = "execution_api"
+CUSTOM = "custom"
+METASTORE = "metastore"
+
+
def initialize_secrets_backends(
-default_backends: list[str] = DEFAULT_SECRETS_SEARCH_PATH,
+default_backends: list[str] | None = None,
) -> list[BaseSecretsBackend]:
"""
Initialize secrets backend.
* import secrets backend classes
* instantiate them and return them in a list
"""
-backend_list = []
worker_mode = False
-if default_backends != DEFAULT_SECRETS_SEARCH_PATH:
+search_section = "secrets"
+environment_variable_args: str | None = (
+"airflow.secrets.environment_variables.EnvironmentVariablesBackend"
+)
+metastore_args: str | None = "airflow.secrets.metastore.MetastoreBackend"
+execution_args: str | None = None
+
+if default_backends is not None:
worker_mode = True
+search_section = "workers"
+environment_variable_args = (
+environment_variable_args if environment_variable_args in
default_backends else None
+)
+metastore_args = metastore_args if metastore_args in default_backends
else None
+execution_args = (
+
"airflow.sdk.execution_time.secrets.execution_api.ExecutionAPISecretsBackend"
+if
"airflow.sdk.execution_time.secrets.execution_api.ExecutionAPISecretsBackend"
+in default_backends
+else None
+)
+
+backends_map: dict[str, dict[str, Any]] = {
+"environment_variable": {
+"callback": get_importable_secret_backend,
+"args": (environment_variable_args,),
+},
+"metastore": {
+"callback": get_importable_secret_backend,
+"args": (metastore_args,),
+},
+"custom": {
+"callback": get_custom_secret_backend,
+"args": (worker_mode,),
+},
+"execution_api": {
+"callback": get_importable_secret_backend,
+"args": (execution_args,),
+},
+}
-custom_secret_backend = get_custom_secret_backend(worker_mode)
+backends_order = conf.getlist(search_section, "backends_order",
delimiter=",")
-if custom_secret_backend is not None:
-from airflow.models import Connection
+required_backends = (
+[Backends.ENVIRONMENT_VARIABLE, Backends.EXECUTION_API]
+if worker_mode
+else [Backends.METASTORE, Backends.ENVIRONMENT_VARIABLE]
+)
-custom_secret_backend._set_connection_class(Connection)
-backend_list.append(custom_secret_backend)
+if missing_backends := [b.value for b in required_backends if b.value not
in backends_order]:
+raise AirflowConfigException(
+f"The configuration option [{search_section}]backends_order is
misconfigured. "
+f"The following backend types are missing: {missing_backends}",
+search_section,
Review Comment:
In worker mode, this required-backends check only validates that the *type
names* are present in `backends_order`, but it doesn’t validate that the
corresponding backend classpaths are actually available (e.g.
`execution_args`/`environment_variable_args` can be set to `None` when they
aren’t in `default_backends`, causing the backend to be silently skipped).
Please either (a) stop nulling out required backend args, or (b) extend
validation to error when a required backend’s classpath is missing from
`default_backends` so workers can’t start without required backends
instantiated.
##
airflow-core/src/airflow/ui/src/pages/Variables/BackendsOrderCard.tsx:
##
@@ -0,0 +1,42 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distribu
Re: [PR] Add config option [secrets]backends_order [airflow]
potiuk commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-4206186300 @amoghrajesh -> you had comments on it, can you re-review as well? I thnk this one is indeed long overdue. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
potiuk commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-4204064274 As soon as we deal with the current overload we shall come back to it. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
potiuk commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-4204056177 Yes we certainly lost track of it -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-4199854965 Hello @potiuk @eladkal I rebased this PR again. CI fails not because of my changes. Could we please move forward with this PR? It's been a year since it was created... -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
potiuk commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-4195141758 > @eladkal could you please re-review this PR? @moiseenkov @chrisnordqvist -> cna you please rebase and make the PR green again. This can happen multple times, I know, but keeping the PR to be green for review is quite important as otherwise we do not knowwhatis really not working and what needs conflict resolution. We had a lot of troubles recently with ***lots of ** PRS and bad ones and also with lots of flaky tests- they are mitigated now so it should be easier. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
potiuk commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-4177320453 @moiseenkov This PR has a few issues that need to be addressed before it can be reviewed — please see our [Pull Request quality criteria](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-quality-criteria). **Issues found:** - :x: **Merge conflicts**: This PR has merge conflicts with the `main` branch. Your branch is 35 commits behind `main`. Please rebase your branch (`git fetch origin && git rebase origin/main`), resolve the conflicts, and push again. See [contributing quick start](https://github.com/apache/airflow/blob/main/contributing-docs/03a_contributors_quick_start_beginners.rst). - :x: **Other failing CI checks**: Failing: Additional PROD image tests / Test e2e integration tests with PROD image / Regular e2e test. Run `prek run --from-ref main` locally to reproduce. See [static checks docs](https://github.com/apache/airflow/blob/main/contributing-docs/08_static_code_checks.rst). - :warning: **Unresolved review comments**: This PR has 10 unresolved review threads from maintainers: **@eladkal** (MEMBER): 1 unresolved threads; **@amoghrajesh** (MEMBER): 8 unresolved threads; **@uranusjr** (MEMBER): 1 unresolved threads. Please review and resolve all inline review comments before requesting another review. You can resolve a conversation by clicking 'Resolve conversation' on each thread after addressing the feedback. See [pull request guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst). **What to do next:** - The comment informs you what you need to do. - Fix each issue, then mark the PR as "Ready for review" in the GitHub UI - but only after making sure that all the issues are fixed. - There is no rush — take your time and work at your own pace. We appreciate your contribution and are happy to wait for updates. - Maintainers will then proceed with a normal review. There is no rush — take your time and work at your own pace. We appreciate your contribution and are happy to wait for updates. If you have questions, feel free to ask on the [Airflow Slack](https://s.apache.org/airflow-slack). -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-4171969017 Hello @amoghrajesh I rebased and set the backends_order feature for Airflow 3.2.1 @eladkal could you please re-review this PR? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
amoghrajesh commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-4124259653 @eladkal do you mind taking a look at this one again? @Crowiant would you resolve the merge conflicts too please? It doesn't seem like we will make this one in time for 3.2 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
potiuk commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3998927808 @eladkal - looks like your concerns are alleviated - can you please remove "request changes"? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
potiuk commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3979933901 Sorry for that - I've been a bit busy - can you please rebase one more time as it needs more conflict resolution - also the UI part is somethign maybe @jason810496 can you re-review? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3907436419 Hello @potiuk I updated the PR. Since checks are passing, can we merge if there are no objections? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
potiuk commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3841511233 Can you please resolve conflicts and fix the remaining static check? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3819963246 Hello @potiuk @eladkal Thank you for your comments. I added experimental tag to the feature. Here are screenshots of how backends order looks on UI: https://github.com/user-attachments/assets/59fc7fc0-061f-40ba-ac73-21ad9e83952d"; /> https://github.com/user-attachments/assets/abc3d7e5-1b76-4757-a805-6c677d59fdd3"; /> -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
potiuk commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3784567693 > I advise to tag this as experimental feature so community will use this feature with judgment. Good idea @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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
eladkal commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3784275267 I don't have time to re-review but I still have some concerns. I advise to tag this as experimental feature so community will use this feature with judgment. I will however ask to please present a UI image of how the indication of backend order is shown to users. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
potiuk commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3784010613 @amoghrajesh @uranusjr -> any comments here? I think there were some important questions/comments from you and I would love to merge that one to make sure it makes it for 3.2 (it looks good in general). -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3738772066 Hello @amoghrajesh @uranusjr I responded to your comments in the PR. Could you please help with the review? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
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_r2627268679 ## airflow-core/src/airflow/configuration.py: ## @@ -2348,28 +2334,101 @@ def get_custom_secret_backend(worker_mode: bool = False) -> BaseSecretsBackend | return secrets_backend_cls(**backend_kwargs) +def get_importable_secret_backend(class_name: str | None) -> BaseSecretsBackend | None: +"""Get secret backend defined in the given class name.""" +if class_name is not None: +secrets_backend_cls = import_string(class_name) +return secrets_backend_cls() +return None + + +class Backends(Enum): +"""Type of the secrets backend.""" + +ENVIRONMENT_VARIABLE = "environment_variable" +EXECUTION_API = "execution_api" +CUSTOM = "custom" +METASTORE = "metastore" + + def initialize_secrets_backends( -default_backends: list[str] = DEFAULT_SECRETS_SEARCH_PATH, +default_backends: list[str] | None = None, ) -> list[BaseSecretsBackend]: """ Initialize secrets backend. * import secrets backend classes * instantiate them and return them in a list """ -backend_list = [] worker_mode = False -if default_backends != DEFAULT_SECRETS_SEARCH_PATH: +search_section = "secrets" +environment_variable_args: str | None = ( +"airflow.secrets.environment_variables.EnvironmentVariablesBackend" +) +metastore_args: str | None = "airflow.secrets.metastore.MetastoreBackend" +execution_args: str | None = None +if default_backends is not None: worker_mode = True +search_section = "workers" +environment_variable_args = ( +environment_variable_args if environment_variable_args in default_backends else None +) +metastore_args = metastore_args if metastore_args in default_backends else None +execution_args = ( + "airflow.sdk.execution_time.secrets.execution_api.ExecutionAPISecretsBackend" +if "airflow.sdk.execution_time.secrets.execution_api.ExecutionAPISecretsBackend" +in default_backends +else None +) Review Comment: Could you expand a little bit on this? ExecutionAPISecretsBackend exists only in task-sdk and applies for the workers. Where else could it be imported? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
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_r2627247913 ## airflow-core/src/airflow/configuration.py: ## @@ -2301,9 +2288,8 @@ def ensure_secrets_loaded( """ # Check if the secrets_backend_list contains only 2 default backends. -# Check if we are loading the backends for worker too by checking if the default_backends is equal -# to DEFAULT_SECRETS_SEARCH_PATH. -if len(secrets_backend_list) == 2 or default_backends != DEFAULT_SECRETS_SEARCH_PATH: +# Check if we are loading the backends for worker too by checking if the default_backends is not None +if len(secrets_backend_list) == 2 or default_backends is not None: return initialize_secrets_backends(default_backends=default_backends) return secrets_backend_list Review Comment: Hello @amoghrajesh ! Thank you for mentioning the PR, I updated the code. Please check when you have time! -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
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_r2623754538 ## airflow-core/docs/security/secrets/secrets-backend/index.rst: ## @@ -89,13 +100,21 @@ configure separate secrets backend for workers, you can do that using: [workers] secrets_backend = secrets_backend_kwargs = - +backends_order = Review Comment: Hello @uranusjr ! Thank you for your review and comments! No, actually it should be under both sections: [secrets] and [workers]. Because after talking with @amoghrajesh We agreed that implementation should be extended on workers too. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
amoghrajesh commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r2558826591 ## airflow-core/docs/security/secrets/secrets-backend/index.rst: ## @@ -89,13 +100,21 @@ configure separate secrets backend for workers, you can do that using: [workers] secrets_backend = secrets_backend_kwargs = - +backends_order = Set ``secrets_backend`` to the fully qualified class name of the backend you want to enable. You can provide ``secrets_backend_kwargs`` with json and it will be passed as kwargs to the ``__init__`` method of your secrets backend for the workers. +``backends_order`` comma-separated list of secret backends for workers. These backends will be used in the order they are specified. Review Comment: ```suggestion ``backends_order`` is a comma-separated list of secret backends for workers. These backends will be used in the order they are specified. ``` ## airflow-core/src/airflow/configuration.py: ## @@ -2348,28 +2334,101 @@ def get_custom_secret_backend(worker_mode: bool = False) -> BaseSecretsBackend | return secrets_backend_cls(**backend_kwargs) +def get_importable_secret_backend(class_name: str | None) -> BaseSecretsBackend | None: +"""Get secret backend defined in the given class name.""" +if class_name is not None: +secrets_backend_cls = import_string(class_name) +return secrets_backend_cls() +return None + + +class Backends(Enum): +"""Type of the secrets backend.""" + +ENVIRONMENT_VARIABLE = "environment_variable" +EXECUTION_API = "execution_api" +CUSTOM = "custom" +METASTORE = "metastore" + + def initialize_secrets_backends( -default_backends: list[str] = DEFAULT_SECRETS_SEARCH_PATH, +default_backends: list[str] | None = None, ) -> list[BaseSecretsBackend]: """ Initialize secrets backend. * import secrets backend classes * instantiate them and return them in a list """ -backend_list = [] worker_mode = False -if default_backends != DEFAULT_SECRETS_SEARCH_PATH: +search_section = "secrets" +environment_variable_args: str | None = ( +"airflow.secrets.environment_variables.EnvironmentVariablesBackend" +) +metastore_args: str | None = "airflow.secrets.metastore.MetastoreBackend" +execution_args: str | None = None +if default_backends is not None: worker_mode = True +search_section = "workers" +environment_variable_args = ( +environment_variable_args if environment_variable_args in default_backends else None +) +metastore_args = metastore_args if metastore_args in default_backends else None +execution_args = ( + "airflow.sdk.execution_time.secrets.execution_api.ExecutionAPISecretsBackend" +if "airflow.sdk.execution_time.secrets.execution_api.ExecutionAPISecretsBackend" +in default_backends +else None +) Review Comment: We are trying to avoid importing sdk in airflow core when possible ## airflow-core/docs/security/secrets/secrets-backend/index.rst: ## @@ -64,12 +66,21 @@ The ``[secrets]`` section has the following options: [secrets] backend = backend_kwargs = +backends_order = Set ``backend`` to the fully qualified class name of the backend you want to enable. You can provide ``backend_kwargs`` with json and it will be passed as kwargs to the ``__init__`` method of your secrets backend. +``backends_order`` comma-separated list of secret backends. These backends will be used in the order they are specified. Review Comment: ```suggestion ``backends_order`` is a comma-separated list of secret backends. These backends will be used in the order they are specified. ``` ## airflow-core/src/airflow/configuration.py: ## @@ -2301,9 +2288,8 @@ def ensure_secrets_loaded( """ # Check if the secrets_backend_list contains only 2 default backends. -# Check if we are loading the backends for worker too by checking if the default_backends is equal -# to DEFAULT_SECRETS_SEARCH_PATH. -if len(secrets_backend_list) == 2 or default_backends != DEFAULT_SECRETS_SEARCH_PATH: +# Check if we are loading the backends for worker too by checking if the default_backends is not None +if len(secrets_backend_list) == 2 or default_backends is not None: return initialize_secrets_backends(default_backends=default_backends) return secrets_backend_list Review Comment: With: https://github.com/apache/airflow/pull/57744 merged, there is a shared config parser now. You will have to update this in sdk/configuration.py too -- 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.
Re: [PR] Add config option [secrets]backends_order [airflow]
uranusjr commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r2548867570 ## airflow-core/docs/security/secrets/secrets-backend/index.rst: ## @@ -89,13 +100,21 @@ configure separate secrets backend for workers, you can do that using: [workers] secrets_backend = secrets_backend_kwargs = - +backends_order = Review Comment: This doesn’t read right? The key implemented here is not under `[workders]`. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
uranusjr commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3561862735 I wonder if this should just be called `backends` and have the description explain how the ordering is significant. The implementation is fine to me. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
uranusjr commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r2548860808 ## airflow-core/docs/security/secrets/secrets-backend/index.rst: ## @@ -64,12 +66,21 @@ The ``[secrets]`` section has the following options: [secrets] backend = backend_kwargs = +backends_order = Set ``backend`` to the fully qualified class name of the backend you want to enable. You can provide ``backend_kwargs`` with json and it will be passed as kwargs to the ``__init__`` method of your secrets backend. +``backends_order`` 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 Review Comment: OK then I think this is fine. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
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_r2545346813 ## airflow-core/docs/security/secrets/secrets-backend/index.rst: ## @@ -64,12 +66,21 @@ The ``[secrets]`` section has the following options: [secrets] backend = backend_kwargs = +backends_order = Set ``backend`` to the fully qualified class name of the backend you want to enable. You can provide ``backend_kwargs`` with json and it will be passed as kwargs to the ``__init__`` method of your secrets backend. +``backends_order`` 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 Review Comment: Hello @uranusjr as it listed in the code here: https://github.com/apache/airflow/pull/45931/files#diff-12a2a29eab84b723e9342e89877c875d3e979220a53884a9103d8c067ffdc286R2409 Exception will be raised in this case. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
uranusjr commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r2544542438 ## airflow-core/docs/security/secrets/secrets-backend/index.rst: ## @@ -64,12 +66,21 @@ The ``[secrets]`` section has the following options: [secrets] backend = backend_kwargs = +backends_order = Set ``backend`` to the fully qualified class name of the backend you want to enable. You can provide ``backend_kwargs`` with json and it will be passed as kwargs to the ``__init__`` method of your secrets backend. +``backends_order`` 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 Review Comment: What happens if the user only list some of the backends? ``` [secrets] backends_order = custom ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
amoghrajesh commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3556160258 @VladaZakharova @moiseenkov @Crowiant I will take a look at this one soon. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
VladaZakharova commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3552210262 hi @amoghrajesh :) can you please check if we need to add additional changes here in PR? 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3546744163 Hello @amoghrajesh Could you please take a look at the PR? I addressed all your comments. Thank you! -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3541671716 Hello @amoghrajesh Could you please review this PR? I responded to your comments. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
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_r2487177214 ## airflow-core/src/airflow/config_templates/config.yml: ## @@ -1323,6 +1323,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: Yeah, I agree. Added [workers]backends_order as a separate option that could be configured. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
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_r2487166122
##
airflow-core/src/airflow/configuration.py:
##
@@ -2210,28 +2208,73 @@ def get_custom_secret_backend(worker_mode: bool =
False) -> BaseSecretsBackend |
return secrets_backend_cls(**backend_kwargs)
+def get_importable_secret_backend(class_name: str | None) ->
BaseSecretsBackend | None:
+"""Get secret backend defined in the given class name."""
+if class_name is not None:
+secrets_backend_cls = import_string(class_name)
+return secrets_backend_cls()
+return None
+
+
def initialize_secrets_backends(
-default_backends: list[str] = DEFAULT_SECRETS_SEARCH_PATH,
+default_backends: list[str] | None = None,
) -> list[BaseSecretsBackend]:
"""
Initialize secrets backend.
* import secrets backend classes
* instantiate them and return them in a list
"""
-backend_list = []
worker_mode = False
-if default_backends != DEFAULT_SECRETS_SEARCH_PATH:
+environment_variable_args: str | None = (
+"airflow.secrets.environment_variables.EnvironmentVariablesBackend"
+)
+metastore_args: str | None = "airflow.secrets.metastore.MetastoreBackend"
+if default_backends is not None:
worker_mode = True
+environment_variable_args = (
+environment_variable_args if environment_variable_args in
default_backends else None
+)
+metastore_args = metastore_args if metastore_args in default_backends
else None
+backends_map: dict[str, dict[str, Any]] = {
+"environment_variable": {
+"callback": get_importable_secret_backend,
+"args": (environment_variable_args,),
+},
+"metastore": {
+"callback": get_importable_secret_backend,
+"args": (metastore_args,),
+},
+"custom": {
+"callback": get_custom_secret_backend,
+"args": (worker_mode,),
+},
+}
-custom_secret_backend = get_custom_secret_backend(worker_mode)
+backends_order = conf.getlist("secrets", "backends_order", delimiter=",")
-if custom_secret_backend is not None:
-backend_list.append(custom_secret_backend)
+required_backends = ["environment_variable"] if worker_mode else
["metastore", "environment_variable"]
+if missing_backends := [b for b in required_backends if b not in
backends_order]:
Review Comment:
Yes, I agree, added to the PR.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
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_r2487167401
##
airflow-core/tests/unit/always/test_secrets.py:
##
@@ -119,6 +120,38 @@ def test_backend_fallback_to_env_var(self,
mock_get_connection):
assert conn.get_uri() == "mysql://airflow:airflow@host:5432/airflow"
+@conf_vars(
+{
+(
+"secrets",
+"backend",
+):
"airflow.providers.amazon.aws.secrets.systems_manager.SystemsManagerParameterStoreBackend",
+("secrets", "backend_kwargs"): '{"connections_prefix": "/airflow",
"profile_name": null}',
+("secrets", "backends_order"):
"custom,environment_variable,metastore",
+}
+)
+def test_backends_order(self):
+backends = ensure_secrets_loaded()
+backend_classes = [backend.__class__.__name__ for backend in backends]
+assert backend_classes == [
+"SystemsManagerParameterStoreBackend",
+"EnvironmentVariablesBackend",
+"MetastoreBackend",
+]
+
[email protected](
+"backends_order",
+[
+pytest.param("custom,metastore",
id="no_environment_variable_backend"),
+pytest.param("environment_variable", id="no_metastore_backend"),
+pytest.param("metastore,environment_variable,unsupported",
id="unsupported_backend"),
+],
+)
+def test_backends_order_invalid_cases(self, backends_order):
+with conf_vars({("secrets", "backends_order"): backends_order}):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
Review Comment:
Added
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3481487585 Hello @amoghrajesh Thank you for your review! > I have some comments re the design of the config. > > I would also not add the UI portion in the same PR, maybe a orthogonal one would be the place to add it. Regarding UI: as it was mentioned here that UI should be introduced in this PR: https://github.com/apache/airflow/pull/45931#issuecomment-3023344132 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
amoghrajesh commented on code in PR #45931:
URL: https://github.com/apache/airflow/pull/45931#discussion_r2452090298
##
airflow-core/src/airflow/config_templates/config.yml:
##
@@ -1323,6 +1323,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:
On giving this some more thought, I am not very convinced that having the
config under "secrets" is the right thing to do.
For someone wanting to configure workers backend, they have to set this:
[secrets] backends_order, [workers] secrets_backend which is not at all
intuitive and is confusing.
We should consider having backends_order as a config for workers too, maybe:
[workers] backends_order which plays nice with workers backend and the separate
one that we have for secrets can be used there.
##
airflow-core/src/airflow/configuration.py:
##
@@ -2210,28 +2208,73 @@ def get_custom_secret_backend(worker_mode: bool =
False) -> BaseSecretsBackend |
return secrets_backend_cls(**backend_kwargs)
+def get_importable_secret_backend(class_name: str | None) ->
BaseSecretsBackend | None:
+"""Get secret backend defined in the given class name."""
+if class_name is not None:
+secrets_backend_cls = import_string(class_name)
+return secrets_backend_cls()
+return None
+
+
def initialize_secrets_backends(
-default_backends: list[str] = DEFAULT_SECRETS_SEARCH_PATH,
+default_backends: list[str] | None = None,
) -> list[BaseSecretsBackend]:
"""
Initialize secrets backend.
* import secrets backend classes
* instantiate them and return them in a list
"""
-backend_list = []
worker_mode = False
-if default_backends != DEFAULT_SECRETS_SEARCH_PATH:
+environment_variable_args: str | None = (
+"airflow.secrets.environment_variables.EnvironmentVariablesBackend"
+)
+metastore_args: str | None = "airflow.secrets.metastore.MetastoreBackend"
+if default_backends is not None:
worker_mode = True
+environment_variable_args = (
+environment_variable_args if environment_variable_args in
default_backends else None
+)
+metastore_args = metastore_args if metastore_args in default_backends
else None
+backends_map: dict[str, dict[str, Any]] = {
+"environment_variable": {
+"callback": get_importable_secret_backend,
+"args": (environment_variable_args,),
+},
+"metastore": {
+"callback": get_importable_secret_backend,
+"args": (metastore_args,),
+},
+"custom": {
+"callback": get_custom_secret_backend,
+"args": (worker_mode,),
+},
+}
-custom_secret_backend = get_custom_secret_backend(worker_mode)
+backends_order = conf.getlist("secrets", "backends_order", delimiter=",")
-if custom_secret_backend is not None:
-backend_list.append(custom_secret_backend)
+required_backends = ["environment_variable"] if worker_mode else
["metastore", "environment_variable"]
+if missing_backends := [b for b in required_backends if b not in
backends_order]:
Review Comment:
Do you think defining an Enum here would make things cleaner?
```
class Backend(Enum):
ENVIRONMENT_VARIABLE = "environment_variable"
METASTORE = "metastore"
CUSTOM = "custom"
```
The hardcoded strings are error prone imo
##
airflow-core/tests/unit/always/test_secrets.py:
##
@@ -119,6 +120,38 @@ def test_backend_fallback_to_env_var(self,
mock_get_connection):
assert conn.get_uri() == "mysql://airflow:airflow@host:5432/airflow"
+@conf_vars(
+{
+(
+"secrets",
+"backend",
+):
"airflow.providers.amazon.aws.secrets.systems_manager.SystemsManagerParameterStoreBackend",
+("secrets", "backend_kwargs"): '{"connections_prefix": "/airflow",
"profile_name": null}',
+("secrets", "backends_order"):
"custom,environment_variable,metastore",
+}
+)
+def test_backends_order(self):
+bac
Re: [PR] Add config option [secrets]backends_order [airflow]
amoghrajesh commented on code in PR #45931: URL: https://github.com/apache/airflow/pull/45931#discussion_r2452064529 ## airflow-core/src/airflow/configuration.py: ## @@ -2138,7 +2137,7 @@ def make_group_other_inaccessible(file_path: str): def ensure_secrets_loaded( -default_backends: list[str] = DEFAULT_SECRETS_SEARCH_PATH, +default_backends: list[str] | None = None, Review Comment: In the previous version, a value was always passed to default_backends. Either `DEFAULT_SECRETS_SEARCH_PATH` or `DEFAULT_SECRETS_SEARCH_PATH_WORKERS` which imo is far more intuitive than None... -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
amoghrajesh commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3432248321 @VladaZakharova I meant I am looking at it. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
VladaZakharova commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3432236972 > @moiseenkov looking at it He is not, because he doesn't work in our team anymore, @Crowiant is the one responsible for it, so that's why he is requesting review from you -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
amoghrajesh commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3432226543 @moiseenkov looking at it -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3422033604 Hello @amoghrajesh can you please take a look at the PR? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
Crowiant commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3330266822 Hello @amoghrajesh can you please take a look at the PR? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
amoghrajesh commented on PR #45931: URL: https://github.com/apache/airflow/pull/45931#issuecomment-3219370503 @moiseenkov 3.1 is fast approaching, could you take a look at the comments if we want to make it for Airflow 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
amoghrajesh commented on code in PR #45931:
URL: https://github.com/apache/airflow/pull/45931#discussion_r2275843984
##
airflow-core/src/airflow/configuration.py:
##
@@ -2194,28 +2192,73 @@ def get_custom_secret_backend(worker_mode: bool =
False) -> BaseSecretsBackend |
return secrets_backend_cls(**backend_kwargs)
+def get_importable_secret_backend(class_name: str | None) ->
BaseSecretsBackend | None:
+"""Get secret backend defined in the given class name."""
+if class_name is not None:
+secrets_backend_cls = import_string(class_name)
+return secrets_backend_cls()
+return None
+
+
def initialize_secrets_backends(
-default_backends: list[str] = DEFAULT_SECRETS_SEARCH_PATH,
+default_backends: list[str] | None = None,
) -> list[BaseSecretsBackend]:
"""
Initialize secrets backend.
* import secrets backend classes
* instantiate them and return them in a list
"""
-backend_list = []
worker_mode = False
-if default_backends != DEFAULT_SECRETS_SEARCH_PATH:
+environment_variable_args: str | None = (
+"airflow.secrets.environment_variables.EnvironmentVariablesBackend"
+)
+metastore_args: str | None = "airflow.secrets.metastore.MetastoreBackend"
+if default_backends is not None:
worker_mode = True
+environment_variable_args = (
+environment_variable_args if environment_variable_args in
default_backends else None
+)
+metastore_args = metastore_args if metastore_args in default_backends
else None
+backends_map: dict[str, dict[str, Any]] = {
+"environment_variable": {
+"callback": get_importable_secret_backend,
+"args": (environment_variable_args,),
+},
+"metastore": {
+"callback": get_importable_secret_backend,
+"args": (metastore_args,),
+},
+"custom": {
+"callback": get_custom_secret_backend,
+"args": (worker_mode,),
+},
+}
-custom_secret_backend = get_custom_secret_backend(worker_mode)
+backends_order = conf.getlist("secrets", "backends_order", delimiter=",")
-if custom_secret_backend is not None:
-backend_list.append(custom_secret_backend)
+required_backends = ["metastore", "environment_variable"]
+if missing_backends := [b for b in required_backends if b not in
backends_order]:
+raise AirflowConfigException(
+"The configuration option [secrets]backends_order is
misconfigured. "
+"The following backend types are missing: %s",
+missing_backends,
+)
Review Comment:
This check would behave badly for workers?
Example:
```
required_backends = ["metastore", "environment_variable"]
backends_order = ["custom", "environment_variable"]
```
`required_backends` need not have `metastore` for workers.
##
airflow-core/src/airflow/configuration.py:
##
@@ -2138,7 +2137,7 @@ def make_group_other_inaccessible(file_path: str):
def ensure_secrets_loaded(
-default_backends: list[str] = DEFAULT_SECRETS_SEARCH_PATH,
+default_backends: list[str] | None = None,
Review Comment:
The convention here now is:
- If None, its called from someplace other than worker
- If not None, its called from worker (for `[workers][secrets_backend]`
config)
Its not very intuitive, could we go back with the previous behaviour?
##
airflow-core/docs/security/secrets/secrets-backend/index.rst:
##
@@ -64,12 +66,21 @@ The ``[secrets]`` section has the following options:
[secrets]
backend =
backend_kwargs =
+backends_order =
Set ``backend`` to the fully qualified class name of the backend you want to
enable.
You can provide ``backend_kwargs`` with json and it will be passed as kwargs
to the ``__init__`` method of
your secrets backend.
+``backends_order`` 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``.
Review Comment:
I also request you to have some user guidelines in the docs to configure it
properly.
For example:
Lets say DB lookup has higher precedence than that of say ENV backend.
This would be shooting ourselves in the foot by compromising the performance
here? DB lookup will be more expensive than DB.
##
airflow-core/src/airflow/configuration.py:
##
@@ -2194,28 +2192,73 @@ def g
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_r2259651486
##
airflow-core/tests/unit/always/test_secrets.py:
##
@@ -119,6 +120,40 @@ def test_backend_fallback_to_env_var(self,
mock_get_connection):
assert conn.get_uri() == "mysql://airflow:airflow@host:5432/airflow"
+@conf_vars(
+{
+(
+"secrets",
+"backend",
+):
"airflow.providers.amazon.aws.secrets.systems_manager.SystemsManagerParameterStoreBackend",
+("secrets", "backend_kwargs"): '{"connections_prefix": "/airflow",
"profile_name": null}',
+("secrets", "backends_order"):
"custom,environment_variable,metastore",
+}
+)
+def test_backends_order(self):
+backends = ensure_secrets_loaded()
+backend_classes = [backend.__class__.__name__ for backend in backends]
+assert backend_classes == [
+"SystemsManagerParameterStoreBackend",
+"EnvironmentVariablesBackend",
+"MetastoreBackend",
+]
+
+@conf_vars({("secrets", "backends_order"): "custom,metastore"})
+def test_backends_order_no_environment_variable_backend(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
+
+@conf_vars({("secrets", "backends_order"): "environment_variable"})
+def test_backends_order_no_metastore_backend(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
+
+@conf_vars({("secrets", "backends_order"):
"metastore,environment_variable,unsupported"})
+def test_backends_order_unsupported(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
Review Comment:
Thank you very much @jason810496 ! Added to the PR
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
jason810496 commented on code in PR #45931:
URL: https://github.com/apache/airflow/pull/45931#discussion_r2257466928
##
airflow-core/tests/unit/always/test_secrets.py:
##
@@ -119,6 +120,40 @@ def test_backend_fallback_to_env_var(self,
mock_get_connection):
assert conn.get_uri() == "mysql://airflow:airflow@host:5432/airflow"
+@conf_vars(
+{
+(
+"secrets",
+"backend",
+):
"airflow.providers.amazon.aws.secrets.systems_manager.SystemsManagerParameterStoreBackend",
+("secrets", "backend_kwargs"): '{"connections_prefix": "/airflow",
"profile_name": null}',
+("secrets", "backends_order"):
"custom,environment_variable,metastore",
+}
+)
+def test_backends_order(self):
+backends = ensure_secrets_loaded()
+backend_classes = [backend.__class__.__name__ for backend in backends]
+assert backend_classes == [
+"SystemsManagerParameterStoreBackend",
+"EnvironmentVariablesBackend",
+"MetastoreBackend",
+]
+
+@conf_vars({("secrets", "backends_order"): "custom,metastore"})
+def test_backends_order_no_environment_variable_backend(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
+
+@conf_vars({("secrets", "backends_order"): "environment_variable"})
+def test_backends_order_no_metastore_backend(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
+
+@conf_vars({("secrets", "backends_order"):
"metastore,environment_variable,unsupported"})
+def test_backends_order_unsupported(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
Review Comment:
The usage of `parametrize` will be somehow like the following change (the
indent and format might still need fix).
```suggestion
@pytest.mark.parametrize(
"backends_order",
[
pytest.param(
"custom,metastore",
id="no_environment_variable_backend"
),
pytest.param(
"environment_variable",
id="no_metastore_backend"
),
pytest.param(
"metastore,environment_variable,unsupported",
id="unsupported_backend"
),
]
)
def test_backends_order_invalid_cases(backends_order):
with conf_vars({("secrets", "backends_order"): backends_order}):
with pytest.raises(AirflowConfigException):
ensure_secrets_loaded()
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
jason810496 commented on code in PR #45931:
URL: https://github.com/apache/airflow/pull/45931#discussion_r2257466928
##
airflow-core/tests/unit/always/test_secrets.py:
##
@@ -119,6 +120,40 @@ def test_backend_fallback_to_env_var(self,
mock_get_connection):
assert conn.get_uri() == "mysql://airflow:airflow@host:5432/airflow"
+@conf_vars(
+{
+(
+"secrets",
+"backend",
+):
"airflow.providers.amazon.aws.secrets.systems_manager.SystemsManagerParameterStoreBackend",
+("secrets", "backend_kwargs"): '{"connections_prefix": "/airflow",
"profile_name": null}',
+("secrets", "backends_order"):
"custom,environment_variable,metastore",
+}
+)
+def test_backends_order(self):
+backends = ensure_secrets_loaded()
+backend_classes = [backend.__class__.__name__ for backend in backends]
+assert backend_classes == [
+"SystemsManagerParameterStoreBackend",
+"EnvironmentVariablesBackend",
+"MetastoreBackend",
+]
+
+@conf_vars({("secrets", "backends_order"): "custom,metastore"})
+def test_backends_order_no_environment_variable_backend(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
+
+@conf_vars({("secrets", "backends_order"): "environment_variable"})
+def test_backends_order_no_metastore_backend(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
+
+@conf_vars({("secrets", "backends_order"):
"metastore,environment_variable,unsupported"})
+def test_backends_order_unsupported(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
Review Comment:
The usage of `parametrize` will be like the following change (the indent and
format might still need fix).
```suggestion
@pytest.mark.parametrize(
"backends_order",
[
pytest.param(
"custom,metastore",
id="no_environment_variable_backend"
),
pytest.param(
"environment_variable",
id="no_metastore_backend"
),
pytest.param(
"metastore,environment_variable,unsupported",
id="unsupported_backend"
),
]
)
def test_backends_order_invalid_cases(backends_order):
with conf_vars({("secrets", "backends_order"): backends_order}):
with pytest.raises(AirflowConfigException):
ensure_secrets_loaded()
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
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_r2257280351
##
airflow-core/tests/unit/always/test_secrets.py:
##
@@ -119,6 +120,40 @@ def test_backend_fallback_to_env_var(self,
mock_get_connection):
assert conn.get_uri() == "mysql://airflow:airflow@host:5432/airflow"
+@conf_vars(
+{
+(
+"secrets",
+"backend",
+):
"airflow.providers.amazon.aws.secrets.systems_manager.SystemsManagerParameterStoreBackend",
+("secrets", "backend_kwargs"): '{"connections_prefix": "/airflow",
"profile_name": null}',
+("secrets", "backends_order"):
"custom,environment_variable,metastore",
+}
+)
+def test_backends_order(self):
+backends = ensure_secrets_loaded()
+backend_classes = [backend.__class__.__name__ for backend in backends]
+assert backend_classes == [
+"SystemsManagerParameterStoreBackend",
+"EnvironmentVariablesBackend",
+"MetastoreBackend",
+]
+
+@conf_vars({("secrets", "backends_order"): "custom,metastore"})
+def test_backends_order_no_environment_variable_backend(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
+
+@conf_vars({("secrets", "backends_order"): "environment_variable"})
+def test_backends_order_no_metastore_backend(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
+
+@conf_vars({("secrets", "backends_order"):
"metastore,environment_variable,unsupported"})
+def test_backends_order_unsupported(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
Review Comment:
I cannot find usage of this decorator with pytest.mark.parametrize. So if it
is not so important can we leave it as it is?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
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_r2257242842
##
airflow-core/src/airflow/api_fastapi/core_api/routes/public/config.py:
##
@@ -155,3 +155,36 @@ def get_config_value(
config = Config(sections=[ConfigSection(name=section,
options=[ConfigOption(key=option, value=value)])])
return _response_based_on_accept(accept, config)
+
+
+@config_router.get(
+"/backends_order",
+responses={
+**create_openapi_http_exception_doc(
+[
+status.HTTP_404_NOT_FOUND,
+status.HTTP_406_NOT_ACCEPTABLE,
+]
+),
+status.HTTP_200_OK: {
+"description": "Successful Response",
+"content": text_example_response_for_get_config_value,
+},
+},
+response_model=Config,
+dependencies=[Depends(requires_access_configuration("GET"))],
+)
+def get_backends_order_value(
Review Comment:
Hello @jason810496 agree. Moved.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
jason810496 commented on code in PR #45931:
URL: https://github.com/apache/airflow/pull/45931#discussion_r2252043506
##
airflow-core/src/airflow/api_fastapi/core_api/routes/public/config.py:
##
@@ -155,3 +155,36 @@ def get_config_value(
config = Config(sections=[ConfigSection(name=section,
options=[ConfigOption(key=option, value=value)])])
return _response_based_on_accept(accept, config)
+
+
+@config_router.get(
+"/backends_order",
+responses={
+**create_openapi_http_exception_doc(
+[
+status.HTTP_404_NOT_FOUND,
+status.HTTP_406_NOT_ACCEPTABLE,
+]
+),
+status.HTTP_200_OK: {
+"description": "Successful Response",
+"content": text_example_response_for_get_config_value,
+},
+},
+response_model=Config,
+dependencies=[Depends(requires_access_configuration("GET"))],
+)
+def get_backends_order_value(
Review Comment:
No problem! That sounds reasonable. If that's the case, I think we can move
on to the UI-specific routes.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
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_r2251761667
##
airflow-core/src/airflow/api_fastapi/core_api/routes/public/config.py:
##
@@ -155,3 +155,36 @@ def get_config_value(
config = Config(sections=[ConfigSection(name=section,
options=[ConfigOption(key=option, value=value)])])
return _response_based_on_accept(accept, config)
+
+
+@config_router.get(
+"/backends_order",
+responses={
+**create_openapi_http_exception_doc(
+[
+status.HTTP_404_NOT_FOUND,
+status.HTTP_406_NOT_ACCEPTABLE,
+]
+),
+status.HTTP_200_OK: {
+"description": "Successful Response",
+"content": text_example_response_for_get_config_value,
+},
+},
+response_model=Config,
+dependencies=[Depends(requires_access_configuration("GET"))],
+)
+def get_backends_order_value(
Review Comment:
Hello @jason810496 ! Thank you for your review. Regarding the new endpoint.
I decided to do so because get_config_value can be restricted to all users
including admin by deployment managers and not allow to see any option.
_check_expose_config() prevents from doing so. In order not to change the
logic of this endpoint I added get_backends_order_value.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Add config option [secrets]backends_order [airflow]
jason810496 commented on code in PR #45931:
URL: https://github.com/apache/airflow/pull/45931#discussion_r2251688093
##
airflow-core/tests/unit/always/test_secrets.py:
##
@@ -119,6 +120,40 @@ def test_backend_fallback_to_env_var(self,
mock_get_connection):
assert conn.get_uri() == "mysql://airflow:airflow@host:5432/airflow"
+@conf_vars(
+{
+(
+"secrets",
+"backend",
+):
"airflow.providers.amazon.aws.secrets.systems_manager.SystemsManagerParameterStoreBackend",
+("secrets", "backend_kwargs"): '{"connections_prefix": "/airflow",
"profile_name": null}',
+("secrets", "backends_order"):
"custom,environment_variable,metastore",
+}
+)
+def test_backends_order(self):
+backends = ensure_secrets_loaded()
+backend_classes = [backend.__class__.__name__ for backend in backends]
+assert backend_classes == [
+"SystemsManagerParameterStoreBackend",
+"EnvironmentVariablesBackend",
+"MetastoreBackend",
+]
+
+@conf_vars({("secrets", "backends_order"): "custom,metastore"})
+def test_backends_order_no_environment_variable_backend(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
+
+@conf_vars({("secrets", "backends_order"): "environment_variable"})
+def test_backends_order_no_metastore_backend(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
+
+@conf_vars({("secrets", "backends_order"):
"metastore,environment_variable,unsupported"})
+def test_backends_order_unsupported(self):
+with pytest.raises(AirflowConfigException):
+ensure_secrets_loaded()
Review Comment:
Small nits, no strong opnion.
Maybe we can combine the test cases with `pytest.mark.parameterize`.
##
airflow-core/src/airflow/api_fastapi/core_api/routes/public/config.py:
##
@@ -155,3 +155,36 @@ def get_config_value(
config = Config(sections=[ConfigSection(name=section,
options=[ConfigOption(key=option, value=value)])])
return _response_based_on_accept(accept, config)
+
+
+@config_router.get(
+"/backends_order",
+responses={
+**create_openapi_http_exception_doc(
+[
+status.HTTP_404_NOT_FOUND,
+status.HTTP_406_NOT_ACCEPTABLE,
+]
+),
+status.HTTP_200_OK: {
+"description": "Successful Response",
+"content": text_example_response_for_get_config_value,
+},
+},
+response_model=Config,
+dependencies=[Depends(requires_access_configuration("GET"))],
+)
+def get_backends_order_value(
Review Comment:
It seems we can reuse with `get_config_value` route.
Or if the `get_backends_order_value` is necessary, it will be better to move
the route under `airflow-core/src/airflow/api_fastapi/core_api/routes/ui`
##
airflow-core/src/airflow/api_fastapi/core_api/routes/public/config.py:
##
@@ -155,3 +155,36 @@ def get_config_value(
config = Config(sections=[ConfigSection(name=section,
options=[ConfigOption(key=option, value=value)])])
return _response_based_on_accept(accept, config)
+
+
+@config_router.get(
+"/backends_order",
+responses={
+**create_openapi_http_exception_doc(
+[
+status.HTTP_404_NOT_FOUND,
+status.HTTP_406_NOT_ACCEPTABLE,
+]
+),
+status.HTTP_200_OK: {
+"description": "Successful Response",
+"content": text_example_response_for_get_config_value,
+},
+},
+response_model=Config,
+dependencies=[Depends(requires_access_configuration("GET"))],
+)
+def get_backends_order_value(
Review Comment:
May I ask why do we need "backends_order" specific new endpoint?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
