You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "lubimow-xwf (via GitHub)" <gi...@apache.org> on 2026/07/29 12:40:38 UTC
[PR] Secrets backends order updated [airflow]
lubimow-xwf opened a new pull request, #70681:
URL: https://github.com/apache/airflow/pull/70681
Takeover of https://github.com/apache/airflow/pull/45931
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Secrets backends order updated [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Secrets backends order updated [airflow]
Posted by "lubimow-xwf (via GitHub)" <gi...@apache.org>.
lubimow-xwf commented on code in PR #70681:
URL: https://github.com/apache/airflow/pull/70681#discussion_r3829638684
##########
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:
Replaced with StatsCard.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Secrets backends order updated [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #70681:
URL: https://github.com/apache/airflow/pull/70681#issuecomment-5177198361
In the meantime - please tak a look at potential CI issues
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Secrets backends order updated [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #70681:
URL: https://github.com/apache/airflow/pull/70681#discussion_r3873767794
##########
airflow-core/src/airflow/configuration.py:
##########
@@ -740,6 +741,55 @@ def get_custom_secret_backend(worker_mode: bool = False) -> BaseSecretsBackend |
return conf._get_custom_secret_backend(worker_mode=worker_mode)
+class Backend(enum.Enum):
+ """Known secrets backends."""
+
+ ENVIRONMENT_VARIABLE = "environment_variable"
+ EXECUTION_API = "execution_api"
+ CUSTOM = "custom"
+ METASTORE = "metastore"
+
+ @classmethod
+ def from_module(cls, default_backend: str) -> Backend:
Review Comment:
These three literals are exactly the ones `scripts/ci/prek/check_secrets_search_path_sync.py` exists to keep in sync (registered at `.pre-commit-config.yaml:588`) — but that hook only extracts and compares the two list constants, `DEFAULT_SECRETS_SEARCH_PATH` in core and `_SERVER_DEFAULT_SECRETS_SEARCH_PATH` in task-sdk. It has no visibility into `from_module`.
Restoring the constants addressed the first half of my earlier comment, but the coupling moved rather than went away — and it now exists in two copies. This block is duplicated verbatim in `task-sdk/src/airflow/sdk/configuration.py` (lines 272-321): 50 lines identical except `from airflow.configuration import conf` vs `from airflow.sdk.configuration import conf`.
The concrete failure: add a fourth backend to `DEFAULT_SECRETS_SEARCH_PATH`. The hook stays green, because it only checks that the two constants match each other and they both moved. Then `initialize_secrets_backends` raises `ValueError: Unknown module provided: <class path>` on every start, in both core and the SDK.
`shared/configuration/` is the natural home for `Backend`, `from_module` and `_get_secrets_backend_order` — one definition instead of two, and the class-path mapping ends up next to the constants the hook already guards.
##########
task-sdk/src/airflow/sdk/configuration.py:
##########
@@ -267,6 +269,55 @@ def get_custom_secret_backend(worker_mode: bool = False):
return conf._get_custom_secret_backend(worker_mode=worker_mode)
+class Backend(enum.Enum):
Review Comment:
This block is identical to `airflow-core/src/airflow/configuration.py` lines 744-793 apart from the `conf` import — I diffed the two. See the comment there for the drift hazard; moving this into `shared/configuration/` would let both sides import a single definition.
##########
airflow-core/src/airflow/config_templates/config.yml:
##########
@@ -1589,6 +1589,22 @@ secrets:
sensitive: true
example: ~
default: ""
+ backends_order:
+ description: |
+ .. note:: |experimental|
+
+ 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:
Single backticks are RST's default role rather than literal markup, so these render as italics instead of code. The `workers.backends_order` description immediately below uses double backticks for the same two values.
```suggestion
Please note that the ``environment_variable`` and ``metastore`` are required values and cannot be removed
```
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Secrets backends order updated [airflow]
Posted by "olayiwola-gbenga (via GitHub)" <gi...@apache.org>.
olayiwola-gbenga commented on PR #70681:
URL: https://github.com/apache/airflow/pull/70681#issuecomment-5907672407
> 1. This breaks the airflow entrypoint for everyone who is running airflow and accidentally adds a bad config. The worst part is they cannot even use `airflow config get-value` to debug.
> 2. I also noticed this while trying to view my variables:
>
> <img alt="image" width="2000" height="718" src="https://private-user-images.githubusercontent.com/35884252/661061498-10478749-c663-4bb8-9659-81899bb157e3.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3OTA3NTA2OTUsIm5iZiI6MTc5MDc1MDM5NSwicGF0aCI6Ii8zNTg4NDI1Mi82NjEwNjE0OTgtMTA0Nzg3NDktYzY2My00YmI4LTk2NTktODE4OTliYjE1N2UzLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNjA5MzAlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjYwOTMwVDA2Mzk1NVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTNmMzRkOGViNjZmYTIwNDg3MTQ1MzMyNzdmNTMxOTI4OTE4MDRiYzE4YjcxYjI0MjQ1YjIwMWVlZDgxYTljNmYmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JnJlc3BvbnNlLWNvbnRlbnQtdHlwZT1pbWFnZSUyRnBuZyJ9.MgD7IGdmBsgPq49mJL_BXjQEIDLnXVIQRrSWTDZs8qU">
Thanks for pointing this out. The `403 Forbidden` error happens in the modal because `_check_expose_config()` was added to `GET /ui/backends_order` during an earlier review, and `[api] expose_config` is `False` by default.
We can fix this by reading `backends_order` from the existing `/ui/config` endpoint instead so it works out of the box without needing `expose_config=True`.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Secrets backends order updated [airflow]
Posted by "lubimow-xwf (via GitHub)" <gi...@apache.org>.
lubimow-xwf commented on code in PR #70681:
URL: https://github.com/apache/airflow/pull/70681#discussion_r3820850225
##########
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:
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Secrets backends order updated [airflow]
Posted by "olayiwola-gbenga (via GitHub)" <gi...@apache.org>.
olayiwola-gbenga commented on PR #70681:
URL: https://github.com/apache/airflow/pull/70681#issuecomment-5870869237
> > Please have a look again, I did move the ordering code to the shared module.
>
> LGTM. Thanks for being receptive.
>
> > Well you had it hardcoded all around the project, I tried to address this issue too, I hope its fine now.
>
> Yeah. We always try to leave the project better with every change, not worse, so thanks for that !
>
> cc: @amoghrajesh @ephraimbuddy @kaxil - I would love if you take a look before I merge it finally.
Hi @potiuk, I have rebased the branch onto `main` to resolve the latest merge conflicts.
Since this PR has been open for a while and keeps picking up conflicts as `main` moves forward and the other reviewers mentioned above haven't had a chance to take a look yet, would you be able to take a look at it?
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Secrets backends order updated [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #70681:
URL: https://github.com/apache/airflow/pull/70681#issuecomment-5872228227
cc: @kaxil @amoghrajesh @ephraimbuddy - > I merged it, it looks good and indeed would be great to have it in 3.4.0 finally. I guess you might also later take a look at it - and @vatsrahul1001 -> something to take a look more closely at test plan / tests.
(Sorry @olayiwola-gbenga for such long process).
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Secrets backends order updated [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #70681:
URL: https://github.com/apache/airflow/pull/70681#issuecomment-5887025129
@VladaZakharova @lubimow-xwf I will have to attempt reverting this PR: https://github.com/apache/airflow/pull/73891, the review comments I left last are important and need to be handled.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Secrets backends order updated [airflow]
Posted by "olayiwola-gbenga (via GitHub)" <gi...@apache.org>.
olayiwola-gbenga commented on code in PR #70681:
URL: https://github.com/apache/airflow/pull/70681#discussion_r4142799868
##########
airflow-core/src/airflow/configuration.py:
##########
@@ -760,17 +761,17 @@ def initialize_secrets_backends(
from airflow.models import Connection
custom_secret_backend._set_connection_class(Connection)
- backend_list.append(custom_secret_backend)
+ backend_list.append((Backend.CUSTOM, custom_secret_backend))
for class_name in default_backends:
from airflow.models import Connection
secrets_backend_cls = import_string(class_name)
backend = secrets_backend_cls()
backend._set_connection_class(Connection)
- backend_list.append(backend)
+ backend_list.append((Backend.from_path(class_name), backend))
- return backend_list
+ return sorted_backends(conf, backend_list, worker_mode)
Review Comment:
Good catch, thank you! Because `secrets_backend_list = initialize_secrets_backends()` runs at module import time at the bottom of `configuration.py`, any config error in `backends_order` crashes every CLI command on startup.
We will make `secrets_backend_list` load lazily on first secret lookup via `ensure_secrets_loaded()` so commands like `airflow config get-value` and `airflow version` still work when `backends_order` is misconfigured.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Secrets backends order updated [airflow]
Posted by "lubimow-xwf (via GitHub)" <gi...@apache.org>.
lubimow-xwf commented on code in PR #70681:
URL: https://github.com/apache/airflow/pull/70681#discussion_r3878887080
##########
airflow-core/src/airflow/configuration.py:
##########
@@ -740,6 +741,55 @@ def get_custom_secret_backend(worker_mode: bool = False) -> BaseSecretsBackend |
return conf._get_custom_secret_backend(worker_mode=worker_mode)
+class Backend(enum.Enum):
+ """Known secrets backends."""
+
+ ENVIRONMENT_VARIABLE = "environment_variable"
+ EXECUTION_API = "execution_api"
+ CUSTOM = "custom"
+ METASTORE = "metastore"
+
+ @classmethod
+ def from_module(cls, default_backend: str) -> Backend:
Review Comment:
Quickly regarding duplication. I know it's duplicated, I copied it myself :) I wasn't aware there is a shared space, I thought you wanted core and sdk to be fully decoupled. By the way, why don't you move DEFAULT_SECRETS_SEARCH_PATH to shared space too? Wouldn't it be easier than having a script to compare them?
> The concrete failure: add a fourth backend to DEFAULT_SECRETS_SEARCH_PATH. The hook stays green, because it only checks that the two constants match each other and they both moved. Then initialize_secrets_backends raises ValueError: Unknown module provided: <class path> on every start, in both core and the SDK.
That's not a failure. Adding a new backend to that list and not extending the enum would be a bug. I think it's best to fail fast and let the developer fix the issues.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Secrets backends order updated [airflow]
Posted by "lubimow-xwf (via GitHub)" <gi...@apache.org>.
lubimow-xwf commented on code in PR #70681:
URL: https://github.com/apache/airflow/pull/70681#discussion_r3828601777
##########
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:
Restored to the original shape.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Secrets backends order updated [airflow]
Posted by "lubimow-xwf (via GitHub)" <gi...@apache.org>.
lubimow-xwf commented on code in PR #70681:
URL: https://github.com/apache/airflow/pull/70681#discussion_r3828594983
##########
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:
Restored to the original shape.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Secrets backends order updated [airflow]
Posted by "lubimow-xwf (via GitHub)" <gi...@apache.org>.
lubimow-xwf commented on code in PR #70681:
URL: https://github.com/apache/airflow/pull/70681#discussion_r3828600173
##########
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:
Restored to the original shape.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Secrets backends order updated [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #70681:
URL: https://github.com/apache/airflow/pull/70681#issuecomment-5442283478
cc: @amoghrajesh @ephraimbuddy @kaxil -> I would also love to see your review here.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Secrets backends order updated [airflow]
Posted by "lubimow-xwf (via GitHub)" <gi...@apache.org>.
lubimow-xwf commented on PR #70681:
URL: https://github.com/apache/airflow/pull/70681#issuecomment-5452476193
Hey @potiuk
Please have a look again, I did move the ordering code to the shared module.
Regarding
> Backend.from_module re-embeds the three literals that check-secrets-search-path-sync exists to keep in sync
Well you had it hardcoded all around the project, I tried to address this issue too, I hope its fine now.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Secrets backends order updated [airflow]
Posted by "lubimow-xwf (via GitHub)" <gi...@apache.org>.
lubimow-xwf commented on code in PR #70681:
URL: https://github.com/apache/airflow/pull/70681#discussion_r3878891609
##########
task-sdk/src/airflow/sdk/configuration.py:
##########
@@ -267,6 +269,55 @@ def get_custom_secret_backend(worker_mode: bool = False):
return conf._get_custom_secret_backend(worker_mode=worker_mode)
+class Backend(enum.Enum):
Review Comment:
Addressed in the other thread.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Secrets backends order updated [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #70681:
URL: https://github.com/apache/airflow/pull/70681#issuecomment-5453087483
> Please have a look again, I did move the ordering code to the shared module.
LGTM. Thanks for being receptive.
> Well you had it hardcoded all around the project, I tried to address this issue too, I hope its fine now.
Yeah. We always try to leave the project better with every change, not worse, so thanks for that !
cc: @amoghrajesh @ephraimbuddy @kaxil - I would love if you take a look before I merge it finally.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Secrets backends order updated [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #70681:
URL: https://github.com/apache/airflow/pull/70681
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Secrets backends order updated [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #70681:
URL: https://github.com/apache/airflow/pull/70681#issuecomment-5177193912
Assigning to myself - will take a look - but irmt might not be very quick
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Secrets backends order updated [airflow]
Posted by "lubimow-xwf (via GitHub)" <gi...@apache.org>.
lubimow-xwf commented on code in PR #70681:
URL: https://github.com/apache/airflow/pull/70681#discussion_r3829640559
##########
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:
Applied both fixes.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Secrets backends order updated [airflow]
Posted by "lubimow-xwf (via GitHub)" <gi...@apache.org>.
lubimow-xwf commented on PR #70681:
URL: https://github.com/apache/airflow/pull/70681#issuecomment-5369372339
Hey @potiuk
I addressed all of your comments.
I've made significant changes to both `configuration.py` files. I believe I simplified the changes, now sorting does not change the existing implementation too much, it only extends it. I apologize I did it now but this wasn't my PR originally and it was approved before, so I thought it was a matter of a couple of minor fixes.
Please take a look again and let me know what it looks like now, thanks
Btw, just quickly regarding UI changes, right now this change adds a button to show a modal to show a single line of text. Was it discussed before where and what should it look like? If it was me, I would just add a note in the footer of the Variables table, without any additional clicks. But that's just my five cents, right now it's unchanged.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Secrets backends order updated [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #70681:
URL: https://github.com/apache/airflow/pull/70681#discussion_r4131477172
##########
airflow-core/src/airflow/configuration.py:
##########
@@ -760,17 +761,17 @@ def initialize_secrets_backends(
from airflow.models import Connection
custom_secret_backend._set_connection_class(Connection)
- backend_list.append(custom_secret_backend)
+ backend_list.append((Backend.CUSTOM, custom_secret_backend))
for class_name in default_backends:
from airflow.models import Connection
secrets_backend_cls = import_string(class_name)
backend = secrets_backend_cls()
backend._set_connection_class(Connection)
- backend_list.append(backend)
+ backend_list.append((Backend.from_path(class_name), backend))
- return backend_list
+ return sorted_backends(conf, backend_list, worker_mode)
Review Comment:
This makes a bad `[secrets] backends_order` cause debugging airflow deployment to go into a deadlock.
Look at this:
```shell
[Breeze:3.11.16] root@23c5082cc375:/opt/airflow$ airflow config get-value secrets backends_order
custom,environment_variable,metastore
[Breeze:3.11.16] root@23c5082cc375:/opt/airflow$ airflow config get-value secrets backends_order
^C
[Breeze:3.11.16] root@23c5082cc375:/opt/airflow$ export AIRFLOW__SECRETS__BACKENDS_ORDER=environment_variable,metastor
airflow config get-value secrets backends_order
Traceback (most recent call last):
File "/usr/python/bin/airflow", line 4, in <module>
from airflow.__main__ import main
File "/opt/airflow/airflow-core/src/airflow/__init__.py", line 64, in <module>
from airflow import configuration, settings
File "/opt/airflow/airflow-core/src/airflow/configuration.py", line 821, in <module>
secrets_backend_list = initialize_secrets_backends()
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/opt/airflow/airflow-core/src/airflow/configuration.py", line 774, in initialize_secrets_backends
return sorted_backends(conf, backend_list, worker_mode)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/opt/airflow/airflow-core/src/airflow/_shared/configuration/secrets_backends.py", line 87, in sorted_backends
backends_order = _get_secrets_backend_order(conf, [b[0] for b in backend_list], worker_mode)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/opt/airflow/airflow-core/src/airflow/_shared/configuration/secrets_backends.py", line 69, in _get_secrets_backend_order
raise AirflowConfigException(
airflow._shared.configuration.exceptions.AirflowConfigException: The configuration option [secrets]backends_order is misconfigured. The following backend types are unsupported: ['metastor']
[Breeze:3.11.16] root@23c5082cc375:/opt/airflow$ airflow version
Traceback (most recent call last):
File "/usr/python/bin/airflow", line 4, in <module>
from airflow.__main__ import main
File "/opt/airflow/airflow-core/src/airflow/__init__.py", line 64, in <module>
from airflow import configuration, settings
File "/opt/airflow/airflow-core/src/airflow/configuration.py", line 821, in <module>
secrets_backend_list = initialize_secrets_backends()
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/opt/airflow/airflow-core/src/airflow/configuration.py", line 774, in initialize_secrets_backends
return sorted_backends(conf, backend_list, worker_mode)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/opt/airflow/airflow-core/src/airflow/_shared/configuration/secrets_backends.py", line 87, in sorted_backends
backends_order = _get_secrets_backend_order(conf, [b[0] for b in backend_list], worker_mode)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/opt/airflow/airflow-core/src/airflow/_shared/configuration/secrets_backends.py", line 69, in _get_secrets_backend_order
raise AirflowConfigException(
airflow._shared.configuration.exceptions.AirflowConfigException: The configuration option [secrets]backends_order is misconfigured. The following backend types are unsupported: ['metastor']
```
Rejecting a bad value is right behaviour but this bricks every entrypoint with a raw traceback, which isnt right at all.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org