Secret masker ignores passwords with special chars

[aritra24]

Connection uri's get connection uses quote to change the password and certain other fields to escape special chars due to this, when the connection object is passed through the masker this changed string is skipped.

closes: #36688

---

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

[aritra24]

Wasn't entirely sure how to add testing for this since it needs to pass through the masker? 🤔 If anyone has ideas let me know. Nvm, I think the tests do already test for this, they just didn't have any special chars. I've added special chars into a few of them.

[hussein-awala]

I duplicated the updated tests on the main branch, and they passed without your change. Could you check if your change is really necessary, or you need to create a new test that fail on main and works with your change?

[aritra24]

@hussein-awala I see the test failing on main

=========================== short test summary info ============================
FAILED tests/always/test_connection.py::TestConnection::test_masking_from_db - AssertionError: assert equals failed
                                   [
                                     call('s3cr3t!'),
                                     call('s3cr3t%21'),
  [call('s3cr3t!'), call({'apikey    call({'apikey': 'masked too'}
  ': 'masked too'})]               ),
                                   ]
======================= 1 failed, 13 warnings in 20.21s ========================

[aritra24]

@hussein-awala could I get a re-review on this.

[hussein-awala]

@hussein-awala I see the test failing on main

=========================== short test summary info ============================
FAILED tests/always/test_connection.py::TestConnection::test_masking_from_db - AssertionError: assert equals failed
                                   [
                                     call('s3cr3t!'),
                                     call('s3cr3t%21'),
  [call('s3cr3t!'), call({'apikey    call({'apikey': 'masked too'}
  ': 'masked too'})]               ),
                                   ]
======================= 1 failed, 13 warnings in 20.21s ========================

That's because you check if the mock is called with quote method, which is not currently the case.

Could you add a test that checks if the final result is correct?

[aritra24]

Added a secret_masker test which tests that the logs do get redacted. @hussein-awala if you could re-review.

[amoghrajesh]

@aritra24 I personally think this should be the case by design, aligning with the postgres documentation too.
Check my comment: #36688 (comment)

cc @hussein-awala

[aritra24]

@amoghrajesh made a comment on the issue. Could you take a look again?

[amoghrajesh]

Thanks for making the changes, @aritra24
LGTM +1

[potiuk]

@hussein-awala - I run the tests in main now and they nicely fail there. I think this one is ready to go.

[amoghrajesh]

Ping @hussein-awala, I think this one is ready, can you take a look?

[potiuk]

Yep. for me looks good too.

[aritra24]

@hussein-awala pinging you for a re-review. This commits ready to merge.

I'm a bit busy this week, I'll let the other committers test it and merge it if they think it's ready.

[aritra24]

@amoghrajesh / @potiuk could you please have a look and merge if you believe it's done.

[amoghrajesh]

The changes look fine to me. @potiuk WDYT on a last review prior to merge?

[potiuk]

Nice!