SameerMesiah97 commented on code in PR #70891:
URL: https://github.com/apache/airflow/pull/70891#discussion_r3696967650
##########
task-sdk/src/airflow/sdk/execution_time/context.py:
##########
@@ -323,9 +323,22 @@ def _mask_and_deserialize_variable(raw: str, key: str,
deserialize_json: bool) -
return raw
val = json.loads(raw)
if isinstance(val, str):
+ # No key names of its own, so the variable's key decides whether it is
masked.
mask_secret(val, key)
elif isinstance(val, dict):
+ # ``add_mask`` walks a dict per key and masks by the *inner* key name,
ignoring the
+ # name passed here.
mask_secret(val)
+ elif isinstance(val, list):
+ # A list was previously skipped entirely, even though ``add_mask``
walks iterables --
+ # the same list nested one level inside a dict was masked, only a
top-level one was not.
+ #
+ # It is passed under the variable's key, not anonymously: a list has
no key names of
+ # its own, so bare values inside follow the same rule as the
plain-string case above.
+ # Masking them unconditionally would add every element to the global
pattern set, so a
+ # list of ordinary values such as region names would be redacted
everywhere it appeared.
+ # Dicts *inside* the list are still masked by their own key names.
Review Comment:
I had to read this a few times to understand what it was trying to convey. I
think the comment can be more concise and clear. Please see below:
```
# Pass the Variable's key so list elements inherit the Variable's sensitivity
# instead of being added to the global mask patterns.
```
##########
task-sdk/tests/task_sdk/execution_time/test_context.py:
##########
@@ -428,6 +428,41 @@ def test_var_json_masks_raw_string_and_dict_values(self,
mock_mask_secret, mock_
# Second call: deserialized dict so internal sensitive fields like
"password" get masked
mock_mask_secret.assert_any_call({"password": "s3cr3t", "host":
"db.example.com"})
+ @mock.patch("airflow.sdk.execution_time.context.mask_secret")
+ def test_var_json_masks_list_values(self, mock_mask_secret,
mock_supervisor_comms):
+ """A JSON list is handed to the masker whole, exactly as a dict is.
+
Review Comment:
I would keep the first line for the docstring.
##########
task-sdk/tests/task_sdk/execution_time/test_context.py:
##########
@@ -456,15 +491,25 @@ def test_var_json_string_value_masks_both_forms(self,
mock_mask_secret, mock_sup
@mock.patch("airflow.sdk.execution_time.context.mask_secret")
def test_var_json_list_value_does_not_over_mask(self, mock_mask_secret,
mock_supervisor_comms):
- """var.json with a non-sensitive list variable does not mask
individual list elements."""
+ """var.json with a non-sensitive list variable does not mask
individual list elements.
+
+ The list is handed to the masker **under the variable's key**, which
is what preserves
+ this: bare values inside a list have no key names of their own, so
they follow the
+ variable key's sensitivity. Passing the list anonymously instead would
add every element
+ to the global pattern set, and an ordinary value such as a region name
would then be
+ redacted everywhere it appeared in the logs.
Review Comment:
Same here. The first line of the docstring can be kept and extra context can
be retained in the comments.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
