From 0e963db2c52bb860be535ae22fab32a8cc8a8682 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 5 Apr 2017 15:34:59 -0700 Subject: [PATCH] Detect and reject unknown attributes in "connection" blocks Since the validation of connection blocks is delegated to the communicator selected by "type", we were not previously doing any validation of the attribute names in these blocks until running provisioners during apply. Proper validation here requires us to already have the instance state, since the final connection info is a merge of values provided in config with values assigned automatically by the resource. However, we can do some basic name validation to catch typos during the validation pass, even though semantic validation and checking for missing attributes will still wait until the provisioner is instantiated. This fixes #6582 as much as we reasonably can. --- terraform/eval_validate.go | 80 ++++++++++++++++++- terraform/eval_validate_test.go | 114 ++++++++++++++++++++++++++++ terraform/node_resource_validate.go | 34 ++++++--- 3 files changed, 216 insertions(+), 12 deletions(-) diff --git a/terraform/eval_validate.go b/terraform/eval_validate.go index a2c122d6a..478aa6400 100644 --- a/terraform/eval_validate.go +++ b/terraform/eval_validate.go @@ -4,6 +4,7 @@ import ( "fmt" "github.com/hashicorp/terraform/config" + "github.com/mitchellh/mapstructure" ) // EvalValidateError is the error structure returned if there were @@ -85,12 +86,31 @@ func (n *EvalValidateProvider) Eval(ctx EvalContext) (interface{}, error) { type EvalValidateProvisioner struct { Provisioner *ResourceProvisioner Config **ResourceConfig + ConnConfig **ResourceConfig } func (n *EvalValidateProvisioner) Eval(ctx EvalContext) (interface{}, error) { provisioner := *n.Provisioner config := *n.Config - warns, errs := provisioner.Validate(config) + var warns []string + var errs []error + + { + // Validate the provisioner's own config first + w, e := provisioner.Validate(config) + warns = append(warns, w...) + errs = append(errs, e...) + } + + { + // Now validate the connection config, which might either be from + // the provisioner block itself or inherited from the resource's + // shared connection info. + w, e := n.validateConnConfig(*n.ConnConfig) + warns = append(warns, w...) + errs = append(errs, e...) + } + if len(warns) == 0 && len(errs) == 0 { return nil, nil } @@ -101,6 +121,64 @@ func (n *EvalValidateProvisioner) Eval(ctx EvalContext) (interface{}, error) { } } +func (n *EvalValidateProvisioner) validateConnConfig(connConfig *ResourceConfig) (warns []string, errs []error) { + // 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. + + type connConfigSuperset struct { + // All attribute types are interface{} here because at this point we + // may still have unresolved interpolation expressions, which will + // appear as strings regardless of the final goal type. + + Type interface{} `mapstructure:"type"` + User interface{} `mapstructure:"user"` + Password interface{} `mapstructure:"password"` + Host interface{} `mapstructure:"host"` + Port interface{} `mapstructure:"port"` + Timeout interface{} `mapstructure:"timeout"` + ScriptPath interface{} `mapstructure:"script_path"` + + // For type=ssh only (enforced in ssh communicator) + PrivateKey interface{} `mapstructure:"private_key"` + Agent interface{} `mapstructure:"agent"` + BastionHost interface{} `mapstructure:"bastion_host"` + BastionPort interface{} `mapstructure:"bastion_port"` + BastionUser interface{} `mapstructure:"bastion_user"` + BastionPassword interface{} `mapstructure:"bastion_password"` + BastionPrivateKey interface{} `mapstructure:"bastion_private_key"` + + // For type=winrm only (enforced in winrm communicator) + HTTPS interface{} `mapstructure:"https"` + Insecure interface{} `mapstructure:"insecure"` + CACert interface{} `mapstructure:"cacert"` + } + + var metadata mapstructure.Metadata + decoder, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ + Metadata: &metadata, + Result: &connConfigSuperset{}, // result is disregarded; we only care about unused keys + }) + if err != nil { + // should never happen + errs = append(errs, err) + return + } + + if err := decoder.Decode(connConfig.Config); err != nil { + errs = append(errs, err) + return + } + + for _, attrName := range metadata.Unused { + errs = append(errs, fmt.Errorf("unknown 'connection' argument %q", attrName)) + } + return +} + // EvalValidateResource is an EvalNode implementation that validates // the configuration of a resource. type EvalValidateResource struct { diff --git a/terraform/eval_validate_test.go b/terraform/eval_validate_test.go index ac20f617c..0bde2a870 100644 --- a/terraform/eval_validate_test.go +++ b/terraform/eval_validate_test.go @@ -178,3 +178,117 @@ func TestEvalValidateResource_ignoreWarnings(t *testing.T) { t.Fatalf("Expected no error, got: %s", err) } } + +func TestEvalValidateProvisioner_valid(t *testing.T) { + mp := &MockResourceProvisioner{} + var p ResourceProvisioner = mp + ctx := &MockEvalContext{} + + cfg := &ResourceConfig{} + connInfo, err := config.NewRawConfig(map[string]interface{}{}) + if err != nil { + t.Fatalf("failed to make connInfo: %s", err) + } + connConfig := NewResourceConfig(connInfo) + + node := &EvalValidateProvisioner{ + Provisioner: &p, + Config: &cfg, + ConnConfig: &connConfig, + } + + result, err := node.Eval(ctx) + if err != nil { + t.Fatalf("node.Eval failed: %s", err) + } + if result != nil { + t.Errorf("node.Eval returned non-nil result") + } + + if !mp.ValidateCalled { + t.Fatalf("p.Config not called") + } + if mp.ValidateConfig != cfg { + t.Errorf("p.Config called with wrong config") + } +} + +func TestEvalValidateProvisioner_warning(t *testing.T) { + mp := &MockResourceProvisioner{} + var p ResourceProvisioner = mp + ctx := &MockEvalContext{} + + cfg := &ResourceConfig{} + connInfo, err := config.NewRawConfig(map[string]interface{}{}) + if err != nil { + t.Fatalf("failed to make connInfo: %s", err) + } + connConfig := NewResourceConfig(connInfo) + + node := &EvalValidateProvisioner{ + Provisioner: &p, + Config: &cfg, + ConnConfig: &connConfig, + } + + mp.ValidateReturnWarns = []string{"foo is deprecated"} + + _, err = node.Eval(ctx) + if err == nil { + t.Fatalf("node.Eval succeeded; want error") + } + + valErr, ok := err.(*EvalValidateError) + if !ok { + t.Fatalf("node.Eval error is %#v; want *EvalValidateError", valErr) + } + + warns := valErr.Warnings + if warns == nil || len(warns) != 1 { + t.Fatalf("wrong number of warnings in %#v; want one warning", warns) + } + if warns[0] != mp.ValidateReturnWarns[0] { + t.Fatalf("wrong warning %q; want %q", warns[0], mp.ValidateReturnWarns[0]) + } +} + +func TestEvalValidateProvisioner_connectionInvalid(t *testing.T) { + var p ResourceProvisioner = &MockResourceProvisioner{} + ctx := &MockEvalContext{} + + cfg := &ResourceConfig{} + connInfo, err := config.NewRawConfig(map[string]interface{}{ + "bananananananana": "foo", + "bazaz": "bar", + }) + if err != nil { + t.Fatalf("failed to make connInfo: %s", err) + } + connConfig := NewResourceConfig(connInfo) + + node := &EvalValidateProvisioner{ + Provisioner: &p, + Config: &cfg, + ConnConfig: &connConfig, + } + + _, err = node.Eval(ctx) + if err == nil { + t.Fatalf("node.Eval succeeded; want error") + } + + valErr, ok := err.(*EvalValidateError) + if !ok { + t.Fatalf("node.Eval error is %#v; want *EvalValidateError", valErr) + } + + errs := valErr.Errors + if errs == nil || len(errs) != 2 { + t.Fatalf("wrong number of errors in %#v; want two errors", errs) + } + + errStr := errs[0].Error() + if !(strings.Contains(errStr, "bananananananana") || strings.Contains(errStr, "bazaz")) { + t.Fatalf("wrong first error %q; want something about our invalid connInfo keys", errStr) + } +} diff --git a/terraform/node_resource_validate.go b/terraform/node_resource_validate.go index e01518de4..f528f24b1 100644 --- a/terraform/node_resource_validate.go +++ b/terraform/node_resource_validate.go @@ -129,17 +129,29 @@ func (n *NodeValidatableResourceInstance) EvalTree() EvalNode { // Validate all the provisioners for _, p := range n.Config.Provisioners { var provisioner ResourceProvisioner - seq.Nodes = append(seq.Nodes, &EvalGetProvisioner{ - Name: p.Type, - Output: &provisioner, - }, &EvalInterpolate{ - Config: p.RawConfig.Copy(), - Resource: resource, - Output: &config, - }, &EvalValidateProvisioner{ - Provisioner: &provisioner, - Config: &config, - }) + var connConfig *ResourceConfig + seq.Nodes = append( + seq.Nodes, + &EvalGetProvisioner{ + Name: p.Type, + Output: &provisioner, + }, + &EvalInterpolate{ + Config: p.RawConfig.Copy(), + Resource: resource, + Output: &config, + }, + &EvalInterpolate{ + Config: p.ConnInfo.Copy(), + Resource: resource, + Output: &connConfig, + }, + &EvalValidateProvisioner{ + Provisioner: &provisioner, + Config: &config, + ConnConfig: &connConfig, + }, + ) } return seq