[GitHub] [airflow] potiuk commented on a change in pull request #18064: Fixing Vault AppRole authentication with CONN_URI

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

potiuk commented on a change in pull request #18064:
URL: https://github.com/apache/airflow/pull/18064#discussion_r705213451



##########
File path: airflow/providers/hashicorp/hooks/vault.py
##########
@@ -148,9 +149,29 @@ def __init__(
             except ValueError:
                 raise VaultError(f"The version is not an int: {conn_version}. ")
 
-        if auth_type in ["approle", "aws_iam"]:
+        if auth_type == "approle":
+            if not role_id:
+                if self.connection.extra_dejson.get('role_id'):
+                    role_id = self.connection.extra_dejson.get('role_id')
+                else:
+                    role_id = self.connection.login
+            else:
+                warnings.warn(
+                    """The usage of role_id has been deprecated.
+                    Please use either the connection login or extras.""",
+                    DeprecationWarning,
+                    stacklevel=2,
+                )
+
+        if auth_type == "aws_iam":
             if not role_id:
                 role_id = self.connection.extra_dejson.get('role_id')
+            else:

Review comment:
       Hmm. That got me thinking.I think this warning is not really needed here (and the above warning should be updated).
   
   As I understand the hook original design (and I have not designed it, just extended it) was that:
   
   a) you can use connection to get all authentication information (login + password + extras)
    
   b) you can override some of the information via parameters passed to the hook directly (and it is not 'deprecated' - this is perfectly valid way of overriding the "roles", key paths and other parameters if you choose to change them (so that for example you do not have to change the connection if in one task you decide to use different role for "aws_iam"  for example.
   
   c) however you can't override login/password because they are so "basic" authentication information that you REALLY want separate connection if you change one of those is different.
   
   Now - this change deprecates overriding of role for "aws_iam" role and deprecates overriding "role" for approle via Hook parameters - which I think is not intended behaviour of the hook. You should still be able to override (without warning) the "aws_iam" role via Hook param, because it is not "basic" authentication information (secret and key are) 
   
   So I think this  warning should not be generated here.  Similarly - the warning above should be changed - we should only recommend using "login" to add "Approle" role. 
   
   I think there should still be a way to override the role in "approle" via Hook param (and for sure overrid
   




-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org