You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/09/07 15:45:14 UTC
[GitHub] [airflow] nathadfield opened a new pull request #18064: Fixing Vault AppRole authentication with CONN_URI
nathadfield opened a new pull request #18064:
URL: https://github.com/apache/airflow/pull/18064
This PR adds some additional logic to the VaultHook to ensure that if a connection to Vault defined as a `CONN_URI` using AppRole authentication is used that the `role_id` is retrieved from `connection.login`.
closes: #18053
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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
[GitHub] [airflow] nathadfield commented on a change in pull request #18064: Fixing Vault AppRole authentication with CONN_URI
Posted by GitBox <gi...@apache.org>.
nathadfield commented on a change in pull request #18064:
URL: https://github.com/apache/airflow/pull/18064#discussion_r706113088
##########
File path: airflow/providers/hashicorp/hooks/vault.py
##########
@@ -148,7 +149,21 @@ 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')
Review comment:
@potiuk Yeah, fair enough. What about?
```
if auth_type == "approle":
if role_id:
warnings.warn(
"""The usage of role_id for AppRole authentication has been deprecated.
Please use connection login.""",
DeprecationWarning,
stacklevel=2,
)
if self.connection.extra_dejson.get('role_id'):
role_id = self.connection.extra_dejson.get('role_id')
warnings.warn(
"""The usage of role_id in connection extra for AppRole authentication has been deprecated.
Please use connection login.""",
DeprecationWarning,
stacklevel=2,
)
elif self.connection.login:
role_id = self.connection.login
```
Happy to rephrase the messages as you suggest.
--
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
[GitHub] [airflow] nathadfield commented on pull request #18064: Fixing Vault AppRole authentication with CONN_URI
Posted by GitBox <gi...@apache.org>.
nathadfield commented on pull request #18064:
URL: https://github.com/apache/airflow/pull/18064#issuecomment-915528124
@potiuk I'd appreciate your input again when you get a free moment. No rush though.
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #18064: Fixing Vault AppRole authentication with CONN_URI
Posted by 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
[GitHub] [airflow] nathadfield commented on pull request #18064: Fixing Vault AppRole authentication with CONN_URI
Posted by GitBox <gi...@apache.org>.
nathadfield commented on pull request #18064:
URL: https://github.com/apache/airflow/pull/18064#issuecomment-915014371
@potiuk If I add the deprecation warning should I also change the tests that now flag the warnings?
--
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
[GitHub] [airflow] nathadfield commented on a change in pull request #18064: Fixing Vault AppRole authentication with CONN_URI
Posted by GitBox <gi...@apache.org>.
nathadfield commented on a change in pull request #18064:
URL: https://github.com/apache/airflow/pull/18064#discussion_r705243210
##########
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.""",
Review comment:
Ok, so, to clarify. No deprecation of `role_id` for `aws_am` and a modified warning for `AppRole`?
--
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
[GitHub] [airflow] potiuk merged pull request #18064: Fixing Vault AppRole authentication with CONN_URI
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #18064:
URL: https://github.com/apache/airflow/pull/18064
--
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
[GitHub] [airflow] nathadfield commented on a change in pull request #18064: Fixing Vault AppRole authentication with CONN_URI
Posted by GitBox <gi...@apache.org>.
nathadfield commented on a change in pull request #18064:
URL: https://github.com/apache/airflow/pull/18064#discussion_r706236888
##########
File path: airflow/providers/hashicorp/hooks/vault.py
##########
@@ -148,7 +149,21 @@ 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')
Review comment:
Good catch!
--
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
[GitHub] [airflow] nathadfield commented on pull request #18064: Fixing Vault AppRole authentication with CONN_URI
Posted by GitBox <gi...@apache.org>.
nathadfield commented on pull request #18064:
URL: https://github.com/apache/airflow/pull/18064#issuecomment-914995916
@potiuk No problem on the docstring.
Regarding the `aws_iam`, I did look at this briefly and it seemed like a CONN_URI for this authentication method would look the same and therefore also face the same problem.
Would you like me to add he deprecation warning too?
--
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
[GitHub] [airflow] github-actions[bot] commented on pull request #18064: Fixing Vault AppRole authentication with CONN_URI
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18064:
URL: https://github.com/apache/airflow/pull/18064#issuecomment-914470149
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
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
[GitHub] [airflow] potiuk commented on pull request #18064: Fixing Vault AppRole authentication with CONN_URI
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #18064:
URL: https://github.com/apache/airflow/pull/18064#issuecomment-915028522
> Regarding the `aws_iam`, I did look at this briefly and it seemed like a CONN_URI for this authentication method would look the same and therefore also face the same problem.
Not quite sure as I think aws_iam (at least it looks like from the docs) requires three things for authentication: key (login), secret (password) and role - see example here https://www.vaultproject.io/docs/auth/aws#code-example. So I believe role is fine for `aws_iam` and you should pass it by extra. But please double check,
> @potiuk If I add the deprecation warning should I also change the tests that now flag the warnings
A test in `test_vaulr.py` would be nice indeed.
--
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
[GitHub] [airflow] nathadfield commented on a change in pull request #18064: Fixing Vault AppRole authentication with CONN_URI
Posted by GitBox <gi...@apache.org>.
nathadfield commented on a change in pull request #18064:
URL: https://github.com/apache/airflow/pull/18064#discussion_r706118393
##########
File path: airflow/providers/hashicorp/hooks/vault.py
##########
@@ -148,7 +149,21 @@ 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')
Review comment:
I think this provides different warnings for the current ways of submitting `role_id` without breaking anything.
--
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
[GitHub] [airflow] nathadfield commented on pull request #18064: Fixing Vault AppRole authentication with CONN_URI
Posted by GitBox <gi...@apache.org>.
nathadfield commented on pull request #18064:
URL: https://github.com/apache/airflow/pull/18064#issuecomment-915030060
> https://www.vaultproject.io/docs/auth/aws#code-example
Ah, ok. Quite right! Perhaps I should just separate out these two auth methods then.
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #18064: Fixing Vault AppRole authentication with CONN_URI
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #18064:
URL: https://github.com/apache/airflow/pull/18064#discussion_r706089404
##########
File path: airflow/providers/hashicorp/hooks/vault.py
##########
@@ -148,7 +149,21 @@ 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')
Review comment:
Almost perfect :).
I think we not only want to warn when uses passes role_id for "approle" but we also want to deprecate this (pseudo-code):
```
dummy_login:password@host?auth_type="approle"&role_id="nn"
```
we should here raise a similar warning as below and ask users to change it to:
```
role:password@host?auth_type="approle"
```
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #18064: Fixing Vault AppRole authentication with CONN_URI
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #18064:
URL: https://github.com/apache/airflow/pull/18064#discussion_r706157413
##########
File path: airflow/providers/hashicorp/hooks/vault.py
##########
@@ -148,7 +149,21 @@ 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')
Review comment:
I'd say this (if-> elif):
```
if auth_type == "approle":
if role_id:
warnings.warn(
"""The usage of role_id for AppRole authentication has been deprecated.
Please use connection login.""",
DeprecationWarning,
stacklevel=2,
)
elif self.connection.extra_dejson.get('role_id'):
role_id = self.connection.extra_dejson.get('role_id')
warnings.warn(
"""The usage of role_id in connection extra for AppRole authentication has been deprecated.
Please use connection login.""",
DeprecationWarning,
stacklevel=2,
)
elif self.connection.login:
role_id = self.connection.login
```
otherwise when you pass both `role_id` as parameter and login, the login one will be used, which is unexpected.
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #18064: Fixing Vault AppRole authentication with CONN_URI
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #18064:
URL: https://github.com/apache/airflow/pull/18064#discussion_r705213725
##########
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.""",
Review comment:
```suggestion
Please use connection login.""",
```
See my comment below
--
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