SFTP hook to prefer the SSH paramiko key over the key file path

[hx-markterry]

Related: #16323

The SFTP hook does not allow for a SSH private key string to be used, only a private key file path. As it is based on the SSH hook, which does allow for a private key string to be used, a small change could align these two hooks.

This change first checks if a paramiko object is present after the SSH hook has been initialised, and the prefers that rather than the ssh key file path.

This is more a working idea than a finished PR, If this approach is deemed acceptable I'll be happy to turn this into a fully formed code change.

Thanks in advance.

[boring-cyborg]

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

• Pay attention to the quality of your code (flake8, pylint and type annotations). Our pre-commits will help you with that.
• In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
• Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
• Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
• Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
• Be sure to read the Airflow Coding style.
  Apache Airflow is a community-driven project and together we are making it better 🚀.
  In case of doubts contact the developers at:
  Mailing List: dev@airflow.apache.org
  Slack: https://s.apache.org/airflow-slack

[lidalei]

suggestion: Better to remove line 107 and 108

                if 'private_key' in extra_options:
                    self.key_file = extra_options.get('private_key')

and update doc for SFTPHook.

Note pysftp only supports RSA and DSS keys while SSHHook also supports ECDSA and Ed25519 keys. Worth to note in doc.

Ref: https://bitbucket.org/dundeemt/pysftp/src/1c0791759688a733a558b1a25d9ae04f52cf6a64/pysftp/__init__.py#lines-165:171

[potiuk]

Would you please rebase to latest main @hx-markterry and add the doc explanation ? Also a unit test covering this case is needed.

[hx-markterry]

ok will do

[aladinoss]

@potiuk & @hx-markterry what is the benefit of using separate hook for sftp sensor and operator ?
Is it better to change the documentation and use ssh hook for both sftp and ssh ?
example:

import logging
from datetime import datetime
from paramiko import SFTP_NO_SUCH_FILE
from airflow.contrib.hooks.ssh_hook import SSHHook
from airflow.operators.sensors import BaseSensorOperator
from airflow.utils.decorators import apply_defaults
from airflow.plugins_manager import AirflowPlugin

log = logging.getLogger(__name__)


class SgSFTPSensor(BaseSensorOperator):
    """
    Waits for a file or directory to be present on SFTP.

    :param path: Remote file or directory path
    :type path: str
    :param ssh_conn_id: The connection to run the sensor against
    :type shh_conn_id: str
    """
    template_fields = ('path',)

    @apply_defaults
    def __init__(self, path, shh_conn_id='ssh_default', *args, **kwargs):
        super(SgSFTPSensor, self).__init__(*args, **kwargs)
        self.path = path
        self.hook = None
        self.shh_conn_id = shh_conn_id

    def get_mod_time(self, path):
        ssh_hook = SSHHook(ssh_conn_id=self.shh_conn_id)
        conn = ssh_hook.get_conn().open_sftp()
        ftp_mdtm = conn.stat(path).st_mtime
        return datetime.fromtimestamp(ftp_mdtm).strftime('%Y%m%d%H%M%S')

    def poke(self, context):
        self.log.info('Poking for %s', self.path)
        try:
            self.get_mod_time(self.path)
        except IOError as e:
            log.info(SFTP_NO_SUCH_FILE)
            if e.errno != SFTP_NO_SUCH_FILE:
                raise e
            log.info(f"{self.path} NOT FOUND, sensor will retry!")
            return False
        log.info(f"{self.path} FOUND, sensor will exit now.")
        return True


class SgSFTPPlugin(AirflowPlugin):
    name = "sgsftpplugin"
    sensors = [
        SgSFTPSensor
    ]

[potiuk]

SFTPHook has several methods that are sftp-specific (such as retrieve_file/store_file but also many others). Those make no sense for "ssh hook".

[30blay]

@lidalei I like your suggestion because it makes the definition of private_key consistent between SFTPHook and SSHHook.

But it would be a breaking change for clients who currently pass a file path to SFTPHook's private_key. How should I go about that?

[lidalei]

@30blay Thx! I don't think private_key works for SFTPHook. SFTPHook inherits SSHHook. If we have an SFTP connection with private_key in extra fields, it will fail to initialize an SFTPHook.

https://github.com/apache/airflow/blob/main/airflow/providers/sftp/hooks/sftp.py#L69
https://github.com/apache/airflow/blob/main/airflow/providers/ssh/hooks/ssh.py#L155-L158

[lidalei]

Forgot to mention that, it didn't work for us.

[30blay]

You're right, passing a private key file path as private_key does not work for SFTPHook, despite what the doc says. I will update the doc and implement your solution

[30blay]

Done! In #18988

[potiuk]

@hx-markterry are you going to rebase that one :)?

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

[30blay]

@hx-markterry I made this PR on your fork a week ago, to move this forward. Have you seen it?

[eladkal]

@30blay since the PR is stale you can take over and open a new PR.
You can preserve the original commits so it will be co-authored by the original contributor and you.

[30blay]

@eladkal Thanks, done! #18988