From fbe3219fbe6813c122a846314e5661d96d5d78c8 Mon Sep 17 00:00:00 2001 From: Kristin Laemmert Date: Tue, 8 Dec 2020 13:17:31 -0500 Subject: [PATCH] terraform: refactor EvalValidateProvisioner EvalValidateProvisioner is now a method on NodeValidatableResource. --- terraform/eval_validate.go | 218 ----------------------- terraform/eval_validate_test.go | 150 ---------------- terraform/node_resource_validate.go | 218 ++++++++++++++++++++--- terraform/node_resource_validate_test.go | 151 ++++++++++++++++ 4 files changed, 349 insertions(+), 388 deletions(-) create mode 100644 terraform/node_resource_validate_test.go diff --git a/terraform/eval_validate.go b/terraform/eval_validate.go index 114fcc333..300293e8e 100644 --- a/terraform/eval_validate.go +++ b/terraform/eval_validate.go @@ -6,9 +6,7 @@ import ( "github.com/hashicorp/hcl/v2" "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/configs" - "github.com/hashicorp/terraform/configs/configschema" "github.com/hashicorp/terraform/providers" - "github.com/hashicorp/terraform/provisioners" "github.com/hashicorp/terraform/tfdiags" "github.com/zclconf/go-cty/cty" "github.com/zclconf/go-cty/cty/convert" @@ -18,222 +16,6 @@ import ( // EvalValidateProvisioner validates the configuration of a provisioner // belonging to a resource. The provisioner config is expected to contain the // merged connection configurations. -type EvalValidateProvisioner struct { - ResourceAddr addrs.Resource - Provisioner *provisioners.Interface - Schema **configschema.Block - Config *configs.Provisioner - ResourceHasCount bool - ResourceHasForEach bool -} - -func (n *EvalValidateProvisioner) Validate(ctx EvalContext) error { - provisioner := *n.Provisioner - config := *n.Config - schema := *n.Schema - - var diags tfdiags.Diagnostics - - // Validate the provisioner's own config first - configVal, _, configDiags := n.evaluateBlock(ctx, config.Config, schema) - diags = diags.Append(configDiags) - if configDiags.HasErrors() { - return diags.Err() - } - - if configVal == cty.NilVal { - // Should never happen for a well-behaved EvaluateBlock implementation - return fmt.Errorf("EvaluateBlock returned nil value") - } - - req := provisioners.ValidateProvisionerConfigRequest{ - Config: configVal, - } - - resp := provisioner.ValidateProvisionerConfig(req) - diags = diags.Append(resp.Diagnostics) - - // Now validate the connection config, which contains the merged bodies - // of the resource and provisioner connection blocks. - connDiags := n.validateConnConfig(ctx, config.Connection, n.ResourceAddr) - diags = diags.Append(connDiags) - - return diags.NonFatalErr() -} - -func (n *EvalValidateProvisioner) validateConnConfig(ctx EvalContext, config *configs.Connection, self addrs.Referenceable) tfdiags.Diagnostics { - // We can't comprehensively validate the connection config since its - // final structure is decided by the communicator and we can't instantiate - // that until we have a complete instance state. However, we *can* catch - // configuration keys that are not valid for *any* communicator, catching - // typos early rather than waiting until we actually try to run one of - // the resource's provisioners. - - var diags tfdiags.Diagnostics - - if config == nil || config.Config == nil { - // No block to validate - return diags - } - - // We evaluate here just by evaluating the block and returning any - // diagnostics we get, since evaluation alone is enough to check for - // extraneous arguments and incorrectly-typed arguments. - _, _, configDiags := n.evaluateBlock(ctx, config.Config, connectionBlockSupersetSchema) - diags = diags.Append(configDiags) - - return diags -} - -func (n *EvalValidateProvisioner) evaluateBlock(ctx EvalContext, body hcl.Body, schema *configschema.Block) (cty.Value, hcl.Body, tfdiags.Diagnostics) { - keyData := EvalDataForNoInstanceKey - selfAddr := n.ResourceAddr.Instance(addrs.NoKey) - - if n.ResourceHasCount { - // For a resource that has count, we allow count.index but don't - // know at this stage what it will return. - keyData = InstanceKeyEvalData{ - CountIndex: cty.UnknownVal(cty.Number), - } - - // "self" can't point to an unknown key, but we'll force it to be - // key 0 here, which should return an unknown value of the - // expected type since none of these elements are known at this - // point anyway. - selfAddr = n.ResourceAddr.Instance(addrs.IntKey(0)) - } else if n.ResourceHasForEach { - // For a resource that has for_each, we allow each.value and each.key - // but don't know at this stage what it will return. - keyData = InstanceKeyEvalData{ - EachKey: cty.UnknownVal(cty.String), - EachValue: cty.DynamicVal, - } - - // "self" can't point to an unknown key, but we'll force it to be - // key "" here, which should return an unknown value of the - // expected type since none of these elements are known at - // this point anyway. - selfAddr = n.ResourceAddr.Instance(addrs.StringKey("")) - } - - return ctx.EvaluateBlock(body, schema, selfAddr, keyData) -} - -// connectionBlockSupersetSchema is a schema representing the superset of all -// possible arguments for "connection" blocks across all supported connection -// types. -// -// This currently lives here because we've not yet updated our communicator -// subsystem to be aware of schema itself. Once that is done, we can remove -// this and use a type-specific schema from the communicator to validate -// exactly what is expected for a given connection type. -var connectionBlockSupersetSchema = &configschema.Block{ - Attributes: map[string]*configschema.Attribute{ - // NOTE: "type" is not included here because it's treated special - // by the config loader and stored away in a separate field. - - // Common attributes for both connection types - "host": { - Type: cty.String, - Required: true, - }, - "type": { - Type: cty.String, - Optional: true, - }, - "user": { - Type: cty.String, - Optional: true, - }, - "password": { - Type: cty.String, - Optional: true, - }, - "port": { - Type: cty.String, - Optional: true, - }, - "timeout": { - Type: cty.String, - Optional: true, - }, - "script_path": { - Type: cty.String, - Optional: true, - }, - // For type=ssh only (enforced in ssh communicator) - "target_platform": { - Type: cty.String, - Optional: true, - }, - "private_key": { - Type: cty.String, - Optional: true, - }, - "certificate": { - Type: cty.String, - Optional: true, - }, - "host_key": { - Type: cty.String, - Optional: true, - }, - "agent": { - Type: cty.Bool, - Optional: true, - }, - "agent_identity": { - Type: cty.String, - Optional: true, - }, - "bastion_host": { - Type: cty.String, - Optional: true, - }, - "bastion_host_key": { - Type: cty.String, - Optional: true, - }, - "bastion_port": { - Type: cty.Number, - Optional: true, - }, - "bastion_user": { - Type: cty.String, - Optional: true, - }, - "bastion_password": { - Type: cty.String, - Optional: true, - }, - "bastion_private_key": { - Type: cty.String, - Optional: true, - }, - "bastion_certificate": { - Type: cty.String, - Optional: true, - }, - - // For type=winrm only (enforced in winrm communicator) - "https": { - Type: cty.Bool, - Optional: true, - }, - "insecure": { - Type: cty.Bool, - Optional: true, - }, - "cacert": { - Type: cty.String, - Optional: true, - }, - "use_ntlm": { - Type: cty.Bool, - Optional: true, - }, - }, -} // EvalValidateResource validates the configuration of a resource. type EvalValidateResource struct { diff --git a/terraform/eval_validate_test.go b/terraform/eval_validate_test.go index 4d23704e6..1efd28c37 100644 --- a/terraform/eval_validate_test.go +++ b/terraform/eval_validate_test.go @@ -11,9 +11,7 @@ import ( "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/configs" - "github.com/hashicorp/terraform/configs/configschema" "github.com/hashicorp/terraform/providers" - "github.com/hashicorp/terraform/provisioners" "github.com/hashicorp/terraform/tfdiags" ) @@ -337,151 +335,3 @@ func TestEvalValidateResource_invalidDependsOn(t *testing.T) { t.Fatalf("wrong error\ngot: %s\nwant: Message containing %q", got, want) } } - -func TestEvalValidateProvisioner_valid(t *testing.T) { - mp := &MockProvisioner{} - var p provisioners.Interface = mp - ctx := &MockEvalContext{} - ctx.installSimpleEval() - - schema := &configschema.Block{} - - node := &EvalValidateProvisioner{ - ResourceAddr: addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "foo", - Name: "bar", - }, - Provisioner: &p, - Schema: &schema, - Config: &configs.Provisioner{ - Type: "baz", - Config: hcl.EmptyBody(), - Connection: &configs.Connection{ - Config: configs.SynthBody("", map[string]cty.Value{ - "host": cty.StringVal("localhost"), - "type": cty.StringVal("ssh"), - }), - }, - }, - } - - err := node.Validate(ctx) - if err != nil { - t.Fatalf("node.Eval failed: %s", err) - } - if !mp.ValidateProvisionerConfigCalled { - t.Fatalf("p.ValidateProvisionerConfig not called") - } -} - -func TestEvalValidateProvisioner_warning(t *testing.T) { - mp := &MockProvisioner{} - var p provisioners.Interface = mp - ctx := &MockEvalContext{} - ctx.installSimpleEval() - - schema := &configschema.Block{ - Attributes: map[string]*configschema.Attribute{ - "type": { - Type: cty.String, - Optional: true, - }, - }, - } - - node := &EvalValidateProvisioner{ - ResourceAddr: addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "foo", - Name: "bar", - }, - Provisioner: &p, - Schema: &schema, - Config: &configs.Provisioner{ - Type: "baz", - Config: hcl.EmptyBody(), - Connection: &configs.Connection{ - Config: configs.SynthBody("", map[string]cty.Value{ - "host": cty.StringVal("localhost"), - "type": cty.StringVal("ssh"), - }), - }, - }, - } - - { - var diags tfdiags.Diagnostics - diags = diags.Append(tfdiags.SimpleWarning("foo is deprecated")) - mp.ValidateProvisionerConfigResponse = provisioners.ValidateProvisionerConfigResponse{ - Diagnostics: diags, - } - } - - err := node.Validate(ctx) - if err == nil { - t.Fatalf("node.Eval succeeded; want error") - } - - var diags tfdiags.Diagnostics - diags = diags.Append(err) - if len(diags) != 1 { - t.Fatalf("wrong number of diagnostics in %s; want one warning", diags.ErrWithWarnings()) - } - - if got, want := diags[0].Description().Summary, mp.ValidateProvisionerConfigResponse.Diagnostics[0].Description().Summary; got != want { - t.Fatalf("wrong warning %q; want %q", got, want) - } -} - -func TestEvalValidateProvisioner_connectionInvalid(t *testing.T) { - var p provisioners.Interface = &MockProvisioner{} - ctx := &MockEvalContext{} - ctx.installSimpleEval() - - schema := &configschema.Block{ - Attributes: map[string]*configschema.Attribute{ - "type": { - Type: cty.String, - Optional: true, - }, - }, - } - - node := &EvalValidateProvisioner{ - ResourceAddr: addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "foo", - Name: "bar", - }, - Provisioner: &p, - Schema: &schema, - Config: &configs.Provisioner{ - Type: "baz", - Config: hcl.EmptyBody(), - Connection: &configs.Connection{ - Config: configs.SynthBody("", map[string]cty.Value{ - "type": cty.StringVal("ssh"), - "bananananananana": cty.StringVal("foo"), - "bazaz": cty.StringVal("bar"), - }), - }, - }, - } - - err := node.Validate(ctx) - if err == nil { - t.Fatalf("node.Eval succeeded; want error") - } - - var diags tfdiags.Diagnostics - diags = diags.Append(err) - if len(diags) != 3 { - t.Fatalf("wrong number of diagnostics; want two errors\n\n%s", diags.Err()) - } - - errStr := diags.Err().Error() - if !(strings.Contains(errStr, "bananananananana") && strings.Contains(errStr, "bazaz")) { - t.Fatalf("wrong errors %q; want something about each of our invalid connInfo keys", errStr) - } -} diff --git a/terraform/node_resource_validate.go b/terraform/node_resource_validate.go index 3a5bd6d0c..8e9b4b3c9 100644 --- a/terraform/node_resource_validate.go +++ b/terraform/node_resource_validate.go @@ -3,8 +3,11 @@ package terraform import ( "fmt" + "github.com/hashicorp/hcl/v2" "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/configs" + "github.com/hashicorp/terraform/configs/configschema" + "github.com/hashicorp/terraform/provisioners" "github.com/hashicorp/terraform/tfdiags" "github.com/zclconf/go-cty/cty" ) @@ -71,27 +74,8 @@ func (n *NodeValidatableResource) Execute(ctx EvalContext, op walkOperation) (di p.Connection.Config = configs.MergeBodies(config.Managed.Connection.Config, p.Connection.Config) } - provisioner := ctx.Provisioner(p.Type) - if provisioner == nil { - diags = diags.Append(fmt.Errorf("provisioner %s not initialized", p.Type)) - return diags - } - provisionerSchema := ctx.ProvisionerSchema(p.Type) - if provisionerSchema == nil { - diags = diags.Append(fmt.Errorf("provisioner %s not initialized", p.Type)) - return diags - } - // Validate Provisioner Config - validateProvisioner := &EvalValidateProvisioner{ - ResourceAddr: addr.Resource, - Provisioner: &provisioner, - Schema: &provisionerSchema, - Config: p, - ResourceHasCount: hasCount, - ResourceHasForEach: hasForEach, - } - diags = diags.Append(validateProvisioner.Validate(ctx)) + diags = diags.Append(n.validateProvisioner(ctx, p, hasCount, hasForEach)) if diags.HasErrors() { return diags } @@ -99,3 +83,197 @@ func (n *NodeValidatableResource) Execute(ctx EvalContext, op walkOperation) (di } return diags } + +// validateProvisioner validates the configuration of a provisioner belonging to +// a resource. The provisioner config is expected to contain the merged +// connection configurations. +func (n *NodeValidatableResource) validateProvisioner(ctx EvalContext, p *configs.Provisioner, hasCount, hasForEach bool) tfdiags.Diagnostics { + var diags tfdiags.Diagnostics + + provisioner := ctx.Provisioner(p.Type) + if provisioner == nil { + return diags.Append(fmt.Errorf("provisioner %s not initialized", p.Type)) + } + provisionerSchema := ctx.ProvisionerSchema(p.Type) + if provisionerSchema == nil { + return diags.Append(fmt.Errorf("provisioner %s not initialized", p.Type)) + } + + // Validate the provisioner's own config first + configVal, _, configDiags := n.evaluateBlock(ctx, p.Config, provisionerSchema, hasCount, hasForEach) + diags = diags.Append(configDiags) + + if configVal == cty.NilVal { + // Should never happen for a well-behaved EvaluateBlock implementation + return diags.Append(fmt.Errorf("EvaluateBlock returned nil value")) + } + + req := provisioners.ValidateProvisionerConfigRequest{ + Config: configVal, + } + + resp := provisioner.ValidateProvisionerConfig(req) + diags = diags.Append(resp.Diagnostics) + + if p.Connection != nil { + // We can't comprehensively validate the connection config since its + // final structure is decided by the communicator and we can't instantiate + // that until we have a complete instance state. However, we *can* catch + // configuration keys that are not valid for *any* communicator, catching + // typos early rather than waiting until we actually try to run one of + // the resource's provisioners. + _, _, connDiags := n.evaluateBlock(ctx, p.Connection.Config, connectionBlockSupersetSchema, hasCount, hasForEach) + diags = diags.Append(connDiags) + } + return diags +} + +func (n *NodeValidatableResource) evaluateBlock(ctx EvalContext, body hcl.Body, schema *configschema.Block, hasCount, hasForEach bool) (cty.Value, hcl.Body, tfdiags.Diagnostics) { + keyData := EvalDataForNoInstanceKey + selfAddr := n.ResourceAddr().Resource.Instance(addrs.NoKey) + + if hasCount { + // For a resource that has count, we allow count.index but don't + // know at this stage what it will return. + keyData = InstanceKeyEvalData{ + CountIndex: cty.UnknownVal(cty.Number), + } + + // "self" can't point to an unknown key, but we'll force it to be + // key 0 here, which should return an unknown value of the + // expected type since none of these elements are known at this + // point anyway. + selfAddr = n.ResourceAddr().Resource.Instance(addrs.IntKey(0)) + } else if hasForEach { + // For a resource that has for_each, we allow each.value and each.key + // but don't know at this stage what it will return. + keyData = InstanceKeyEvalData{ + EachKey: cty.UnknownVal(cty.String), + EachValue: cty.DynamicVal, + } + + // "self" can't point to an unknown key, but we'll force it to be + // key "" here, which should return an unknown value of the + // expected type since none of these elements are known at + // this point anyway. + selfAddr = n.ResourceAddr().Resource.Instance(addrs.StringKey("")) + } + + return ctx.EvaluateBlock(body, schema, selfAddr, keyData) +} + +// connectionBlockSupersetSchema is a schema representing the superset of all +// possible arguments for "connection" blocks across all supported connection +// types. +// +// This currently lives here because we've not yet updated our communicator +// subsystem to be aware of schema itself. Once that is done, we can remove +// this and use a type-specific schema from the communicator to validate +// exactly what is expected for a given connection type. +var connectionBlockSupersetSchema = &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + // NOTE: "type" is not included here because it's treated special + // by the config loader and stored away in a separate field. + + // Common attributes for both connection types + "host": { + Type: cty.String, + Required: true, + }, + "type": { + Type: cty.String, + Optional: true, + }, + "user": { + Type: cty.String, + Optional: true, + }, + "password": { + Type: cty.String, + Optional: true, + }, + "port": { + Type: cty.String, + Optional: true, + }, + "timeout": { + Type: cty.String, + Optional: true, + }, + "script_path": { + Type: cty.String, + Optional: true, + }, + // For type=ssh only (enforced in ssh communicator) + "target_platform": { + Type: cty.String, + Optional: true, + }, + "private_key": { + Type: cty.String, + Optional: true, + }, + "certificate": { + Type: cty.String, + Optional: true, + }, + "host_key": { + Type: cty.String, + Optional: true, + }, + "agent": { + Type: cty.Bool, + Optional: true, + }, + "agent_identity": { + Type: cty.String, + Optional: true, + }, + "bastion_host": { + Type: cty.String, + Optional: true, + }, + "bastion_host_key": { + Type: cty.String, + Optional: true, + }, + "bastion_port": { + Type: cty.Number, + Optional: true, + }, + "bastion_user": { + Type: cty.String, + Optional: true, + }, + "bastion_password": { + Type: cty.String, + Optional: true, + }, + "bastion_private_key": { + Type: cty.String, + Optional: true, + }, + "bastion_certificate": { + Type: cty.String, + Optional: true, + }, + + // For type=winrm only (enforced in winrm communicator) + "https": { + Type: cty.Bool, + Optional: true, + }, + "insecure": { + Type: cty.Bool, + Optional: true, + }, + "cacert": { + Type: cty.String, + Optional: true, + }, + "use_ntlm": { + Type: cty.Bool, + Optional: true, + }, + }, +} diff --git a/terraform/node_resource_validate_test.go b/terraform/node_resource_validate_test.go new file mode 100644 index 000000000..6c94bffeb --- /dev/null +++ b/terraform/node_resource_validate_test.go @@ -0,0 +1,151 @@ +package terraform + +import ( + "strings" + "testing" + + "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/terraform/addrs" + "github.com/hashicorp/terraform/configs" + "github.com/hashicorp/terraform/configs/configschema" + "github.com/hashicorp/terraform/provisioners" + "github.com/hashicorp/terraform/tfdiags" + "github.com/zclconf/go-cty/cty" +) + +func TestNodeValidatableResource_ValidateProvisioner_valid(t *testing.T) { + ctx := &MockEvalContext{} + ctx.installSimpleEval() + mp := &MockProvisioner{} + ps := &configschema.Block{} + ctx.ProvisionerSchemaSchema = ps + ctx.ProvisionerProvisioner = mp + + pc := &configs.Provisioner{ + Type: "baz", + Config: hcl.EmptyBody(), + Connection: &configs.Connection{ + Config: configs.SynthBody("", map[string]cty.Value{ + "host": cty.StringVal("localhost"), + "type": cty.StringVal("ssh"), + }), + }, + } + + rc := &configs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_foo", + Name: "bar", + Config: configs.SynthBody("", map[string]cty.Value{}), + } + + node := NodeValidatableResource{ + NodeAbstractResource: &NodeAbstractResource{ + Addr: mustConfigResourceAddr("test_foo.bar"), + Config: rc, + }, + } + + diags := node.validateProvisioner(ctx, pc, false, false) + if diags.HasErrors() { + t.Fatalf("node.Eval failed: %s", diags.Err()) + } + if !mp.ValidateProvisionerConfigCalled { + t.Fatalf("p.ValidateProvisionerConfig not called") + } +} + +func TestNodeValidatableResource_ValidateProvisioner__warning(t *testing.T) { + ctx := &MockEvalContext{} + ctx.installSimpleEval() + mp := &MockProvisioner{} + ps := &configschema.Block{} + ctx.ProvisionerSchemaSchema = ps + ctx.ProvisionerProvisioner = mp + + pc := &configs.Provisioner{ + Type: "baz", + Config: hcl.EmptyBody(), + } + + rc := &configs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_foo", + Name: "bar", + Config: configs.SynthBody("", map[string]cty.Value{}), + Managed: &configs.ManagedResource{}, + } + + node := NodeValidatableResource{ + NodeAbstractResource: &NodeAbstractResource{ + Addr: mustConfigResourceAddr("test_foo.bar"), + Config: rc, + }, + } + + { + var diags tfdiags.Diagnostics + diags = diags.Append(tfdiags.SimpleWarning("foo is deprecated")) + mp.ValidateProvisionerConfigResponse = provisioners.ValidateProvisionerConfigResponse{ + Diagnostics: diags, + } + } + + diags := node.validateProvisioner(ctx, pc, false, false) + if len(diags) != 1 { + t.Fatalf("wrong number of diagnostics in %s; want one warning", diags.ErrWithWarnings()) + } + + if got, want := diags[0].Description().Summary, mp.ValidateProvisionerConfigResponse.Diagnostics[0].Description().Summary; got != want { + t.Fatalf("wrong warning %q; want %q", got, want) + } +} + +func TestNodeValidatableResource_ValidateProvisioner__conntectionInvalid(t *testing.T) { + ctx := &MockEvalContext{} + ctx.installSimpleEval() + mp := &MockProvisioner{} + ps := &configschema.Block{} + ctx.ProvisionerSchemaSchema = ps + ctx.ProvisionerProvisioner = mp + + pc := &configs.Provisioner{ + Type: "baz", + Config: hcl.EmptyBody(), + Connection: &configs.Connection{ + Config: configs.SynthBody("", map[string]cty.Value{ + "type": cty.StringVal("ssh"), + "bananananananana": cty.StringVal("foo"), + "bazaz": cty.StringVal("bar"), + }), + }, + } + + rc := &configs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_foo", + Name: "bar", + Config: configs.SynthBody("", map[string]cty.Value{}), + Managed: &configs.ManagedResource{}, + } + + node := NodeValidatableResource{ + NodeAbstractResource: &NodeAbstractResource{ + Addr: mustConfigResourceAddr("test_foo.bar"), + Config: rc, + }, + } + + diags := node.validateProvisioner(ctx, pc, false, false) + if !diags.HasErrors() { + t.Fatalf("node.Eval succeeded; want error") + } + if len(diags) != 3 { + t.Fatalf("wrong number of diagnostics; want two errors\n\n%s", diags.Err()) + } + + errStr := diags.Err().Error() + if !(strings.Contains(errStr, "bananananananana") && strings.Contains(errStr, "bazaz")) { + t.Fatalf("wrong errors %q; want something about each of our invalid connInfo keys", errStr) + } +}