Update apache-airflow-providers-ssh>=4.0.0 dependency

[jroachgolf84]

As specified in #53084, starting in v5.0.0, the apache-airflow-providers-sftp is using keyword argument introduced in apache-airflow-providers-ssh v4.0.0. Thus, the minimum version of apache-airflow-providers-ssh was updated to be >=4.0.0.

closes: #53084

[eladkal]

This means we are missing a unit test in sftp provider to cover this. If we had a test we would have catch this before merging the PR
can you add the missing test?

[jroachgolf84]

This means we are missing a unit test in sftp provider to cover this. If we had a test we would have catch this before merging the PR
can you add the missing test?

@eladkal, are you referring to a unit-test to cover the instantiation of the SFTPHook, which in turn is calling SSHHook.__init__(...)? These tests already exist. There is also a test fr passing in the hook_proxy_cmd.

[eladkal]

These tests already exist. There is also a test fr passing in the hook_proxy_cmd

Attributes are frequently added to hooks around the code base with no similar issues.
If sftp provider is sensitive to changes made to ssh hook then the sftp provider needs to have proper testing to avoid such future problems. Will the sftp code break again if tomorrow attribute x is added to SSHHook?

[jroachgolf84]

Attributes are frequently added to hooks around the code base with no similar issues.
If sftp provider is sensitive to changes made to ssh hook then the sftp provider needs to have proper testing to avoid such future problems. Will the sftp code break again if tomorrow attribute x is added to SSHHook?

The issue here is that the SFTPHook adds the host_proxy_cmd to kwargs, then passes kwargs to super().__init__(...) (the SSHHook). It was the addition of the host_proxy_cmd that broke the older version of the SSHHook. With the newer version of the SSHHook, host_proxy_cmd was added as an attribute. I could be a bit confused here; where would you suggest making a change/adding a test (there are already tests that cover the instantiation of the SFTPHook, which in turn instantiates the SSHHook)?

[eladkal]

@potiuk correct me if I am wrong but we are testing against lower dependencies?

[potiuk]

Yeah - I am here with @eladkal -> it does mean we miss some test case that should be showimg the problem

[potiuk]

But it's also hard to say why. - what we do in "lowest deps" tests we bring the deps as low as possible for each provider .. so I wonder why it was passing?

[jroachgolf84]

But it's also hard to say why. - what we do in "lowest deps" tests we bring the deps as low as possible for each provider .. so I wonder why it was passing?

Agreed, it's a bit weird that it is in-fact passing. Let me take a look and see why that might be the case.

[jroachgolf84]

Reverted to the previous lowest dependency, all tests pass, even this one (which is explicitly instantiating SFTPHook and referencing host_proxy_command).

    @patch("paramiko.SSHClient")
    @patch("paramiko.ProxyCommand")
    @patch("airflow.providers.sftp.hooks.sftp.SFTPHook.get_connection")
    def test_sftp_hook_with_proxy_command(self, mock_get_connection, mock_proxy_command, mock_ssh_client):
        # Mock the connection to not have a password
        mock_connection = MagicMock()
        mock_connection.login = "user"
        mock_connection.password = None
        mock_connection.host = "example.com"
        mock_connection.port = 22
        mock_connection.extra = None
        mock_get_connection.return_value = mock_connection

        mock_sftp_client = MagicMock(spec=SFTPClient)
        mock_ssh_client.open_sftp.return_value = mock_sftp_client

        mock_transport = MagicMock()
        mock_ssh_client.return_value.get_transport.return_value = mock_transport
        mock_proxy_command.return_value = MagicMock()

        host_proxy_cmd = "ncat --proxy-auth proxy_user:**** --proxy proxy_host:port %h %p"

        hook = SFTPHook(
            remote_host="example.com",
            username="user",
            host_proxy_cmd=host_proxy_cmd,
        )

        with hook.get_managed_conn():
            mock_proxy_command.assert_called_once_with(host_proxy_cmd)
            mock_ssh_client.return_value.connect.assert_called_once_with(
                hostname="example.com",
                username="user",
                timeout=None,
                compress=True,
                port=22,
                sock=mock_proxy_command.return_value,
                look_for_keys=True,
                banner_timeout=30.0,
                auth_timeout=None,
            )

[ivan-toriya-precis]

Probably I don't understand something here, but why the PR was merged without fixing the test? As it was passing on a previous lowest dependency where it should fail?

[potiuk]

Probably I don't understand something here, but why the PR was merged without fixing the test? As it was passing on a previous lowest dependency where it should fail?

Good call. Likely test was missing to test the error in #53084 - maybe you could add one @ivan-toriya-precis ? That would be awesome.