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 2020/05/27 03:53:07 UTC
[GitHub] [airflow] JavierLopezT opened a new pull request #9008: Get connections uri with AWS Secrets Manager backend
JavierLopezT opened a new pull request #9008:
URL: https://github.com/apache/airflow/pull/9008
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.
---
Make sure to mark the boxes below before creating PR: [x]
- [x] Description above provides context of the change
- [x] Unit tests coverage for changes (not needed for documentation changes)
- [x] Target Github ISSUE in description if exists
- [x] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [x] Relevant documentation is updated including usage instructions.
- [x] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
----------------------------------------------------------------
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 a change in pull request #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#discussion_r455691248
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -83,43 +90,121 @@ def client(self):
)
return session.client(service_name="secretsmanager", **self.kwargs)
- def get_conn_uri(self, conn_id: str) -> Optional[str]:
+ def _get_user_and_password(self, secret):
+ for user_denomination in ['user', 'username', 'login']:
+ if user_denomination in secret:
+ user = secret[user_denomination]
+
+ for password_denomination in ['pass', 'password', 'key']:
+ if password_denomination in secret:
+ password = secret[password_denomination]
+
+ return user, password
+
+ def _get_host(self, secret):
+ for host_denomination in ['host', 'remote_host', 'server']:
+ if host_denomination in secret:
+ host = secret[host_denomination]
+
+ return host
+
+ def _get_conn_type(self, secret):
+ for type_word in ['conn_type', 'conn_id', 'connection_type', 'engine']:
+ if type_word in secret:
+ conn_type = secret[type_word]
+ conn_type = 'postgresql' if conn_type == 'redshift' else conn_type
+ else:
+ conn_type = 'connection'
+ return conn_type
+
+ def _get_port_and_database(self, secret):
+ if 'port' in secret:
+ port = secret['port']
+ else:
+ port = 5432
Review comment:
Let's not hardcode
----------------------------------------------------------------
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 #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#discussion_r455686570
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -60,15 +59,23 @@ class SecretsManagerBackend(BaseSecretsBackend, LoggingMixin):
def __init__(
self,
- connections_prefix: str = 'airflow/connections',
- variables_prefix: str = 'airflow/variables',
- profile_name: Optional[str] = None,
- sep: str = "/",
+ connections_prefix=None,
Review comment:
Why do these need to be None? this controls where in the secrets hierarchy to look at, so should be a required field, 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] kaxil commented on a change in pull request #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#discussion_r455690869
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -60,15 +59,23 @@ class SecretsManagerBackend(BaseSecretsBackend, LoggingMixin):
def __init__(
self,
- connections_prefix: str = 'airflow/connections',
- variables_prefix: str = 'airflow/variables',
- profile_name: Optional[str] = None,
- sep: str = "/",
+ connections_prefix=None,
+ variables_prefix=None,
Review comment:
It also breaks backwards-compatibility
----------------------------------------------------------------
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] JavierLopezT commented on a change in pull request #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
JavierLopezT commented on a change in pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#discussion_r466333453
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -60,15 +59,23 @@ class SecretsManagerBackend(BaseSecretsBackend, LoggingMixin):
def __init__(
self,
- connections_prefix: str = 'airflow/connections',
- variables_prefix: str = 'airflow/variables',
- profile_name: Optional[str] = None,
- sep: str = "/",
+ connections_prefix=None,
+ variables_prefix=None,
Review comment:
Anyway, I don't see that these should be required arguments. In my current company, we share an AWS account with other departments. As so, we have to start our secrets with the prefix 'data_', hence in my current situation it does make sense to have `connections_prefix='data'`. However, in my previous company, we as BI team had an AWS account for ourselves, and thus we had the secrets just like _'db_redshift'_, _'api_salesforce'_ and so on, so the None value made more sense.
Furthermore, I would never expect to have the default variables as airflow/variables, because I won't ever think at all using the '/' sep in this case.
I know that what I think or believe is pointless, but I am wondering if there are more people with similar reasonings as me that the default values don't fit with their project.
----------------------------------------------------------------
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] JavierLopezT commented on a change in pull request #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
JavierLopezT commented on a change in pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#discussion_r466321060
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -60,15 +59,23 @@ class SecretsManagerBackend(BaseSecretsBackend, LoggingMixin):
def __init__(
self,
- connections_prefix: str = 'airflow/connections',
- variables_prefix: str = 'airflow/variables',
- profile_name: Optional[str] = None,
- sep: str = "/",
+ connections_prefix=None,
Review comment:
That was because I was not able to make it works using connections prefix, so I ended up ignoring them. In my set up those values are None and I just type the entire name of the secret.
----------------------------------------------------------------
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] JavierLopezT commented on pull request #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
JavierLopezT commented on pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#issuecomment-691984621
The other day I took a look at this code again, and indeed the code is ugly as f**k.
@ashb I re-read your comment and I will try to narrow the scope of my question regarding your comment. When you say _the existing conn_uri approach_ are you referring to the way in this script, or something else?
From your second bullet, I understand that you say for example (using the picture of my previous comment), that if I call the secret data_snowflake, since it won't be found, all data_airflow_master_user, data_api_google_drive and data_airflow_backend_database would be fetched?
I have been thinking also about what you said of extra parameters. It sounds good. But, what happens if you want to save something that it is not used in the connection but somewhere else? Then the conn will take those fields in the extra and the conn will fail. I came up with adding some suffix or prefix to those fields, like no_whatever or whatever_dont_use, but in some manner that sounds counterintuitive for me as well. So I am not sure which would be a good solution. What do you think?
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] ashb edited a comment on pull request #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
ashb edited a comment on pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#issuecomment-659324861
The way this is implemented is far to specialized, and hacky.
They way I would expect this to work:
- We first try to get the named secret -- if that finds a valid secret then we use the existing conn_uri approach; otherwise
- We list secrets under the named path (i.e. `List("/airflow/connections/my_conn_id")` and then we fetch each of those secrets, feeding them in to a `Connection()` call in some manner.
This way we an support arbitrary extra parameters, i.e. the all the extra params for a Connection too, and we don't ever need to use `ast`.
Probably something like: if the name is not one of conn_type, host, login, password, schema, or port, extra then it gets fed in to the extra dict. (That way you can specify extra fields individually in Secrets Manager without having to manage a JSON value.)
----------------------------------------------------------------
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 a change in pull request #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#discussion_r432544185
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -83,43 +86,87 @@ def client(self):
)
return session.client(service_name="secretsmanager", **self.kwargs)
- def get_conn_uri(self, conn_id: str) -> Optional[str]:
+ def get_conn_uri(self, conn_id: str):
"""
Get Connection Value
:param conn_id: connection id
:type conn_id: str
"""
- return self._get_secret(self.connections_prefix, conn_id)
+ if self.connections_prefix and self.sep:
+ conn_id = self.build_path(self.connections_prefix, conn_id, self.sep)
- def get_variable(self, key: str) -> Optional[str]:
+ try:
+ secret_string = self._get_secret(conn_id)
+ secret = ast.literal_eval(secret_string)
+ except ValueError: # 'malformed node or string: ' error, for empty conns
+ connection = None
+
+ # These lines will check if we have with some denomination stored an username, password and host
+ if secret:
+ for user_denomination in ['user', 'username', 'login']:
Review comment:
yes avoid the nested blocks if possible, if it is unavoidable then just add `# pylint: disable=too-many-nested-blocks` at the end of the line where it complains.
----------------------------------------------------------------
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] JavierLTPromofarma commented on pull request #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
JavierLTPromofarma commented on pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#issuecomment-635939220
I get the error `Too many nested blocks`, any suggestions to refactor the code @kaxil or @someonelese?
Thank you very much
----------------------------------------------------------------
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 a change in pull request #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#discussion_r455698455
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -60,15 +59,23 @@ class SecretsManagerBackend(BaseSecretsBackend, LoggingMixin):
def __init__(
self,
- connections_prefix: str = 'airflow/connections',
- variables_prefix: str = 'airflow/variables',
- profile_name: Optional[str] = None,
- sep: str = "/",
+ connections_prefix=None,
+ variables_prefix=None,
Review comment:
it does. If someone passes nothing, the default it used to take is `airflow/variables`
----------------------------------------------------------------
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] JavierLopezT commented on a change in pull request #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
JavierLopezT commented on a change in pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#discussion_r466333453
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -60,15 +59,23 @@ class SecretsManagerBackend(BaseSecretsBackend, LoggingMixin):
def __init__(
self,
- connections_prefix: str = 'airflow/connections',
- variables_prefix: str = 'airflow/variables',
- profile_name: Optional[str] = None,
- sep: str = "/",
+ connections_prefix=None,
+ variables_prefix=None,
Review comment:
Anyway, I don't see that these should be required arguments. In my current company, we share an AWS account with other departments. As so, we have to start our secrets with the prefix 'data_', hence in my current situation it does make sense to have `connections_prefix='data'`. However, in my previous company, we as BI team had an AWS account for ourselves, and thus we had the secrets just like _'db_redshift'_, _'api_salesforce'_ and so on, so the None value made more sense.
Furthermore, I would never expect to have the default variables as airflow/variables, because I won't ever think at all using the '/' sep in this case.
I know that what I think or believe doesn't matter at all, but I am just wondering if there are more people with similar reasonings as me that the default values don't fit with their project.
Also, just curiosity (not saying that this is a case in which should be applied), is there any procedure to implement a change that breaks previous workflows?
----------------------------------------------------------------
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 #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #9008:
URL: https://github.com/apache/airflow/pull/9008
--
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 a change in pull request #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#discussion_r455691248
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -83,43 +90,121 @@ def client(self):
)
return session.client(service_name="secretsmanager", **self.kwargs)
- def get_conn_uri(self, conn_id: str) -> Optional[str]:
+ def _get_user_and_password(self, secret):
+ for user_denomination in ['user', 'username', 'login']:
+ if user_denomination in secret:
+ user = secret[user_denomination]
+
+ for password_denomination in ['pass', 'password', 'key']:
+ if password_denomination in secret:
+ password = secret[password_denomination]
+
+ return user, password
+
+ def _get_host(self, secret):
+ for host_denomination in ['host', 'remote_host', 'server']:
+ if host_denomination in secret:
+ host = secret[host_denomination]
+
+ return host
+
+ def _get_conn_type(self, secret):
+ for type_word in ['conn_type', 'conn_id', 'connection_type', 'engine']:
+ if type_word in secret:
+ conn_type = secret[type_word]
+ conn_type = 'postgresql' if conn_type == 'redshift' else conn_type
+ else:
+ conn_type = 'connection'
+ return conn_type
+
+ def _get_port_and_database(self, secret):
+ if 'port' in secret:
+ port = secret['port']
+ else:
+ port = 5432
Review comment:
Let's not hardcode or keep any default 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] mik-laj commented on a change in pull request #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#discussion_r455697323
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -60,15 +59,23 @@ class SecretsManagerBackend(BaseSecretsBackend, LoggingMixin):
def __init__(
self,
- connections_prefix: str = 'airflow/connections',
- variables_prefix: str = 'airflow/variables',
- profile_name: Optional[str] = None,
- sep: str = "/",
+ connections_prefix=None,
+ variables_prefix=None,
Review comment:
No. It's doesn't break, because we always use keyword arguments.
https://airflow.readthedocs.io/en/latest/configurations-ref.html#backend-kwargs
----------------------------------------------------------------
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 #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#issuecomment-802413187
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] mik-laj commented on a change in pull request #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#discussion_r432514532
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -83,43 +86,87 @@ def client(self):
)
return session.client(service_name="secretsmanager", **self.kwargs)
- def get_conn_uri(self, conn_id: str) -> Optional[str]:
+ def get_conn_uri(self, conn_id: str):
"""
Get Connection Value
:param conn_id: connection id
:type conn_id: str
"""
- return self._get_secret(self.connections_prefix, conn_id)
+ if self.connections_prefix and self.sep:
+ conn_id = self.build_path(self.connections_prefix, conn_id, self.sep)
- def get_variable(self, key: str) -> Optional[str]:
+ try:
+ secret_string = self._get_secret(conn_id)
+ secret = ast.literal_eval(secret_string)
+ except ValueError: # 'malformed node or string: ' error, for empty conns
+ connection = None
+
+ # These lines will check if we have with some denomination stored an username, password and host
+ if secret:
+ for user_denomination in ['user', 'username', 'login']:
Review comment:
Can you extract this to a new method?
----------------------------------------------------------------
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] JavierLopezT commented on pull request #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
JavierLopezT commented on pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#issuecomment-635432873
> Please add tests
Sure, but what kind of tests? I assumed that since there are already tests, if they work with the new script it'll be fine.
----------------------------------------------------------------
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 #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#issuecomment-659322116
> > Please add tests
>
> Sure, but what kind of tests? I assumed that since there are already tests, if they work with the new script it'll be fine.
Tests that check that you get set `conn_id`, `conn_type` etc and that you can still get that Connection back in here: https://github.com/apache/airflow/blob/master/tests/providers/amazon/aws/secrets/test_secrets_manager.py
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#discussion_r466649936
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -60,15 +59,23 @@ class SecretsManagerBackend(BaseSecretsBackend, LoggingMixin):
def __init__(
self,
- connections_prefix: str = 'airflow/connections',
- variables_prefix: str = 'airflow/variables',
- profile_name: Optional[str] = None,
- sep: str = "/",
+ connections_prefix=None,
Review comment:
The following should work in your case (can you try):
```ini
[secrets]
backend = airflow.contrib.secrets.aws_secrets_manager.SecretsManagerBackend
backend_kwargs = {"connections_prefix": "", "sep": ""}
```
----------------------------------------------------------------
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 #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#issuecomment-659324861
The way this is implemented is far to specialized, and hacky.
They way I would expect this to work:
- We first try to get the named secret -- if that finds a valid secret then we use the existing conn_uri approach; otherwise
- We list secrets under the named path (i.e. `List("/airflow/connections/my_conn_id")` and then we fetch each of those secrets, feeding them in to a `Connection()` call in some manner.
This way we an support arbitrary extra parameters, i.e. the all the extra params for a Connection too, and we don't ever need to use `ast`.
----------------------------------------------------------------
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] andres-lowrie commented on a change in pull request #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
andres-lowrie commented on a change in pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#discussion_r539816366
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -83,43 +90,121 @@ def client(self):
)
return session.client(service_name="secretsmanager", **self.kwargs)
- def get_conn_uri(self, conn_id: str) -> Optional[str]:
+ def _get_user_and_password(self, secret):
+ for user_denomination in ['user', 'username', 'login']:
+ if user_denomination in secret:
+ user = secret[user_denomination]
+
+ for password_denomination in ['pass', 'password', 'key']:
+ if password_denomination in secret:
+ password = secret[password_denomination]
+
+ return user, password
+
+ def _get_host(self, secret):
+ for host_denomination in ['host', 'remote_host', 'server']:
+ if host_denomination in secret:
+ host = secret[host_denomination]
+
+ return host
+
+ def _get_conn_type(self, secret):
+ for type_word in ['conn_type', 'conn_id', 'connection_type', 'engine']:
+ if type_word in secret:
+ conn_type = secret[type_word]
+ conn_type = 'postgresql' if conn_type == 'redshift' else conn_type
+ else:
+ conn_type = 'connection'
+ return conn_type
+
+ def _get_port_and_database(self, secret):
+ if 'port' in secret:
+ port = secret['port']
+ else:
+ port = 5432
+ if 'database' in secret:
+ database = secret['database']
+ else:
+ database = ''
+
+ return port, database
+
+ 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.values():
+ if counter == 0:
+ conn_string += f'{key}={value}'
+ else:
+ conn_string += f'&{key}={value}'
Review comment:
hello,
I'm wondering if this approach would work for nested extra values.
For example the `emr_default` one for aws looks like this:
https://github.com/apache/airflow/blob/c704293f75475671f11e240094fee36736adb1d9/airflow/utils/db.py#L205
I think you mentioned something about using key tokens like `dont_use_*` or something like that in order to have a way to ignore stuff; I'm just making a comment to make it visible
----------------------------------------------------------------
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 #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#issuecomment-659321669
Love the feature! (And I had this in mind when reviewing the original 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] JavierLopezT commented on pull request #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
JavierLopezT commented on pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#issuecomment-720303600
Commenting so that it doesn't get closed
----------------------------------------------------------------
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 a change in pull request #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#discussion_r455691530
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -83,43 +90,121 @@ def client(self):
)
return session.client(service_name="secretsmanager", **self.kwargs)
- def get_conn_uri(self, conn_id: str) -> Optional[str]:
+ def _get_user_and_password(self, secret):
+ for user_denomination in ['user', 'username', 'login']:
+ if user_denomination in secret:
+ user = secret[user_denomination]
+
+ for password_denomination in ['pass', 'password', 'key']:
+ if password_denomination in secret:
+ password = secret[password_denomination]
+
+ return user, password
+
+ def _get_host(self, secret):
+ for host_denomination in ['host', 'remote_host', 'server']:
+ if host_denomination in secret:
+ host = secret[host_denomination]
+
+ return host
+
+ def _get_conn_type(self, secret):
+ for type_word in ['conn_type', 'conn_id', 'connection_type', 'engine']:
+ if type_word in secret:
+ conn_type = secret[type_word]
+ conn_type = 'postgresql' if conn_type == 'redshift' else conn_type
+ else:
+ conn_type = 'connection'
+ return conn_type
+
+ def _get_port_and_database(self, secret):
+ if 'port' in secret:
+ port = secret['port']
+ else:
+ port = 5432
Review comment:
Check https://github.com/apache/airflow/blob/master/airflow/secrets/local_filesystem.py out
----------------------------------------------------------------
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 a change in pull request #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#discussion_r455698455
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -60,15 +59,23 @@ class SecretsManagerBackend(BaseSecretsBackend, LoggingMixin):
def __init__(
self,
- connections_prefix: str = 'airflow/connections',
- variables_prefix: str = 'airflow/variables',
- profile_name: Optional[str] = None,
- sep: str = "/",
+ connections_prefix=None,
+ variables_prefix=None,
Review comment:
it does. If no-one passes anything, the default it used to take is `airflow/variables`
----------------------------------------------------------------
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] JavierLopezT commented on a change in pull request #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
JavierLopezT commented on a change in pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#discussion_r466333453
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -60,15 +59,23 @@ class SecretsManagerBackend(BaseSecretsBackend, LoggingMixin):
def __init__(
self,
- connections_prefix: str = 'airflow/connections',
- variables_prefix: str = 'airflow/variables',
- profile_name: Optional[str] = None,
- sep: str = "/",
+ connections_prefix=None,
+ variables_prefix=None,
Review comment:
Anyway, I don't see that these should be required arguments. In my current company, we share an AWS account with other departments. As so, we have to start our secrets with the prefix 'data_', hence in my current situation it does make sense to have `connections_prefix='data'`. However, in my previous company, we as BI team had an AWS account for ourselves, and thus we had the secrets just like _'db_redshift'_, _'api_salesforce'_ and so on, so the None value made more sense.
Furthermore, I would never expect to have the default variables as airflow/variables, because I won't ever think at all using the '/' sep in this case.
I know that what I think or believe is pointless, but I am wondering if there are more people with similar reasonings as me that the default values don't fit with their project.
Also, just curiosity (not saying that this is a case in which should be applied), is there any procedure to implement a change that breaks previous workflows?
----------------------------------------------------------------
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 a change in pull request #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#discussion_r466649936
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -60,15 +59,23 @@ class SecretsManagerBackend(BaseSecretsBackend, LoggingMixin):
def __init__(
self,
- connections_prefix: str = 'airflow/connections',
- variables_prefix: str = 'airflow/variables',
- profile_name: Optional[str] = None,
- sep: str = "/",
+ connections_prefix=None,
Review comment:
The following should work in your case (can you try):
```ini
[secrets]
backend = airflow.contrib.secrets.aws_secrets_manager.SecretsManagerBackend
backend_kwargs = {"connections_prefix": "", "variables_prefix": "", "sep": ""}
```
----------------------------------------------------------------
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] JavierLopezT commented on pull request #9008: Get connections uri with AWS Secrets Manager backend
Posted by GitBox <gi...@apache.org>.
JavierLopezT commented on pull request #9008:
URL: https://github.com/apache/airflow/pull/9008#issuecomment-669857264
> The way this is implemented is far to specialized, and hacky.
>
> They way I would expect this to work:
>
> * We first try to get the named secret -- if that finds a valid secret then we use the existing conn_uri approach; otherwise
> * We list secrets under the named path (i.e. `List("/airflow/connections/my_conn_id")` and then we fetch each of those secrets, feeding them in to a `Connection()` call in some manner.
>
> This way we an support arbitrary extra parameters, i.e. the all the extra params for a Connection too, and we don't ever need to use `ast`.
>
> Probably something like: if the name is not one of conn_type, host, login, password, schema, or port, extra then it gets fed in to the extra dict. (That way you can specify extra fields individually in Secrets Manager without having to manage a JSON value.)
Thanks for your answer ash. Sorry, I am not sure I am following you. I am a bit lost with your proposition of functionality. Could you be more specific using this example of how I use AWS Secrets Manager in my company, please?
In the picture, there are three secrets, and within each of them, there are secret keys (pass, user, api_key.. whatever) with its corresponding secret values.
I do understand what you say about removing the ast, and I think it's a very good idea, thank you for your suggestion.
----------------------------------------------------------------
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