dabla commented on code in PR #73419:
URL: https://github.com/apache/airflow/pull/73419#discussion_r4187393708
##########
providers/sftp/src/airflow/providers/sftp/hooks/sftp.py:
##########
@@ -478,7 +478,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:
**[warning]** #73647 landed on main on 5 Oct; after the rebase its worker
helper needs `no_host_key_check` as a constructor argument.
Main now builds the workers through `SFTPHook._build_worker_hook()`
(dbee3c6c11), which is why this file conflicts. The helper constructs
`SFTPHook(ssh_conn_id=..., remote_host=..., ...)` first and assigns
`worker_hook.no_host_key_check = self.no_host_key_check` afterwards, because
there was no constructor parameter at the time.
With the validation this PR adds to `SSHHook.__init__`, that order is no
longer enough. For a connection that carries both `host_key` and
`no_host_key_check=true`, a parent built with `no_host_key_check=False` is
valid (the case
`test_constructor_no_host_key_check_false_resolves_conflicting_extras` covers),
but the worker is constructed without the argument, so it raises
`ValueError("Must check host key when provided")` before the attribute copy
runs.
Suggestion: drop these two hunks in favour of `_build_worker_hook()`, pass
`no_host_key_check=self.no_host_key_check` there as a constructor argument (and
take it out of the "no constructor parameter" attribute block), and add a
concurrent-transfer test with such a connection. #74209 now overlaps with
#73647 as well.
##########
providers/sftp/src/airflow/providers/sftp/hooks/sftp.py:
##########
@@ -891,8 +902,20 @@ def _parse_extras(self, conn: Connection) -> None:
host_key = extra_options.get("host_key")
nhkc_raw = extra_options.get("no_host_key_check")
- no_host_key_check = True if nhkc_raw is None else
(str(nhkc_raw).lower() == "true")
+ if nhkc_raw is None and "ignore_hostkey_verification" in extra_options:
+ warnings.warn(
+ "The `ignore_hostkey_verification` connection extra is
deprecated; "
+ "use `no_host_key_check` instead.",
+ AirflowProviderDeprecationWarning,
+ stacklevel=2,
+ )
+ nhkc_raw = extra_options["ignore_hostkey_verification"]
+ no_host_key_check = False if nhkc_raw is None else
(str(nhkc_raw).lower() == "true")
Review Comment:
**[warning]** With verification as the default, a missing
`~/.ssh/known_hosts` surfaces as `FileNotFoundError` instead of a host key
error.
`_get_conn()` always passes the expanded default path to
`asyncssh.connect(known_hosts=...)`. asyncssh tolerates a missing file only
when `known_hosts` is left unset; an explicit path is read as is. Probe against
a local asyncssh 2.24.0 server:
```
known_hosts="/nonexistent/.ssh/known_hosts" -> FileNotFoundError: [Errno 2]
No such file or directory
known_hosts=<existing file, no entry> ->
asyncssh.misc.HostKeyNotVerifiable: Host key is not trusted for host 127.0.0.1
known_hosts unset, no ~/.ssh/known_hosts ->
asyncssh.misc.HostKeyNotVerifiable: Host key is not trusted for host 127.0.0.1
```
Before this PR that path was reached only with an explicit
`no_host_key_check=false`. Now every connection without host key extras takes
it, so on a triggerer that has no `~/.ssh/known_hosts` the first error a
deferrable user sees after upgrading points at a missing file rather than at
the host key options from the changelog. The connection is still refused, so
this is about the message only; the sync hook reports paramiko's `Server
'<host>' not found in known_hosts` in the same situation.
Suggestion: when `self.known_hosts` is still the default path and the file
does not exist, leave `known_hosts` out of `conn_config` so asyncssh raises
`HostKeyNotVerifiable` (third line of the probe), or raise an error that names
`host_key` and `no_host_key_check`. (See also comment [3] in
`providers/ssh/src/airflow/providers/ssh/hooks/ssh.py`.)
##########
providers/ssh/src/airflow/providers/ssh/hooks/ssh.py:
##########
@@ -619,7 +655,15 @@ 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
+ if nhkc_raw is None and "ignore_hostkey_verification" in extra_options:
+ warnings.warn(
+ "The `ignore_hostkey_verification` connection extra is
deprecated; "
+ "use `no_host_key_check` instead.",
+ AirflowProviderDeprecationWarning,
+ stacklevel=2,
+ )
+ nhkc_raw = extra_options["ignore_hostkey_verification"]
+ no_host_key_check = str(nhkc_raw).lower() == "true" if nhkc_raw is not
None else False
Review Comment:
**[warning]** Same as comment [2] in
`providers/sftp/src/airflow/providers/sftp/hooks/sftp.py`:
`SSHHookAsync._get_conn()` hands the default `~/.ssh/known_hosts` path to
asyncssh, so with the new default a missing file gives `FileNotFoundError` in
`SSHRemoteJobTrigger` instead of a host key error.
The same fix applies here: skip `known_hosts` in `conn_config` when it is
the default path and the file is absent, or raise an error that names the host
key options.
--
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]
