You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/08/14 18:21:39 UTC
[GitHub] [airflow] potiuk opened a new pull request #17618: Fixes redacting secrets in context exceptions.
potiuk opened a new pull request #17618:
URL: https://github.com/apache/airflow/pull/17618
Secret masking did not work in context exceptions. When
there was a try/except/raise sequence, the original
exceptions were not redacted.
Fixes: #17604
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #17618: Fix redacting secrets in context exceptions.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #17618:
URL: https://github.com/apache/airflow/pull/17618#discussion_r688986501
##########
File path: airflow/utils/log/secrets_masker.py
##########
@@ -144,6 +144,13 @@ def _record_attrs_to_ignore(self) -> Iterable[str]:
)
return frozenset(record.__dict__).difference({'msg', 'args'})
+ def _redact_exception_with_context(self, exception):
+ exception.args = (self.redact(v) for v in exception.args)
+ if exception.__context__:
+ self._redact_exception_with_context(exception.__context__)
+ if exception.__cause__:
Review comment:
Yep. Super-duper-optimised :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #17618: Fix redacting secrets in context exceptions.
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17618:
URL: https://github.com/apache/airflow/pull/17618#issuecomment-898957610
I cherry-picked that one to v2-1 @jhtimmins @kaxil
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #17618: Fix redacting secrets in context exceptions.
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #17618:
URL: https://github.com/apache/airflow/pull/17618#discussion_r688978588
##########
File path: airflow/utils/log/secrets_masker.py
##########
@@ -144,6 +144,11 @@ def _record_attrs_to_ignore(self) -> Iterable[str]:
)
return frozenset(record.__dict__).difference({'msg', 'args'})
+ def _redact_exception_with_context(self, exception):
+ exception.args = (self.redact(v) for v in exception.args)
+ if exception.__context__:
Review comment:
Oh there's also a `__cause__` attr that we should probably redact too.
##########
File path: airflow/utils/log/secrets_masker.py
##########
@@ -144,6 +144,11 @@ def _record_attrs_to_ignore(self) -> Iterable[str]:
)
return frozenset(record.__dict__).difference({'msg', 'args'})
+ def _redact_exception_with_context(self, exception):
+ exception.args = (self.redact(v) for v in exception.args)
+ if exception.__context__:
Review comment:
Oh there's also a `__cause__` attr that we should probably redact too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #17618: Fixes redacting secrets in context exceptions.
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17618:
URL: https://github.com/apache/airflow/pull/17618#issuecomment-898941056
@kaxil @jhtimmins - we might want to include that in 2.1.3
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #17618: Fix redacting secrets in context exceptions.
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #17618:
URL: https://github.com/apache/airflow/pull/17618#issuecomment-898953181
Ahh cause and context are the same object in some cases:
```
In [12]: err.__context__ is err.__cause__
Out[12]: True
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #17618: Fix redacting secrets in context exceptions.
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #17618:
URL: https://github.com/apache/airflow/pull/17618#discussion_r688986409
##########
File path: airflow/utils/log/secrets_masker.py
##########
@@ -144,6 +144,13 @@ def _record_attrs_to_ignore(self) -> Iterable[str]:
)
return frozenset(record.__dict__).difference({'msg', 'args'})
+ def _redact_exception_with_context(self, exception):
+ exception.args = (self.redact(v) for v in exception.args)
+ if exception.__context__:
+ self._redact_exception_with_context(exception.__context__)
+ if exception.__cause__:
Review comment:
```suggestion
if exception.__cause__ and exception.__cause__ is not exception.__context__:
```
to avoid processing it twice?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #17618: Fix redacting secrets in context exceptions.
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #17618:
URL: https://github.com/apache/airflow/pull/17618#discussion_r688978506
##########
File path: airflow/utils/log/secrets_masker.py
##########
@@ -144,6 +144,11 @@ def _record_attrs_to_ignore(self) -> Iterable[str]:
)
return frozenset(record.__dict__).difference({'msg', 'args'})
+ def _redact_exception_with_context(self, exception):
+ exception.args = (self.redact(v) for v in exception.args)
+ if exception.__context__:
Review comment:
Do we need a `hasattr`/`getattr` here, or do all exceptions have this field?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #17618: Fix redacting secrets in context exceptions.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #17618:
URL: https://github.com/apache/airflow/pull/17618#issuecomment-898942986
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #17618: Fix redacting secrets in context exceptions.
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17618:
URL: https://github.com/apache/airflow/pull/17618#issuecomment-898953052
Interesting thing... Actually that would work even before latest change, because in this case, the "cause" is also in "context":
```
try:
try:
raise Exception("user:pass")
except Exception as e:
# This is how you get an exception with a __cause__
raise Exception("two") from e
except Exception as e:
err = e
```
But this wuldn't work:
```
exception = None
try:
try:
raise Exception("user:pass")
except Exception as e:
# This is how you get an exception with a __cause__
exception = e
try:
raise Exception("x") from exception
except Exception as e:
err = e
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #17618: Fix redacting secrets in context exceptions.
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #17618:
URL: https://github.com/apache/airflow/pull/17618#issuecomment-898943488
```python
try:
try:
raise Exception("user:pass")
except Exception as e:
# This is how you get an exception with a __cause__
raise Exception("two") from e
except Exception as e:
err = e
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #17618: Fix redacting secrets in context exceptions.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #17618:
URL: https://github.com/apache/airflow/pull/17618#discussion_r688982373
##########
File path: airflow/utils/log/secrets_masker.py
##########
@@ -144,6 +144,11 @@ def _record_attrs_to_ignore(self) -> Iterable[str]:
)
return frozenset(record.__dict__).difference({'msg', 'args'})
+ def _redact_exception_with_context(self, exception):
+ exception.args = (self.redact(v) for v in exception.args)
+ if exception.__context__:
Review comment:
Yeah. https://www.python.org/dev/peps/pep-3134/ . I never before saw the "from" syntax, but yeah... __cause__ should be there as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #17618: Fix redacting secrets in context exceptions.
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #17618:
URL: https://github.com/apache/airflow/pull/17618
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #17618: Fix redacting secrets in context exceptions.
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17618:
URL: https://github.com/apache/airflow/pull/17618#issuecomment-898952012
Added `__cause__` handling. Good call @ashb!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org