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/03/25 17:37:18 UTC
[GitHub] [airflow] davlum opened a new pull request #15013: Enable Connection creation from Vault parameters
davlum opened a new pull request #15013:
URL: https://github.com/apache/airflow/pull/15013
Currently using the Vault secrets backends requires that users store
the secrets in connection URI format:
https://airflow.apache.org/docs/apache-airflow/stable/howto/connection.html#connection-uri-format
Unfortunately the connection URI format is not capable of expressing
all vales of the Connection class. In particular the Connection
class allows for arbitrary string values for the `extra` parameter,
while the URI format requires that this parameter be unnested JSON
so that it can serialize into query parameters.
```
>>> Connection(conn_id='id', conn_type='http', extra='foobar').get_uri()
[2021-03-25 13:31:07,535] {connection.py:337} ERROR - Expecting value: line 1 column 1 (char 0)
Traceback (most recent call last):
File "/Users/da.lum/code/python/airflow/airflow/models/connection.py", line 335, in extra_dejson
obj = json.loads(self.extra)
File "/nix/store/8kzdflq0v06fq0mh9m2fd73gnyqp57xr-python3-3.7.3/lib/python3.7/json/__init__.py", line 348, in loads
return _default_decoder.decode(s)
File "/nix/store/8kzdflq0v06fq0mh9m2fd73gnyqp57xr-python3-3.7.3/lib/python3.7/json/decoder.py", line 337, in decode
obj, end = self.raw_decode(s, idx=_w(s, 0).end())
File "/nix/store/8kzdflq0v06fq0mh9m2fd73gnyqp57xr-python3-3.7.3/lib/python3.7/json/decoder.py", line 355, in raw_decode
raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)
[2021-03-25 13:31:07,535] {connection.py:338} ERROR - Failed parsing the json for conn_id id
'http://'
```
As shown, the `extra` data is missing from the return value `http://`.
Although there is an error logged, this does not help users who were
previously able to store other data.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/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/master/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810699071
@mik-laj curious what do you think about deprecating the conn uri format and replacing with JSON, or perhaps allowing both globally? We could implement backward compat with try json parse except.
--
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
[GitHub] [airflow] davlum edited a comment on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
davlum edited a comment on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810443658
@dstandish If you read the description of PR you can see an example of `get_uri` failing. The fact is that connection URI is a lossy format and is required by Vault integration. As somebody who manages several Airflow instances and had to migrate all the tenants connections into Vault this was a breaking change as users who did not have unnested JSON had their connection broken when calling `.get_uri()` on it.
It was then required to go in and manually change the application code for tenants that used their `extra` parameters to use the appropriate format. This is not a good workflow.
> There is a benefit inherent in having only one perfectly good way to do things.
There does not currently exist one perfectly good way to do things. There are two incompatible ways. All values of `Connection` cannot be serialized to connection URI.
If you want `get_uri` to be a legitimate format you need to do validation on the `Connection` object to verify that it serializes properly. This would be a breaking change.
--
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
[GitHub] [airflow] mik-laj commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r606666991
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -18,8 +18,10 @@
"""Objects relating to sourcing connections & variables from Hashicorp Vault"""
from typing import Optional
+from airflow.models.connection import Connection
from airflow.providers.hashicorp._internal_client.vault_client import _VaultClient # noqa
from airflow.secrets import BaseSecretsBackend
+from airflow.secrets.local_filesystem import _create_connection
Review comment:
You can't add a new feature to the core because we want to be backwards compatible with Airflow 2.0.0. I opened the discussions to loosen this restriction, because they do not conform to reality. I think we should maintain backward compatibility with the MINOR release, not the MAJOR.
I invite you to the discussion on the mailing list if you would like to share your thoughts.
--
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
[GitHub] [airflow] natanweinberger commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
natanweinberger commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r656404549
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -188,12 +204,28 @@ def get_conn_uri(self, conn_id: str) -> Optional[str]:
:rtype: str
:return: The connection uri retrieved from the secret
"""
- if self.connections_path is None:
+ response = self.get_response(conn_id)
+
+ return response.get("conn_uri") if response else None
+
+ def get_connection(self, conn_id: str) -> Optional[Connection]:
+ """
+ Get connection from Vault as secret. Prioritize conn_uri if exists,
+ if not fall back to normal Connection creation.
+
+ :type conn_id: str
+ :rtype: Connection
+ :return: A Connection object constructed from Vault data
+ """
+ response = self.get_response(conn_id)
+ if response is None:
return None
- else:
- secret_path = self.build_path(self.connections_path, conn_id)
- response = self.vault_client.get_secret(secret_path=secret_path)
- return response.get("conn_uri") if response else None
+
+ uri = response.get("conn_uri")
+ if uri:
+ return uri
Review comment:
This won't return a connection object. I think this should be
```python
if uri:
return Connection(conn_id, uri=uri)
return Connection(conn_id, **response)
# or _create_connection, but constructor handles this logic pretty well now without secrets backend
```
--
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
[GitHub] [airflow] nathadfield commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
nathadfield commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810013687
> > Unfortunately the connection URI format is not capable of expressing
> > all values of the Connection class. In particular the Connection
> > class allows for arbitrary string values for the extra parameter,
> > while the URI format requires that this parameter be unnested JSON
> > so that it can serialize into query parameters.
>
> That's not true. Or at least I don't _think_ that's true.
>
> We have a convenience method `get_uri` you can use to produce the URI.
>
> Try creating your connection as desired with init params including your json `extra`. Then call `get_uri` on the connection.
>
> Use this URI in vault.
>
> If you have a json value for extra that can not be serialized to airflow URI please post it here.
@dstandish Even so, might not this be a good feature anyway?
A `conn_uri` is fine but they can be a bit unwieldy and it is not that straightforward to create one correctly. I would personally also like the ability to store the individual elements of a connection as a secret in Vault/GSM and for Airflow to build the connection at runtime.
--
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
[GitHub] [airflow] mik-laj commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810709459
I see no reason to do this. Too many users already use URIs. Besides, for simple connections it is a very good representation.
--
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
[GitHub] [airflow] mik-laj commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r606666991
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -18,8 +18,10 @@
"""Objects relating to sourcing connections & variables from Hashicorp Vault"""
from typing import Optional
+from airflow.models.connection import Connection
from airflow.providers.hashicorp._internal_client.vault_client import _VaultClient # noqa
from airflow.secrets import BaseSecretsBackend
+from airflow.secrets.local_filesystem import _create_connection
Review comment:
You can't add a new feature to the core because we want to be backwards compatible with Airflow 2.0.0. I opened the discussions to loosen this file, because they do not conform to reality. I think we should maintain backward compatibility with the MINOR release, not the MAJOR.
I invite you to the discussion on the mailing list if you would like to share your thoughts.
--
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
[GitHub] [airflow] davlum edited a comment on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
davlum edited a comment on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810443658
@dstandish If you read the description of PR you can see an example of `get_uri` failing. The fact is that connection URI is a lossy format and is required by Vault integration. As somebody who manages several Airflow instances and had to migrate all the tenants connections into Vault this was a breaking change as users who did not have unnested JSON had their connection broken when calling `.get_uri()` on it.
It was then required to go in and manually change the application code for tenants that used their `extra` parameters to use the appropriate format. This is not a good workflow.
> There is a benefit inherent in having only one perfectly good way to do things.
There does not currently exist one perfectly good way to do things. There are two incompatible ways, and the main interface users use is the `Connection` object. All values of `Connection` cannot be serialized to connection URI.
If you want `get_uri` to be a legitimate format you need to do validation on the `Connection` object to verify that it serializes properly. This would be a breaking change.
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r605975880
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -188,12 +204,28 @@ def get_conn_uri(self, conn_id: str) -> Optional[str]:
:rtype: str
:return: The connection uri retrieved from the secret
"""
- if self.connections_path is None:
+ response = self.get_response(conn_id)
+
+ return response.get("conn_uri") if response else None
+
+ def get_connection(self, conn_id: str) -> Optional['Connection']:
+ """
+ Get connection from Vault as secret. Prioritize conn_uri if exists,
+ if not fall back to normal Connection creation.
+
+ :type conn_id: str
+ :rtype: Connection
+ :return: A Connection object constructed from Vault data
+ """
+ response = self.get_response(conn_id)
+ if response is None:
return None
- else:
- secret_path = self.build_path(self.connections_path, conn_id)
- response = self.vault_client.get_secret(secret_path=secret_path)
- return response.get("conn_uri") if response else None
+
+ uri = response.get("conn_uri")
+ if uri:
+ return uri
+
+ _create_connection(conn_id, response)
Review comment:
i think what makes sense to do here is to promote this to a standard way of parsing creds from json, so that all such implementations can use it, and so that it is not associated with one particular backend (in this case local files)
i think the right place for this would probably be in connection.py but i could see it going in the root secrets module / package also.
what do you think @mik-laj
--
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
[GitHub] [airflow] mik-laj edited a comment on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
mik-laj edited a comment on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810692491
> Curious what the release target will be because it will be a breaking change.
This will be the release manager's decision, but I would consider that this is not a breaking change as the design assumption of this feature was to store objects that have a name and a value. Now we only improve this feature by adding validation. So I think, we should release it in Airflow 2.1.
>. Support object backend on Vault in this PR?
Yes. Exactly. It would be fantastic if we could separate the common part with LocalFilesystemBackend.
--
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
[GitHub] [airflow] mik-laj commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r606683165
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -18,8 +18,10 @@
"""Objects relating to sourcing connections & variables from Hashicorp Vault"""
from typing import Optional
+from airflow.models.connection import Connection
from airflow.providers.hashicorp._internal_client.vault_client import _VaultClient # noqa
from airflow.secrets import BaseSecretsBackend
+from airflow.secrets.local_filesystem import _create_connection
Review comment:
Secret Backend won't work with Airflow 2.0.0, so this is a breaking change to providers packages. This doesn't make breaking changes to Airflow 2.0.0, but to providers packages it does.
--
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
[GitHub] [airflow] dstandish edited a comment on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810705746
> I think we support both. The specific choice depends on the specific use case. To be able to use the same value as an environment variable and to copy easily to the clipboard, you can use a URI. for more convenient editing, you can use object representation.
No I mean deprecate across airflow including with env vars and cli ( or support both simultaneously )
There's not really anything special about the uri format. We could store json in env vars
My support of using conn uri is like you said the convenience of switching between backend and env and cli but if we switch to json globally that could be a good thing
--
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
[GitHub] [airflow] dstandish commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810025108
I see you added an example where extra is not valid json. Why do you want to do that? Why not just use JSON? E.g. if you put fubar in quotes probably it works
--
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
[GitHub] [airflow] mik-laj commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r613753386
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -18,8 +18,10 @@
"""Objects relating to sourcing connections & variables from Hashicorp Vault"""
from typing import Optional
+from airflow.models.connection import Connection
from airflow.providers.hashicorp._internal_client.vault_client import _VaultClient # noqa
from airflow.secrets import BaseSecretsBackend
+from airflow.secrets.local_filesystem import _create_connection
Review comment:
I started discussions on this subject on the mailing list, but I haven't checked it recently. I encourage you to discuss it yourself because then we will be able to work out a solution to this problem faster.
https://lists.apache.org/thread.html/r713f180120d0a39b53567812eb5db34f992ec81979818d2175598b71%40%3Cdev.airflow.apache.org%3E
--
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
[GitHub] [airflow] dstandish commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810604429
@avieth
>However, AFAICT, the constructor will not check this, and will not fail when extra is not JSON. But get_uri does check this (implicitly, by trying to decode then urlencode), and so we have the very weird case in which we can construct a Connection object for which get_uri fails, as @davlum has shown. I'd say that's fairly counterintuitive and surprising.
> Is there a good reason why extra must be encoded JSON? Seems like a nice solution would be to allow for any string there
OK i was able to add support in get_uri for arbitrary i.e. non-json string in #15100. behavior is still to return `{}` when you call `extra_dejson` in this case. However, it can now be serialized to URI and back without loss.
--
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
[GitHub] [airflow] dstandish edited a comment on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810705746
> I think we support both. The specific choice depends on the specific use case. To be able to use the same value as an environment variable and to copy easily to the clipboard, you can use a URI. for more convenient editing, you can use object representation.
No I mean deprecate across airflow including with env vars ( or support both simultaneously )
There's not really anything special about the uri format. We could store json in env vars
My support of using conn uri is like you said the convenience of switching between backend and env and cli but if we switch to json globally that could be a good thing
--
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
[GitHub] [airflow] dstandish commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-809839608
> Unfortunately the connection URI format is not capable of expressing
all values of the Connection class. In particular the Connection
class allows for arbitrary string values for the extra parameter,
while the URI format requires that this parameter be unnested JSON
so that it can serialize into query parameters.
That's not true.
We have a convenience method `get_uri` you can use to produce the URI.
Try creating your connection as desired with init params including your json `extra`. Then call `get_uri` on the connection. Use this URI.
--
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
[GitHub] [airflow] mik-laj commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810672565
> I would be in favor of only using the airflow conn uri format. But let's see what others think -- I'm just one voice here so don't be too discouraged.
I like the idea of supporting other formats. LocalFilesystemBackend supports already object representation also:
https://github.com/apache/airflow/blob/a4aee3f1d0c27f7c6010e784611ee943009c7498/airflow/secrets/local_filesystem.py#L188-L223
--
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
[GitHub] [airflow] mik-laj commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r615969275
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -18,8 +18,10 @@
"""Objects relating to sourcing connections & variables from Hashicorp Vault"""
from typing import Optional
+from airflow.models.connection import Connection
from airflow.providers.hashicorp._internal_client.vault_client import _VaultClient # noqa
from airflow.secrets import BaseSecretsBackend
+from airflow.secrets.local_filesystem import _create_connection
Review comment:
I have the impression that we have managed to reach a consensus. This change will be in the next provider package after Airflow 2.1 is released. There is a slight problem with the provider packages releases process. We always release providers packages from the main branch, so we cannot merge this PR until we release Airflow 2.1 and we will start releasing providers packages that will be compatible with this version.
In the meantime, we can create a new public method that will create connections from the dictionary. This method will be released in Airflow 2.1.
https://github.com/apache/airflow/pull/15425
--
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
[GitHub] [airflow] dstandish edited a comment on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810025108
I see you added an example where extra is not valid json. Why do you want to do that? Why not just use JSON? E.g. if you put fubar in quotes probably it works
Then when you call extra_dejson it should return just the string fubar
--
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
[GitHub] [airflow] mik-laj commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810694589
> Seems like a nice solution would be to allow for any string there.
Do you have any use cases that you can't solve with JSON? Object representation is more future-proof because you can always add a new key and tag the old one as deprecaated. It allows for smooth updates.
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r605975880
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -188,12 +204,28 @@ def get_conn_uri(self, conn_id: str) -> Optional[str]:
:rtype: str
:return: The connection uri retrieved from the secret
"""
- if self.connections_path is None:
+ response = self.get_response(conn_id)
+
+ return response.get("conn_uri") if response else None
+
+ def get_connection(self, conn_id: str) -> Optional['Connection']:
+ """
+ Get connection from Vault as secret. Prioritize conn_uri if exists,
+ if not fall back to normal Connection creation.
+
+ :type conn_id: str
+ :rtype: Connection
+ :return: A Connection object constructed from Vault data
+ """
+ response = self.get_response(conn_id)
+ if response is None:
return None
- else:
- secret_path = self.build_path(self.connections_path, conn_id)
- response = self.vault_client.get_secret(secret_path=secret_path)
- return response.get("conn_uri") if response else None
+
+ uri = response.get("conn_uri")
+ if uri:
+ return uri
+
+ _create_connection(conn_id, response)
Review comment:
i think what makes sense to do here is to promote `_create_connection` to a standard way of parsing creds from json _(and a non-private function)_, so that all such implementations can use it, and so that it is not associated with one particular backend (in this case local files)
i think the right place for this would probably be in connection.py but i could see it going in the root secrets module / package also.
what do you think @mik-laj
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r613745380
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -18,8 +18,10 @@
"""Objects relating to sourcing connections & variables from Hashicorp Vault"""
from typing import Optional
+from airflow.models.connection import Connection
from airflow.providers.hashicorp._internal_client.vault_client import _VaultClient # noqa
from airflow.secrets import BaseSecretsBackend
+from airflow.secrets.local_filesystem import _create_connection
Review comment:
So you are saying that if this vault provider expects a `from_dict` method on `Connection` (say, to be added in 2.1) the provider won't work with 2.0, but we've promised that it will.
should we perhaps then just merge this as is? or perhaps duplicate this private function into a vault provider utils module with a deprecation warning?
--
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
[GitHub] [airflow] mik-laj commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810672840
> The additional validation should have been added on the Connection object
Yes. I think we should add it. Can you help with 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] davlum commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
davlum commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810671903
@mik-laj
> We expect the extra field to be a dictionary in Web UI.
This must be as of Airflow 2.0, because this isn't the case in 1.10.x. The additional validation should have been added on the `Connection` object to because it is still perfectly valid to add any data to the `extra` column.
--
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
[GitHub] [airflow] kaxil commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-865934306
marked it as 2.2 since it is not a bug fix
--
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
[GitHub] [airflow] mik-laj commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810692491
> Curious what the release target will be because it will be a breaking change.
This will be the release manager's decision, but I would consider that this is not a breaking change as the design assumption of this feature was to store objects that have a name and a value. Now we only improve this feature by adding validation. So I think, we should release it in Airflow 2.1.
--
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
[GitHub] [airflow] mik-laj commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r606649998
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -188,12 +204,28 @@ def get_conn_uri(self, conn_id: str) -> Optional[str]:
:rtype: str
:return: The connection uri retrieved from the secret
"""
- if self.connections_path is None:
+ response = self.get_response(conn_id)
+
+ return response.get("conn_uri") if response else None
+
+ def get_connection(self, conn_id: str) -> Optional['Connection']:
+ """
+ Get connection from Vault as secret. Prioritize conn_uri if exists,
+ if not fall back to normal Connection creation.
+
+ :type conn_id: str
+ :rtype: Connection
+ :return: A Connection object constructed from Vault data
+ """
+ response = self.get_response(conn_id)
+ if response is None:
return None
- else:
- secret_path = self.build_path(self.connections_path, conn_id)
- response = self.vault_client.get_secret(secret_path=secret_path)
- return response.get("conn_uri") if response else None
+
+ uri = response.get("conn_uri")
+ if uri:
+ return uri
+
+ _create_connection(conn_id, response)
Review comment:
I support this idea, but it can be a bit problematic now.
See: https://github.com/apache/airflow/pull/15013#discussion_r606649464
--
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
[GitHub] [airflow] dstandish edited a comment on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810020430
There is a benefit inherent in having only one perfectly good way to do things.
Sometimes when you want to do something another way it's best kept in a private repo for your company or posted on a blog or your own github.
I would be in favor of only using the airflow conn uri format. But let's see what others think -- I'm just one voice here so don't be too discouraged.
In the meantime I encourage you to try the uri generation function we have. There is documentation on generating the uri in the managing connections doc.
--
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
[GitHub] [airflow] mik-laj edited a comment on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
mik-laj edited a comment on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810692491
> Curious what the release target will be because it will be a breaking change.
This will be the release manager's decision, but I would consider that this is not a breaking change as the design assumption of this feature was to store objects that have a name and a value. Now we only improve this feature by adding validation. So I think, we should release it in Airflow 2.1.
>. Support object backend on Vault in this PR?
Yes. Exactly. It would be fantastic if we could separate the common part with LocalFilesystemBackend.
--
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
[GitHub] [airflow] dstandish commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810477193
Yeah you make a good point that it can't store arbitrary json _(I do imagine we could add support for this within the URI format)_.
Based on that assumption, we do have [fairly comprehensive tests](https://github.com/apache/airflow/blob/master/tests/models/test_connection.py#L308-L358) that verify that URI parsing and URI generation produce consistent results.
But it's true we don't test the case where you store an arbitrary value in extra such as `hello` or `fubar`.
Currently it is assumed dict, so like `{'hello': 'fubar'}`
Anyway I'll let others chime in at this point.
--
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
[GitHub] [airflow] avieth commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
avieth commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810480452
In the documentation of the `Connection` class, it is stated that `extra` should be encoded JSON.
https://github.com/apache/airflow/blob/6b9b0675c5ece22a1c382ebb9b904fb18f486211/airflow/models/connection.py#L87
However, AFAICT, the constructor will not check this, and will not fail when `extra` is not JSON. But `get_uri` _does_ check this (implicitly, by trying to decode then urlencode), and so we have the very weird case in which we can construct a `Connection` object for which `get_uri` fails, as @davlum has shown. I'd say that's fairly counterintuitive and surprising.
Is there a good reason why `extra` must be encoded JSON? Seems like a nice solution would be to allow for any string there.
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r613745380
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -18,8 +18,10 @@
"""Objects relating to sourcing connections & variables from Hashicorp Vault"""
from typing import Optional
+from airflow.models.connection import Connection
from airflow.providers.hashicorp._internal_client.vault_client import _VaultClient # noqa
from airflow.secrets import BaseSecretsBackend
+from airflow.secrets.local_filesystem import _create_connection
Review comment:
So you are saying that if this vault provider expects a `from_dict` method on `Connection` (say, to be added in 2.1) the provider won't work with 2.0, but we've promised that it will.
should we perhaps then just merge this as is? or perhaps duplicate this private function into this provider into a vault provider utils module with a deprecation warning?
--
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
[GitHub] [airflow] mik-laj commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r606683165
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -18,8 +18,10 @@
"""Objects relating to sourcing connections & variables from Hashicorp Vault"""
from typing import Optional
+from airflow.models.connection import Connection
from airflow.providers.hashicorp._internal_client.vault_client import _VaultClient # noqa
from airflow.secrets import BaseSecretsBackend
+from airflow.secrets.local_filesystem import _create_connection
Review comment:
Secret Backend won't work with Airflow 2.0.0, so this is a breaking change to providers packages. This doesn't make some breaking changes to Airflow 2.0.0, but to providers packages it does.
--
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
[GitHub] [airflow] dstandish commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810562815
I was inspired by this conversation to make a PR that adds support for arbitrary json in conn uri format: https://github.com/apache/airflow/pull/15100.
We could also add support of the case of an unquoted arbitrary string value by modifying extra_dejson to return the string rather than empty dict in this case (see note at end of the PR description).
It's orthogonal to this PR but closely related topic :)
--
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
[GitHub] [airflow] dstandish edited a comment on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810025108
I see you added an example where extra is not valid json. Why do you want to do that? Why not just use JSON? E.g. if you put fubar in quotes probably it works
Then when you call extra_dejson it should return just the string fubar
**update**
tried this and doesn't work. has to be urlencodable json
```python
c = Connection(extra='"hello"')
c.extra_dejson # works
c.get_uri() # does not
```
--
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
[GitHub] [airflow] mik-laj edited a comment on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
mik-laj edited a comment on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810694589
> Seems like a nice solution would be to allow for any string there.
@avieth. Do you have any use cases that you can't solve with JSON? Object representation is more future-proof because you can always add a new key and tag the old one as deprecaated. It allows for smooth updates.
--
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
[GitHub] [airflow] dstandish edited a comment on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810562815
I was inspired by this conversation to make a PR that adds support for arbitrary json in conn uri format: https://github.com/apache/airflow/pull/15100.
We could also add support of the case of an unquoted arbitrary string value by modifying extra_dejson to return the string rather than empty dict in this case (see note at end of the PR description).
It's orthogonal to this PR but obviously a closely related topic.
> Is there a good reason why extra must be encoded JSON? Seems like a nice solution would be to allow for any string there.
I think that the reason is to provide a reliable interface so that you know that extra params are accessible in a dictionary at property `extra_dejson`.
> However, AFAICT, the constructor will not check this, and will not fail when extra is not JSON. But get_uri does check this (implicitly, by trying to decode then urlencode), and so we have the very weird case in which we can construct a Connection object for which get_uri fails, as @davlum has shown. I'd say that's fairly counterintuitive and surprising.
Yeah probably it makes sense to enforce that `extra` is json in init but this would be breaking. I'll think about supporting the arbitrary and non-json-encoded string in #15100
--
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
[GitHub] [airflow] dstandish edited a comment on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810025108
I see you added an example where extra is not valid json. Why do you want to do that? Why not just use JSON? E.g. if you put fubar in quotes probably it works
Then when you call extra_dejson it should return just the string fubar
update:
tried this and doesn't work. has to be urlencodable json
```python
c = Connection(extra='"hello"')
c.extra_dejson # works
c.get_uri() # does not
```
--
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
[GitHub] [airflow] ashb commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-849569357
@jhtimmins Unfortunately not, as https://github.com/apache/airflow/pull/15425 didn't get finished (or possibly reviewed! Sorry) in time to make it for 2.1
--
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
[GitHub] [airflow] dstandish edited a comment on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-809839608
> Unfortunately the connection URI format is not capable of expressing
all values of the Connection class. In particular the Connection
class allows for arbitrary string values for the extra parameter,
while the URI format requires that this parameter be unnested JSON
so that it can serialize into query parameters.
That's not true. Or at least I don't _think_ that's true.
We have a convenience method `get_uri` you can use to produce the URI.
Try creating your connection as desired with init params including your json `extra`. Then call `get_uri` on the connection.
Use this URI in vault.
If you have a json value for extra that can not be serialized to airflow URI please post it here.
**edit**
it _is_ true. currently uri can only store key-value pairs. PR to add support for this: https://github.com/apache/airflow/pull/15100
--
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
[GitHub] [airflow] dstandish commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810705746
> I think we support both. The specific choice depends on the specific use case. To be able to use the same value as an environment variable and to copy easily to the clipboard, you can use a URI. for more convenient editing, you can use object representation.
No I mean deprecate across airflow including with env vars ( or support both simultaneously )
There's not really anything special about the uri format. We could store json in env vars
--
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
[GitHub] [airflow] davlum edited a comment on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
davlum edited a comment on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810671903
@mik-laj
> We expect the extra field to be a dictionary in Web UI.
This must be as of Airflow 2.0, because this isn't the case in 1.10.x. The additional validation should have been added on the `Connection` object to because it is still perfectly valid to add any string data to the `extra` column. Furthermore anyone migrating from 1.10.x -> 2.0+ will have to manually migrate their data.
--
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
[GitHub] [airflow] ashb commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-902258603
(Code on this looks good now! A nice simple change 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] mik-laj edited a comment on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
mik-laj edited a comment on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810669033
> Is there a good reason why extra must be encoded JSON? Seems like a nice solution would be to allow for any string there.
We expect the extra field to be a dictionary in Web UI. The use of primitive types also causes a problem with extending a given connection ie. it is not future-proff. We also have a design misunderstood here. This field should contain extra **fields**, not additional data.
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r613745380
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -18,8 +18,10 @@
"""Objects relating to sourcing connections & variables from Hashicorp Vault"""
from typing import Optional
+from airflow.models.connection import Connection
from airflow.providers.hashicorp._internal_client.vault_client import _VaultClient # noqa
from airflow.secrets import BaseSecretsBackend
+from airflow.secrets.local_filesystem import _create_connection
Review comment:
So you are saying that if this vault provider expects a `from_dict` method on `Connection` (added in 2.1) the provider won't work with 2.0, but we've promised that it will.
should we perhaps then just merge this as is? or perhaps duplicate this private function into this provider into a vault provider utils module with a deprecation warning?
--
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
[GitHub] [airflow] davlum commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
davlum commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r681881872
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -188,12 +204,28 @@ def get_conn_uri(self, conn_id: str) -> Optional[str]:
:rtype: str
:return: The connection uri retrieved from the secret
"""
- if self.connections_path is None:
+ response = self.get_response(conn_id)
+
+ return response.get("conn_uri") if response else None
+
+ def get_connection(self, conn_id: str) -> Optional[Connection]:
+ """
+ Get connection from Vault as secret. Prioritize conn_uri if exists,
+ if not fall back to normal Connection creation.
+
+ :type conn_id: str
+ :rtype: Connection
+ :return: A Connection object constructed from Vault data
+ """
+ response = self.get_response(conn_id)
+ if response is None:
return None
- else:
- secret_path = self.build_path(self.connections_path, conn_id)
- response = self.vault_client.get_secret(secret_path=secret_path)
- return response.get("conn_uri") if response else None
+
+ uri = response.get("conn_uri")
+ if uri:
+ return uri
Review comment:
Wait so what are the changes I should make?
--
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] mik-laj commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810669033
> Is there a good reason why extra must be encoded JSON? Seems like a nice solution would be to allow for any string there.
We expect the extra field to be a Web UI dictionary. The use of primitive types also causes a problem with extending a given connection ie. it is not future-proff. We also have a design misunderstood here. This field should contain extra **fields**, not additional data.
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r613745380
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -18,8 +18,10 @@
"""Objects relating to sourcing connections & variables from Hashicorp Vault"""
from typing import Optional
+from airflow.models.connection import Connection
from airflow.providers.hashicorp._internal_client.vault_client import _VaultClient # noqa
from airflow.secrets import BaseSecretsBackend
+from airflow.secrets.local_filesystem import _create_connection
Review comment:
So you are saying that if this vault provider expects a `from_dict` method on `Connection` (say, added in 2.1) the provider won't work with 2.0, but we've promised that it will.
should we perhaps then just merge this as is? or perhaps duplicate this private function into this provider into a vault provider utils module with a deprecation warning?
--
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
[GitHub] [airflow] davlum commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
davlum commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r605942070
##########
File path: tests/providers/hashicorp/secrets/test_vault.py
##########
@@ -59,6 +59,48 @@ def test_get_conn_uri(self, mock_hvac):
returned_uri = test_client.get_conn_uri(conn_id="test_postgres")
assert 'postgresql://airflow:airflow@host:5432/airflow' == returned_uri
+ @mock.patch("airflow.providers.hashicorp._internal_client.vault_client.hvac")
+ def test_get_connection(self, mock_hvac):
+ mock_client = mock.MagicMock()
+ mock_hvac.Client.return_value = mock_client
+ mock_client.secrets.kv.v2.read_secret_version.return_value = {
+ 'request_id': '94011e25-f8dc-ec29-221b-1f9c1d9ad2ae',
+ 'lease_id': '',
+ 'renewable': False,
+ 'lease_duration': 0,
+ 'data': {
+ 'data': {
+ 'conn_type': 'postgresql',
+ 'login': 'airflow',
+ 'password': 'airflow',
+ 'host': 'host',
+ 'port': '5432',
+ 'schema': 'airflow',
+ },
Review comment:
I'm going to piggyback off of the validation that happens in `LocalFilesystemBackend` so I don't think that will be possible.
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r605979696
##########
File path: tests/providers/hashicorp/secrets/test_vault.py
##########
@@ -59,6 +59,48 @@ def test_get_conn_uri(self, mock_hvac):
returned_uri = test_client.get_conn_uri(conn_id="test_postgres")
assert 'postgresql://airflow:airflow@host:5432/airflow' == returned_uri
+ @mock.patch("airflow.providers.hashicorp._internal_client.vault_client.hvac")
+ def test_get_connection(self, mock_hvac):
+ mock_client = mock.MagicMock()
+ mock_hvac.Client.return_value = mock_client
+ mock_client.secrets.kv.v2.read_secret_version.return_value = {
+ 'request_id': '94011e25-f8dc-ec29-221b-1f9c1d9ad2ae',
+ 'lease_id': '',
+ 'renewable': False,
+ 'lease_duration': 0,
+ 'data': {
+ 'data': {
+ 'conn_type': 'postgresql',
+ 'login': 'airflow',
+ 'password': 'airflow',
+ 'host': 'host',
+ 'port': '5432',
+ 'schema': 'airflow',
+ },
Review comment:
ah i see or more than validation you are reusing the conn generation logic. i have commented suggesting you promote that bit of code.
meanwhile, don't you think the mock here should include the extra field?
--
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
[GitHub] [airflow] davlum commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
davlum commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r606670638
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -18,8 +18,10 @@
"""Objects relating to sourcing connections & variables from Hashicorp Vault"""
from typing import Optional
+from airflow.models.connection import Connection
from airflow.providers.hashicorp._internal_client.vault_client import _VaultClient # noqa
from airflow.secrets import BaseSecretsBackend
+from airflow.secrets.local_filesystem import _create_connection
Review comment:
It's an additional function on the Connection class so that retains backwards compatibility no?
--
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
[GitHub] [airflow] dstandish edited a comment on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810705746
> I think we support both. The specific choice depends on the specific use case. To be able to use the same value as an environment variable and to copy easily to the clipboard, you can use a URI. for more convenient editing, you can use object representation.
No I mean deprecate across airflow including with env vars and cli ( or support both simultaneously )
There's not really anything special about the uri format. We could store json in env vars
My support of using conn uri is like you said the convenience of switching between backend and env and cli and the virtue of uniformity but if we switch to json globally that could be a good thing
--
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
[GitHub] [airflow] davlum commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
davlum commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810676461
> Can you help with it?
Sure thing, I'll open another PR for that. Curious what the release target will be because it will be a breaking change.
So just to summarize where we're at:
* Support object backend on Vault in this PR?
* Add validation on `extra` column in another PR
--
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
[GitHub] [airflow] dstandish commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810020430
There is a benefit inherent in having only one perfectly good way to do things.
Sometimes when you want to do something another way it's best kept in a private repo for your company or posted on a blog.
I would be in favor of only using the airflow conn uri format. But let's see what others think -- I'm just one voice here so don't be too discouraged.
In the meantime I encourage you to try the uri generation function we have. There is documentation on generating the uri in the managing connections doc.
--
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
[GitHub] [airflow] mik-laj commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810704995
I think we support both. The specific choice depends on the specific use case. To be able to use the same value as an environment variable and to copy easily to the clipboard, you can use a URI. for more convenient editing, you can use object representation.
--
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
[GitHub] [airflow] mik-laj commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r613750962
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -18,8 +18,10 @@
"""Objects relating to sourcing connections & variables from Hashicorp Vault"""
from typing import Optional
+from airflow.models.connection import Connection
from airflow.providers.hashicorp._internal_client.vault_client import _VaultClient # noqa
from airflow.secrets import BaseSecretsBackend
+from airflow.secrets.local_filesystem import _create_connection
Review comment:
Yes. Exactly.
We should discuss the approach on the mailing list and choose the best solution that everyone accepts. I personally think that we should mark this package as only supported by Airflow 2.1, because trying to maintain backward compatibility will limit our development possibilities. It should be normal for users that new packages/library versions may require a core version if we add new features. See: https://lists.apache.org/thread.html/r713f180120d0a39b53567812eb5db34f992ec81979818d2175598b71%40%3Cdev.airflow.apache.org%3E
--
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
[GitHub] [airflow] ashb merged pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #15013:
URL: https://github.com/apache/airflow/pull/15013
--
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] mik-laj commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r615969275
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -18,8 +18,10 @@
"""Objects relating to sourcing connections & variables from Hashicorp Vault"""
from typing import Optional
+from airflow.models.connection import Connection
from airflow.providers.hashicorp._internal_client.vault_client import _VaultClient # noqa
from airflow.secrets import BaseSecretsBackend
+from airflow.secrets.local_filesystem import _create_connection
Review comment:
I have the impression that we have managed to reach a consensus. This change will be in the next provider package after Airflow 2.1 is released. There is a slight problem with the provider packages releases process. We always release providers packages from the main branch, so we cannot merge this PR until we release Airflow 2.1 and we will start releasing providers packages that will be compatible with this version.
In the meantime, we can create a new public method that will create connections from the dictionary. This method will be released in Airflow 2.1. PR: https://github.com/apache/airflow/pull/15425
I added this PR to [the Airflow 2.1](https://github.com/apache/airflow/milestone/8) milestone.
--
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
[GitHub] [airflow] davlum edited a comment on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
davlum edited a comment on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810671903
@mik-laj
> We expect the extra field to be a dictionary in Web UI.
This must be as of Airflow 2.0, because this isn't the case in 1.10.x. The additional validation should have been added on the `Connection` object because it is still perfectly valid to add any string data to the `extra` column. Furthermore anyone migrating from 1.10.x -> 2.0+ will have to manually migrate their data.
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r613745380
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -18,8 +18,10 @@
"""Objects relating to sourcing connections & variables from Hashicorp Vault"""
from typing import Optional
+from airflow.models.connection import Connection
from airflow.providers.hashicorp._internal_client.vault_client import _VaultClient # noqa
from airflow.secrets import BaseSecretsBackend
+from airflow.secrets.local_filesystem import _create_connection
Review comment:
So you are saying that if this vault provider expects a `from_dict` method on `Connection` (say, to be added in 2.1) the provider won't work with 2.0, but we've promised that it will.
should we perhaps then just merge this as is? or perhaps duplicate this private function into a vault provider utils module with a note of some kind to replace with Connection.from_dict when it is available?
--
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
[GitHub] [airflow] mik-laj commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r606649464
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -18,8 +18,10 @@
"""Objects relating to sourcing connections & variables from Hashicorp Vault"""
from typing import Optional
+from airflow.models.connection import Connection
from airflow.providers.hashicorp._internal_client.vault_client import _VaultClient # noqa
from airflow.secrets import BaseSecretsBackend
+from airflow.secrets.local_filesystem import _create_connection
Review comment:
We should extract the common part to the new package. This is problematic now, so I started discussions on the mailing list. See: https://lists.apache.org/thread.html/r713f180120d0a39b53567812eb5db34f992ec81979818d2175598b71%40%3Cdev.airflow.apache.org%3E
--
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
[GitHub] [airflow] mik-laj commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-822577682
> We cannot merge this PR until we release Airflow 2.1 and we will start releasing providers packages that will be compatible with this version.
More info: https://github.com/apache/airflow/pull/15013#discussion_r615969275
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r605388747
##########
File path: tests/providers/hashicorp/secrets/test_vault.py
##########
@@ -59,6 +59,48 @@ def test_get_conn_uri(self, mock_hvac):
returned_uri = test_client.get_conn_uri(conn_id="test_postgres")
assert 'postgresql://airflow:airflow@host:5432/airflow' == returned_uri
+ @mock.patch("airflow.providers.hashicorp._internal_client.vault_client.hvac")
+ def test_get_connection(self, mock_hvac):
+ mock_client = mock.MagicMock()
+ mock_hvac.Client.return_value = mock_client
+ mock_client.secrets.kv.v2.read_secret_version.return_value = {
+ 'request_id': '94011e25-f8dc-ec29-221b-1f9c1d9ad2ae',
+ 'lease_id': '',
+ 'renewable': False,
+ 'lease_duration': 0,
+ 'data': {
+ 'data': {
+ 'conn_type': 'postgresql',
+ 'login': 'airflow',
+ 'password': 'airflow',
+ 'host': 'host',
+ 'port': '5432',
+ 'schema': 'airflow',
+ },
Review comment:
probably should include `extra` since it's a case that should be supported
one thought i had is, when storing creds in the "object" style, should we make it so that you can add values here at top level and they'll automatically get added to extra.
so e.g. we could support this:
```
'data': {
'conn_type': 'postgresql',
'login': 'airflow',
'password': 'airflow',
'host': 'host',
'port': '5432',
'myfield1': 'hello',
'myfield2': 'hello again',
'schema': 'airflow',
},
```
and then when bulding the connection object, add these to the extra dict.
it's maybe a little wacky but it would allow for less nesting. and then when defining the values in vault you wouldn't have to dump to json and store under `extra` as json string. just a thought 🤷♂️
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r605975880
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -188,12 +204,28 @@ def get_conn_uri(self, conn_id: str) -> Optional[str]:
:rtype: str
:return: The connection uri retrieved from the secret
"""
- if self.connections_path is None:
+ response = self.get_response(conn_id)
+
+ return response.get("conn_uri") if response else None
+
+ def get_connection(self, conn_id: str) -> Optional['Connection']:
+ """
+ Get connection from Vault as secret. Prioritize conn_uri if exists,
+ if not fall back to normal Connection creation.
+
+ :type conn_id: str
+ :rtype: Connection
+ :return: A Connection object constructed from Vault data
+ """
+ response = self.get_response(conn_id)
+ if response is None:
return None
- else:
- secret_path = self.build_path(self.connections_path, conn_id)
- response = self.vault_client.get_secret(secret_path=secret_path)
- return response.get("conn_uri") if response else None
+
+ uri = response.get("conn_uri")
+ if uri:
+ return uri
+
+ _create_connection(conn_id, response)
Review comment:
your response to another comment drew my attention here.
i think the leading underscore is meant to signal, don't use me outside of this module
i think what makes sense to do here is to promote this to a standard way of parsing creds from json, so that all such implementations can use it, and so that it is not associated with one particular backend (in this case local files)
i think the right place for this would probably be in connection.py but i could see it going in the root secrets module / package also.
what do you think @mik-laj
--
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
[GitHub] [airflow] avieth edited a comment on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
avieth edited a comment on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810480452
--
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
[GitHub] [airflow] davlum commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
davlum commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r606665776
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -18,8 +18,10 @@
"""Objects relating to sourcing connections & variables from Hashicorp Vault"""
from typing import Optional
+from airflow.models.connection import Connection
from airflow.providers.hashicorp._internal_client.vault_client import _VaultClient # noqa
from airflow.secrets import BaseSecretsBackend
+from airflow.secrets.local_filesystem import _create_connection
Review comment:
In this particular case I can see this function existing in `airflow.models.connections` as `from_dict() -> Connection`. Transforming a dict to the object is the sort of function that usually exists with the class. I agree with the general case that something needs to be done about common functionality between 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish edited a comment on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-809839608
> Unfortunately the connection URI format is not capable of expressing
all values of the Connection class. In particular the Connection
class allows for arbitrary string values for the extra parameter,
while the URI format requires that this parameter be unnested JSON
so that it can serialize into query parameters.
That's not true. Or at least I don't _think_ that's true.
We have a convenience method `get_uri` you can use to produce the URI.
Try creating your connection as desired with init params including your json `extra`. Then call `get_uri` on the connection.
Use this URI in vault.
If you have a json value for extra that can not be serialized to airflow URI please post it here.
--
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
[GitHub] [airflow] nathadfield commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
nathadfield commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810025532
> There is a benefit inherent in having only one perfectly good way to do things.
>
> Sometimes when you want to do something another way it's best kept in a private repo for your company or posted on a blog.
>
> I would be in favor of only using the airflow conn uri format. But let's see what others think -- I'm just one voice here so don't be too discouraged.
>
> In the meantime I encourage you to try the uri generation function we have. There is documentation on generating the uri in the managing connections doc.
@dstandish Sure, I take your point but it is also nice to have options.
For the record, I have tried the `conn_uri` method. Its fine but it does require a certain level of knowledge in order to create one. There are a certain category of users who may only ever interact with the Airflow UI but may also need to update a secret used by a connection.
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r605975880
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -188,12 +204,28 @@ def get_conn_uri(self, conn_id: str) -> Optional[str]:
:rtype: str
:return: The connection uri retrieved from the secret
"""
- if self.connections_path is None:
+ response = self.get_response(conn_id)
+
+ return response.get("conn_uri") if response else None
+
+ def get_connection(self, conn_id: str) -> Optional['Connection']:
+ """
+ Get connection from Vault as secret. Prioritize conn_uri if exists,
+ if not fall back to normal Connection creation.
+
+ :type conn_id: str
+ :rtype: Connection
+ :return: A Connection object constructed from Vault data
+ """
+ response = self.get_response(conn_id)
+ if response is None:
return None
- else:
- secret_path = self.build_path(self.connections_path, conn_id)
- response = self.vault_client.get_secret(secret_path=secret_path)
- return response.get("conn_uri") if response else None
+
+ uri = response.get("conn_uri")
+ if uri:
+ return uri
+
+ _create_connection(conn_id, response)
Review comment:
your response to another comment drew my attention here.
i think the leading underscore is meant to signal, don't use me outside of this module
i think what makes sense to do here is to promote this to a standard way of parsing creds from json, so that all such implementations can use it, and so that it is not associated with one particular backend (in this case local files)
i think the right place for this would probably be in connection.py but i could see it going in the root secrets module / package also.
--
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
[GitHub] [airflow] dstandish edited a comment on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810562815
I was inspired by this conversation to make a PR that adds support for arbitrary json in conn uri format: https://github.com/apache/airflow/pull/15100.
We could also add support of the case of an unquoted arbitrary string value by modifying extra_dejson to return the string rather than empty dict in this case (see note at end of the PR description).
It's orthogonal to this PR but obviously a closely related topic.
> Is there a good reason why extra must be encoded JSON? Seems like a nice solution would be to allow for any string there.
I think that the reason is to provide a reliable interface so that you know that extra params are accessible in a dictionary at property `extra_dejson`.
> However, AFAICT, the constructor will not check this, and will not fail when extra is not JSON. But get_uri does check this (implicitly, by trying to decode then urlencode), and so we have the very weird case in which we can construct a Connection object for which get_uri fails, as @davlum has shown. I'd say that's fairly counterintuitive and surprising.
Yeah probably it makes sense to enforce that `extra` is json in init but this would be breaking.
--
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
[GitHub] [airflow] mik-laj commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r613750962
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -18,8 +18,10 @@
"""Objects relating to sourcing connections & variables from Hashicorp Vault"""
from typing import Optional
+from airflow.models.connection import Connection
from airflow.providers.hashicorp._internal_client.vault_client import _VaultClient # noqa
from airflow.secrets import BaseSecretsBackend
+from airflow.secrets.local_filesystem import _create_connection
Review comment:
Yes. Exactly.
We should discuss the approach on the mailing list and choose the best solution that everyone accepts. I personally think that we should mark this package as only supported by Airflow 2.1, because trying to maintain backward compatibility will limit our development possibilities. It should be normal for users that new packages/library versions may require a core version if we add new features.
--
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
[GitHub] [airflow] davlum edited a comment on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
davlum edited a comment on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810443658
@dstandish If you read the description of PR you can see an example of `get_uri` failing. The fact is that connection URI is a lossy format and is required by Vault integration. As somebody who manages several Airflow instances and had to migrate all the tenants connections into Vault this was a breaking change as users who did not have unnested JSON had their connection broken when calling `.get_uri()` on it.
It was then required to go in and manually change the application code for tenants that used their `extra` parameters to use the appropriate format. This is not a good workflow.
> There is a benefit inherent in having only one perfectly good way to do things.
There does not currently exist one perfectly good way to do things. There are two incompatible ways, and the main interface users use is the `Connection` object. All values of `Connection` cannot be serialized to connection URI.
Just to clarify again, the connection URI format doesn't even handle _all_ valid JSON. It only handles _unnested JSON_.
If you want `get_uri` to be a legitimate format you need to do validation on the `Connection` object to verify that it serializes properly. This would be a breaking change.
--
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
[GitHub] [airflow] davlum commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
davlum commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810443658
@dstandish If you read the description of PR you can see an example of `get_uri` failing. The fact is that connection URI is a lossy format and is required by Vault integration. As somebody who manages several Airflow instances and had to migrate all the tenants connections into Vault this was a breaking change as users who did not have unnested JSON had their connection broken when calling `.get_uri()` on it.
It was then required to go in and manually change the application code for tenants that used their `extra` parameters to use the appropriate format. This is not a good workflow.
--
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
[GitHub] [airflow] mik-laj edited a comment on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
mik-laj edited a comment on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-822577682
**We cannot merge this PR until we release Airflow 2.1 and we will start releasing providers packages that will be compatible with this version.**
More info: https://github.com/apache/airflow/pull/15013#discussion_r615969275
--
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
[GitHub] [airflow] dstandish edited a comment on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-810699071
@mik-laj curious what do you think about deprecating the conn uri format and replacing with JSON, or perhaps allowing both globally? We could implement backward compat with try json parse except conn uri parse
--
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
[GitHub] [airflow] jhtimmins commented on pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#issuecomment-849272199
@mik-laj are we able to merge this now? Or is there something else we're waiting on?
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r613745380
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -18,8 +18,10 @@
"""Objects relating to sourcing connections & variables from Hashicorp Vault"""
from typing import Optional
+from airflow.models.connection import Connection
from airflow.providers.hashicorp._internal_client.vault_client import _VaultClient # noqa
from airflow.secrets import BaseSecretsBackend
+from airflow.secrets.local_filesystem import _create_connection
Review comment:
So you are saying that if this vault provider expects a `from_dict` method on `Connection` (added in 2.1) the provider won't work with 2.0, and but we've promised that it will.
should we perhaps then just merge this as is? or perhaps duplicate this private function into this provider into a vault provider utils module with a deprecation warning?
--
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
[GitHub] [airflow] natanweinberger commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
natanweinberger commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r682064811
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -188,12 +204,28 @@ def get_conn_uri(self, conn_id: str) -> Optional[str]:
:rtype: str
:return: The connection uri retrieved from the secret
"""
- if self.connections_path is None:
+ response = self.get_response(conn_id)
+
+ return response.get("conn_uri") if response else None
+
+ def get_connection(self, conn_id: str) -> Optional[Connection]:
+ """
+ Get connection from Vault as secret. Prioritize conn_uri if exists,
+ if not fall back to normal Connection creation.
+
+ :type conn_id: str
+ :rtype: Connection
+ :return: A Connection object constructed from Vault data
+ """
+ response = self.get_response(conn_id)
+ if response is None:
return None
- else:
- secret_path = self.build_path(self.connections_path, conn_id)
- response = self.vault_client.get_secret(secret_path=secret_path)
- return response.get("conn_uri") if response else None
+
+ uri = response.get("conn_uri")
+ if uri:
+ return uri
Review comment:
I was raising two suggestions:
- the Connection constructor can replace the `_create_connection` function for this use case
- in one case, this function returns a Connection object; in another it returns a uri. I think it should return a Connection both times?
--
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] natanweinberger commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
natanweinberger commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r656407964
##########
File path: airflow/providers/hashicorp/secrets/vault.py
##########
@@ -188,12 +204,28 @@ def get_conn_uri(self, conn_id: str) -> Optional[str]:
:rtype: str
:return: The connection uri retrieved from the secret
"""
- if self.connections_path is None:
+ response = self.get_response(conn_id)
+
+ return response.get("conn_uri") if response else None
+
+ def get_connection(self, conn_id: str) -> Optional[Connection]:
+ """
+ Get connection from Vault as secret. Prioritize conn_uri if exists,
+ if not fall back to normal Connection creation.
+
+ :type conn_id: str
+ :rtype: Connection
+ :return: A Connection object constructed from Vault data
+ """
+ response = self.get_response(conn_id)
+ if response is None:
return None
- else:
- secret_path = self.build_path(self.connections_path, conn_id)
- response = self.vault_client.get_secret(secret_path=secret_path)
- return response.get("conn_uri") if response else None
+
+ uri = response.get("conn_uri")
+ if uri:
+ return uri
Review comment:
@ashb Based on the changes we made to the constructor for `Connection`, I don't see any reason not to use it here too.
That said, note that this function won't persist the connection to the DB (but neither would `_create_connection`, so no difference really).
--
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
[GitHub] [airflow] ashb commented on a change in pull request #15013: Enable Connection creation from Vault parameters
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r692496427
##########
File path: docs/apache-airflow-providers-hashicorp/secrets-backends/hashicorp-vault.rst
##########
@@ -100,6 +100,46 @@ Verify that you can get the secret from ``vault``:
The value of the Vault key must be the :ref:`connection URI representation <generating_connection_uri>`
of the connection object to get connection.
+Storing and Retrieving Connections using Connection class representation
+""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""
+
+If you have set ``connections_path`` as ``connections`` and ``mount_point`` as ``airflow``, then for a connection id of
+``smtp_default``, you would want to store your secret as:
+
+.. code-block:: bash
+
+ vault kv put airflow/connections/smtp_default conn_type=smtps login=user password=host host=relay.example.com port=465
+
+Note that the ``Keys`` are parameters of the ``Connection`` class and the ``Value`` their argument.
+
+You can make a ``mount_point`` for ``airflow`` as follows:
+
+.. code-block:: bash
+
+ vault secrets enable -path=airflow -version=2 kv
Review comment:
Do we need to say this again? Maybe it should be moved to a "setup" section in this doc instead
--
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