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/06/14 08:00:48 UTC
[GitHub] [airflow] potiuk opened a new pull request #16424: Switch to primitive types in SecretsMasker
potiuk opened a new pull request #16424:
URL: https://github.com/apache/airflow/pull/16424
Using Iterable in SecretsMasker might cause undesireable
side effect in case the object passed as log parameter
is an iterable object and actually iterating it is not idempotent.
For example in case of botocore, it passes StreamingBody
object to log and this object is Iterable. However it can be
iterated only once. Masking causes the object to be iterated
during logging and results in empty body when actual results
are retrieved later.
This change only iterates list type of objects and recurrently
redacts only dicts/strs/tuples/sets/lists which should never
produce any side effects as all those objects do not have side
effects when they are accessed.
Fixes: #16148
<!--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16424: Switch to built-in data structures in SecretsMasker
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16424:
URL: https://github.com/apache/airflow/pull/16424#discussion_r652509870
##########
File path: airflow/utils/log/secrets_masker.py
##########
@@ -165,7 +164,7 @@ def _redact_all(self, item: "RedactableItem") -> "RedactableItem":
elif isinstance(item, (tuple, set)):
# Turn set in to tuple!
return tuple(self._redact_all(subval) for subval in item)
- elif isinstance(item, Iterable):
+ elif isinstance(item, list):
Review comment:
Oh that's a few lines up
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16424: Switch to primitive types in SecretsMasker
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16424:
URL: https://github.com/apache/airflow/pull/16424#issuecomment-860248161
cc: @uranusjr @mik-laj
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #16424: Switch to built-in data structures in SecretsMasker
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #16424:
URL: https://github.com/apache/airflow/pull/16424#discussion_r650561963
##########
File path: airflow/utils/log/secrets_masker.py
##########
@@ -196,12 +195,11 @@ def redact(self, item: "RedactableItem", name: str = None) -> "RedactableItem":
elif isinstance(item, (tuple, set)):
# Turn set in to tuple!
return tuple(self.redact(subval) for subval in item)
- elif isinstance(item, io.IOBase):
- return item
- elif isinstance(item, Iterable):
+ elif isinstance(item, list):
return list(self.redact(subval) for subval in item)
else:
return item
+ # I think this should never happen, but it does not hurt to leave it just in case
Review comment:
I could not find an easy way to raise an exception any more after removing Iterable. I believe we cannot get the Exception if we do what we do now - i.e. walking the built-in structures in the way we do. But maybe there is some way an exception can be raised here?
##########
File path: airflow/utils/log/secrets_masker.py
##########
@@ -196,12 +195,11 @@ def redact(self, item: "RedactableItem", name: str = None) -> "RedactableItem":
elif isinstance(item, (tuple, set)):
# Turn set in to tuple!
return tuple(self.redact(subval) for subval in item)
- elif isinstance(item, io.IOBase):
Review comment:
I think the only reason IOBase was added here specifically was that it was Iterable. This else is not needed now as this one will fall through the last `return item` anyway.
##########
File path: tests/utils/log/test_secrets_masker.py
##########
@@ -72,22 +72,6 @@ def test_args(self, logger, caplog):
assert caplog.text == "INFO Cannot connect to user:***\n"
- def test_non_redactable(self, logger, caplog):
Review comment:
Could not find a way to trigger Exception :)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16424: Switch to built-in data structures in SecretsMasker
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16424:
URL: https://github.com/apache/airflow/pull/16424#issuecomment-862155483
@ashb , does it look good for you? I think switching to list solves the problem entirely.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16424: Switch to built-in data structures in SecretsMasker
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16424:
URL: https://github.com/apache/airflow/pull/16424#discussion_r652508465
##########
File path: airflow/utils/log/secrets_masker.py
##########
@@ -196,12 +195,11 @@ def redact(self, item: "RedactableItem", name: str = None) -> "RedactableItem":
elif isinstance(item, (tuple, set)):
# Turn set in to tuple!
return tuple(self.redact(subval) for subval in item)
- elif isinstance(item, io.IOBase):
- return item
- elif isinstance(item, Iterable):
+ elif isinstance(item, list):
return list(self.redact(subval) for subval in item)
else:
return item
+ # I think this should never happen, but it does not hurt to leave it just in case
Review comment:
Agreet
##########
File path: airflow/utils/log/secrets_masker.py
##########
@@ -165,7 +164,7 @@ def _redact_all(self, item: "RedactableItem") -> "RedactableItem":
elif isinstance(item, (tuple, set)):
# Turn set in to tuple!
return tuple(self._redact_all(subval) for subval in item)
- elif isinstance(item, Iterable):
+ elif isinstance(item, list):
Review comment:
Tuple too please
##########
File path: tests/utils/log/test_secrets_masker.py
##########
@@ -72,22 +72,6 @@ def test_args(self, logger, caplog):
assert caplog.text == "INFO Cannot connect to user:***\n"
- def test_non_redactable(self, logger, caplog):
Review comment:
Inherit from list or dict should trigger it, but don't mind deleting this test
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #16424: Switch to built-in data structures in SecretsMasker
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #16424:
URL: https://github.com/apache/airflow/pull/16424#issuecomment-862203715
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #16424: Switch to built-in data structures in SecretsMasker
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #16424:
URL: https://github.com/apache/airflow/pull/16424
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16424: Switch to built-in data structures in SecretsMasker
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16424:
URL: https://github.com/apache/airflow/pull/16424#discussion_r650578039
##########
File path: airflow/utils/log/secrets_masker.py
##########
@@ -196,12 +195,11 @@ def redact(self, item: "RedactableItem", name: str = None) -> "RedactableItem":
elif isinstance(item, (tuple, set)):
# Turn set in to tuple!
return tuple(self.redact(subval) for subval in item)
- elif isinstance(item, io.IOBase):
- return item
- elif isinstance(item, Iterable):
+ elif isinstance(item, list):
return list(self.redact(subval) for subval in item)
else:
return item
+ # I think this should never happen, but it does not hurt to leave it just in case
Review comment:
I think it’s technically possible if the user passes in some kind of custom subclass (e.g. `class MyList(list):` that overrides some weird stuff), so yeah let’s keep the exception handler there, but I don’t think it’s worthwhile to have a test for it.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org