Merge pull request #36186 from hashicorp/sams/skip-graph-cycle-valida…

…tion

Skip graph cycle validation when using the graph command

internal/command/graph.go

@@ -115,6 +115,7 @@ func (c *GraphCommand) Run(args []string) int {
 		c.showDiagnostics(diags)
 		return 1
 	}
+	lr.Core.SetGraphOpts(&terraform.ContextGraphOpts{SkipGraphValidation: drawCycles})
 
 	if graphTypeStr == "" {
 		if planFile == nil {

internal/command/graph_test.go

@@ -50,6 +50,124 @@ func TestGraph_planPhase(t *testing.T) {
 	}
 }
 
+func TestGraph_cyclic(t *testing.T) {
+	td := t.TempDir()
+	testCopyDir(t, testFixturePath("graph-cyclic"), td)
+	defer testChdir(t, td)()
+
+	tests := []struct {
+		name     string
+		args     []string
+		expected string
+
+		// The cyclic errors do not maintain a consistent order, so we can't
+		// predict the exact output. We'll just check that the error messages
+		// are present for the things we know are cyclic.
+		errors []string
+	}{
+		{
+			name: "plan",
+			args: []string{"-type=plan"},
+			errors: []string{`Error: Cycle: test_instance.`,
+				`Error: Cycle: local.`},
+		},
+		{
+			name: "plan with -draw-cycles option",
+			args: []string{"-draw-cycles", "-type=plan"},
+			expected: `digraph {
+	compound = "true"
+	newrank = "true"
+	subgraph "root" {
+		"[root] provider[\"registry.terraform.io/hashicorp/test\"]" [label = "provider[\"registry.terraform.io/hashicorp/test\"]", shape = "diamond"]
+		"[root] test_instance.bar (expand)" [label = "test_instance.bar", shape = "box"]
+		"[root] test_instance.foo (expand)" [label = "test_instance.foo", shape = "box"]
+		"[root] local.test1 (expand)" -> "[root] local.test2 (expand)"
+		"[root] local.test2 (expand)" -> "[root] local.test1 (expand)"
+		"[root] provider[\"registry.terraform.io/hashicorp/test\"] (close)" -> "[root] provider[\"registry.terraform.io/hashicorp/test\"]"
+		"[root] provider[\"registry.terraform.io/hashicorp/test\"] (close)" -> "[root] test_instance.bar (expand)"
+		"[root] provider[\"registry.terraform.io/hashicorp/test\"] (close)" -> "[root] test_instance.foo (expand)"
+		"[root] root" -> "[root] provider[\"registry.terraform.io/hashicorp/test\"] (close)"
+		"[root] test_instance.bar (expand)" -> "[root] provider[\"registry.terraform.io/hashicorp/test\"]"
+		"[root] test_instance.bar (expand)" -> "[root] test_instance.foo (expand)" [color = "red", penwidth = "2.0"]
+		"[root] test_instance.foo (expand)" -> "[root] provider[\"registry.terraform.io/hashicorp/test\"]"
+		"[root] test_instance.foo (expand)" -> "[root] test_instance.bar (expand)" [color = "red", penwidth = "2.0"]
+	}
+}`,
+		},
+		{
+			name: "apply",
+			args: []string{"-type=apply"},
+			// The cyclic errors do not maintain a consistent order, so we can't
+			// predict the exact output. We'll just check that the error messages
+			// are present for the things we know are cyclic.
+			errors: []string{`Error: Cycle: test_instance.`,
+				`Error: Cycle: local.`},
+		},
+		{
+			name: "apply with -draw-cycles option",
+			args: []string{"-draw-cycles", "-type=apply"},
+			expected: `digraph {
+	compound = "true"
+	newrank = "true"
+	subgraph "root" {
+		"[root] provider[\"registry.terraform.io/hashicorp/test\"]" [label = "provider[\"registry.terraform.io/hashicorp/test\"]", shape = "diamond"]
+		"[root] test_instance.bar (expand)" [label = "test_instance.bar", shape = "box"]
+		"[root] test_instance.foo (expand)" [label = "test_instance.foo", shape = "box"]
+		"[root] local.test1 (expand)" -> "[root] local.test2 (expand)"
+		"[root] local.test2 (expand)" -> "[root] local.test1 (expand)"
+		"[root] provider[\"registry.terraform.io/hashicorp/test\"] (close)" -> "[root] provider[\"registry.terraform.io/hashicorp/test\"]"
+		"[root] provider[\"registry.terraform.io/hashicorp/test\"] (close)" -> "[root] test_instance.bar (expand)"
+		"[root] provider[\"registry.terraform.io/hashicorp/test\"] (close)" -> "[root] test_instance.foo (expand)"
+		"[root] root" -> "[root] provider[\"registry.terraform.io/hashicorp/test\"] (close)"
+		"[root] test_instance.bar (expand)" -> "[root] provider[\"registry.terraform.io/hashicorp/test\"]"
+		"[root] test_instance.bar (expand)" -> "[root] test_instance.foo (expand)" [color = "red", penwidth = "2.0"]
+		"[root] test_instance.foo (expand)" -> "[root] provider[\"registry.terraform.io/hashicorp/test\"]"
+		"[root] test_instance.foo (expand)" -> "[root] test_instance.bar (expand)" [color = "red", penwidth = "2.0"]
+	}
+}`,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ui := new(cli.MockUi)
+			streams, closeStreams := terminal.StreamsForTesting(t)
+			c := &GraphCommand{
+				Meta: Meta{
+					testingOverrides: metaOverridesForProvider(applyFixtureProvider()),
+					Ui:               ui,
+					Streams:          streams,
+				},
+			}
+
+			code := c.Run(tt.args)
+			// If we expect errors, make sure they are present
+			if len(tt.errors) > 0 {
+				if code == 0 {
+					t.Fatalf("expected error, got none")
+				}
+				got := strings.TrimSpace(ui.ErrorWriter.String())
+				for _, err := range tt.errors {
+					if !strings.Contains(got, err) {
+						t.Fatalf("expected error:\n%s\n\nactual error:\n%s", err, got)
+					}
+				}
+				return
+			}
+
+			// If we don't expect errors, make sure the command ran successfully
+			if code != 0 {
+				t.Fatalf("bad: \n%s", ui.ErrorWriter.String())
+			}
+			output := closeStreams(t)
+			if strings.TrimSpace(output.Stdout()) != strings.TrimSpace(tt.expected) {
+				t.Fatalf("expected dot graph to match:\n%s", cmp.Diff(output.Stdout(), tt.expected))
+			}
+
+		})
+	}
+}
+
 func TestGraph_multipleArgs(t *testing.T) {
 	ui := new(cli.MockUi)
 	c := &GraphCommand{

internal/command/testdata/graph-cyclic/main.tf

@@ -0,0 +1,12 @@
+locals {
+  test1 = local.test2
+  test2 = local.test1
+}
+
+resource "test_instance" "foo" {
+    ami = resource.test_instance.bar.ami
+}
+
+resource "test_instance" "bar" {
+    ami = resource.test_instance.foo.ami
+}

internal/dag/dot.go

@@ -177,7 +177,7 @@ func (g *marshalGraph) writeBody(opts *DotOpts, w *indentWriter) {
 		w.Write(v.dot(g, opts))
 	}
 
-	var dotEdges []string
+	dotEdges := make(map[string]string)
 
 	if opts.DrawCycles {
 		for _, c := range g.Cycles {
@@ -203,20 +203,28 @@ func (g *marshalGraph) writeBody(opts *DotOpts, w *indentWriter) {
 					Attrs:  make(map[string]string),
 				}
 
-				dotEdges = append(dotEdges, cycleDot(e, g))
+				dotEdges[e.Name] = cycleDot(e, g)
 				src = tgt
 			}
 		}
 	}
 
 	for _, e := range g.Edges {
-		dotEdges = append(dotEdges, e.dot(g))
+		// only add the edge if it's not been added as part of a cycle
+		// or if there are duplicates.
+		if _, ok := dotEdges[e.Name]; !ok {
+			dotEdges[e.Name] = e.dot(g)
+		}
 	}
 
-	// srot these again to match the old output
-	sort.Strings(dotEdges)
+	// sort these again to match the old output
+	dotEdgesList := make([]string, 0, len(dotEdges))
+	for _, v := range dotEdges {
+		dotEdgesList = append(dotEdgesList, v)
+	}
+	sort.Strings(dotEdgesList)
 
-	for _, e := range dotEdges {
+	for _, e := range dotEdgesList {
 		w.WriteString(e + "\n")
 	}
 

internal/terraform/context.go

@@ -95,9 +95,10 @@ type Context struct {
 
 	plugins *contextPlugins
 
-	hooks   []Hook
-	sh      *stopHook
-	uiInput UIInput
+	hooks     []Hook
+	sh        *stopHook
+	uiInput   UIInput
+	graphOpts *ContextGraphOpts
 
 	l                   sync.Mutex // Lock acquired during any task
 	parallelSem         Semaphore
@@ -151,9 +152,10 @@ func NewContext(opts *ContextOpts) (*Context, tfdiags.Diagnostics) {
 	log.Printf("[TRACE] terraform.NewContext: complete")
 
 	return &Context{
-		hooks:   hooks,
-		meta:    opts.Meta,
-		uiInput: opts.UIInput,
+		hooks:     hooks,
+		meta:      opts.Meta,
+		uiInput:   opts.UIInput,
+		graphOpts: &ContextGraphOpts{},
 
 		plugins: plugins,
 
@@ -163,6 +165,11 @@ func NewContext(opts *ContextOpts) (*Context, tfdiags.Diagnostics) {
 	}, diags
 }
 
+func (c *Context) SetGraphOpts(opts *ContextGraphOpts) tfdiags.Diagnostics {
+	c.graphOpts = opts
+	return nil
+}
+
 func (c *Context) Schemas(config *configs.Config, state *states.State) (*Schemas, tfdiags.Diagnostics) {
 	var diags tfdiags.Diagnostics
 
@@ -179,8 +186,8 @@ func (c *Context) Schemas(config *configs.Config, state *states.State) (*Schemas
 }
 
 type ContextGraphOpts struct {
-	// If true, validates the graph structure (checks for cycles).
-	Validate bool
+	// If false, skip the graph structure validation.
+	SkipGraphValidation bool
 
 	// Legacy graphs only: won't prune the graph
 	Verbose bool

internal/terraform/context_apply.go

@@ -370,6 +370,7 @@ func (c *Context) applyGraph(plan *plans.Plan, config *configs.Config, opts *App
 		Operation:               operation,
 		ExternalReferences:      plan.ExternalReferences,
 		Overrides:               plan.Overrides,
+		SkipGraphValidation:     c.graphOpts.SkipGraphValidation,
 	}).Build(addrs.RootModuleInstance)
 	diags = diags.Append(moreDiags)
 	if moreDiags.HasErrors() {

internal/terraform/context_plan.go

@@ -915,6 +915,7 @@ func (c *Context) planGraph(config *configs.Config, prevRunState *states.State,
 			forgetResources:         forgetResources,
 			forgetModules:           forgetModules,
 			GenerateConfigPath:      opts.GenerateConfigPath,
+			SkipGraphValidation:     c.graphOpts.SkipGraphValidation,
 		}).Build(addrs.RootModuleInstance)
 		return graph, walkPlan, diags
 	case plans.RefreshOnlyMode:
@@ -930,6 +931,7 @@ func (c *Context) planGraph(config *configs.Config, prevRunState *states.State,
 			Operation:               walkPlan,
 			ExternalReferences:      opts.ExternalReferences,
 			Overrides:               opts.Overrides,
+			SkipGraphValidation:     c.graphOpts.SkipGraphValidation,
 		}).Build(addrs.RootModuleInstance)
 		return graph, walkPlan, diags
 	case plans.DestroyMode:
@@ -943,6 +945,7 @@ func (c *Context) planGraph(config *configs.Config, prevRunState *states.State,
 			skipRefresh:             opts.SkipRefresh,
 			Operation:               walkPlanDestroy,
 			Overrides:               opts.Overrides,
+			SkipGraphValidation:     c.graphOpts.SkipGraphValidation,
 		}).Build(addrs.RootModuleInstance)
 		return graph, walkPlanDestroy, diags
 	default:

internal/terraform/graph_builder.go

@@ -27,6 +27,10 @@ type BasicGraphBuilder struct {
 	Steps []GraphTransformer
 	// Optional name to add to the graph debug log
 	Name string
+
+	// SkipGraphValidation indicates whether the graph validation (enabled by default)
+	// should be skipped after the graph is built.
+	SkipGraphValidation bool
 }
 
 func (b *BasicGraphBuilder) Build(path addrs.ModuleInstance) (*Graph, tfdiags.Diagnostics) {
@@ -58,6 +62,13 @@ func (b *BasicGraphBuilder) Build(path addrs.ModuleInstance) (*Graph, tfdiags.Di
 		}
 	}
 
+	// Return early if the graph validation is skipped
+	// This behavior is currently only used by the graph command
+	// which only wants to display the dot representation of the graph
+	if b.SkipGraphValidation {
+		return g, diags
+	}
+
 	if err := g.Validate(); err != nil {
 		log.Printf("[ERROR] Graph validation failed. Graph:\n\n%s", g.String())
 		diags = diags.Append(err)

internal/terraform/graph_builder_apply.go

@@ -73,13 +73,18 @@ type ApplyGraphBuilder struct {
 	// Overrides provides the set of overrides supplied by the testing
 	// framework.
 	Overrides *mocking.Overrides
+
+	// SkipGraphValidation indicates whether the graph builder should skip
+	// validation of the graph.
+	SkipGraphValidation bool
 }
 
 // See GraphBuilder
 func (b *ApplyGraphBuilder) Build(path addrs.ModuleInstance) (*Graph, tfdiags.Diagnostics) {
 	return (&BasicGraphBuilder{
-		Steps: b.Steps(),
-		Name:  "ApplyGraphBuilder",
+		Steps:               b.Steps(),
+		Name:                "ApplyGraphBuilder",
+		SkipGraphValidation: b.SkipGraphValidation,
 	}).Build(path)
 }
 

internal/terraform/graph_builder_plan.go

@@ -106,14 +106,19 @@ type PlanGraphBuilder struct {
 	//
 	// If empty, then config will not be generated.
 	GenerateConfigPath string
+
+	// SkipGraphValidation indicates whether the graph builder should skip
+	// validation of the graph.
+	SkipGraphValidation bool
 }
 
 // See GraphBuilder
 func (b *PlanGraphBuilder) Build(path addrs.ModuleInstance) (*Graph, tfdiags.Diagnostics) {
 	log.Printf("[TRACE] building graph for %s", b.Operation)
 	return (&BasicGraphBuilder{
-		Steps: b.Steps(),
-		Name:  "PlanGraphBuilder",
+		Steps:               b.Steps(),
+		Name:                "PlanGraphBuilder",
+		SkipGraphValidation: b.SkipGraphValidation,
 	}).Build(path)
 }