divyanshus2404 commented on code in PR #71711:
URL: https://github.com/apache/airflow/pull/71711#discussion_r3944235400
##########
providers/apache/spark/tests/unit/apache/spark/hooks/test_spark_submit.py:
##########
@@ -1315,6 +1356,35 @@ def test_masks_passwords(self, command: str, expected:
str) -> None:
# Then
assert command_masked == expected
+ @pytest.mark.db_test
+ def test_masks_passwords_stays_fast_on_large_input(self) -> None:
+ # The previous pattern retried at every offset on long inputs, taking
tens of
+ # seconds for this payload and blocking the worker slot.
+ hook = SparkSubmitHook()
+ payload = ("spark-submit", "--arg", "x " * 25_000)
Review Comment:
The trailing space is deliberate. `"x " * 25_000` produces 25,000 separate
tokens, and it's the number of token boundaries that exercises the per-offset
retry the old pattern did — `"x" * 25_000` is one long token, which is a much
weaker case for this regression. I've added a comment on the test saying so.
There's a separate test right below
(`test_masks_passwords_stays_fast_on_repeated_keywords`) covering the
single-long-token shape.
##########
providers/apache/spark/src/airflow/providers/apache/spark/hooks/spark_submit.py:
##########
@@ -55,6 +55,29 @@
_K8S_WAIT_APP_COMPLETION_CONF = "spark.kubernetes.submission.waitAppCompletion"
+# Values to mask are anchored at a token boundary: without the lookbehind the
leading
+# \S*? retries at every offset in the string, which is what made masking
pathologically
+# slow on long arguments and log lines. Anchoring does not make this strictly
O(n) -- a
+# token packing many "secret"/"password" occurrences still backtracks
quadratically --
+# but it removes the retry-per-offset factor and is orders of magnitude faster
in
+# practice. A quote only closes the value when whitespace or the end of the
string
+# follows it, so quoted values may themselves contain quotes.
+_SENSITIVE_VALUE_RE = re.compile(
+ r"(?<!\S)(\S*?(?:secret|password)\S*?(?:=|\s+))"
+ r"(?:'((?:[^']|'(?!\s|$))*)'|\"((?:[^\"]|\"(?!\s|$))*)\"|(\S*))",
Review Comment:
Good catch, applied. Added
`test_masks_passwords_does_not_swallow_following_lines` using your exact
example as a regression test.
One note: the `old:` / `new:` labels in your example look swapped. I
reproduced it before changing anything — `password="******"tail` (the swallow)
is what the *current* pattern produces, and your `\n` exclusion is what yields
the correct `password=******\nERROR: job failed\n--other=1 "tail`. Same
conclusion, just flagging it so nobody reads it as a regression.
--
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]
