kaxil commented on code in PR #71078:
URL: https://github.com/apache/airflow/pull/71078#discussion_r3713408479
##########
providers/google/src/airflow/providers/google/cloud/secrets/secret_manager.py:
##########
@@ -248,7 +248,12 @@ def _names_a_team_namespace(self, secret_id: str) -> bool:
backend does, is wrong here: the inherited implementation prepends a
separator to an
empty prefix (``'' -> '-smtp_default'``) and normalizes nothing, so
the guard would
both mis-anchor and miss ids whose separator only appears after
normalization.
+
+ Only checked in multi-team mode: ``team_name`` is never non-``None``
otherwise, so no
+ team scoped secret can exist to collide with.
"""
+ if not conf.getboolean("core", "multi_team", fallback=False):
Review Comment:
The Google backend docs still say the opposite.
`providers/google/docs/secrets-backends/google-cloud-secret-manager-backend.rst:236`
warns that the refusal "applies whether or not you use teams" and that an id
already containing `--` "stops resolving after upgrading and you must rename
it". That rename advice is wrong after this change, and the new failure mode
(an id with `--` resolves fine until someone enables `multi_team`, then
silently stops) isn't documented anywhere. Can this PR update that section?
##########
providers/microsoft/azure/src/airflow/providers/microsoft/azure/secrets/key_vault.py:
##########
@@ -247,7 +248,12 @@ def _names_a_team_namespace(self, secret_id: str) -> bool:
The id is normalised first because :meth:`build_path` maps ``_`` onto
the separator
everywhere in this backend, so ``b__c`` reaches Key Vault as ``b--c``
and would
otherwise manufacture the team separator from an id that does not
visibly contain it.
+
+ Only checked in multi-team mode: ``team_name`` is never non-``None``
otherwise, so no
+ team scoped secret can exist to collide with.
"""
+ if not conf.getboolean("core", "multi_team", fallback=False):
Review Comment:
`get_config` runs through this guard too (line 193), but config lookups are
never team-scoped: `_get_secret(self.config_prefix, key)` passes no
`team_name`, so a config key has no team-scoped name to collide with in either
mode. The key here is the operator-chosen path from `<option>_secret`, and this
backend maps `_` onto the separator, so a path named after the env var
(`airflow__database__sql_alchemy_conn` becomes
`airflow--database--sql-alchemy-conn`) is still refused whenever `multi_team`
is on. Google and Yandex never guarded `get_config`, and the Google docs state
config lookups are unaffected, so dropping it from `get_config` here and in the
two AWS backends would make all five agree. Happy for that to be a follow-up if
you want to keep this PR narrow.
##########
providers/microsoft/azure/tests/unit/microsoft/azure/secrets/test_key_vault.py:
##########
@@ -215,6 +224,17 @@ def test_refusing_an_ambiguous_id_is_logged(self,
mock_client, caplog):
assert sum(refused_id in r.getMessage() for r in refusals) == 1
mock_client.get_secret.assert_not_called()
+ @mock.patch(f"{KEY_VAULT_MODULE}.AzureKeyVaultBackend.client")
+ def test_ambiguous_id_resolves_when_multi_team_is_disabled(self,
mock_client):
+ """No team scoped secret can exist without multi-team mode, so there
is no ambiguity
+ to refuse -- an ordinary id containing the separator must resolve
normally."""
+ mock_client.get_secret.return_value = mock.Mock(value="world")
+ backend = AzureKeyVaultBackend()
+
+ assert backend.get_conn_value("prod--my_db") == "world"
Review Comment:
`get_secret` returns `world` for any name here, so these pass regardless of
the name the backend computes, which is the part the `prod__hello` case in the
description turns on. The tests above assert the resolved name (lines 45, 88,
111). Worth at least
`mock_client.get_secret.assert_any_call(name="airflow-variables-prod--hello")`
for the `prod__hello` lookup.
##########
providers/amazon/tests/unit/amazon/aws/secrets/test_systems_manager.py:
##########
@@ -116,6 +116,7 @@ def test_get_conn_value_with_team_name(self):
returned_uri = ssm_backend.get_conn_value(conn_id="test_postgres",
team_name="my_team")
assert returned_uri == "postgresql://airflow:airflow@host:5432/airflow"
+ @conf_vars({("core", "multi_team"): "True"})
Review Comment:
The other four test files in this PR define `multi_team_enabled =
conf_vars({("core", "multi_team"): "True"})` at module level. Worth doing the
same here rather than repeating the literal on six tests.
--
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]
