Merge pull request #30656 from hashicorp/jbardin/always-validate

Always validate the graph

internal/terraform/context_apply.go

@@ -110,7 +110,6 @@ func (c *Context) applyGraph(plan *plans.Plan, config *configs.Config, validate
 		Plugins:            c.plugins,
 		Targets:            plan.TargetAddrs,
 		ForceReplace:       plan.ForceReplaceAddrs,
-		Validate:           validate,
 	}).Build(addrs.RootModuleInstance)
 	diags = diags.Append(moreDiags)
 	if moreDiags.HasErrors() {

internal/terraform/graph_builder.go

@@ -21,8 +21,7 @@ type GraphBuilder interface {
 // series of transforms and (optionally) validates the graph is a valid
 // structure.
 type BasicGraphBuilder struct {
-	Steps    []GraphTransformer
+	Steps []GraphTransformer
-	Validate bool
 	// Optional name to add to the graph debug log
 	Name string
 }
@@ -56,13 +55,10 @@ func (b *BasicGraphBuilder) Build(path addrs.ModuleInstance) (*Graph, tfdiags.Di
 		}
 	}
 
-	// Validate the graph structure
+	if err := g.Validate(); err != nil {
-	if b.Validate {
+		log.Printf("[ERROR] Graph validation failed. Graph:\n\n%s", g.String())
-		if err := g.Validate(); err != nil {
+		diags = diags.Append(err)
-			log.Printf("[ERROR] Graph validation failed. Graph:\n\n%s", g.String())
+		return nil, diags
-			diags = diags.Append(err)
-			return nil, diags
-		}
 	}
 
 	return g, diags

internal/terraform/graph_builder_apply.go

@@ -46,17 +46,13 @@ type ApplyGraphBuilder struct {
 	// The apply step refers to these as part of verifying that the planned
 	// actions remain consistent between plan and apply.
 	ForceReplace []addrs.AbsResourceInstance
-
-	// Validate will do structural validation of the graph.
-	Validate bool
 }
 
 // See GraphBuilder
 func (b *ApplyGraphBuilder) Build(path addrs.ModuleInstance) (*Graph, tfdiags.Diagnostics) {
 	return (&BasicGraphBuilder{
-		Steps:    b.Steps(),
+		Steps: b.Steps(),
-		Validate: b.Validate,
+		Name:  "ApplyGraphBuilder",
-		Name:     "ApplyGraphBuilder",
 	}).Build(path)
 }
 

internal/terraform/graph_builder_destroy_plan.go

@@ -35,9 +35,6 @@ type DestroyPlanGraphBuilder struct {
 	// Targets are resources to target
 	Targets []addrs.Targetable
 
-	// Validate will do structural validation of the graph.
-	Validate bool
-
 	// If set, skipRefresh will cause us stop skip refreshing any existing
 	// resource instances as part of our planning. This will cause us to fail
 	// to detect if an object has already been deleted outside of Terraform.
@@ -47,9 +44,8 @@ type DestroyPlanGraphBuilder struct {
 // See GraphBuilder
 func (b *DestroyPlanGraphBuilder) Build(path addrs.ModuleInstance) (*Graph, tfdiags.Diagnostics) {
 	return (&BasicGraphBuilder{
-		Steps:    b.Steps(),
+		Steps: b.Steps(),
-		Validate: b.Validate,
+		Name:  "DestroyPlanGraphBuilder",
-		Name:     "DestroyPlanGraphBuilder",
 	}).Build(path)
 }
 

internal/terraform/graph_builder_eval.go

@@ -43,9 +43,8 @@ type EvalGraphBuilder struct {
 // See GraphBuilder
 func (b *EvalGraphBuilder) Build(path addrs.ModuleInstance) (*Graph, tfdiags.Diagnostics) {
 	return (&BasicGraphBuilder{
-		Steps:    b.Steps(),
+		Steps: b.Steps(),
-		Validate: true,
+		Name:  "EvalGraphBuilder",
-		Name:     "EvalGraphBuilder",
 	}).Build(path)
 }
 

internal/terraform/graph_builder_import.go

@@ -30,9 +30,8 @@ type ImportGraphBuilder struct {
 // Build builds the graph according to the steps returned by Steps.
 func (b *ImportGraphBuilder) Build(path addrs.ModuleInstance) (*Graph, tfdiags.Diagnostics) {
 	return (&BasicGraphBuilder{
-		Steps:    b.Steps(),
+		Steps: b.Steps(),
-		Validate: true,
+		Name:  "ImportGraphBuilder",
-		Name:     "ImportGraphBuilder",
 	}).Build(path)
 }
 

internal/terraform/graph_builder_test.go

@@ -42,7 +42,6 @@ func TestBasicGraphBuilder_validate(t *testing.T) {
 			&testBasicGraphBuilderTransform{1},
 			&testBasicGraphBuilderTransform{2},
 		},
-		Validate: true,
 	}
 
 	_, err := b.Build(addrs.RootModuleInstance)
@@ -51,21 +50,6 @@ func TestBasicGraphBuilder_validate(t *testing.T) {
 	}
 }
 
-func TestBasicGraphBuilder_validateOff(t *testing.T) {
-	b := &BasicGraphBuilder{
-		Steps: []GraphTransformer{
-			&testBasicGraphBuilderTransform{1},
-			&testBasicGraphBuilderTransform{2},
-		},
-		Validate: false,
-	}
-
-	_, err := b.Build(addrs.RootModuleInstance)
-	if err != nil {
-		t.Fatalf("expected no error, got: %s", err)
-	}
-}
-
 type testBasicGraphBuilderTransform struct {
 	V dag.Vertex
 }

internal/terraform/node_resource_plan.go

@@ -390,9 +390,8 @@ func (n *NodePlannableResource) DynamicExpand(ctx EvalContext) (*Graph, error) {
 
 	// Build the graph
 	b := &BasicGraphBuilder{
-		Steps:    steps,
+		Steps: steps,
-		Validate: true,
+		Name:  "NodePlannableResource",
-		Name:     "NodePlannableResource",
 	}
 	graph, diags := b.Build(ctx.Path())
 	return graph, diags.ErrWithWarnings()

internal/terraform/transform_destroy_cbd_test.go

@@ -46,7 +46,12 @@ func cbdTestSteps(steps []GraphTransformer) []GraphTransformer {
 		panic("CBDEdgeTransformer not found")
 	}
 
-	return steps[:i+1]
+	// re-add the root node so we have a valid graph for a walk, then reduce
+	// the graph for less output
+	steps = append(steps[:i+1], &CloseRootModuleTransformer{})
+	steps = append(steps, &TransitiveReductionTransformer{})
+
+	return steps
 }
 
 // remove extra nodes for easier test comparisons
@@ -105,7 +110,6 @@ func TestCBDEdgeTransformer(t *testing.T) {
 	expected := regexp.MustCompile(strings.TrimSpace(`
 (?m)test_object.A
 test_object.A \(destroy deposed \w+\)
-  test_object.A
   test_object.B
 test_object.B
   test_object.A
@@ -178,11 +182,9 @@ func TestCBDEdgeTransformerMulti(t *testing.T) {
 	expected := regexp.MustCompile(strings.TrimSpace(`
 (?m)test_object.A
 test_object.A \(destroy deposed \w+\)
-  test_object.A
   test_object.C
 test_object.B
 test_object.B \(destroy deposed \w+\)
-  test_object.B
   test_object.C
 test_object.C
   test_object.A
@@ -253,7 +255,6 @@ func TestCBDEdgeTransformer_depNonCBDCount(t *testing.T) {
 	expected := regexp.MustCompile(strings.TrimSpace(`
 (?m)test_object.A
 test_object.A \(destroy deposed \w+\)
-  test_object.A
   test_object.B\[0\]
   test_object.B\[1\]
 test_object.B\[0\]
@@ -339,12 +340,10 @@ func TestCBDEdgeTransformer_depNonCBDCountBoth(t *testing.T) {
 	expected := regexp.MustCompile(strings.TrimSpace(`
 test_object.A\[0\]
 test_object.A\[0\] \(destroy deposed \w+\)
-  test_object.A\[0\]
   test_object.B\[0\]
   test_object.B\[1\]
 test_object.A\[1\]
 test_object.A\[1\] \(destroy deposed \w+\)
-  test_object.A\[1\]
   test_object.B\[0\]
   test_object.B\[1\]
 test_object.B\[0\]