kaxil commented on code in PR #68305:
URL: https://github.com/apache/airflow/pull/68305#discussion_r3484502388
##########
providers/hashicorp/tests/unit/hashicorp/secrets/test_vault.py:
##########
@@ -759,3 +784,78 @@ def test_config_path_none_value(self, mock_hvac):
test_client = VaultBackend(**kwargs)
assert test_client.get_config("test") is None
mock_hvac.Client.assert_not_called()
+
+
@mock.patch("airflow.providers.hashicorp._internal_client.vault_client.hvac")
+ @pytest.mark.skipif(
+ not hasattr(VaultBackend, "_set_connection_class"),
Review Comment:
These gate on `hasattr(VaultBackend, "_set_connection_class")` and then call
`_set_connection_class(SdkConnection)` by hand. That method only exists on
3.2+, so the tests only run against 3.2+ and never exercise 2.11/3.0/3.1, the
versions where behavior actually differs. If the override is dropped per the
other comment, the unit test reduces to asserting `get_conn_value` returns the
URI verbatim or `json.dumps(dict)`, leaving deserialization to the base.
##########
providers/hashicorp/src/airflow/providers/hashicorp/secrets/vault.py:
##########
@@ -224,32 +224,68 @@ def _get_team_or_global_secret(self, base_path: str |
None, team_name: str | Non
return self._get_secret_with_base(path, key)
- # Make sure connection is imported this way for type checking, otherwise
when importing
- # the backend it will get a circular dependency and fail
- if TYPE_CHECKING:
- from airflow.models.connection import Connection
+ def get_connection(self, conn_id: str, team_name: str | None = None):
+ """
+ Return connection object with a given ``conn_id``.
+
+ Overrides the base to accept ``team_name`` on all supported Airflow
versions and
+ to fall back to the compat SDK Connection when the framework-level
class injection
+ (``deserialize_connection`` / ``_get_connection_class``) is not yet
available.
- def get_connection(self, conn_id: str, team_name: str | None = None) ->
Connection | None:
+ :param conn_id: connection id
+ :param team_name: Team name associated to the task trying to access
the connection (if any)
+ :return: Connection object or None
"""
- Get connection from Vault as secret.
+ value = self.get_conn_value(conn_id=conn_id, team_name=team_name)
+ if value is None:
+ return None
+ # Prefer the base-class deserializer when available — it uses
_get_connection_class()
+ # which the framework populates with the right Connection class per
execution context.
+ if hasattr(self, "deserialize_connection"):
Review Comment:
The fallback below this line never runs: `deserialize_connection` is defined
on the base on every supported version (checked the 2.11.0, 3.0.0, 3.1.0 tags
and main), so `hasattr(self, "deserialize_connection")` is always true and the
`from_json` / `from_uri` / `Connection(uri=)` ladder is dead code. It can be
removed.
The bigger problem is that delegating to `self.deserialize_connection` only
fixes the worker on 3.2+. On 3.0/3.1 the base `deserialize_connection`
hard-imports `airflow.models.connection.Connection`
(`airflow-core/src/airflow/secrets/base_secrets.py:61` at those tags), the ORM
model whose mapper init is the bug. The per-process class injection
(`_get_connection_class` / `_set_connection_class`) that lets the base avoid
the ORM only landed in 3.2+, so as written this doesn't fix the 3.0/3.1 workers
the title targets, only 3.2+.
The clean shape is what Amazon (`secrets_manager.py:200`) and Google
(`secret_manager.py:159`) do: override only `get_conn_value` (you already have
it) and drop the `get_connection` override. The base `get_connection` forwards
`team_name` to `get_conn_value` via `call_secrets_backend_method`, so the
override's "accept team_name on all versions" reason isn't needed. That deletes
the dead ladder and the re-implementation of `_deserialize_connection_value`,
and fixes 3.2+ through the framework's injection. For 3.0/3.1 the base imports
the ORM regardless of what the provider does, so a provider-only change can't
avoid it there; worth either scoping the claim to 3.2+ or calling 3.0/3.1 out
as a separate core limitation. (akeyless has the same hard-coded-ORM override
and wants the same treatment.)
--
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]
