Repository navigation
Warn when connections/variables list output may be incomplete - #72484
Conversation
`airflow connections list` and `airflow variables list` only enumerate rows in the metadata database. Connections and variables can also be defined via `AIRFLOW_CONN_*`/`AIRFLOW_VAR_*` environment variables or a configured secrets backend, and those sources are checked ahead of the database when Airflow actually resolves a connection or variable at runtime -- so an environment variable or secrets-backend entry can silently override a database row with the same ID without ever appearing in the list output. Users had no indication the listing was incomplete, making it easy to assume the database is the only source of truth. This adds a stderr warning to both list commands (so it doesn't interfere with `--output json`/`yaml` piping) whenever an `AIRFLOW_CONN_*`/`AIRFLOW_VAR_*` environment variable or a secrets backend is present, naming the source(s) that may hold additional or overriding entries. This covers the CLI surface only. Extending the same warning to the Web UI would require adding fields to the REST API's connection/variable response models, which are generated from an OpenAPI spec and would require regenerating the TypeScript client -- a much larger, separate change better suited to its own PR. Related: apache#10867
SameerMesiah97
left a comment
There was a problem hiding this comment.
Looks fine. Just some minor comments to address.
ColtenOuO
left a comment
There was a problem hiding this comment.
I think the overall direction looks good, but I have a concern about the maintainability of this detection logic.
Airflow 3 supports a worker-specific secrets backend through [workers] secrets_backend, which the Task SDK prioritizes during secret resolution. The current implementation only checks [secrets] backend, so it misses that case.
More generally, if future Airflow versions introduce secrets backend settings for other components or contexts, the CLI logic would also need to be updated to recognize each new configuration.
Otherwise, the same problem this PR is trying to solve could reappear.
Could we avoid duplicating knowledge of the available secret sources in the CLI?
Alternatively, the CLI could simply display a generic note that the command only lists entries stored in the metadata database and that the result may not represent every source used during runtime, without trying to identify the specific sources.
Workers can override the general secrets backend with their own [workers] secrets_backend setting, which the Task SDK prioritizes during secret resolution; the CLI warning missed that source. Checking both keys keeps the warning accurate for worker-backed deployments. The import moves to module scope since there is no cycle with airflow.configuration, and the wording drops knowledge of which specific backend is configured.
potiuk
left a comment
There was a problem hiding this comment.
Approved this earlier; adding three follow-up observations from a closer read. None of them gates the merge — but the first is a factually wrong comment, and the third is the maintainability concern from the earlier review, which I don't think is fully closed.
airflow-core/tests/unit/cli/commands/test_connection_command.py:152— the comment explaining whyAIRFLOW_CONN_*is cleared names the wrong mechanism.test_connection_command.py:174/test_variable_command.py:346— bothdoes_not_warn_by_defaulttests assertstderr == ""without pinning[secrets] backend/[workers] secrets_backend.airflow-core/src/airflow/cli/utils.py:123— the section/key pairs duplicate what_get_custom_secret_backend()already owns; the drift risk raised earlier in this PR is still open.
Details inline.
For the record, things I checked that are fine: moving from airflow.configuration import conf to module level is not an eager-config-load regression (airflow/__main__.py:36 already imports airflow.configuration, and cli_parser.py:49 pulls in providers_manager), so that change was the right response to the earlier review; AirflowConsole(stderr=True) forwards correctly to rich's Console through **kwargs; both config keys exist with "" defaults, so the any() check behaves; and no newsfragment is needed here per AGENTS.md.
One judgment call I'll leave to the thread rather than file as a finding: with a secrets backend configured — which is the norm in production — this warning now fires on every connections list / variables list invocation, with no way to suppress it. Routing it to stderr keeps pipes clean, so it is probably the right trade, but it is a permanent banner for a lot of users.
This review was drafted by an AI-assisted tool and confirmed by an Airflow maintainer. The findings above are observations, not blockers; an Airflow maintainer — a real person — will take the next look at the PR. If you think a finding is mis-applied, please reply on the PR and a maintainer will weigh in.
More on how Airflow handles maintainer review: contributing-docs/05_pull_requests.rst.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
The secrets-backend (section, key) pairs now live as a single constant in the shared parser, read by both _get_custom_secret_backend() and this warning, so a new backend source only needs to be added once. Also fixes a test comment that named the wrong fixture as the source of ambient AIRFLOW_CONN_* env vars, and pins [secrets] backend / [workers] secrets_backend to empty in the does-not-warn-by-default tests so they don't depend on the ambient shell environment.
|
Thanks for the detailed follow-up — addressed all three in 8a5420e:
All 120 tests in the three affected files pass, plus the shared-parser and airflow-core configuration secret tests (unaffected, confirming the constant refactor didn't change Drafted-by: Claude Code (Sonnet 5) (no human review before posting) |
CodeQL's clear-text-logging query treats any value sourced from an identifier containing "secret" as sensitive data. The (section, key) lookup table added in the previous commit was named SECRETS_BACKEND_CONFIG_KEYS, so section/key strings pulled from it tripped that heuristic wherever they later reach a log statement (_get_custom_secret_backend()'s parse-failure warnings, and the generic config lookup's not-found warning) — even though the values themselves are just config coordinates like "secrets"/"backend", never the secret they point at. Renaming the constant to CUSTOM_BACKEND_CONFIG_KEYS removes the name-based trigger without changing any behavior.
|
CodeQL flagged 4 high-severity "clear-text logging of sensitive information" alerts introduced by the previous commit here, which was blocking the required check despite the approvals above. Root cause: that commit introduced a new lookup table named Pushed a follow-up commit renaming the constant to Drafted-by: Claude Code (Sonnet 5) (no human review before posting) |
potiuk
left a comment
There was a problem hiding this comment.
Thanks — all three follow-ups are addressed, and the constant in the shared parser is a better answer to the drift concern than the comment I suggested: _get_custom_secret_backend() and the CLI now read the same (section, key) pairs, so the next backend source can't be added to one and missed in the other. I traced the _get_custom_secret_backend() refactor including the worker → general fallback, and it behaves identically. Approving.
Two optional notes, neither blocking:
- On the CodeQL rename: those alerts are genuine false positives (the values are config coordinates, not secrets), so renaming works, but dismissing the alerts as "false positive" in the Security tab is the more usual route and would let the constant keep the more descriptive name. Either is fine — your comment explains the choice, which is what matters.
AGENTS.mdprefersconf_varsas a decorator when the values are fixed; the sixwith conf_vars({...}):blocks here all use fixed values.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Airflow maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.More on how Apache Airflow handles maintainer review:
contributing-docs/05_pull_requests.rst.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
airflow connections listandairflow variables listonly enumerate metadata-database rows, but connections/variables can also be defined viaAIRFLOW_CONN_*/AIRFLOW_VAR_*environment variables or a configured secrets backend — sources that take precedence over the database at resolution time and can silently override a DB row with the same ID, yet never appeared in the list output.Adds
get_hidden_entries_warning()(airflow-core/src/airflow/cli/utils.py), used by bothconnections_list()andvariables_list(), printing a warning naming which source(s) — env-var prefix and/or secrets backend — may hold additional entries not visible in the listing. The warning goes to stderr viaAirflowConsole(stderr=True), so--output json/yamlpiping is unaffected.Scoped to the CLI only in this PR — the Web UI listing mentioned in the issue would need new fields on the OpenAPI-generated REST API response models plus a TypeScript client regen, which is a materially larger, separate change. Happy to follow up with that as a second PR if useful.
related: #10867
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Sonnet 5) following the guidelines