Copilot commented on code in PR #65493:
URL: https://github.com/apache/airflow/pull/65493#discussion_r3115567526
##########
providers/hashicorp/src/airflow/providers/hashicorp/secrets/vault.py:
##########
@@ -189,6 +200,30 @@ def _get_secret_with_base(self, base_path: str | None,
key: str) -> dict | None:
secret_path=(mount_point + "/" if mount_point else "") +
secret_path
)
+ def _get_team_or_global_secret(self, base_path: str | None, team_name: str
| None, key: str):
+ """
+ Get a secret from a team specific path or the global path.
+
+ If multi team is enabled, check {base_path}/{team_name}/{key}, then
fallback to {base_path}/{global_path}/{key} or {base_path}/{key}.
+ """
+ if base_path is None:
+ return None
+ if (
+ conf.get("core", "multi_team", fallback=False)
+ and self.use_team_secrets_path
+ and team_name is not None
+ ):
+ response = self._get_secret_with_base(base_path + "/" + team_name,
key)
+ if response is not None:
+ return response
+ # Fallback to global secret
+ if conf.get("core", "multi_team", fallback=False) and
self.global_secrets_path is not None:
+ path = base_path + "/" + self.global_secrets_path
Review Comment:
When `connections_path`/`variables_path` is configured as an empty string
(which the backend already treats specially in `_get_secret_with_base`),
building `base_path + "/" + team_name` produces a leading slash (e.g. "/team")
and changes the secret path format. Consider using `build_path(base_path,
team_name)` (or handling `base_path == ""`) when constructing the team/global
base paths to preserve the existing no-leading-slash behavior.
```suggestion
response = self._get_secret_with_base(self.build_path(base_path,
team_name), key)
if response is not None:
return response
# Fallback to global secret
if conf.get("core", "multi_team", fallback=False) and
self.global_secrets_path is not None:
path = self.build_path(base_path, self.global_secrets_path)
```
##########
providers/hashicorp/src/airflow/providers/hashicorp/secrets/vault.py:
##########
@@ -189,6 +200,30 @@ def _get_secret_with_base(self, base_path: str | None,
key: str) -> dict | None:
secret_path=(mount_point + "/" if mount_point else "") +
secret_path
)
+ def _get_team_or_global_secret(self, base_path: str | None, team_name: str
| None, key: str):
+ """
+ Get a secret from a team specific path or the global path.
+
+ If multi team is enabled, check {base_path}/{team_name}/{key}, then
fallback to {base_path}/{global_path}/{key} or {base_path}/{key}.
+ """
+ if base_path is None:
+ return None
+ if (
+ conf.get("core", "multi_team", fallback=False)
+ and self.use_team_secrets_path
+ and team_name is not None
+ ):
+ response = self._get_secret_with_base(base_path + "/" + team_name,
key)
+ if response is not None:
+ return response
+ # Fallback to global secret
+ if conf.get("core", "multi_team", fallback=False) and
self.global_secrets_path is not None:
Review Comment:
`multi_team` is a boolean config, but this code uses `conf.get(...)` in
boolean contexts. If the config value is the string "False" (common when set
via env/ini), it will still be truthy and incorrectly enable team/global
lookup. Use `conf.getboolean("core", "multi_team", fallback=False)` for these
checks (and avoid mixing different fallbacks in different places).
```suggestion
multi_team_enabled = conf.getboolean("core", "multi_team",
fallback=False)
if multi_team_enabled and self.use_team_secrets_path and team_name
is not None:
response = self._get_secret_with_base(base_path + "/" +
team_name, key)
if response is not None:
return response
# Fallback to global secret
if multi_team_enabled and self.global_secrets_path is not None:
```
##########
providers/hashicorp/src/airflow/providers/hashicorp/secrets/vault.py:
##########
@@ -214,7 +249,10 @@ def get_connection(self, conn_id: str, team_name: str |
None = None) -> Connecti
# problems when instantiating the backend during configuration
from airflow.models.connection import Connection
- response = self.get_response(conn_id)
+ if conf.get("core", "multi_team", fallback=True):
+ response = self._get_team_or_global_secret(self.connections_path,
team_name, conn_id)
+ else:
+ response = self.get_response(conn_id)
Review Comment:
`get_connection()` currently uses `conf.get("core", "multi_team",
fallback=True)` which (a) defaults to enabling the multi-team path even when
unset, and (b) has the same truthy-string problem as above. This should be
`conf.getboolean("core", "multi_team", fallback=False)` to match core/provider
usage and ensure team scoping is only active when explicitly enabled.
##########
providers/hashicorp/src/airflow/providers/hashicorp/secrets/vault.py:
##########
@@ -52,6 +53,10 @@ class VaultBackend(BaseSecretsBackend, LoggingMixin):
(default: 'variables'). If set to None (null), requests for variables
will not be sent to Vault.
:param config_path: Specifies the path of the secret to read Airflow
Configurations
(default: 'config'). If set to None (null), requests for
configurations will not be sent to Vault.
+ :param use_team_secrets_paths: Flag to enable team scoped secret retrieval
from {base_path}/{team_name}/{key}
Review Comment:
Docstring parameter name looks inconsistent: it documents
`use_team_secrets_paths` but the actual init kwarg is `use_team_secrets_path`.
Align the docstring with the real argument name so users configuring
`backend_kwargs` don’t get misled.
```suggestion
:param use_team_secrets_path: Flag to enable team scoped secret
retrieval from {base_path}/{team_name}/{key}
```
##########
providers/hashicorp/tests/unit/hashicorp/secrets/test_vault.py:
##########
@@ -68,6 +70,175 @@ def test_get_connection(self, mock_hvac):
connection = test_client.get_connection(conn_id="test_postgres")
assert connection.get_uri() ==
"postgresql://airflow:airflow@host:5432/airflow?foo=bar&baz=taz"
+
@mock.patch("airflow.providers.hashicorp._internal_client.vault_client.hvac")
+ def test_get_connection_team_value(self, mock_hvac):
+ mock_client = mock.MagicMock()
+ mock_hvac.Client.return_value = mock_client
+ mock_client.secrets.kv.v2.read_secret_version.return_value = {
+ "request_id": "94011e25-f8dc-ec29-221b-1f9c1d9ad2ae",
+ "lease_id": "",
+ "renewable": False,
+ "lease_duration": 0,
+ "data": {
+ "data": {
+ "conn_type": "postgresql",
+ "login": "airflow",
+ "password": "airflow",
+ "host": "host",
+ "port": "5432",
+ "schema": "airflow",
+ "extra": '{"foo":"bar","baz":"taz"}',
+ },
+ "metadata": {
+ "created_time": "2020-03-16T21:01:43.331126Z",
+ "deletion_time": "",
+ "destroyed": False,
+ "version": 1,
+ },
+ },
+ "wrap_info": None,
+ "warnings": None,
+ "auth": None,
+ }
+
+ kwargs = {
+ "connections_path": "connections",
+ "mount_point": "airflow",
+ "auth_type": "token",
+ "url": "http://127.0.0.1:8200";,
+ "token": "s.7AU0I51yv1Q1lxOIg1F3ZRAS",
+ }
+
+ with conf_vars({("core", "multi_team"): "True"}):
+ test_client = VaultBackend(**kwargs)
+ connection = test_client.get_connection(conn_id="test_postgres",
team_name="foo")
+ mock_client.secrets.kv.v2.read_secret_version.assert_called_with(
Review Comment:
The new multi-team tests cover the enabled (`multi_team=True`) cases, but
there’s no regression test ensuring that when `core.multi_team` is disabled and
a `team_name` is passed, VaultBackend does *not* add the team prefix. Adding
that case would prevent future regressions (and would have caught the
`conf.get(...)` truthy-string issue).
--
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]
