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.

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

config: smarter provider alias usage validation #10807

Merged
merged 1 commit into from
Dec 19, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 0 additions & 9 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -553,15 +553,6 @@ func (c *Config) Validate() error {
// Validate DependsOn
errs = append(errs, c.validateDependsOn(n, r.DependsOn, resources, modules)...)

// Verify provider points to a provider that is configured
if r.Provider != "" {
if _, ok := providerSet[r.Provider]; !ok {
errs = append(errs, fmt.Errorf(
"%s: resource depends on non-configured provider '%s'",
n, r.Provider))
}
}

// Verify provisioners don't contain any splats
for _, p := range r.Provisioners {
// This validation checks that there are now splat variables
Expand Down
7 changes: 0 additions & 7 deletions config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -456,13 +456,6 @@ func TestConfigValidate_providerMultiRefGood(t *testing.T) {
}
}

func TestConfigValidate_providerMultiRefBad(t *testing.T) {
c := testConfig(t, "validate-provider-multi-ref-bad")
if err := c.Validate(); err == nil {
t.Fatal("should not be valid")
}
}

func TestConfigValidate_provConnSplatOther(t *testing.T) {
c := testConfig(t, "validate-prov-conn-splat-other")
if err := c.Validate(); err != nil {
Expand Down
3 changes: 3 additions & 0 deletions config/module/test-fixtures/validate-alias-bad/child/main.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
resource "aws_instance" "foo" {
provider = "aws.foo"
}
3 changes: 3 additions & 0 deletions config/module/test-fixtures/validate-alias-bad/main.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
module "child" {
source = "./child"
}
3 changes: 3 additions & 0 deletions config/module/test-fixtures/validate-alias-good/child/main.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
resource "aws_instance" "foo" {
provider = "aws.foo"
}
5 changes: 5 additions & 0 deletions config/module/test-fixtures/validate-alias-good/main.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
provider "aws" { alias = "foo" }

module "child" {
source = "./child"
}
8 changes: 8 additions & 0 deletions config/module/tree.go
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,14 @@ func (t *Tree) Validate() error {
return newErr
}

// If we're the root, we do extra validation. This validation usually
// requires the entire tree (since children don't have parent pointers).
if len(t.path) == 0 {
if err := t.validateProviderAlias(); err != nil {
return err
}
}

// Get the child trees
children := t.Children()

Expand Down
44 changes: 44 additions & 0 deletions config/module/tree_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package module

import (
"fmt"
"os"
"reflect"
"strings"
Expand Down Expand Up @@ -267,6 +268,49 @@ func TestTreeName(t *testing.T) {
}
}

// This is a table-driven test for tree validation. This is the preferred
// way to test Validate. Non table-driven tests exist historically but
// that style shouldn't be done anymore.
func TestTreeValidate_table(t *testing.T) {
cases := []struct {
Name string
Fixture string
Err string
}{
{
"provider alias in child",
"validate-alias-good",
"",
},

{
"undefined provider alias in child",
"validate-alias-bad",
"alias must be defined",
},
}

for i, tc := range cases {
t.Run(fmt.Sprintf("%d-%s", i, tc.Name), func(t *testing.T) {
tree := NewTree("", testConfig(t, tc.Fixture))
if err := tree.Load(testStorage(t), GetModeGet); err != nil {
t.Fatalf("err: %s", err)
}

err := tree.Validate()
if (err != nil) != (tc.Err != "") {
t.Fatalf("err: %s", err)
}
if err == nil {
return
}
if !strings.Contains(err.Error(), tc.Err) {
t.Fatalf("err should contain %q: %s", tc.Err, err)
}
})
}
}

func TestTreeValidate_badChild(t *testing.T) {
tree := NewTree("", testConfig(t, "validate-child-bad"))

Expand Down
118 changes: 118 additions & 0 deletions config/module/validate_provider_alias.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
package module

import (
"fmt"
"strings"

"github.com/hashicorp/go-multierror"
"github.com/hashicorp/terraform/dag"
)

// validateProviderAlias validates that all provider alias references are
// defined at some point in the parent tree. This improves UX by catching
// alias typos at the slight cost of requiring a declaration of usage. This
// is usually a good tradeoff since not many aliases are used.
func (t *Tree) validateProviderAlias() error {
// If we're not the root, don't perform this validation. We must be the
// root since we require full tree visibilty.
if len(t.path) != 0 {
return nil
}

// We'll use a graph to keep track of defined aliases at each level.
// As long as a parent defines an alias, it is okay.
var g dag.AcyclicGraph
t.buildProviderAliasGraph(&g, nil)

// Go through the graph and check that the usage is all good.
var err error
for _, v := range g.Vertices() {
pv, ok := v.(*providerAliasVertex)
if !ok {
// This shouldn't happen, just ignore it.
continue
}

// If we're not using any aliases, fast track and just continue
if len(pv.Used) == 0 {
continue
}

// Grab the ancestors since we're going to have to check if our
// parents define any of our aliases.
var parents []*providerAliasVertex
ancestors, _ := g.Ancestors(v)
for _, raw := range ancestors.List() {
if pv, ok := raw.(*providerAliasVertex); ok {
parents = append(parents, pv)
}
}
for k, _ := range pv.Used {
// Check if we define this
if _, ok := pv.Defined[k]; ok {
continue
}

// Check for a parent
found := false
for _, parent := range parents {
_, found = parent.Defined[k]
if found {
break
}
}
if found {
continue
}

// We didn't find the alias, error!
err = multierror.Append(err, fmt.Errorf(
"module %s: provider alias must be defined by the module or a parent: %s",
strings.Join(pv.Path, "."), k))
}
}

return err
}

func (t *Tree) buildProviderAliasGraph(g *dag.AcyclicGraph, parent dag.Vertex) {
// Add all our defined aliases
defined := make(map[string]struct{})
for _, p := range t.config.ProviderConfigs {
defined[p.FullName()] = struct{}{}
}

// Add all our used aliases
used := make(map[string]struct{})
for _, r := range t.config.Resources {
if r.Provider != "" {
used[r.Provider] = struct{}{}
}
}

// Add it to the graph
vertex := &providerAliasVertex{
Path: t.Path(),
Defined: defined,
Used: used,
}
g.Add(vertex)

// Connect to our parent if we have one
if parent != nil {
g.Connect(dag.BasicEdge(vertex, parent))
}

// Build all our children
for _, c := range t.Children() {
c.buildProviderAliasGraph(g, vertex)
}
}

// providerAliasVertex is the vertex for the graph that keeps track of
// defined provider aliases.
type providerAliasVertex struct {
Path []string
Defined map[string]struct{}
Used map[string]struct{}
}