Only refuse team scoped like secret ids when multi_team is on - #71078
Conversation
boring-cyborg
Bot
added
area:providers
area:secrets
provider:amazon
|
@potiuk this touches providers only, so no need to backport. |
|
Here's the companion PR for env backend: #71079 |
| 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): |
There was a problem hiding this 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?
| 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): |
There was a problem hiding this 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.
| mock_client.get_secret.return_value = mock.Mock(value="world") | ||
| backend = AzureKeyVaultBackend() | ||
|
|
||
| assert backend.get_conn_value("prod--my_db") == "world" |
There was a problem hiding this 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.
| 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"}) |
There was a problem hiding this 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.
4 participants
Was generative AI tooling used to co-author this PR?
Related to (#70878), (#70876), (#70899), (#70869), (#70877), (#70736)
Summary
connection id containing a double underscore (e.g. aws__prod) was silently refused as if it were a missing secret: https://github.com/apache/airflow/blob/main/providers/microsoft/azure/src/airflow/providers/microsoft/azure/secrets/key_vault.py#L230-L251
Trying to gate this by only run the check when
[core] multi_teamis enabled, same as Airflow already does at the model layer.{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.