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
cloud-gov · Nov 27, 2024 · 70dd3f1 · 70dd3f1
1 parent 2bab812
commit 70dd3f1
Show file tree

Hide file tree

Showing 6 changed files with 171 additions and 52 deletions.
diff --git 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
@@ -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 <JSON string or file>" 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
@@ -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
@@ -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
@@ -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
@@ -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)
+			}
+		})
+	}
+}