Skip to content

fix(docker): disable AddPreviousOutputInEnv on security-report#305

Merged
Cre-eD merged 1 commit into
mainfrom
fix/security-report-replace-on-changes
May 30, 2026
Merged

fix(docker): disable AddPreviousOutputInEnv on security-report#305
Cre-eD merged 1 commit into
mainfrom
fix/security-report-replace-on-changes

Conversation

@Cre-eD

@Cre-eD Cre-eD commented May 30, 2026

Copy link
Copy Markdown
Contributor

Root-cause fix for the post-v2026.5.43 E2BIG failure observed in integrail/baas run 26690935574.

What we saw

SC v2026.5.43 deploy retry on baas — tempfile staging from #303 was in effect (Create was the short sh '/tmp/sc-security-report-…sh', ~80 bytes), yet kernel still returned E2BIG. Pulumi diff showed an in-place update on command:local:Command::security-report-baas--test/baas and a coincidental provider URN upgrade (default → default_1_2_1). My first instinct was the migration was carrying old state somewhere — wrong.

Actual root cause

Reading pulumi-command/provider/pkg/provider/local/commandOutputs.go::run():

if in.AddPreviousOutputInEnv == nil || *in.AddPreviousOutputInEnv {
    if out.Stdout != "" {
        cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s",
            util.PulumiCommandStdout, out.Stdout))
    }
    if out.Stderr != "" { ... }
}

AddPreviousOutputInEnv defaults to true. On every Update, the provider injects the previous run's full stdout as a single PULUMI_COMMAND_STDOUT=<…> env var.

The security-report script writes the rendered vulnerability table (thousands of CVE rows from Trivy + Grype merged) to stdout for inline build-log visibility — on a chrome-base-derived image, ~150-200 KB. pulumi-command captures that into the resource's Stdout output. On the next Update, kernel ARG_MAX (~128 KB on Linux, covering argv + envp combined) trips at fork/exec — even though our argv is the ~80-byte sh '/tmp/...'.

The earlier suspect (provider migration) was a red herring. Update path reads news.Create, not olds.Create:

// commandController.go
func (c *Command) Update(ctx, req) {
    cmd := news.Create   // not olds.Create
    err := run(ctx, *cmd, state.BaseInputs, ...)
}

Empirical signature this matches

  • scan-baas--test/baas-merged Update succeeded before security-report failed. Both used the same provider migration; the difference is scan-merged's stdout is a short summary (~few KB).
  • security-report-baas--test/baas Update failed with E2BIG. Its stdout is the full ~150 KB table.
  • Argv shown in the error is short. The bloat is envp, not argv.

Fix

AddPreviousOutputInEnv: sdk.Bool(false) on the security-report CommandArgs. The security script doesn't read PULUMI_COMMAND_STDOUT, so disabling the env injection is a no-op for script behaviour. Stdout is still captured into Pulumi state for pulumi stack output and downstream references — just not re-injected into envp.

Why this is better than ReplaceOnChanges + DeleteBeforeReplace

The first draft of this PR used those options. They happen to work (replace creates a fresh resource with empty out.Stdout, so the env injection has nothing to pass) — but for the wrong reason:

  1. Replace cycle every time report content changes (every deploy with new CVEs) is wasteful churn.
  2. Doesn't address the actual root cause; if pulumi-command ever passes state by other means we'd regress.
  3. Replace-vs-update in operator-visible Pulumi diffs is noise.

AddPreviousOutputInEnv: false is surgical and pinpoints the real issue.

Blast radius

Audited every local.NewCommand site in SC's docker package:

Resource Stdout size Risk Action
security-report-<image> Unbounded (rendered CVE table) HIGH Fix in this PR
scan-<image>-merged / scan-<image>-<tool> Bounded (~few KB summary) Low Worth defense-in-depth follow-up
registry-login-<image> ~zero Safe
sign-<image> / verify-<image> / sbom-<image> / provenance-<image> (via newSecurityCommand) Bounded (cosign/syft output) Low Worth defense-in-depth follow-up

Defense-in-depth across all security commands is a follow-up; this PR addresses the bug that's currently breaking deploys.

Test plan

  • go build ./pkg/clouds/pulumi/docker/... clean
  • go test ./pkg/clouds/pulumi/docker/... — all 9 existing tests pass
  • go vet ./pkg/clouds/pulumi/docker/... clean
  • Read pulumi-command source to confirm root cause
  • Cross-reference scan-merged success vs security-report failure to validate the stdout-size hypothesis
  • CI on this PR
  • Release + install-sc bump + retry baas deploy 26690935574 → confirm security-report Pulumi resource completes

@github-actions

github-actions Bot commented May 30, 2026

Copy link
Copy Markdown

Semgrep Scan Results

Repository: api | Commit: 3c479e0

Check Status Details
✅ Semgrep Pass 0 total findings (no error/warning)

Scanned at 2026-05-30 21:08 UTC

@github-actions

github-actions Bot commented May 30, 2026

Copy link
Copy Markdown

Security Scan Results

Repository: api | Commit: 3c479e0

Check Status Details
✅ Secret Scan Pass No secrets detected
✅ Dependencies (Trivy) Pass 0 total (no critical/high)
✅ Dependencies (Grype) Pass 0 total (no critical/high)
📦 SBOM Generated 527 components (CycloneDX)

Scanned at 2026-05-30 21:08 UTC

Root cause for the post-v2026.5.43 E2BIG (run 26690935574 in
integrail/baas).

## Wrong diagnosis (initial version of this PR)

I originally hypothesised that the pulumi-command provider's `default
→ default_1_2_1` migration was passing the OLD serialised Create
through fork/exec, and proposed `ReplaceOnChanges` + `DeleteBeforeReplace`
as the fix. Reading pulumi-command's source carefully refutes that:

  // commandController.go
  func (c *Command) Update(ctx, req) {
    ...
    cmd := news.Create        // uses NEW Create, not olds.
    ...
    err := run(ctx, *cmd, state.BaseInputs, ...)
  }

The Update path runs the *new* Create. The migration carries no old
Create through fork/exec.

## Actual root cause

`pulumi-command/provider/pkg/provider/local/commandOutputs.go::run()`:

  if in.AddPreviousOutputInEnv == nil || *in.AddPreviousOutputInEnv {
    if out.Stdout != "" {
      cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s",
        util.PulumiCommandStdout, out.Stdout))
    }
    if out.Stderr != "" {
      cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s",
        util.PulumiCommandStderr, out.Stderr))
    }
  }

`AddPreviousOutputInEnv` **defaults to TRUE**. On every Update,
pulumi-command appends the previous run's stdout to envp as a single
`PULUMI_COMMAND_STDOUT=<...>` entry.

The security-report script writes the full rendered vulnerability
table to stdout for inline build-log visibility — on a chrome-base-
derived image that's a 150-200 KB markdown table (thousands of CVE
rows from Trivy + Grype merged). pulumi-command captures that into
the resource's `Stdout` output. On the next Update the captured
stdout is re-injected as a single envp entry. Kernel ARG_MAX (~128
KB on Linux) covers argv+envp combined → exec fails with E2BIG even
though our argv is now ~80 bytes thanks to tempfile staging.

This explains the empirical observations:
- `scan-baas--test/baas-merged` succeeded — stdout is a short summary.
- `security-report-baas--test/baas` failed — stdout is the full table.
- Argv visible in the error is short (~80 bytes), but envp is the bloat.
- The "migration" appearance in the diff was a coincidence — provider
  upgrades changed identifier-style but didn't change envp semantics.

## Fix

Set `AddPreviousOutputInEnv: sdk.Bool(false)` on the security-report
resource. The security scripts don't read `PULUMI_COMMAND_STDOUT`, so
disabling the env injection is a no-op for script behaviour. Stdout is
still captured into Pulumi state for `stack output` / downstream
references, just not re-injected into envp.

## Why not `ReplaceOnChanges` + `DeleteBeforeReplace`

Initial draft used those options. They *happen* to work (replace creates
a fresh resource with empty `out.Stdout`, so the env injection has
nothing to pass), but for the wrong reason:

- The replace path is wasteful — every change to the report content
  (i.e. every deploy with a new CVE set) replaces the resource.
- It doesn't address the actual root cause; if pulumi-command ever
  starts passing state by other means we'd hit a regression.
- Pulumi state churn (replace vs update) clutters operator-visible diff.

`AddPreviousOutputInEnv: false` is surgical and addresses the actual
problem.

## Tests

Existing 9 tests still pass — this change only affects Pulumi resource
options, not the helper's behaviour. The envp injection path is
exercised end-to-end on the next baas deploy retry after this fix
releases.

## Blast radius

Audited every `local.NewCommand` site in SC:

- `security-report-<image>` — bug manifests here. Fix in this PR.
- `scan-<image>-merged` / `scan-<image>-<tool>` — `createScanLocalCommand`.
  Stdout is bounded (Trivy/Grype's summary output ~few KB). Not at
  risk, but inconsistent default. Worth setting to false too for
  defense-in-depth, in a follow-up PR after we validate this lands.
- `registry-login-<image>` — short fixed-size output. Safe.
- `sign-<image>` / `verify-<image>` / `sbom-<image>` / `provenance-<image>`
  via `newSecurityCommand` — cosign/syft output bounded. Safe.

Out of scope for this PR but tracked.

Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
@Cre-eD
Cre-eD force-pushed the fix/security-report-replace-on-changes branch from 1fa5818 to 20d1226 Compare May 30, 2026 21:07
@Cre-eD Cre-eD changed the title fix(docker): force replace on security-report Create change fix(docker): disable AddPreviousOutputInEnv on security-report May 30, 2026
@Cre-eD
Cre-eD merged commit 4d37dca into main May 30, 2026
21 checks passed
@Cre-eD
Cre-eD deleted the fix/security-report-replace-on-changes branch May 30, 2026 21:32
Cre-eD added a commit that referenced this pull request May 30, 2026
The 28-line block on a 1-line directive — the embedded pulumi-command
source snippet + multi-paragraph context — was disproportionate. Reduced
to a 5-line summary that captures WHY (ARG_MAX from re-injected stdout)
and the safety claim (scripts don't read PULUMI_COMMAND_STDOUT). Full
context lives in PR #305's commit message + body for future readers.

Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants