henry3260 commented on code in PR #69780:
URL: https://github.com/apache/airflow/pull/69780#discussion_r3629340288
##########
shared/secrets_backend/src/airflow_shared/secrets_backend/base.py:
##########
@@ -122,6 +123,11 @@ def _get_connection_class(self) -> type:
def _deserialize_connection_value(conn_class: type, conn_id: str, value:
str):
value = value.strip()
if value[0] == "{":
+ connection_data = json.loads(value)
+ conn_type = connection_data.get("conn_type")
+ uri = connection_data.get("uri")
+ if not (isinstance(conn_type, str) and conn_type.strip() or
isinstance(uri, str) and uri.strip()):
+ raise ValueError("Connection secret JSON must include a
non-empty 'conn_type' or 'uri'.")
Review Comment:
> This is incorrect, check: #61728. Connections without a `conn_type` are
important for airflow 2-> 3 compatibility.
>
> #69780 raises error for the same secret shape #61728's own regression test
proves should succeed (`{"host":..., "login":..., "password":...}`, no
`conn_type`, no `uri`), so merging it would regress that fix and break
migration compatibility for users with secrets stored without `conn_type`.
>
> And relatedly, for a connection served from a secrets backend (AWS Secrets
Manager, Vault, custom backend, etc.), resolution normally happens on the
worker itself, not via the Execution API:
>
> * worker (task-sdk configuration.py) instantiates the configured secrets
backend directly so any deployment with a secrets backend configured gets it
resolved locally on the worker.
> * `_deserialize_connection_value()`runs there, and [Make conn_type
optional in task SDK Connection datamodel
#61728](https://github.com/apache/airflow/pull/61728) made `conn_type` optional
on the task-SDK Connection model specifically so this resolves cleanly with no
`conn_type`
> * `ExecutionAPISecretsBackend` only comes into play as a last resort
fallback if the worker's own backend chain doesn't resolve the `conn_id`. When
that happens, the server resolves via its own chain: env vars, then the
metastore DB. The DB path is safe by construction: `conn_type` is a `NOT NULL`
column, so a connection reaching ConnectionResponse via that fallback always
has a conn_type.
>
> So the "misleading connection not found error" this PR is trying to
prevent doesn't actually occur on the path it changes, that path already works
correctly per #61728, and tightening it back to reject missing conn_type only
breaks that compatibility fix without fixing anything real.
Sorry for the misunderstanding here. I’ve updated the PR to just add an
explanatory comment in the code so others don’t attempt the same change in the
future.
--
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]
