Merge pull request #19286 from hashicorp/radeksimko/b-timeouts-parsing-fix

helper/schema: Fix timeout parsing during Provider.Diff

helper/schema/provider_test.go

@@ -3,6 +3,7 @@ package schema
 import (
 	"fmt"
 	"reflect"
+	"strings"
 	"testing"
 	"time"
 
@@ -277,6 +278,100 @@ 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 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

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{}{
+			TimeoutsConfigKey: map[string]interface{}{
-				map[string]interface{}{
+				"create": "2h",
-					"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)
 	}
 }
 

helper/schema/resource_timeout.go

@@ -62,55 +62,52 @@ func (t *ResourceTimeout) ConfigDecode(s *Resource, c *terraform.ResourceConfig)
 	}
 
 	if raw, ok := c.Config[TimeoutsConfigKey]; ok {
-		if configTimeouts, ok := raw.([]map[string]interface{}); ok {
+		if timeoutValues, ok := raw.(map[string]interface{}); ok {
-			for _, timeoutValues := range configTimeouts {
+			for timeKey, timeValue := range timeoutValues {
-				// loop through each Timeout given in the configuration and validate they
+				// validate that we're dealing with the normal CRUD actions
-				// the Timeout defined in the resource
+				var found bool
-				for timeKey, timeValue := range timeoutValues {
+				for _, key := range timeoutKeys() {
-					// validate that we're dealing with the normal CRUD actions
+					if timeKey == key {
-					var found bool
+						found = true
-					for _, key := range timeoutKeys() {
+						break
-						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("[WARN] Invalid Timeout structure found, skipping timeouts")
+			log.Printf("[ERROR] Invalid timeout structure: %T", raw)
+			return fmt.Errorf("Invalid Timeout structure found")
 		}
 	}
 

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{}{
+			Config: map[string]interface{}{
-				map[string]interface{}{
+				"create": "2h",
-					"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{}{
+			TimeoutsConfigKey: map[string]interface{}{
-				map[string]interface{}{
+				"create": "2m",
-					"create": "2m",
+				"update": "1m",
-				},
-				map[string]interface{}{
-					"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{} {
+func expectedConfigForValues(create, read, update, delete, def int) map[string]interface{} {
-	ex := make([]map[string]interface{}, 0)
+	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
 }

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{}{
+			TimeoutsConfigKey: map[string]interface{}{
-				map[string]interface{}{
+				"create": "2h",
-					"create": "2h",
+			},
-				}},
 		})
 	if err != nil {
 		t.Fatalf("err: %s", err)