potiuk commented on code in PR #66022:
URL: https://github.com/apache/airflow/pull/66022#discussion_r3215276647
##########
airflow-core/src/airflow/api_fastapi/execution_api/versions/__init__.py:
##########
@@ -45,11 +45,16 @@
AddStateEndpoints,
AddTeamNameField,
)
+from airflow.api_fastapi.execution_api.versions.v2026_04_28 import
AddVariableKeysEndpoint
from airflow.api_fastapi.execution_api.versions.v2026_06_16 import
AddRetryPolicyFields
bundle = VersionBundle(
HeadVersion(),
Version("2026-06-16", AddRetryPolicyFields),
+ Version(
+ "2026-04-28",
Review Comment:
The version date is **earlier** than the previous entry in the bundle
(`2026-06-16`, `AddRetryPolicyFields`). For an additive change being added in
May 2026, the new version should be placed *after* the most recent existing
version, not before — otherwise:
1. Clients pinned to `2026-06-16` retroactively gain access to an endpoint
that wasn't part of that release.
2. The version-bundle history loses its monotonic-time invariant, which the
rest of the cadwyn migration logic assumes.
Could you bump this to a date after `2026-06-16` (e.g. today's date)? The
`v2026_04_28.py` filename and the test directory under `versions/v2026_04_28/`
would also need to be renamed.
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting
##########
task-sdk/src/airflow/sdk/execution_time/context.py:
##########
@@ -277,6 +277,17 @@ def _get_variable(key: str, deserialize_json: bool) -> Any:
)
+def _get_variable_keys(prefix: str | None = None) -> list[str]:
+ from airflow.sdk.execution_time.comms import GetVariableKeys,
VariableKeysResult
+ from airflow.sdk.execution_time.task_runner import SUPERVISOR_COMMS
+
+ msg = SUPERVISOR_COMMS.send(GetVariableKeys(prefix=prefix))
+ if not isinstance(msg, VariableKeysResult):
+ return []
Review Comment:
Returning `[]` on every non-`VariableKeysResult` response (including
`ErrorResponse`, transport errors, version-mismatch responses) makes "no keys
exist" indistinguishable from "the API call failed" — a Dag author has no way
to tell the difference, and a transient failure looks like a deliberately empty
namespace.
Compare to `_get_variable` which propagates errors. Suggest either raising
on unexpected response types, or surfacing the error response so callers can
decide. Silently masking is the option I'd avoid.
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/variables.py:
##########
@@ -120,3 +125,25 @@ def delete_variable(
):
"""Delete an Airflow Variable."""
Variable.delete(key=variable_key, team_name=team_name)
+
+
+@keys_router.get(
+ "/keys",
+ responses={
+ status.HTTP_401_UNAUTHORIZED: {"description": "Unauthorized"},
+ },
+)
+def get_variable_keys(
+ session: SessionDep,
+ team_name: Annotated[str | None, Depends(get_team_name_dep)] = None,
+ prefix: Annotated[str | None, Query()] = None,
+) -> VariableKeysResponse:
+ """Get Airflow Variable keys, optionally filtered by prefix."""
+ stmt = select(Variable.key)
+ if prefix is not None:
+ stmt = stmt.where(Variable.key.startswith(prefix))
Review Comment:
SQLAlchemy's `String.startswith()` does **not** escape SQL `LIKE`
metacharacters by default. So `prefix="prod_"` becomes `LIKE 'prod_%'`, which
also matches `prodXdb_url`, not just keys that literally start with `prod_`.
Likewise for `%`.
Fix:
```python
stmt = stmt.where(Variable.key.startswith(prefix, autoescape=True))
```
The existing parametrized test happens to pass because both fixture keys
(`prod_db_url`, `prod_api_key`) start with literal `prod_`. Worth adding a test
case with a key like `prodXdb` to lock the behavior down.
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/variables.py:
##########
@@ -120,3 +125,25 @@ def delete_variable(
):
"""Delete an Airflow Variable."""
Variable.delete(key=variable_key, team_name=team_name)
+
+
+@keys_router.get(
+ "/keys",
+ responses={
+ status.HTTP_401_UNAUTHORIZED: {"description": "Unauthorized"},
+ },
+)
+def get_variable_keys(
+ session: SessionDep,
+ team_name: Annotated[str | None, Depends(get_team_name_dep)] = None,
+ prefix: Annotated[str | None, Query()] = None,
+) -> VariableKeysResponse:
+ """Get Airflow Variable keys, optionally filtered by prefix."""
+ stmt = select(Variable.key)
+ if prefix is not None:
+ stmt = stmt.where(Variable.key.startswith(prefix))
+ if team_name is not None:
+ stmt = stmt.where(Variable.team_name == team_name)
+
+ keys = session.scalars(stmt).all()
Review Comment:
This materializes every matching key in one response with no upper bound.
For deployments with 10k+ variables (not unusual at scale) this is a real perf
concern and a slow-query risk on the metadata DB. Other Variable endpoints
paginate.
Either:
1. Add `limit` / `offset` query params (consistent with the rest of the
public API), or
2. Enforce a hard cap (e.g. 10_000) and document it explicitly, or
3. Document the lack of a cap as a known constraint and tracking issue.
Even option 3 is better than the current state where there's no signal that
this is unbounded.
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting
##########
task-sdk/src/airflow/sdk/definitions/variable.py:
##########
@@ -67,6 +67,23 @@ def set(cls, key: str, value: Any, description: str | None =
None, serialize_jso
except AirflowRuntimeError as e:
log.exception(e)
+ @classmethod
+ def keys(cls, prefix: str | None = None) -> list[str]:
+ """
+ Return Variable keys that start with the given prefix.
+
+ The keys are fetched lazily on first access (iteration, indexing, len,
etc.)
+ and cached for subsequent access. Only keys stored in the metadata
database
+ are returned — secrets backends are not consulted.
+
+ :param prefix: Optional key prefix to filter by. If None, all keys are
returned.
+ """
+ import lazy_object_proxy
+
+ from airflow.sdk.execution_time.context import _get_variable_keys
+
+ return lazy_object_proxy.Proxy(lambda:
_get_variable_keys(prefix=prefix))
Review Comment:
The annotated return type is `list[str]` (line 71) but the runtime value is
a `lazy_object_proxy.Proxy`. Static checkers will infer `list[str]` for
callers, which is a quiet lie — code like `Variable.keys() + ["x"]` will
succeed via duck typing but the result is still a `Proxy`, not a `list`.
Two options:
- Change the annotation to `Sequence[str]` (or wrap the `Proxy` in a typed
wrapper) — keeps lazy semantics, but downstream typing is honest.
- Drop the lazy proxy and just return `_get_variable_keys(prefix=prefix)` —
laziness only matters if callers frequently construct `keys()` results they
never iterate, which seems unlikely for this API.
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/__init__.py:
##########
@@ -51,6 +51,7 @@
authenticated_router.include_router(
task_reschedules.router, prefix="/task-reschedules", tags=["Task
Reschedules"]
)
+authenticated_router.include_router(variables.keys_router,
prefix="/variables", tags=["Variables"])
Review Comment:
The order of these two `include_router` calls **matters**: `keys_router`
must come before `router`, otherwise `/variables/{variable_key}` on the
existing router would capture `/variables/keys` (returning 404 because there's
no variable named `keys`).
This works today because the line ordering is correct, but it's a foot-gun —
the next refactor that alphabetizes or reorders this list will silently break
the keys endpoint, and the existing tests probably wouldn't catch it
(`test_variable_keys_endpoint_not_available_in_previous_version` hits a 404 by
design, which is the same status the broken case would produce).
A few options:
1. Add a one-line comment explaining the order requirement.
2. Move the keys path off `/variables/{variable_key}`'s collision course —
e.g. mount it as `/variable-keys` or under a query parameter on the existing
list endpoint.
3. Add `dependencies=[]` override and merge into the same router via a
path-static route declared before the path-parameter route.
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting
##########
task-sdk/src/airflow/sdk/execution_time/request_handlers.py:
##########
@@ -70,6 +73,16 @@ def handle_get_variable(client: Client, msg: GetVariable) ->
tuple[BaseModel | N
return var, {}
+def handle_get_variable_keys(
+ client: Client, msg: GetVariableKeys
+) -> tuple[BaseModel | None, dict[str, bool]]:
+ """Fetch variable keys filtered by prefix."""
+ result = client.variables.keys(prefix=msg.prefix)
+ if not isinstance(result, VariableKeysResponse):
Review Comment:
`client.variables.keys()` is typed to return `VariableKeysResponse` and
never returns anything else — and the HTTP path has no error handling, so a
4xx/5xx would raise inside `model_validate_json` rather than reach this branch.
So this `isinstance` guard is dead code.
Either drop the branch, or — better — add real error handling in
`client.variables.keys()` so the branch becomes meaningful. The current state
matches a defensive pattern from elsewhere in the codebase but doesn't actually
defend against anything here.
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting
##########
task-sdk/src/airflow/sdk/definitions/variable.py:
##########
@@ -67,6 +67,23 @@ def set(cls, key: str, value: Any, description: str | None =
None, serialize_jso
except AirflowRuntimeError as e:
log.exception(e)
+ @classmethod
+ def keys(cls, prefix: str | None = None) -> list[str]:
+ """
+ Return Variable keys that start with the given prefix.
+
+ The keys are fetched lazily on first access (iteration, indexing, len,
etc.)
+ and cached for subsequent access. Only keys stored in the metadata
database
+ are returned — secrets backends are not consulted.
Review Comment:
This will surprise users running with a secrets backend
(`AwsSecretsManagerBackend`, etc.) — they'll reasonably expect
`Variable.keys()` to be the listing counterpart of `Variable.get()`, which
**does** consult secrets backends.
Suggest adding a `.. note::` block calling out that the asymmetry is a
deliberate design decision (secrets backends generally don't expose listing
APIs, or do so inefficiently) and linking to #61166 for context.
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/variables.py:
##########
@@ -120,3 +125,25 @@ def delete_variable(
):
"""Delete an Airflow Variable."""
Variable.delete(key=variable_key, team_name=team_name)
+
+
+@keys_router.get(
Review Comment:
Worth noting in the route docstring (or in `security_model.rst`): unlike
per-variable `GET`, this endpoint deliberately can't go through
`has_variable_access`, so any authenticated task within a team can enumerate
every variable key in that team — including keys for variables it would not be
allowed to read.
This is consistent with the documented Airflow security model (workers
within a deployment trust each other), so it's not a vulnerability per se. But
the gap between "key enumeration" and "value access" deserves an explicit
mention so it isn't later flagged as a finding by an auditor.
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting
--
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]
