-
Notifications
You must be signed in to change notification settings - Fork 16.3k
TaskSDK: Make secrets masking work when conns are loaded from secrets backends #54574
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
I think I've caused an infinite loop |
|
Ah no wasn't an infinite loop, was just not updating the dag processor handler |
9af3346 to
700acd1
Compare
… backends If the connection is loaded from a secrets backend (be it something like Hashicorp Vault, or even just as simple as env vars!) the mask will only be applied to the subprocess, so won't catch much of the output. To fix this we send a message to the Supervisor process of the value to redact. This also captures and "mirrors" direct calls to `mask_secret` from user code to the supervisor so that it can mask the output correctly.
700acd1 to
ce03d93
Compare
|
Nice ! Good one @ashb the pull it so quicky |
Backport failed to create: v3-0-test. View the failure log Run details
You can attempt to backport this manually by running: cherry_picker 1f4c55c v3-0-testThis should apply the commit to the v3-0-test branch and leave the commit in conflict state marking After you have resolved the conflicts, you can continue the backport process by running: cherry_picker --continue |
Somewhat annoyingly it has to be `sync_to_async(mask_secret)` but that is un-avoidable unfortunately. Similar to #54574, but for the trigger.
Somewhat annoyingly it has to be `sync_to_async(mask_secret)` but that is unavoidable unfortunately. Similar to #54574, but for the trigger.
|
after upgrading to airflow 3.0.5 operators/hooks failed with similar messages: |
|
hi @dshvedchenko |
|
Which hook is that coming from? |
|
All of them :) |
|
I think it depends on what fields are in the connection -- In my (admittedly simple) testing with http connection it worked fine. Can you give me a connection to test with? |
|
|
I was creating connection using: the problem was originally with some of the triggered where we couldn't pass Enum values from tigggerer to worker (also needs to be reworked). We need I think more types here to support and it will solve the problem for everyone. |
|
Looks like this is an unsolved bug in pydantic pydantic/pydantic#9541 (and also impropper testing on our part) |
|
Looks like yes, more complicated test cases would be good to have :) |
|
Nothing providers can do, it needs a change in Airflow core. For people hitting this, if you can't/don't want to downgrade you can apply a patch to your Airflow to work around it - instructions in #54769 (comment) |
If the connection is loaded from a secrets backend (be it something like
Hashicorp Vault, or even just as simple as env vars!) the mask will only be
applied to the subprocess, so won't catch much of the output. To fix this we
send a message to the Supervisor process of the value to redact.
This also captures and "mirrors" direct calls to
mask_secretfrom user codeto the supervisor so that it can mask the output correctly.
Docs look like this
Closes #54540
I was testing with this dag. In a follow up I'll add it to the sdk integration tests to ensure masking works fully end-to-end.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.