RehanAhmad25 commented on PR #72499:
URL: https://github.com/apache/airflow/pull/72499#issuecomment-5578400903
> @RehanAhmad25 Thanks, I like the "DAGRunResponse" validator approach,
especially the fail-closed behavior when the DAG context is missing.
>
> One thing I'd verify before merging is that the "dag" passed through
"ValidationInfo.context" is always the exact DAG version associated with that
DagRun (including "bundle_version" overrides), rather than just the latest DAG
for the "dag_id".
>
> Could we add/keep a regression test where two runs have the same "dag_id"
but different DAG versions, and only one version declares "format="password""?
That would verify the response-layer redaction is using the correct Param
schema.
>
> I’d also like to hear the other reviewers opinions on this new approach
before we settle on it, since moving the redaction into the response model
changes the current design a bit.
@soupam05, Checked all 6 call sites again:
- `get_dag_run`, both branches of `get_dag_runs`, `get_list_dag_runs_batch`,
`patch_dag_run`, and `clear_dag_runs` all resolve `dag` via
`get_dag_for_run(dag_bag, dag_run, session)`, which internally keys off
`dag_run.created_dag_version_id` (falling back to latest only when a run
genuinely has no version of its own, per `DBDagBag._version_from_dag_run`).
This is exactly the bug fixed earlier in this PR: it used to be cached by
`dag_id` alone, which collapsed different versions together.
- `trigger_dag_run` uses `context_dag`, which defaults to the resolved `dag`
but gets explicitly overridden to `preloaded_dag_version.serialized_dag.dag`
whenever the trigger request specifies a version/`bundle_version` override
(source: `dag_run.py` lines 828/837). So that path is also version-correct,
including the override case.
So yes: the `dag` object available at every one of these 6 sites today is
already the correct per-run version, not a blanket latest-for-dag_id lookup.
Moving to a context-based validator wouldn't change *which* `dag` gets
resolved, only *where* the masking call happens, since we'd pass that same
already-correct `dag` into `context={"dag": dag}` at each site.
On the regression test: we already have one for exactly this scenario,
`TestGetDagRunsMasksPasswordConfAcrossVersions::test_list_masks_only_the_version_that_declares_password`,
two runs sharing a `dag_id` with different Dag versions, only the second
declaring `format="password"`, asserting only that run gets masked. It's
written against the HTTP API response, not the internal implementation, so it
should keep passing unchanged after the validator refactor and continue to
catch this specific failure mode either way.
Agreed on hearing from the other reviewers before settling on the approach,
I won't start implementing this until there's some consensus, wanted to answer
the version-correctness question concretely in the meantime.
--
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]
