From bb73fb1d699722923f504d473fddffc2cf1af681 Mon Sep 17 00:00:00 2001 From: Liam Cervante Date: Wed, 14 Aug 2024 09:47:13 +0200 Subject: [PATCH] stacks: don't validate input variables during the apply phase (#35561) --- internal/stacks/stackruntime/apply_test.go | 133 +----------------- .../internal/stackeval/component_instance.go | 100 +++---------- 2 files changed, 23 insertions(+), 210 deletions(-) diff --git a/internal/stacks/stackruntime/apply_test.go b/internal/stacks/stackruntime/apply_test.go index 667581165e..c50b6233fa 100644 --- 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 index da009164e1..70239525d5 100644 --- 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