Get connections uri with AWS Secrets Manager backend #9008

JavierLopezT · 2020-05-25T16:34:59Z

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]

Description above provides context of the change
Unit tests coverage for changes (not needed for documentation changes)
Target Github ISSUE in description if exists
Commits follow "How to write a good git commit message"
Relevant documentation is updated including usage instructions.
I will engage committers as explained in Contribution Workflow Example.

…flow into secrets_backend

kaxil

Please add tests

JavierLopezT · 2020-05-28T15:49:22Z

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.

JavierLTPromofarma · 2020-05-29T12:12:48Z

I get the error Too many nested blocks, any suggestions to refactor the code @kaxil or @someonelese?
Thank you very much

airflow/providers/amazon/aws/secrets/secrets_manager.py

ashb · 2020-07-16T10:25:32Z

Love the feature! (And I had this in mind when reviewing the original secrets backend)

kaxil · 2020-07-16T10:26:27Z

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

ashb

At the very least this PR needs heavy tests.

Secondly: why do we need to use the AST at all? can you explain your design?

ashb · 2020-07-16T10:28:33Z

airflow/providers/amazon/aws/secrets/secrets_manager.py

-        variables_prefix: str = 'airflow/variables',
-        profile_name: Optional[str] = None,
-        sep: str = "/",
+        connections_prefix=None,


Why do these need to be None? this controls where in the secrets hierarchy to look at, so should be a required field, no?

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.

The following should work in your case (can you try):

[secrets] backend = airflow.contrib.secrets.aws_secrets_manager.SecretsManagerBackend backend_kwargs = {"connections_prefix": "", "variables_prefix": "", "sep": ""}

ashb · 2020-07-16T10:32:14Z

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.)

kaxil · 2020-07-16T10:36:25Z

airflow/providers/amazon/aws/secrets/secrets_manager.py

-        profile_name: Optional[str] = None,
-        sep: str = "/",
+        connections_prefix=None,
+        variables_prefix=None,


It also breaks backwards-compatibility

No. It's doesn't break, because we always use keyword arguments.
https://airflow.readthedocs.io/en/latest/configurations-ref.html#backend-kwargs

it does. If someone passes nothing, the default it used to take is airflow/variables

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?

kaxil · 2020-07-16T10:37:13Z

airflow/providers/amazon/aws/secrets/secrets_manager.py

+        if 'port' in secret:
+            port = secret['port']
+        else:
+            port = 5432


Let's not hardcode or keep any default here

Check https://github.com/apache/airflow/blob/master/airflow/secrets/local_filesystem.py out

JavierLopezT · 2020-08-06T10:51:49Z

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.

JavierLopezT · 2020-09-14T11:10:39Z

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!

stale · 2020-11-01T07:50:02Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

JavierLopezT · 2020-11-02T07:54:46Z

Commenting so that it doesn't get closed

andres-lowrie · 2020-12-10T03:25:16Z

airflow/providers/amazon/aws/secrets/secrets_manager.py

+            for key, value in extra_dict.values():
+                if counter == 0:
+                    conn_string += f'{key}={value}'
+                else:
+                    conn_string += f'&{key}={value}'


hello,

I'm wondering if this approach would work for nested extra values.

For example the emr_default one for aws looks like this:

airflow/airflow/utils/db.py

Line 205 in c704293

conn_id="emr_default",

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

github-actions · 2021-03-19T00:30:43Z

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.

JavierLTPromofarma added 4 commits May 25, 2020 18:13

new functionality

acec072

new functionality

9ede0df

new functionality

963febc

Merge branch 'secrets_backend' of https://github.com/JavierLopezT/air…

f4fc849

…flow into secrets_backend

boring-cyborg bot added area:secrets provider:amazon-aws AWS/Amazon - related issues labels May 25, 2020

kaxil reviewed May 25, 2020

View reviewed changes

static cheks

260c34a

JavierLTPromofarma added 2 commits May 29, 2020 10:00

incompatible type

b220b76

removing optional

ce95ea7

refactoring nested blocks

dc2b07a

mik-laj reviewed May 29, 2020

View reviewed changes

airflow/providers/amazon/aws/secrets/secrets_manager.py Outdated Show resolved Hide resolved

JavierLTPromofarma added 6 commits June 12, 2020 12:54

new method

85fd8ab

fixes

20fc685

several changes

c2db650

nested

19470b7

extra

f66fe74

solving Attribute Error

28dd9f5

ashb requested changes Jul 16, 2020

View reviewed changes

ashb reviewed Jul 16, 2020

View reviewed changes

kaxil reviewed Jul 16, 2020

View reviewed changes

stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Nov 1, 2020

stale bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Nov 2, 2020

andres-lowrie reviewed Dec 10, 2020

View reviewed changes

github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Mar 19, 2021

github-actions bot closed this Mar 25, 2021

JavierLopezT mentioned this pull request Aug 5, 2021

Aws secrets manager backend #17448

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get connections uri with AWS Secrets Manager backend #9008

Get connections uri with AWS Secrets Manager backend #9008

JavierLopezT commented May 25, 2020

kaxil left a comment

JavierLopezT commented May 28, 2020

JavierLTPromofarma commented May 29, 2020

ashb commented Jul 16, 2020

kaxil commented Jul 16, 2020

ashb left a comment

ashb Jul 16, 2020

JavierLopezT Aug 6, 2020

kaxil Aug 6, 2020 •

edited

ashb commented Jul 16, 2020 •

edited

kaxil Jul 16, 2020

mik-laj Jul 16, 2020

kaxil Jul 16, 2020 •

edited

JavierLopezT Aug 6, 2020 •

edited

kaxil Jul 16, 2020 •

edited

kaxil Jul 16, 2020

JavierLopezT commented Aug 6, 2020

JavierLopezT commented Sep 14, 2020

stale bot commented Nov 1, 2020

JavierLopezT commented Nov 2, 2020

andres-lowrie Dec 10, 2020

github-actions bot commented Mar 19, 2021

Get connections uri with AWS Secrets Manager backend #9008

Get connections uri with AWS Secrets Manager backend #9008

Conversation

JavierLopezT commented May 25, 2020

kaxil left a comment

Choose a reason for hiding this comment

JavierLopezT commented May 28, 2020

JavierLTPromofarma commented May 29, 2020

ashb commented Jul 16, 2020

kaxil commented Jul 16, 2020

ashb left a comment

Choose a reason for hiding this comment

ashb Jul 16, 2020

Choose a reason for hiding this comment

JavierLopezT Aug 6, 2020

Choose a reason for hiding this comment

kaxil Aug 6, 2020 • edited

Choose a reason for hiding this comment

ashb commented Jul 16, 2020 • edited

kaxil Jul 16, 2020

Choose a reason for hiding this comment

mik-laj Jul 16, 2020

Choose a reason for hiding this comment

kaxil Jul 16, 2020 • edited

Choose a reason for hiding this comment

JavierLopezT Aug 6, 2020 • edited

Choose a reason for hiding this comment

kaxil Jul 16, 2020 • edited

Choose a reason for hiding this comment

kaxil Jul 16, 2020

Choose a reason for hiding this comment

JavierLopezT commented Aug 6, 2020

JavierLopezT commented Sep 14, 2020

stale bot commented Nov 1, 2020

JavierLopezT commented Nov 2, 2020

andres-lowrie Dec 10, 2020

Choose a reason for hiding this comment

github-actions bot commented Mar 19, 2021

kaxil Aug 6, 2020 •

edited

ashb commented Jul 16, 2020 •

edited

kaxil Jul 16, 2020 •

edited

JavierLopezT Aug 6, 2020 •

edited

kaxil Jul 16, 2020 •

edited