From 70dd3f1a3ed91283209d24c8290891f6bc20b61c Mon Sep 17 00:00:00 2001 From: Mark Boyd Date: Wed, 27 Nov 2024 14:16:49 -0500 Subject: [PATCH] Fix update retention logic (#385) * fix logic for validating RDS params to not allow a retention period less than the configuring minimum backup retention period * add more test cases * add logic to ensure that backup retention cannot be less tha configured minimum retention on update * change backup_retention_period update param to use pointer to separate the difference between nil and 0 as values * update test to verify error is thrown when backup retention period of 0 is specified * fix invalid pointer reference * update expected property for test * add more test cases * update org name in github repo from 18F -> cloud-gov --- ci/config.yml | 2 +- services/rds/broker.go | 10 +-- services/rds/broker_test.go | 123 ++++++++++++++++++++++--------- services/rds/rdsinstance.go | 21 ++++-- services/rds/rdsinstance_test.go | 23 ++++-- services/rds/validate_test.go | 44 +++++++++++ 6 files changed, 171 insertions(+), 52 deletions(-) diff --git a/ci/config.yml b/ci/config.yml index 1e73b6af..fdca29c8 100644 --- a/ci/config.yml +++ b/ci/config.yml @@ -1,4 +1,4 @@ # Git sources -aws-broker-url: https://github.com/18F/aws-broker +aws-broker-url: https://github.com/cloud-gov/aws-broker aws-broker-branch: main aws-broker-branch-development: main diff --git a/services/rds/broker.go b/services/rds/broker.go index 7ce46dde..8253be9f 100644 --- a/services/rds/broker.go +++ b/services/rds/broker.go @@ -19,15 +19,15 @@ import ( "github.com/18F/aws-broker/helpers/response" ) -// Options is a struct containing all of the custom parameters supported by -// the broker for the "cf create-service" and "cf update-service" commands - +// Options is a struct containing all of the custom parameters supported by +// the broker for the "cf create-service" and "cf update-service" commands - // they are passed in via the "-c " flag. type Options struct { AllocatedStorage int64 `json:"storage"` EnableFunctions bool `json:"enable_functions"` PubliclyAccessible bool `json:"publicly_accessible"` Version string `json:"version"` - BackupRetentionPeriod int64 `json:"backup_retention_period"` + BackupRetentionPeriod *int64 `json:"backup_retention_period"` BinaryLogFormat string `json:"binary_log_format"` EnablePgCron *bool `json:"enable_pg_cron"` RotateCredentials *bool `json:"rotate_credentials"` @@ -43,11 +43,11 @@ func (o Options) Validate(settings *config.Settings) error { return fmt.Errorf("Invalid storage %d; must be <= %d", o.AllocatedStorage, settings.MaxAllocatedStorage) } - if o.BackupRetentionPeriod > settings.MaxBackupRetention { + if o.BackupRetentionPeriod != nil && *o.BackupRetentionPeriod > settings.MaxBackupRetention { return fmt.Errorf("Invalid Retention Period %d; must be <= %d", o.BackupRetentionPeriod, settings.MaxBackupRetention) } - if o.BackupRetentionPeriod != 0 && o.BackupRetentionPeriod < settings.MinBackupRetention { + if o.BackupRetentionPeriod != nil && *o.BackupRetentionPeriod < settings.MinBackupRetention { return fmt.Errorf("Invalid Retention Period %d; must be => %d", o.BackupRetentionPeriod, settings.MinBackupRetention) } diff --git a/services/rds/broker_test.go b/services/rds/broker_test.go index c87f8962..7ea015ad 100644 --- a/services/rds/broker_test.go +++ b/services/rds/broker_test.go @@ -63,6 +63,7 @@ func TestParseModifyOptionsFromRequest(t *testing.T) { broker *rdsBroker modifyRequest request.Request expectedOptions Options + expectErr bool }{ "enable PG cron not specified": { broker: &rdsBroker{ @@ -72,12 +73,11 @@ func TestParseModifyOptionsFromRequest(t *testing.T) { RawParameters: []byte(``), }, expectedOptions: Options{ - AllocatedStorage: 0, - BackupRetentionPeriod: 0, - EnableFunctions: false, - PubliclyAccessible: false, - Version: "", - BinaryLogFormat: "", + AllocatedStorage: 0, + EnableFunctions: false, + PubliclyAccessible: false, + Version: "", + BinaryLogFormat: "", }, }, "enable PG cron true": { @@ -88,13 +88,12 @@ func TestParseModifyOptionsFromRequest(t *testing.T) { RawParameters: []byte(`{ "enable_pg_cron": true }`), }, expectedOptions: Options{ - AllocatedStorage: 0, - BackupRetentionPeriod: 0, - EnablePgCron: aws.Bool(true), - EnableFunctions: false, - PubliclyAccessible: false, - Version: "", - BinaryLogFormat: "", + AllocatedStorage: 0, + EnablePgCron: aws.Bool(true), + EnableFunctions: false, + PubliclyAccessible: false, + Version: "", + BinaryLogFormat: "", }, }, "enable PG cron false": { @@ -105,13 +104,12 @@ func TestParseModifyOptionsFromRequest(t *testing.T) { RawParameters: []byte(`{ "enable_pg_cron": false }`), }, expectedOptions: Options{ - AllocatedStorage: 0, - BackupRetentionPeriod: 0, - EnablePgCron: aws.Bool(false), - EnableFunctions: false, - PubliclyAccessible: false, - Version: "", - BinaryLogFormat: "", + AllocatedStorage: 0, + EnablePgCron: aws.Bool(false), + EnableFunctions: false, + PubliclyAccessible: false, + Version: "", + BinaryLogFormat: "", }, }, "rotate creds true": { @@ -122,13 +120,12 @@ func TestParseModifyOptionsFromRequest(t *testing.T) { RawParameters: []byte(`{ "rotate_credentials": true }`), }, expectedOptions: Options{ - AllocatedStorage: 0, - BackupRetentionPeriod: 0, - RotateCredentials: aws.Bool(true), - EnableFunctions: false, - PubliclyAccessible: false, - Version: "", - BinaryLogFormat: "", + AllocatedStorage: 0, + RotateCredentials: aws.Bool(true), + EnableFunctions: false, + PubliclyAccessible: false, + Version: "", + BinaryLogFormat: "", }, }, "rotate creds false": { @@ -139,13 +136,12 @@ func TestParseModifyOptionsFromRequest(t *testing.T) { RawParameters: []byte(`{ "rotate_credentials": false }`), }, expectedOptions: Options{ - AllocatedStorage: 0, - BackupRetentionPeriod: 0, - RotateCredentials: aws.Bool(false), - EnableFunctions: false, - PubliclyAccessible: false, - Version: "", - BinaryLogFormat: "", + AllocatedStorage: 0, + RotateCredentials: aws.Bool(false), + EnableFunctions: false, + PubliclyAccessible: false, + Version: "", + BinaryLogFormat: "", }, }, "rotate creds not specified": { @@ -155,22 +151,77 @@ func TestParseModifyOptionsFromRequest(t *testing.T) { modifyRequest: request.Request{ RawParameters: []byte(`{}`), }, + expectedOptions: Options{ + AllocatedStorage: 0, + EnableFunctions: false, + PubliclyAccessible: false, + Version: "", + BinaryLogFormat: "", + }, + }, + "backup retention period less than minimum is rejected": { + broker: &rdsBroker{ + settings: &config.Settings{ + MinBackupRetention: 14, + }, + }, + modifyRequest: request.Request{ + RawParameters: []byte(`{"backup_retention_period": 0}`), + }, expectedOptions: Options{ AllocatedStorage: 0, - BackupRetentionPeriod: 0, EnableFunctions: false, PubliclyAccessible: false, Version: "", BinaryLogFormat: "", + BackupRetentionPeriod: aws.Int64(0), + }, + expectErr: true, + }, + "allocated storage exceeding maxmimum is rejected": { + broker: &rdsBroker{ + settings: &config.Settings{ + MaxAllocatedStorage: 100, + }, + }, + modifyRequest: request.Request{ + RawParameters: []byte(`{"storage": 150}`), + }, + expectedOptions: Options{ + AllocatedStorage: 150, + EnableFunctions: false, + PubliclyAccessible: false, + Version: "", + BinaryLogFormat: "", + }, + expectErr: true, + }, + "throws error on invalid JSON": { + broker: &rdsBroker{ + settings: &config.Settings{}, + }, + modifyRequest: request.Request{ + RawParameters: []byte(`{"foo": }`), }, + expectedOptions: Options{ + AllocatedStorage: 0, + EnableFunctions: false, + PubliclyAccessible: false, + Version: "", + BinaryLogFormat: "", + }, + expectErr: true, }, } for name, test := range testCases { t.Run(name, func(t *testing.T) { options, err := test.broker.parseModifyOptionsFromRequest(test.modifyRequest) - if err != nil { + if !test.expectErr && err != nil { t.Fatalf("unexpected error: %s", err) } + if test.expectErr && err == nil { + t.Errorf("expected error, got nil") + } if !reflect.DeepEqual(test.expectedOptions, options) { t.Errorf("expected: %+v, got %+v", test.expectedOptions, options) } diff --git a/services/rds/rdsinstance.go b/services/rds/rdsinstance.go index 703039c4..62edb9ce 100644 --- a/services/rds/rdsinstance.go +++ b/services/rds/rdsinstance.go @@ -206,9 +206,17 @@ func (i *RDSInstance) modify(options Options, plan catalog.RDSPlan, settings *co i.StorageType = options.StorageType } - // Check if there is a backup retention change: - if options.BackupRetentionPeriod > 0 { - i.BackupRetentionPeriod = options.BackupRetentionPeriod + // Check if there is a backup retention change + if options.BackupRetentionPeriod != nil && *options.BackupRetentionPeriod > 0 { + i.BackupRetentionPeriod = *options.BackupRetentionPeriod + } + + // There may be some instances which were previously updated to have + // i.BackupRetentionPeriod = 0. Make sure those instances get updated + // to the minimum backup retention period, since 0 will disable backups + // on the database. + if i.BackupRetentionPeriod < settings.MinBackupRetention { + i.BackupRetentionPeriod = settings.MinBackupRetention } // Check if there is a binary log format change and if so, apply it @@ -264,8 +272,11 @@ func (i *RDSInstance) init( i.DbVersion = plan.DbVersion } - i.BackupRetentionPeriod = options.BackupRetentionPeriod - if options.BackupRetentionPeriod == 0 { + if options.BackupRetentionPeriod != nil { + i.BackupRetentionPeriod = *options.BackupRetentionPeriod + } + + if i.BackupRetentionPeriod == 0 { i.BackupRetentionPeriod = plan.BackupRetentionPeriod } diff --git a/services/rds/rdsinstance_test.go b/services/rds/rdsinstance_test.go index 7b96583d..63244d04 100644 --- a/services/rds/rdsinstance_test.go +++ b/services/rds/rdsinstance_test.go @@ -85,7 +85,7 @@ func TestInit(t *testing.T) { }{ "sets expected properties": { options: Options{ - BackupRetentionPeriod: 14, + BackupRetentionPeriod: aws.Int64(21), }, plan: catalog.RDSPlan{ Plan: catalog.Plan{ @@ -133,7 +133,7 @@ func TestInit(t *testing.T) { Adapter: "adapter-1", DbType: "postgres", DbVersion: "15", - BackupRetentionPeriod: 14, + BackupRetentionPeriod: 21, Tags: map[string]string{}, StorageType: "gp3", AllocatedStorage: 20, @@ -297,7 +297,7 @@ func TestInit(t *testing.T) { }, "merges plan and instance tags": { options: Options{ - BackupRetentionPeriod: 14, + BackupRetentionPeriod: aws.Int64(14), }, plan: catalog.RDSPlan{ Plan: catalog.Plan{ @@ -449,7 +449,7 @@ func TestModifyInstance(t *testing.T) { }, "update backup retention period": { options: Options{ - BackupRetentionPeriod: 20, + BackupRetentionPeriod: aws.Int64(20), }, existingInstance: &RDSInstance{ BackupRetentionPeriod: 10, @@ -462,7 +462,7 @@ func TestModifyInstance(t *testing.T) { }, "does not update backup retention period": { options: Options{ - BackupRetentionPeriod: 0, + BackupRetentionPeriod: aws.Int64(0), }, existingInstance: &RDSInstance{ BackupRetentionPeriod: 20, @@ -540,6 +540,19 @@ func TestModifyInstance(t *testing.T) { plan: catalog.RDSPlan{}, settings: &config.Settings{}, }, + "does not allow backup retention less than minimum backup retention": { + options: Options{}, + existingInstance: &RDSInstance{ + BackupRetentionPeriod: 0, + }, + expectedInstance: &RDSInstance{ + BackupRetentionPeriod: 14, + }, + plan: catalog.RDSPlan{}, + settings: &config.Settings{ + MinBackupRetention: 14, + }, + }, } for name, test := range testCases { diff --git a/services/rds/validate_test.go b/services/rds/validate_test.go index 57e6bd5a..e0bf83c9 100644 --- a/services/rds/validate_test.go +++ b/services/rds/validate_test.go @@ -2,6 +2,9 @@ package rds import ( "testing" + + "github.com/18F/aws-broker/config" + "github.com/aws/aws-sdk-go/aws" ) func TestBinaryLogFormatValidation(t *testing.T) { @@ -79,3 +82,44 @@ func TestValidateStorageType(t *testing.T) { }) } } + +func TestValidateRetentionPeriod(t *testing.T) { + testCases := map[string]struct { + retentionPeriod int64 + expectedErr bool + settings *config.Settings + }{ + // 0 was a special case in the previous buggy code, so should be + // left as a standalone test case. A retention period value of 0 + // will disable backups on a database. + "should not allow retention period of 0": { + retentionPeriod: 0, + expectedErr: true, + settings: &config.Settings{ + MinBackupRetention: 14, + }, + }, + "should not allow retention period of less than the minimum": { + retentionPeriod: 5, + expectedErr: true, + settings: &config.Settings{ + MinBackupRetention: 14, + }, + }, + } + + for name, test := range testCases { + t.Run(name, func(t *testing.T) { + opts := &Options{ + BackupRetentionPeriod: aws.Int64(test.retentionPeriod), + } + err := opts.Validate(test.settings) + if test.expectedErr && err == nil { + t.Fatalf("expected error") + } + if !test.expectedErr && err != nil { + t.Fatalf("unexpected error: %s", err) + } + }) + } +}