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 2022/06/29 15:34:59 UTC
[GitHub] [airflow] bluek1te opened a new pull request, #24737: Pass kwargs from vault hook to hvac client
bluek1te opened a new pull request, #24737:
URL: https://github.com/apache/airflow/pull/24737
Pass kwargs from vault hook to the underlying hvac client. This allows arguments such as namespace to be passed which can be useful for enterprise applications of Vault. The same pattern is already also used in VaultSecretsBackend here:
https://github.com/apache/airflow/blob/main/airflow/providers/hashicorp/secrets/vault.py
--
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] bluek1te commented on a diff in pull request #24737: Pass kwargs from vault hook to hvac client
Posted by GitBox <gi...@apache.org>.
bluek1te commented on code in PR #24737:
URL: https://github.com/apache/airflow/pull/24737#discussion_r922499293
##########
airflow/providers/hashicorp/hooks/vault.py:
##########
@@ -220,6 +224,7 @@ def __init__(
radius_host=radius_host,
radius_secret=self.connection.password,
radius_port=radius_port,
+ **client_kwargs
Review Comment:
I tried to capture the spirit of everything you said in the most recent amend. Will start working on verifying the precedence in the tests. Just to double check, the precedence should be:
conn attributes > conn.extra keyword arguments > conn.extra.client_kwargs keyword arguments?
--
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] bluek1te commented on pull request #24737: Pass kwargs from vault hook to hvac client
Posted by GitBox <gi...@apache.org>.
bluek1te commented on PR #24737:
URL: https://github.com/apache/airflow/pull/24737#issuecomment-1175099330
@dstandish, I've just edited the commit to use extra like you said, I am still going through the process of testing to see if this works from the webserver GUI, but are there any tests you think that I should run or is this worth writing a separate set of tests for? ( I am kind of new to this project, so any protips are appreciated, thnx!)
--
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] bluek1te commented on pull request #24737: Pass kwargs from vault hook to hvac client
Posted by GitBox <gi...@apache.org>.
bluek1te commented on PR #24737:
URL: https://github.com/apache/airflow/pull/24737#issuecomment-1189131323
I added a test in providers/hashicorp/vault.py that looks to see if client kwargs gets passed all the way down to the underlying hvac component. Lmk if that is not sufficient.
--
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] bluek1te commented on a diff in pull request #24737: Pass kwargs from vault hook to hvac client
Posted by GitBox <gi...@apache.org>.
bluek1te commented on code in PR #24737:
URL: https://github.com/apache/airflow/pull/24737#discussion_r925839358
##########
tests/providers/hashicorp/hooks/test_vault.py:
##########
@@ -618,6 +618,33 @@ def test_kubernetes_dejson(self, mock_hvac, mock_get_connection):
test_client.auth_kubernetes.assert_called_with(role="kube_role", jwt="data")
test_client.is_authenticated.assert_called_with()
assert 2 == test_hook.vault_client.kv_engine_version
+
+ @mock.patch("airflow.providers.hashicorp.hooks.vault.VaultHook.get_connection")
+ @mock.patch("airflow.providers.hashicorp._internal_client.vault_client.hvac")
+ def test_client_kwargs(self, mock_hvac, mock_get_connection):
+ mock_client = mock.MagicMock()
Review Comment:
I added a summary docstring, I didn't find any other examples in this file to bounce off of, but if it's not formatted correctly or if you have a link to a good example of how it should be formatted/written I'll be happy to change it.
--
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] bluek1te commented on a diff in pull request #24737: Pass kwargs from vault hook to hvac client
Posted by GitBox <gi...@apache.org>.
bluek1te commented on code in PR #24737:
URL: https://github.com/apache/airflow/pull/24737#discussion_r925838572
##########
airflow/providers/hashicorp/hooks/vault.py:
##########
@@ -135,6 +136,9 @@ def __init__(
except ValueError:
raise VaultError(f"The version is not an int: {conn_version}. ")
+ if not client_kwargs:
+ client_kwargs = self.connection.extra_dejson.get("client_kwargs", {})
Review Comment:
I did some refactoring around this - I changed the name of the kwargs passed in through the Vault Hook init back to kwargs (I had it as client_kwargs before), and client_kwargs is now what gets pulled out of connection extra. client_kwargs gets fused with kwargs as client_kwargs, with kwargs taking precedence. client_kwargs is what ultimately gets passed to the HVAC client init.
--
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] dstandish commented on a diff in pull request #24737: Pass kwargs from vault hook to hvac client
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #24737:
URL: https://github.com/apache/airflow/pull/24737#discussion_r916262411
##########
airflow/providers/hashicorp/hooks/vault.py:
##########
@@ -220,6 +224,7 @@ def __init__(
radius_host=radius_host,
radius_secret=self.connection.password,
radius_port=radius_port,
+ **client_kwargs
Review Comment:
so if you are up to it you could make this a little nicer by handling the case where user specifies something both in client kwargs (e.g. from conn extra) and it's already specified by the hook -- e.g. if user puts `azure_tenant_id` in conn.extra.
so you 'd do something like
```
client_kwargs.update(
**dict(
...,
password=self.connection.password,
key_id=self.connection.login,
secret_id=self.connection.password,
role_id=role_id,
kubernetes_role=kubernetes_role,
kubernetes_jwt_path=kubernetes_jwt_path,
gcp_key_path=gcp_key_path,
gcp_keyfile_dict=gcp_keyfile_dict,
gcp_scopes=gcp_scopes,
azure_tenant_id=azure_tenant_id,
azure_resource=azure_resource,
radius_host=radius_host,
radius_secret=self.connection.password,
radius_port=radius_port,
...
)
```
and if you you want to go the extra mile you can support passing something like `key_id` through extra also (cus, really, that's more natural / intuitive than putting it in `login`)
so then what you'd want to do in tests is pretty much just verify that your order of precedence is working properly.
##########
airflow/providers/hashicorp/hooks/vault.py:
##########
@@ -135,6 +136,9 @@ def __init__(
except ValueError:
raise VaultError(f"The version is not an int: {conn_version}. ")
+ if not client_kwargs:
+ client_kwargs = self.connection.extra_dejson.get("client_kwargs")
Review Comment:
should ensure that client_kwargs is at least a {} or else it will break when you try to spread it with `**`
--
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] closed pull request #24737: Pass kwargs from vault hook to hvac client
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #24737: Pass kwargs from vault hook to hvac client
URL: https://github.com/apache/airflow/pull/24737
--
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 #24737: Pass kwargs from vault hook to hvac client
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24737:
URL: https://github.com/apache/airflow/pull/24737#issuecomment-1199090772
still some fixes :). I think you also need to rebase to account for other errors:
--
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] bluek1te commented on pull request #24737: Pass kwargs from vault hook to hvac client
Posted by GitBox <gi...@apache.org>.
bluek1te commented on PR #24737:
URL: https://github.com/apache/airflow/pull/24737#issuecomment-1171546810
Sure, I think that would be an excellent idea. I will take some time to see how it is handled through the other connection types so I can mimic the same patterns.
--
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 #24737: Pass kwargs from vault hook to hvac client
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24737:
URL: https://github.com/apache/airflow/pull/24737#issuecomment-1178125619
> @dstandish, I've just edited the commit to use extra like you said, I am still going through the process of testing to see if this works from the webserver GUI, but are there any tests you think that I should run or is this worth writing a separate set of tests for? ( I am kind of new to this project, so any protips are appreciated, thnx!)
Just look at the tests in "tests/providers" - they are usually corresponding to the objects in "airflow/providers"
--
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] bluek1te commented on pull request #24737: Pass kwargs from vault hook to hvac client
Posted by GitBox <gi...@apache.org>.
bluek1te commented on PR #24737:
URL: https://github.com/apache/airflow/pull/24737#issuecomment-1185654352
@potiuk Ah sorry, I came down with covid this week, am just getting back into the flow of things - I should have something this weekend and hopefully can make it to the next wave.
--
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] dstandish commented on a diff in pull request #24737: Pass kwargs from vault hook to hvac client
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #24737:
URL: https://github.com/apache/airflow/pull/24737#discussion_r924594321
##########
tests/providers/hashicorp/hooks/test_vault.py:
##########
@@ -618,6 +618,33 @@ def test_kubernetes_dejson(self, mock_hvac, mock_get_connection):
test_client.auth_kubernetes.assert_called_with(role="kube_role", jwt="data")
test_client.is_authenticated.assert_called_with()
assert 2 == test_hook.vault_client.kv_engine_version
+
+ @mock.patch("airflow.providers.hashicorp.hooks.vault.VaultHook.get_connection")
+ @mock.patch("airflow.providers.hashicorp._internal_client.vault_client.hvac")
+ def test_client_kwargs(self, mock_hvac, mock_get_connection):
+ mock_client = mock.MagicMock()
Review Comment:
can you add a docstring here just to briefly explain what you're testing in this test
##########
tests/providers/hashicorp/hooks/test_vault.py:
##########
@@ -618,6 +618,33 @@ def test_kubernetes_dejson(self, mock_hvac, mock_get_connection):
test_client.auth_kubernetes.assert_called_with(role="kube_role", jwt="data")
test_client.is_authenticated.assert_called_with()
assert 2 == test_hook.vault_client.kv_engine_version
+
+ @mock.patch("airflow.providers.hashicorp.hooks.vault.VaultHook.get_connection")
+ @mock.patch("airflow.providers.hashicorp._internal_client.vault_client.hvac")
+ def test_client_kwargs(self, mock_hvac, mock_get_connection):
+ mock_client = mock.MagicMock()
+ mock_hvac.Client.return_value = mock_client
+ mock_connection = self.get_mock_connection()
+ mock_get_connection.return_value = mock_connection
+
+ connection_dict = {
+ "client_kwargs": {
+ "namespace": "name",
+ "timeout": 50,
+ }
+ }
+
+ mock_connection.extra_dejson.get.side_effect = connection_dict.get
+ kwargs = {
+ "vault_conn_id": "vault_conn_id",
+ }
+ with patch("builtins.open", mock_open(read_data="data")) as mock_file:
+ test_hook = VaultHook(**kwargs)
+ test_client = test_hook.get_conn()
+ mock_get_connection.assert_called_with("vault_conn_id")
+ mock_hvac.Client.assert_called_with(url='http://localhost:8180', namespace="name", timeout=50)
Review Comment:
best if you can verify the order of precedence here. not seeing that.
##########
airflow/providers/hashicorp/hooks/vault.py:
##########
@@ -135,6 +136,9 @@ def __init__(
except ValueError:
raise VaultError(f"The version is not an int: {conn_version}. ")
+ if not client_kwargs:
+ client_kwargs = self.connection.extra_dejson.get("client_kwargs", {})
Review Comment:
here you should use `merge_dicts` from airflow.utils.helpers
--
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] dstandish commented on pull request #24737: Pass kwargs from vault hook to hvac client
Posted by GitBox <gi...@apache.org>.
dstandish commented on PR #24737:
URL: https://github.com/apache/airflow/pull/24737#issuecomment-1170386373
seems like a good idea but think you need to think about how to incorporate this support for the Connection object. if you can do it through hook init params, probably you should be able to configure it through connection.
in connection i've seen it done two ways. either (1) you forward anything in extra (besides maybe a list of reserved params) to client as kwargs or (2) you add a `client_kwargs` dict within extra.
wdyt?
--
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] boring-cyborg[bot] commented on pull request #24737: Pass kwargs from vault hook to hvac client
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #24737:
URL: https://github.com/apache/airflow/pull/24737#issuecomment-1170133404
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, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) 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](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) 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](https://www.apache.org/foundation/policies/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]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
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
--
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 #24737: Pass kwargs from vault hook to hvac client
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24737:
URL: https://github.com/apache/airflow/pull/24737#issuecomment-1180332835
I am preparing to the new provider's release so @bluek1te if you want it to make it to this wave, making it green and adress the comments today/tomorrow is the prerequisite.
--
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] bluek1te commented on a diff in pull request #24737: Pass kwargs from vault hook to hvac client
Posted by GitBox <gi...@apache.org>.
bluek1te commented on code in PR #24737:
URL: https://github.com/apache/airflow/pull/24737#discussion_r925839995
##########
tests/providers/hashicorp/hooks/test_vault.py:
##########
@@ -618,6 +618,33 @@ def test_kubernetes_dejson(self, mock_hvac, mock_get_connection):
test_client.auth_kubernetes.assert_called_with(role="kube_role", jwt="data")
test_client.is_authenticated.assert_called_with()
assert 2 == test_hook.vault_client.kv_engine_version
+
+ @mock.patch("airflow.providers.hashicorp.hooks.vault.VaultHook.get_connection")
+ @mock.patch("airflow.providers.hashicorp._internal_client.vault_client.hvac")
+ def test_client_kwargs(self, mock_hvac, mock_get_connection):
+ mock_client = mock.MagicMock()
+ mock_hvac.Client.return_value = mock_client
+ mock_connection = self.get_mock_connection()
+ mock_get_connection.return_value = mock_connection
+
+ connection_dict = {
+ "client_kwargs": {
+ "namespace": "name",
+ "timeout": 50,
+ }
+ }
+
+ mock_connection.extra_dejson.get.side_effect = connection_dict.get
+ kwargs = {
+ "vault_conn_id": "vault_conn_id",
+ }
+ with patch("builtins.open", mock_open(read_data="data")) as mock_file:
+ test_hook = VaultHook(**kwargs)
+ test_client = test_hook.get_conn()
+ mock_get_connection.assert_called_with("vault_conn_id")
+ mock_hvac.Client.assert_called_with(url='http://localhost:8180', namespace="name", timeout=50)
Review Comment:
I added a param in kwargs and connection_dict that is keyed with "generic_arg". They both have different values and the test now makes sure that the value in kwargs takes precedence.
--
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] bluek1te commented on pull request #24737: Pass kwargs from vault hook to hvac client
Posted by GitBox <gi...@apache.org>.
bluek1te commented on PR #24737:
URL: https://github.com/apache/airflow/pull/24737#issuecomment-1197234717
Sorry, just realized I failed some static checks - I've fixed the issue.
--
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 #24737: Pass kwargs from vault hook to hvac client
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #24737:
URL: https://github.com/apache/airflow/pull/24737#issuecomment-1244734226
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.
--
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