Merge pull request #6 from hashicorp/vancluever-add-unknown-attr-checks-fix-tests

plugin: add unknown attr checks, fix E2E test

plugin/attributes.go

@@ -0,0 +1,189 @@
+package plugin
+
+import (
+	"fmt"
+	"sort"
+	"strings"
+	"time"
+
+	"github.com/mitchellh/mapstructure"
+	"google.golang.org/protobuf/types/known/structpb"
+)
+
+// CatalogAttributes is a Go-native representation of the Attributes
+// map.
+type CatalogAttributes struct {
+	Region                    string
+	DisableCredentialRotation bool
+}
+
+func getCatalogAttributes(in *structpb.Struct) (*CatalogAttributes, error) {
+	unknownFields := structFields(in)
+	result := new(CatalogAttributes)
+
+	var err error
+	result.Region, err = getStringValue(in, constRegion, true)
+	if err != nil {
+		return nil, err
+	}
+	delete(unknownFields, constRegion)
+
+	result.DisableCredentialRotation, err = getBoolValue(in, constDisableCredentialRotation, false)
+	if err != nil {
+		return nil, err
+	}
+	delete(unknownFields, constDisableCredentialRotation)
+
+	if len(unknownFields) != 0 {
+		return nil, fmt.Errorf("unknown catalog attribute fields provided: %s", keysAsString(unknownFields))
+	}
+
+	return result, nil
+}
+
+// CatalogSecrets is a Go-native representation of the Secrets map.
+type CatalogSecrets struct {
+	AccessKeyId     string
+	SecretAccessKey string
+}
+
+func getCatalogSecrets(in *structpb.Struct) (*CatalogSecrets, error) {
+	unknownFields := structFields(in)
+	result := new(CatalogSecrets)
+
+	var err error
+	result.AccessKeyId, err = getStringValue(in, constAccessKeyId, true)
+	if err != nil {
+		return nil, err
+	}
+	delete(unknownFields, constAccessKeyId)
+
+	result.SecretAccessKey, err = getStringValue(in, constSecretAccessKey, true)
+	if err != nil {
+		return nil, err
+	}
+	delete(unknownFields, constSecretAccessKey)
+
+	if len(unknownFields) != 0 {
+		return nil, fmt.Errorf("unknown catalog secret fields provided: %s", keysAsString(unknownFields))
+	}
+
+	return result, nil
+}
+
+// SetAttributes is a Go-native representation of the Attributes map that can be
+// used for decoding the incoming map via mapstructure.
+type SetAttributes struct {
+	Filters []string
+}
+
+func getSetAttributes(in *structpb.Struct) (*SetAttributes, error) {
+	var setAttrs SetAttributes
+
+	// Quick validation to ensure that there's no non-filter attributes
+	// here for now. Make this more complex if we add more attributes
+	// to host sets.
+	unknownFields := structFields(in)
+	delete(unknownFields, constDescribeInstancesFilters)
+	if len(unknownFields) != 0 {
+		return nil, fmt.Errorf("unknown set attribute fields provided: %s", keysAsString(unknownFields))
+	}
+
+	// Mapstructure complains if it expects a slice as output and sees a scalar
+	// value. Rather than use WeakDecode and risk unintended consequences, I'm
+	// manually making this change if necessary.
+	inMap := in.AsMap()
+	if filtersRaw, ok := inMap[constDescribeInstancesFilters]; ok {
+		switch filterVal := filtersRaw.(type) {
+		case string:
+			inMap[constDescribeInstancesFilters] = []string{filterVal}
+		}
+	}
+
+	if err := mapstructure.Decode(inMap, &setAttrs); err != nil {
+		return nil, fmt.Errorf("error decoding set attributes: %w", err)
+	}
+
+	return &setAttrs, nil
+}
+
+func getStringValue(in *structpb.Struct, k string, required bool) (string, error) {
+	mv := in.GetFields()
+	v, ok := mv[k]
+	if !ok {
+		if required {
+			return "", fmt.Errorf("missing required value %q", k)
+		}
+
+		return "", nil
+	}
+
+	s, ok := v.AsInterface().(string)
+	if !ok {
+		return "", fmt.Errorf("unexpected type for value %q: want string, got %T", k, v.AsInterface())
+	}
+
+	if s == "" && required {
+		return "", fmt.Errorf("value %q cannot be empty", k)
+	}
+
+	return s, nil
+}
+
+func getBoolValue(in *structpb.Struct, k string, required bool) (bool, error) {
+	mv := in.GetFields()
+	v, ok := mv[k]
+	if !ok {
+		if required {
+			return false, fmt.Errorf("missing required value %q", k)
+		}
+
+		return false, nil
+	}
+
+	b, ok := v.AsInterface().(bool)
+	if !ok {
+		return false, fmt.Errorf("unexpected type for value %q: want bool, got %T", k, v.AsInterface())
+	}
+
+	return b, nil
+}
+
+func getTimeValue(in *structpb.Struct, k string) (time.Time, error) {
+	mv := in.GetFields()
+	v, ok := mv[k]
+	if !ok {
+		return time.Time{}, nil
+	}
+
+	tRaw, ok := v.AsInterface().(string)
+	if !ok {
+		return time.Time{}, fmt.Errorf("unexpected type for value %q: want string, got %T", k, v.AsInterface())
+	}
+
+	t, err := time.Parse(time.RFC3339Nano, tRaw)
+	if err != nil {
+		return time.Time{}, fmt.Errorf("could not parse time in value %q: %w", k, err)
+	}
+
+	return t, nil
+}
+
+func structFields(s *structpb.Struct) map[string]struct{} {
+	m := make(map[string]struct{}, len(s.GetFields()))
+	for k := range s.GetFields() {
+		m[k] = struct{}{}
+	}
+
+	return m
+}
+
+func keysAsString(m map[string]struct{}) string {
+	s := make([]string, 0, len(m))
+	for k := range m {
+		s = append(s, k)
+	}
+
+	sort.Strings(s)
+	return strings.Join(s, ", ")
+}

plugin/attributes_test.go

@@ -0,0 +1,423 @@
+package plugin
+
+import (
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+	"google.golang.org/protobuf/types/known/structpb"
+)
+
+func TestGetCatalogAttributes(t *testing.T) {
+	cases := []struct {
+		name                string
+		in                  *structpb.Struct
+		expected            *CatalogAttributes
+		expectedErrContains string
+	}{
+		{
+			name:                "missing region",
+			in:                  mustStruct(map[string]interface{}{}),
+			expectedErrContains: "missing required value \"region\"",
+		},
+		{
+			name: "bad value for disable_credential_rotation",
+			in: mustStruct(map[string]interface{}{
+				constRegion:                    "us-west-2",
+				constDisableCredentialRotation: "sure",
+			}),
+			expectedErrContains: "unexpected type for value \"disable_credential_rotation\": want bool, got string",
+		},
+		{
+			name: "unknown fields",
+			in: mustStruct(map[string]interface{}{
+				constRegion: "us-west-2",
+				"foo":       true,
+				"bar":       true,
+			}),
+			expectedErrContains: "unknown catalog attribute fields provided: bar, foo",
+		},
+		{
+			name: "default",
+			in: mustStruct(map[string]interface{}{
+				constRegion: "us-west-2",
+			}),
+			expected: &CatalogAttributes{
+				Region:                    "us-west-2",
+				DisableCredentialRotation: false,
+			},
+		},
+		{
+			name: "with disable_credential_rotation",
+			in: mustStruct(map[string]interface{}{
+				constRegion:                    "us-west-2",
+				constDisableCredentialRotation: true,
+			}),
+			expected: &CatalogAttributes{
+				Region:                    "us-west-2",
+				DisableCredentialRotation: true,
+			},
+		},
+	}
+
+	for _, tc := range cases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			require := require.New(t)
+			actual, err := getCatalogAttributes(tc.in)
+			if tc.expectedErrContains != "" {
+				require.Error(err)
+				require.Contains(err.Error(), tc.expectedErrContains)
+				return
+			}
+
+			require.NoError(err)
+			require.Equal(tc.expected, actual)
+		})
+	}
+}
+
+func TestGetCatalogSecrets(t *testing.T) {
+	cases := []struct {
+		name                string
+		in                  *structpb.Struct
+		expected            *CatalogSecrets
+		expectedErrContains string
+	}{
+		{
+			name:                "missing access_key_id",
+			in:                  mustStruct(map[string]interface{}{}),
+			expectedErrContains: "missing required value \"access_key_id\"",
+		},
+		{
+			name: "missing secret_access_key",
+			in: mustStruct(map[string]interface{}{
+				constAccessKeyId: "foobar",
+			}),
+			expectedErrContains: "missing required value \"secret_access_key\"",
+		},
+		{
+			name: "unknown fields",
+			in: mustStruct(map[string]interface{}{
+				constAccessKeyId:     "foobar",
+				constSecretAccessKey: "bazqux",
+				"foo":                true,
+				"bar":                true,
+			}),
+			expectedErrContains: "unknown catalog secret fields provided: bar, foo",
+		},
+		{
+			name: "good",
+			in: mustStruct(map[string]interface{}{
+				constAccessKeyId:     "foobar",
+				constSecretAccessKey: "bazqux",
+			}),
+			expected: &CatalogSecrets{
+				AccessKeyId:     "foobar",
+				SecretAccessKey: "bazqux",
+			},
+		},
+	}
+
+	for _, tc := range cases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			require := require.New(t)
+			actual, err := getCatalogSecrets(tc.in)
+			if tc.expectedErrContains != "" {
+				require.Error(err)
+				require.Contains(err.Error(), tc.expectedErrContains)
+				return
+			}
+
+			require.NoError(err)
+			require.Equal(tc.expected, actual)
+		})
+	}
+}
+
+func TestGetSetAttributes(t *testing.T) {
+	cases := []struct {
+		name                string
+		in                  *structpb.Struct
+		expected            *SetAttributes
+		expectedErrContains string
+	}{
+		{
+			name:     "missing",
+			in:       mustStruct(map[string]interface{}{}),
+			expected: &SetAttributes{},
+		},
+		{
+			name: "non-string-slice value",
+			in: mustStruct(map[string]interface{}{
+				constDescribeInstancesFilters: "zip=foo,bar",
+			}),
+			expected: &SetAttributes{
+				Filters: []string{"zip=foo,bar"},
+			},
+		},
+		{
+			name: "bad filter element value",
+			in: mustStruct(map[string]interface{}{
+				constDescribeInstancesFilters: []interface{}{1},
+			}),
+			expectedErrContains: "expected type 'string', got unconvertible type 'float64'",
+		},
+		{
+			name: "unknown fields",
+			in: mustStruct(map[string]interface{}{
+				"foo": true,
+				"bar": true,
+			}),
+			expectedErrContains: "unknown set attribute fields provided: bar, foo",
+		},
+		{
+			name: "good",
+			in: mustStruct(map[string]interface{}{
+				constDescribeInstancesFilters: []interface{}{
+					"foo=bar",
+					"zip=zap",
+				},
+			}),
+			expected: &SetAttributes{
+				Filters: []string{"foo=bar", "zip=zap"},
+			},
+		},
+	}
+
+	for _, tc := range cases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			require := require.New(t)
+			actual, err := getSetAttributes(tc.in)
+			if tc.expectedErrContains != "" {
+				require.Error(err)
+				require.Contains(err.Error(), tc.expectedErrContains)
+				return
+			}
+
+			require.NoError(err)
+			require.Equal(tc.expected, actual)
+		})
+	}
+}
+
+func TestGetStringValue(t *testing.T) {
+	cases := []struct {
+		name        string
+		in          *structpb.Struct
+		key         string
+		required    bool
+		expected    string
+		expectedErr string
+	}{
+		{
+			name:        "required missing",
+			in:          mustStruct(map[string]interface{}{}),
+			key:         "foo",
+			required:    true,
+			expectedErr: "missing required value \"foo\"",
+		},
+		{
+			name:     "optional missing",
+			in:       mustStruct(map[string]interface{}{}),
+			key:      "foo",
+			expected: "",
+		},
+		{
+			name: "non-string value",
+			in: mustStruct(map[string]interface{}{
+				"foo": 1,
+			}),
+			key:         "foo",
+			expectedErr: "unexpected type for value \"foo\": want string, got float64",
+		},
+		{
+			name: "required empty",
+			in: mustStruct(map[string]interface{}{
+				"foo": "",
+			}),
+			key:         "foo",
+			required:    true,
+			expectedErr: "value \"foo\" cannot be empty",
+		},
+		{
+			name: "good",
+			in: mustStruct(map[string]interface{}{
+				"foo": "bar",
+			}),
+			key:      "foo",
+			expected: "bar",
+		},
+	}
+
+	for _, tc := range cases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			require := require.New(t)
+			actual, err := getStringValue(tc.in, tc.key, tc.required)
+			if tc.expectedErr != "" {
+				require.EqualError(err, tc.expectedErr)
+				return
+			}
+
+			require.NoError(err)
+			require.Equal(tc.expected, actual)
+		})
+	}
+}
+
+func TestGetBoolValue(t *testing.T) {
+	cases := []struct {
+		name        string
+		in          *structpb.Struct
+		key         string
+		required    bool
+		expected    bool
+		expectedErr string
+	}{
+		{
+			name:        "required missing",
+			in:          mustStruct(map[string]interface{}{}),
+			key:         "foo",
+			required:    true,
+			expectedErr: "missing required value \"foo\"",
+		},
+		{
+			name:     "optional missing",
+			in:       mustStruct(map[string]interface{}{}),
+			key:      "foo",
+			expected: false,
+		},
+		{
+			name: "non-bool value",
+			in: mustStruct(map[string]interface{}{
+				"foo": "bar",
+			}),
+			key:         "foo",
+			expectedErr: "unexpected type for value \"foo\": want bool, got string",
+		},
+		{
+			name: "good",
+			in: mustStruct(map[string]interface{}{
+				"foo": true,
+			}),
+			key:      "foo",
+			expected: true,
+		},
+	}
+
+	for _, tc := range cases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			require := require.New(t)
+			actual, err := getBoolValue(tc.in, tc.key, tc.required)
+			if tc.expectedErr != "" {
+				require.EqualError(err, tc.expectedErr)
+				return
+			}
+
+			require.NoError(err)
+			require.Equal(tc.expected, actual)
+		})
+	}
+}
+
+func TestGetTimeValue(t *testing.T) {
+	staticTime := time.Now()
+
+	cases := []struct {
+		name        string
+		in          *structpb.Struct
+		key         string
+		expected    time.Time
+		expectedErr string
+	}{
+		{
+			name:     "missing",
+			in:       mustStruct(map[string]interface{}{}),
+			key:      "foo",
+			expected: time.Time{},
+		},
+		{
+			name: "non-time value",
+			in: mustStruct(map[string]interface{}{
+				"foo": 1,
+			}),
+			key:         "foo",
+			expectedErr: "unexpected type for value \"foo\": want string, got float64",
+		},
+		{
+			name: "bad parse",
+			in: mustStruct(map[string]interface{}{
+				"foo": "bar",
+			}),
+			key:         "foo",
+			expectedErr: "could not parse time in value \"foo\": parsing time \"bar\" as \"2006-01-02T15:04:05.999999999Z07:00\": cannot parse \"bar\" as \"2006\"",
+		},
+		{
+			name: "good",
+			in: mustStruct(map[string]interface{}{
+				"foo": staticTime.Format(time.RFC3339Nano),
+			}),
+			key: "foo",
+			expected: func() time.Time {
+				u, err := time.Parse(time.RFC3339Nano, staticTime.Format(time.RFC3339Nano))
+				if err != nil {
+					panic(err)
+				}
+
+				return u
+			}(),
+		},
+	}
+
+	for _, tc := range cases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			require := require.New(t)
+			actual, err := getTimeValue(tc.in, tc.key)
+			if tc.expectedErr != "" {
+				require.EqualError(err, tc.expectedErr)
+				return
+			}
+
+			require.NoError(err)
+			require.Equal(tc.expected, actual)
+		})
+	}
+}
+
+func TestValidateRegion(t *testing.T) {
+	cases := []struct {
+		name        string
+		in          string
+		expectedErr string
+	}{
+		{
+			name:        "invalid region",
+			in:          "foobar",
+			expectedErr: "not a valid region: foobar",
+		},
+		{
+			name:        "good",
+			in:          "us-west-2",
+			expectedErr: "",
+		},
+	}
+
+	for _, tc := range cases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			require := require.New(t)
+			err := validateRegion(tc.in)
+			if tc.expectedErr != "" {
+				require.EqualError(err, tc.expectedErr)
+				return
+			}
+
+			require.NoError(err)
+		})
+	}
+}

plugin/e2e_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"strings"
 	"testing"
 	"time"
 
@@ -453,16 +454,7 @@ func testPluginOnCreateUpdateSet(ctx context.Context, t *testing.T, p *AwsPlugin
 	})
 	require.NoError(err)
 	setAttrs, err := structpb.NewStruct(map[string]interface{}{
-		constDescribeInstancesFilters: map[string]interface{}{
-			"tag-key": func() []interface{} {
-				result := make([]interface{}, len(tags))
-				for i, tag := range tags {
-					result[i] = tag
-				}
-
-				return result
-			}(),
-		},
+		constDescribeInstancesFilters: []interface{}{fmt.Sprintf("tag-key=%s", strings.Join(tags, ","))},
 	})
 	require.NoError(err)
 	reqPersistedSecrets, err := structpb.NewStruct(map[string]interface{}{
@@ -520,9 +512,7 @@ func testPluginListHosts(ctx context.Context, t *testing.T, p *AwsPlugin, region
 	sets := make([]*hostsets.HostSet, len(tags))
 	for i, tag := range tags {
 		setAttrs, err := structpb.NewStruct(map[string]interface{}{
-			constDescribeInstancesFilters: map[string]interface{}{
-				"tag-key": []interface{}{tag},
-			},
+			constDescribeInstancesFilters: []interface{}{fmt.Sprintf("tag-key=%s", tag)},
 		})
 		require.NoError(err)
 		sets[i] = &hostsets.HostSet{

plugin/plugin.go

@@ -12,8 +12,6 @@ import (
 	"github.com/aws/aws-sdk-go/aws/endpoints"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	pb "github.com/hashicorp/boundary/sdk/pbs/plugin"
-	"github.com/mitchellh/mapstructure"
-	"google.golang.org/protobuf/types/known/structpb"
 )
 
 // rotationWaitTimeout controls the time we wait for credential rotation to
@@ -35,12 +33,6 @@ var (
 	_ pb.HostPluginServiceServer = (*AwsPlugin)(nil)
 )
 
-// SetAttributes is a Go-native representation of the Attributes map that can be
-// used for decoding the incoming map via mapstructure.
-type SetAttributes struct {
-	Filters []string
-}
-
 func (p *AwsPlugin) OnCreateCatalog(ctx context.Context, req *pb.OnCreateCatalogRequest) (*pb.OnCreateCatalogResponse, error) {
 	catalog := req.GetCatalog()
 	if catalog == nil {
@@ -57,36 +49,30 @@ func (p *AwsPlugin) OnCreateCatalog(ctx context.Context, req *pb.OnCreateCatalog
 		return nil, errors.New("attributes are required")
 	}
 
-	if _, err := getValidateRegionValue(attrs); err != nil {
+	catalogAttributes, err := getCatalogAttributes(attrs)
+	if err != nil {
 		return nil, err
 	}
-
-	accessKeyId, err := getStringValue(secrets, constAccessKeyId, true)
+	catalogSecrets, err := getCatalogSecrets(secrets)
 	if err != nil {
 		return nil, err
 	}
 
-	secretAccessKey, err := getStringValue(secrets, constSecretAccessKey, true)
-	if err != nil {
-		return nil, err
-	}
-
-	skipRotate, err := getBoolValue(attrs, constDisableCredentialRotation, false)
-	if err != nil {
+	if err := validateRegion(catalogAttributes.Region); err != nil {
 		return nil, err
 	}
 
 	// Set up the creds in our persisted state.
 	state, err := newAwsCatalogPersistedState(append([]awsCatalogPersistedStateOption{
-		withAccessKeyId(accessKeyId),
-		withSecretAccessKey(secretAccessKey),
+		withAccessKeyId(catalogSecrets.AccessKeyId),
+		withSecretAccessKey(catalogSecrets.SecretAccessKey),
 	}, p.testStateOpts...)...)
 	if err != nil {
 		return nil, fmt.Errorf("error setting up persisted state: %w", err)
 	}
 
 	// Try to rotate the credentials if we're not skipping them.
-	if !skipRotate {
+	if !catalogAttributes.DisableCredentialRotation {
 		if err := state.RotateCreds(); err != nil {
 			return nil, fmt.Errorf("error during credential rotation: %w", err)
 		}
@@ -129,8 +115,12 @@ func (p *AwsPlugin) OnUpdateCatalog(ctx context.Context, req *pb.OnUpdateCatalog
 		return nil, errors.New("new catalog missing attributes")
 	}
 
-	// Validate the region.
-	if _, err := getValidateRegionValue(attrs); err != nil {
+	catalogAttributes, err := getCatalogAttributes(attrs)
+	if err != nil {
+		return nil, err
+	}
+
+	if err := validateRegion(catalogAttributes.Region); err != nil {
 		return nil, err
 	}
 
@@ -147,12 +137,7 @@ func (p *AwsPlugin) OnUpdateCatalog(ctx context.Context, req *pb.OnUpdateCatalog
 		return nil, fmt.Errorf("error loading persisted state: %w", err)
 	}
 
-	skipRotate, err := getBoolValue(attrs, constDisableCredentialRotation, false)
-	if err != nil {
-		return nil, err
-	}
-
-	if skipRotate && !updateSecrets {
+	if catalogAttributes.DisableCredentialRotation && !updateSecrets {
 		// This is a validate check to make sure that we aren't disabling
 		// rotation for credentials currently being managed by rotation.
 		// This is not allowed.
@@ -162,12 +147,7 @@ func (p *AwsPlugin) OnUpdateCatalog(ctx context.Context, req *pb.OnUpdateCatalog
 	}
 
 	if updateSecrets {
-		accessKeyId, err := getStringValue(secrets, constAccessKeyId, true)
-		if err != nil {
-			return nil, err
-		}
-
-		secretAccessKey, err := getStringValue(secrets, constSecretAccessKey, true)
+		catalogSecrets, err := getCatalogSecrets(secrets)
 		if err != nil {
 			return nil, err
 		}
@@ -175,12 +155,12 @@ func (p *AwsPlugin) OnUpdateCatalog(ctx context.Context, req *pb.OnUpdateCatalog
 		// Replace the credentials. This checks the timestamp on the last
 		// rotation time as well and deletes the credentials if we are
 		// managing them (ie: if we've rotated them before).
-		if err := state.ReplaceCreds(accessKeyId, secretAccessKey); err != nil {
+		if err := state.ReplaceCreds(catalogSecrets.AccessKeyId, catalogSecrets.SecretAccessKey); err != nil {
 			return nil, fmt.Errorf("error attempting to replace credentials: %w", err)
 		}
 	}
 
-	if !skipRotate && state.CredsLastRotatedTime.IsZero() {
+	if !catalogAttributes.DisableCredentialRotation && state.CredsLastRotatedTime.IsZero() {
 		// If we're enabling rotation now but didn't before, or have
 		// freshly replaced credentials, we can rotate here.
 		if err := state.RotateCreds(); err != nil {
@@ -229,22 +209,25 @@ func (p *AwsPlugin) OnCreateSet(ctx context.Context, req *pb.OnCreateSetRequest)
 		return nil, errors.New("catalog is nil")
 	}
 
-	catalogAttrs := catalog.GetAttributes()
-	if catalogAttrs == nil {
+	catalogAttrsRaw := catalog.GetAttributes()
+	if catalogAttrsRaw == nil {
 		return nil, errors.New("catalog missing attributes")
 	}
 
+	catalogAttributes, err := getCatalogAttributes(catalogAttrsRaw)
+	if err != nil {
+		return nil, fmt.Errorf("integrity error in host catalog attributes: %w", err)
+	}
+
+	if err := validateRegion(catalogAttributes.Region); err != nil {
+		return nil, fmt.Errorf("integrity error in host catalog attributes: %w", err)
+	}
+
 	state, err := awsCatalogPersistedStateFromProto(req.GetPersisted(), p.testStateOpts...)
 	if err != nil {
 		return nil, fmt.Errorf("error loading persisted state: %w", err)
 	}
 
-	// Get/validate the region.
-	region, err := getValidateRegionValue(catalogAttrs)
-	if err != nil {
-		return nil, fmt.Errorf("catalog validation error: %s", err)
-	}
-
 	set := req.GetSet()
 	if set == nil {
 		return nil, errors.New("set is nil")
@@ -258,7 +241,7 @@ func (p *AwsPlugin) OnCreateSet(ctx context.Context, req *pb.OnCreateSetRequest)
 		return nil, fmt.Errorf("error parsing set attributes: %w", err)
 	}
 
-	ec2Client, err := state.EC2Client(region)
+	ec2Client, err := state.EC2Client(catalogAttributes.Region)
 	if err != nil {
 		return nil, fmt.Errorf("error getting EC2 client: %w", err)
 	}
@@ -287,22 +270,25 @@ func (p *AwsPlugin) OnUpdateSet(ctx context.Context, req *pb.OnUpdateSetRequest)
 		return nil, errors.New("catalog is nil")
 	}
 
-	catalogAttrs := catalog.GetAttributes()
-	if catalogAttrs == nil {
+	catalogAttrsRaw := catalog.GetAttributes()
+	if catalogAttrsRaw == nil {
 		return nil, errors.New("catalog missing attributes")
 	}
 
+	catalogAttributes, err := getCatalogAttributes(catalogAttrsRaw)
+	if err != nil {
+		return nil, fmt.Errorf("integrity error in host catalog attributes: %w", err)
+	}
+
+	if err := validateRegion(catalogAttributes.Region); err != nil {
+		return nil, fmt.Errorf("integrity error in host catalog attributes: %w", err)
+	}
+
 	state, err := awsCatalogPersistedStateFromProto(req.GetPersisted(), p.testStateOpts...)
 	if err != nil {
 		return nil, fmt.Errorf("error loading persisted state: %w", err)
 	}
 
-	// Get/validate the region.
-	region, err := getValidateRegionValue(catalogAttrs)
-	if err != nil {
-		return nil, fmt.Errorf("catalog validation error: %s", err)
-	}
-
 	// As with catalog, we don't need to really look at the old host
 	// set here, just need to work off of/validate the new set config
 	set := req.GetNewSet()
@@ -318,7 +304,7 @@ func (p *AwsPlugin) OnUpdateSet(ctx context.Context, req *pb.OnUpdateSetRequest)
 		return nil, fmt.Errorf("error parsing set attributes: %w", err)
 	}
 
-	ec2Client, err := state.EC2Client(region)
+	ec2Client, err := state.EC2Client(catalogAttributes.Region)
 	if err != nil {
 		return nil, fmt.Errorf("error getting EC2 client: %w", err)
 	}
@@ -353,22 +339,25 @@ func (p *AwsPlugin) ListHosts(ctx context.Context, req *pb.ListHostsRequest) (*p
 		return nil, errors.New("catalog is nil")
 	}
 
-	catalogAttrs := catalog.GetAttributes()
-	if catalogAttrs == nil {
+	catalogAttrsRaw := catalog.GetAttributes()
+	if catalogAttrsRaw == nil {
 		return nil, errors.New("catalog missing attributes")
 	}
 
+	catalogAttributes, err := getCatalogAttributes(catalogAttrsRaw)
+	if err != nil {
+		return nil, fmt.Errorf("integrity error in host catalog attributes: %w", err)
+	}
+
+	if err := validateRegion(catalogAttributes.Region); err != nil {
+		return nil, fmt.Errorf("integrity error in host catalog attributes: %w", err)
+	}
+
 	state, err := awsCatalogPersistedStateFromProto(req.GetPersisted(), p.testStateOpts...)
 	if err != nil {
 		return nil, fmt.Errorf("error loading persisted state: %w", err)
 	}
 
-	// Get/validate the region.
-	region, err := getValidateRegionValue(catalogAttrs)
-	if err != nil {
-		return nil, fmt.Errorf("catalog validation error: %w", err)
-	}
-
 	sets := req.GetSets()
 	if sets == nil {
 		return nil, errors.New("sets is nil")
@@ -407,7 +396,7 @@ func (p *AwsPlugin) ListHosts(ctx context.Context, req *pb.ListHostsRequest) (*p
 		}
 	}
 
-	ec2Client, err := state.EC2Client(region)
+	ec2Client, err := state.EC2Client(catalogAttributes.Region)
 	if err != nil {
 		return nil, fmt.Errorf("error getting EC2 client: %w", err)
 	}
@@ -469,101 +458,13 @@ func (p *AwsPlugin) ListHosts(ctx context.Context, req *pb.ListHostsRequest) (*p
 	}, nil
 }
 
-func getStringValue(in *structpb.Struct, k string, required bool) (string, error) {
-	mv := in.AsMap()
-	v, ok := mv[k]
-	if !ok {
-		if required {
-			return "", fmt.Errorf("missing required value %q", k)
-		}
-
-		return "", nil
-	}
-
-	s, ok := v.(string)
-	if !ok {
-		return "", fmt.Errorf("unexpected type for value %q: want string, got %T", k, v)
-	}
-
-	if s == "" && required {
-		return "", fmt.Errorf("value %q cannot be empty", k)
-	}
-
-	return s, nil
-}
-
-func getBoolValue(in *structpb.Struct, k string, required bool) (bool, error) {
-	mv := in.AsMap()
-	v, ok := mv[k]
-	if !ok {
-		if required {
-			return false, fmt.Errorf("missing required value %q", k)
-		}
-
-		return false, nil
-	}
-
-	b, ok := v.(bool)
-	if !ok {
-		return false, fmt.Errorf("unexpected type for value %q: want bool, got %T", k, v)
-	}
-
-	return b, nil
-}
-
-func getTimeValue(in *structpb.Struct, k string) (time.Time, error) {
-	mv := in.AsMap()
-	v, ok := mv[k]
-	if !ok {
-		return time.Time{}, nil
-	}
-
-	tRaw, ok := v.(string)
-	if !ok {
-		return time.Time{}, fmt.Errorf("unexpected type for value %q: want string, got %T", k, v)
-	}
-
-	t, err := time.Parse(time.RFC3339Nano, tRaw)
-	if err != nil {
-		return time.Time{}, fmt.Errorf("could not parse time in value %q: %w", k, err)
-	}
-
-	return t, nil
-}
-
-func getSetAttributes(in *structpb.Struct) (*SetAttributes, error) {
-	var setAttrs SetAttributes
-
-	// Mapstructure complains if it expects a slice as output and sees a scalar
-	// value. Rather than use WeakDecode and risk unintended consequences, I'm
-	// manually making this change if necessary.
-	inMap := in.AsMap()
-	if filtersRaw, ok := inMap[constDescribeInstancesFilters]; ok {
-		switch filterVal := filtersRaw.(type) {
-		case string:
-			inMap[constDescribeInstancesFilters] = []string{filterVal}
-		}
-	}
-
-	if err := mapstructure.Decode(inMap, &setAttrs); err != nil {
-		return nil, fmt.Errorf("error decoding set attributes: %w", err)
-	}
-
-	return &setAttrs, nil
-}
-
-func getValidateRegionValue(in *structpb.Struct) (string, error) {
-	region, err := getStringValue(in, constRegion, true)
-	if err != nil {
-		return "", err
-	}
-
+func validateRegion(region string) error {
 	_, found := endpoints.PartitionForRegion(endpoints.DefaultPartitions(), region)
 	if !found {
-		return "", fmt.Errorf("not a valid region: %s", region)
+		return fmt.Errorf("not a valid region: %s", region)
 	}
 
-	return region, nil
+	return nil
 }
 
 func buildFilters(attrs *SetAttributes) ([]*ec2.Filter, error) {

plugin/plugin_test.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"sort"
 	"testing"
-	"time"
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
@@ -47,19 +46,17 @@ func TestPluginOnCreateCatalogErr(t *testing.T) {
 			expectedErr: "attributes are required",
 		},
 		{
-			name: "invalid region",
+			name: "error reading attributes",
 			req: &pb.OnCreateCatalogRequest{
 				Catalog: &hostcatalogs.HostCatalog{
-					Secrets: new(structpb.Struct),
-					Attributes: mustStruct(map[string]interface{}{
-						constRegion: "foobar",
-					}),
+					Secrets:    new(structpb.Struct),
+					Attributes: new(structpb.Struct),
 				},
 			},
-			expectedErr: "not a valid region: foobar",
+			expectedErr: "missing required value \"region\"",
 		},
 		{
-			name: "missing access key ID",
+			name: "error reading secrets",
 			req: &pb.OnCreateCatalogRequest{
 				Catalog: &hostcatalogs.HostCatalog{
 					Secrets: new(structpb.Struct),
@@ -71,21 +68,7 @@ func TestPluginOnCreateCatalogErr(t *testing.T) {
 			expectedErr: "missing required value \"access_key_id\"",
 		},
 		{
-			name: "missing secret access key",
-			req: &pb.OnCreateCatalogRequest{
-				Catalog: &hostcatalogs.HostCatalog{
-					Secrets: mustStruct(map[string]interface{}{
-						constAccessKeyId: "foobar",
-					}),
-					Attributes: mustStruct(map[string]interface{}{
-						constRegion: "us-west-2",
-					}),
-				},
-			},
-			expectedErr: "missing required value \"secret_access_key\"",
-		},
-		{
-			name: "invalid value for skipping rotation",
+			name: "invalid region",
 			req: &pb.OnCreateCatalogRequest{
 				Catalog: &hostcatalogs.HostCatalog{
 					Secrets: mustStruct(map[string]interface{}{
@@ -93,12 +76,11 @@ func TestPluginOnCreateCatalogErr(t *testing.T) {
 						constSecretAccessKey: "bazqux",
 					}),
 					Attributes: mustStruct(map[string]interface{}{
-						constRegion:                    "us-west-2",
-						constDisableCredentialRotation: "sure",
+						constRegion: "foobar",
 					}),
 				},
 			},
-			expectedErr: "unexpected type for value \"disable_credential_rotation\": want bool, got string",
+			expectedErr: "not a valid region: foobar",
 		},
 		{
 			name: "persisted state setup error",
@@ -205,6 +187,16 @@ func TestPluginOnUpdateCatalogErr(t *testing.T) {
 			},
 			expectedErr: "new catalog missing attributes",
 		},
+		{
+			name: "error reading attributes",
+			req: &pb.OnUpdateCatalogRequest{
+				NewCatalog: &hostcatalogs.HostCatalog{
+					Secrets:    new(structpb.Struct),
+					Attributes: new(structpb.Struct),
+				},
+			},
+			expectedErr: "missing required value \"region\"",
+		},
 		{
 			name: "invalid region",
 			req: &pb.OnUpdateCatalogRequest{
@@ -239,25 +231,6 @@ func TestPluginOnUpdateCatalogErr(t *testing.T) {
 			},
 			expectedErr: fmt.Sprintf("error loading persisted state: %s", testOptionErr),
 		},
-		{
-			name: "invalid value for credential rotation",
-			req: &pb.OnUpdateCatalogRequest{
-				NewCatalog: &hostcatalogs.HostCatalog{
-					Attributes: mustStruct(map[string]interface{}{
-						constRegion:                    "us-west-2",
-						constDisableCredentialRotation: "sure",
-					}),
-				},
-				Persisted: &pb.HostCatalogPersisted{
-					Secrets: mustStruct(map[string]interface{}{
-						constAccessKeyId:          "foobar",
-						constSecretAccessKey:      "bazqux",
-						constCredsLastRotatedTime: "2006-01-02T15:04:05+07:00",
-					}),
-				},
-			},
-			expectedErr: "unexpected type for value \"disable_credential_rotation\": want bool, got string",
-		},
 		{
 			name: "cannot disable rotation for already rotated credentials",
 			req: &pb.OnUpdateCatalogRequest{
@@ -278,7 +251,7 @@ func TestPluginOnUpdateCatalogErr(t *testing.T) {
 			expectedErr: "cannot disable rotation for already-rotated credentials",
 		},
 		{
-			name: "updating secrets, missing access key id",
+			name: "error reading secrets",
 			req: &pb.OnUpdateCatalogRequest{
 				NewCatalog: &hostcatalogs.HostCatalog{
 					Secrets: new(structpb.Struct),
@@ -296,27 +269,6 @@ func TestPluginOnUpdateCatalogErr(t *testing.T) {
 			},
 			expectedErr: "missing required value \"access_key_id\"",
 		},
-		{
-			name: "updating secrets, missing secret access key",
-			req: &pb.OnUpdateCatalogRequest{
-				NewCatalog: &hostcatalogs.HostCatalog{
-					Secrets: mustStruct(map[string]interface{}{
-						constAccessKeyId: "foobar",
-					}),
-					Attributes: mustStruct(map[string]interface{}{
-						constRegion: "us-west-2",
-					}),
-				},
-				Persisted: &pb.HostCatalogPersisted{
-					Secrets: mustStruct(map[string]interface{}{
-						constAccessKeyId:          "foobar",
-						constSecretAccessKey:      "bazqux",
-						constCredsLastRotatedTime: "2006-01-02T15:04:05+07:00",
-					}),
-				},
-			},
-			expectedErr: "missing required value \"secret_access_key\"",
-		},
 		{
 			name: "updating secrets, replace error",
 			req: &pb.OnUpdateCatalogRequest{
@@ -483,6 +435,27 @@ func TestPluginOnCreateSetErr(t *testing.T) {
 			},
 			expectedErr: "catalog missing attributes",
 		},
+		{
+			name: "error reading catalog attributes",
+			req: &pb.OnCreateSetRequest{
+				Catalog: &hostcatalogs.HostCatalog{
+					Secrets:    new(structpb.Struct),
+					Attributes: new(structpb.Struct),
+				},
+			},
+			expectedErr: "integrity error in host catalog attributes: missing required value \"region\"",
+		},
+		{
+			name: "invalid region",
+			req: &pb.OnCreateSetRequest{
+				Catalog: &hostcatalogs.HostCatalog{
+					Attributes: mustStruct(map[string]interface{}{
+						constRegion: "foobar",
+					}),
+				},
+			},
+			expectedErr: "integrity error in host catalog attributes: not a valid region: foobar",
+		},
 		{
 			name: "persisted state setup error",
 			req: &pb.OnCreateSetRequest{
@@ -506,24 +479,6 @@ func TestPluginOnCreateSetErr(t *testing.T) {
 			},
 			expectedErr: fmt.Sprintf("error loading persisted state: %s", testOptionErr),
 		},
-		{
-			name: "invalid region",
-			req: &pb.OnCreateSetRequest{
-				Catalog: &hostcatalogs.HostCatalog{
-					Attributes: mustStruct(map[string]interface{}{
-						constRegion: "foobar",
-					}),
-				},
-				Persisted: &pb.HostCatalogPersisted{
-					Secrets: mustStruct(map[string]interface{}{
-						constAccessKeyId:          "foobar",
-						constSecretAccessKey:      "bazqux",
-						constCredsLastRotatedTime: "2006-01-02T15:04:05+07:00",
-					}),
-				},
-			},
-			expectedErr: "catalog validation error: not a valid region: foobar",
-		},
 		{
 			name: "nil set",
 			req: &pb.OnCreateSetRequest{
@@ -561,6 +516,30 @@ func TestPluginOnCreateSetErr(t *testing.T) {
 			},
 			expectedErr: "set missing attributes",
 		},
+		{
+			name: "set attribute load error",
+			req: &pb.OnCreateSetRequest{
+				Catalog: &hostcatalogs.HostCatalog{
+					Attributes: mustStruct(map[string]interface{}{
+						constRegion: "us-west-2",
+					}),
+				},
+				Persisted: &pb.HostCatalogPersisted{
+					Secrets: mustStruct(map[string]interface{}{
+						constAccessKeyId:          "foobar",
+						constSecretAccessKey:      "bazqux",
+						constCredsLastRotatedTime: "2006-01-02T15:04:05+07:00",
+					}),
+				},
+				Set: &hostsets.HostSet{
+					Attributes: mustStruct(map[string]interface{}{
+						"foo": true,
+						"bar": true,
+					}),
+				},
+			},
+			expectedErr: "error parsing set attributes: unknown set attribute fields provided: bar, foo",
+		},
 		{
 			name: "client load error",
 			req: &pb.OnCreateSetRequest{
@@ -710,6 +689,28 @@ func TestPluginOnUpdateSetErr(t *testing.T) {
 			},
 			expectedErr: "catalog missing attributes",
 		},
+		{
+			name: "catalog attribute load error",
+			req: &pb.OnUpdateSetRequest{
+				Catalog: &hostcatalogs.HostCatalog{
+					Secrets:    new(structpb.Struct),
+					Attributes: new(structpb.Struct),
+				},
+			},
+			expectedErr: "integrity error in host catalog attributes: missing required value \"region\"",
+		},
+		{
+			name: "invalid region",
+			req: &pb.OnUpdateSetRequest{
+				Catalog: &hostcatalogs.HostCatalog{
+					Secrets: new(structpb.Struct),
+					Attributes: mustStruct(map[string]interface{}{
+						constRegion: "foobar",
+					}),
+				},
+			},
+			expectedErr: "integrity error in host catalog attributes: not a valid region: foobar",
+		},
 		{
 			name: "persisted state setup error",
 			req: &pb.OnUpdateSetRequest{
@@ -733,24 +734,6 @@ func TestPluginOnUpdateSetErr(t *testing.T) {
 			},
 			expectedErr: fmt.Sprintf("error loading persisted state: %s", testOptionErr),
 		},
-		{
-			name: "invalid region",
-			req: &pb.OnUpdateSetRequest{
-				Catalog: &hostcatalogs.HostCatalog{
-					Attributes: mustStruct(map[string]interface{}{
-						constRegion: "foobar",
-					}),
-				},
-				Persisted: &pb.HostCatalogPersisted{
-					Secrets: mustStruct(map[string]interface{}{
-						constAccessKeyId:          "foobar",
-						constSecretAccessKey:      "bazqux",
-						constCredsLastRotatedTime: "2006-01-02T15:04:05+07:00",
-					}),
-				},
-			},
-			expectedErr: "catalog validation error: not a valid region: foobar",
-		},
 		{
 			name: "nil set",
 			req: &pb.OnUpdateSetRequest{
@@ -788,6 +771,30 @@ func TestPluginOnUpdateSetErr(t *testing.T) {
 			},
 			expectedErr: "new set missing attributes",
 		},
+		{
+			name: "set attribute load error",
+			req: &pb.OnUpdateSetRequest{
+				Catalog: &hostcatalogs.HostCatalog{
+					Attributes: mustStruct(map[string]interface{}{
+						constRegion: "us-west-2",
+					}),
+				},
+				Persisted: &pb.HostCatalogPersisted{
+					Secrets: mustStruct(map[string]interface{}{
+						constAccessKeyId:          "foobar",
+						constSecretAccessKey:      "bazqux",
+						constCredsLastRotatedTime: "2006-01-02T15:04:05+07:00",
+					}),
+				},
+				NewSet: &hostsets.HostSet{
+					Attributes: mustStruct(map[string]interface{}{
+						"foo": true,
+						"bar": true,
+					}),
+				},
+			},
+			expectedErr: "error parsing set attributes: unknown set attribute fields provided: bar, foo",
+		},
 		{
 			name: "client load error",
 			req: &pb.OnUpdateSetRequest{
@@ -909,6 +916,28 @@ func TestPluginListHostsErr(t *testing.T) {
 			},
 			expectedErr: "catalog missing attributes",
 		},
+		{
+			name: "error reading catalog attributes",
+			req: &pb.ListHostsRequest{
+				Catalog: &hostcatalogs.HostCatalog{
+					Secrets:    new(structpb.Struct),
+					Attributes: new(structpb.Struct),
+				},
+			},
+			expectedErr: "integrity error in host catalog attributes: missing required value \"region\"",
+		},
+		{
+			name: "invalid region",
+			req: &pb.ListHostsRequest{
+				Catalog: &hostcatalogs.HostCatalog{
+					Secrets: new(structpb.Struct),
+					Attributes: mustStruct(map[string]interface{}{
+						constRegion: "foobar",
+					}),
+				},
+			},
+			expectedErr: "integrity error in host catalog attributes: not a valid region: foobar",
+		},
 		{
 			name: "persisted state setup error",
 			req: &pb.ListHostsRequest{
@@ -932,24 +961,6 @@ func TestPluginListHostsErr(t *testing.T) {
 			},
 			expectedErr: fmt.Sprintf("error loading persisted state: %s", testOptionErr),
 		},
-		{
-			name: "invalid region",
-			req: &pb.ListHostsRequest{
-				Catalog: &hostcatalogs.HostCatalog{
-					Attributes: mustStruct(map[string]interface{}{
-						constRegion: "foobar",
-					}),
-				},
-				Persisted: &pb.HostCatalogPersisted{
-					Secrets: mustStruct(map[string]interface{}{
-						constAccessKeyId:          "foobar",
-						constSecretAccessKey:      "bazqux",
-						constCredsLastRotatedTime: "2006-01-02T15:04:05+07:00",
-					}),
-				},
-			},
-			expectedErr: "catalog validation error: not a valid region: foobar",
-		},
 		{
 			name: "nil sets",
 			req: &pb.ListHostsRequest{
@@ -1010,6 +1021,33 @@ func TestPluginListHostsErr(t *testing.T) {
 			},
 			expectedErr: "set missing attributes",
 		},
+		{
+			name: "set attribute load error",
+			req: &pb.ListHostsRequest{
+				Catalog: &hostcatalogs.HostCatalog{
+					Attributes: mustStruct(map[string]interface{}{
+						constRegion: "us-west-2",
+					}),
+				},
+				Persisted: &pb.HostCatalogPersisted{
+					Secrets: mustStruct(map[string]interface{}{
+						constAccessKeyId:          "foobar",
+						constSecretAccessKey:      "bazqux",
+						constCredsLastRotatedTime: "2006-01-02T15:04:05+07:00",
+					}),
+				},
+				Sets: []*hostsets.HostSet{
+					&hostsets.HostSet{
+						Id: "foobar",
+						Attributes: mustStruct(map[string]interface{}{
+							"foo": true,
+							"bar": true,
+						}),
+					},
+				},
+			},
+			expectedErr: "error parsing set attributes: unknown set attribute fields provided: bar, foo",
+		},
 		{
 			name: "client load error",
 			req: &pb.ListHostsRequest{
@@ -1132,297 +1170,6 @@ func TestPluginListHostsErr(t *testing.T) {
 	}
 }
 
-func TestGetStringValue(t *testing.T) {
-	cases := []struct {
-		name        string
-		in          *structpb.Struct
-		key         string
-		required    bool
-		expected    string
-		expectedErr string
-	}{
-		{
-			name:        "required missing",
-			in:          mustStruct(map[string]interface{}{}),
-			key:         "foo",
-			required:    true,
-			expectedErr: "missing required value \"foo\"",
-		},
-		{
-			name:     "optional missing",
-			in:       mustStruct(map[string]interface{}{}),
-			key:      "foo",
-			expected: "",
-		},
-		{
-			name: "non-string value",
-			in: mustStruct(map[string]interface{}{
-				"foo": 1,
-			}),
-			key:         "foo",
-			expectedErr: "unexpected type for value \"foo\": want string, got float64",
-		},
-		{
-			name: "required empty",
-			in: mustStruct(map[string]interface{}{
-				"foo": "",
-			}),
-			key:         "foo",
-			required:    true,
-			expectedErr: "value \"foo\" cannot be empty",
-		},
-		{
-			name: "good",
-			in: mustStruct(map[string]interface{}{
-				"foo": "bar",
-			}),
-			key:      "foo",
-			expected: "bar",
-		},
-	}
-
-	for _, tc := range cases {
-		tc := tc
-		t.Run(tc.name, func(t *testing.T) {
-			require := require.New(t)
-			actual, err := getStringValue(tc.in, tc.key, tc.required)
-			if tc.expectedErr != "" {
-				require.EqualError(err, tc.expectedErr)
-				return
-			}
-
-			require.NoError(err)
-			require.Equal(tc.expected, actual)
-		})
-	}
-}
-
-func TestGetBoolValue(t *testing.T) {
-	cases := []struct {
-		name        string
-		in          *structpb.Struct
-		key         string
-		required    bool
-		expected    bool
-		expectedErr string
-	}{
-		{
-			name:        "required missing",
-			in:          mustStruct(map[string]interface{}{}),
-			key:         "foo",
-			required:    true,
-			expectedErr: "missing required value \"foo\"",
-		},
-		{
-			name:     "optional missing",
-			in:       mustStruct(map[string]interface{}{}),
-			key:      "foo",
-			expected: false,
-		},
-		{
-			name: "non-bool value",
-			in: mustStruct(map[string]interface{}{
-				"foo": "bar",
-			}),
-			key:         "foo",
-			expectedErr: "unexpected type for value \"foo\": want bool, got string",
-		},
-		{
-			name: "good",
-			in: mustStruct(map[string]interface{}{
-				"foo": true,
-			}),
-			key:      "foo",
-			expected: true,
-		},
-	}
-
-	for _, tc := range cases {
-		tc := tc
-		t.Run(tc.name, func(t *testing.T) {
-			require := require.New(t)
-			actual, err := getBoolValue(tc.in, tc.key, tc.required)
-			if tc.expectedErr != "" {
-				require.EqualError(err, tc.expectedErr)
-				return
-			}
-
-			require.NoError(err)
-			require.Equal(tc.expected, actual)
-		})
-	}
-}
-
-func TestGetTimeValue(t *testing.T) {
-	staticTime := time.Now()
-
-	cases := []struct {
-		name        string
-		in          *structpb.Struct
-		key         string
-		expected    time.Time
-		expectedErr string
-	}{
-		{
-			name:     "missing",
-			in:       mustStruct(map[string]interface{}{}),
-			key:      "foo",
-			expected: time.Time{},
-		},
-		{
-			name: "non-time value",
-			in: mustStruct(map[string]interface{}{
-				"foo": 1,
-			}),
-			key:         "foo",
-			expectedErr: "unexpected type for value \"foo\": want string, got float64",
-		},
-		{
-			name: "bad parse",
-			in: mustStruct(map[string]interface{}{
-				"foo": "bar",
-			}),
-			key:         "foo",
-			expectedErr: "could not parse time in value \"foo\": parsing time \"bar\" as \"2006-01-02T15:04:05.999999999Z07:00\": cannot parse \"bar\" as \"2006\"",
-		},
-		{
-			name: "good",
-			in: mustStruct(map[string]interface{}{
-				"foo": staticTime.Format(time.RFC3339Nano),
-			}),
-			key: "foo",
-			expected: func() time.Time {
-				u, err := time.Parse(time.RFC3339Nano, staticTime.Format(time.RFC3339Nano))
-				if err != nil {
-					panic(err)
-				}
-
-				return u
-			}(),
-		},
-	}
-
-	for _, tc := range cases {
-		tc := tc
-		t.Run(tc.name, func(t *testing.T) {
-			require := require.New(t)
-			actual, err := getTimeValue(tc.in, tc.key)
-			if tc.expectedErr != "" {
-				require.EqualError(err, tc.expectedErr)
-				return
-			}
-
-			require.NoError(err)
-			require.Equal(tc.expected, actual)
-		})
-	}
-}
-
-func TestGetSetAttributes(t *testing.T) {
-	cases := []struct {
-		name                string
-		in                  *structpb.Struct
-		expected            *SetAttributes
-		expectedErrContains string
-	}{
-		{
-			name:     "missing",
-			in:       mustStruct(map[string]interface{}{}),
-			expected: &SetAttributes{},
-		},
-		{
-			name: "non-string-slice value",
-			in: mustStruct(map[string]interface{}{
-				constDescribeInstancesFilters: "zip=foo,bar",
-			}),
-			expected: &SetAttributes{
-				Filters: []string{"zip=foo,bar"},
-			},
-		},
-		{
-			name: "bad filter element value",
-			in: mustStruct(map[string]interface{}{
-				constDescribeInstancesFilters: []interface{}{1},
-			}),
-			expectedErrContains: "expected type 'string', got unconvertible type 'float64'",
-		},
-		{
-			name: "good",
-			in: mustStruct(map[string]interface{}{
-				constDescribeInstancesFilters: []interface{}{
-					"foo=bar",
-					"zip=zap",
-				},
-			}),
-			expected: &SetAttributes{
-				Filters: []string{"foo=bar", "zip=zap"},
-			},
-		},
-	}
-
-	for _, tc := range cases {
-		tc := tc
-		t.Run(tc.name, func(t *testing.T) {
-			require := require.New(t)
-			actual, err := getSetAttributes(tc.in)
-			if tc.expectedErrContains != "" {
-				require.Error(err)
-				require.Contains(err.Error(), tc.expectedErrContains)
-				return
-			}
-
-			require.NoError(err)
-			require.Equal(tc.expected, actual)
-		})
-	}
-}
-
-func TestGetValidateRegionValue(t *testing.T) {
-	cases := []struct {
-		name        string
-		in          *structpb.Struct
-		key         string
-		expected    string
-		expectedErr string
-	}{
-		{
-			name:        "missing",
-			in:          mustStruct(map[string]interface{}{}),
-			key:         "region",
-			expectedErr: fmt.Sprintf("missing required value %q", constRegion),
-		},
-		{
-			name: "invalid region",
-			in: mustStruct(map[string]interface{}{
-				constRegion: "foobar",
-			}),
-			expectedErr: "not a valid region: foobar",
-		},
-		{
-			name: "good",
-			in: mustStruct(map[string]interface{}{
-				constRegion: "us-west-2",
-			}),
-			expected: "us-west-2",
-		},
-	}
-
-	for _, tc := range cases {
-		tc := tc
-		t.Run(tc.name, func(t *testing.T) {
-			require := require.New(t)
-			actual, err := getValidateRegionValue(tc.in)
-			if tc.expectedErr != "" {
-				require.EqualError(err, tc.expectedErr)
-				return
-			}
-
-			require.NoError(err)
-			require.Equal(tc.expected, actual)
-		})
-	}
-}
-
 func TestBuildFilters(t *testing.T) {
 	cases := []struct {
 		name                string