potiuk commented on code in PR #70681:
URL: https://github.com/apache/airflow/pull/70681#discussion_r3778989846
##########
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__ = [
Review Comment:
`DEFAULT_SECRETS_SEARCH_PATH` was in `__all__` and re-exported here, so
`from airflow.secrets import DEFAULT_SECRETS_SEARCH_PATH` was public API. After
this change it raises `AttributeError`.
The sibling symbol in this same file, `DEFAULT_SECRETS_SEARCH_PATH_WORKERS`,
gets a `__getattr__` shim with a `DeprecatedImportWarning` and a back-compat
fallback. This one gets nothing. Nothing in-tree still imports it, but
out-of-tree code may.
Please either add the same deprecation shim, or keep the removal and
document it in a `.significant.rst` newsfragment.
##########
task-sdk/src/airflow/sdk/configuration.py:
##########
@@ -340,3 +410,6 @@ def __getattr__(name: str):
globals()[name] = val
return val
raise AttributeError(f"module '{__name__}' has no attribute '{name}'")
+
+
+secrets_backend_list = initialize_secrets_backends()
Review Comment:
This runs at module import, and `airflow.sdk.configuration` is imported at
module level by `sdk/bases/sensor.py`, `sdk/definitions/asset/__init__.py` and
`sdk/definitions/_internal/abstractoperator.py` — so it executes on every SDK
import, i.e. every Dag parse. Previously this was computed lazily inside
`ensure_secrets_loaded()`.
Two effects worth weighing:
- With the default `[secrets] backends_order`, this imports and constructs
`airflow.secrets.metastore.MetastoreBackend` — a core, metadata-DB-backed
backend — inside the Dag File Processor and on workers, purely as an import
side effect.
- A misconfigured `backends_order` now raises `AirflowConfigException` at
import time, so it surfaces as a Dag import error rather than a config error at
first secrets lookup.
I realise core does the same at module scope and this is probably
consistency with it — but core is the heavyweight side, and this module's
`conf` is deliberately lazy behind `__getattr__`, which this line forces
eagerly. Could this stay lazy in the SDK?
##########
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 = null,
+}: {
+ readonly colorScheme: string;
+ readonly icon?: React.ReactNode;
+ readonly isLoading?: boolean;
+ readonly label: string;
+ readonly link?: string;
+ readonly onClick?: () => void;
+ readonly state?: TaskInstanceState | null;
Review Comment:
This component is a near-verbatim copy of `src/components/StatsCard.tsx`
with `count` and `isRTL` removed. It keeps `state?: TaskInstanceState | null`,
the `StateBadge` and the `link`/`RouterLink` branch — none of which the single
call site in `BackendsOrderCard` uses, and task-instance state has no meaning
for a settings button.
It also drops `StatsCard`'s RTL handling (`isRTL ? <FiChevronLeft /> :
<FiChevronRight />`), so the chevron points the wrong way in the `ar` and `he`
locales.
Could this just use `StatsCard` directly? If a separate component is
genuinely needed, please drop the unused props and keep the RTL branch.
##########
airflow-core/src/airflow/ui/src/pages/Variables/BackendsOrderModal.tsx:
##########
@@ -0,0 +1,67 @@
+/*!
+ * 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 { Heading, Text, HStack } from "@chakra-ui/react";
+import { useEffect, useState } from "react";
+import { useTranslation } from "react-i18next";
+import { LuSettings } from "react-icons/lu";
+
+import { useConfigServiceGetBackendsOrderValue } from "openapi/queries";
+import { ErrorAlert } from "src/components/ErrorAlert";
+import { Dialog } from "src/components/ui";
+
+type BackendsOrderModalProps = {
+ onClose: () => void;
+ open: boolean;
+};
+
+export const BackendsOrderModal: React.FC<BackendsOrderModalProps> = ({
onClose, open }) => {
+ const { t: translate } = useTranslation("admin");
+ const [backendsOrder, setBackendsOrder] = useState<Array<string> | string>();
+ const { data, error } = useConfigServiceGetBackendsOrderValue();
+
+ const onOpenChange = () => {
+ onClose();
+ };
+
+ useEffect(() => {
+ setBackendsOrder(data?.sections[0]?.options[0]?.value ?? "");
+ }, [data, open]);
Review Comment:
`backendsOrder` is derived entirely from `data`, so this is the `useState +
useEffect` sync pattern the review guidelines call out:
> Avoid `useState + useEffect` to sync derived state. Use nullish coalescing
or nullable override patterns instead.
— `.github/instructions/code-review.instructions.md` § UI Code
```suggestion
const { data, error } = useConfigServiceGetBackendsOrderValue();
const backendsOrder = data?.sections[0]?.options[0]?.value ?? "";
const onOpenChange = () => {
onClose();
};
```
That also lets the `useState`/`useEffect` imports go, along with the
`Array<string> | string` type (the value is only ever a string).
Unrelated but nearby: this modal is always mounted by `BackendsOrderCard`,
so the query fires on every Variables page render even when the dialog is
closed — `enabled: open` would avoid that.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/config.py:
##########
@@ -74,3 +81,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 handler is `get_config_value` from `routes/public/config.py` with both
of its guards removed: `requires_access_configuration("GET")` became
`requires_authenticated()`, and `_check_expose_config()` is gone.
`[api] expose_config` defaults to `"False"`, and `_check_expose_config()`
raises 403 "Your Airflow administrator chose not to expose the configuration,
most likely for security reasons." So on a default install, `GET
/api/v2/config/section/secrets/option/backends_order` returns 403 while this
returns 200 with the value to the lowest-privileged authenticated user.
`backends_order` isn't a secret, so I'm not calling this a vulnerability —
but it is a control the deployment manager chose, and the same value now has
two different authorization answers.
The neighbouring `/ui/config` endpoint is authenticated-only because it
serves a fixed curated allowlist (`API_CONFIG_KEYS` plus `instance_name`,
`theme`, …), not arbitrary config reads. Folding `backends_order` into that
`get_configs()` payload would match the established pattern; otherwise please
restore both guards.
Either way `TestGetBackendsOrder` should gain an `unauthorized_test_client`
case and an `expose_config=False` case — neither is covered today.
##########
airflow-core/src/airflow/configuration.py:
##########
@@ -740,35 +739,103 @@ 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 = (
Review Comment:
These three class paths are the ones
`scripts/ci/prek/check_secrets_search_path_sync.py` exists to keep in sync
(registered at `.pre-commit-config.yaml:588`) — it compares core's
`DEFAULT_SECRETS_SEARCH_PATH` against task-sdk's
`_SERVER_DEFAULT_SECRETS_SEARCH_PATH`.
This PR stops using both constants and embeds the same literals here and
again in `task-sdk/src/airflow/sdk/configuration.py`. That's four copies, of
which the hook only checks two — so the two new ones can drift silently, which
is the exact failure the hook was written to prevent.
Cleanest fix is to build `backends_map` from the existing constants. Failing
that, please extend the hook to cover these literals 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]
