-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
Masking configuration values irrelevant to DAG author #43040
Conversation
Just adding a unit test, and I think it's ready to go :) |
Struggling with some test related setup. I am running into this as of now:
I see some discussions around, does anyone have any suggestions for this? I use Pycharm |
I believe (I have not yet had time to look at it) this requires to use I guess those should be updated ?
|
I think we generally should provide rather comprehensive guide (at least with some links) to our contributors how to setup the venv now (Breeze is covered because it uses workspace installation internally I believe)? Am I right @ashb ? |
Breeze is all set up yes. UV workspace isn't 100% required yet (though I don't think anyone tested running tests from inside pycharm), but for pycharm to find imports you need to set up some paths as the right kind of folder in the UI I'm not a pycharm user, but @kaxil @dstandish can provide some insight. |
#42951 should help Pycharm too. Remaining "manual" steps as pycharm user are:
|
I just pulled in #42951, it doesn't seem to fix it for me. I still keep getting
|
Anything in particular you wanted me to look at? |
@ashb @potiuk @kaxil I changed the logic to incorporate what was suggested above. Now I am able to get this result:
From my original list, these aren't masked:
As per the comments above, looks like I think:
Are ok to be unmasked. That leaves us with:
Should these be masked or its alright to keep them unmasked? |
|
Okay, in that case, this should be ok |
Fixed in #43082 |
@ashb does it look good to go? Got a green CI |
Looks like all the comments are resolved and address by @amoghrajesh . LGTM. @kaxil @ashb - are you ok with merging it? |
Some configurations are irrelevant to DAG authors and hence we need to mask those to avoid it from getting logged unknowingly. Co-authored-by: adesai <[email protected]> Co-authored-by: Ash Berlin-Taylor <[email protected]>
This change has caused the |
Fix here: #43335 |
Some configurations are irrelevant to DAG authors and hence we need to mask those to avoid it from getting logged unknowingly. Co-authored-by: adesai <[email protected]> Co-authored-by: Ash Berlin-Taylor <[email protected]> (cherry picked from commit 0b030c5)
Some configurations are irrelevant to DAG authors and hence we need to mask those to avoid it from getting logged unknowingly. Co-authored-by: adesai <[email protected]> Co-authored-by: Ash Berlin-Taylor <[email protected]> (cherry picked from commit 0b030c5) Co-authored-by: Amogh Desai <[email protected]>
Some configurations are irrelevant to DAG authors and hence we need to mask those to avoid it from getting logged unknowingly. Co-authored-by: adesai <[email protected]> Co-authored-by: Ash Berlin-Taylor <[email protected]>
@amoghrajesh , shouldn't the list of sensitive_config_values be updated with the list of keys that need to redact ? |
Not really @saurabhb-dev , It's even better to mask values in this case, rather than keys. The secrets_masker works in two modes:
This is what happens here - we retrieve all the secret config values (whether they come by env vars or by other means) and we add values (i.e. actual secrets) to be masked. This way when any secret is printed anywhere where secrets_masker is used, it will automatically mask it - regardless if it is a structure (secrets_masker checks values of dicts for example) or whether it's already converted to string. |
Some configurations are irrelevant to DAG authors and hence we need to mask those to avoid it from getting logged unknowingly. Co-authored-by: adesai <[email protected]> Co-authored-by: Ash Berlin-Taylor <[email protected]> (cherry picked from commit 0b030c5) Co-authored-by: Amogh Desai <[email protected]>
figure i'd ask here before creating a new issue airflow/airflow/configuration.py Lines 857 to 859 in c99887e
hence the
which is a bit confusing since i removed all references to |
@zachliu we do not return ('database', 'sql_alchemy_conn') as part of self.sensitive_config_values. Here are the sensitive values: https://github.com/amoghrajesh/airflow/blob/840ea3efb9533837e9f36b75fa527a0fbafeb23a/airflow/configuration.py#L122-L129 The warning comes from configuration.py#919 |
@amoghrajesh i beg to differ
i'm on https://github.com/apache/airflow/tree/2.10.3 |
@zachliu from the PR it doesnt look like it was targetted for 2.10.3. |
I created a new issue to track this: #43794 |
Some configurations are irrelevant to DAG authors and hence we need to mask those to avoid it from getting logged unknowingly. Co-authored-by: adesai <[email protected]> Co-authored-by: Ash Berlin-Taylor <[email protected]>
Some configurations are irrelevant to DAG authors and hence we need to mask those to avoid it from getting logged unknowingly.
^ 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.rst
or{issue_number}.significant.rst
, in newsfragments.