amoghrajesh commented on code in PR #70891:
URL: https://github.com/apache/airflow/pull/70891#discussion_r3701976429
##########
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:
+1 agreed.
##########
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.
+
+ ``add_mask`` walks dicts and iterables itself, so the top-level type
dispatch was the
+ only thing deciding whether nested values were reached: a list at the
top level matched
+ no branch and was skipped, while the same list nested inside a dict
was masked.
+ """
Review Comment:
```suggestion
```
Since its already there in the code path.
##########
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:
Agree with @SameerMesiah97, either that or remove docstring too.
##########
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):
Review Comment:
In dict branch, we dont pass key. Inside `add_mask` dict is handled this way:
```python
if isinstance(secret, dict):
for k, v in secret.items():
self.add_mask(v, k)
```
Therefore,
- `mask_secret({"password": "x"})` -> calls `add_mask("x", "password")`
- `mask_secret({"password": "x"}, "some_key")` -> calls `add_mask("x",
"password")`
So, you could just do this to simplify it:
```python
if isinstance(val, (str, dict, list)):
mask_secret(val, key)
```
--
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]
