potiuk commented on code in PR #73419:
URL: https://github.com/apache/airflow/pull/73419#discussion_r4066602420
##########
providers/sftp/src/airflow/providers/sftp/hooks/sftp.py:
##########
@@ -477,7 +477,10 @@ def retrieve_file_chunk(
remote_file_chunks = [remote_file_paths[i::workers] for i in
range(workers)]
local_file_chunks = [new_local_file_paths[i::workers] for i in
range(workers)]
self.log.info("Opening %s new SFTP connections", workers)
- conns = [SFTPHook(ssh_conn_id=self.ssh_conn_id).get_conn() for _ in
range(workers)]
+ conns = [
+ SFTPHook(ssh_conn_id=self.ssh_conn_id,
no_host_key_check=self.no_host_key_check).get_conn()
Review Comment:
Good catch. `SFTPHook` forwards `**kwargs` to `SSHHook.__init__`, so with
`apache-airflow-providers-ssh` 6.0.x installed the two concurrent-transfer
paths raise `TypeError` on the new keyword, and the sync hooks keep the old
permissive default on top of that. Marking the SSH dependency with `# use next
version` so release preparation raises the minimum.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
providers/ssh/src/airflow/providers/ssh/hooks/ssh.py:
##########
@@ -619,7 +645,7 @@ def _parse_extras(self, conn: Any) -> None:
host_key = extra_options.get("host_key")
nhkc_raw = extra_options.get("no_host_key_check")
- no_host_key_check = str(nhkc_raw).lower() == "true" if nhkc_raw is not
None else True
+ no_host_key_check = str(nhkc_raw).lower() == "true" if nhkc_raw is not
None else False
Review Comment:
Right. The alias is resolved in `SSHHook.__init__` only, while
`SSHHookAsync._parse_extras` and `SFTPHookAsync._parse_extras` both took the
default flip without it — and the alias is precisely the migration path for the
connections that flip affects. `SSHRemoteJobOperator` makes it concrete: sync
submission honours it, `SSHRemoteJobTrigger` on `SSHHookAsync` does not. Adding
the resolution to both async parsers with the canonical key taking precedence
and the same deprecation warning, plus tests on each path.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
providers/ssh/src/airflow/providers/ssh/hooks/ssh.py:
##########
@@ -262,6 +282,12 @@ def __init__(
self.host_key = key_constructor(data=decoded_host_key)
self.no_host_key_check = False
+ # An explicit constructor argument wins over the connection extra and
the default.
+ # Without this there is no way to opt out of host key verification
when the hook is
+ # built directly rather than from a Connection.
+ if constructor_no_host_key_check is not None:
+ self.no_host_key_check = constructor_no_host_key_check
Review Comment:
Correct. The check predates this PR, but the constructor override makes it
reachable for a case that is now resolvable: connection with `host_key` and
`no_host_key_check=true`, constructor `no_host_key_check=False`, which is
coherent and should connect. The reverse is inconsistent as well — constructor
`True` against a connection carrying only `host_key` skips verification without
raising. Moving the validation after the effective value is computed and
covering the precedence case.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
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]
