amoghrajesh commented on code in PR #69780:
URL: https://github.com/apache/airflow/pull/69780#discussion_r3628674604
##########
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: https://github.com/apache/airflow/pull/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 #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.
--
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]
