Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Detect and reject unknown attributes in "connection" blocks #13400

Merged
merged 1 commit into from
Apr 6, 2017
Merged

Detect and reject unknown attributes in "connection" blocks #13400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 79 additions & 1 deletion terraform/eval_validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"

"github.com/hashicorp/terraform/config"
"github.com/mitchellh/mapstructure"
)

// EvalValidateError is the error structure returned if there were
Expand Down Expand Up @@ -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
}
Expand All @@ -101,6 +121,64 @@ func (n *EvalValidateProvisioner) Eval(ctx EvalContext) (interface{}, error) {
}
}

func (n *EvalValidateProvisioner) validateConnConfig(connConfig *ResourceConfig) (warns []string, errs []error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this functionality into the communicator package, or at least somehow get the connConfigSuperset definition in there? It wold be nice to have some logical locality next time we're trying to figure out why a new communicator fails validation ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A fair point... I was a little unsure where to place this because the ownership of the "schema" of this block is rather unclear anyway, with the documentation implying that there are some "global attributes" which are in fact just supported by all the communicators by convention.

However, we do have the global communicator package that has the function for instantiating the specific communicators, so assuming that's what you were referring to here then I agree, I'll move it over there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh... it turns out that this is more problematic than I initially thought because the communicator package already depends on the terraform package and so we can't call into it from terraform without creating a dependency cycle.

We've got away with this so far because it's the provisioners that call into communicator and the plugin indirection prevents Go from seeing it as a circular dependency.

I think there is a path here where the communicator interface can be adjusted so that it receives directly the connection info as a map[string]interface{} rather than getting a terraform.InstanceState and pulling it out of there itself, but that's a bit beyond the scope of what I wanted to do for this bug.

Do you think it's reasonable to move ahead with what I had here and address this later, or should I just shelve this for the moment and come back to it when there's more time for such refactoring?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that's what I was afraid of.

connConfig.Config is already just a map[string]interface{}; couldn't this just be a function like

communicator.ValidateConnConfig(cfg map[string]interface{}) error

and pass it ResourceConfig.Config?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's exactly what I tried, but then that adds a dependency from terraform to communicator, which is problematic. In order for us to call from terraform to communicator we need to eliminate all of the terraform dependencies from communicator itself, which I think is possible (it seems to use it only because currently communicators get passed a full terraform.InstanceState on init) but a pretty disruptive interface change for a relatively-minor UX improvement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♂️ sorry, of course, I was worrying about the inverse.

Yeah, lets have this live here for now.

// 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 {
Expand Down
114 changes: 114 additions & 0 deletions terraform/eval_validate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
34 changes: 23 additions & 11 deletions terraform/node_resource_validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down