From 9b8d25b9e9858a707d63e7e9deecd66a8cefe6dd Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 4 Apr 2024 11:22:01 -0700 Subject: [PATCH] terraform: new nodes for variable validation (not yet implemented) So far we've handled custom validation rules as a part of the same graph node that is responsible for deciding the final value for an input variable, but that's bothersome for two reasons: - There are two different pieces of code both trying to implement the same rules: the root variable node and the module variable node. Those have accidentally diverged in the past. - Each graph node can evaluate expressions only in one module's scope, and so the module variable node is set up to evaluate in the parent module's scope because its purpose is to explicitly pass parent module data into the child module. That is why we haven't yet allowed input variable validation rules to refer to objects declared in the module where the variable is declared. This commit does not actually yet change any real behavior, because this new graph node has no "Execute" implementation. The focus here is just to make sure the graph nodes are inserted into the graph with the needed edges so that a future commit can move the validation logic into here, and hopefully eventually also allow the validation rules to refer to objects declared in the child module. --- internal/terraform/graph_builder_apply.go | 1 + internal/terraform/graph_builder_eval.go | 1 + internal/terraform/graph_builder_plan.go | 1 + internal/terraform/graph_builder_plan_test.go | 4 +- internal/terraform/node_module_variable.go | 9 ++ internal/terraform/node_root_variable.go | 9 ++ .../terraform/node_variable_validation.go | 86 +++++++++++ .../transform_variable_validation.go | 60 ++++++++ .../transform_variable_validation_test.go | 140 ++++++++++++++++++ 9 files changed, 310 insertions(+), 1 deletion(-) create mode 100644 internal/terraform/node_variable_validation.go create mode 100644 internal/terraform/transform_variable_validation.go create mode 100644 internal/terraform/transform_variable_validation_test.go diff --git a/internal/terraform/graph_builder_apply.go b/internal/terraform/graph_builder_apply.go index 807674377e..f402657a54 100644 --- a/internal/terraform/graph_builder_apply.go +++ b/internal/terraform/graph_builder_apply.go @@ -116,6 +116,7 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { Config: b.Config, DestroyApply: b.Operation == walkDestroy, }, + &variableValidationTransformer{}, &LocalTransformer{Config: b.Config}, &OutputTransformer{ Config: b.Config, diff --git a/internal/terraform/graph_builder_eval.go b/internal/terraform/graph_builder_eval.go index 386d0c73ab..697b66fee0 100644 --- a/internal/terraform/graph_builder_eval.go +++ b/internal/terraform/graph_builder_eval.go @@ -76,6 +76,7 @@ func (b *EvalGraphBuilder) Steps() []GraphTransformer { // Add dynamic values &RootVariableTransformer{Config: b.Config, RawValues: b.RootVariableValues, Planning: true}, &ModuleVariableTransformer{Config: b.Config, Planning: true}, + &variableValidationTransformer{}, &LocalTransformer{Config: b.Config}, &OutputTransformer{ Config: b.Config, diff --git a/internal/terraform/graph_builder_plan.go b/internal/terraform/graph_builder_plan.go index 22d8522fdf..07104baa3b 100644 --- a/internal/terraform/graph_builder_plan.go +++ b/internal/terraform/graph_builder_plan.go @@ -154,6 +154,7 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer { Planning: true, DestroyApply: false, // always false for planning }, + &variableValidationTransformer{}, &LocalTransformer{Config: b.Config}, &OutputTransformer{ Config: b.Config, diff --git a/internal/terraform/graph_builder_plan_test.go b/internal/terraform/graph_builder_plan_test.go index bc7018f2d5..d880a7f377 100644 --- a/internal/terraform/graph_builder_plan_test.go +++ b/internal/terraform/graph_builder_plan_test.go @@ -262,7 +262,7 @@ func TestPlanGraphBuilder_forEach(t *testing.T) { const testPlanGraphBuilderStr = ` aws_instance.web (expand) aws_security_group.firewall (expand) - var.foo + var.foo (validation) aws_load_balancer.weblb (expand) aws_instance.web (expand) aws_security_group.firewall (expand) @@ -285,6 +285,8 @@ root provider["registry.terraform.io/hashicorp/aws"] (close) provider["registry.terraform.io/hashicorp/openstack"] (close) var.foo +var.foo (validation) + var.foo ` const testPlanGraphBuilderForEachStr = ` aws_instance.bar (expand) diff --git a/internal/terraform/node_module_variable.go b/internal/terraform/node_module_variable.go index c52ecca144..d08009db9e 100644 --- a/internal/terraform/node_module_variable.go +++ b/internal/terraform/node_module_variable.go @@ -150,6 +150,15 @@ func (n *nodeExpandModuleVariable) ReferenceableAddrs() []addrs.Referenceable { return []addrs.Referenceable{n.Addr} } +// variableValidationRules implements [graphNodeValidatableVariable]. +func (n *nodeExpandModuleVariable) variableValidationRules() (addrs.ConfigInputVariable, []*configs.CheckRule) { + var rules []*configs.CheckRule + if n.Config != nil { // always in normal code, but sometimes not in unit tests + rules = n.Config.Validations + } + return n.Addr.InModule(n.Module), rules +} + // nodeModuleVariable represents a module variable input during // the apply step. type nodeModuleVariable struct { diff --git a/internal/terraform/node_root_variable.go b/internal/terraform/node_root_variable.go index 29cc4b1aa2..c3919af696 100644 --- a/internal/terraform/node_root_variable.go +++ b/internal/terraform/node_root_variable.go @@ -135,3 +135,12 @@ func (n *NodeRootVariable) DotNode(name string, opts *dag.DotOpts) *dag.DotNode }, } } + +// variableValidationRules implements [graphNodeValidatableVariable]. +func (n *NodeRootVariable) variableValidationRules() (addrs.ConfigInputVariable, []*configs.CheckRule) { + var rules []*configs.CheckRule + if n.Config != nil { // always in normal code, but sometimes not in unit tests + rules = n.Config.Validations + } + return n.Addr.InModule(addrs.RootModule), rules +} diff --git a/internal/terraform/node_variable_validation.go b/internal/terraform/node_variable_validation.go new file mode 100644 index 0000000000..b8c2b79834 --- /dev/null +++ b/internal/terraform/node_variable_validation.go @@ -0,0 +1,86 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package terraform + +import ( + "fmt" + "slices" + + "github.com/hashicorp/terraform/internal/addrs" + "github.com/hashicorp/terraform/internal/configs" + "github.com/hashicorp/terraform/internal/lang/langrefs" +) + +// nodeVariableValidation checks the author-specified validation rules against +// the final value of all expanded instances of a given input variable. +// +// A node of this type should always depend on another node that's responsible +// for deciding the final values for the nominated variable and registering +// them in the current "named values" state. [variableValidationTransformer] +// is the one responsible for inserting nodes of this type and ensuring that +// they each depend on the node that will register the final variable value. +type nodeVariableValidation struct { + configAddr addrs.ConfigInputVariable + rules []*configs.CheckRule +} + +var _ GraphNodeModulePath = (*nodeVariableValidation)(nil) +var _ GraphNodeReferenceable = (*nodeVariableValidation)(nil) +var _ GraphNodeReferencer = (*nodeVariableValidation)(nil) + +func (n *nodeVariableValidation) Name() string { + return fmt.Sprintf("%s (validation)", n.configAddr.String()) +} + +// ModulePath implements [GraphNodeModulePath]. +func (n *nodeVariableValidation) ModulePath() addrs.Module { + return n.configAddr.Module +} + +// ReferenceableAddrs implements [GraphNodeReferenceable], announcing that +// this node contributes to the value for the input variable that it's +// validating, and must therefore run before any nodes that refer to it. +func (n *nodeVariableValidation) ReferenceableAddrs() []addrs.Referenceable { + return []addrs.Referenceable{n.configAddr.Variable} +} + +// References implements [GraphNodeReferencer], announcing anything that +// the check rules refer to, other than the variable that's being validated +// (which gets its dependency connected by [variableValidationTransformer] +// instead). +func (n *nodeVariableValidation) References() []*addrs.Reference { + var ret []*addrs.Reference + for _, rule := range n.rules { + // We ignore all diagnostics here because if an expression contains + // invalid references then we'll catch them once we visit the + // node (method Execute). + condRefs, _ := langrefs.ReferencesInExpr(addrs.ParseRef, rule.Condition) + msgRefs, _ := langrefs.ReferencesInExpr(addrs.ParseRef, rule.ErrorMessage) + ret = n.appendRefsFilterSelf(ret, condRefs...) + ret = n.appendRefsFilterSelf(ret, msgRefs...) + } + return ret +} + +// appendRefsFilterSelf is a specialized version of builtin [append] that +// ignores any new references to the input variable represented by the +// reciever. +func (n *nodeVariableValidation) appendRefsFilterSelf(to []*addrs.Reference, new ...*addrs.Reference) []*addrs.Reference { + // We need to filter out any self-references, because those would + // make the resulting graph invalid and we don't need them because + // variableValidationTransformer should've arranged for us to + // already depend on whatever node provides the final value for + // this variable. + ret := slices.Grow(to, len(new)) + ourAddr := n.configAddr.Variable + for _, ref := range new { + if refAddr, ok := ref.Subject.(addrs.InputVariable); ok { + if refAddr == ourAddr { + continue + } + } + ret = append(ret, ref) + } + return ret +} diff --git a/internal/terraform/transform_variable_validation.go b/internal/terraform/transform_variable_validation.go new file mode 100644 index 0000000000..d7c0200c75 --- /dev/null +++ b/internal/terraform/transform_variable_validation.go @@ -0,0 +1,60 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package terraform + +import ( + "github.com/hashicorp/terraform/internal/addrs" + "github.com/hashicorp/terraform/internal/configs" + "github.com/hashicorp/terraform/internal/dag" +) + +// graphNodeValidatableVariable is implemented by nodes that represent +// input variables, and which must therefore have variable validation +// nodes created alongside them to verify that the final value matches +// the author's validation rules. +type graphNodeValidatableVariable interface { + variableValidationRules() (addrs.ConfigInputVariable, []*configs.CheckRule) +} + +// Correct behavior requires both of the input variable node types to +// announce themselves as producing final input variable values that need +// to be validated. +var _ graphNodeValidatableVariable = (*NodeRootVariable)(nil) +var _ graphNodeValidatableVariable = (*nodeExpandModuleVariable)(nil) + +// variableValidationTransformer searches the given graph for any nodes +// that implement [graphNodeValidatableVariable]. For each one found, it +// inserts a new [nodeVariableValidation] and makes it depend on the original +// node, to cause the validation action to happen only after the variable's +// final value has been registered. +// +// This transformer should run after any transformer that might insert a +// node that implements [graphNodeValidatableVariable], and before the +// [ReferenceTransformer] because references like "var.foo" must be connected +// with the new [nodeVariableValidation] nodes to prevent downstream nodes +// from relying on unvalidated values. +type variableValidationTransformer struct { +} + +var _ GraphTransformer = (*variableValidationTransformer)(nil) + +func (t *variableValidationTransformer) Transform(g *Graph) error { + for _, v := range g.Vertices() { + v, ok := v.(graphNodeValidatableVariable) + if !ok { + continue // irrelevant node + } + + configAddr, rules := v.variableValidationRules() + + newV := &nodeVariableValidation{ + configAddr: configAddr, + rules: rules, + } + + g.Add(newV) + g.Connect(dag.BasicEdge(newV, v)) + } + return nil +} diff --git a/internal/terraform/transform_variable_validation_test.go b/internal/terraform/transform_variable_validation_test.go new file mode 100644 index 0000000000..4052625883 --- /dev/null +++ b/internal/terraform/transform_variable_validation_test.go @@ -0,0 +1,140 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package terraform + +import ( + "fmt" + "strings" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/hashicorp/hcl/v2/hcltest" + "github.com/zclconf/go-cty/cty" + + "github.com/hashicorp/terraform/internal/addrs" + "github.com/hashicorp/terraform/internal/configs" +) + +func TestVariableValidationTransformer(t *testing.T) { + // This is a unit test focused on just the validation transformer's + // behavior, which assumes that the caller correctly arranges for + // its invariants to be met: + // 1. all input variable evaluation nodes must already be present + // in the graph before running the transformer. + // 2. the reference transformer must run after this transformer. + // + // To avoid this depending on transformers other than the one we're + // testing we'll arrange for those to be met in a rather artificial + // way. Our integration tests complement this by verifying that + // the variable validation feature as a whole is working. For + // example: [TestContext2Plan_variableValidation]. + + g := &Graph{} + fooNode := &nodeTestOnlyInputVariable{ + configAddr: addrs.InputVariable{Name: "foo"}.InModule(addrs.RootModule), + rules: []*configs.CheckRule{ + { + // The condition contains a self-reference, which is required + // for a realistic input variable validation because otherwise + // it wouldn't actually be checking the variable it's + // supposed to be validating. (This transformer is not the + // one responsible for validating that though, so it's + // okay for the examples below to not meet that requirement.) + Condition: hcltest.MockExprTraversalSrc("var.foo"), + ErrorMessage: hcltest.MockExprLiteral(cty.StringVal("wrong")), + }, + }, + } + barNode := &nodeTestOnlyInputVariable{ + configAddr: addrs.InputVariable{Name: "bar"}.InModule(addrs.RootModule), + rules: []*configs.CheckRule{ + { + // The condition of this one refers to var.foo + Condition: hcltest.MockExprTraversalSrc("var.foo"), + ErrorMessage: hcltest.MockExprLiteral(cty.StringVal("wrong")), + }, + }, + } + bazNode := &nodeTestOnlyInputVariable{ + configAddr: addrs.InputVariable{Name: "baz"}.InModule(addrs.RootModule), + rules: []*configs.CheckRule{ + { + // The error message of this one refers to var.foo + Condition: hcltest.MockExprLiteral(cty.False), + ErrorMessage: hcltest.MockExprTraversalSrc("var.foo"), + }, + }, + } + g.Add(fooNode) + g.Add(barNode) + g.Add(bazNode) + + transformer := &variableValidationTransformer{} + err := transformer.Transform(g) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + + gotStr := strings.TrimSpace(g.String()) + wantStr := strings.TrimSpace(` +var.bar (test fake) +var.bar (validation) + var.bar (test fake) +var.baz (test fake) +var.baz (validation) + var.baz (test fake) +var.foo (test fake) +var.foo (validation) + var.foo (test fake) +`) + if diff := cmp.Diff(wantStr, gotStr); diff != "" { + t.Errorf("wrong graph after transform\n%s", diff) + } + + // This transformer is not responsible for wiring up dependencies based + // on references -- that's ReferenceTransformer's job -- but we'll + // verify that the nodes that were added by this transformer do at least + // report the references we expect them to report, in the way that + // ReferenceTransformer would expect. + gotRefs := map[string]map[string]struct{}{} + for _, v := range g.Vertices() { + v, ok := v.(*nodeVariableValidation) // the type of all nodes that this transformer adds + if !ok { + continue + } + var _ GraphNodeReferencer = v // static assertion just to make sure we'll fail to compile if GraphNodeReferencer changes later + + refs := v.References() + gotRefs[v.Name()] = map[string]struct{}{} + for _, ref := range refs { + gotRefs[v.Name()][ref.Subject.String()] = struct{}{} + } + } + wantRefs := map[string]map[string]struct{}{ + "var.bar (validation)": { + "var.foo": struct{}{}, + }, + "var.baz (validation)": { + "var.foo": struct{}{}, + }, + "var.foo (validation)": {}, + } + if diff := cmp.Diff(wantRefs, gotRefs); diff != "" { + t.Errorf("wrong references for the added nodes\n%s", diff) + } +} + +type nodeTestOnlyInputVariable struct { + configAddr addrs.ConfigInputVariable + rules []*configs.CheckRule +} + +func (n *nodeTestOnlyInputVariable) Name() string { + return fmt.Sprintf("%s (test fake)", n.configAddr) +} + +// variableValidationRules implements [graphNodeValidatableVariable]. +func (n *nodeTestOnlyInputVariable) variableValidationRules() (addrs.ConfigInputVariable, []*configs.CheckRule) { + return n.configAddr, n.rules +}