From b0a83adea4f2912455da6a98e4fc33636e855cae Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 7 Jul 2014 16:56:23 -0700 Subject: [PATCH] terraform: manually interpolate variables in walker functions This avoids issues where we were interpolating when we didn't need to --- terraform/context.go | 30 +++-- terraform/context_test.go | 44 ++++++++ terraform/resource.go | 111 +++++++++++++++++++ terraform/resource_provider.go | 102 ----------------- terraform/test-fixtures/refresh-vars/main.tf | 5 + 5 files changed, 174 insertions(+), 118 deletions(-) create mode 100644 terraform/test-fixtures/refresh-vars/main.tf diff --git a/terraform/context.go b/terraform/context.go index 91415a3b0..57b209d59 100644 --- a/terraform/context.go +++ b/terraform/context.go @@ -416,6 +416,11 @@ func (c *Context) applyWalkFn() depgraph.WalkFunc { } if !diff.Destroy { + // Since we need the configuration, interpolate the variables + if err := r.Config.interpolate(c); err != nil { + return err + } + var err error diff, err = r.Provider.Diff(r.State, r.Config) if err != nil { @@ -503,9 +508,13 @@ func (c *Context) planWalkFn(result *Plan) depgraph.WalkFunc { // This is an orphan (no config), so we mark it to be destroyed diff = &ResourceDiff{Destroy: true} } else { - log.Printf("[DEBUG] %s: Executing diff", r.Id) + // Make sure the configuration is interpolated + if err := r.Config.interpolate(c); err != nil { + return err + } // Get a diff from the newest state + log.Printf("[DEBUG] %s: Executing diff", r.Id) var err error diff, err = r.Provider.Diff(r.State, r.Config) if err != nil { @@ -703,23 +712,12 @@ func (c *Context) genericWalkFn(cb genericWalkFunc) depgraph.WalkFunc { rn := n.Meta.(*GraphNodeResource) - if rn.Config != nil { - if err := c.computeVars(rn.Config.RawConfig); err != nil { - panic(fmt.Sprintf("Interpolate error: %s", err)) - } - - // Force the config to be set later - rn.Resource.Config = nil - } - // Make sure that at least some resource configuration is set if !rn.Orphan { - if rn.Resource.Config == nil { - if rn.Config == nil { - rn.Resource.Config = new(ResourceConfig) - } else { - rn.Resource.Config = NewResourceConfig(rn.Config.RawConfig) - } + if rn.Config == nil { + rn.Resource.Config = new(ResourceConfig) + } else { + rn.Resource.Config = NewResourceConfig(rn.Config.RawConfig) } } else { rn.Resource.Config = nil diff --git a/terraform/context_test.go b/terraform/context_test.go index 51dd95d27..e1ec61724 100644 --- a/terraform/context_test.go +++ b/terraform/context_test.go @@ -1115,6 +1115,50 @@ func TestContextRefresh_state(t *testing.T) { } } +func TestContextRefresh_vars(t *testing.T) { + p := testProvider("aws") + c := testConfig(t, "refresh-vars") + ctx := testContext(t, &ContextOpts{ + Config: c, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + State: &State{ + Resources: map[string]*ResourceState{ + "aws_instance.web": &ResourceState{ + ID: "foo", + Type: "aws_instance", + }, + }, + }, + }) + + p.RefreshFn = nil + p.RefreshReturn = &ResourceState{ + ID: "foo", + } + + s, err := ctx.Refresh() + if err != nil { + t.Fatalf("err: %s", err) + } + if !p.RefreshCalled { + t.Fatal("refresh should be called") + } + if p.RefreshState.ID != "foo" { + t.Fatalf("bad: %#v", p.RefreshState) + } + if !reflect.DeepEqual(s.Resources["aws_instance.web"], p.RefreshReturn) { + t.Fatalf("bad: %#v", s.Resources["aws_instance.web"]) + } + + for _, r := range s.Resources { + if r.Type == "" { + t.Fatalf("no type: %#v", r) + } + } +} + func testContext(t *testing.T, opts *ContextOpts) *Context { return NewContext(opts) } diff --git a/terraform/resource.go b/terraform/resource.go index 202116d63..b32071de0 100644 --- a/terraform/resource.go +++ b/terraform/resource.go @@ -2,6 +2,11 @@ package terraform import ( "fmt" + "reflect" + "strconv" + "strings" + + "github.com/hashicorp/terraform/config" ) // Resource encapsulates a resource, its configuration, its provider, @@ -29,3 +34,109 @@ func (r *Resource) Vars() map[string]string { return vars } + +// ResourceConfig holds the configuration given for a resource. This is +// done instead of a raw `map[string]interface{}` type so that rich +// methods can be added to it to make dealing with it easier. +type ResourceConfig struct { + ComputedKeys []string + Raw map[string]interface{} + Config map[string]interface{} + + raw *config.RawConfig +} + +// NewResourceConfig creates a new ResourceConfig from a config.RawConfig. +func NewResourceConfig(c *config.RawConfig) *ResourceConfig { + result := &ResourceConfig{raw: c} + result.interpolate(nil) + return result +} + +// CheckSet checks that the given list of configuration keys is +// properly set. If not, errors are returned for each unset key. +// +// This is useful to be called in the Validate method of a ResourceProvider. +func (c *ResourceConfig) CheckSet(keys []string) []error { + var errs []error + + for _, k := range keys { + if !c.IsSet(k) { + errs = append(errs, fmt.Errorf("%s must be set", k)) + } + } + + return errs +} + +// Get looks up a configuration value by key and returns the value. +// +// The second return value is true if the get was successful. Get will +// not succeed if the value is being computed. +func (c *ResourceConfig) Get(k string) (interface{}, bool) { + parts := strings.Split(k, ".") + + var current interface{} = c.Raw + for _, part := range parts { + if current == nil { + return nil, false + } + + cv := reflect.ValueOf(current) + switch cv.Kind() { + case reflect.Map: + v := cv.MapIndex(reflect.ValueOf(part)) + if !v.IsValid() { + return nil, false + } + current = v.Interface() + case reflect.Slice: + i, err := strconv.ParseInt(part, 0, 0) + if err != nil { + return nil, false + } + current = cv.Index(int(i)).Interface() + default: + panic(fmt.Sprintf("Unknown kind: %s", cv.Kind())) + } + } + + return current, true +} + +// IsSet checks if the key in the configuration is set. A key is set if +// it has a value or the value is being computed (is unknown currently). +// +// This function should be used rather than checking the keys of the +// raw configuration itself, since a key may be omitted from the raw +// configuration if it is being computed. +func (c *ResourceConfig) IsSet(k string) bool { + if c == nil { + return false + } + + for _, ck := range c.ComputedKeys { + if ck == k { + return true + } + } + + if _, ok := c.Get(k); ok { + return true + } + + return false +} + +func (c *ResourceConfig) interpolate(ctx *Context) error { + if ctx != nil { + if err := ctx.computeVars(c.raw); err != nil { + return err + } + } + + c.ComputedKeys = c.raw.UnknownKeys() + c.Raw = c.raw.Raw + c.Config = c.raw.Config() + return nil +} diff --git a/terraform/resource_provider.go b/terraform/resource_provider.go index 554d31c4f..88f0eeac2 100644 --- a/terraform/resource_provider.go +++ b/terraform/resource_provider.go @@ -1,14 +1,5 @@ package terraform -import ( - "fmt" - "reflect" - "strconv" - "strings" - - "github.com/hashicorp/terraform/config" -) - // ResourceProvider is an interface that must be implemented by any // resource provider: the thing that creates and manages the resources in // a Terraform configuration. @@ -69,15 +60,6 @@ type ResourceProvider interface { Refresh(*ResourceState) (*ResourceState, error) } -// ResourceConfig holds the configuration given for a resource. This is -// done instead of a raw `map[string]interface{}` type so that rich -// methods can be added to it to make dealing with it easier. -type ResourceConfig struct { - ComputedKeys []string - Raw map[string]interface{} - Config map[string]interface{} -} - // ResourceType is a type of resource that a resource provider can manage. type ResourceType struct { Name string @@ -87,90 +69,6 @@ type ResourceType struct { // of a resource provider. type ResourceProviderFactory func() (ResourceProvider, error) -// NewResourceConfig creates a new ResourceConfig from a config.RawConfig. -func NewResourceConfig(c *config.RawConfig) *ResourceConfig { - return &ResourceConfig{ - ComputedKeys: c.UnknownKeys(), - Raw: c.Raw, - Config: c.Config(), - } -} - -// CheckSet checks that the given list of configuration keys is -// properly set. If not, errors are returned for each unset key. -// -// This is useful to be called in the Validate method of a ResourceProvider. -func (c *ResourceConfig) CheckSet(keys []string) []error { - var errs []error - - for _, k := range keys { - if !c.IsSet(k) { - errs = append(errs, fmt.Errorf("%s must be set", k)) - } - } - - return errs -} - -// Get looks up a configuration value by key and returns the value. -// -// The second return value is true if the get was successful. Get will -// not succeed if the value is being computed. -func (c *ResourceConfig) Get(k string) (interface{}, bool) { - parts := strings.Split(k, ".") - - var current interface{} = c.Raw - for _, part := range parts { - if current == nil { - return nil, false - } - - cv := reflect.ValueOf(current) - switch cv.Kind() { - case reflect.Map: - v := cv.MapIndex(reflect.ValueOf(part)) - if !v.IsValid() { - return nil, false - } - current = v.Interface() - case reflect.Slice: - i, err := strconv.ParseInt(part, 0, 0) - if err != nil { - return nil, false - } - current = cv.Index(int(i)).Interface() - default: - panic(fmt.Sprintf("Unknown kind: %s", cv.Kind())) - } - } - - return current, true -} - -// IsSet checks if the key in the configuration is set. A key is set if -// it has a value or the value is being computed (is unknown currently). -// -// This function should be used rather than checking the keys of the -// raw configuration itself, since a key may be omitted from the raw -// configuration if it is being computed. -func (c *ResourceConfig) IsSet(k string) bool { - if c == nil { - return false - } - - for _, ck := range c.ComputedKeys { - if ck == k { - return true - } - } - - if _, ok := c.Get(k); ok { - return true - } - - return false -} - func ProviderSatisfies(p ResourceProvider, n string) bool { for _, rt := range p.Resources() { if rt.Name == n { diff --git a/terraform/test-fixtures/refresh-vars/main.tf b/terraform/test-fixtures/refresh-vars/main.tf new file mode 100644 index 000000000..009514f4f --- /dev/null +++ b/terraform/test-fixtures/refresh-vars/main.tf @@ -0,0 +1,5 @@ +resource "aws_instance" "web" {} + +resource "aws_instance" "db" { + ami = "${aws_instance.web.id}" +}