[AIRFLOW-3112] Fix SFTPHook not validating hosts by default

[Gwildor]

It was checking the deprecated ignore_hostkey_verification option and
setting the correct no_host_key_check option, but the ignore_* option
worked as the inverse of the no_* option, so it should test for true.

Make sure you have checked all steps below.

Jira

• My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
  • https://issues.apache.org/jira/browse/AIRFLOW-3112

Description

• Here are some details about my PR, including screenshots of any UI changes:

Tests

• My PR adds the following unit tests OR does not need testing for this extremely good reason:
• Fixing a bug in a previous PR ([AIRFLOW-3112] Make SFTP hook to inherit SSH hook #3945). There are no new features.

Commits

• My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
  1. Subject is separated from body by a blank line
  2. Subject is limited to 50 characters (not including Jira issue reference)
  3. Subject does not end with a period
  4. Subject uses the imperative mood ("add", not "adding")
  5. Body wraps at 72 characters
  6. Body explains "what" and "why", not "how"

Documentation

• In case of new functionality, my PR adds documentation that describes how to use it.
  • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.

Code Quality

• Passes flake8 airflow/contrib/hooks/sftp_hook.py

[ashb]

This change seems to make the tests fail with SSHException: No hostkey for host localhost found. so we might probably need to update some tests too.

[Gwildor]

Hm, that's annoying, but makes sense. I'll update the tests either Thursday at the office or Saturday.

[johnchenghk01]

Wait, in previous code,

# https://github.com/apache/incubator-airflow/blob/f80138486e7828ff64fa544b4032a445b2ca140a/airflow/contrib/hooks/sftp_hook.py
if ignore_hostkey_verification == True => cnopts.hostkeys = None

# https://github.com/apache/incubator-airflow/blob/a0489d09fe4b0d3a2e0893c9cfba0e379e79390b/airflow/contrib/hooks/sftp_hook.py
# https://github.com/apache/incubator-airflow/blob/a0489d09fe4b0d3a2e0893c9cfba0e379e79390b/airflow/contrib/hooks/ssh_hook.py
if ignore_hostkey_verification == True => no_host_key_check = True  # inherit from SSHHook
if no_host_key_check == True => cnopts.hostkeys = None

[kaxil]

Just needs updating of tests. Other than that LGTM. Thanks @Gwildor

[Gwildor]

Well, I got a little bit more confused, and after reading the code again I agree that @johnchenghk01 is right. The two options are not the inverse as I thought, it's just the default that is different. So the current code does indeed work because of SSHHook's default. Which does beg the question if SSHHook's default is sensible. And also, whether SFTPHook should follow that default. I think the last one is very important, because the default behaviour of 1.10 is to fail when the host key validation failed, but I think the changes of #3945 would allow them to succeed in 2.0. So now I think SFTPHook should change the default value of no_host_key_check to False and just set it to True when ignore_hostkey_validation == 'true'.

[ashb]

The default should be to validate SSH host keys.

[Gwildor]

Ok, I've updated the PR. By default the hook now validates the host keys, which is the same behaviour as 1.10. Note that this is different behaviour than the SSHHook, but it can be argued that the SSHHook is in the wrong there. The setting can be overridden using the deprecated ignore_hostkey_validation setting, or the no_host_key_check setting. I managed to get the tests running locally (after struggling a lot to set up an sftp server in my docker container) and I've added some tests to verify that all the settings now work as intended.

[codecov-io]

Codecov Report

Merging #4085 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4085   +/-   ##
=======================================
  Coverage   76.67%   76.67%           
=======================================
  Files         199      199           
  Lines       16186    16186           
=======================================
  Hits        12411    12411           
  Misses       3775     3775

Impacted Files	Coverage Δ
airflow/utils/db.py	33.6% <ø> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7fc182e...2a7e641. Read the comment docs.

[Fokko]

Thanks @Gwildor!