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/31 09:44:32 UTC
[GitHub] [airflow] JavierLopezT opened a new pull request #15104: Get connection URI with AWS Secrets Manager
JavierLopezT opened a new pull request #15104:
URL: https://github.com/apache/airflow/pull/15104
One of the main advantages of using AWS Secrets Manager is its ability to automatically create secrets of RDS databases and Redshift databases. Those secrets consist of several keys with their values, i.e user, pass, etc. Also, it is normal to store API Keys, sftp, or whatever using different values, as shown in the picture below:
With the current code, all the keys and values obtained from a secret are stored in the schema attribute of the conn object, unless you have just one key with the conn_uri in the value. Thus, the current situation is forcing to use Secrets Manager in a way it is not intended to.
With this proposed modification, you can use AWS Secrets Manager using keys and values and have some kind of freedom to choose different words for each key to make the get_conn work.
I need help and guidance with tests, please. Thanks
--
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] xinbinhuang commented on pull request #15104: Get connection URI with AWS Secrets Manager
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on pull request #15104:
URL: https://github.com/apache/airflow/pull/15104#issuecomment-812374673
I feel like this PR is also related to https://github.com/apache/airflow/pull/15146, https://github.com/apache/airflow/pull/15146, and https://github.com/apache/airflow/pull/15100 . cc: @dstandish @davlum
--
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] xinbinhuang commented on a change in pull request #15104: Get connection URI with AWS Secrets Manager
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #15104:
URL: https://github.com/apache/airflow/pull/15104#discussion_r606105285
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -102,17 +102,66 @@ def client(self):
)
return session.client(service_name="secretsmanager", **self.kwargs)
- def get_conn_uri(self, conn_id: str) -> Optional[str]:
+ def _get_extra(self, secret, conn_string):
+ if 'extra' in secret:
+ extra_dict = ast.literal_eval(secret['extra'])
+ counter = 0
+ for key, value in extra_dict.items():
+ if counter == 0:
+ conn_string += f'?{key}={value}'
+ else:
+ conn_string += f'&{key}={value}'
+
+ counter += 1
+
+ return conn_string
+
+ def get_conn_uri(self, conn_id: str):
"""
Get Connection Value
:param conn_id: connection id
:type conn_id: str
"""
- if self.connections_prefix is None:
- return None
+ if self.connections_prefix and self.sep:
+ conn_id = self.build_path(self.connections_prefix, conn_id, self.sep)
- return self._get_secret(self.connections_prefix, conn_id)
+ try:
+ secret_string = self._get_secret(conn_id)
+ secret = ast.literal_eval(secret_string) # json.loads gives error
+ except ValueError: # 'malformed node or string: ' error, for empty conns
+ connection = None
+ secret = None
+
+ # These lines will check if we have with some denomination stored an username, password and host
+ if secret:
+ possible_words_for_conn_fields = {
+ 'user': ['user', 'username', 'login', 'user_name'],
+ 'password': ['password', 'pass', 'key'],
+ 'host': ['host', 'remote_host', 'server'],
+ 'port': ['port'],
+ 'schema': ['database', 'schema'],
+ 'conn_type': ['conn_type', 'conn_id', 'connection_type', 'engine']
+ }
+
+ conn_d = {}
+ for conn_field, possible_words in possible_words_for_conn_fields.items():
+ try:
+ conn_d[conn_field] = [v for k, v in secret.items() if k in possible_words][0]
+ except IndexError:
+ conn_d[conn_field] = ''
+
+ conn_type = conn_d['conn_type']
+ user = conn_d['user']
+ password = conn_d['password']
+ host = conn_d['host']
+ port = conn_d['port']
+ schema = conn_d['schema']
+ conn_string = f'{conn_type}://{user}:{password}@{host}:{port}/{schema}'
+
+ connection = self._get_extra(secret, conn_string)
Review comment:
Also, I would recommend separating this part of logic (`if secret: `) into another method for easier testing
--
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] github-actions[bot] closed pull request #15104: Get connection URI with AWS Secrets Manager
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #15104:
URL: https://github.com/apache/airflow/pull/15104
--
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] xinbinhuang commented on a change in pull request #15104: Get connection URI with AWS Secrets Manager
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #15104:
URL: https://github.com/apache/airflow/pull/15104#discussion_r606090823
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -102,17 +102,66 @@ def client(self):
)
return session.client(service_name="secretsmanager", **self.kwargs)
- def get_conn_uri(self, conn_id: str) -> Optional[str]:
+ def _get_extra(self, secret, conn_string):
+ if 'extra' in secret:
+ extra_dict = ast.literal_eval(secret['extra'])
+ counter = 0
+ for key, value in extra_dict.items():
+ if counter == 0:
+ conn_string += f'?{key}={value}'
+ else:
+ conn_string += f'&{key}={value}'
+
+ counter += 1
+
+ return conn_string
+
+ def get_conn_uri(self, conn_id: str):
"""
Get Connection Value
:param conn_id: connection id
:type conn_id: str
"""
- if self.connections_prefix is None:
- return None
+ if self.connections_prefix and self.sep:
+ conn_id = self.build_path(self.connections_prefix, conn_id, self.sep)
- return self._get_secret(self.connections_prefix, conn_id)
+ try:
+ secret_string = self._get_secret(conn_id)
+ secret = ast.literal_eval(secret_string) # json.loads gives error
+ except ValueError: # 'malformed node or string: ' error, for empty conns
+ connection = None
+ secret = None
+
+ # These lines will check if we have with some denomination stored an username, password and host
+ if secret:
+ possible_words_for_conn_fields = {
+ 'user': ['user', 'username', 'login', 'user_name'],
+ 'password': ['password', 'pass', 'key'],
+ 'host': ['host', 'remote_host', 'server'],
+ 'port': ['port'],
+ 'schema': ['database', 'schema'],
+ 'conn_type': ['conn_type', 'conn_id', 'connection_type', 'engine']
+ }
+
+ conn_d = {}
+ for conn_field, possible_words in possible_words_for_conn_fields.items():
+ try:
+ conn_d[conn_field] = [v for k, v in secret.items() if k in possible_words][0]
+ except IndexError:
+ conn_d[conn_field] = ''
+
+ conn_type = conn_d['conn_type']
+ user = conn_d['user']
+ password = conn_d['password']
+ host = conn_d['host']
+ port = conn_d['port']
+ schema = conn_d['schema']
+ conn_string = f'{conn_type}://{user}:{password}@{host}:{port}/{schema}'
+
+ connection = self._get_extra(secret, conn_string)
Review comment:
```suggestion
from airflow.models import Connection
connection_uri = Connection(**conn_d, extra=extra).get_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] xinbinhuang commented on a change in pull request #15104: Get connection URI with AWS Secrets Manager
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #15104:
URL: https://github.com/apache/airflow/pull/15104#discussion_r606089064
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -102,17 +102,66 @@ def client(self):
)
return session.client(service_name="secretsmanager", **self.kwargs)
- def get_conn_uri(self, conn_id: str) -> Optional[str]:
+ def _get_extra(self, secret, conn_string):
+ if 'extra' in secret:
+ extra_dict = ast.literal_eval(secret['extra'])
+ counter = 0
+ for key, value in extra_dict.items():
+ if counter == 0:
+ conn_string += f'?{key}={value}'
+ else:
+ conn_string += f'&{key}={value}'
+
+ counter += 1
+
+ return conn_string
+
+ def get_conn_uri(self, conn_id: str):
"""
Get Connection Value
:param conn_id: connection id
:type conn_id: str
"""
- if self.connections_prefix is None:
- return None
+ if self.connections_prefix and self.sep:
+ conn_id = self.build_path(self.connections_prefix, conn_id, self.sep)
- return self._get_secret(self.connections_prefix, conn_id)
+ try:
+ secret_string = self._get_secret(conn_id)
+ secret = ast.literal_eval(secret_string) # json.loads gives error
+ except ValueError: # 'malformed node or string: ' error, for empty conns
+ connection = None
+ secret = None
Review comment:
The signature for is `_get_secret(self, path_prefix: str, secret_id: str)`.
https://github.com/apache/airflow/blob/c651ab71d911e99d4376722611b12bc5be6eeae4/airflow/providers/amazon/aws/secrets/secrets_manager.py#L190-L200
--
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] xinbinhuang commented on a change in pull request #15104: Get connection URI with AWS Secrets Manager
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #15104:
URL: https://github.com/apache/airflow/pull/15104#discussion_r606089064
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -102,17 +102,66 @@ def client(self):
)
return session.client(service_name="secretsmanager", **self.kwargs)
- def get_conn_uri(self, conn_id: str) -> Optional[str]:
+ def _get_extra(self, secret, conn_string):
+ if 'extra' in secret:
+ extra_dict = ast.literal_eval(secret['extra'])
+ counter = 0
+ for key, value in extra_dict.items():
+ if counter == 0:
+ conn_string += f'?{key}={value}'
+ else:
+ conn_string += f'&{key}={value}'
+
+ counter += 1
+
+ return conn_string
+
+ def get_conn_uri(self, conn_id: str):
"""
Get Connection Value
:param conn_id: connection id
:type conn_id: str
"""
- if self.connections_prefix is None:
- return None
+ if self.connections_prefix and self.sep:
+ conn_id = self.build_path(self.connections_prefix, conn_id, self.sep)
- return self._get_secret(self.connections_prefix, conn_id)
+ try:
+ secret_string = self._get_secret(conn_id)
+ secret = ast.literal_eval(secret_string) # json.loads gives error
+ except ValueError: # 'malformed node or string: ' error, for empty conns
+ connection = None
+ secret = None
Review comment:
Another note, the signature for is `_get_secret(self, path_prefix: str, secret_id: str)`.
https://github.com/apache/airflow/blob/c651ab71d911e99d4376722611b12bc5be6eeae4/airflow/providers/amazon/aws/secrets/secrets_manager.py#L190-L200
--
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] github-actions[bot] commented on pull request #15104: Get connection URI with AWS Secrets Manager
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15104:
URL: https://github.com/apache/airflow/pull/15104#issuecomment-811839483
[The Workflow run](https://github.com/apache/airflow/actions/runs/708319537) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
--
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 #15104: Get connection URI with AWS Secrets Manager
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #15104:
URL: https://github.com/apache/airflow/pull/15104#issuecomment-812596462
may i suggest using `_create_connection` function here
https://github.com/apache/airflow/pull/15013/files#diff-258df8f49a97540963b45ae4ac0e0f71720101134f73b2981b8ab3dd1a4cf5fcR24
i think we ultimately ought to provide a shared way of parsing creds from json and this function could be a good starting 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] xinbinhuang commented on a change in pull request #15104: Get connection URI with AWS Secrets Manager
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #15104:
URL: https://github.com/apache/airflow/pull/15104#discussion_r606082085
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -102,17 +102,66 @@ def client(self):
)
return session.client(service_name="secretsmanager", **self.kwargs)
- def get_conn_uri(self, conn_id: str) -> Optional[str]:
+ def _get_extra(self, secret, conn_string):
+ if 'extra' in secret:
+ extra_dict = ast.literal_eval(secret['extra'])
+ counter = 0
+ for key, value in extra_dict.items():
+ if counter == 0:
+ conn_string += f'?{key}={value}'
+ else:
+ conn_string += f'&{key}={value}'
+
+ counter += 1
+
+ return conn_string
Review comment:
```suggestion
kvs = "&".join([f"{key}={value} for key, value in extra_dict.items()])
return f"{conn_string}?{kvs}"
```
--
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] github-actions[bot] commented on pull request #15104: Get connection URI with AWS Secrets Manager
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15104:
URL: https://github.com/apache/airflow/pull/15104#issuecomment-842724438
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish edited a comment on pull request #15104: Get connection URI with AWS Secrets Manager
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #15104:
URL: https://github.com/apache/airflow/pull/15104#issuecomment-812596462
may i suggest using `_create_connection` function here
https://github.com/apache/airflow/pull/15013/files#diff-258df8f49a97540963b45ae4ac0e0f71720101134f73b2981b8ab3dd1a4cf5fcR24
i think we ultimately ought to provide a shared way of parsing creds from json and this function could be a good starting point.
PR #15013 is doing the same thing for Vault and is re-using that function.
--
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] xinbinhuang commented on a change in pull request #15104: Get connection URI with AWS Secrets Manager
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #15104:
URL: https://github.com/apache/airflow/pull/15104#discussion_r606088382
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -102,17 +102,66 @@ def client(self):
)
return session.client(service_name="secretsmanager", **self.kwargs)
- def get_conn_uri(self, conn_id: str) -> Optional[str]:
+ def _get_extra(self, secret, conn_string):
+ if 'extra' in secret:
+ extra_dict = ast.literal_eval(secret['extra'])
+ counter = 0
+ for key, value in extra_dict.items():
+ if counter == 0:
+ conn_string += f'?{key}={value}'
+ else:
+ conn_string += f'&{key}={value}'
+
+ counter += 1
+
+ return conn_string
+
+ def get_conn_uri(self, conn_id: str):
"""
Get Connection Value
:param conn_id: connection id
:type conn_id: str
"""
- if self.connections_prefix is None:
- return None
+ if self.connections_prefix and self.sep:
+ conn_id = self.build_path(self.connections_prefix, conn_id, self.sep)
- return self._get_secret(self.connections_prefix, conn_id)
+ try:
+ secret_string = self._get_secret(conn_id)
+ secret = ast.literal_eval(secret_string) # json.loads gives error
+ except ValueError: # 'malformed node or string: ' error, for empty conns
+ connection = None
+ secret = None
Review comment:
I am not convinced that we need `ast.literal_eval` here. Can you explain a bit more on why we need this?
--
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] xinbinhuang commented on a change in pull request #15104: Get connection URI with AWS Secrets Manager
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #15104:
URL: https://github.com/apache/airflow/pull/15104#discussion_r606089064
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -102,17 +102,66 @@ def client(self):
)
return session.client(service_name="secretsmanager", **self.kwargs)
- def get_conn_uri(self, conn_id: str) -> Optional[str]:
+ def _get_extra(self, secret, conn_string):
+ if 'extra' in secret:
+ extra_dict = ast.literal_eval(secret['extra'])
+ counter = 0
+ for key, value in extra_dict.items():
+ if counter == 0:
+ conn_string += f'?{key}={value}'
+ else:
+ conn_string += f'&{key}={value}'
+
+ counter += 1
+
+ return conn_string
+
+ def get_conn_uri(self, conn_id: str):
"""
Get Connection Value
:param conn_id: connection id
:type conn_id: str
"""
- if self.connections_prefix is None:
- return None
+ if self.connections_prefix and self.sep:
+ conn_id = self.build_path(self.connections_prefix, conn_id, self.sep)
- return self._get_secret(self.connections_prefix, conn_id)
+ try:
+ secret_string = self._get_secret(conn_id)
+ secret = ast.literal_eval(secret_string) # json.loads gives error
+ except ValueError: # 'malformed node or string: ' error, for empty conns
+ connection = None
+ secret = None
Review comment:
Another note, he signature for is `_get_secret(self, path_prefix: str, secret_id: str)`.
https://github.com/apache/airflow/blob/c651ab71d911e99d4376722611b12bc5be6eeae4/airflow/providers/amazon/aws/secrets/secrets_manager.py#L190-L200
--
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