Add config option [secrets]backends_order#45931
Conversation
moiseenkov
force-pushed
the
secrets-backends-order
branch
from
61b5ab6 to
4489808
Compare
moiseenkov
force-pushed
the
secrets-backends-order
branch
from
4489808 to
58d80f3
Compare
|
@eladkal , please take a look at the updates. |
|
I was initially against making it configurable, but seeing the simplicity and flexibility, I am in. |
|
@eladkal ? |
|
hi there! |
|
We are on feature freeze for Airflow 3. |
|
Yeah. I think that might be 3.1 |
Crowiant
force-pushed
the
secrets-backends-order
branch
3 times, most recently
from
8c516f8 to
347e2f1
Compare
eladkal
left a comment
eladkal
left a comment
There was a problem hiding this comment.
prevent accidental merge. We are on feature freeze for Airflow 3.
PR can not be merged till main branch is for 3.1
Crowiant
force-pushed
the
secrets-backends-order
branch
from
347e2f1 to
2b750de
Compare
Crowiant
force-pushed
the
secrets-backends-order
branch
from
2b750de to
4c0d958
Compare
|
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. |
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 |
Crowiant
force-pushed
the
secrets-backends-order
branch
from
41b4d5d to
4ff5632
Compare
|
Can you please resolve conflicts and fix the remaining static check? |
Crowiant
force-pushed
the
secrets-backends-order
branch
3 times, most recently
from
fee065b to
9a7803e
Compare
|
Hello @potiuk I updated the PR. Since checks are passing, can we merge if there are no objections? |
|
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? |
Crowiant
force-pushed
the
secrets-backends-order
branch
3 times, most recently
from
c1085eb to
903e4fb
Compare
|
@eladkal - looks like your concerns are alleviated - can you please remove "request changes"? |
|
Hello @amoghrajesh I rebased and set the backends_order feature for Airflow 3.2.1 @eladkal could you please re-review this PR? |
|
@moiseenkov This PR has a few issues that need to be addressed before it can be reviewed — please see our Pull Request quality criteria. Issues found:
What to do next:
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. 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 so that our maintainers' limited time is spent where it matters most: the conversation with you. |
@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. |
|
Yes we certainly lost track of it |
|
As soon as we deal with the current overload we shall come back to it. |
|
@amoghrajesh -> you had comments on it, can you re-review as well? I thnk this one is indeed long overdue. |
There was a problem hiding this comment.
Pull request overview
This PR introduces configurable ordering for secrets backend lookup via a new backends_order option (for both [secrets] and [workers]), updates docs/config templates accordingly, and adds a small UI surface (plus a UI API endpoint) to display the configured order.
Changes:
- Add
[secrets]backends_orderand[workers]backends_orderto control secrets backend lookup order, including validation and new unit tests. - Add a new authenticated UI endpoint (
GET /ui/backends_order) and generated OpenAPI client bindings. - Add a Variables-page UI control/modal to display the current secrets backends order.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| task-sdk/tests/task_sdk/execution_time/test_context.py | Removes a server-chain assertion related to the previous fixed default search path. |
| task-sdk/src/airflow/sdk/configuration.py | Implements backends_order parsing/validation and backend instantiation logic in the Task SDK. |
| airflow-core/tests/unit/always/test_secrets.py | Adds coverage for ordering + invalid configurations (server + workers). |
| airflow-core/src/airflow/ui/src/pages/Variables/Variables.tsx | Adds the backends-order UI entry point (but currently duplicates an existing button). |
| airflow-core/src/airflow/ui/src/pages/Variables/BackendsOrder*.tsx | New UI components to fetch and display the configured order. |
| airflow-core/src/airflow/ui/public/i18n/locales/en/admin.json | Adds UI label translation for the new control. |
| airflow-core/src/airflow/ui/openapi-gen/** | Regenerated client types/services/queries for the new endpoint. |
| airflow-core/src/airflow/secrets/init.py | Adjusts public exports from airflow.secrets. |
| airflow-core/src/airflow/configuration.py | Implements backends_order parsing/validation and backend instantiation logic in core. |
| airflow-core/src/airflow/config_templates/config.yml | Documents the new config options and sets defaults matching current behavior. |
| airflow-core/src/airflow/api_fastapi/core_api/routes/ui/config.py | Adds /backends_order UI endpoint returning the configured value. |
| airflow-core/src/airflow/api_fastapi/core_api/openapi/_private_ui.yaml | Adds OpenAPI path + schemas for /ui/backends_order. |
| airflow-core/docs/security/secrets/secrets-backend/index.rst | Documents the new ordering options for server and workers. |
Comments suppressed due to low confidence (1)
airflow-core/src/airflow/ui/src/pages/Variables/Variables.tsx:223
ImportVariablesButtonis rendered twice in the same toolbar (<HStack>), which will duplicate the action in the UI. This looks unintentional—please remove one of them (or replace one with the new backends-order control if that was the intent).
<HStack gap={4} mt={2}>
<ImportVariablesButton disabled={selectedRows.size > 0} />
<BackendsOrderCard />
<Spacer />
<ExpandCollapseButtons
collapseLabel={translate("common:expand.collapse")}
expandLabel={translate("common:expand.expand")}
onCollapse={onClose}
onExpand={onOpen}
/>
<Spacer />
<ImportVariablesButton disabled={selectedRows.size > 0} />
<AddVariableButton disabled={selectedRows.size > 0} />
</HStack>
| 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, |
There was a problem hiding this comment.
I don't think that it is needed as there were no such check previously
| __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 | ||
|
|
There was a problem hiding this comment.
Earlier, reviewers didn't request this, so I think it is not strictly necessary
| > | ||
| <StateBadge colorPalette={colorScheme} mr={2} state={state}> | ||
| {icon} | ||
| </StateBadge> | ||
|
|
||
| <Text color="fg" fontSize="sm" fontWeight="bold"> |
| import { BackendsOrderButton } from "src/pages/Variables/BackendsOrderButton"; | ||
| import { BackendsOrderModal } from "src/pages/Variables/BackendsOrderModal"; |
| @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())], | ||
| ) |
| 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: |
There was a problem hiding this comment.
Moved import_string at module scope
| # 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 | ||
|
|
There was a problem hiding this comment.
It is not needed. Other functions in this file use such import.
| # 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 |
There was a problem hiding this comment.
Moved secrets_backend_list as it is in airflow-core
| 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, |
There was a problem hiding this comment.
I don't think that it is needed as there were no such check previously
|
@moiseenkov This PR has a few issues that need to be addressed before it can be reviewed — please see our Pull Request quality criteria. Issues found:
What to do next:
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. 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 so that our maintainers' limited time is spent where it matters most: the conversation with you. |
|
@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 so that our maintainers' limited time is spent where it matters most: the conversation with you. |
|
@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? |
|
@moiseenkov This PR has been converted to draft because it has merge conflicts with Issues found:
What to do next:
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. 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 so that our maintainers' limited time is spent where it matters most: the conversation with you. |
|
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 those threads due to permission issue. |
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Disposition: COMMENT
Per the project's review convention I can't APPROVE while the PR has substantive open conversations — there are currently 16 unresolved review threads (from @amoghrajesh, @uranusjr, @copilot-pull-request-reviewer, etc.) and the branch is 101 commits behind main — so this is a COMMENT pass with observations on top of what's already been raised.
The earlier approvals from me (2025-01-25, 2025-08-07) were against earlier snapshots of the diff. The current diff has grown substantially and I'd want to re-review after the open threads are resolved and the rebase lands.
Where this stands
| Signal | State |
|---|---|
| Mergeable | Yes (currently), but 101 commits behind main — please rebase before the next review pass |
| CI | Green at the moment, but stale relative to current main |
| Unresolved review threads | 16, including multiple from @amoghrajesh and @uranusjr that read as architectural rather than nit-level |
| Existing CHANGES_REQUESTED | @eladkal's CHANGES_REQUESTED from 2025-03-31 is still on the books — would be good to either get it dismissed or re-engaged before another approve cycle |
Additive findings (not duplicating existing thread content)
The unresolved threads already cover the worker-mode docstring inconsistency, the ensure_secrets_loaded double-init concern, the backwards-incompatible removal of DEFAULT_SECRETS_SEARCH_PATH from airflow.secrets.__all__, and the missing tests for the new /backends_order UI endpoint. I won't re-raise those. A few additional points that I didn't see surfaced yet:
-
Code duplication between
airflow-core/src/airflow/configuration.pyandtask-sdk/src/airflow/sdk/configuration.py. The newBackendsenum, theget_importable_secret_backendhelper, and the body ofinitialize_secrets_backends(thebackends_map, therequired_backends/missing_backends/unsupported_backendschecks, the per-entry callback dispatch) are near-verbatim copies — ~150 lines of parallel implementation that has to be kept in lockstep going forward. The repo already has ashared/secrets_backend/workspace member (and ashared/configuration/) — those are the natural home for the shared definitions, with the core and SDK callers consuming them via the symlink scheme described inCLAUDE.mdunder Shared libraries. Without that, every future change to the algorithm (a new backend type, a new validation rule) has to be made and tested twice. -
Module-import-time exception risk in
task-sdk/src/airflow/sdk/configuration.py. The new line at the bottom —secrets_backend_list = initialize_secrets_backends()— runsconf.getlist("secrets", "backends_order", ...)at import time, andinitialize_secrets_backendsraisesAirflowConfigExceptionif the value is missing or invalid. Today the default inconfig.ymlcovers the happy path, but any environment that importsairflow.sdk.configurationbefore the default config has been applied (or with a stripped/custom config that omits the new key) will fail to import the SDK config module entirely. The previous shape returned a list and let the caller decide when to call it. Worth either lazifying this (e.g. compute on first access) or wrapping the call to fall back to the default instead of raising at import. -
Test deletion without replacement.
test_metastore_backend_in_server_chainintask-sdk/tests/task_sdk/execution_time/test_context.pywas removed alongside the removal of_SERVER_DEFAULT_SECRETS_SEARCH_PATH. The invariant it tested — metastore is in the server-side default chain, execution_api is not — is now implicit in therequired_backendsbranching insideinitialize_secrets_backends. That invariant is exactly the kind of thing a future refactor can flip without anyone noticing. Worth adding an explicit test against the newBackends.METASTORE/Backends.EXECUTION_APIrequired-set behavior so the regression remains caught. -
Missing newsfragment. This is a new user-facing config option in
[secrets](and[workers]), with explicit failure modes that didn't exist before — that warrants anairflow-core/newsfragments/45931.feature.rst(or.significant.rstif you want to flag the behavior change for[workers]). Per the repo's CLAUDE.md,task-sdkuser-facing changes ship throughairflow-core/newsfragments/. -
Backendsnaming. Per the project's coding-standard preference to make types read like the thing they describe,BackendType(singular) reads more naturally thanBackendsfor an enum whose members are individual types. Minor and entirely your call.
What I'd find easiest to re-review against
A rebase onto current main, the unresolved threads from @amoghrajesh and @uranusjr either resolved or replied with a clear rationale, a newsfragment, and either (a) extraction of the shared bits into shared/secrets_backend/ or (b) a brief reply explaining why a copy is preferable here. Once those land I'm happy to do another pass — the underlying feature is genuinely useful.
Note: This review was drafted by an AI-assisted code-review tool and may contain mistakes. An Apache Airflow maintainer (myself) reviewed it before posting and will be the human you talk to in the follow-up.
Drafted-by: Claude Code (Opus 4.7, 1M context); reviewed by @potiuk before posting
|
@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 so that our maintainers' limited time is spent where it matters most: the conversation with you. |
Introduce a new configuration option for specifying secret backends load order:
The default value represents current behavior, thus nothing will change for existing users.
Important
🛠️ Maintainer triage note for @moiseenkov · by
@potiuk· 2026-06-22 06:40 UTCFollowing up on a confirmation request from 12 days ago — review feedback from
@amoghrajesh,@uranusjris still waiting:The ball is in your court — Reply or push a fix in each thread, then mark them resolved and ping
@amoghrajesh,@uranusjrwhen ready.Automated triage — may be imperfect; a maintainer takes the next look.