From 0bc69d64ec124e82400460ea3141b303fa28d5b1 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 11 Mar 2022 09:31:24 -0500 Subject: [PATCH 1/2] always validate all graphs Complete the removal of the Validate option for graph building. There is no case where we want to allow an invalid graph, as the primary reason for validation is to ensure we have no cycles, and we can't walk a graph with cycles. The only code which specifically relied on there being no validation was a test to ensure the Validate flag prevented it. --- internal/terraform/context_apply.go | 1 - internal/terraform/graph_builder.go | 14 +++++--------- internal/terraform/graph_builder_apply.go | 8 ++------ internal/terraform/graph_builder_destroy_plan.go | 8 ++------ internal/terraform/graph_builder_eval.go | 5 ++--- internal/terraform/graph_builder_import.go | 5 ++--- internal/terraform/graph_builder_test.go | 16 ---------------- internal/terraform/node_resource_plan.go | 5 ++--- 8 files changed, 15 insertions(+), 47 deletions(-) diff --git a/internal/terraform/context_apply.go b/internal/terraform/context_apply.go index d3cd9dc2d5..fdc835f307 100644 --- a/internal/terraform/context_apply.go +++ b/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() { diff --git a/internal/terraform/graph_builder.go b/internal/terraform/graph_builder.go index 9d0cadde41..1c69ee41f8 100644 --- a/internal/terraform/graph_builder.go +++ b/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 - Validate bool + Steps []GraphTransformer // 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 b.Validate { - if err := g.Validate(); err != nil { - log.Printf("[ERROR] Graph validation failed. Graph:\n\n%s", g.String()) - diags = diags.Append(err) - return nil, diags - } + if err := g.Validate(); err != nil { + log.Printf("[ERROR] Graph validation failed. Graph:\n\n%s", g.String()) + diags = diags.Append(err) + return nil, diags } return g, diags diff --git a/internal/terraform/graph_builder_apply.go b/internal/terraform/graph_builder_apply.go index 86d8255602..eea3235cd1 100644 --- a/internal/terraform/graph_builder_apply.go +++ b/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(), - Validate: b.Validate, - Name: "ApplyGraphBuilder", + Steps: b.Steps(), + Name: "ApplyGraphBuilder", }).Build(path) } diff --git a/internal/terraform/graph_builder_destroy_plan.go b/internal/terraform/graph_builder_destroy_plan.go index def1aa3739..ade01c629b 100644 --- a/internal/terraform/graph_builder_destroy_plan.go +++ b/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(), - Validate: b.Validate, - Name: "DestroyPlanGraphBuilder", + Steps: b.Steps(), + Name: "DestroyPlanGraphBuilder", }).Build(path) } diff --git a/internal/terraform/graph_builder_eval.go b/internal/terraform/graph_builder_eval.go index 78031e21f9..4e205045f7 100644 --- a/internal/terraform/graph_builder_eval.go +++ b/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(), - Validate: true, - Name: "EvalGraphBuilder", + Steps: b.Steps(), + Name: "EvalGraphBuilder", }).Build(path) } diff --git a/internal/terraform/graph_builder_import.go b/internal/terraform/graph_builder_import.go index d8d609ebaa..79c0724f76 100644 --- a/internal/terraform/graph_builder_import.go +++ b/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(), - Validate: true, - Name: "ImportGraphBuilder", + Steps: b.Steps(), + Name: "ImportGraphBuilder", }).Build(path) } diff --git a/internal/terraform/graph_builder_test.go b/internal/terraform/graph_builder_test.go index 91d703f42c..414fc0b8d7 100644 --- a/internal/terraform/graph_builder_test.go +++ b/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 } diff --git a/internal/terraform/node_resource_plan.go b/internal/terraform/node_resource_plan.go index a9f3ca0e5d..5a01d9337d 100644 --- a/internal/terraform/node_resource_plan.go +++ b/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, - Validate: true, - Name: "NodePlannableResource", + Steps: steps, + Name: "NodePlannableResource", } graph, diags := b.Build(ctx.Path()) return graph, diags.ErrWithWarnings() From b1de94a1768cfecdd31d7b065976cc4bc8dafa16 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 11 Mar 2022 09:33:33 -0500 Subject: [PATCH 2/2] make sure CBD test graphs are valid The graphs used for the CBD tests wouldn't validate because they skipped adding the root module node. Re add the root module transformer and transitive reduction transformer to the build steps, and match the new reduced output in the test fixtures. --- internal/terraform/transform_destroy_cbd_test.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/internal/terraform/transform_destroy_cbd_test.go b/internal/terraform/transform_destroy_cbd_test.go index 629ca54778..8f5712b574 100644 --- a/internal/terraform/transform_destroy_cbd_test.go +++ b/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\]