diff --git a/plugin/attributes.go b/plugin/attributes.go new file mode 100644 index 0000000..674be66 --- /dev/null +++ b/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, ", ") +} diff --git a/plugin/attributes_test.go b/plugin/attributes_test.go new file mode 100644 index 0000000..18ccb66 --- /dev/null +++ b/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) + }) + } +} diff --git a/plugin/e2e_test.go b/plugin/e2e_test.go index 1f43e20..6f2c50b 100644 --- a/plugin/e2e_test.go +++ b/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{ diff --git a/plugin/plugin.go b/plugin/plugin.go index 2d96e1a..1c897e8 100644 --- a/plugin/plugin.go +++ b/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) { diff --git a/plugin/plugin_test.go b/plugin/plugin_test.go index 495cbb5..961aa8d 100644 --- a/plugin/plugin_test.go +++ b/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