Allow to specify which connection, variable or config are being looked up in the backend using *_lookup_pattern parameters #29580

vincbeck · 2023-02-16T19:42:31Z

Many Airflow customers use AWS Secret Manager as their backend and they run into their AWS Secrets Manager / Systems Manager cost ballooning as Airflow has a fixed search path and each connection/variable/config looks up first in Secrets Backend. This happen on DAG execution, environment setup and can happen in DAG parsing if the user has defined things in the top-level python code. Adding *_lookup_pattern parameters to allow the user to specify which connection/variable/config they want to be looked in Secrets Manager and/or Systems manager.

Which customers are impacted?

Only customers who use AWS Secret Manager or AWS Systems Manager as secret backend specified through the config [secret]
Among AWS secrets backend customers, only the customers who choose to define the lookup-pattern will see different behavior which will be based on their definition. There will be no change in the default behavior

Why this change is needed?

Some customers want to fallback quickly on the Airflow metastore or env variables instead of looking up first in the secret backend. One example is a user having one connection defined in AWS Secret Manager or AWS Systems Manager and the rest in Airflow metastore. The user does not want to make a request for each connection to the secret backend but only for the specific one.

Related: #19251

@Taragolis you might be interested to look into this one.

AWS folks: @ferruzzi, @vandonr-amz, @syedahsn

…d up in the backend using *_lookup_pattern parameters

Taragolis · 2023-02-16T19:54:05Z

for each variable and connection access and it is done every time the Scheduler parses DAGs

It happen every time if users not follow Best Practices about Top Level Python Code 🤣 . Or some of the Operators call establish connections in constructor, I know only about SSHOperator from Community Providers with this behaviour

eladkal · 2023-02-16T20:08:17Z

I agree with @Taragolis the problem as described in this PR is not the same as described in #19251 in the issue dag parsing is not in the problem domain.

@vincbeck Please share DAG code example that this PR attempts to solve

shubham22 · 2023-02-16T20:21:34Z

Customers don't always write the most optimized DAGs. Conns, Configs and variables are always searched in secrets backend before env variables and meta store. This change will allow customers to only lookup secrets backend when their variables, et al. follow a pattern defined by them. Based on MWAA data, I can confirm that customers are spending hundreds of dollars on their secrets backed, just to use it with Ariflow.

potiuk · 2023-02-16T20:36:48Z

Maybe it's time resurrect this one from @Taragolis ? #23560

I have a feeling that adding lookup filter is quite a bit more complex (in terms of users understanding what to do there properly and in terms of usage protection). What happens if you change the pattern ? Will you remember to update it or you will be surprised by high involce next month?

shubham22 · 2023-02-16T20:43:48Z

in terms of users understanding what to do there properly and in terms of usage protection

This is a fair argument and agree that this can be misused or can lead to issues which are hard to track. We did consider adding only prefix, say "aws-conn-* instead of general regex, but Airflow customers are primed toward flexibility. This is why we went with regex to make it suitable for different needs. With documentation and, likely, a blogpost we will try to share with customers best practices around this. Like many other things, if customers really want to break, there are many ways to break it.

#23560
Oh, I didn't know this was tried before. We did consider making search order path configurable as well, but decided against it as search path order can lead to lot of side-effects, compared to providing customers a configurable lookup pattern option.

eladkal · 2023-02-16T20:46:58Z

I'm sorry but I don't follow which problem are we tackling here.
The PR description talks about DAG parsing but @shubham22 comment is not about that.

shubham22 · 2023-02-16T20:51:18Z

The PR description talks about DAG parsing but @shubham22 comment is not about that.

Sorry for the confusion, Elad. @vincbeck will soon update the description to make the scope and impact clear.

Taragolis · 2023-02-16T21:16:50Z

Customers don't always write the most optimized DAGs. Conns, Configs and variables are always searched in secrets backend before env variables and meta store. This change will allow customers to only lookup secrets backend when their variables, et al. follow a pattern defined by them. Based on MWAA data, I can confirm that customers are spending hundreds of dollars on their secrets backed, just to use it with Ariflow.

That actually just postpone the problem, and might lead to problem with Aurora instances (AFAIK it uses in MWAA as Postgres backend), yeah it would be cheaper for end users, because in this case they would not pay for IOPS on Aurora in case of MWAA, but still can be an issue with backend itself.

Is MWAA still support only Secrets Manager by default? Because I guess SSM Parameter store much cheaper rather than AWS SM. You don't have to pay 0.4 cents secrets/per month, and no cost for API call for Standard Parameter with Standard Throughput, I know because personally use SSM since Secrets Backends introduced in Airflow 1.10.10

I do not have any concern about this changes, just want to mention that the main problem, that users do not follow Best Practice, and as result they have a huge or not bills. The huge bill might happen if user turn on DEBUG level in production and write everything to logs in this case CloudWatch pricing might be, well... pretty surprising.

Maybe it's time resurrect this one from @Taragolis ? #23560

Ooooh... My first PR to Airflow repo 🤣 That still in my list, actually in two list, TODO and make wide discussion about Configurations and consistency with Secrets Backend, still no idea how to resolve the problem, that Airflow DB use both Secrets Backend capability and own implementation for obtain Variables and Connections in CLI/API/Webserver

vincbeck · 2023-02-16T21:52:38Z

Sorry for the bad description guys! I definitely did not expect so much confusion so sorry for that again. I updated the description which, hopefully, make the PR more clear.

I understand we should push people to write DAGs which follow guidelines but I am definitely not targeting specifically these users when doing this change. See the example in the description. And I feel there are a lot of other use cases which this feature can be a good add-on.

dimberman

Looks great! A few small nits then glad to merge.

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

dimberman · 2023-02-19T20:12:20Z

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

+    :param connections_lookup_pattern: Specifies a pattern the connection ID needs to match to be looked up in
+        AWS Secrets Manager. Applies only if `connections_prefix` is not None.
+        If set to None (null value in the configuration), all connections will be looked up first in
+        AWS Secrets Manager.


I really like this idea. Cool feature add on!

dimberman · 2023-02-19T20:13:05Z

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

@@ -264,14 +286,14 @@ def get_conn_uri(self, conn_id: str) -> str | None:

    def get_variable(self, key: str) -> str | None:
        """
-        Get Airflow Variable from Environment Variable
+        Get Airflow Variable


Can you add some more description here. If we're getting this variable from multiple places maybe we should be clear about the options?

In the context of this class, we are actually getting it from only one location: AWS Secrets Manager. If this function returns None, then it is fetched from other location: Environment variable then metastore but this is done outside of this class. See documentation here. Let me know if you still think I should update the documentation

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

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

potiuk · 2023-02-20T21:54:49Z

Yep. I thouthg about it and this seems like non-obvious great way of solving the "cost" problem connected with secret backends.

vincbeck · 2023-02-21T20:02:27Z

Thanks @dimberman for the feedbacks. I addressed them, feel free to review when you get a chance :)

eladkal · 2023-02-23T22:21:10Z

Should we modify a bit the description of https://airflow.apache.org/docs/apache-airflow/stable/administration-and-deployment/security/secrets/secrets-backend/index.html#search-path ?

If you enable an alternative secrets backend, it will be searched first, followed by environment variables, then metastore. This search ordering is not configurable.

When merging this PR the statement becomes not very accurate

vincbeck · 2023-02-24T16:41:15Z

Should we modify a bit the description of https://airflow.apache.org/docs/apache-airflow/stable/administration-and-deployment/security/secrets/secrets-backend/index.html#search-path ?

If you enable an alternative secrets backend, it will be searched first, followed by environment variables, then metastore. This search ordering is not configurable.

When merging this PR the statement becomes not very accurate

Good point. I added a sentence saying that, depending on the secret backend used, there might be an option to filter which connection/variable/config is searched in the secret backend. Let me know if it does look good to you

eladkal · 2023-03-01T19:28:21Z

@pierrejeambrun this needs to go to 2.5.2
from core point of view this PR contains only doc so this is a doc-change only.
The core logic/changes is in the Amazon provider so this will be a feature release for the provider as part of providers release cycle.

Airflow core -> minor release 2.5.2
Amazon provider -> feature release 7.3.0

All points are addressed

eladkal · 2023-03-01T19:32:51Z

@dimberman I'm going to merge this PR so we can start working on providers + core release. If something else is needed we can address it in followup PR.

pierrejeambrun · 2023-03-01T19:55:39Z

Thanks @eladkal for the details, I completely missed that!

pierrejeambrun · 2023-03-23T18:55:54Z

Marked for 2.6, conflicting and requires 2 providers only change to merge the 1 line core doc change. Raise a specific PR to v2-5 branch to avoid cherry picking them.

Allow to specify which connection, variable or config are being looke…

03a3670

…d up in the backend using *_lookup_pattern parameters

vincbeck requested review from eladkal and o-nikolas as code owners February 16, 2023 19:42

boring-cyborg bot added area:providers area:secrets kind:documentation provider:amazon-aws AWS/Amazon - related issues labels Feb 16, 2023

Merge branch 'main' into vincbeck/secrets

9e2f9d7

dimberman previously requested changes Feb 19, 2023

View reviewed changes

vincbeck and others added 2 commits February 21, 2023 13:49

Merge branch 'main' into vincbeck/secrets

11f47da

Address feedbacks

3848b66

vincbeck requested review from dimberman and removed request for eladkal and o-nikolas February 21, 2023 19:02

vincbeck and others added 2 commits February 24, 2023 11:41

Merge branch 'main' into vincbeck/secrets

af499e3

Add documentation

2d8bf8d

vincbeck requested a review from potiuk as a code owner February 24, 2023 16:48

eladkal merged commit 5de4791 into apache:main Mar 1, 2023

eladkal modified the milestones: Airflow 2.6.0, Airflow 2.5.2 Mar 1, 2023

eladkal added the type:doc-only Changelog: Doc Only label Mar 1, 2023

vincbeck deleted the vincbeck/secrets branch March 1, 2023 19:57

eladkal mentioned this pull request Mar 3, 2023

Status of testing Providers that were prepared on March 03, 2023 #29901

Closed

40 tasks

pierrejeambrun modified the milestones: Airflow 2.5.2, Airflow 2.6.0, Airflow 2.5.3 Mar 8, 2023

potiuk mentioned this pull request Mar 23, 2023

Add a cache to Variable and Connection when called at dag parsing time #30259

Merged

pierrejeambrun added the type:improvement Changelog: Improvements label Mar 23, 2023

pierrejeambrun modified the milestones: Airflow 2.5.3, Airflow 2.6.0 Mar 23, 2023

pierrejeambrun removed the type:improvement Changelog: Improvements label Mar 23, 2023

pierrejeambrun mentioned this pull request Mar 23, 2023

AWS Secrets Manager Backend - major update #27920

Merged

pierrejeambrun modified the milestones: Airflow 2.6.0, Airflow 2.5.3 Mar 23, 2023

pierrejeambrun modified the milestones: Airflow 2.5.3, Airflow 2.6.0 Mar 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to specify which connection, variable or config are being looked up in the backend using *_lookup_pattern parameters #29580

Allow to specify which connection, variable or config are being looked up in the backend using *_lookup_pattern parameters #29580

vincbeck commented Feb 16, 2023 •

edited

Loading

Taragolis commented Feb 16, 2023

eladkal commented Feb 16, 2023

shubham22 commented Feb 16, 2023 •

edited

Loading

potiuk commented Feb 16, 2023

shubham22 commented Feb 16, 2023 •

edited

Loading

eladkal commented Feb 16, 2023

shubham22 commented Feb 16, 2023

Taragolis commented Feb 16, 2023

vincbeck commented Feb 16, 2023

dimberman left a comment

dimberman Feb 19, 2023

dimberman Feb 19, 2023

vincbeck Feb 21, 2023

potiuk commented Feb 20, 2023

vincbeck commented Feb 21, 2023

eladkal commented Feb 23, 2023

vincbeck commented Feb 24, 2023

eladkal commented Mar 1, 2023 •

edited

Loading

eladkal commented Mar 1, 2023

pierrejeambrun commented Mar 1, 2023

pierrejeambrun commented Mar 23, 2023 •

edited

Loading

Allow to specify which connection, variable or config are being looked up in the backend using *_lookup_pattern parameters #29580

Allow to specify which connection, variable or config are being looked up in the backend using *_lookup_pattern parameters #29580

Conversation

vincbeck commented Feb 16, 2023 • edited Loading

Which customers are impacted?

Why this change is needed?

Taragolis commented Feb 16, 2023

eladkal commented Feb 16, 2023

shubham22 commented Feb 16, 2023 • edited Loading

potiuk commented Feb 16, 2023

shubham22 commented Feb 16, 2023 • edited Loading

eladkal commented Feb 16, 2023

shubham22 commented Feb 16, 2023

Taragolis commented Feb 16, 2023

vincbeck commented Feb 16, 2023

dimberman left a comment

Choose a reason for hiding this comment

dimberman Feb 19, 2023

Choose a reason for hiding this comment

dimberman Feb 19, 2023

Choose a reason for hiding this comment

vincbeck Feb 21, 2023

Choose a reason for hiding this comment

potiuk commented Feb 20, 2023

vincbeck commented Feb 21, 2023

eladkal commented Feb 23, 2023

vincbeck commented Feb 24, 2023

eladkal commented Mar 1, 2023 • edited Loading

eladkal commented Mar 1, 2023

pierrejeambrun commented Mar 1, 2023

pierrejeambrun commented Mar 23, 2023 • edited Loading

vincbeck commented Feb 16, 2023 •

edited

Loading

shubham22 commented Feb 16, 2023 •

edited

Loading

shubham22 commented Feb 16, 2023 •

edited

Loading

eladkal commented Mar 1, 2023 •

edited

Loading

pierrejeambrun commented Mar 23, 2023 •

edited

Loading