From 186a6dcc388aeefaa295b853aa57fbda08dd4fa5 Mon Sep 17 00:00:00 2001 From: Radek Simko Date: Mon, 5 Nov 2018 12:21:37 +0000 Subject: [PATCH 1/4] helper/schema: Add test for wrong timeout type --- helper/schema/provider_test.go | 49 ++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/helper/schema/provider_test.go b/helper/schema/provider_test.go index 6bf8c5bdf..39d16acbd 100644 --- a/helper/schema/provider_test.go +++ b/helper/schema/provider_test.go @@ -3,6 +3,7 @@ package schema import ( "fmt" "reflect" + "strings" "testing" "time" @@ -277,6 +278,54 @@ func TestProviderValidate(t *testing.T) { } } +func TestProviderDiff_timeoutInvalidType(t *testing.T) { + p := &Provider{ + ResourcesMap: map[string]*Resource{ + "blah": &Resource{ + Schema: map[string]*Schema{ + "foo": { + Type: TypeInt, + Optional: true, + }, + }, + Timeouts: &ResourceTimeout{ + Create: DefaultTimeout(10 * time.Minute), + }, + }, + }, + } + + invalidCfg := map[string]interface{}{ + "foo": 42, + "timeouts": []map[string]interface{}{ + map[string]interface{}{ + "create": "40m", + }, + }, + } + ic, err := config.NewRawConfig(invalidCfg) + if err != nil { + t.Fatalf("err: %s", err) + } + + _, err = p.Diff( + &terraform.InstanceInfo{ + Type: "blah", + }, + nil, + terraform.NewResourceConfig(ic), + ) + if err == nil { + t.Fatal("Expected provider.Diff to fail with invalid timeout type") + } + expectedErrMsg := "Invalid Timeout structure" + if !strings.Contains(err.Error(), expectedErrMsg) { + t.Fatalf("Unexpected error message: %q\nExpected message to contain %q", + err.Error(), + expectedErrMsg) + } +} + func TestProviderValidateResource(t *testing.T) { cases := []struct { P *Provider From 2fe3f16cb325f7cab6f629743e1ad73ff95f81b5 Mon Sep 17 00:00:00 2001 From: Radek Simko Date: Mon, 5 Nov 2018 12:28:56 +0000 Subject: [PATCH 2/4] helper/schema: Return error on invalid timeout type --- helper/schema/resource_timeout.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/helper/schema/resource_timeout.go b/helper/schema/resource_timeout.go index 445819f0f..f558b177c 100644 --- a/helper/schema/resource_timeout.go +++ b/helper/schema/resource_timeout.go @@ -110,7 +110,8 @@ func (t *ResourceTimeout) ConfigDecode(s *Resource, c *terraform.ResourceConfig) } } } else { - log.Printf("[WARN] Invalid Timeout structure found, skipping timeouts") + log.Printf("[ERROR] Invalid timeout structure: %T", raw) + return fmt.Errorf("Invalid Timeout structure found") } } From 82a77f9bb504221f263a0c81b197b9342bf80b74 Mon Sep 17 00:00:00 2001 From: Radek Simko Date: Mon, 5 Nov 2018 12:16:11 +0000 Subject: [PATCH 3/4] helper/schema: Add test for invalid timeout value --- helper/schema/provider_test.go | 46 ++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/helper/schema/provider_test.go b/helper/schema/provider_test.go index 39d16acbd..bebe1f19b 100644 --- a/helper/schema/provider_test.go +++ b/helper/schema/provider_test.go @@ -326,6 +326,52 @@ func TestProviderDiff_timeoutInvalidType(t *testing.T) { } } +func TestProviderDiff_timeoutInvalidValue(t *testing.T) { + p := &Provider{ + ResourcesMap: map[string]*Resource{ + "blah": &Resource{ + Schema: map[string]*Schema{ + "foo": { + Type: TypeInt, + Optional: true, + }, + }, + Timeouts: &ResourceTimeout{ + Create: DefaultTimeout(10 * time.Minute), + }, + }, + }, + } + + invalidCfg := map[string]interface{}{ + "foo": 42, + "timeouts": map[string]interface{}{ + "create": "invalid", + }, + } + ic, err := config.NewRawConfig(invalidCfg) + if err != nil { + t.Fatalf("err: %s", err) + } + + _, err = p.Diff( + &terraform.InstanceInfo{ + Type: "blah", + }, + nil, + terraform.NewResourceConfig(ic), + ) + if err == nil { + t.Fatal("Expected provider.Diff to fail with invalid timeout value") + } + expectedErrMsg := "time: invalid duration invalid" + if !strings.Contains(err.Error(), expectedErrMsg) { + t.Fatalf("Unexpected error message: %q\nExpected message to contain %q", + err.Error(), + expectedErrMsg) + } +} + func TestProviderValidateResource(t *testing.T) { cases := []struct { P *Provider From 1cb8f1df80a493f4aae09f37b491c15ce3da5cf3 Mon Sep 17 00:00:00 2001 From: Radek Simko Date: Mon, 5 Nov 2018 10:51:22 +0000 Subject: [PATCH 4/4] helper/schema: Fix timeout parsing in ResourceTimeout.ConfigDecode --- helper/schema/resource_test.go | 9 ++- helper/schema/resource_timeout.go | 84 ++++++++++++-------------- helper/schema/resource_timeout_test.go | 37 +++++------- helper/schema/shims_test.go | 7 +-- 4 files changed, 63 insertions(+), 74 deletions(-) diff --git a/helper/schema/resource_test.go b/helper/schema/resource_test.go index 0b366d4d6..65ebf0c01 100644 --- a/helper/schema/resource_test.go +++ b/helper/schema/resource_test.go @@ -220,10 +220,9 @@ func TestResourceDiff_Timeout_diff(t *testing.T) { raw, err := config.NewRawConfig( map[string]interface{}{ "foo": 42, - "timeouts": []map[string]interface{}{ - map[string]interface{}{ - "create": "2h", - }}, + TimeoutsConfigKey: map[string]interface{}{ + "create": "2h", + }, }) if err != nil { t.Fatalf("err: %s", err) @@ -256,7 +255,7 @@ func TestResourceDiff_Timeout_diff(t *testing.T) { } if !reflect.DeepEqual(actual, expected) { - t.Fatalf("Not equal in Timeout Diff:\n\texpected: %#v\n\tactual: %#v", expected.Meta, actual.Meta) + t.Fatalf("Not equal Meta in Timeout Diff:\n\texpected: %#v\n\tactual: %#v", expected.Meta, actual.Meta) } } diff --git a/helper/schema/resource_timeout.go b/helper/schema/resource_timeout.go index f558b177c..33cbce185 100644 --- a/helper/schema/resource_timeout.go +++ b/helper/schema/resource_timeout.go @@ -62,52 +62,48 @@ func (t *ResourceTimeout) ConfigDecode(s *Resource, c *terraform.ResourceConfig) } if raw, ok := c.Config[TimeoutsConfigKey]; ok { - if configTimeouts, ok := raw.([]map[string]interface{}); ok { - for _, timeoutValues := range configTimeouts { - // loop through each Timeout given in the configuration and validate they - // the Timeout defined in the resource - for timeKey, timeValue := range timeoutValues { - // validate that we're dealing with the normal CRUD actions - var found bool - for _, key := range timeoutKeys() { - if timeKey == key { - found = true - break - } + if timeoutValues, ok := raw.(map[string]interface{}); ok { + for timeKey, timeValue := range timeoutValues { + // validate that we're dealing with the normal CRUD actions + var found bool + for _, key := range timeoutKeys() { + if timeKey == key { + found = true + break } - - if !found { - return fmt.Errorf("Unsupported Timeout configuration key found (%s)", timeKey) - } - - // Get timeout - rt, err := time.ParseDuration(timeValue.(string)) - if err != nil { - return fmt.Errorf("Error parsing Timeout for (%s): %s", timeKey, err) - } - - var timeout *time.Duration - switch timeKey { - case TimeoutCreate: - timeout = t.Create - case TimeoutUpdate: - timeout = t.Update - case TimeoutRead: - timeout = t.Read - case TimeoutDelete: - timeout = t.Delete - case TimeoutDefault: - timeout = t.Default - } - - // If the resource has not delcared this in the definition, then error - // with an unsupported message - if timeout == nil { - return unsupportedTimeoutKeyError(timeKey) - } - - *timeout = rt } + + if !found { + return fmt.Errorf("Unsupported Timeout configuration key found (%s)", timeKey) + } + + // Get timeout + rt, err := time.ParseDuration(timeValue.(string)) + if err != nil { + return fmt.Errorf("Error parsing %q timeout: %s", timeKey, err) + } + + var timeout *time.Duration + switch timeKey { + case TimeoutCreate: + timeout = t.Create + case TimeoutUpdate: + timeout = t.Update + case TimeoutRead: + timeout = t.Read + case TimeoutDelete: + timeout = t.Delete + case TimeoutDefault: + timeout = t.Default + } + + // If the resource has not delcared this in the definition, then error + // with an unsupported message + if timeout == nil { + return unsupportedTimeoutKeyError(timeKey) + } + + *timeout = rt } } else { log.Printf("[ERROR] Invalid timeout structure: %T", raw) diff --git a/helper/schema/resource_timeout_test.go b/helper/schema/resource_timeout_test.go index bef98071b..f48ba883b 100644 --- a/helper/schema/resource_timeout_test.go +++ b/helper/schema/resource_timeout_test.go @@ -16,7 +16,7 @@ func TestResourceTimeout_ConfigDecode_badkey(t *testing.T) { // what the resource has defined in source ResourceDefaultTimeout *ResourceTimeout // configuration provider by user in tf file - Config []map[string]interface{} + Config map[string]interface{} // what we expect the parsed ResourceTimeout to be Expected *ResourceTimeout // Should we have an error (key not defined in source) @@ -46,10 +46,9 @@ func TestResourceTimeout_ConfigDecode_badkey(t *testing.T) { { Name: "Use something besides 'minutes'", ResourceDefaultTimeout: timeoutForValues(10, 0, 5, 0, 3), - Config: []map[string]interface{}{ - map[string]interface{}{ - "create": "2h", - }}, + Config: map[string]interface{}{ + "create": "2h", + }, Expected: timeoutForValues(120, 0, 5, 0, 3), ShouldErr: false, }, @@ -87,7 +86,7 @@ func TestResourceTimeout_ConfigDecode_badkey(t *testing.T) { } if !reflect.DeepEqual(c.Expected, timeout) { - t.Fatalf("ConfigDecode match error case (%d), expected:\n%#v\ngot:\n%#v", i, c.Expected, timeout) + t.Fatalf("ConfigDecode match error case (%d).\nExpected:\n%#v\nGot:\n%#v", i, c.Expected, timeout) } }) } @@ -104,13 +103,9 @@ func TestResourceTimeout_ConfigDecode(t *testing.T) { raw, err := config.NewRawConfig( map[string]interface{}{ "foo": "bar", - TimeoutsConfigKey: []map[string]interface{}{ - map[string]interface{}{ - "create": "2m", - }, - map[string]interface{}{ - "update": "1m", - }, + TimeoutsConfigKey: map[string]interface{}{ + "create": "2m", + "update": "1m", }, }) if err != nil { @@ -130,7 +125,7 @@ func TestResourceTimeout_ConfigDecode(t *testing.T) { } if !reflect.DeepEqual(timeout, expected) { - t.Fatalf("bad timeout decode, expected (%#v), got (%#v)", expected, timeout) + t.Fatalf("bad timeout decode.\nExpected:\n%#v\nGot:\n%#v\n", expected, timeout) } } @@ -329,24 +324,24 @@ func expectedForValues(create, read, update, del, def int) map[string]interface{ return ex } -func expectedConfigForValues(create, read, update, delete, def int) []map[string]interface{} { - ex := make([]map[string]interface{}, 0) +func expectedConfigForValues(create, read, update, delete, def int) map[string]interface{} { + ex := make(map[string]interface{}, 0) if create != 0 { - ex = append(ex, map[string]interface{}{"create": fmt.Sprintf("%dm", create)}) + ex["create"] = fmt.Sprintf("%dm", create) } if read != 0 { - ex = append(ex, map[string]interface{}{"read": fmt.Sprintf("%dm", read)}) + ex["read"] = fmt.Sprintf("%dm", read) } if update != 0 { - ex = append(ex, map[string]interface{}{"update": fmt.Sprintf("%dm", update)}) + ex["update"] = fmt.Sprintf("%dm", update) } if delete != 0 { - ex = append(ex, map[string]interface{}{"delete": fmt.Sprintf("%dm", delete)}) + ex["delete"] = fmt.Sprintf("%dm", delete) } if def != 0 { - ex = append(ex, map[string]interface{}{"default": fmt.Sprintf("%dm", def)}) + ex["default"] = fmt.Sprintf("%dm", def) } return ex } diff --git a/helper/schema/shims_test.go b/helper/schema/shims_test.go index e08126893..68186497b 100644 --- a/helper/schema/shims_test.go +++ b/helper/schema/shims_test.go @@ -286,10 +286,9 @@ func TestShimResourceDiff_Timeout_diff(t *testing.T) { raw, err := config.NewRawConfig( map[string]interface{}{ "foo": 42, - "timeouts": []map[string]interface{}{ - map[string]interface{}{ - "create": "2h", - }}, + TimeoutsConfigKey: map[string]interface{}{ + "create": "2h", + }, }) if err != nil { t.Fatalf("err: %s", err)