ashb commented on code in PR #54449:
URL: https://github.com/apache/airflow/pull/54449#discussion_r2284605757
##########
shared/secrets_masker/src/airflow_shared/secrets_masker/secrets_masker.py:
##########
Review Comment:
The double "secrets_masker" in the import paths bugs me.
What do you think about putting all the imports in
`shared/secrets_masker/src/airflow_shared/secrets_masker/__init__.py`
##########
shared/secrets_masker/src/airflow_shared/secrets_masker/secrets_masker.py:
##########
@@ -157,17 +148,25 @@ def merge(
return _secrets_masker().merge(new_value, old_value, name, max_depth)
-@cache
+_global_secrets_masker: SecretsMasker | None = None
+
+
def _secrets_masker() -> SecretsMasker:
- for flt in logging.getLogger("airflow.task").filters:
- if isinstance(flt, SecretsMasker):
- return flt
- raise RuntimeError(
- "Logging Configuration Error! No SecretsMasker found! If you have
custom logging, please make "
- "sure you configure it taking airflow configuration as a base as
explained at "
-
"https://airflow.apache.org/docs/apache-airflow/stable/logging-monitoring/logging-tasks.html";
- "#advanced-configuration"
- )
+ """
+ Get or create the global secrets masker instance.
+
+ This function implements a module level singleton pattern to ensure
consistent
+ secrets masking behavior across all shared Airflow dists, regardless of
import path.
+
+ Needed because:
+ - shared code can be accessed via different symlink paths (airflow._shared
vs
+ airflow.sdk._shared)
Review Comment:
This isn't an accurate comment -- the different symlink paths end up with
different global variables which isn't clear from this comment.
##########
task-sdk/src/airflow/sdk/secrets_masker.py:
##########
Review Comment:
How much of the exports here do we actually need? The `mask_secret` function
could stay in `airflow.sdk.log`, and I'm not sure anything else in here is
really part of the "public API", so my first thought is to nix this file and
anything that needs these should be using `airflow.sdk._shared.secrets_masker`
##########
airflow-core/tests/unit/jobs/test_triggerer_job.py:
##########
@@ -656,7 +656,9 @@ async def run(self, **args) -> AsyncIterator[TriggerEvent]:
from airflow.sdk import Variable
from airflow.sdk.execution_time.xcom import XCom
- from airflow.sdk.log import mask_secret
+
+ # Use sdk masker in dag processor and triggerer because those use the
task sdk machinery
+ from airflow.sdk.secrets_masker import mask_secret
Review Comment:
I was thinking that the public interface for users would stay as
`airflow.sdk.log.mask_secret` tbh.
##########
task-sdk/tests/task_sdk/log/test_log.py:
##########
@@ -112,7 +111,7 @@ def test_logs_are_masked(captured_logs):
secrets_masker = SecretsMasker()
secrets_masker.add_mask("password")
with mock.patch(
Review Comment:
WAT. Not a change in this Pr, but why are we mocking the redact function,
rather than.... letting it run?
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -54,6 +54,7 @@
from pydantic import BaseModel, TypeAdapter
from airflow.configuration import conf
+from airflow.sdk._shared.secrets_masker.secrets_masker import mask_secret
Review Comment:
We should definitely be consistent inside the project how we import this .
Compare this to context.py just above
##########
shared/secrets_masker/tests/conftest.py:
##########
@@ -0,0 +1,30 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from unittest.mock import patch
+
+import pytest
+
+
[email protected]
+def patched_secrets_masker():
+ from airflow.sdk.secrets_masker import SecretsMasker
Review Comment:
This doesn't seem like the right import for here?
--
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]
