From 710169e5a662eef7346b80d28c702c80de20c2db Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Wed, 5 Aug 2026 10:34:51 +1000 Subject: [PATCH] fix: eliminate RDS Terraform drift (#729) --- spinifex/handlers/rds/arn.go | 12 ++++-- spinifex/handlers/rds/arn_parse_test.go | 17 ++++++--- spinifex/handlers/rds/create.go | 5 ++- spinifex/handlers/rds/create_test.go | 9 ++++- spinifex/handlers/rds/describe.go | 12 ++++-- spinifex/handlers/rds/describe_test.go | 40 ++++++++++++++++++++ spinifex/handlers/rds/engine.go | 11 ++---- spinifex/handlers/rds/engine_test.go | 4 +- spinifex/handlers/rds/modify.go | 30 ++++++++++++++- spinifex/handlers/rds/modify_test.go | 15 ++++++++ spinifex/handlers/rds/parametergroup.go | 21 ++++++---- spinifex/handlers/rds/parametergroup_test.go | 22 ++++++++++- spinifex/handlers/rds/records.go | 11 +++--- spinifex/handlers/rds/restore.go | 1 + spinifex/handlers/rds/restore_test.go | 5 ++- spinifex/handlers/rds/store.go | 6 +-- spinifex/handlers/rds/tags_test.go | 19 ++++++++++ spinifex/handlers/rds/validate.go | 18 +++++++-- tests/e2e/rds/api_matrix_test.go | 32 ++++++++++------ 19 files changed, 233 insertions(+), 57 deletions(-) diff --git a/spinifex/handlers/rds/arn.go b/spinifex/handlers/rds/arn.go index 3a16b1282..61e9fbb77 100644 --- a/spinifex/handlers/rds/arn.go +++ b/spinifex/handlers/rds/arn.go @@ -70,8 +70,8 @@ const arnSegmentCount = 7 // Region and account are checked here rather than at policy evaluation, so a // foreign-account reference never reaches the evaluator at all. func ParseARN(arn, region, accountID string) (ParsedARN, error) { - // The identifier cannot contain a colon, so the trailing segment SplitN - // leaves intact is rejected below if it holds one. + // SplitN leaves an automated snapshot's rds: prefix in the identifier. Other + // resource kinds still reject that extra separator below. parts := strings.SplitN(arn, ":", arnSegmentCount) if len(parts) != arnSegmentCount { return ParsedARN{}, arnError(arn, "expected the form arn:aws:rds:{region}:{account}:{type}:{name}") @@ -91,7 +91,13 @@ func ParseARN(arn, region, accountID string) (ParsedARN, error) { return ParsedARN{}, arnError(arn, fmt.Sprintf("%q is not an RDS resource type", parts[5])) } identifier := parts[6] - if identifier == "" || strings.ContainsAny(identifier, ":/") { + malformed := identifier == "" || strings.Contains(identifier, "/") + if kind == ResourceKindDBSnapshot { + malformed = malformed || validateDBSnapshotReference(identifier) != nil + } else { + malformed = malformed || strings.Contains(identifier, ":") + } + if malformed { return ParsedARN{}, arnError(arn, "the resource name is empty or malformed") } diff --git a/spinifex/handlers/rds/arn_parse_test.go b/spinifex/handlers/rds/arn_parse_test.go index 2e22f21dd..da14addf7 100644 --- a/spinifex/handlers/rds/arn_parse_test.go +++ b/spinifex/handlers/rds/arn_parse_test.go @@ -2,6 +2,7 @@ package handlers_rds import ( "testing" + "time" "github.com/mulgadc/spinifex/spinifex/awserrors" "github.com/stretchr/testify/assert" @@ -11,19 +12,22 @@ import ( // Every ARN the builders mint must parse back into the parts it was built // from, so a resource named by a describe is addressable by a tag action. func TestParseARN_RoundTripsEveryResourceType(t *testing.T) { + automated := AutomatedSnapshotIdentifier("orders-db", time.Date(2026, 7, 24, 3, 4, 0, 0, time.UTC)) cases := []struct { + name string kind ResourceKind identifier string arn string }{ - {ResourceKindDBInstance, "orders-db", DBInstanceARN(testRegion, testAccountID, "orders-db")}, - {ResourceKindDBSnapshot, "orders-db-2026-07-24", DBSnapshotARN(testRegion, testAccountID, "orders-db-2026-07-24")}, - {ResourceKindDBSubnetGroup, "prod-db-subnets", DBSubnetGroupARN(testRegion, testAccountID, "prod-db-subnets")}, - {ResourceKindDBParameterGroup, "postgres16-tuned", DBParameterGroupARN(testRegion, testAccountID, "postgres16-tuned")}, + {"instance", ResourceKindDBInstance, "orders-db", DBInstanceARN(testRegion, testAccountID, "orders-db")}, + {"manual snapshot", ResourceKindDBSnapshot, "orders-db-2026-07-24", DBSnapshotARN(testRegion, testAccountID, "orders-db-2026-07-24")}, + {"automated snapshot", ResourceKindDBSnapshot, automated, DBSnapshotARN(testRegion, testAccountID, automated)}, + {"subnet group", ResourceKindDBSubnetGroup, "prod-db-subnets", DBSubnetGroupARN(testRegion, testAccountID, "prod-db-subnets")}, + {"parameter group", ResourceKindDBParameterGroup, "postgres16-tuned", DBParameterGroupARN(testRegion, testAccountID, "postgres16-tuned")}, } for _, tc := range cases { - t.Run(string(tc.kind), func(t *testing.T) { + t.Run(tc.name, func(t *testing.T) { parsed, err := ParseARN(tc.arn, testRegion, testAccountID) require.NoError(t, err) assert.Equal(t, tc.kind, parsed.Kind) @@ -44,6 +48,9 @@ func TestParseARN_RejectsMalformedAndForeignARNs(t *testing.T) { {"slash delimited", "arn:aws:rds:" + testRegion + ":" + testAccountID + ":db/orders-db"}, {"truncated", "arn:aws:rds:" + testRegion + ":" + testAccountID + ":db"}, {"extra segment", DBInstanceARN(testRegion, testAccountID, "orders-db") + ":extra"}, + {"snapshot with a foreign namespace", DBSnapshotARN(testRegion, testAccountID, "other:orders-db")}, + {"snapshot with two embedded colons", DBSnapshotARN(testRegion, testAccountID, "rds:orders-db:extra")}, + {"snapshot with a slash", DBSnapshotARN(testRegion, testAccountID, "rds:orders/db")}, {"empty identifier", "arn:aws:rds:" + testRegion + ":" + testAccountID + ":db:"}, {"another service", "arn:aws:ec2:" + testRegion + ":" + testAccountID + ":db:orders-db"}, {"another partition", "arn:aws-cn:rds:" + testRegion + ":" + testAccountID + ":db:orders-db"}, diff --git a/spinifex/handlers/rds/create.go b/spinifex/handlers/rds/create.go index b1372740a..79b278433 100644 --- a/spinifex/handlers/rds/create.go +++ b/spinifex/handlers/rds/create.go @@ -158,7 +158,10 @@ func newDBInstanceRecord(accountID string, req *validatedCreate, placement *endp DBParameterGroupName: req.DBParameterGroupName, DeletionProtection: req.DeletionProtection, - AutoMinorVersionUpgrade: req.AutoMinorVersionUpgrade, + AutoMinorVersionUpgrade: req.AutoMinorVersionUpgrade, + CopyTagsToSnapshot: req.CopyTagsToSnapshot, + MonitoringInterval: req.MonitoringInterval, + EnablePerformanceInsights: req.EnablePerformanceInsights, BackupRetentionPeriod: req.BackupRetentionPeriod, PreferredBackupWindow: req.PreferredBackupWindow, diff --git a/spinifex/handlers/rds/create_test.go b/spinifex/handlers/rds/create_test.go index 07d25ed43..48cf3a75e 100644 --- a/spinifex/handlers/rds/create_test.go +++ b/spinifex/handlers/rds/create_test.go @@ -520,6 +520,8 @@ func TestValidateCreateRequest_RejectsUnimplementedParameters(t *testing.T) { {"StorageEncryptedFalse", func(in *rds.CreateDBInstanceInput) { in.StorageEncrypted = aws.Bool(false) }, "StorageEncrypted"}, {"IAMDatabaseAuth", func(in *rds.CreateDBInstanceInput) { in.EnableIAMDatabaseAuthentication = aws.Bool(true) }, "EnableIAMDatabaseAuthentication"}, {"Iops", func(in *rds.CreateDBInstanceInput) { in.Iops = aws.Int64(3000) }, "Iops"}, + {"MaxAllocatedStorage", func(in *rds.CreateDBInstanceInput) { in.MaxAllocatedStorage = aws.Int64(500) }, "MaxAllocatedStorage"}, + {"StorageThroughput", func(in *rds.CreateDBInstanceInput) { in.StorageThroughput = aws.Int64(250) }, "StorageThroughput"}, {"KmsKeyId", func(in *rds.CreateDBInstanceInput) { in.KmsKeyId = aws.String("arn:aws:kms:::key/abc") }, "KmsKeyId"}, {"AvailabilityZone", func(in *rds.CreateDBInstanceInput) { in.AvailabilityZone = aws.String("ap-southeast-2b") }, "AvailabilityZone"}, {"DBSecurityGroups", func(in *rds.CreateDBInstanceInput) { in.DBSecurityGroups = aws.StringSlice([]string{"classic"}) }, "DBSecurityGroups"}, @@ -567,6 +569,7 @@ func TestValidateCreateRequest_RejectsMalformedRequests(t *testing.T) { in.DBInstanceIdentifier = aws.String(strings.Repeat("a", maxDBInstanceIdentifierLen+1)) }, awserrors.ErrorInvalidParameterValue}, {"UnknownEngine", func(in *rds.CreateDBInstanceInput) { in.Engine = aws.String("mysql") }, awserrors.ErrorInvalidParameterValue}, + {"MinorEngineVersion", func(in *rds.CreateDBInstanceInput) { in.EngineVersion = aws.String("18.4") }, awserrors.ErrorInvalidParameterValue}, {"UnknownClass", func(in *rds.CreateDBInstanceInput) { in.DBInstanceClass = aws.String("db.x99.mega") }, awserrors.ErrorInvalidParameterValue}, {"StorageBelowMinimum", func(in *rds.CreateDBInstanceInput) { in.AllocatedStorage = aws.Int64(minAllocatedStorageGiB - 1) @@ -614,6 +617,10 @@ func TestValidateCreateRequest_AcceptsInertParameters(t *testing.T) { input.MonitoringInterval = aws.Int64(60) input.StorageEncrypted = aws.Bool(true) - _, err := newCreateValidator().validateCreateRequest(input) + req, err := newCreateValidator().validateCreateRequest(input) require.NoError(t, err) + assert.True(t, req.AutoMinorVersionUpgrade) + assert.True(t, req.CopyTagsToSnapshot) + assert.True(t, req.EnablePerformanceInsights) + assert.Equal(t, int64(60), req.MonitoringInterval) } diff --git a/spinifex/handlers/rds/describe.go b/spinifex/handlers/rds/describe.go index 28acc25e2..2236659f8 100644 --- a/spinifex/handlers/rds/describe.go +++ b/spinifex/handlers/rds/describe.go @@ -145,13 +145,17 @@ func (s *Service) projectDBInstance(rec *DBInstanceRecord) *rds.DBInstance { StorageType: aws.String(rec.StorageType), StorageEncrypted: aws.Bool(rec.StorageEncrypted), MasterUsername: aws.String(rec.MasterUsername), + DbInstancePort: aws.Int64(rec.Port), MultiAZ: aws.Bool(false), PubliclyAccessible: aws.Bool(false), DeletionProtection: aws.Bool(rec.DeletionProtection), - // Echoed, not acted on: nothing upgrades a pinned version, but a client - // that set it has to read back what it set. - AutoMinorVersionUpgrade: aws.Bool(rec.AutoMinorVersionUpgrade), - InstanceCreateTime: aws.Time(rec.CreatedAt), + // Accepted inert settings are echoed so a client reads back what it set + // instead of planning a change no modify can deliver. + AutoMinorVersionUpgrade: aws.Bool(rec.AutoMinorVersionUpgrade), + CopyTagsToSnapshot: aws.Bool(rec.CopyTagsToSnapshot), + MonitoringInterval: aws.Int64(rec.MonitoringInterval), + PerformanceInsightsEnabled: aws.Bool(rec.EnablePerformanceInsights), + InstanceCreateTime: aws.Time(rec.CreatedAt), // The Terraform provider reads tags from the describe as well as from // ListTagsForResource, so the two have to agree. TagList: tagsToAWS(rec.Tags), diff --git a/spinifex/handlers/rds/describe_test.go b/spinifex/handlers/rds/describe_test.go index 45131980d..7d1236b4d 100644 --- a/spinifex/handlers/rds/describe_test.go +++ b/spinifex/handlers/rds/describe_test.go @@ -156,6 +156,45 @@ func TestDescribeDBInstances_EchoesAutoMinorVersionUpgrade(t *testing.T) { assert.False(t, aws.BoolValue(out.DBInstances[0].AutoMinorVersionUpgrade)) } +func TestDescribeDBInstances_EchoesAcceptedInertSettings(t *testing.T) { + h := newCreateHarness(t, "") + input := validCreateInput() + input.CopyTagsToSnapshot = aws.Bool(true) + input.EnablePerformanceInsights = aws.Bool(true) + input.MonitoringInterval = aws.Int64(60) + + _, err := h.svc.CreateDBInstance(t.Context(), input, testAccountID) + require.NoError(t, err) + + rec := h.record(t, testDBInstanceID) + assert.True(t, rec.CopyTagsToSnapshot) + assert.True(t, rec.EnablePerformanceInsights) + assert.Equal(t, int64(60), rec.MonitoringInterval) + + out, err := h.svc.DescribeDBInstances(t.Context(), &rds.DescribeDBInstancesInput{ + DBInstanceIdentifier: aws.String(testDBInstanceID), + }, testAccountID) + require.NoError(t, err) + require.Len(t, out.DBInstances, 1) + assert.True(t, aws.BoolValue(out.DBInstances[0].CopyTagsToSnapshot)) + assert.True(t, aws.BoolValue(out.DBInstances[0].PerformanceInsightsEnabled)) + assert.Equal(t, int64(60), aws.Int64Value(out.DBInstances[0].MonitoringInterval)) +} + +func TestDescribeDBInstances_EchoesOmittedInertSettingsAsDisabled(t *testing.T) { + h := newCreateHarness(t, "") + seedCreated(t, h, testDBInstanceID) + + out, err := h.svc.DescribeDBInstances(t.Context(), &rds.DescribeDBInstancesInput{ + DBInstanceIdentifier: aws.String(testDBInstanceID), + }, testAccountID) + require.NoError(t, err) + require.Len(t, out.DBInstances, 1) + assert.False(t, aws.BoolValue(out.DBInstances[0].CopyTagsToSnapshot)) + assert.False(t, aws.BoolValue(out.DBInstances[0].PerformanceInsightsEnabled)) + 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) { @@ -208,6 +247,7 @@ func TestProjectDBInstance_OmitsAnUnsettledEndpoint(t *testing.T) { Port: 5432, }) assert.Nil(t, instance.Endpoint) + assert.Equal(t, int64(5432), aws.Int64Value(instance.DbInstancePort)) assert.Nil(t, instance.DBName, "a request that named no database has none, not an empty one") assert.Nil(t, h.svc.projectDBInstance(nil)) } diff --git a/spinifex/handlers/rds/engine.go b/spinifex/handlers/rds/engine.go index 479402f2a..aea3ef814 100644 --- a/spinifex/handlers/rds/engine.go +++ b/spinifex/handlers/rds/engine.go @@ -62,15 +62,10 @@ func SupportedEngines() []string { return slices.Sorted(maps.Keys(engines)) } -// An empty version takes the pin. A supplied one must name the pinned major, -// with or without a minor, since a mismatch would silently run a version the -// customer did not ask for. +// An empty version takes the pin. A supplied one must name the pinned major +// exactly, since the image does not promise any particular minor version. func (e Engine) ValidateVersion(version string) error { - v := strings.TrimSpace(version) - if v == "" || v == e.MajorVersion { - return nil - } - if major, _, ok := strings.Cut(v, "."); ok && major == e.MajorVersion { + if version == "" || version == e.MajorVersion { return nil } return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, diff --git a/spinifex/handlers/rds/engine_test.go b/spinifex/handlers/rds/engine_test.go index c2490184a..48cf73f1b 100644 --- a/spinifex/handlers/rds/engine_test.go +++ b/spinifex/handlers/rds/engine_test.go @@ -31,10 +31,10 @@ func TestEngineValidateVersion(t *testing.T) { engine, err := LookupEngine("postgres") require.NoError(t, err) - for _, version := range []string{"", "18", "18.1", "18.4"} { + for _, version := range []string{"", "18"} { assert.NoError(t, engine.ValidateVersion(version), "version %q", version) } - for _, version := range []string{"17", "16.2", "19", "latest"} { + for _, version := range []string{"17", "16.2", "18.1", "18.4", " 18 ", "19", "latest"} { assert.Error(t, engine.ValidateVersion(version), "version %q", version) } diff --git a/spinifex/handlers/rds/modify.go b/spinifex/handlers/rds/modify.go index 7f92bbf38..05b602932 100644 --- a/spinifex/handlers/rds/modify.go +++ b/spinifex/handlers/rds/modify.go @@ -29,6 +29,9 @@ type modifyPlan struct { SecurityGroupIDs []string DeletionProtection *bool AutoMinorVersionUpgrade *bool + CopyTagsToSnapshot *bool + MonitoringInterval *int64 + EnablePerformanceInsights *bool BackupRetentionPeriod *int64 PreferredBackupWindow string PreferredMaintenanceWindow string @@ -54,7 +57,8 @@ func (p *modifyPlan) disruptive() bool { // deferred must not report a configuration change that has not happened. func (p *modifyPlan) immediate() bool { return p.MasterUserPassword != "" || p.SecurityGroupIDs != nil || p.DeletionProtection != nil || - p.AutoMinorVersionUpgrade != nil || p.BackupRetentionPeriod != nil || + p.AutoMinorVersionUpgrade != nil || p.CopyTagsToSnapshot != nil || + p.MonitoringInterval != nil || p.EnablePerformanceInsights != nil || p.BackupRetentionPeriod != nil || p.PreferredBackupWindow != "" || p.PreferredMaintenanceWindow != "" } @@ -139,7 +143,11 @@ func (s *Service) ModifyDBInstance(ctx context.Context, input *rds.ModifyDBInsta s.RecordEvent(ctx, accountID, EventSourceTypeDBInstance, id, "Modification recorded; it will be applied during the next maintenance window, and the database will be unavailable while it is.", EventCategoryConfigurationChange, EventCategoryNotification) - return &rds.ModifyDBInstanceOutput{DBInstance: s.projectDBInstance(rec)}, nil + stored, _, err := s.getDBInstance(ctx, kv, id) + if err != nil { + return nil, err + } + return &rds.ModifyDBInstanceOutput{DBInstance: s.projectDBInstance(stored)}, nil } moved, kv, err := s.beginTransition(ctx, accountID, id, StatusModifying, StatusAvailable, StatusFailed) @@ -224,6 +232,15 @@ func (s *Service) planModify(ctx context.Context, input *rds.ModifyDBInstanceInp if input.AutoMinorVersionUpgrade != nil && aws.BoolValue(input.AutoMinorVersionUpgrade) != rec.AutoMinorVersionUpgrade { plan.AutoMinorVersionUpgrade = input.AutoMinorVersionUpgrade } + if input.CopyTagsToSnapshot != nil && aws.BoolValue(input.CopyTagsToSnapshot) != rec.CopyTagsToSnapshot { + plan.CopyTagsToSnapshot = input.CopyTagsToSnapshot + } + if input.MonitoringInterval != nil && aws.Int64Value(input.MonitoringInterval) != rec.MonitoringInterval { + plan.MonitoringInterval = input.MonitoringInterval + } + if input.EnablePerformanceInsights != nil && aws.BoolValue(input.EnablePerformanceInsights) != rec.EnablePerformanceInsights { + plan.EnablePerformanceInsights = input.EnablePerformanceInsights + } if err := s.planBackupSettings(input, rec, plan); err != nil { return nil, err } @@ -338,6 +355,15 @@ func (s *Service) applyImmediateModify(ctx context.Context, kv jetstream.KeyValu if plan.AutoMinorVersionUpgrade != nil { stored.AutoMinorVersionUpgrade = *plan.AutoMinorVersionUpgrade } + if plan.CopyTagsToSnapshot != nil { + stored.CopyTagsToSnapshot = *plan.CopyTagsToSnapshot + } + if plan.MonitoringInterval != nil { + stored.MonitoringInterval = *plan.MonitoringInterval + } + if plan.EnablePerformanceInsights != nil { + stored.EnablePerformanceInsights = *plan.EnablePerformanceInsights + } if plan.BackupRetentionPeriod != nil { stored.BackupRetentionPeriod = *plan.BackupRetentionPeriod } diff --git a/spinifex/handlers/rds/modify_test.go b/spinifex/handlers/rds/modify_test.go index 8bae0d6a1..1f6ec88c7 100644 --- a/spinifex/handlers/rds/modify_test.go +++ b/spinifex/handlers/rds/modify_test.go @@ -120,6 +120,10 @@ func TestModifyDBInstance_AppliesTheNonDisruptiveSettingsAtOnce(t *testing.T) { in := modifyInput() in.MasterUserPassword = aws.String("N3w-Sup3rSecret!") in.DeletionProtection = aws.Bool(true) + in.AutoMinorVersionUpgrade = aws.Bool(true) + in.CopyTagsToSnapshot = aws.Bool(true) + in.MonitoringInterval = aws.Int64(60) + in.EnablePerformanceInsights = aws.Bool(true) in.BackupRetentionPeriod = aws.Int64(7) in.PreferredBackupWindow = aws.String("03:00-04:00") in.PreferredMaintenanceWindow = aws.String("sun:05:00-sun:06:00") @@ -138,11 +142,18 @@ func TestModifyDBInstance_AppliesTheNonDisruptiveSettingsAtOnce(t *testing.T) { rec := h.record(t) assert.True(t, rec.DeletionProtection) + assert.True(t, rec.AutoMinorVersionUpgrade) + assert.True(t, rec.CopyTagsToSnapshot) + assert.Equal(t, int64(60), rec.MonitoringInterval) + assert.True(t, rec.EnablePerformanceInsights) assert.Equal(t, int64(7), rec.BackupRetentionPeriod) assert.Equal(t, "03:00-04:00", rec.PreferredBackupWindow) assert.Equal(t, "sun:05:00-sun:06:00", rec.PreferredMaintenanceWindow) assert.NotNil(t, rec.MasterPasswordUpdatedAt) assert.Nil(t, rec.PendingModifiedValues) + assert.True(t, aws.BoolValue(out.DBInstance.CopyTagsToSnapshot)) + assert.Equal(t, int64(60), aws.Int64Value(out.DBInstance.MonitoringInterval)) + assert.True(t, aws.BoolValue(out.DBInstance.PerformanceInsightsEnabled)) } // Only the fact of the rotation is recorded. A password that reached KV @@ -406,15 +417,19 @@ func TestModifyDBInstance_DefersADisruptiveChangeToTheMaintenanceWindow(t *testi in := modifyInput() in.AllocatedStorage = aws.Int64(50) in.DBInstanceClass = aws.String("db.m5.large") + in.CopyTagsToSnapshot = aws.Bool(true) out, err := h.svc.ModifyDBInstance(t.Context(), in, testAccountID) require.NoError(t, err) assert.Equal(t, string(StatusAvailable), aws.StringValue(out.DBInstance.DBInstanceStatus)) + assert.True(t, aws.BoolValue(out.DBInstance.CopyTagsToSnapshot), + "the response must include immediate fields applied alongside a deferred change") assert.Empty(t, h.cmdr.calls) assert.Empty(t, h.storage.modified) assert.Empty(t, h.launch.launcher.terminated) rec := h.record(t) + assert.True(t, rec.CopyTagsToSnapshot) require.NotNil(t, rec.PendingModifiedValues) assert.Equal(t, int64(50), aws.Int64Value(rec.PendingModifiedValues.AllocatedStorage)) assert.Equal(t, "db.m5.large", rec.PendingModifiedValues.DBInstanceClass) diff --git a/spinifex/handlers/rds/parametergroup.go b/spinifex/handlers/rds/parametergroup.go index 5e44b218e..b40509d2a 100644 --- a/spinifex/handlers/rds/parametergroup.go +++ b/spinifex/handlers/rds/parametergroup.go @@ -222,14 +222,15 @@ func (s *Service) DescribeDBParameters(ctx context.Context, input *rds.DescribeD out := &rds.DescribeDBParametersOutput{} for _, param := range CatalogParameterNames() { spec, _ := LookupParameter(param) - value, isOverride := overrides[param] + override, isOverride := overrides[param] + value := override.Value if !isOverride { value = spec.DefaultAt(memoryMiB) } if source != "" && source != parameterSource(isOverride) { continue } - out.Parameters = append(out.Parameters, projectParameter(spec, value, isOverride)) + out.Parameters = append(out.Parameters, projectParameter(spec, value, override.ApplyMethod, isOverride)) } return out, nil } @@ -421,7 +422,7 @@ func parameterSource(isOverride bool) string { // A computed default is reported as engine-default with its literal value, never // as the formula that produced it. -func projectParameter(spec ParameterSpec, value string, isOverride bool) *rds.Parameter { +func projectParameter(spec ParameterSpec, value, storedApplyMethod string, isOverride bool) *rds.Parameter { out := &rds.Parameter{ ParameterName: aws.String(spec.Name), ParameterValue: aws.String(value), @@ -434,9 +435,11 @@ func projectParameter(spec ParameterSpec, value string, isOverride bool) *rds.Pa if allowed := spec.AllowedValues(); allowed != "" { out.AllowedValues = aws.String(allowed) } - // AWS reports the method a change to this parameter would take, which for a - // static setting is the only one it can take. - if spec.ApplyType == ApplyTypeStatic { + // An override reports the method the customer stored. The fallback preserves + // defaults and records written before ApplyMethod was persisted. + if storedApplyMethod != "" { + out.ApplyMethod = aws.String(storedApplyMethod) + } else if spec.ApplyType == ApplyTypeStatic { out.ApplyMethod = aws.String(ApplyMethodPendingReboot) } else { out.ApplyMethod = aws.String(ApplyMethodImmediate) @@ -468,5 +471,9 @@ func (s *Service) resolveGroupParameters(ctx context.Context, kv jetstream.KeyVa if err != nil { return nil, err } - return ResolveEffectiveParameters(instanceClass, overrides) + values := make(map[string]string, len(overrides)) + for name, override := range overrides { + values[name] = override.Value + } + return ResolveEffectiveParameters(instanceClass, values) } diff --git a/spinifex/handlers/rds/parametergroup_test.go b/spinifex/handlers/rds/parametergroup_test.go index 5d08fe7fd..344efbd61 100644 --- a/spinifex/handlers/rds/parametergroup_test.go +++ b/spinifex/handlers/rds/parametergroup_test.go @@ -177,7 +177,7 @@ func TestModifyDBParameterGroup_StoresValidatedOverrides(t *testing.T) { require.NoError(t, err) out, err := h.svc.ModifyDBParameterGroup(t.Context(), modifyParameters(testParameterGroup, - parameter("work_mem", "16384", ""), + parameter("work_mem", "16384", ApplyMethodPendingReboot), parameter("shared_buffers", "65536", ApplyMethodPendingReboot), ), testAccountID) require.NoError(t, err) @@ -186,6 +186,8 @@ func TestModifyDBParameterGroup_StoresValidatedOverrides(t *testing.T) { params := describedParameters(t, h, testParameterGroup) assert.Equal(t, "16384", aws.StringValue(params["work_mem"].ParameterValue)) assert.Equal(t, ParameterSourceUser, aws.StringValue(params["work_mem"].Source)) + assert.Equal(t, ApplyMethodPendingReboot, aws.StringValue(params["work_mem"].ApplyMethod), + "a dynamic parameter must report the customer's stored method") assert.Equal(t, "65536", aws.StringValue(params["shared_buffers"].ParameterValue)) assert.Equal(t, ParameterSourceUser, aws.StringValue(params["shared_buffers"].Source)) // Untouched parameters stay engine defaults rather than becoming user values. @@ -290,6 +292,24 @@ func TestDescribeDBParameters_ReportsComputedDefaultsAsLiterals(t *testing.T) { assert.NotEmpty(t, aws.StringValue(shared.AllowedValues)) } +func TestDescribeDBParameters_DerivesApplyMethodForLegacyOverrides(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + _, err := h.svc.CreateDBParameterGroup(t.Context(), parameterGroupInput(testParameterGroup), testAccountID) + require.NoError(t, err) + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + for _, rec := range []DBParameterRecord{ + {Name: "work_mem", Value: "16384"}, + {Name: "shared_buffers", Value: "65536"}, + } { + require.NoError(t, putJSON(t.Context(), kv, DBParameterGroupParamKey(testParameterGroup, rec.Name), &rec)) + } + + params := describedParameters(t, h, testParameterGroup) + assert.Equal(t, ApplyMethodImmediate, aws.StringValue(params["work_mem"].ApplyMethod)) + assert.Equal(t, ApplyMethodPendingReboot, aws.StringValue(params["shared_buffers"].ApplyMethod)) +} + func TestDescribeDBParameters_FiltersOnSource(t *testing.T) { h := newCreateHarness(t, testBaseDomain) _, err := h.svc.CreateDBParameterGroup(t.Context(), parameterGroupInput(testParameterGroup), testAccountID) diff --git a/spinifex/handlers/rds/records.go b/spinifex/handlers/rds/records.go index cd100b4b0..39471c1de 100644 --- a/spinifex/handlers/rds/records.go +++ b/spinifex/handlers/rds/records.go @@ -31,11 +31,12 @@ type DBInstanceRecord struct { // Blocks DeleteDBInstance outright. Settable at create and modify. DeletionProtection bool `json:"deletionProtection,omitempty"` - // Inert — the engine version is pinned, so nothing upgrades either way. - // Recorded anyway because a describe that does not echo it back reads as - // false against a Terraform schema that defaults it to true, and the plan - // then never settles on a change no modify can deliver. - AutoMinorVersionUpgrade bool `json:"autoMinorVersionUpgrade"` + // Accepted inert settings are recorded so describe echoes what the client set. + // Dropping them would leave Terraform planning changes no modify can deliver. + AutoMinorVersionUpgrade bool `json:"autoMinorVersionUpgrade"` + CopyTagsToSnapshot bool `json:"copyTagsToSnapshot"` + MonitoringInterval int64 `json:"monitoringInterval"` + EnablePerformanceInsights bool `json:"enablePerformanceInsights"` // The backup policy in force. Both windows are stored in AWS's canonical text // so a describe reads back the string a later modify compares against; an diff --git a/spinifex/handlers/rds/restore.go b/spinifex/handlers/rds/restore.go index 744b2f7af..e065bcb76 100644 --- a/spinifex/handlers/rds/restore.go +++ b/spinifex/handlers/rds/restore.go @@ -240,6 +240,7 @@ func (s *Service) resolveRestoreRequest(input *rds.RestoreDBInstanceFromDBSnapsh DeletionProtection: aws.BoolValue(input.DeletionProtection), AutoMinorVersionUpgrade: input.AutoMinorVersionUpgrade == nil || aws.BoolValue(input.AutoMinorVersionUpgrade), + CopyTagsToSnapshot: aws.BoolValue(input.CopyTagsToSnapshot), BackupRetentionPeriod: s.defaultRetentionDays(), Tags: tagMap, diff --git a/spinifex/handlers/rds/restore_test.go b/spinifex/handlers/rds/restore_test.go index e9bc6b5af..9823591b7 100644 --- a/spinifex/handlers/rds/restore_test.go +++ b/spinifex/handlers/rds/restore_test.go @@ -159,15 +159,18 @@ func TestRestoreDBInstanceFromDBSnapshot_HonoursTheRequestedOverrides(t *testing input.DBInstanceClass = aws.String("db.t3.large") input.AllocatedStorage = aws.Int64(50) input.Port = aws.Int64(6432) + input.CopyTagsToSnapshot = aws.Bool(true) input.Tags = []*rds.Tag{{Key: aws.String("env"), Value: aws.String("staging")}} - _, err := h.svc.RestoreDBInstanceFromDBSnapshot(t.Context(), input, testAccountID) + out, err := h.svc.RestoreDBInstanceFromDBSnapshot(t.Context(), input, testAccountID) require.NoError(t, err) stored := h.instance(t, testRestoredID) assert.Equal(t, "db.t3.large", stored.DBInstanceClass) assert.Equal(t, int64(50), stored.AllocatedStorage) assert.Equal(t, int64(6432), stored.Port) + assert.True(t, stored.CopyTagsToSnapshot) + assert.True(t, aws.BoolValue(out.DBInstance.CopyTagsToSnapshot)) assert.Equal(t, map[string]string{"env": "staging"}, stored.Tags) assert.Equal(t, int64(50), aws.Int64Value(h.launch.volumes.created[0].Size)) } diff --git a/spinifex/handlers/rds/store.go b/spinifex/handlers/rds/store.go index 71ef1b084..433f857d0 100644 --- a/spinifex/handlers/rds/store.go +++ b/spinifex/handlers/rds/store.go @@ -291,13 +291,13 @@ func ListDBParameterGroupNames(ctx context.Context, kv jetstream.KeyValue) ([]st } // The stored overrides of one parameter group, keyed by parameter name. -func ListDBParameterOverrides(ctx context.Context, kv jetstream.KeyValue, group string) (map[string]string, error) { +func ListDBParameterOverrides(ctx context.Context, kv jetstream.KeyValue, group string) (map[string]DBParameterRecord, error) { prefix := DBParameterGroupParamsPrefix(group) names, err := listNames(ctx, kv, prefix) if err != nil { return nil, err } - out := make(map[string]string, len(names)) + out := make(map[string]DBParameterRecord, len(names)) for _, name := range names { var rec DBParameterRecord found, err := getJSON(ctx, kv, prefix+name, &rec) @@ -307,7 +307,7 @@ func ListDBParameterOverrides(ctx context.Context, kv jetstream.KeyValue, group // A parameter reset between the listing and this read is simply gone, // which is the answer a resolve one tick later would give too. if found { - out[name] = rec.Value + out[name] = rec } } return out, nil diff --git a/spinifex/handlers/rds/tags_test.go b/spinifex/handlers/rds/tags_test.go index dd86b3f5f..5ad480912 100644 --- a/spinifex/handlers/rds/tags_test.go +++ b/spinifex/handlers/rds/tags_test.go @@ -5,6 +5,7 @@ import ( "strings" "sync" "testing" + "time" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/rds" @@ -231,6 +232,24 @@ func TestTagActions_MissingSnapshotIsRejected(t *testing.T) { assert.Contains(t, err.Error(), awserrors.ErrorDBSnapshotNotFound) } +func TestListTagsForResource_AcceptsAnAutomatedSnapshotARN(t *testing.T) { + h := newCreateHarness(t, "") + id := AutomatedSnapshotIdentifier(testDBInstanceID, time.Date(2026, 7, 24, 3, 4, 0, 0, time.UTC)) + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + require.NoError(t, putJSON(t.Context(), kv, DBSnapshotKey(id), &DBSnapshotRecord{ + DBSnapshotIdentifier: id, + AccountID: testAccountID, + Tags: map[string]string{"retention": "automated"}, + })) + + out, err := h.svc.ListTagsForResource(t.Context(), &rds.ListTagsForResourceInput{ + ResourceName: aws.String(DBSnapshotARN(testRegion, testAccountID, id)), + }, testAccountID) + require.NoError(t, err) + assert.Equal(t, awsTags("retention", "automated"), out.TagList) +} + func TestTagActions_ForeignAccountARNIsRejected(t *testing.T) { h := newCreateHarness(t, "") seedCreated(t, h, testDBInstanceID) diff --git a/spinifex/handlers/rds/validate.go b/spinifex/handlers/rds/validate.go index 63462dc58..22da76132 100644 --- a/spinifex/handlers/rds/validate.go +++ b/spinifex/handlers/rds/validate.go @@ -52,7 +52,10 @@ type validatedCreate struct { // engine version is pinned, so nothing upgrades either way. AWS defaults it // to true and so does the Terraform provider, so an unset request must not // read back as false. - AutoMinorVersionUpgrade bool + AutoMinorVersionUpgrade bool + CopyTagsToSnapshot bool + MonitoringInterval int64 + EnablePerformanceInsights bool // Automated backup settings in force. Create assigns unnamed windows, while // restore leaves them empty for lazy deterministic assignment. BackupRetentionPeriod int64 @@ -171,7 +174,10 @@ func (s *Service) validateCreateRequest(input *rds.CreateDBInstanceInput) (*vali DBParameterGroupName: paramGroup, DeletionProtection: aws.BoolValue(input.DeletionProtection), - AutoMinorVersionUpgrade: input.AutoMinorVersionUpgrade == nil || aws.BoolValue(input.AutoMinorVersionUpgrade), + AutoMinorVersionUpgrade: input.AutoMinorVersionUpgrade == nil || aws.BoolValue(input.AutoMinorVersionUpgrade), + CopyTagsToSnapshot: aws.BoolValue(input.CopyTagsToSnapshot), + MonitoringInterval: aws.Int64Value(input.MonitoringInterval), + EnablePerformanceInsights: aws.BoolValue(input.EnablePerformanceInsights), BackupRetentionPeriod: retention, PreferredBackupWindow: backupWindow, @@ -211,7 +217,7 @@ func validateDBInstanceIdentifier(id string) error { // drop it. Each rejection below is a parameter whose omission would create a // false safety, security or availability guarantee. Parameters that are merely // inert — AutoMinorVersionUpgrade, Performance Insights, Enhanced Monitoring, -// CopyTagsToSnapshot — are deliberately absent and accepted as no-ops. +// CopyTagsToSnapshot — are accepted and echoed so clients can converge. func rejectUnimplemented(input *rds.CreateDBInstanceInput) error { if aws.BoolValue(input.MultiAZ) { return unimplemented("MultiAZ", "this platform is single-AZ; a standby would not exist") @@ -231,6 +237,12 @@ func rejectUnimplemented(input *rds.CreateDBInstanceInput) error { if aws.Int64Value(input.Iops) > 0 { return unimplemented("Iops", "provisioned IOPS are not implemented; storage is gp3") } + if aws.Int64Value(input.MaxAllocatedStorage) > 0 { + return unimplemented("MaxAllocatedStorage", "storage autoscaling is not implemented") + } + if aws.Int64Value(input.StorageThroughput) > 0 { + return unimplemented("StorageThroughput", "provisioned throughput is not implemented; storage is gp3") + } if aws.StringValue(input.KmsKeyId) != "" { return unimplemented("KmsKeyId", "storage is encrypted with the cluster key, not a customer-managed one") } diff --git a/tests/e2e/rds/api_matrix_test.go b/tests/e2e/rds/api_matrix_test.go index 8b22249a9..65b2ae1e9 100644 --- a/tests/e2e/rds/api_matrix_test.go +++ b/tests/e2e/rds/api_matrix_test.go @@ -83,6 +83,9 @@ func TestAPIMatrix(t *testing.T) { {"MultiAZ", func(in *rds.CreateDBInstanceInput) { in.MultiAZ = aws.Bool(true) }}, {"StorageEncryptedFalse", func(in *rds.CreateDBInstanceInput) { in.StorageEncrypted = aws.Bool(false) }}, {"EngineVersionOtherThanThePin", func(in *rds.CreateDBInstanceInput) { in.EngineVersion = aws.String("16.4") }}, + {"MinorEngineVersion", func(in *rds.CreateDBInstanceInput) { in.EngineVersion = aws.String("18.4") }}, + {"MaxAllocatedStorage", func(in *rds.CreateDBInstanceInput) { in.MaxAllocatedStorage = aws.Int64(100) }}, + {"StorageThroughput", func(in *rds.CreateDBInstanceInput) { in.StorageThroughput = aws.Int64(250) }}, {"IAMDatabaseAuthentication", func(in *rds.CreateDBInstanceInput) { in.EnableIAMDatabaseAuthentication = aws.Bool(true) }}, @@ -242,19 +245,26 @@ func TestAPIMatrix(t *testing.T) { require.NotNil(t, created.DBInstance) t.Cleanup(func() { deleteInstance(t, f, probeID) }) - // An inert parameter is accepted and then reported honestly, - // so a customer reading the instance back is never told a feature is on when - // nothing implements it. - t.Run("InertParametersAreAcceptedAndReportedOff", func(t *testing.T) { + // Accepted inert parameters are echoed so Terraform does not plan changes no + // modify can deliver. + t.Run("InertParametersAreAcceptedAndEchoed", func(t *testing.T) { instance := created.DBInstance - // The exception, echoed rather than reported off: nothing upgrades a pinned - // version, but the provider's schema defaults it to true, so a false - // read-back would leave every default configuration with an uncleanable diff. - assert.True(t, aws.BoolValue(instance.AutoMinorVersionUpgrade), - "a client that set it has to read back what it set") + assert.True(t, aws.BoolValue(instance.AutoMinorVersionUpgrade)) + assert.True(t, aws.BoolValue(instance.PerformanceInsightsEnabled)) + assert.Equal(t, int64(60), aws.Int64Value(instance.MonitoringInterval)) + assert.True(t, aws.BoolValue(instance.CopyTagsToSnapshot)) + + _, err := f.AWS.RDS.ModifyDBInstance(&rds.ModifyDBInstanceInput{ + DBInstanceIdentifier: aws.String(probeID), + EnablePerformanceInsights: aws.Bool(false), + MonitoringInterval: aws.Int64(0), + CopyTagsToSnapshot: aws.Bool(false), + }) + require.NoError(t, err) + instance, err = harness.DescribeDBInstance(f.AWS, probeID) + require.NoError(t, err) assert.False(t, aws.BoolValue(instance.PerformanceInsightsEnabled)) - assert.Zero(t, aws.Int64Value(instance.MonitoringInterval), - "enhanced monitoring is not implemented, so no interval is in effect") + assert.Zero(t, aws.Int64Value(instance.MonitoringInterval)) assert.False(t, aws.BoolValue(instance.CopyTagsToSnapshot)) })