Switch to built-in data structures in SecretsMasker #16424

potiuk · 2021-06-13T17:55:25Z

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

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

potiuk · 2021-06-13T17:55:52Z

cc: @uranusjr @mik-laj

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: apache#16148

potiuk · 2021-06-13T18:37:55Z

airflow/utils/log/secrets_masker.py

                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


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?

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.

potiuk · 2021-06-13T18:39:04Z

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):


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.

potiuk · 2021-06-13T18:39:45Z

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):


Could not find a way to trigger Exception :)

Inherit from list or dict should trigger it, but don't mind deleting this test

potiuk · 2021-06-16T08:17:49Z

@ashb , does it look good for you? I think switching to list solves the problem entirely.

ashb · 2021-06-16T09:17:31Z

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):


Tuple too please

Oh that's a few lines up

ashb · 2021-06-16T09:17:54Z

airflow/utils/log/secrets_masker.py

                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


ashb · 2021-06-16T09:18:57Z

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):


Inherit from list or dict should trigger it, but don't mind deleting this test

github-actions · 2021-06-16T09:27:46Z

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.

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 (cherry picked from commit d1d02b6)

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: apache#16148 (cherry picked from commit d1d02b6) (cherry picked from commit 4c37aea)

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: apache#16148 (cherry picked from commit d1d02b6) (cherry picked from commit 4c37aea) (cherry picked from commit 7e5968a)

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: apache#16148 (cherry picked from commit d1d02b6) (cherry picked from commit 4c37aea) (cherry picked from commit 7e5968a) (cherry picked from commit 523bba0)

boring-cyborg bot added the area:logging label Jun 13, 2021

potiuk requested a review from ashb June 13, 2021 17:55

potiuk force-pushed the just_redact_primitive_values branch from 2bb0b92 to ceab5ac Compare June 13, 2021 17:57

potiuk changed the title ~~Switch to primitive types in SecretsMasker~~ Switch to built-in data structures in SecretsMasker Jun 13, 2021

potiuk force-pushed the just_redact_primitive_values branch from ceab5ac to af39d40 Compare June 13, 2021 18:36

potiuk commented Jun 13, 2021

View reviewed changes

uranusjr approved these changes Jun 13, 2021

View reviewed changes

ashb reviewed Jun 16, 2021

View reviewed changes

ashb added this to the Airflow 2.1.1 milestone Jun 16, 2021

ashb approved these changes Jun 16, 2021

View reviewed changes

github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Jun 16, 2021

potiuk merged commit d1d02b6 into apache:main Jun 16, 2021

potiuk deleted the just_redact_primitive_values branch June 16, 2021 09:29

potiuk mentioned this pull request Jun 16, 2021

secrets_masker RecursionError with nested TriggerDagRunOperators #16473

Closed

kaxil mentioned this pull request Jun 23, 2021

Add AC 2.0.0-8.dev & 2.0.2-4.dev astronomer/ap-airflow#276

Merged

potiuk restored the just_redact_primitive_values branch April 26, 2022 20:47

potiuk deleted the just_redact_primitive_values branch July 29, 2022 19:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to built-in data structures in SecretsMasker #16424

Switch to built-in data structures in SecretsMasker #16424

potiuk commented Jun 13, 2021

potiuk commented Jun 13, 2021

potiuk Jun 13, 2021

uranusjr Jun 13, 2021

ashb Jun 16, 2021

potiuk Jun 13, 2021

potiuk Jun 13, 2021

ashb Jun 16, 2021

potiuk commented Jun 16, 2021

ashb Jun 16, 2021

ashb Jun 16, 2021

ashb Jun 16, 2021

ashb Jun 16, 2021

github-actions bot commented Jun 16, 2021

		@@ -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):

Switch to built-in data structures in SecretsMasker #16424

Switch to built-in data structures in SecretsMasker #16424

Conversation

potiuk commented Jun 13, 2021

potiuk commented Jun 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk commented Jun 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 16, 2021