You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "henry3260 (via GitHub)" <gi...@apache.org> on 2026/07/12 16:51:36 UTC
[PR] Reject connection secrets without a connection type [airflow]
henry3260 opened a new pull request, #69780:
URL: https://github.com/apache/airflow/pull/69780
## Why
Connection secrets in JSON format could omit `conn_type`. Although the secrets backend deserialized those values, the Execution API requires a connection type and the task could ultimately receive a misleading “connection not found” error.
This addresses the `conn_type` portion of related: #57864.
## What
- Reject JSON connection secrets whose `conn_type` is missing, null, empty, or whitespace-only.
- Raise a clear `ValueError` before constructing an incomplete Connection.
- Add regression coverage for each invalid `conn_type` value.
---
##### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this PR, please
change below checkbox to `[X]` followed by the name of the tool, uncomment the "Generated-by".
-->
- [ ] Yes (please specify the tool below)
<!--
Generated-by: [Tool Name] following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
-->
---
* Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
* For fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
* When adding dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
* For significant user-facing changes create newsfragment: `{pr_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments). You can add this file in a follow-up commit after the PR is created so you know the PR number.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject connection secrets without a connection type [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject connection secrets without a connection type [airflow]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on PR #69780:
URL: https://github.com/apache/airflow/pull/69780#issuecomment-4966077781
Went through `_deserialize_connection_value` and the new guard reads correct to me. Good call accepting `uri` as an alternative rather than requiring `conn_type` outright, since `from_json` passes a bare `uri` to the constructor which routes through `from_uri` and parses the scheme into the conn_type, so those blobs still build a usable connection and the `{"conn_type": null, "uri": ...}` test covers exactly that. Raising a `ValueError` up front is a clear improvement over the misleading connection not found error the PR describes. Only nit, you `json.loads(value)` in the guard and then `from_json` parses the same string again, negligible for a small secret blob so I wouldn't bother.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Document that connection secrets without conn_type remain supported [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #69780:
URL: https://github.com/apache/airflow/pull/69780#discussion_r3636026541
##########
shared/secrets_backend/src/airflow_shared/secrets_backend/base.py:
##########
@@ -122,6 +122,12 @@ def _get_connection_class(self) -> type:
def _deserialize_connection_value(conn_class: type, conn_id: str, value: str):
value = value.strip()
if value[0] == "{":
+ # JSON secrets stored by Airflow 2-era backends may lack both "conn_type"
Review Comment:
```suggestion
# JSON secrets stored by Airflow 2 backends may lack both "conn_type"
```
##########
shared/secrets_backend/src/airflow_shared/secrets_backend/base.py:
##########
@@ -122,6 +122,12 @@ def _get_connection_class(self) -> type:
def _deserialize_connection_value(conn_class: type, conn_id: str, value: str):
value = value.strip()
if value[0] == "{":
+ # JSON secrets stored by Airflow 2-era backends may lack both "conn_type"
+ # and "uri" (e.g. {"host": ..., "login": ..., "password": ...}). This is
+ # valid: conn_type is intentionally optional on the SDK Connection model
+ # for Airflow 2 -> 3 migration compatibility. Do not add validation here
+ # that rejects such secrets; see
+ # https://github.com/apache/airflow/pull/61728
Review Comment:
```suggestion
# for Airflow 2 -> 3 migration compatibility.
# Check: https://github.com/apache/airflow/pull/61728
```
##########
shared/secrets_backend/tests/secrets_backend/test_base.py:
##########
@@ -124,6 +124,22 @@ def test_deserialize_connection_json(self, sample_conn_json):
assert conn.conn_id == "test_conn"
assert conn._kwargs["conn_type"] == "mysql"
+ def test_deserialize_connection_json_without_conn_type(self):
+ """Airflow 2-era JSON secrets without conn_type or uri must keep deserializing.
+
+ Guards the Airflow 2 -> 3 migration compatibility established in
+ https://github.com/apache/airflow/pull/61728.
+ """
Review Comment:
```suggestion
"""
Guards the Airflow 2 -> 3 migration compatibility established in
https://github.com/apache/airflow/pull/61728.
"""
```
##########
shared/secrets_backend/src/airflow_shared/secrets_backend/base.py:
##########
@@ -122,6 +122,12 @@ def _get_connection_class(self) -> type:
def _deserialize_connection_value(conn_class: type, conn_id: str, value: str):
value = value.strip()
if value[0] == "{":
+ # JSON secrets stored by Airflow 2-era backends may lack both "conn_type"
Review Comment:
Maybe also mention a TODO to remove it after min supported airflow version in providers is 3.0
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Document that connection secrets without conn_type remain supported [airflow]
Posted by "henry3260 (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Document that connection secrets without conn_type remain supported [airflow]
Posted by "henry3260 (via GitHub)" <gi...@apache.org>.
henry3260 commented on code in PR #69780:
URL: https://github.com/apache/airflow/pull/69780#discussion_r3636354141
##########
shared/secrets_backend/src/airflow_shared/secrets_backend/base.py:
##########
@@ -122,6 +122,12 @@ def _get_connection_class(self) -> type:
def _deserialize_connection_value(conn_class: type, conn_id: str, value: str):
value = value.strip()
if value[0] == "{":
+ # JSON secrets stored by Airflow 2-era backends may lack both "conn_type"
Review Comment:
> Maybe also mention a TODO to remove it after min supported airflow version in providers is 3.0
Thanks, applied all of comments!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject connection secrets without a connection type [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #69780:
URL: https://github.com/apache/airflow/pull/69780#discussion_r3628798416
##########
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:
Thanks for the clarification. I forgot we already had that PR. 🤦♂️
We could also add a comment or some documentation around the relevant function so others don't end up trying the same thing again.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Document that connection secrets without conn_type remain supported [airflow]
Posted by "henry3260 (via GitHub)" <gi...@apache.org>.
henry3260 merged PR #69780:
URL: https://github.com/apache/airflow/pull/69780
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org