Copilot commented on code in PR #64016:
URL: https://github.com/apache/airflow/pull/64016#discussion_r3025369012
##########
task-sdk/src/airflow/sdk/configuration.py:
##########
@@ -200,6 +203,31 @@ def load_test_config(self):
log.info("Unit test configuration loaded from 'unit_tests.cfg'")
+ def mask_secrets(self) -> None:
+ """Register sensitive configuration values with the SDK secrets
masker."""
+ from airflow.sdk.log import mask_secret
+
Review Comment:
`mask_secrets()` introduces a `from airflow.sdk.log import mask_secret`
import inside a non-test method. Per project guidelines, imports should be at
module scope unless needed for circular import avoidance/lazy
loading/TYPE_CHECKING. Please move this import to the top of the module, or add
a brief comment explaining the specific cycle/lazy-loading reason it must
remain inside the method.
##########
task-sdk/tests/task_sdk/execution_time/test_supervisor.py:
##########
@@ -232,6 +232,43 @@ def test_supervise(
with expectation:
supervise(**kw)
+ @pytest.mark.enable_redact
+ def test_supervise_remasks_config_secrets_after_reset(self):
+ """Config-level secrets survive reset_secrets_masker() inside
supervise().
+
+ Regression test for https://github.com/apache/airflow/issues/63921
+ """
+ from airflow.sdk._shared.secrets_masker import redact,
reset_secrets_masker
+ from airflow.sdk.configuration import conf
+
+ expected_secrets = {
+ ("api", "secret_key"): "shared-api-secret",
+ ("api_auth", "jwt_secret"): "jwt-super-secret",
+ }
+
+ with conf_vars(expected_secrets):
+ conf.mask_secrets()
+
+ observed_secrets = {
+ ("api", "secret_key"): conf.get("api", "secret_key",
suppress_warnings=True),
+ ("webserver", "secret_key"): conf.get("webserver",
"secret_key", suppress_warnings=True),
+ ("api_auth", "jwt_secret"): conf.get("api_auth", "jwt_secret",
suppress_warnings=True),
+ }
+
+ for secret in observed_secrets.values():
+ assert redact(secret) == "***"
+
+ reset_secrets_masker()
+ for secret in observed_secrets.values():
+ assert redact(secret) == secret, "reset_secrets_masker should
clear all patterns"
+
+ conf.mask_secrets()
+
+ for (section, key), secret in observed_secrets.items():
+ assert redact(secret) == "***", (
+ f"Config secret {section}/{key} should be masked after
conf.mask_secrets()"
+ )
Review Comment:
This regression test doesn’t actually exercise `supervise()` (it only calls
`reset_secrets_masker()` + `conf.mask_secrets()` directly). As a result, it
would still pass even if the new `conf.mask_secrets()` call in `supervise()`
were removed or moved. Consider making this an integration-style unit test by
invoking `supervise()` with `ActivitySubprocess.start` patched to avoid
spawning a subprocess, and assert (e.g., via a spy/patch) that
`conf.mask_secrets()` is called immediately after `reset_secrets_masker()`
(and/or that redaction works after `supervise()` runs).
##########
task-sdk/tests/task_sdk/execution_time/test_supervisor.py:
##########
@@ -232,6 +232,43 @@ def test_supervise(
with expectation:
supervise(**kw)
+ @pytest.mark.enable_redact
+ def test_supervise_remasks_config_secrets_after_reset(self):
+ """Config-level secrets survive reset_secrets_masker() inside
supervise().
+
+ Regression test for https://github.com/apache/airflow/issues/63921
+ """
+ from airflow.sdk._shared.secrets_masker import redact,
reset_secrets_masker
+ from airflow.sdk.configuration import conf
+
+ expected_secrets = {
+ ("api", "secret_key"): "shared-api-secret",
+ ("api_auth", "jwt_secret"): "jwt-super-secret",
+ }
+
+ with conf_vars(expected_secrets):
+ conf.mask_secrets()
+
+ observed_secrets = {
+ ("api", "secret_key"): conf.get("api", "secret_key",
suppress_warnings=True),
+ ("webserver", "secret_key"): conf.get("webserver",
"secret_key", suppress_warnings=True),
+ ("api_auth", "jwt_secret"): conf.get("api_auth", "jwt_secret",
suppress_warnings=True),
+ }
Review Comment:
This test asserts masking for `webserver/secret_key` but the `conf_vars`
overrides only set values for `api/secret_key` and `api_auth/jwt_secret`. Since
`api.secret_key` is deprecated-mapped to `webserver.secret_key`, whether
`conf.get("webserver", "secret_key")` returns the overridden value is ambiguous
and can depend on config lookup order/defaults. To make the test deterministic,
explicitly override `("webserver", "secret_key")` in `expected_secrets` (or
assert `observed_secrets[("webserver","secret_key")]` equals the expected value
before checking redaction).
--
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]
