From 99e706b50280aa3f8c92d674946a11a9209a27ae Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 27 Sep 2016 16:08:54 -0700 Subject: [PATCH 1/5] vendor: update copystructure/reflectwalk This adds fixes that are needed for ResourceConfig copying, namely related to copying interfaces, avoiding unexported fields. --- .../mitchellh/copystructure/copystructure.go | 12 ++++++++++++ .../mitchellh/reflectwalk/reflectwalk.go | 16 +++++++++++++--- vendor/vendor.json | 10 ++++++---- 3 files changed, 31 insertions(+), 7 deletions(-) diff --git a/vendor/github.com/mitchellh/copystructure/copystructure.go b/vendor/github.com/mitchellh/copystructure/copystructure.go index 831c37de85..349d38d61a 100644 --- a/vendor/github.com/mitchellh/copystructure/copystructure.go +++ b/vendor/github.com/mitchellh/copystructure/copystructure.go @@ -295,12 +295,24 @@ func (w *walker) StructField(f reflect.StructField, v reflect.Value) error { return nil } + // If PkgPath is non-empty, this is a private (unexported) field. + // We do not set this unexported since the Go runtime doesn't allow us. + if f.PkgPath != "" { + w.ignore() + return nil + } + // Push the field onto the stack, we'll handle it when we exit // the struct field in Exit... w.valPush(reflect.ValueOf(f)) return nil } +// ignore causes the walker to ignore any more values until we exit this on +func (w *walker) ignore() { + w.ignoreDepth = w.depth +} + func (w *walker) ignoring() bool { return w.ignoreDepth > 0 && w.depth >= w.ignoreDepth } diff --git a/vendor/github.com/mitchellh/reflectwalk/reflectwalk.go b/vendor/github.com/mitchellh/reflectwalk/reflectwalk.go index 1f20665980..fafd443a43 100644 --- a/vendor/github.com/mitchellh/reflectwalk/reflectwalk.go +++ b/vendor/github.com/mitchellh/reflectwalk/reflectwalk.go @@ -4,9 +4,7 @@ // those elements. package reflectwalk -import ( - "reflect" -) +import "reflect" // PrimitiveWalker implementations are able to handle primitive values // within complex structures. Primitive values are numbers, strings, @@ -145,6 +143,12 @@ func walkMap(v reflect.Value, w interface{}) error { for _, k := range v.MapKeys() { kv := v.MapIndex(k) + // if the map value type is an interface, we need to extract the Elem + // for the next call to walk + if kv.Kind() == reflect.Interface { + kv = kv.Elem() + } + if mw, ok := w.(MapWalker); ok { if err := mw.MapElem(v, k, kv); err != nil { return err @@ -204,6 +208,12 @@ func walkSlice(v reflect.Value, w interface{}) (err error) { for i := 0; i < v.Len(); i++ { elem := v.Index(i) + // if the value type is an interface, we need to extract the Elem + // for the next call to walk + if elem.Kind() == reflect.Interface { + elem = elem.Elem() + } + if sw, ok := w.(SliceWalker); ok { if err := sw.SliceElem(i, elem); err != nil { return err diff --git a/vendor/vendor.json b/vendor/vendor.json index 86a293b0d9..8bd359be97 100644 --- a/vendor/vendor.json +++ b/vendor/vendor.json @@ -1472,10 +1472,10 @@ "revision": "8631ce90f28644f54aeedcb3e389a85174e067d1" }, { - "checksumSHA1": "Vfkp+PcZ1wZ4+D6AsHTpKkdsQG0=", + "checksumSHA1": "EDAtec3XSbTjw6gWG+NNScows9M=", "path": "github.com/mitchellh/copystructure", - "revision": "501dcbdc7c358c4d0bfa066018834bedca79fde3", - "revisionTime": "2016-09-16T19:51:24Z" + "revision": "8f3c396a26dadccbd29ee24c76c89166249cc16f", + "revisionTime": "2016-09-27T21:34:29Z" }, { "path": "github.com/mitchellh/go-homedir", @@ -1507,8 +1507,10 @@ "revision": "6e6954073784f7ee67b28f2d22749d6479151ed7" }, { + "checksumSHA1": "Qfbnn/PnYNR2ZHraUeHTxT4H7lM=", "path": "github.com/mitchellh/reflectwalk", - "revision": "eecf4c70c626c7cfbb95c90195bc34d386c74ac6" + "revision": "2d53f44828cae4c770d745a3560e37d3356774a6", + "revisionTime": "2016-09-27T21:55:38Z" }, { "checksumSHA1": "/iig5lYSPCL3C8J7e4nTAevYNDE=", From 56901e5cfd962628582d66def9e163375faa7c1d Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 27 Sep 2016 16:09:32 -0700 Subject: [PATCH 2/5] terraform: ResourceConfig.DeepCopy This implements DeepCopy, still need to implement Equals to make this more useful. Coming in the next commit but this still has its own full functionality + tests. --- terraform/resource.go | 20 ++++++++++++++++++++ terraform/resource_test.go | 14 ++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/terraform/resource.go b/terraform/resource.go index 795bd9672c..6661fa3f04 100644 --- a/terraform/resource.go +++ b/terraform/resource.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/hashicorp/terraform/config" + "github.com/mitchellh/copystructure" ) // ResourceProvisionerConfig is used to pair a provisioner @@ -93,6 +94,25 @@ func NewResourceConfig(c *config.RawConfig) *ResourceConfig { return result } +// DeepCopy performs a deep copy of the configuration. This makes it safe +// to modify any of the structures that are part of the resource config without +// affecting the original configuration. +func (c *ResourceConfig) DeepCopy() *ResourceConfig { + // Copy, this will copy all the exported attributes + copy, err := copystructure.Config{Lock: true}.Copy(c) + if err != nil { + panic(err) + } + + // Force the type + result := copy.(*ResourceConfig) + + // For the raw configuration, we can just use its own copy method + result.raw = c.raw.Copy() + + return result +} + // CheckSet checks that the given list of configuration keys is // properly set. If not, errors are returned for each unset key. // diff --git a/terraform/resource_test.go b/terraform/resource_test.go index dba1636a29..3171a40a20 100644 --- a/terraform/resource_test.go +++ b/terraform/resource_test.go @@ -1,6 +1,7 @@ package terraform import ( + "fmt" "reflect" "testing" @@ -212,6 +213,19 @@ func TestResourceConfigGet(t *testing.T) { if !reflect.DeepEqual(v, tc.Value) { t.Fatalf("%d bad: %#v", i, v) } + + // If we have vars, we don't test copying + if len(tc.Vars) > 0 { + continue + } + + // Test copying + t.Run(fmt.Sprintf("copy-%d", i), func(t *testing.T) { + copy := rc.DeepCopy() + if !reflect.DeepEqual(copy, rc) { + t.Fatalf("bad:\n\n%#v\n\n%#v", copy, rc) + } + }) } } From f897fa470115f37fa4f93046023e1519b5acbc98 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 27 Sep 2016 18:52:32 -0700 Subject: [PATCH 3/5] terraform: ResourceConfig.Equal and tests --- terraform/resource.go | 20 ++++++++++++++++++++ terraform/resource_test.go | 22 ++++++++++++++++------ 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/terraform/resource.go b/terraform/resource.go index 6661fa3f04..ba4251b0f2 100644 --- a/terraform/resource.go +++ b/terraform/resource.go @@ -113,6 +113,26 @@ func (c *ResourceConfig) DeepCopy() *ResourceConfig { return result } +// Equal checks the equality of two resource configs. +func (c *ResourceConfig) Equal(c2 *ResourceConfig) bool { + // Two resource configs if their exported properties are equal. + // We don't compare "raw" because it is never used again after + // initialization and for all intents and purposes they are equal + // if the exported properties are equal. + check := [][2]interface{}{ + {c.ComputedKeys, c2.ComputedKeys}, + {c.Raw, c2.Raw}, + {c.Config, c2.Config}, + } + for _, pair := range check { + if !reflect.DeepEqual(pair[0], pair[1]) { + return false + } + } + + return true +} + // CheckSet checks that the given list of configuration keys is // properly set. If not, errors are returned for each unset key. // diff --git a/terraform/resource_test.go b/terraform/resource_test.go index 3171a40a20..b6a9577263 100644 --- a/terraform/resource_test.go +++ b/terraform/resource_test.go @@ -209,22 +209,32 @@ func TestResourceConfigGet(t *testing.T) { rc := NewResourceConfig(rawC) rc.interpolateForce() - v, _ := rc.Get(tc.Key) - if !reflect.DeepEqual(v, tc.Value) { - t.Fatalf("%d bad: %#v", i, v) - } + // Test getting a key + t.Run(fmt.Sprintf("get-%d", i), func(t *testing.T) { + v, _ := rc.Get(tc.Key) + if !reflect.DeepEqual(v, tc.Value) { + t.Fatalf("%d bad: %#v", i, v) + } + }) // If we have vars, we don't test copying if len(tc.Vars) > 0 { continue } - // Test copying - t.Run(fmt.Sprintf("copy-%d", i), func(t *testing.T) { + // Test copying and equality + t.Run(fmt.Sprintf("copy-and-equal-%d", i), func(t *testing.T) { copy := rc.DeepCopy() if !reflect.DeepEqual(copy, rc) { t.Fatalf("bad:\n\n%#v\n\n%#v", copy, rc) } + + if !copy.Equal(rc) { + t.Fatalf("copy != rc:\n\n%#v\n\n%#v", copy, rc) + } + if !rc.Equal(copy) { + t.Fatalf("rc != copy:\n\n%#v\n\n%#v", copy, rc) + } }) } } From 37c880c377adb51652d7b75f67005a8a8d00d37f Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 27 Sep 2016 19:16:29 -0700 Subject: [PATCH 4/5] Update reflectwalk to fix failing tests As part of working on ResourceConfig.DeepCopy, Equal I updated reflectwalk (to fix some issues in the new functions) but this introduced more issues in other parts of Terraform. This update fixes those. --- config/interpolate_walk_test.go | 18 ++++++------ .../mitchellh/reflectwalk/reflectwalk.go | 28 ++++++------------- vendor/vendor.json | 6 ++-- 3 files changed, 21 insertions(+), 31 deletions(-) diff --git a/config/interpolate_walk_test.go b/config/interpolate_walk_test.go index 70067a99cb..35b8f1f579 100644 --- a/config/interpolate_walk_test.go +++ b/config/interpolate_walk_test.go @@ -179,13 +179,15 @@ func TestInterpolationWalker_replace(t *testing.T) { return tc.Value, nil } - w := &interpolationWalker{F: fn, Replace: true} - if err := reflectwalk.Walk(tc.Input, w); err != nil { - t.Fatalf("err: %s", err) - } - - if !reflect.DeepEqual(tc.Input, tc.Output) { - t.Fatalf("%d: bad:\n\nexpected:%#v\ngot:%#v", i, tc.Output, tc.Input) - } + t.Run(fmt.Sprintf("walk-%d", i), func(t *testing.T) { + w := &interpolationWalker{F: fn, Replace: true} + if err := reflectwalk.Walk(tc.Input, w); err != nil { + t.Fatalf("err: %s", err) + } + + if !reflect.DeepEqual(tc.Input, tc.Output) { + t.Fatalf("%d: bad:\n\nexpected:%#v\ngot:%#v", i, tc.Output, tc.Input) + } + }) } } diff --git a/vendor/github.com/mitchellh/reflectwalk/reflectwalk.go b/vendor/github.com/mitchellh/reflectwalk/reflectwalk.go index fafd443a43..6066014c49 100644 --- a/vendor/github.com/mitchellh/reflectwalk/reflectwalk.go +++ b/vendor/github.com/mitchellh/reflectwalk/reflectwalk.go @@ -76,6 +76,14 @@ func Walk(data, walker interface{}) (err error) { } func walk(v reflect.Value, w interface{}) (err error) { + // We preserve the original value here because if it is an interface + // type, we want to pass that directly into the walkPrimitive, so that + // we can set it. + originalV := v + if v.Kind() == reflect.Interface { + v = v.Elem() + } + // Determine if we're receiving a pointer and if so notify the walker. pointer := false if v.Kind() == reflect.Ptr { @@ -96,14 +104,6 @@ func walk(v reflect.Value, w interface{}) (err error) { }() } - // We preserve the original value here because if it is an interface - // type, we want to pass that directly into the walkPrimitive, so that - // we can set it. - originalV := v - if v.Kind() == reflect.Interface { - v = v.Elem() - } - k := v.Kind() if k >= reflect.Int && k <= reflect.Complex128 { k = reflect.Int @@ -143,12 +143,6 @@ func walkMap(v reflect.Value, w interface{}) error { for _, k := range v.MapKeys() { kv := v.MapIndex(k) - // if the map value type is an interface, we need to extract the Elem - // for the next call to walk - if kv.Kind() == reflect.Interface { - kv = kv.Elem() - } - if mw, ok := w.(MapWalker); ok { if err := mw.MapElem(v, k, kv); err != nil { return err @@ -208,12 +202,6 @@ func walkSlice(v reflect.Value, w interface{}) (err error) { for i := 0; i < v.Len(); i++ { elem := v.Index(i) - // if the value type is an interface, we need to extract the Elem - // for the next call to walk - if elem.Kind() == reflect.Interface { - elem = elem.Elem() - } - if sw, ok := w.(SliceWalker); ok { if err := sw.SliceElem(i, elem); err != nil { return err diff --git a/vendor/vendor.json b/vendor/vendor.json index 8bd359be97..4b9d1902e2 100644 --- a/vendor/vendor.json +++ b/vendor/vendor.json @@ -1507,10 +1507,10 @@ "revision": "6e6954073784f7ee67b28f2d22749d6479151ed7" }, { - "checksumSHA1": "Qfbnn/PnYNR2ZHraUeHTxT4H7lM=", + "checksumSHA1": "5fFCVadmQVH6jqnB6Zd739s28QM=", "path": "github.com/mitchellh/reflectwalk", - "revision": "2d53f44828cae4c770d745a3560e37d3356774a6", - "revisionTime": "2016-09-27T21:55:38Z" + "revision": "84fc159ad78a797bb094a1e0c364392d837e1cd8", + "revisionTime": "2016-09-28T02:14:22Z" }, { "checksumSHA1": "/iig5lYSPCL3C8J7e4nTAevYNDE=", From ea342b793bfc85dcb8006e928ef93837c5e4b39e Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 27 Sep 2016 19:52:12 -0700 Subject: [PATCH 5/5] Update reflectwalk vendor to fix State.DeepCopy The real reasoning for this can be found in #9, #10, and #11. All these vendor updates aim to fix that issue, with minor adjustments --- terraform/state_test.go | 13 ++++--- .../mitchellh/reflectwalk/reflectwalk.go | 36 +++++++++++++------ vendor/vendor.json | 10 +++--- 3 files changed, 39 insertions(+), 20 deletions(-) diff --git a/terraform/state_test.go b/terraform/state_test.go index b5a23fafb8..569f5ef941 100644 --- a/terraform/state_test.go +++ b/terraform/state_test.go @@ -3,6 +3,7 @@ package terraform import ( "bytes" "encoding/json" + "fmt" "reflect" "strings" "testing" @@ -272,11 +273,13 @@ func TestStateDeepCopy(t *testing.T) { } for i, tc := range cases { - actual := tc.F(tc.One.DeepCopy()) - expected := tc.F(tc.Two) - if !reflect.DeepEqual(actual, expected) { - t.Fatalf("Bad: %d\n\n%s\n\n%s", i, actual, expected) - } + t.Run(fmt.Sprintf("copy-%d", i), func(t *testing.T) { + actual := tc.F(tc.One.DeepCopy()) + expected := tc.F(tc.Two) + if !reflect.DeepEqual(actual, expected) { + t.Fatalf("Bad: %d\n\n%s\n\n%s", i, actual, expected) + } + }) } } diff --git a/vendor/github.com/mitchellh/reflectwalk/reflectwalk.go b/vendor/github.com/mitchellh/reflectwalk/reflectwalk.go index 6066014c49..ecce023e1d 100644 --- a/vendor/github.com/mitchellh/reflectwalk/reflectwalk.go +++ b/vendor/github.com/mitchellh/reflectwalk/reflectwalk.go @@ -76,19 +76,27 @@ func Walk(data, walker interface{}) (err error) { } func walk(v reflect.Value, w interface{}) (err error) { - // We preserve the original value here because if it is an interface - // type, we want to pass that directly into the walkPrimitive, so that - // we can set it. - originalV := v - if v.Kind() == reflect.Interface { - v = v.Elem() - } - // Determine if we're receiving a pointer and if so notify the walker. + // The logic here is convoluted but very important (tests will fail if + // almost any part is changed). I will try to explain here. + // + // First, we check if the value is an interface, if so, we really need + // to check the interface's VALUE to see whether it is a pointer (pointers + // to interfaces are not allowed). + // + // Check whether the value is then an interface. If so, then set pointer + // to true to notify the user. + // + // At this time, we also set "v" to be the dereferenced value. This is + // because once we've unwrapped the pointer we want to use that value. pointer := false - if v.Kind() == reflect.Ptr { + pointerV := v + if pointerV.Kind() == reflect.Interface { + pointerV = pointerV.Elem() + } + if pointerV.Kind() == reflect.Ptr { pointer = true - v = reflect.Indirect(v) + v = reflect.Indirect(pointerV) } if pw, ok := w.(PointerWalker); ok { if err = pw.PointerEnter(pointer); err != nil { @@ -104,6 +112,14 @@ func walk(v reflect.Value, w interface{}) (err error) { }() } + // We preserve the original value here because if it is an interface + // type, we want to pass that directly into the walkPrimitive, so that + // we can set it. + originalV := v + if v.Kind() == reflect.Interface { + v = v.Elem() + } + k := v.Kind() if k >= reflect.Int && k <= reflect.Complex128 { k = reflect.Int diff --git a/vendor/vendor.json b/vendor/vendor.json index 4b9d1902e2..3e9621e11d 100644 --- a/vendor/vendor.json +++ b/vendor/vendor.json @@ -1474,8 +1474,8 @@ { "checksumSHA1": "EDAtec3XSbTjw6gWG+NNScows9M=", "path": "github.com/mitchellh/copystructure", - "revision": "8f3c396a26dadccbd29ee24c76c89166249cc16f", - "revisionTime": "2016-09-27T21:34:29Z" + "revision": "49a4444999946bce1882f9db0eb3ba0a44ed1fbb", + "revisionTime": "2016-09-28T02:49:35Z" }, { "path": "github.com/mitchellh/go-homedir", @@ -1507,10 +1507,10 @@ "revision": "6e6954073784f7ee67b28f2d22749d6479151ed7" }, { - "checksumSHA1": "5fFCVadmQVH6jqnB6Zd739s28QM=", + "checksumSHA1": "kXh6sdGViiRK0REpIWydJvpsyY0=", "path": "github.com/mitchellh/reflectwalk", - "revision": "84fc159ad78a797bb094a1e0c364392d837e1cd8", - "revisionTime": "2016-09-28T02:14:22Z" + "revision": "0c9480f65513be815a88d6076a3d8d95d4274236", + "revisionTime": "2016-09-28T02:49:03Z" }, { "checksumSHA1": "/iig5lYSPCL3C8J7e4nTAevYNDE=",