vatsrahul1001 commented on code in PR #74840:
URL: https://github.com/apache/airflow/pull/74840#discussion_r4293859359
##########
shared/secrets_masker/src/airflow_shared/secrets_masker/secrets_masker.py:
##########
@@ -588,9 +595,13 @@ def should_hide_value_for_key(self, name):
Name might be a Variable name, or key in conn.extra_dejson, for
example.
"""
if isinstance(name, str) and self.hide_sensitive_var_conn_fields:
- name = name.strip().lower()
- normalized = re.sub(r"\W+", "_", name)
- return any(s in normalized for s in
self.sensitive_variables_fields)
+ name = name.strip()
+ normalized = _NON_WORD_RE.sub("_", name.lower())
+ split = _CAMEL_ACRONYM_RE.sub(r"\1_\2",
_CAMEL_LOWER_UPPER_RE.sub(r"\1_\2", name))
Review Comment:
Nit: since this is a substring match with no word-end anchor, I think any
camelCase key that merely *starts with* a multi-word sensitive name now matches
too. Running the default fields over a few keys, `serviceAccountName`,
`accessKeyPath`, `authHeaderName`, `keyfileDictPath` and `webhookUrlTemplate`
all flip from visible to `***`.
`serviceAccountName` is the one I'd think about most, since it's all over
k8s pod specs / KubernetesPodOperator / executor config and would now be masked
in rendered templates, the UI and logs. `service_account_name` is already
masked in snake_case today, so this is arguably consistent, but it's a visible
change. Is that intended? If so a line in the newsfragment would help; if not,
an end anchor would be needed.
Also optional: this runs per dict key and is roughly 3x slower than before
in a quick micro-benchmark (about 0.8us -> 2.5us per call). Negligible in
absolute terms, so just mentioning it in case a small cache or precomputed
regex is easy.
--
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]
