shahar1 commented on code in PR #70884:
URL: https://github.com/apache/airflow/pull/70884#discussion_r3694818006
##########
airflow-core/src/airflow/secrets/environment_variables.py:
##########
@@ -65,25 +74,36 @@ def get_variable(self, key: str, team_name: str | None =
None) -> str | None:
@staticmethod
def _names_a_team_namespace(secret_id: str) -> bool:
"""
- Whether ``secret_id`` spells out a team namespaced environment
variable name.
+ Whether ``secret_id`` could spell out a team namespaced environment
variable name.
A team specific secret lives in the ``_<TEAM_NAME>___<SECRET_ID>``
namespace of the
- environment. An id of that shape therefore makes the team agnostic
lookup -- which
- prepends only ``AIRFLOW_CONN_`` / ``AIRFLOW_VAR_`` -- land inside some
team's namespace,
- so that lookup is refused for such an id.
+ environment. An id of that shape makes the team agnostic lookup --
which prepends only
+ ``AIRFLOW_CONN_`` / ``AIRFLOW_VAR_`` -- land inside some team's
namespace, so that
+ lookup is refused for such an id.
+
+ The test is whether the leading segment **could be a team name**, not
merely whether
+ the id contains the separator. Team names are validated on creation,
so an id like
+ ``_a___b`` cannot name a team namespace -- ``a`` is too short to be a
team -- and
+ refusing it would block a legitimate team agnostic secret for no
benefit. Every id
+ that does spell a real team's namespace has a valid team name in that
position by
+ construction, so nothing reachable is let through.
**The id is never attributed to a particular team**, because it cannot
be: a team name
- may itself contain the ``___`` separator, so ``_a___b___c`` is both
team ``a`` with id
- ``b___c`` and team ``a___b`` with id ``c``, and nothing in the string
distinguishes them.
- Only the caller's own namespace is ever constructed, never parsed.
-
- This is why the check is not "does the id belong to some team other
than the caller's".
- Comparing the id against the prefix the caller's team builds looks
equivalent and is not:
- for a caller in team ``a`` the id ``_a___b___c`` starts with
``_A___``, yet the variable
- it resolves, ``AIRFLOW_CONN__A___B___C``, is team ``a___b``'s.
Treating a prefix match as
- ownership hands one team the secrets of every team whose name extends
it. Callers reach
- their own team's secrets with the bare id plus their team scope --
handled above, and safe
- because that path can only ever build the caller's own namespace -- so
nothing legitimate
- needs a namespaced id here.
+ may itself contain the separator, so ``_a___b___c`` is both team ``a``
with id
+ ``b___c`` and team ``a___b`` with id ``c``. Every split is therefore
considered, and
+ one plausible team name is enough to refuse. Comparing the id against
the prefix the
+ caller's own team builds looks equivalent and is not: for a caller in
team ``a`` the
+ id ``_a___b___c`` starts with ``_A___``, yet the variable it resolves,
+ ``AIRFLOW_CONN__A___B___C``, is team ``a___b``'s. Treating a prefix
match as ownership
+ hands one team the secrets of every team whose name extends it.
Review Comment:
Deleting the old "that path can only ever build the caller's own namespace"
sentence is the right call — it was false. But the gap it papered over is still
open, and it's the same cross-team read #70736 closed, just spelled differently.
A caller in `team_a` supplying the **bare** id `prod___dbconn` never reaches
this guard: the scoped branch above returns first, and the id has no leading
`_` anyway.
```python
# caller: get_conn_value("prod___dbconn", team_name="team_a")
f"{CONN_ENV_PREFIX}_{team_name.upper()}___" + conn_id.upper()
# -> AIRFLOW_CONN__TEAM_A___PROD___DBCONN
# team "team_a___prod" stores "dbconn" as
# "AIRFLOW_CONN_" + "_TEAM_A___PROD" + "___" + "DBCONN"
# -> AIRFLOW_CONN__TEAM_A___PROD___DBCONN <- identical
```
I checked; the strings are byte-identical.
`test_team_whose_name_extends_the_callers_is_not_readable` uses this exact pair
of team names but only the namespaced spelling.
Pre-existing rather than introduced here, so not a blocker on this diff —
but worth a follow-up, and the root cause is shared with the loop below: `___`
isn't reserved, so `_<TEAM>___<ID>` doesn't parse uniquely. Forbidding `___`
inside a team name would make it unambiguous, collapse this loop into one exact
split, and close the scoped-path collision too.
##########
airflow-core/src/airflow/cli/commands/team_command.py:
##########
@@ -169,6 +172,18 @@ def team_sync(args, *, session=NEW_SESSION):
if bundle.team_name is not None
}
+ # The bundle config is a second creation path for teams, so it has to
enforce the same
+ # name rule as `teams create`. Other code relies on a stored team name
being a valid one
+ # -- the environment secrets backend decides whether a supplied secret id
could name a
+ # team namespace by testing the leading segment against this pattern, and
an unvalidated
+ # short name such as "a" would make that test miss.
+ invalid = sorted(name for name in dag_bundle_teams if not
re.match(TEAM_NAME_PATTERN, name))
+ if invalid:
+ raise SystemExit(
+ f"Invalid team name(s) in the dag bundle config: {',
'.join(invalid)}. "
+ f"Team names must match regex {TEAM_NAME_PATTERN}."
+ )
Review Comment:
No test covers this new behaviour — the diff touches only
`test_secrets_environment_variables.py`, and `test_team_sync` is unchanged and
happy-path only.
> **Flag any changed or added behaviour without a corresponding test, and
flag tests that a reviewer cannot fail by reverting the PR's change.** The
target is exactly 100% coverage of what the PR changes — no more, no less.
>
> — [`.github/instructions/code-review.instructions.md` § Testing
Requirements](https://github.com/apache/airflow/blob/main/.github/instructions/code-review.instructions.md)
The PR body's "21 cases pass with the shared pattern" confirms nothing
regressed, which isn't the same thing: delete this `if invalid:` block and the
suite stays green. Since this guard is what makes the secrets relaxation sound,
it's the behaviour here that most needs pinning — one case with `"team_name":
"a"` in the bundle config asserting `SystemExit` and that no team row is
created.
##########
airflow-core/src/airflow/secrets/environment_variables.py:
##########
@@ -27,6 +27,15 @@
CONN_ENV_PREFIX = "AIRFLOW_CONN_"
VAR_ENV_PREFIX = "AIRFLOW_VAR_"
+# Separator between the team name and the secret id in a team namespaced
+# environment variable name: AIRFLOW_CONN__<TEAM>___<ID>.
+TEAM_SEP = "___"
+
+# What ``airflow teams create`` accepts as a team name, so what a team
+# namespace can actually be spelled with.
+# Kept in sync with ``airflow.cli.commands.team_command``.
+_TEAM_NAME = re.compile(r"[a-zA-Z0-9_-]{3,50}")
Review Comment:
These two spellings aren't equivalent, which undercuts the "kept in sync"
promise: `$` also matches *before a trailing newline*, so
```python
re.match(r"^[a-zA-Z0-9_-]{3,50}$", "team1\n") # match
_TEAM_NAME.fullmatch("team1\n") # None
```
(verified). `teams create` is saved by its `.strip()`; the new `teams sync`
path has none, so a Dag bundle config with a trailing newline in `team_name`
creates a team this guard cannot recognise. One *unanchored* pattern plus
`re.fullmatch` at both sites removes the divergence entirely.
Also: a `# Kept in sync with ...` comment is the kind of promise better held
by a one-line test asserting the two constants are equal — otherwise drift
silently reopens the hole instead of failing CI.
Minor: the comment above now understates the coupling — after this PR the
pattern is what `teams create` **and** `teams sync` accept.
##########
airflow-core/src/airflow/cli/commands/team_command.py:
##########
@@ -169,6 +172,18 @@ def team_sync(args, *, session=NEW_SESSION):
if bundle.team_name is not None
}
+ # The bundle config is a second creation path for teams, so it has to
enforce the same
+ # name rule as `teams create`. Other code relies on a stored team name
being a valid one
+ # -- the environment secrets backend decides whether a supplied secret id
could name a
+ # team namespace by testing the leading segment against this pattern, and
an unvalidated
+ # short name such as "a" would make that test miss.
+ invalid = sorted(name for name in dag_bundle_teams if not
re.match(TEAM_NAME_PATTERN, name))
Review Comment:
This validates the names in the Dag bundle config, but the secrets guard
depends on every **stored** team name being valid — and `teams sync` shipped in
3.3.0 with no validation at all. A live 3.3.0 deployment can already hold a
team named `ab`, and after the companion change `_ab___x` resolves through the
team agnostic lookup into `AIRFLOW_CONN__AB___X`, that team's secret. That's
the read #70736 closed, and `_ab___x` is one of the two ids the new test
asserts must resolve.
The config-side check catches this only while the short name is still in the
config; a team row created by an earlier sync and since dropped from the config
is missed. The stored names are in hand one line later
(`Team.get_all_team_names(session=session)`), so covering them too is nearly
free — hard error, or a warning if failing on pre-existing data is too
aggressive for a patch release.
Related, given the `backport-to-v3-3-test` label: on a 3.3.x patch upgrade,
a deployment whose bundle config carries a short team name will see `airflow
teams sync` start exiting non-zero where it previously succeeded. Defensible,
but better as a deliberate call than a side effect.
Separately — `re.match` with a `$`-anchored pattern accepts a trailing
newline, and unlike `teams create` this path has no `.strip()`. See the note on
`_TEAM_NAME` in `environment_variables.py`.
--
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]
