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 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:
These imports use an absolute `src/pages/...` path for components that live
in the same directory. In this folder, other components are imported via
relative paths (e.g. `./DeleteVariablesButton` in `Variables.tsx`), so this is
inconsistent and makes refactors harder. Prefer `./BackendsOrderButton` and
`./BackendsOrderModal` here.
##########
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:
Removing `DEFAULT_SECRETS_SEARCH_PATH` from `airflow.secrets`’ public
exports is a backwards-incompatible change for any external code that imports
it from this module. Consider keeping the re-export (potentially with a
deprecation warning) since the constant still exists in
`airflow.secrets.base_secrets` and this PR’s stated goal is adding a new config
option, not removing a public symbol.
##########
airflow-core/src/airflow/ui/src/pages/Variables/BackendsOrderButton.tsx:
##########
@@ -0,0 +1,76 @@
+/*!
+ * 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, HStack, Skeleton, Text } from "@chakra-ui/react";
+import { FiChevronRight } from "react-icons/fi";
+import { Link as RouterLink } from "react-router-dom";
+
+import type { TaskInstanceState } from "openapi/requests/types.gen";
+import { StateBadge } from "src/components/StateBadge";
+
+export const BackendsOrderButton = ({
+ colorScheme,
+ icon,
+ isLoading = false,
+ label,
+ link,
+ onClick,
+ state,
+}: {
+ readonly colorScheme: string;
+ readonly icon?: React.ReactNode;
+ readonly isLoading?: boolean;
+ readonly label: string;
+ readonly link?: string;
+ readonly onClick?: () => void;
+ readonly state?: TaskInstanceState | null;
+}) => {
+ if (isLoading) {
+ return <Skeleton borderRadius="lg" height="42px" width="175px" />;
+ }
+
+ const content = (
+ <HStack
+ alignItems="center"
+ borderRadius="lg"
+ borderWidth={1}
+ color="fg.emphasized"
+ cursor="pointer"
+ p={2}
+ >
+ <StateBadge colorPalette={colorScheme} mr={2} state={state}>
+ {icon}
+ </StateBadge>
+
+ <Text color="fg" fontSize="sm" fontWeight="bold">
Review Comment:
`StateBadge` expects a defined `state` (or `null`) to avoid generating
invalid Chakra color tokens. When `state` is left `undefined` here (as in
`BackendsOrderCard`), `StateBadge` computes `color="undefined.contrast"`, which
can render incorrectly or trigger theme warnings. Use a neutral container (e.g.
`Badge`/`Box`) for the icon, or pass `state={null}` explicitly (and adjust
styling as needed).
##########
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:
The comment and logic here are out of sync: worker mode is determined by
`default_backends is not None`, not by comparing against a server-default list
anymore. Also, `from airflow.sdk.configuration import conf` is a self-import
used just to trigger `__getattr__`; consider refactoring to a local helper
(e.g. `get_conf()`) or explicitly initializing/caching `conf` once, to avoid
self-imports and make the control flow clearer.
##########
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:
Like in core, this required-backends validation only checks that the backend
*type names* exist in `backends_order`, but it doesn’t ensure the required
backend implementations are actually enabled/instantiable (e.g.
`execution_args`/`environment_variable_args` can be set to `None` when they
aren’t present in `default_backends`, and then the backend is silently
skipped). Please validate that required backends have non-None classpaths in
worker mode (or don’t null them out) so workers can’t run with required
backends missing.
##########
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:
`ensure_secrets_loaded` always calls `initialize_secrets_backends()` once,
and then calls it again when `default_backends` is provided (the common worker
case), making the first initialization redundant. This can instantiate secret
backends twice (potentially expensive / side-effectful). Consider passing
`default_backends` through to the first call (or skipping the first call
entirely when `default_backends` is not `None`).
##########
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:
`get_importable_secret_backend` performs an import inside the function body.
Unless this is strictly needed for circular-import avoidance or process
isolation, prefer importing `import_string` at module scope to avoid repeated
imports and to match the project’s import conventions.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/config.py:
##########
@@ -67,3 +74,32 @@ def get_configs() -> ConfigResponse:
config.update({key: value for key, value in additional_config.items()})
return ConfigResponse.model_validate(config)
+
+
+@config_router.get(
+ "/backends_order",
+ responses={
+ **create_openapi_http_exception_doc(
+ [
+ status.HTTP_404_NOT_FOUND,
+ status.HTTP_406_NOT_ACCEPTABLE,
+ ]
+ ),
+ },
+ response_model=Config,
+ dependencies=[Depends(requires_authenticated())],
+)
Review Comment:
This new `/backends_order` UI route isn’t covered by the existing FastAPI UI
config route tests (see
`airflow-core/tests/unit/api_fastapi/core_api/routes/ui/test_config.py`, which
currently only asserts `/config`). Please add tests for the new endpoint (at
least: 200 JSON response, 200 text/plain via `Accept`, and 401 for
unauthenticated).
--
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]
