Strip stale provider defaults from inputs during Diff

[kmosher]

Summary

Strip stale provider defaults from inputs during Diff and Update to prevent validation errors when provider upgrades remove a default value or remove a field from the schema entirely.

Background

How Terraform state upgrades work

When a Terraform provider changes its resource schema, it bumps a SchemaVersion number and registers StateUpgraders — functions that migrate old state representations to the new format. For example, terraform-provider-aws v6 set auth_token_update_strategy: "ROTATE" as a default on ElastiCache ReplicationGroup. In the v6→v7 upgrade, a state upgrader (v2→v3) was added to remove this field from state when auth_token isn't configured.

The Pulumi bridge invokes these state upgraders automatically via the Terraform SDK's gRPC UpgradeResourceState method. When the bridge loads old state from the Pulumi state store, it checks the schema_version in __meta, and if it's lower than the current schema version, the upgraders run and clean up the state. This part works correctly.

The gap: state vs inputs

In Pulumi, each resource has two stored representations:

• Outputs (state): the last known cloud state, stored with __meta containing schema_version
• Inputs: the last known desired configuration, stored with __defaults listing which values were injected by the provider rather than set by the user

The Terraform state upgrade mechanism only operates on outputs/state. There is no equivalent mechanism for inputs. When a provider changes or removes a default value, the state upgrader cleans it from the outputs, but the stale default persists in inputs via __defaults.

How this causes failures during refresh

During pulumi up --refresh, the engine calls the provider's Diff method as part of the refresh phase. In this call:

• olds = old outputs → the bridge creates the TF prior state from this, running state upgraders (which remove the stale field) ✓
• news = old inputs → the bridge creates the TF config from this, but no upgraders run on inputs ✗

Normally, refresh reuses the original provider version that created the resource, however when --run-program is added, problems arise. This is an engine bug tracked in pulumi/pulumi#19922.

The stale default (e.g. authTokenUpdateStrategy: "ROTATE") is still in the old inputs under __defaults. The bridge converts it into the TF config. PlanResourceChange then runs CustomizeDiff validation, which checks diff.GetRawConfig() (the config, not the state). The v7 provider's validation sees auth_token_update_strategy in the config without auth_token and rejects it:

"auth_token_update_strategy": "auth_token" must be specified

The --run-program flag doesn't help because it only provides clean program inputs for the update diff — the refresh diff (which fires first) still uses old stored inputs.

This is a widespread problem

Default changes happen routinely across all major Terraform providers, in both major and minor versions. A survey of the three largest bridged providers shows the scale:

Defaults removed — the original failure case, where a stale value reaches PlanResourceChange and fails validation:

Provider	Resource	Field	Change
AWS	ElastiCache ReplicationGroup	auth_token_update_strategy	Default: "ROTATE" removed in v6 (the original bug)
AWS	Route53 Resolver Rule	port, protocol	Default: 53 / Default: "Do53" removed, switched to Optional+Computed (#46928)
AWS	ASG instance_refresh	max_healthy_percentage	Default: 100 removed — API treats omitted vs 100 differently (#47188)
Azure	Redis Cache	data_persistence_authentication_method	Default removed, was wrong for Basic/Standard SKUs (#27069)
GCP	Compute Instance Template	disk.type, disk.mode, disk.interface	Hardcoded defaults removed, API handles them (#24055)

Defaults changed (e.g. Azure LB sku going Basic → Standard, GCP disable_on_destroy going true → false) — these silently preserve the stored old value across the upgrade. This PR intentionally does not change that behavior; see "Why these scope decisions" below for the rationale and the architectural follow-up that addresses it.

Hardcoded → API-computed — providers increasingly replace Default: X with Optional+Computed, letting the API determine the value. When the field is removed from the schema (no Default and no schema entry), this PR strips the stale value, matching the field-removed case above.

The current universe of defaults is large: Azure alone has 548 files with Default: values, 197 files with StateUpgraders, and 167 files with CustomizeDiff validators. Most default changes happen without state upgraders — providers rely on PlanResourceChange to handle it, which works for Terraform but breaks for Pulumi's stored __defaults.

The fix

In the Diff and Update methods, before building the TF config from news, strip fields listed in __defaults that have neither a bridge-managed default nor a current TF schema default. Those values are genuinely stale — the schema no longer has any way to derive them — and would otherwise reach PlanResourceChange and could trigger validation errors.

The strip predicate is deliberately narrow:

Field's __defaults entry has...	Action	Why
Bridge default (auto-naming, EnvVars, From)	preserve	Bridge owns the default; Check re-derives it. Stripping would break auto-naming stability.
Current TF schema Default (changed or unchanged)	preserve	Avoids the changed-default phantom diff (see below) and maintains the legacy-stack falsy round-trip from PR #3420.
Removed/Deprecated marker on the field (SchemaInfo.Removed, TF Removed, or TF Deprecated && !Required)	strip	Mirrors applyDefaults' eligibility gate: if the field would not be defaulted by Check, it must not be forwarded to PlanResourceChange.
No bridge default AND no current TF Default	strip	The recorded value is genuinely stale — the field's Default was removed, or the field itself was removed from the schema. This is the case that fails validation today.

The shared eligibility gate defaultExcluded(sch, psi) lives in pkg/tfbridge/schema.go and is called from applyDefaults' overlay branch, applyDefaults' TF branch, and shouldStripStaleDefault in provider.go. Keeping the parity invariant in one place makes future schema-marker additions land in a single function.

The strip recurses into nested objects and array elements — including TypeSet elements — since each nested block can carry its own __defaults with independently-stale entries. Stripping a stale field from a Set element does change the element's content hash, so the next plan surfaces a one-time membership rearrangement. That mirrors what terraform apply would show against the same provider schema change and is preferable to silently retaining a value the current schema can no longer attribute to a default. For Set fields whose elements back identity-keyed cloud objects (security-group rules, IAM bindings, ALB listener rules), the rearrangement may translate to a delete-then-create at the API layer; this is a one-time effect that lands the stack in a stable state matching the v2 schema, after which subsequent plans show no further churn.

Known limitations

• Changed TF schema default (v1 → v2): the field is preserved with its stale v1 value because the schema still declares a Default. Users silently keep the old default until they explicitly set the new value in their program. The architectural follow-up in #3434 resolves this.

Test plan

• Verified end-to-end against a real @pulumi/aws v6→v7 upgrade: built a custom pulumi-resource-aws with this bridge change and ran pulumi up --refresh --run-program on a stack with an ElastiCache ReplicationGroup that had authTokenUpdateStrategy: "ROTATE" in __defaults — the error is gone
• Unit tests in pkg/tfbridge/strip_stale_defaults_test.go covering: top-level strip/preserve classification (TF Default present/absent/removed/deprecated, bridge Default with Value/EnvVars/From/ComputeDefault, the parity carve-outs for Required-and-Deprecated and bridge Removed shadowing TF Default); recursion through nested objects, TypeList-of-blocks, MaxItemsOne blocks, secret-wrapped objects/arrays/scalars including fully-secret-wrapped Set arrays; TypeSet element stripping (both array-shaped and MaxItemsOne-flattened); deep nesting (object → array → object); top-level and nested simultaneous strip; non-mutation of the original map; __defaults listing a key absent from news; non-string __defaults entries
• Integration tests in pkg/tests/strip_stale_defaults_integration_test.go exercising the runtime SDKv2 paths through pulcheck: default-removed, field-removed-from-schema, nested TypeListBlock, TypeListOfBlocks, BridgeDefaultPreserved, stale-default-in-Set-element lifecycle (verifies Up succeeds, the strip lands in stable state, and a follow-up preview reports no further churn), TypeSet hash stability for the still-declared-default case, and a regression guard for strip symmetry across the Diff and Update RPCs
• Updated TestRegress1020 and TestRegressAws2352 to include SchemaInfo.Default for auto-named fields, matching real provider behavior
• Verified TestUpdatePreservesLegacyFalsyTFDefaults (the PR Preserve required falsy default config values #3420 invariant) continues to pass — the strip predicate preserves any field whose schema declares a Default, so legacy-stack falsy-default round-trip semantics are unaffected

Alternatives considered

Run state upgraders on inputs too — Conceptually the most "correct" fix: if inputs had the same upgrade pipeline as outputs, the problem vanishes. But state upgraders expect the full output shape (all computed fields, IDs, etc.) and inputs are a subset. Feeding inputs through UpgradeResourceState would fail on missing fields or corrupt data. A separate "input upgrader" mechanism doesn't exist in Terraform and would be a bridge-only invention.

Engine fix: don't pass old inputs as news during refresh diff — The most architecturally clean fix. The refresh diff currently sends news = old stored inputs, but it could use program inputs (from --run-program) or skip calling the provider's Diff entirely for the refresh comparison (compute it at the engine level by comparing property maps). This fixes the root cause — stale inputs never reach the provider. But it requires Pulumi engine changes (not bridge). The engine bug is tracked by pulumi/pulumi#19922. However, even if we fix the issue there, we probably still want this default cleanup function as the first piece of #3434.

Store schema_version on inputs (parallel to __meta on outputs) — If inputs tracked their schema version, the bridge could detect stale inputs and run cleanup. But this requires a new metadata field with state migration for all existing stacks, and there's still no upgrader to run on inputs.

This PR: strip stale defaults in Diff and Update — Targeted, backward-compatible, bridge-only. Uses metadata that already exists (__defaults) to identify provider-injected values and strips them so PlanResourceChange can re-determine them with the current schema. This fix would still be useful as defense-in-depth even if the engine fix lands later.

Why these scope decisions

Why apply the strip in both Diff and Update? Diff is the user-visible failure path: the engine's refresh phase calls Diff with stored inputs as news, and that is where stale values reach the provider unfiltered. The strip is also applied in Update for symmetry — both RPCs feed PlanResourceChange, both receive news from Pulumi's engine, and both should see the same shape. In current code Check sanitizes news upstream of Update, so the Update-path strip is defense-in-depth for paths where Check is bypassed (custom state-edit hooks, future RPC additions, refresh flows that don't pass through Check).

Why preserve fields whose Default just changed (v1 → v2)? The strip predicate intentionally treats "schema has any current Default or DefaultFunc" as preserve, even when the value changed across versions. Two reasons:

1. The legacy-stack falsy round-trip invariant from PR #3420. That invariant relies on stored falsy values for fields whose schema has a matching Default reaching RawConfig as cty.False/0/"" rather than null — providers read RawConfig presence as meaningful. A broader strip would regress this.
2. The deeper "old default" reuse path. Even if the strip removed a changed-default field from news, applyDefaults's overlay branch reads olds[__defaults] (not news) and re-injects the stored old value via the "old default" reuse path. So a strip in this case would be silently undone for any non-empty SchemaInfo.Default overlay. The proper fix is to stop tracking TF schema defaults in __defaults at all — the architectural follow-up below.

A user who wants to migrate to a new default value can set it explicitly in their program; Check will then record it as user-set rather than provider-defaulted.

Why not run state upgraders on inputs? Upgraders expect the full output shape (computed fields, IDs) and would corrupt the input subset. No Terraform-side mechanism does what we'd need.

Architectural follow-up (not in this PR)

The cleanup that fully resolves the changed-default case: applyDefaults should stop tracking TF schema defaults in __defaults at all. Then Check would re-derive TF defaults from the current schema each call, the "old default" reuse path would only fire for bridge-managed defaults (auto-naming, EnvVars, From), and a changed default would simply produce the new value in news — no stripping needed at runtime, no phantom diff. The strip in this PR remains as the migration path for legacy stacks with already-stored stale entries.

PR #3398 took the first step (suppressing fresh falsy TF defaults from being materialized) but explicitly held off on the broader change pending evidence that it is safe across the full SDKv2 lifecycle (validation interactions, GetRawConfig semantics, provider Configure with Required + DefaultFunc, data source Invoke). Tracked as #3434 and referenced by a TODO near stripStaleDefaults.

[codecov]

Codecov Report

❌ Patch coverage is 96.77419% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.15%. Comparing base (cb30acd) to head (952b215).
⚠️ Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/tfbridge/provider.go	96.46%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3405      +/-   ##
==========================================
+ Coverage   70.06%   70.15%   +0.08%     
==========================================
  Files         343      343              
  Lines       37164    37276     +112     
==========================================
+ Hits        26040    26152     +112     
+ Misses       9370     9369       -1     
- Partials     1754     1755       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Review summary

The fix is logically sound for the described scenario (provider upgrade removes a Default from a schema field, stale value persists in __defaults, and the stale value later causes PlanResourceChange validation to fail). The call site placement in Diff—before MakeTerraformConfig—is correct, and the shallow-copy approach avoids mutating the caller's map.

Blocking

• stripStaleDefaults has no unit tests. The PR description acknowledges this ("Need to add unit tests"), but they must be present before this merges given the function lives on every Diff hot path. See inline comment on the function declaration for a suggested test matrix.

Non-blocking observations (inline comments posted)

• tfSchema == nil edge case: fields completely removed from the TF schema (not just their default removed) are silently kept rather than stripped. Likely harmless today but worth a comment or an explicit decision.
• Consider adding the same stripping in Check if it can ever be called with stored (rather than program) inputs.
• A glog.V(9) log when keys are stripped would greatly aid future debugging.

Reviewed by Internal Trusted PR Reviewer

To install this agentic workflow, run

gh aw add pulumi-labs/gh-aw-internal/.github/workflows/gh-aw-pr-review.md@99a309bf18258dbae98c8176ea10657e0b08deca

[guineveresaenger]

I think #3398 may be related.

[guineveresaenger]

/review-again

[github-actions]

Review summary

The fix is well-motivated and correctly implemented. The core logic in stripStaleDefaults is sound:

• Only fields that appear in __defaults (bridge/TF-injected, never explicitly user-set) are candidates for stripping.
• A field is stripped only when tfSchema != nil && !hasTFDefault && !hasBridgeDefault — the tfSchema == nil guard is a safe bail-out since MakeTerraformConfig silently drops fully-removed fields anyway.
• The shallow-copy before mutating (result := make(resource.PropertyMap, len(m))) correctly avoids mutating the caller's map, and the unit test "does not mutate original map" directly verifies this.
• The function is called before MakeTerraformConfig and before NewInputs is passed to p.tf.Diff, so both the config and the inputs side of the diff see consistent (stripped) values.

The test modifications to TestRegress1020 and TestRegressAws2352 are necessary: both tests carry name in __defaults in their stored-input fixtures, and without SchemaInfo.Default registered, stripStaleDefaults would incorrectly classify the auto-named field as stale. Adding the bridge default preserves the original test intent.

Two minor non-blocking comments inline — no changes required to merge.

Reviewed by Internal PR Re-Review (Slash Command)

To install this agentic workflow, run

gh aw add pulumi-labs/gh-aw-internal/.github/workflows/gh-aw-pr-rereview.md@99a309bf18258dbae98c8176ea10657e0b08deca

[pgavlin]

The stale default (e.g. authTokenUpdateStrategy: "ROTATE") is still in the old inputs under __defaults. The bridge converts it into the TF config. PlanResourceChange then runs CustomizeDiff validation, which checks diff.GetRawConfig() (the config, not the state). The v7 provider's validation sees auth_token_update_strategy in the config without auth_token and rejects it:

this is interesting. so TF providers are doing validation in Diff via PlanResourceChange? that's a bit unfortunate...

this is sort of an odd interaction in general. the way we do diffs for refresh steps relies on the engine calling Diff with the last inputs passed in by the program--which in this case is exactly what's triggering the issue. I suppose TF doesn't have this problem for two reasons:

• TF doesn't run diff on refresh results
• TF doesn't persist defaults anywhere but in state

so a simple state upgrade works fine.

overall I think the approach seems sound? worth looking for some specific test cases in addition to what's here though.

[pgavlin]

wondering if this needs to recur on property values. I think default application is recursive.

[kmosher]

Good call on the recursion. Added that and tests.

[kmosher]

Review findings and analysis

Ran this through several code reviewers (Claude code-reviewer agent, Codex codex review, Gemini CLI). Sharing the findings and analysis.

Addressed in code

• Secret-wrapped nested objects — stripStaleDefaultsValue now unwraps secrets, recurses, and re-wraps. Stale defaults in sensitive blocks are correctly stripped.
• TypeMap-of-objects — Added handling for TypeMap with Elem=Resource. Map values (keyed by arbitrary user keys) are now recursed into individually.
• Performance (scalar skip) — Added early exit for non-Object/non-Array values before the getInfoFromPulumiName lookup, avoiding the O(n) schema scan for every scalar field.
• Deep nesting — Added test for object → array → object (2 levels). The existing recursive design already handles arbitrary depth correctly.

---

TypeSet hash identity

The concern

When stripStaleDefaults removes a defaulted field from a TypeSet element, the element's hash could change. TypeSet uses hash-based identity to match elements between prior state and proposed config — if the hashes differ, TF sees the old element as "deleted" and the new one as "added" rather than recognizing them as the same element being modified. This could trigger spurious force-replaces on resources with TypeSet blocks.

For example, a TypeSet element {type: "A", protocol: "tcp"} where protocol was provider-defaulted. After stripping, the config has {type: "A"}. If the hash is computed on the stripped config, it won't match the prior state hash of {type: "A", protocol: "tcp"}, and TF would treat this as a different element entirely.

Analysis

This is a false alarm. In the TF SDK v2 diff flow, schemaMap.Diff() applies defaults to config fields before set element hashes are computed. The sequence is:

1. Raw config received (with protocol absent)
2. SDK applies Default: "tcp" to fill in the missing field
3. Resolved config is {type: "A", protocol: "tcp"}
4. Hash is computed on the resolved config → matches prior state hash

So {type: "A"} with protocol stripped becomes {type: "A", protocol: "tcp"} after default application — producing the same hash as prior state. No spurious delete+add. If TF hashed raw config before applying defaults, it would also break native Terraform (where users routinely omit defaulted fields), so this ordering is well-established.

---

DefaultFunc re-derivation

The concern

Fields populated via DefaultFunc derive their values from runtime context — provider configuration, environment variables, or computed state. Common examples include GCP's project/region/zone fields, which default to the provider-level configuration via DefaultFunc.

If such a field is in __defaults and we strip it, PlanResourceChange must re-derive the value by calling DefaultFunc again. Two failure modes:

1. Environment changed: If the user had GOOGLE_PROJECT=foo when they originally deployed but now has GOOGLE_PROJECT=bar (or unset), stripping the stored project: "foo" would cause PlanResourceChange to derive a different value (or fail entirely). The old behavior (keeping the stored value) provided stability — the resource kept its original project regardless of env changes.

2. DefaultFunc context differs between Check and Plan: The bridge calls DefaultFunc during Check (when applying defaults) and TF calls it again during PlanResourceChange. If DefaultFunc behaves differently in these two contexts (different available state, different provider config), the re-derived value might not match what was originally stored.

Analysis

Not a practical concern. Provider initialization requires these values — GCP provider won't start without project configuration, AWS provider won't start without region, etc. So the values are always available during PlanResourceChange.

If the environment genuinely changed (user switched projects), showing a diff is arguably correct behavior. The field was never explicitly set by the user (it's in __defaults), so the resource should track the current provider configuration rather than preserving a stale value from a previous environment.

The narrow failure case (env var removed but provider still initializes and DefaultFunc returns nil) is extremely unlikely in practice. And even if it did happen, the resulting error ("project is required") would correctly tell the user their configuration is incomplete — better than silently preserving a stale project from a defunct environment.

---

DetailedDiff coarseness for TypeSet

The concern

stripStaleDefaults modifies news before it's passed both to MakeTerraformConfig (for TF) and as NewInputs in shim.DiffOptions (for Pulumi's detailed diff computation). The detailed diff logic in makeDetailedDiffV2 uses validInputsFromPlan to correlate planned state elements with user inputs. This matching assumes "inputs will have defaults already applied" — it compares planned state values against input values to identify which set elements correspond to each other.

If we strip defaults from inputs but TF re-applies them in the planned state, there's a mismatch: the planned state has {type: "A", protocol: "tcp"} but inputs only have {type: "A"}. The matching logic may fail to correlate these, and the bridge falls back to reporting a coarse "Update" or "UpdateReplace" for the entire set rather than showing which specific element changed.

This affects the granularity of pulumi preview output — instead of seeing per-element diffs, users would see a single "~update" for the whole set block.

Analysis

Real but low-severity and cosmetic. The impact is:

• Only affects TypeSet resources — validInputsFromPlan is specifically for matching set elements. Top-level scalar fields and TypeList fields use positional matching and are unaffected.
• Only when a stripped default participates in set matching — If the defaulted field isn't part of what validInputsFromPlan uses to correlate elements, there's no issue.
• The diff is less granular, not incorrect — The resource still shows as needing an update; the user just sees "~update set block" instead of "~update set element[0].field". No incorrect replaces or missed changes.

If this turns out to be noisy in practice, we could maintain separate property maps: stripped inputs for MakeTerraformConfig (the TF config path) and original inputs for NewInputs (the Pulumi detailed diff path). But this seems premature without reports of the issue, and the fix is straightforward if needed.

[corymhall]

Left a few non-blocking review comments around default detection and test coverage.

[corymhall]

I think this predicate should avoid calling DefaultValue().

DefaultValue() executes DefaultFunc(), and this code ignores the returned error. For schemas that still have a current DefaultFunc, especially env-backed or Required + DefaultFunc shapes, a nil/error result at Diff time would classify the stored value as stale and strip it from news. That defeats the old-default reuse path in MakeTerraformConfig and can change raw-config presence semantics.

The safer predicate seems to be schema ownership rather than evaluated value:

if tfSchema != nil && (tfSchema.Default() != nil || tfSchema.DefaultFunc() != nil) {
    keptDefaults = append(keptDefaults, key)
    continue
}

Can we also add a test where the current schema has a DefaultFunc that returns nil or errors, and verify that a stored __defaults value is preserved?

created with Codex

[kmosher]

Good catch — fixed in b7a44781. Switched the predicate to tfSchema.Default() != nil || tfSchema.DefaultFunc() != nil (structural ownership) and added a unit test field with current TF DefaultFunc is preserved (even if it would return nil) that uses a DefaultFunc returning (nil, nil) to confirm a runtime-nil result no longer classifies the field as stale.

[corymhall]

This preview result is discarded, so the test does not actually prove the “no spurious diff” behavior described above.

As written, the test would still pass if preview showed an update/replacement and Up later cleaned the stored inputs. Since avoiding the refresh/preview failure is the user-facing behavior this PR is fixing, can we capture the result and assert the change summary?

res := pt2.Preview(t, optpreview.Diff())
assertNoChanges(t, res.ChangeSummary, "DefaultRemoved")

Same pattern looks worth applying to the field-removed and nested migration tests.

created with Codex

[kmosher]

Acted on this with a small adjustment. Schema-migration scenarios (_DefaultRemoved, _FieldRemovedFromSchema, _NestedTypeListBlock, _TypeListOfBlocks) are expected to show a transitional update diff — the strip removes the stale value, so TF sees the field going from present-in-state to absent-in-config and produces an in-place update. assertNoChanges would fail there.

What would be a real bug is the strip causing a replace (e.g. via a TypeSet hash regression). Added a new helper assertOnlySameOrUpdate and applied it on the Preview result in all four schema-migration tests; the post-Up state assertion remains as the strong outcome guarantee.

[corymhall]

Can we clarify or test this shape?

This treats every TypeMap with Elem().(shim.Resource) as a map of nested objects. The existing makeTerraformInput object path treats Elem().(shim.Resource) as a single object schema when the Pulumi value is an object. If this TypeMap-of-Resource shape is impossible or not relevant for SDKv2 providers, a short comment would help. If it is possible, I think we should add a test that proves stripStaleDefaults and MakeTerraformConfig agree on the shape.

created with Codex

[kmosher]

You're right, and the answer is stronger than "add a test." Per pkg/tfshim/shim.go:175-181, TypeMap+Elem=Resource represents a single-nested block in PF, but SDKv2 reinterprets it as a string-string map. Since this strip only runs on the SDKv2 Diff path, the values would always be scalars, not nested objects — stripMapOfBlocks was handling a shape that doesn't exist for SDKv2 providers (the inner.IsObject() guard would skip every element).

Removed the TypeMap special case, deleted the stripMapOfBlocks helper, and removed the two unit tests that were exercising the fictional shape. The IsObject() branch now falls through to stripStaleDefaults directly, which would also handle PF correctly if/when this strip is extended there.