fix: resolve env-provider secrets in parent before subprocess filtering#1853
fix: resolve env-provider secrets in parent before subprocess filtering#1853
Conversation
…ng (#1852) The env whitelist introduced in v2.3.3 filters os.Environ() before passing it to agent subprocesses. This breaks env-provider secrets that reference non-whitelisted variables, since os.LookupEnv in the child only sees the filtered set. Pre-resolve env-provider secret source variables in the parent process and transport them via _DAGU_PRESOLVED_SECRET_<KEY>=<value> prefixed env vars. The env resolver checks for these prefixed vars before its normal os.LookupEnv fallback. The prefixed vars are filtered out of EnvScope so they never leak to step commands.
📝 WalkthroughWalkthroughThe PR implements a pre-resolution mechanism for environment-variable-based secrets to restore functionality broken in version 2.3.3. Env vars prefixed with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/runtime/subcmd.go (1)
169-209:⚠️ Potential issue | 🔴 CriticalWorker handler should extract and pass secretHints to TaskStart.
The
EnqueueandDequeuemethods correctly useb.env()without presolved secrets—they only queue/dequeue without executing subprocesses. However,TaskStartis called withnilsecretHints inhandler.go:90even though the worker hastask.Definition(the full DAG YAML) available.The coordinator handler demonstrates the pattern at lines 272 and 357: load the DAG from
task.Definitionusingspec.LoadYAML(), then extractdag.Secretsto pass assecretHints. When a DAG with env-provider secrets is enqueued and later started by a worker lacking access to the original env vars,preResolveEnvSecrets(nil)will fail to resolve them. UpdatebuildCommandSpecto load the DAG definition and passsecretHintswhen callingTaskStartandTaskRetry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/runtime/subcmd.go` around lines 169 - 209, buildCommandSpec currently calls TaskStart/TaskRetry with nil secretHints causing env-provider secrets to remain unresolved; load the DAG from the task definition (use spec.LoadYAML(task.Definition)) inside buildCommandSpec, extract dag.Secrets, and pass that as the secretHints argument to TaskStart and TaskRetry (mirror the coordinator pattern used around the coordinator handler examples). Ensure you handle LoadYAML errors and propagate them appropriately before calling TaskStart/TaskRetry so preResolveEnvSecrets receives the correct dag.Secrets.
🧹 Nitpick comments (1)
internal/cmn/eval/envscope.go (1)
14-16: Consider adding verification thatinternalSecretPrefixstays synchronized withsecrets.PresolvedEnvPrefix.The constants currently match and the duplication is necessary to avoid circular imports. However, if
secrets.PresolvedEnvPrefixever changes, this constant must be updated manually. A build-time or test-time verification would help catch drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmn/eval/envscope.go` around lines 14 - 16, The duplicated constant internalSecretPrefix can drift from secrets.PresolvedEnvPrefix; add an automated check that fails CI if they differ by creating a test (e.g., TestPresolvedEnvPrefixSync) in a neutral test file that does not import either package but reads the two source files (the file that defines secrets.PresolvedEnvPrefix and internal/cmn/eval/envscope.go), extracts the literal string values (via regexp or simple parsing), and asserts they are identical so any change to one will break the test and force sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/runtime/subcmd.go`:
- Around line 169-209: buildCommandSpec currently calls TaskStart/TaskRetry with
nil secretHints causing env-provider secrets to remain unresolved; load the DAG
from the task definition (use spec.LoadYAML(task.Definition)) inside
buildCommandSpec, extract dag.Secrets, and pass that as the secretHints argument
to TaskStart and TaskRetry (mirror the coordinator pattern used around the
coordinator handler examples). Ensure you handle LoadYAML errors and propagate
them appropriately before calling TaskStart/TaskRetry so preResolveEnvSecrets
receives the correct dag.Secrets.
---
Nitpick comments:
In `@internal/cmn/eval/envscope.go`:
- Around line 14-16: The duplicated constant internalSecretPrefix can drift from
secrets.PresolvedEnvPrefix; add an automated check that fails CI if they differ
by creating a test (e.g., TestPresolvedEnvPrefixSync) in a neutral test file
that does not import either package but reads the two source files (the file
that defines secrets.PresolvedEnvPrefix and internal/cmn/eval/envscope.go),
extracts the literal string values (via regexp or simple parsing), and asserts
they are identical so any change to one will break the test and force sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 03a12b98-b07c-4db4-8cf2-a02b0a11333d
📒 Files selected for processing (8)
internal/cmn/eval/envscope.gointernal/cmn/eval/envscope_test.gointernal/cmn/secrets/env.gointernal/cmn/secrets/env_test.gointernal/runtime/presolved_secrets_test.gointernal/runtime/subcmd.gointernal/runtime/subcmd_test.gointernal/service/worker/handler.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1853 +/- ##
==========================================
+ Coverage 69.29% 69.36% +0.06%
==========================================
Files 434 434
Lines 52364 52378 +14
==========================================
+ Hits 36288 36331 +43
+ Misses 12966 12946 -20
+ Partials 3110 3101 -9
... and 13 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
1 participant
Summary
preResolveEnvSecrets()which looks up env-provider secret source variables in the parent process and passes them to the subprocess via_DAGU_PRESOLVED_SECRET_<KEY>transport env vars_DAGU_PRESOLVED_SECRET_*vars fromEnvScopeso they never leak into step environmentsTaskStartandTaskRetrysignatures to accept optional secret hints for distributed worker modeTesting
make test TEST_TARGET=./internal/cmn/eval/... -count=1make test TEST_TARGET=./internal/cmn/secrets/... -count=1make test TEST_TARGET=./internal/runtime/... -count=1Closes #1852
Summary by CodeRabbit
Release Notes