fix: RDS Describe Filters and DbiResourceId refusals as AWS answers them (#1136)

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
 

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 |

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, ", "))
-}

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)
 		})
 	}
 }

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.

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")},
 	}

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

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

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

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
+}

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)
+}

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.

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

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) {

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{}

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