Deprecation warning for airflow.sdk.execution_time.secrets_masker points at the wrong module for mask_secret
- Dominant language
- Python
- Stars
- 46.9k
- Forks
- 17.8k
- Avg merge
- 2d 9h
- Merged PRs (30d)
- 472
Description
### Apache Airflow version
3.2.2 (apache-airflow-task-sdk 1.2.2). Also present on `main` at 79fa2e622218138f1638fe3a225156e254b3e748.
### If "Other Airflow 2/3 version" selected, which one?
_No response_
### What happened?
The deprecation shim at `airflow.sdk.execution_time.secrets_masker` tells you to migrate to `airflow.sdk._shared.secrets_masker`. For `mask_secret` its own code deliberately does something else, and the two are not equivalent: `airflow.sdk.log.mask_secret` also forwards the secret to the supervisor, `airflow.sdk._shared.secrets_masker.mask_secret` does not. Following the warning silently drops supervisor masking.
The warning says `_shared` ([L33-L37](https://github.com/apache/airflow/blob/79fa2e622218138f1638fe3a225156e254b3e748/task-sdk/src/airflow/sdk/execution_time/secrets_masker.py#L33-L37)):
```python
warnings.warn(
"Importing from 'airflow.sdk.execution_time.secrets_masker' is deprecated and will be "
"removed in a future version. Please use 'airflow.sdk._shared.secrets_masker' instead.",
DeprecatedImportWarning,
stacklevel=2,
)
```
The code hands you `airflow.sdk.log` ([L43-L47](https://github.com/apache/airflow/blob/79fa2e622218138f1638fe3a225156e254b3e748/task-sdk/src/airflow/sdk/execution_time/secrets_masker.py#L43-L47)):
```python
def __getattr__(name: str):
if name == "mask_secret":
from airflow.sdk.log import mask_secret
return mask_secret
```
Confirmed by object identity on 3.2.2:
```
shim.mask_secret is airflow.sdk.log.mask_secret -> True
shim.mask_secret is airflow.sdk._shared.secrets_masker.mask_secret -> False
```
That special case is correct, and it is load bearing. `airflow.sdk.log.mask_secret` registers with the shared masker **and** notifies the supervisor ([log.py L250-L266](https://github.com/apache/airflow/blob/79fa2e622218138f1638fe3a225156e254b3e748/task-sdk/src/airflow/sdk/log.py#L250-L266)):
```python
def mask_secret(secret: JsonValue, name: str | None = None) -> None:
"""Mask a secret in both task process and supervisor process. ..."""
_secrets_masker().add_mask(secret, name)
with suppress(Exception):
from airflow.sdk.execution_time import task_runner
from airflow.sdk.execution_time.comms import MaskSecret
if comms := getattr(task_runner, "SUPERVISOR_COMMS", None):
comms.send(MaskSecret(value=secret, name=name))
```
The `_shared` one the warning recommends stops at the first line ([secrets_masker.py L92-…](https://github.com/apache/airflow/blob/79fa2e622218138f1638fe3a225156e254b3e748/shared/secrets_masker/src/airflow_shared/secrets_masker/secrets_masker.py#L92)); there is no `SUPERVISOR_COMMS` reference anywhere in that module. Under AIP-72 the supervisor is a separate process, so a secret registered only through `_shared` can still appear in supervisor log output.
The failure is silent. Both modules import, both expose `mask_secret`, both accept the call and return `None`. A test asserting the symbol is importable and callable passes against either one, including the one that misses the supervisor.
### What you think should happen instead?
The warning text and the module docstring should name `airflow.sdk.log` for `mask_secret`, matching what `__getattr__` already does. Either carve out the exception in the message, or drop the special case so the message becomes true.
The shim came in with https://github.com/apache/airflow/pull/54915, added defensively after https://github.com/apache/airflow/pull/54449 removed the path, so this reads as incidental rather than intended.
### How to reproduce
```python
import warnings
warnings.simplefilter("ignore")
import airflow.sdk.execution_time.secrets_masker as shim
import airflow.sdk._shared.secrets_masker as shared
import airflow.sdk.log as sdklog
print(shim.mask_secret is sdklog.mask_secret) # True, what the shim gives you
print(shim.mask_secret is shared.mask_secret) # False, what the warning tells you to use
```
### Operating System
macOS 15 (not OS specific)
### Versions of Apache Airflow Providers
_No response_
### Deployment
Other
### Deployment details
_No response_
### Anything else?
Two engineers migrating the same codebase independently read the deprecation message, moved to `airflow.sdk._shared.secrets_masker`, and ended up with weaker masking than they started with. Neither noticed, because every wrong answer in this area imports and runs fine. The human readable guidance is the only signal most people act on, and right now it points away from the implementation the shim itself chose.
### Are you willing to submit PR?
- [X] Yes I am willing to submit a PR!
### Code of Conduct
- [X] I agree to follow this project's [Code of Conduct](https://github.com/apache/airflow/blob/main/CODE_OF_CONDUCT.md)
Contributor guide
Research direction
Start with task-sdk/src/airflow/sdk/execution_time/secrets_masker.py, especially the deprecation warning and __getattr__, then compare airflow.sdk.log.mask_secret with shared/secrets_masker/src/airflow_shared/secrets_masker/secrets_masker.py. Run the reproduction snippet from the issue to confirm the symbol identity. Done means the warning and module docstring direct mask_secret users to airflow.sdk.log without removing the existing special case.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend, security
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 75/100