diff --git a/docs/coverage/rds/README.md b/docs/coverage/rds/README.md index 5f6de84b1..1007c6a42 100644 --- a/docs/coverage/rds/README.md +++ b/docs/coverage/rds/README.md @@ -67,6 +67,7 @@ A parameter whose omission would create a false safety, security or availability | `EnableCustomerOwnedIp` | An Outposts feature | | `ForceFailover` (reboot) | No standby to fail over to | | `DBSnapshotIdentifier` (stop) | Snapshot-on-stop is not implemented | +| `dbi-resource-id` filter (DescribeDBSnapshots, DescribeDBInstanceAutomatedBackups) | Snapshots and automated backups do not record their source instance's resource ID | ### Operations diff --git a/docs/coverage/rds/intro.md b/docs/coverage/rds/intro.md index f56508dc0..2eb5c7c09 100644 --- a/docs/coverage/rds/intro.md +++ b/docs/coverage/rds/intro.md @@ -44,3 +44,4 @@ A parameter whose omission would create a false safety, security or availability | `EnableCustomerOwnedIp` | An Outposts feature | | `ForceFailover` (reboot) | No standby to fail over to | | `DBSnapshotIdentifier` (stop) | Snapshot-on-stop is not implemented | +| `dbi-resource-id` filter (DescribeDBSnapshots, DescribeDBInstanceAutomatedBackups) | Snapshots and automated backups do not record their source instance's resource ID | diff --git a/spinifex/gateway/rds/enginecatalog.go b/spinifex/gateway/rds/enginecatalog.go index d29ec9483..34ab79bba 100644 --- a/spinifex/gateway/rds/enginecatalog.go +++ b/spinifex/gateway/rds/enginecatalog.go @@ -17,9 +17,8 @@ import ( "github.com/nats-io/nats.go" ) -// The filter names each action recognises. Rejecting an unknown name is only -// implementable against a closed vocabulary, and the two actions take disjoint -// typed parameters, so each keeps its own list rather than sharing one. +// The filter names each action recognises, compared case-sensitively as AWS +// does. The two actions take disjoint typed parameters, so each keeps its own list. const ( filterNameEngine = "engine" filterNameEngineVersion = "engine-version" @@ -48,22 +47,20 @@ func DescribeDBEngineVersions(ctx context.Context, input *rds.DescribeDBEngineVe filter.EngineVersion.AddParam(aws.StringValue(input.EngineVersion)) filter.ParameterGroupFamily.AddParam(aws.StringValue(input.DBParameterGroupFamily)) - for _, entry := range input.Filters { - name, values, err := filterEntry(entry) - if err != nil { - return nil, err - } - switch name { + entries, err := handlers_rds.ReadFilters(input.Filters, engineVersionFilterNames...) + if err != nil { + return nil, err + } + for _, entry := range entries { + switch entry.Name { case filterNameEngine: - filter.Engine.AddFilter(values) + filter.Engine.AddFilter(entry.Values) case filterNameEngineVersion: - filter.EngineVersion.AddFilter(values) + filter.EngineVersion.AddFilter(entry.Values) case filterNameParameterGroupFamily: - filter.ParameterGroupFamily.AddFilter(values) + filter.ParameterGroupFamily.AddFilter(entry.Values) case filterNameStatus: - filter.Status.AddFilter(values) - default: - return nil, unknownFilterName(name, engineVersionFilterNames) + filter.Status.AddFilter(entry.Values) } } @@ -102,28 +99,26 @@ func DescribeOrderableDBInstanceOptions(ctx context.Context, input *rds.Describe filter.Vpc.AddParam(strconv.FormatBool(aws.BoolValue(input.Vpc))) } - for _, entry := range input.Filters { - name, values, err := filterEntry(entry) - if err != nil { - return nil, err - } - switch name { + entries, err := handlers_rds.ReadFilters(input.Filters, orderableFilterNames...) + if err != nil { + return nil, err + } + for _, entry := range entries { + switch entry.Name { case filterNameEngine: - filter.Engine.AddFilter(values) + filter.Engine.AddFilter(entry.Values) case filterNameEngineVersion: - filter.EngineVersion.AddFilter(values) + filter.EngineVersion.AddFilter(entry.Values) case filterNameDBInstanceClass: - filter.DBInstanceClass.AddFilter(values) + filter.DBInstanceClass.AddFilter(entry.Values) case filterNameLicenseModel: - filter.LicenseModel.AddFilter(values) + filter.LicenseModel.AddFilter(entry.Values) case filterNameVpc: - parsed, perr := boolFilterValues(name, values) + parsed, perr := boolFilterValues(entry.Name, entry.Values) if perr != nil { return nil, perr } filter.Vpc.AddFilter(parsed) - default: - return nil, unknownFilterName(name, orderableFilterNames) } } @@ -172,20 +167,6 @@ func clusterRunnableTypes(ctx context.Context, nc *nats.Conn, env Env) (func(str return func(instanceType string) bool { return supported[instanceType] }, nil } -// Both members are required by the shape, and treating either as absent would -// silently widen the result rather than narrow it. -func filterEntry(entry *rds.Filter) (string, []string, error) { - if entry == nil || strings.TrimSpace(aws.StringValue(entry.Name)) == "" { - return "", nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "each filter must carry a name") - } - name := strings.ToLower(strings.TrimSpace(aws.StringValue(entry.Name))) - if len(entry.Values) == 0 { - return "", nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, - "filter %q must carry at least one value", name) - } - return name, aws.StringValueSlice(entry.Values), nil -} - // A bool-shaped filter is parsed rather than compared as text, so it accepts // every spelling the typed parameter does. Matching only "true" and "false" // would answer "1" with an empty catalog the caller cannot tell from a real one. @@ -201,10 +182,3 @@ func boolFilterValues(name string, values []string) ([]string, error) { } return parsed, nil } - -// Rejected rather than ignored: these two actions exist only to be filtered, so -// a dropped filter returns rows the caller asked not to see and cannot detect. -func unknownFilterName(name string, recognised []string) error { - return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, - "filter %q is not recognised; supported filters are %s", name, strings.Join(recognised, ", ")) -} diff --git a/spinifex/gateway/rds/enginecatalog_test.go b/spinifex/gateway/rds/enginecatalog_test.go index 9a3a99138..dca3dafaf 100644 --- a/spinifex/gateway/rds/enginecatalog_test.go +++ b/spinifex/gateway/rds/enginecatalog_test.go @@ -219,13 +219,25 @@ func TestDescribeOrderableDBInstanceOptions_RejectANonBooleanVpcFilter(t *testin } } +// AWS's answers: names are compared case-sensitively and checked before the +// values, and a missing name is reported as "null". func TestDescribeCatalogs_RejectMalformedFilters(t *testing.T) { cases := []struct { name string query map[string]string + code string + msg string }{ - {"no values", map[string]string{"Filters.Filter.1.Name": "engine"}}, - {"no name", map[string]string{"Filters.Filter.1.Values.Value.1": "postgres"}}, + {"no values", map[string]string{"Filters.Filter.1.Name": "engine"}, + awserrors.ErrorInvalidParameterCombination, "The values list cannot be null for the filter engine."}, + {"empty values", map[string]string{"Filters.Filter.1.Name": "engine", "Filters.Filter.1.Values": ""}, + awserrors.ErrorInvalidParameterCombination, "The values list cannot be null for the filter engine."}, + {"no name", map[string]string{"Filters.Filter.1.Values.Value.1": "postgres"}, + awserrors.ErrorInvalidParameterValue, "Unrecognized filter name: null"}, + {"name case", map[string]string{"Filters.Filter.1.Name": "Engine", "Filters.Filter.1.Values.Value.1": "postgres"}, + awserrors.ErrorInvalidParameterValue, "Unrecognized filter name: Engine"}, + {"unknown name without values", map[string]string{"Filters.Filter.1.Name": "bogus"}, + awserrors.ErrorInvalidParameterValue, "Unrecognized filter name: bogus"}, } for _, tc := range cases { @@ -233,7 +245,8 @@ func TestDescribeCatalogs_RejectMalformedFilters(t *testing.T) { tc.query["Action"] = "DescribeDBEngineVersions" _, err := Dispatch(t.Context(), "DescribeDBEngineVersions", tc.query, nil, testCaller, testEnv) require.Error(t, err) - assert.Contains(t, err.Error(), awserrors.ErrorInvalidParameterValue) + assert.Contains(t, err.Error(), tc.code) + assert.Contains(t, err.Error(), tc.msg) }) } } diff --git a/spinifex/handlers/rds/backup.go b/spinifex/handlers/rds/backup.go index 3b7673e72..87726c7e8 100644 --- a/spinifex/handlers/rds/backup.go +++ b/spinifex/handlers/rds/backup.go @@ -398,7 +398,7 @@ func (s *Service) DescribeDBInstanceAutomatedBackups(ctx context.Context, if input == nil { input = &rds.DescribeDBInstanceAutomatedBackupsInput{} } - wanted, err := validateDescribeAutomatedBackupsRequest(input) + wanted, filters, err := validateDescribeAutomatedBackupsRequest(input) if err != nil { return nil, err } @@ -437,7 +437,19 @@ func (s *Service) DescribeDBInstanceAutomatedBackups(ctx context.Context, if err != nil { return nil, err } - backups = append(backups, s.projectAutomatedBackup(&rec, len(stamps))) + backup := s.projectAutomatedBackup(&rec, len(stamps)) + if !matchesFilters(filters, func(name string) (string, bool) { + switch name { + case filterDBInstanceID: + return aws.StringValue(backup.DBInstanceIdentifier), true + case filterStatus: + return aws.StringValue(backup.Status), true + } + return "", false + }) { + continue + } + backups = append(backups, backup) } backups, next, err := Page(backups, automatedBackupPageKey, input.MaxRecords, input.Marker) if err != nil { @@ -446,26 +458,47 @@ func (s *Service) DescribeDBInstanceAutomatedBackups(ctx context.Context, return &rds.DescribeDBInstanceAutomatedBackupsOutput{DBInstanceAutomatedBackups: backups, Marker: next}, nil } -// A filter this phase cannot honour is rejected rather than dropped, since a +// A scope this phase cannot honour is rejected rather than dropped, since a // silently unfiltered list reads as a complete answer. Returns the DB instance to -// report on, empty for every one. -func validateDescribeAutomatedBackupsRequest(input *rds.DescribeDBInstanceAutomatedBackupsInput) (string, error) { +// report on, empty for every one, and the Filters. +func validateDescribeAutomatedBackupsRequest(input *rds.DescribeDBInstanceAutomatedBackupsInput) (string, []Filter, error) { if input == nil { - return "", nil + return "", nil, nil } - if len(input.Filters) > 0 { - return "", unimplemented("Filters", "DescribeDBInstanceAutomatedBackups filters on DBInstanceIdentifier only") + filters, err := ReadFilters(input.Filters, filterDBInstanceID, filterDbiResourceID, filterStatus) + if err != nil { + return "", nil, err + } + for _, filter := range filters { + switch filter.Name { + case filterDbiResourceID: + return "", nil, unimplemented("Filter dbi-resource-id", automatedBackupNoResourceID) + case filterStatus: + for _, value := range filter.Values { + if !slices.Contains(awsAutomatedBackupStatuses, value) { + return "", nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "The parameter Filter: status is not a valid Automated Backup status. Allowed values are creating, active, retained, pending, replicating, deleting.") + } + } + } } if aws.StringValue(input.DbiResourceId) != "" { - return "", unimplemented("DbiResourceId", "a DB instance has no resource ID distinct from its identifier here") + return "", nil, unimplemented("DbiResourceId", automatedBackupNoResourceID) } if aws.StringValue(input.DBInstanceAutomatedBackupsArn) != "" { - return "", unimplemented("DBInstanceAutomatedBackupsArn", + return "", nil, unimplemented("DBInstanceAutomatedBackupsArn", "cross-region automated backup replication is not offered, so no replicated backup has an ARN") } - return aws.StringValue(input.DBInstanceIdentifier), nil + return aws.StringValue(input.DBInstanceIdentifier), filters, nil } +// Why DbiResourceId and the dbi-resource-id filter are refused: an instance has +// one, but its automated backups are addressed by its identifier only. +const automatedBackupNoResourceID = "automated backups are addressed by DBInstanceIdentifier only" + +// AWS's whole status vocabulary; the ones never reported here match nothing. +var awsAutomatedBackupStatuses = []string{"creating", "active", "retained", "pending", "replicating", "deleting"} + // RestoreWindow and LatestRestorableTime are deliberately absent: this phase // backs discrete daily snapshots, and reporting a restore window would tell a // client it can recover to any instant inside it. diff --git a/spinifex/handlers/rds/backup_test.go b/spinifex/handlers/rds/backup_test.go index 22bf0960a..a61dd02b3 100644 --- a/spinifex/handlers/rds/backup_test.go +++ b/spinifex/handlers/rds/backup_test.go @@ -766,14 +766,37 @@ func TestDescribeDBInstanceAutomatedBackups_RejectsAnUnknownInstance(t *testing. assert.Contains(t, err.Error(), awserrors.ErrorDBInstanceNotFound) } -// A filter this phase cannot honour is rejected, because a silently -// unfiltered list reads as a complete answer. +func TestDescribeDBInstanceAutomatedBackups_FiltersByInstanceAndStatus(t *testing.T) { + t.Parallel() + h := newSnapshotHarness(t, false) + seedInstance(t, h.svc, backupReadyRecord()) + + for name, tc := range map[string]struct { + filter *rds.Filter + want int + }{ + "own instance": {&rds.Filter{Name: aws.String("db-instance-id"), Values: aws.StringSlice([]string{testDBID})}, 1}, + "other instance": {&rds.Filter{Name: aws.String("db-instance-id"), Values: aws.StringSlice([]string{"other-db"})}, 0}, + "own status": {&rds.Filter{Name: aws.String("status"), Values: aws.StringSlice([]string{"retained", "creating"})}, 1}, + "other status": {&rds.Filter{Name: aws.String("status"), Values: aws.StringSlice([]string{"active"})}, 0}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + out, err := h.svc.DescribeDBInstanceAutomatedBackups(t.Context(), + &rds.DescribeDBInstanceAutomatedBackupsInput{Filters: []*rds.Filter{tc.filter}}, testAccountID) + require.NoError(t, err) + assert.Len(t, out.DBInstanceAutomatedBackups, tc.want) + }) + } +} + +// A scope this phase cannot honour is rejected, because a silently unfiltered +// list reads as a complete answer; so are names and statuses AWS refuses. func TestDescribeDBInstanceAutomatedBackups_RejectsUnimplementedFilters(t *testing.T) { cases := map[string]*rds.DescribeDBInstanceAutomatedBackupsInput{ - "Filters": {Filters: []*rds.Filter{{ - Name: aws.String("db-instance-id"), - Values: aws.StringSlice([]string{testDBID}), - }}}, + "dbi-resource-id filter": {Filters: filter("dbi-resource-id", "db-ABCDEF")}, + "unknown filter": {Filters: filter("engine", "postgres")}, + "bad status": {Filters: filter("status", "available")}, "DbiResourceId": {DbiResourceId: aws.String("db-ABCDEF")}, "DBInstanceAutomatedBackupsArn": {DBInstanceAutomatedBackupsArn: aws.String("arn:aws:rds:::auto-backup:x")}, } diff --git a/spinifex/handlers/rds/describe.go b/spinifex/handlers/rds/describe.go index 54ab9a3c8..b9060710e 100644 --- a/spinifex/handlers/rds/describe.go +++ b/spinifex/handlers/rds/describe.go @@ -4,6 +4,8 @@ import ( "context" "errors" "slices" + "strings" + "unicode/utf8" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/rds" @@ -11,14 +13,8 @@ import ( "github.com/mulgadc/spinifex/spinifex/kvstore" ) -// The prefix AWS gives a DB instance's immutable resource ID, and the two filter -// names DescribeDBInstances accepts against it. -const ( - dbiResourceIDPrefix = "db" - - filterDbiResourceID = "dbi-resource-id" - filterDBInstanceID = "db-instance-id" -) +// The prefix AWS gives a DB instance's immutable resource ID. +const dbiResourceIDPrefix = "db" // Named DB instances that do not exist are an error, matching AWS: a client // polling a create would otherwise read an empty list as "gone" rather than @@ -79,46 +75,58 @@ func (s *Service) DescribeDBInstances(ctx context.Context, input *rds.DescribeDB return &rds.DescribeDBInstancesOutput{DBInstances: instances, Marker: next}, nil } -// The filter names AWS documents for this action and clients actually send. -// Ignoring them is not a safe default here: the Terraform provider keys its +// Ignoring a filter is not a safe default here: the Terraform provider keys its // state off DbiResourceId and reads an instance back by filtering on it, so an // unapplied filter answers with every instance in the account, which the -// provider rejects as an ambiguous match. An unrecognised name is refused for -// the same reason — a filter silently dropped returns rows the caller asked to -// exclude. +// provider rejects as an ambiguous match. // // Returns nil when there is nothing to filter on, so the common path allocates // no closure. -func dbInstanceFilterMatcher(filters []*rds.Filter) (func(*DBInstanceRecord) bool, error) { - if len(filters) == 0 { - return nil, nil +func dbInstanceFilterMatcher(entries []*rds.Filter) (func(*DBInstanceRecord) bool, error) { + filters, err := ReadFilters(entries, filterDBClusterID, filterDBInstanceID, filterDbiResourceID, filterDomain, filterEngine) + if err != nil || len(filters) == 0 { + return nil, err } for _, filter := range filters { - switch name := aws.StringValue(filter.Name); name { - case filterDbiResourceID, filterDBInstanceID: - default: - return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, - "unrecognized filter name: %s", name) + if filter.Name != filterDomain { + continue + } + for _, value := range filter.Values { + if !validDomainID(value) { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "The parameter Filter: domain is not a valid Domain identifier. Domain identifier must begin with d- prefix, must contain only ASCII characters, and must not be empty") + } } } - // AWS's own semantics: a record must match every filter, and matches a filter - // by carrying any one of its values. + // No instance belongs to a cluster or joins a directory domain, so those two + // filters match nothing. return func(rec *DBInstanceRecord) bool { - for _, filter := range filters { - var value string - if aws.StringValue(filter.Name) == filterDbiResourceID { - value = rec.DbiResourceID - } else { - value = rec.DBInstanceIdentifier + return matchesFilters(filters, func(name string) (string, bool) { + switch name { + case filterDbiResourceID: + return rec.DbiResourceID, true + case filterDBInstanceID: + return rec.DBInstanceIdentifier, true + case filterEngine: + return rec.Engine, true } - if !slices.Contains(aws.StringValueSlice(filter.Values), value) { - return false - } - } - return true + return "", false + }) }, nil } +func validDomainID(value string) bool { + if !strings.HasPrefix(value, "d-") { + return false + } + for i := 0; i < len(value); i++ { + if value[i] >= utf8.RuneSelf { + return false + } + } + return true +} + // Returns the record plus its revision, for callers that follow with a CAS. func (s *Service) getDBInstance(ctx context.Context, kv *kvstore.Bucket, id string) (*DBInstanceRecord, uint64, error) { var rec DBInstanceRecord diff --git a/spinifex/handlers/rds/describe_test.go b/spinifex/handlers/rds/describe_test.go index 048dc2bb5..89187f03d 100644 --- a/spinifex/handlers/rds/describe_test.go +++ b/spinifex/handlers/rds/describe_test.go @@ -203,18 +203,61 @@ func TestDescribeDBInstances_EchoesOmittedInertSettingsAsDisabled(t *testing.T) assert.Zero(t, aws.Int64Value(out.DBInstances[0].MonitoringInterval)) } -// A filter nobody implemented is refused rather than dropped: dropping it -// returns exactly the rows the caller asked to exclude. -func TestDescribeDBInstances_RejectsAnUnrecognizedFilter(t *testing.T) { +// AWS refuses a name it does not document for this call, and an ill-formed +// domain identifier, rather than dropping the filter. +func TestDescribeDBInstances_RejectsUnrecognizedFiltersAndBadDomains(t *testing.T) { t.Parallel() h := newCreateHarness(t, "") seedCreated(t, h, testDBInstanceID) - _, err := h.svc.DescribeDBInstances(t.Context(), &rds.DescribeDBInstancesInput{ - Filters: []*rds.Filter{{Name: aws.String("engine"), Values: aws.StringSlice([]string{"postgres"})}}, - }, testAccountID) - require.Error(t, err) - assert.Contains(t, err.Error(), awserrors.ErrorInvalidParameterValue) + for name, tc := range map[string]struct { + filter *rds.Filter + msg string + }{ + "unknown name": {&rds.Filter{Name: aws.String("snapshot-type"), Values: aws.StringSlice([]string{"manual"})}, + "Unrecognized filter name: snapshot-type"}, + "domain without prefix": {&rds.Filter{Name: aws.String(filterDomain), Values: aws.StringSlice([]string{"nope"})}, + "is not a valid Domain identifier"}, + "non-ASCII domain": {&rds.Filter{Name: aws.String(filterDomain), Values: aws.StringSlice([]string{"d-é"})}, + "is not a valid Domain identifier"}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + _, err := h.svc.DescribeDBInstances(t.Context(), &rds.DescribeDBInstancesInput{ + Filters: []*rds.Filter{tc.filter}, + }, testAccountID) + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorInvalidParameterValue) + assert.Contains(t, err.Error(), tc.msg) + }) + } +} + +// engine narrows like the identifiers do; no instance is in a cluster or a +// directory domain, so a well-formed value for either matches nothing. +func TestDescribeDBInstances_FiltersOnEngineClusterAndDomain(t *testing.T) { + t.Parallel() + h := newCreateHarness(t, "") + rec := seedCreated(t, h, testDBInstanceID) + + for name, tc := range map[string]struct { + filter *rds.Filter + want int + }{ + "own engine": {&rds.Filter{Name: aws.String(filterEngine), Values: aws.StringSlice([]string{"mariadb", rec.Engine})}, 1}, + "other engine": {&rds.Filter{Name: aws.String(filterEngine), Values: aws.StringSlice([]string{"mariadb"})}, 0}, + "cluster": {&rds.Filter{Name: aws.String(filterDBClusterID), Values: aws.StringSlice([]string{"any-cluster"})}, 0}, + "domain": {&rds.Filter{Name: aws.String(filterDomain), Values: aws.StringSlice([]string{"d-1234567890"})}, 0}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + out, err := h.svc.DescribeDBInstances(t.Context(), &rds.DescribeDBInstancesInput{ + Filters: []*rds.Filter{tc.filter}, + }, testAccountID) + require.NoError(t, err) + assert.Len(t, out.DBInstances, tc.want) + }) + } } // A named instance and a filter that excludes it disagree; reporting the diff --git a/spinifex/handlers/rds/events.go b/spinifex/handlers/rds/events.go index e43da2f6f..f406d1cfe 100644 --- a/spinifex/handlers/rds/events.go +++ b/spinifex/handlers/rds/events.go @@ -157,6 +157,11 @@ func trimEvents(events []Event) []Event { // to a resource, so an unfiltered call reports the whole account's recent // history, including resources that have since been deleted. func (s *Service) DescribeEvents(ctx context.Context, input *rds.DescribeEventsInput, accountID string) (*rds.DescribeEventsOutput, error) { + // AWS recognises these names. They are not applied, as recognised names are + // not on the other calls documented as not supporting Filters. + if _, err := ReadFilters(input.Filters, filterEventCategory, filterDBInstanceID); err != nil { + return nil, err + } window, err := eventWindow(input) if err != nil { return nil, err diff --git a/spinifex/handlers/rds/filters.go b/spinifex/handlers/rds/filters.go new file mode 100644 index 000000000..b85c51d87 --- /dev/null +++ b/spinifex/handlers/rds/filters.go @@ -0,0 +1,68 @@ +package handlers_rds + +import ( + "slices" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/rds" + "github.com/mulgadc/spinifex/spinifex/awserrors" +) + +// Filter names, as AWS spells them. Each Describe call recognises its own subset. +const ( + filterDBClusterID = "db-cluster-id" + filterDBInstanceID = "db-instance-id" + filterDBParameterGroupFamily = "db-parameter-group-family" + filterDBSnapshotID = "db-snapshot-id" + filterDataType = "data-type" + filterDbiResourceID = "dbi-resource-id" + filterDomain = "domain" + filterEngine = "engine" + filterEventCategory = "event-category" + filterParameterName = "parameter-name" + filterResourceID = "resource-id" + filterSnapshotType = "snapshot-type" + filterStatus = "status" +) + +// Filter is one validated Filters entry. +type Filter struct { + Name string + Values []string +} + +// ReadFilters validates Filters against the names one Describe call recognises, +// in AWS's order: an unknown name, compared case-sensitively, before an absent +// value list. The query parser reads an empty list as absent, so both answer +// the same. +func ReadFilters(filters []*rds.Filter, recognised ...string) ([]Filter, error) { + out := make([]Filter, 0, len(filters)) + for _, entry := range filters { + name := "null" + if entry != nil && entry.Name != nil { + name = *entry.Name + } + if entry == nil || entry.Name == nil || !slices.Contains(recognised, name) { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "Unrecognized filter name: %s", name) + } + if len(entry.Values) == 0 { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterCombination, + "The values list cannot be null for the filter %s.", name) + } + out = append(out, Filter{Name: name, Values: aws.StringValueSlice(entry.Values)}) + } + return out, nil +} + +// matchesFilters is AWS's rule: a row must match every filter, and matches one +// by carrying any of its values. field returns the row's value for a filter +// name; a name the row has no field for matches nothing. +func matchesFilters(filters []Filter, field func(name string) (string, bool)) bool { + for _, filter := range filters { + value, ok := field(filter.Name) + if !ok || !slices.Contains(filter.Values, value) { + return false + } + } + return true +} diff --git a/spinifex/handlers/rds/filters_test.go b/spinifex/handlers/rds/filters_test.go new file mode 100644 index 000000000..812cfa7b1 --- /dev/null +++ b/spinifex/handlers/rds/filters_test.go @@ -0,0 +1,155 @@ +//test:in-package — reuses the in-package create harness and event seeding, and +// the filter-name constants are unexported + +package handlers_rds + +import ( + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/rds" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func filter(name string, values ...string) []*rds.Filter { + return []*rds.Filter{{Name: aws.String(name), Values: aws.StringSlice(values)}} +} + +func TestReadFilters_AnswersAsAWSDoes(t *testing.T) { + cases := map[string]struct { + filters []*rds.Filter + code string + msg string + }{ + "unknown name": {filter("bogus", "x"), awserrors.ErrorInvalidParameterValue, "Unrecognized filter name: bogus"}, + "name case": {filter("Engine", "x"), awserrors.ErrorInvalidParameterValue, "Unrecognized filter name: Engine"}, + "missing name": {[]*rds.Filter{{Values: aws.StringSlice([]string{"x"})}}, awserrors.ErrorInvalidParameterValue, "Unrecognized filter name: null"}, + "nil entry": {[]*rds.Filter{nil}, awserrors.ErrorInvalidParameterValue, "Unrecognized filter name: null"}, + "unknown first": {[]*rds.Filter{{Name: aws.String("bogus")}}, awserrors.ErrorInvalidParameterValue, "Unrecognized filter name: bogus"}, + "no values": {[]*rds.Filter{{Name: aws.String("engine")}}, awserrors.ErrorInvalidParameterCombination, + "The values list cannot be null for the filter engine."}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + _, err := ReadFilters(tc.filters, filterEngine) + require.Error(t, err) + assert.Contains(t, err.Error(), tc.code) + assert.Contains(t, err.Error(), tc.msg) + }) + } + + got, err := ReadFilters(append(filter(filterEngine, "postgres"), filter(filterEngine, "mariadb")...), filterEngine) + require.NoError(t, err) + assert.Equal(t, []Filter{{filterEngine, []string{"postgres"}}, {filterEngine, []string{"mariadb"}}}, got) +} + +// AWS applies parameter-name, matching names in any case, and accepts data-type +// without narrowing by it. +func TestDescribeDBParameters_FiltersOnParameterName(t *testing.T) { + t.Parallel() + h := newCreateHarness(t, testBaseDomain) + _, err := h.svc.CreateDBParameterGroup(t.Context(), parameterGroupInput(testParameterGroup), testAccountID) + require.NoError(t, err) + describe := func(filters []*rds.Filter) (*rds.DescribeDBParametersOutput, error) { + return h.svc.DescribeDBParameters(t.Context(), &rds.DescribeDBParametersInput{ + DBParameterGroupName: aws.String(testParameterGroup), + Filters: filters, + MaxRecords: aws.Int64(100), + }, testAccountID) + } + + byName, err := describe(filter(filterParameterName, "nope", "WORK_MEM")) + require.NoError(t, err) + require.Len(t, byName.Parameters, 1) + assert.Equal(t, "work_mem", aws.StringValue(byName.Parameters[0].ParameterName)) + + all, err := describe(nil) + require.NoError(t, err) + none, err := describe(filter(filterParameterName, "nope")) + require.NoError(t, err) + assert.NotNil(t, none.Parameters, "AWS answers an empty list, not an absent one") + assert.Empty(t, none.Parameters) + + byType, err := describe(filter(filterDataType, "nope")) + require.NoError(t, err) + assert.Len(t, byType.Parameters, len(all.Parameters)) + + _, err = describe(filter("source", "user")) + require.Error(t, err) + assert.Contains(t, err.Error(), "Unrecognized filter name: source") +} + +// On these calls AWS refuses a name it does not recognise but narrows by none +// of the names it does. +func TestRecognisedFiltersThatDoNotNarrow(t *testing.T) { + t.Parallel() + h := newCreateHarness(t, "") + input := validCreateInput() + input.Tags = awsTags("env", "prod") + _, err := h.svc.CreateDBInstance(t.Context(), input, testAccountID) + require.NoError(t, err) + h.svc.RecordEvent(t.Context(), testAccountID, EventSourceTypeDBInstance, testDBInstanceID, + "DB instance stopped.", EventCategoryAvailability) + arn := FormatARN(ResourceKindDBInstance, testRegion, testAccountID, testDBInstanceID) + + groups := func(filters []*rds.Filter) (int, error) { + out, err := h.svc.DescribeDBParameterGroups(t.Context(), &rds.DescribeDBParameterGroupsInput{Filters: filters}, testAccountID) + if err != nil { + return 0, err + } + return len(out.DBParameterGroups), nil + } + tags := func(filters []*rds.Filter) (int, error) { + out, err := h.svc.ListTagsForResource(t.Context(), &rds.ListTagsForResourceInput{ResourceName: aws.String(arn), Filters: filters}, testAccountID) + if err != nil { + return 0, err + } + return len(out.TagList), nil + } + events := func(filters []*rds.Filter) (int, error) { + out, err := h.svc.DescribeEvents(t.Context(), &rds.DescribeEventsInput{Filters: filters}, testAccountID) + if err != nil { + return 0, err + } + return len(out.Events), nil + } + + for name, tc := range map[string]struct { + call func([]*rds.Filter) (int, error) + recognised [][]*rds.Filter + unknown string + }{ + "DescribeDBParameterGroups": {groups, + [][]*rds.Filter{filter(filterDBParameterGroupFamily, "mysql8.0"), filter(filterEngine, "mysql")}, "db-parameter-group-name"}, + "ListTagsForResource": {tags, [][]*rds.Filter{filter(filterResourceID, "nope")}, "tag-key"}, + "DescribeEvents": {events, + [][]*rds.Filter{filter(filterEventCategory, "backup"), filter(filterDBInstanceID, "other-db")}, "source-id"}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + want, err := tc.call(nil) + require.NoError(t, err) + require.NotZero(t, want) + for _, filters := range tc.recognised { + got, err := tc.call(filters) + require.NoError(t, err) + assert.Equal(t, want, got, aws.StringValue(filters[0].Name)) + } + _, err = tc.call(filter(tc.unknown, "x")) + require.Error(t, err) + assert.Contains(t, err.Error(), "Unrecognized filter name: "+tc.unknown) + }) + } +} + +// AWS ignores Filters here entirely, even an unknown or malformed entry. +func TestDescribeDBSubnetGroups_IgnoresFilters(t *testing.T) { + t.Parallel() + h := newCreateHarness(t, "") + _, err := h.svc.DescribeDBSubnetGroups(t.Context(), &rds.DescribeDBSubnetGroupsInput{ + Filters: []*rds.Filter{{Name: aws.String("bogus")}, {Values: aws.StringSlice([]string{"x"})}}, + }, testAccountID) + require.NoError(t, err) +} diff --git a/spinifex/handlers/rds/parametergroup.go b/spinifex/handlers/rds/parametergroup.go index 561597551..24a7bf51d 100644 --- a/spinifex/handlers/rds/parametergroup.go +++ b/spinifex/handlers/rds/parametergroup.go @@ -87,6 +87,10 @@ func (s *Service) DescribeDBParameterGroups(ctx context.Context, input *rds.Desc if input == nil { input = &rds.DescribeDBParameterGroupsInput{} } + // AWS recognises both names and narrows by neither. + if _, err := ReadFilters(input.Filters, filterDBParameterGroupFamily, filterEngine); err != nil { + return nil, err + } kv, err := s.bucket(ctx, accountID) if err != nil { return nil, err @@ -248,6 +252,11 @@ func (s *Service) DescribeDBParameters(ctx context.Context, input *rds.DescribeD return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "Source %q is not one of %s or %s", source, ParameterSourceUser, ParameterSourceEngineDefault) } + // AWS narrows by parameter-name, matching names in any case, and not by data-type. + filters, err := ReadFilters(input.Filters, filterParameterName, filterDataType) + if err != nil { + return nil, err + } kv, err := s.bucket(ctx, accountID) if err != nil { @@ -272,7 +281,8 @@ func (s *Service) DescribeDBParameters(ctx context.Context, input *rds.DescribeD return nil, err } - out := &rds.DescribeDBParametersOutput{} + // Non-nil, so a filter that matches nothing answers with an empty list, as AWS does. + out := &rds.DescribeDBParametersOutput{Parameters: []*rds.Parameter{}} for _, param := range engine.CatalogParameterNames() { spec, _ := engine.LookupParameter(param) override, isOverride := overrides[param] @@ -283,6 +293,9 @@ func (s *Service) DescribeDBParameters(ctx context.Context, input *rds.DescribeD if source != "" && source != parameterSource(isOverride) { continue } + if !parameterNameMatches(filters, param) { + continue + } out.Parameters = append(out.Parameters, projectParameter(spec, value, override.ApplyMethod, isOverride)) } out.Parameters, out.Marker, err = Page(out.Parameters, parameterPageKey, input.MaxRecords, input.Marker) @@ -292,6 +305,17 @@ func (s *Service) DescribeDBParameters(ctx context.Context, input *rds.DescribeD return out, nil } +func parameterNameMatches(filters []Filter, param string) bool { + for _, filter := range filters { + if filter.Name == filterParameterName && !slices.ContainsFunc(filter.Values, func(v string) bool { + return strings.EqualFold(v, param) + }) { + return false + } + } + return true +} + // Refused for a default group, and while any instance still references it — // including one that is only deleting, so a destroy that races the teardown // fails cleanly rather than leaving a live engine's configuration unreadable. diff --git a/spinifex/handlers/rds/snapshot.go b/spinifex/handlers/rds/snapshot.go index 0a831bbf2..2b285186f 100644 --- a/spinifex/handlers/rds/snapshot.go +++ b/spinifex/handlers/rds/snapshot.go @@ -222,7 +222,7 @@ func (s *Service) releaseQuiesce(ctx context.Context, accountID, dbInstanceIdent // empty list, matching AWS: a client polling a create would otherwise read // "gone" for "not ready". func (s *Service) DescribeDBSnapshots(ctx context.Context, input *rds.DescribeDBSnapshotsInput, accountID string) (*rds.DescribeDBSnapshotsOutput, error) { - snapshotType, err := validateDescribeSnapshotsRequest(input) + snapshotType, filters, err := validateDescribeSnapshotsRequest(input) if err != nil { return nil, err } @@ -236,7 +236,7 @@ func (s *Service) DescribeDBSnapshots(ctx context.Context, input *rds.DescribeDB if err != nil { return nil, err } - if !snapshotMatches(rec, aws.StringValue(input.DBInstanceIdentifier), snapshotType) { + if !snapshotMatches(rec, aws.StringValue(input.DBInstanceIdentifier), snapshotType, filters) { return &rds.DescribeDBSnapshotsOutput{DBSnapshots: []*rds.DBSnapshot{}}, nil } return &rds.DescribeDBSnapshotsOutput{DBSnapshots: []*rds.DBSnapshot{s.projectDBSnapshot(rec)}}, nil @@ -257,7 +257,7 @@ func (s *Service) DescribeDBSnapshots(ctx context.Context, input *rds.DescribeDB } // A snapshot deleted between the key listing and this read is simply gone, // which is what a describe one tick later would report too. - if !found || !snapshotMatches(&rec, aws.StringValue(input.DBInstanceIdentifier), snapshotType) { + if !found || !snapshotMatches(&rec, aws.StringValue(input.DBInstanceIdentifier), snapshotType, filters) { continue } snapshots = append(snapshots, s.projectDBSnapshot(&rec)) @@ -269,12 +269,27 @@ func (s *Service) DescribeDBSnapshots(ctx context.Context, input *rds.DescribeDB return &rds.DescribeDBSnapshotsOutput{DBSnapshots: snapshots, Marker: next}, nil } -// An empty filter matches everything, as AWS does. -func snapshotMatches(rec *DBSnapshotRecord, dbInstanceIdentifier, snapshotType string) bool { +// An empty parameter matches everything, as AWS does. +func snapshotMatches(rec *DBSnapshotRecord, dbInstanceIdentifier, snapshotType string, filters []Filter) bool { if dbInstanceIdentifier != "" && rec.DBInstanceIdentifier != dbInstanceIdentifier { return false } - return snapshotType == "" || rec.SnapshotType == snapshotType + if snapshotType != "" && rec.SnapshotType != snapshotType { + return false + } + return matchesFilters(filters, func(name string) (string, bool) { + switch name { + case filterDBInstanceID: + return rec.DBInstanceIdentifier, true + case filterDBSnapshotID: + return rec.DBSnapshotIdentifier, true + case filterSnapshotType: + return rec.SnapshotType, true + case filterEngine: + return rec.Engine, true + } + return "", false + }) } // Removes the snapshot and, when it was the last thing holding a data volume @@ -485,31 +500,53 @@ func validateCreateSnapshotRequest(input *rds.CreateDBSnapshotInput) (*validated }, nil } -// Returns the snapshot type to filter on, empty for "any". A filter this -// phase cannot honour is rejected rather than dropped, because a silently -// unfiltered list reads as a complete answer. -func validateDescribeSnapshotsRequest(input *rds.DescribeDBSnapshotsInput) (string, error) { +// Returns the snapshot type to filter on, empty for "any", and the Filters. A +// scope this phase cannot honour is rejected rather than dropped, because a +// silently unfiltered list reads as a complete answer. +func validateDescribeSnapshotsRequest(input *rds.DescribeDBSnapshotsInput) (string, []Filter, error) { if input == nil { - return "", nil + return "", nil, nil } - if len(input.Filters) > 0 { - return "", unimplemented("Filters", "DescribeDBSnapshots filters on DBSnapshotIdentifier, DBInstanceIdentifier and SnapshotType only") + filters, err := ReadFilters(input.Filters, + filterDBInstanceID, filterDBSnapshotID, filterDbiResourceID, filterSnapshotType, filterEngine) + if err != nil { + return "", nil, err + } + for _, filter := range filters { + switch filter.Name { + case filterDbiResourceID: + return "", nil, unimplemented("Filter dbi-resource-id", snapshotNoResourceID) + case filterSnapshotType: + // AWS's whole vocabulary is accepted; the types never offered here match nothing. + for _, value := range filter.Values { + if !slices.Contains(awsSnapshotTypes, value) { + return "", nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "Invalid snapshot type. If specified, must be one of: public, shared, manual, awsbackup, automated.") + } + } + } } if aws.StringValue(input.DbiResourceId) != "" { - return "", unimplemented("DbiResourceId", "a DB instance has no resource ID distinct from its identifier here") + return "", nil, unimplemented("DbiResourceId", snapshotNoResourceID) } if aws.BoolValue(input.IncludeShared) || aws.BoolValue(input.IncludePublic) { - return "", unimplemented("IncludeShared/IncludePublic", "cross-account snapshot sharing is not offered") + return "", nil, unimplemented("IncludeShared/IncludePublic", "cross-account snapshot sharing is not offered") } switch snapshotType := aws.StringValue(input.SnapshotType); snapshotType { case "", SnapshotTypeManual, SnapshotTypeAutomated: - return snapshotType, nil + return snapshotType, filters, nil default: - return "", awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + return "", nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "SnapshotType %q is not offered; use %q or %q", snapshotType, SnapshotTypeManual, SnapshotTypeAutomated) } } +// Why DbiResourceId and the dbi-resource-id filter are refused: an instance has +// one, but a snapshot does not record it. +const snapshotNoResourceID = "a snapshot does not record its source instance's resource ID" + +var awsSnapshotTypes = []string{"public", "shared", SnapshotTypeManual, "awsbackup", SnapshotTypeAutomated} + // A name the caller is minting: a customer snapshot, or a final snapshot at // delete. The rds: namespace belongs to automated backups, so it is refused // wherever a name is created — and accepted by validateDBSnapshotReference, which diff --git a/spinifex/handlers/rds/snapshot_test.go b/spinifex/handlers/rds/snapshot_test.go index 209c59117..806422a67 100644 --- a/spinifex/handlers/rds/snapshot_test.go +++ b/spinifex/handlers/rds/snapshot_test.go @@ -341,6 +341,34 @@ func TestDescribeDBSnapshots_FiltersByInstanceAndType(t *testing.T) { }, testAccountID) require.NoError(t, err) assert.Empty(t, byType.DBSnapshots, "nothing in this phase is automated") + + for name, tc := range map[string]struct { + filters []*rds.Filter + want []string + }{ + "instance": {[]*rds.Filter{{Name: aws.String("db-instance-id"), Values: aws.StringSlice([]string{"reports-db"})}}, + []string{"reports-db-daily"}}, + "snapshot": {[]*rds.Filter{{Name: aws.String("db-snapshot-id"), Values: aws.StringSlice([]string{testSnapshotID, "nope"})}}, + []string{testSnapshotID}}, + "type": {[]*rds.Filter{{Name: aws.String("snapshot-type"), Values: aws.StringSlice([]string{"shared", SnapshotTypeManual})}}, + []string{testSnapshotID, "reports-db-daily"}}, + "engine": {[]*rds.Filter{{Name: aws.String("engine"), Values: aws.StringSlice([]string{"mariadb"})}}, nil}, + "every filter must match": {[]*rds.Filter{ + {Name: aws.String("snapshot-type"), Values: aws.StringSlice([]string{SnapshotTypeManual})}, + {Name: aws.String("db-instance-id"), Values: aws.StringSlice([]string{testDBID})}, + }, []string{testSnapshotID}}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + out, err := h.svc.DescribeDBSnapshots(t.Context(), &rds.DescribeDBSnapshotsInput{Filters: tc.filters}, testAccountID) + require.NoError(t, err) + var got []string + for _, snap := range out.DBSnapshots { + got = append(got, aws.StringValue(snap.DBSnapshotIdentifier)) + } + assert.ElementsMatch(t, tc.want, got) + }) + } } // A client polling a create would otherwise read "gone" for "not ready". @@ -361,10 +389,12 @@ func TestDescribeDBSnapshots_RejectsUnhonouredScoping(t *testing.T) { h := newSnapshotHarness(t, false) cases := map[string]*rds.DescribeDBSnapshotsInput{ - "Filters": {Filters: []*rds.Filter{{Name: aws.String("engine")}}}, - "DbiResourceId": {DbiResourceId: aws.String("db-ABC123")}, - "IncludeShared": {IncludeShared: aws.Bool(true)}, - "SnapshotType": {SnapshotType: aws.String("shared")}, + "dbi-resource-id filter": {Filters: filter("dbi-resource-id", "db-ABC123")}, + "unknown filter": {Filters: filter("status", "available")}, + "bad snapshot-type": {Filters: filter("snapshot-type", "bogus")}, + "DbiResourceId": {DbiResourceId: aws.String("db-ABC123")}, + "IncludeShared": {IncludeShared: aws.Bool(true)}, + "SnapshotType": {SnapshotType: aws.String("shared")}, } for name, input := range cases { t.Run(name, func(t *testing.T) { diff --git a/spinifex/handlers/rds/subnetgroup.go b/spinifex/handlers/rds/subnetgroup.go index f05d6df63..a8b49a277 100644 --- a/spinifex/handlers/rds/subnetgroup.go +++ b/spinifex/handlers/rds/subnetgroup.go @@ -89,7 +89,8 @@ func (s *Service) CreateDBSubnetGroup(ctx context.Context, input *rds.CreateDBSu } // A named group that does not exist is an error, matching AWS; an unnamed -// request lists the account's groups. +// request lists the account's groups. Filters is not read: AWS ignores it here, +// even an unknown or malformed entry. func (s *Service) DescribeDBSubnetGroups(ctx context.Context, input *rds.DescribeDBSubnetGroupsInput, accountID string) (*rds.DescribeDBSubnetGroupsOutput, error) { if input == nil { input = &rds.DescribeDBSubnetGroupsInput{} diff --git a/spinifex/handlers/rds/tags.go b/spinifex/handlers/rds/tags.go index 869da4de6..13ad9dbb5 100644 --- a/spinifex/handlers/rds/tags.go +++ b/spinifex/handlers/rds/tags.go @@ -75,6 +75,10 @@ func (s *Service) ListTagsForResource(ctx context.Context, input *rds.ListTagsFo if input == nil { return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "empty request") } + // AWS recognises the name and does not narrow by it. + if _, err := ReadFilters(input.Filters, filterResourceID); err != nil { + return nil, err + } resource, parsed, err := s.resolveTaggable(aws.StringValue(input.ResourceName), accountID) if err != nil { return nil, err