stacks: don't validate input variables during the apply phase (#35561)

liamcervante · web-flow · commit bb73fb1d6997 · 2024-08-14T09:47:13.000+02:00
diff --git a/internal/stacks/stackruntime/apply_test.go b/internal/stacks/stackruntime/apply_test.go
@@ -1707,132 +1707,6 @@ func TestApplyWithStateManipulation(t *testing.T) {
 	}
 }
 
-func TestApplyWithChangedComponentValues(t *testing.T) {
-	ctx := context.Background()
-	cfg := loadMainBundleConfigForTest(t, filepath.Join("with-single-input", "valid"))
-
-	fakePlanTimestamp, err := time.Parse(time.RFC3339, "1991-08-25T20:57:08Z")
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	changesCh := make(chan stackplan.PlannedChange)
-	diagsCh := make(chan tfdiags.Diagnostic)
-	lock := depsfile.NewLocks()
-	lock.SetProvider(
-		addrs.NewDefaultProvider("testing"),
-		providerreqs.MustParseVersion("0.0.0"),
-		providerreqs.MustParseVersionConstraints("=0.0.0"),
-		providerreqs.PreferredHashes([]providerreqs.Hash{}),
-	)
-	req := PlanRequest{
-		Config: cfg,
-		ProviderFactories: map[addrs.Provider]providers.Factory{
-			addrs.NewDefaultProvider("testing"): func() (providers.Interface, error) {
-				return stacks_testing_provider.NewProvider(), nil
-			},
-		},
-		DependencyLocks: *lock,
-
-		ForcePlanTimestamp: &fakePlanTimestamp,
-
-		InputValues: map[stackaddrs.InputVariable]ExternalInputValue{
-			stackaddrs.InputVariable{Name: "input"}: {
-				Value: cty.StringVal("hello"),
-			},
-		},
-	}
-	resp := PlanResponse{
-		PlannedChanges: changesCh,
-		Diagnostics:    diagsCh,
-	}
-
-	go Plan(ctx, &req, &resp)
-	planChanges, diags := collectPlanOutput(changesCh, diagsCh)
-	if len(diags) > 0 {
-		t.Fatalf("expected no diagnostics, got %s", diags.ErrWithWarnings())
-	}
-
-	planLoader := stackplan.NewLoader()
-	for _, change := range planChanges {
-		proto, err := change.PlannedChangeProto()
-		if err != nil {
-			t.Fatal(err)
-		}
-
-		for _, rawMsg := range proto.Raw {
-			err = planLoader.AddRaw(rawMsg)
-			if err != nil {
-				t.Fatal(err)
-			}
-		}
-	}
-	plan, err := planLoader.Plan()
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	// Now deliberately edit the plan so that it'll produce different outputs.
-	plan.RootInputValues[stackaddrs.InputVariable{Name: "input"}] = cty.StringVal("world")
-
-	applyReq := ApplyRequest{
-		Config: cfg,
-		Plan:   plan,
-		ProviderFactories: map[addrs.Provider]providers.Factory{
-			addrs.NewDefaultProvider("testing"): func() (providers.Interface, error) {
-				return stacks_testing_provider.NewProvider(), nil
-			},
-		},
-		DependencyLocks: *lock,
-	}
-
-	applyChangesCh := make(chan stackstate.AppliedChange)
-	diagsCh = make(chan tfdiags.Diagnostic)
-
-	applyResp := ApplyResponse{
-		AppliedChanges: applyChangesCh,
-		Diagnostics:    diagsCh,
-	}
-
-	go Apply(ctx, &applyReq, &applyResp)
-	applyChanges, applyDiags := collectApplyOutput(applyChangesCh, diagsCh)
-	if len(applyDiags) != 1 {
-		t.Fatalf("expected exactly one diagnostic, got %s", applyDiags.ErrWithWarnings())
-	}
-
-	gotSeverity, wantSeverity := applyDiags[0].Severity(), tfdiags.Error
-	gotSummary, wantSummary := applyDiags[0].Description().Summary, "Planned input variable value changed"
-	gotDetail, wantDetail := applyDiags[0].Description().Detail, "The planned value for input variable \"input\" has changed between the planning and apply phases for component.self. This is a bug in Terraform - please report it."
-
-	if gotSeverity != wantSeverity {
-		t.Errorf("expected severity %q, got %q", wantSeverity, gotSeverity)
-	}
-	if gotSummary != wantSummary {
-		t.Errorf("expected summary %q, got %q", wantSummary, gotSummary)
-	}
-	if gotDetail != wantDetail {
-		t.Errorf("expected detail %q, got %q", wantDetail, gotDetail)
-	}
-
-	wantChanges := []stackstate.AppliedChange{
-		&stackstate.AppliedChangeComponentInstance{
-			ComponentAddr:         mustAbsComponent("component.self"),
-			ComponentInstanceAddr: mustAbsComponentInstance("component.self"),
-			OutputValues:          make(map[addrs.OutputValue]cty.Value),
-		},
-		// no resources should have been created because the input variable was
-		// invalid.
-	}
-
-	sort.SliceStable(applyChanges, func(i, j int) bool {
-		return appliedChangeSortKey(applyChanges[i]) < appliedChangeSortKey(applyChanges[j])
-	})
-
-	if diff := cmp.Diff(wantChanges, applyChanges, ctydebug.CmpOptions, cmpCollectionsSet); diff != "" {
-		t.Errorf("wrong changes\n%s", diff)
-	}
-}
-
 func TestApplyWithChangedInputValues(t *testing.T) {
 	ctx := context.Background()
 	cfg := loadMainBundleConfigForTest(t, filepath.Join("with-single-input", "valid"))
@@ -1927,7 +1801,7 @@ func TestApplyWithChangedInputValues(t *testing.T) {
 
 	go Apply(ctx, &applyReq, &applyResp)
 	applyChanges, applyDiags := collectApplyOutput(applyChangesCh, diagsCh)
-	if len(applyDiags) != 2 {
+	if len(applyDiags) != 1 {
 		t.Fatalf("expected exactly two diagnostics, got %s", applyDiags.ErrWithWarnings())
 	}
 
@@ -1937,10 +1811,7 @@ func TestApplyWithChangedInputValues(t *testing.T) {
 			tfdiags.Error,
 			"Inconsistent value for input variable during apply",
 			"The value for non-ephemeral input variable \"input\" was set to a different value during apply than was set during plan. Only ephemeral input variables can change between the plan and apply phases."),
-		expectDiagnostic(
-			tfdiags.Error,
-			"Planned input variable value changed",
-			"The planned value for input variable \"input\" has changed between the planning and apply phases for component.self. This is a bug in Terraform - please report it."))
+	)
 
 	wantChanges := []stackstate.AppliedChange{
 		&stackstate.AppliedChangeComponentInstance{
diff --git a/internal/stacks/stackruntime/internal/stackeval/component_instance.go b/internal/stacks/stackruntime/internal/stackeval/component_instance.go
@@ -17,7 +17,6 @@ import (
 	"github.com/hashicorp/terraform/internal/instances"
 	"github.com/hashicorp/terraform/internal/lang/marks"
 	"github.com/hashicorp/terraform/internal/plans"
-	"github.com/hashicorp/terraform/internal/plans/objchange"
 	"github.com/hashicorp/terraform/internal/promising"
 	"github.com/hashicorp/terraform/internal/providers"
 	"github.com/hashicorp/terraform/internal/stacks/stackaddrs"
@@ -91,7 +90,20 @@ func (c *ComponentInstance) CheckInputVariableValues(ctx context.Context, phase
 
 	// We actually checked the errors statically already, so we only care about
 	// the value here.
-	return EvalComponentInputVariables(ctx, varDecls, wantTy, defs, decl, phase, c)
+	val, diags := EvalComponentInputVariables(ctx, varDecls, wantTy, defs, decl, phase, c)
+	if phase == ApplyPhase {
+		if !val.IsWhollyKnown() {
+			// We can't apply a configuration that has unknown values in it.
+			// This means an error has occured somewhere else, while gathering
+			// the input variables. We return a nil value here, whatever caused
+			// the error should have raised an error diagnostic separately.
+			return cty.NilVal, diags
+		}
+
+		// Note, that unknown values during the planning phase are totally fine.
+	}
+
+	return val, diags
 }
 
 // inputValuesForModulesRuntime adapts the result of
@@ -104,17 +116,12 @@ func (c *ComponentInstance) CheckInputVariableValues(ctx context.Context, phase
 //
 // During the planning phase, the expectedValues should be nil, as they will
 // only be checked during the apply phase.
-func (c *ComponentInstance) inputValuesForModulesRuntime(ctx context.Context, previousValues map[string]plans.DynamicValue, phase EvalPhase) (terraform.InputValues, tfdiags.Diagnostics) {
-	var diags tfdiags.Diagnostics
-
+func (c *ComponentInstance) inputValuesForModulesRuntime(ctx context.Context, phase EvalPhase) terraform.InputValues {
 	valsObj := c.InputVariableValues(ctx, phase)
 	if valsObj == cty.NilVal {
-		return nil, diags
+		return nil
 	}
 
-	// module is the configuration for the root module of this component.
-	module := c.call.Config(ctx).ModuleTree(ctx).Module
-
 	// valsObj might be an unknown value during the planning phase, in which
 	// case we'll return an InputValues with all of the expected variables
 	// defined as unknown values of their expected type constraints. To
@@ -124,7 +131,7 @@ func (c *ComponentInstance) inputValuesForModulesRuntime(ctx context.Context, pr
 	if wantTy == cty.NilType {
 		// The configuration is too invalid for us to know what type we're
 		// expecting, so we'll just bail.
-		return nil, diags
+		return nil
 	}
 	wantAttrs := wantTy.AttributeTypes()
 	ret := make(terraform.InputValues, len(wantAttrs))
@@ -139,71 +146,8 @@ func (c *ComponentInstance) inputValuesForModulesRuntime(ctx context.Context, pr
 			Value:      v,
 			SourceType: terraform.ValueFromCaller,
 		}
-
-		// While we're here, we'll just add a diagnostic if the value has
-		// somehow changed between the planning and apply phases. All of these
-		// diagnostics acknowledge that the root cause here is a bug in
-		// Terraform.
-		if phase == ApplyPhase {
-			config := module.Variables[name]
-			if config.Ephemeral {
-				// Ephemeral variables are allowed to change between the plan
-				// and apply stages, so we won't bother checking them.
-				continue
-			}
-
-			raw, ok := previousValues[name]
-			if !ok {
-				// This shouldn't happen because we should have a value for
-				// every input variable that we have a value for in the plan.
-				diags = diags.Append(&hcl.Diagnostic{
-					Severity: hcl.DiagError,
-					Summary:  "Missing input variable value",
-					Detail: fmt.Sprintf(
-						"The input variable %q is required but was not set in the plan for %s. This is a bug in Terraform - please report it.",
-						name, c.Addr(),
-					),
-					Subject: c.call.Declaration(ctx).DeclRange.ToHCL().Ptr(),
-				})
-				continue
-			}
-
-			plannedValue, err := raw.Decode(cty.DynamicPseudoType)
-			if err != nil {
-				// Then something has gone wrong when decoding the value.
-				diags = diags.Append(&hcl.Diagnostic{
-					Severity: hcl.DiagError,
-					Summary:  "Invalid planned input variable value",
-					Detail:   fmt.Sprintf("Failed to decode the planned value for input variable %q: %s. This is a bug in Terraform - please report it.", name, err),
-					Subject:  c.call.Declaration(ctx).DeclRange.ToHCL().Ptr(),
-				})
-				continue
-			}
-
-			// We're unmarking the value here so that we can compare it to the
-			// planned value. We're only checking for equality from here on out,
-			// so we don't need to worry about the marks.
-			applyValue, _ := v.UnmarkDeep()
-
-			if errs := objchange.AssertValueCompatible(plannedValue, applyValue); len(errs) > 0 {
-				// Then the value has changed between the planning and apply
-				// phases. This is a bug in Terraform. We don't want to expose
-				// the actual values here as they could contain sensitive
-				// information. This should be a rare error message, so we
-				// don't need to be too verbose.
-				diags = diags.Append(&hcl.Diagnostic{
-					Severity: hcl.DiagError,
-					Summary:  "Planned input variable value changed",
-					Detail: fmt.Sprintf(
-						"The planned value for input variable %q has changed between the planning and apply phases for %s. This is a bug in Terraform - please report it.",
-						name, c.Addr(),
-					),
-					Subject: c.call.Declaration(ctx).DeclRange.ToHCL().Ptr(),
-				})
-			}
-		}
 	}
-	return ret, diags
+	return ret
 }
 
 // Providers evaluates the "providers" argument from the component
@@ -615,8 +559,7 @@ func (c *ComponentInstance) CheckModuleTreePlan(ctx context.Context) (*plans.Pla
 			}
 
 			stackPlanOpts := c.main.PlanningOpts()
-			inputValues, inputValueDiags := c.inputValuesForModulesRuntime(ctx, nil, PlanPhase)
-			diags = diags.Append(inputValueDiags)
+			inputValues := c.inputValuesForModulesRuntime(ctx, PlanPhase)
 			if inputValues == nil || diags.HasErrors() {
 				return nil, diags
 			}
@@ -868,9 +811,8 @@ func (c *ComponentInstance) ApplyModuleTreePlan(ctx context.Context, plan *plans
 	// shallow-copy it. This is NOT a deep copy, so don't modify anything
 	// that's reachable through any pointers without copying those first too.
 	modifiedPlan := *plan
-	inputValues, inputValueDiags := c.inputValuesForModulesRuntime(ctx, plan.VariableValues, ApplyPhase)
-	diags = diags.Append(inputValueDiags)
-	if inputValues == nil || inputValueDiags.HasErrors() {
+	inputValues := c.inputValuesForModulesRuntime(ctx, ApplyPhase)
+	if inputValues == nil {
 		// inputValuesForModulesRuntime uses nil (as opposed to a
 		// non-nil zerolen map) to represent that the definition of
 		// the input variables was so invalid that we cannot do
