Copilot commented on code in PR #65694:
URL: https://github.com/apache/airflow/pull/65694#discussion_r3126939863
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/secrets/kubernetes_secrets_backend.py:
##########
@@ -104,6 +106,7 @@ def __init__(
connections_label: str = DEFAULT_CONNECTIONS_LABEL,
variables_label: str = DEFAULT_VARIABLES_LABEL,
config_label: str = DEFAULT_CONFIG_LABEL,
+ team_label: str | None = DEFAULT_TEAM_LABEL,
connections_data_key: str = "value",
variables_data_key: str = "value",
config_data_key: str = "value",
Review Comment:
`connections_label`, `variables_label`, and `config_label` are effectively
optional (the code and tests pass `None` to skip lookups), but `__init__` still
types them as `str`. Please update these annotations to `str | None` (and keep
`team_label` as `str | None`) so type-checking matches actual supported
configuration (`*_label: null` in docs/tests).
##########
providers/cncf/kubernetes/docs/secrets-backends/kubernetes-secrets-backend.rst:
##########
@@ -207,6 +208,33 @@ You can create a variable secret with ``kubectl``:
airflow.apache.org/variable-key=my_var \
--namespace=airflow
+Multi-team lookup
+"""""""""""""""""
+
+In multi-team mode, this backend first looks for a secret whose identifier
label matches the requested
+connection or variable and whose ``team_label`` matches the current team. If
no team-scoped secret is
+found, it falls back to a global secret with the same identifier label and no
team label.
+
+For example, with ``team_label="airflow.apache.org/team"``,
``team_name="team_a"``, and
+``conn_id="my_db"``, the backend queries:
+
+* Team-scoped:
``airflow.apache.org/connection-id=my_db,airflow.apache.org/team=team_a``
+* Global fallback:
``airflow.apache.org/connection-id=my_db,!airflow.apache.org/team``
+
Review Comment:
The new lookup behavior also changes the non-team case: when `team_name` is
not provided, the backend queries for global secrets with `!team_label` (so
team-labeled secrets are not eligible). Please document this explicitly here,
since it affects how existing labeled secrets are resolved and explains why
team-scoped identifiers cannot be accessed without a team context.
```suggestion
In multi-team mode, when ``team_name`` is provided, this backend first looks
for a secret whose
identifier label matches the requested connection or variable and whose
``team_label`` matches the
current team. If no team-scoped secret is found, it falls back to a global
secret with the same
identifier label and no team label.
When ``team_name`` is not provided, the backend only queries for global
secrets by requiring that
the configured ``team_label`` is absent (``!team_label``). This means
secrets that have a team label
are not eligible in the non-team case, even if their connection or variable
identifier matches.
As a result, team-scoped identifiers cannot be accessed without a team
context.
For example, with ``team_label="airflow.apache.org/team"``,
``team_name="team_a"``, and
``conn_id="my_db"``, the backend queries:
* Team-scoped:
``airflow.apache.org/connection-id=my_db,airflow.apache.org/team=team_a``
* Global fallback:
``airflow.apache.org/connection-id=my_db,!airflow.apache.org/team``
If ``team_name`` is unset for the same ``conn_id``, the backend queries only:
* Global only:
``airflow.apache.org/connection-id=my_db,!airflow.apache.org/team``
```
##########
providers/cncf/kubernetes/tests/unit/cncf/kubernetes/secrets/test_kubernetes_secrets_backend.py:
##########
@@ -103,6 +103,33 @@ def test_get_conn_value_not_found(self, mock_client,
mock_namespace):
assert result is None
+ @mock.patch(f"{MODULE_PATH}.namespace", new_callable=mock.PropertyMock,
return_value="default")
+ @mock.patch(f"{MODULE_PATH}.client", new_callable=mock.PropertyMock)
+ def test_get_conn_value_uses_team_specific_secret_first(self, mock_client,
mock_namespace):
+ mock_client.return_value.list_namespaced_secret.side_effect = [
+ _make_secret_list([_make_secret({"value": "team-conn"})]),
+ ]
+
+ backend = KubernetesSecretsBackend()
+ result = backend.get_conn_value("my_db", team_name="team_a")
+
+ assert result == "team-conn"
+
mock_client.return_value.list_namespaced_secret.assert_called_once_with(
+ "default",
+
label_selector="airflow.apache.org/connection-id=my_db,airflow.apache.org/team=team_a",
+ resource_version="0",
+ )
+
Review Comment:
Multi-team behavior for connections includes a global fallback query when
the team-scoped secret is missing, but the tests only cover this fallback for
variables. Please add a regression test for `get_conn_value(...,
team_name=...)` where the first selector returns no items and the backend falls
back to the global `!team_label` selector.
##########
providers/cncf/kubernetes/tests/unit/cncf/kubernetes/secrets/test_kubernetes_secrets_backend.py:
##########
@@ -134,6 +161,33 @@ def test_get_variable_not_found(self, mock_client,
mock_namespace):
assert result is None
+ @mock.patch(f"{MODULE_PATH}.namespace", new_callable=mock.PropertyMock,
return_value="default")
+ @mock.patch(f"{MODULE_PATH}.client", new_callable=mock.PropertyMock)
+ def
test_get_variable_falls_back_to_global_secret_when_team_secret_is_missing(
+ self, mock_client, mock_namespace
+ ):
+ mock_client.return_value.list_namespaced_secret.side_effect = [
+ _make_secret_list([]),
+ _make_secret_list([_make_secret({"value": "global-value"})]),
Review Comment:
`get_variable()` now also implements the team-specific-accessed-as-global
guard, but the unit tests only cover this guard for connections. Please add a
test asserting `get_variable("_teama___api_key") is None` (and that the
Kubernetes client is not called) to prevent regressions.
--
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]
