[GitHub] [airflow] dstandish commented on a change in pull request #6104: [AIRFLOW-4574] allow providing private_key in SSHHook

[GitBox <gi...@apache.org>]

dstandish commented on a change in pull request #6104: [AIRFLOW-4574] allow providing private_key in SSHHook
URL: https://github.com/apache/airflow/pull/6104#discussion_r324483395
 
 

 ##########
 File path: airflow/contrib/hooks/ssh_hook.py
 ##########
 @@ -160,24 +169,27 @@ def get_conn(self):
                              'against Man-In-The-Middle attacks')
             # Default is RejectPolicy
             client.set_missing_host_key_policy(paramiko.AutoAddPolicy())
-
-        if self.password and self.password.strip():
-            client.connect(hostname=self.remote_host,
-                           username=self.username,
-                           password=self.password,
-                           key_filename=self.key_file,
-                           timeout=self.timeout,
-                           compress=self.compress,
-                           port=self.port,
-                           sock=self.host_proxy)
-        else:
-            client.connect(hostname=self.remote_host,
-                           username=self.username,
-                           key_filename=self.key_file,
-                           timeout=self.timeout,
-                           compress=self.compress,
-                           port=self.port,
-                           sock=self.host_proxy)
+        connect_kwargs = dict(
+            hostname=self.remote_host,
+            username=self.username,
+            timeout=self.timeout,
+            compress=self.compress,
+            port=self.port,
+            sock=self.host_proxy
+        )
+
+        if self.password:
+            password = self.password.strip()
+            connect_kwargs.update(password=password)
+
+        # prefer pkey over key_filename when both are given
 
 Review comment:
   Admittedly I was on the fence about this too.  Ultimately of course I defer to you.  
   
   There are clearly two options when both given: 
   
   1. fail
   2. pick one
   
   **Reasoning for picking one**
   
   I guess I don't see the harm in trying at least one of them.  I figured choosing one was better because it would at least try one of them, therefore it would fail in fewer circumstances.  Though I understand throwing error would force user to resolve ambiguity.
   
   **Why pkey, if picking one**
   
   The choice of which one to pick, assuming we were to choose one, is probably less controversial: choosing the private key is better because the private key is actually a private key, while the path to file is just a path, and the file may or may not be there.
   
   **What does paramiko do?**
   
   I was curious and looked into paramiko.  What does it do when given both?  It appears that it picks pkey, but it's not super obvious to me: https://github.com/paramiko/paramiko/blob/master/paramiko/client.py#L655
   
   **Suggestion**
   
   Perhaps better yet, is when given both, then pass both to paramiko, and let it do whatever it does.  What you think?
   
   
   
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services