terraform: remove state from `validate` graph walk (#26063)

This pull reverts a recent change to backend/local which created two context, one with and one without state. Instead I have removed the state entirely from the validate graph (by explicitly passing a states.NewState() to the validate graph builder).

This changed caused a test failure, which (ty so much for the help) @jbardin discovered was inaccurate all along: the test's call to `Validate()` was actually what was removing the output from state. The new expected test output matches terraform's actual behavior on the command line: if you use -target to destroy a resource, an output that references only that resource is *not* removed from state even though that test would lead you to believe it did.

This includes two tests to cover the expected behavior:

TestPlan_varsUnset has been updated so it will panic if it gets more than one request to input a variable
TestPlan_providerArgumentUnset covers #26035

Fixes #26035, #26027

backend/local/backend_local.go

@@ -84,13 +84,6 @@ func (b *Local) context(op *backend.Operation) (*terraform.Context, *configload.
 	log.Printf("[TRACE] backend/local: retrieving local state snapshot for workspace %q", op.Workspace)
 	opts.State = s.State()
 
-	// Prepare a separate opts and context for validation, which doesn't use
-	// any state ensuring that we only validate the config, since evaluation
-	// will automatically reference the state when available.
-	validateOpts := opts
-	validateOpts.State = nil
-	var validateCtx *terraform.Context
-
 	var tfCtx *terraform.Context
 	var ctxDiags tfdiags.Diagnostics
 	var configSnap *configload.Snapshot
@@ -108,18 +101,9 @@ func (b *Local) context(op *backend.Operation) (*terraform.Context, *configload.
 		// Write sources into the cache of the main loader so that they are
 		// available if we need to generate diagnostic message snippets.
 		op.ConfigLoader.ImportSourcesFromSnapshot(configSnap)
-
-		// create a validation context with no state
-		validateCtx, _, _ = b.contextFromPlanFile(op.PlanFile, validateOpts, stateMeta)
-		// diags from here will be caught above
-
 	} else {
 		log.Printf("[TRACE] backend/local: building context for current working directory")
 		tfCtx, configSnap, ctxDiags = b.contextDirect(op, opts)
-
-		// create a validation context with no state
-		validateCtx, _, _ = b.contextDirect(op, validateOpts)
-		// diags from here will be caught above
 	}
 	diags = diags.Append(ctxDiags)
 	if diags.HasErrors() {
@@ -145,7 +129,7 @@ func (b *Local) context(op *backend.Operation) (*terraform.Context, *configload.
 		// If validation is enabled, validate
 		if b.OpValidation {
 			log.Printf("[TRACE] backend/local: running validation operation")
-			validateDiags := validateCtx.Validate()
+			validateDiags := tfCtx.Validate()
 			diags = diags.Append(validateDiags)
 		}
 	}

command/plan_test.go

@@ -560,15 +560,15 @@ func TestPlan_varsUnset(t *testing.T) {
 	tmp, cwd := testCwd(t)
 	defer testFixCwd(t, tmp, cwd)
 
-	// Disable test mode so input would be asked
-	test = false
-	defer func() { test = true }()
-
 	// The plan command will prompt for interactive input of var.foo.
 	// We'll answer "bar" to that prompt, which should then allow this
 	// configuration to apply even though var.foo doesn't have a
 	// default value and there are no -var arguments on our command line.
-	defaultInputReader = bytes.NewBufferString("bar\n")
+
+	// This will (helpfully) panic if more than one variable is requested during plan:
+	// https://github.com/hashicorp/terraform/issues/26027
+	close := testInteractiveInput(t, []string{"bar"})
+	defer close()
 
 	p := planVarsFixtureProvider()
 	ui := new(cli.MockUi)
@@ -587,6 +587,64 @@ func TestPlan_varsUnset(t *testing.T) {
 	}
 }
 
+// This test adds a required argument to the test provider to validate
+// processing of user input:
+// https://github.com/hashicorp/terraform/issues/26035
+func TestPlan_providerArgumentUnset(t *testing.T) {
+	tmp, cwd := testCwd(t)
+	defer testFixCwd(t, tmp, cwd)
+
+	// Disable test mode so input would be asked
+	test = false
+	defer func() { test = true }()
+
+	// The plan command will prompt for interactive input of provider.test.region
+	defaultInputReader = bytes.NewBufferString("us-east-1\n")
+
+	p := planFixtureProvider()
+	// override the planFixtureProvider schema to include a required provider argument
+	p.GetSchemaReturn = &terraform.ProviderSchema{
+		Provider: &configschema.Block{
+			Attributes: map[string]*configschema.Attribute{
+				"region": {Type: cty.String, Required: true},
+			},
+		},
+		ResourceTypes: map[string]*configschema.Block{
+			"test_instance": {
+				Attributes: map[string]*configschema.Attribute{
+					"id":  {Type: cty.String, Optional: true, Computed: true},
+					"ami": {Type: cty.String, Optional: true, Computed: true},
+				},
+				BlockTypes: map[string]*configschema.NestedBlock{
+					"network_interface": {
+						Nesting: configschema.NestingList,
+						Block: configschema.Block{
+							Attributes: map[string]*configschema.Attribute{
+								"device_index": {Type: cty.String, Optional: true},
+								"description":  {Type: cty.String, Optional: true},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+	ui := new(cli.MockUi)
+	c := &PlanCommand{
+		Meta: Meta{
+			testingOverrides: metaOverridesForProvider(p),
+			Ui:               ui,
+		},
+	}
+
+	args := []string{
+		testFixturePath("plan"),
+	}
+	if code := c.Run(args); code != 0 {
+		t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String())
+	}
+}
+
 func TestPlan_varFile(t *testing.T) {
 	tmp, cwd := testCwd(t)
 	defer testFixCwd(t, tmp, cwd)

terraform/context.go

@@ -264,27 +264,27 @@ func (c *Context) Graph(typ GraphType, opts *ContextGraphOpts) (*Graph, tfdiags.
 		}).Build(addrs.RootModuleInstance)
 
 	case GraphTypeValidate:
-		// The validate graph is just a slightly modified plan graph
-		fallthrough
+		// The validate graph is just a slightly modified plan graph: an empty
+		// state is substituted in for Validate.
+		return ValidateGraphBuilder(&PlanGraphBuilder{
+			Config:     c.config,
+			Components: c.components,
+			Schemas:    c.schemas,
+			Targets:    c.targets,
+			Validate:   opts.Validate,
+			State:      states.NewState(),
+		}).Build(addrs.RootModuleInstance)
+
 	case GraphTypePlan:
 		// Create the plan graph builder
-		p := &PlanGraphBuilder{
+		return (&PlanGraphBuilder{
 			Config:     c.config,
 			State:      c.state,
 			Components: c.components,
 			Schemas:    c.schemas,
 			Targets:    c.targets,
 			Validate:   opts.Validate,
-		}
-
-		// Some special cases for other graph types shared with plan currently
-		var b GraphBuilder = p
-		switch typ {
-		case GraphTypeValidate:
-			b = ValidateGraphBuilder(p)
-		}
-
-		return b.Build(addrs.RootModuleInstance)
+		}).Build(addrs.RootModuleInstance)
 
 	case GraphTypePlanDestroy:
 		return (&DestroyPlanGraphBuilder{
@@ -769,6 +769,17 @@ func (c *Context) walk(graph *Graph, operation walkOperation) (*ContextGraphWalk
 }
 
 func (c *Context) graphWalker(operation walkOperation) *ContextGraphWalker {
+	if operation == walkValidate {
+		return &ContextGraphWalker{
+			Context:            c,
+			State:              states.NewState().SyncWrapper(),
+			Changes:            c.changes.SyncWrapper(),
+			InstanceExpander:   instances.NewExpander(),
+			Operation:          operation,
+			StopContext:        c.runContext,
+			RootVariableValues: c.variables,
+		}
+	}
 	return &ContextGraphWalker{
 		Context:            c,
 		State:              c.state.SyncWrapper(),

terraform/context_apply_test.go

@@ -7461,10 +7461,19 @@ func TestContext2Apply_targetedDestroy(t *testing.T) {
 		t.Fatalf("expected 0 resources, got: %#v", mod.Resources)
 	}
 
-	// the root output should have been removed too, since it is derived solely
-	// from the targeted resource
-	if len(mod.OutputValues) != 0 {
-		t.Fatalf("expected 0 outputs, got: %#v", mod.OutputValues)
+	// the root output should not get removed; only the targeted resource.
+	//
+	// Note: earlier versions of this test expected 0 outputs, but it turns out
+	// that was because Validate - not apply or destroy - removed the output
+	// (which depends on the targeted resource) from state. That version of this
+	// test did not match actual terraform behavior: the output remains in
+	// state.
+	//
+	// TODO: Future refactoring may enable us to remove the output from state in
+	// this case, and that would be Just Fine - this test can be modified to
+	// expect 0 outputs.
+	if len(mod.OutputValues) != 1 {
+		t.Fatalf("expected 1 outputs, got: %#v", mod.OutputValues)
 	}
 
 	// the module instance should remain