Repository: crossplane/provider-aws (go)

Issue:
Add RDS storage autoscaling config to RDSInstance
<!--
Thank you for helping to improve Crossplane!

Please be sure to search for open issues before raising a new one. We use issues
for bug reports and feature requests. Please find us at https://slack.crossplane.io
for questions, support, and discussion.
-->

### What problem are you facing?
The `RDS storage autoscaling` configuration is currently not available in the [RDSInstance spec (v0.18.0)](https://doc.crds.dev/github.com/crossplane/provider-aws/database.aws.crossplane.io/RDSInstance/v1beta1@v0.18.0). 
This means that all RDS instances created by Crossplane will not be able to specify whether they want to use [RDS storage autoscaling](https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/USER_PIOPS.StorageTypes.html#USER_PIOPS.Autoscaling) or not. 


### How could Crossplane help solve your problem?
While creating RDS instances with Crossplane, the user could be given a configuration parameter `maxAllocatedStorage` which controls the automatic storage expansion. This enables the automatic expansion of volumes used by RDS instances directly from the RDS service:

> With storage autoscaling enabled, when Amazon RDS detects that you are running out of free database space it automatically scales up your storage. Amazon RDS starts a storage modification for an autoscaling-enabled DB instance when these factors apply: 
- Free available space is less than 10 percent of the allocated storage.
- The low-storage condition lasts at least five minutes.
- At least six hours have passed since the last storage modification.

> The additional storage is in increments of whichever of the following is greater:
- 5 GiB
- 10 percent of currently allocated storage
- Storage growth prediction for 7 hours based on the FreeStorageSpace metrics change in the past hour. For more information on metrics, see Monitoring with Amazon CloudWatch.




Pull request:
rds.rdsinstance: Add storage autoscaling (MaxAllocatedStorage)
### Description of your changes

Add storage autoscaling parameter MaxAllocatedStorage.
This parameter defines the upper limit to which Amazon RDS can automatically
scale the storage of the DB instance.

For more information about this setting, including limitations that apply
to it, see Managing capacity automatically with Amazon RDS storage autoscaling
(https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/USER_PIOPS.StorageTypes.html#USER_PIOPS.Autoscaling)
in the Amazon RDS User Guide.

Make rds.CreatePatch exclude AllocatedStorage if autoscaling is enabled,
and the requested AllocatedStorage is lower than was is currently
allocated.

This allows a user to manually scale *up* storage even if autoscaling is
enabled, and prevents crossplane from attempting to scale *down* if
AWS has performed autoscaling.

One problem is that this change silently ignores any attempts to manually
downscale when autoscaling is enabled. Since downscaling is not
supported and it is not possible to distinguish this from autoscaling, I
don't see any other solutions.

I have added tests to verify the logic for omitting StorageSize and the
up to date-test:

    --- FAIL: TestObserve (0.00s)
        --- FAIL: TestObserve/AutoscaledStorageIsUpToDate (0.00s)
            rdsinstance_test.go:354: r: -want, +got:
                  managed.ExternalObservation{
                  	ResourceExists:          true,
                - 	ResourceUpToDate:        true,
                + 	ResourceUpToDate:        false,
                  	ResourceLateInitialized: false,
                  	ConnectionDetails:       nil,
                  	Diff:                    "",
                  }
    --- FAIL: TestUpdate (0.00s)
        --- FAIL: TestUpdate/AutoscaleExcludeStorage (0.00s)
            rdsinstance_test.go:674: r: -want, +got:
                  interface{}(
                + 	e"cannot modify RDS instance: AllocatedStorage must not be set when on a modify request when AWS has autoscaled the storage",
                  )

Fixes #661

I have:

- [x] Read and followed Crossplane's [contribution process].
- [x] Run `make reviewable test` to ensure this PR is ready for review.

### How has this code been tested
* Add unit test for IsUpToDate and Modify
* Test scaling up by filling disk,  verified that we don't hang on forever trying to scale down (thanks @petteja)
* Test scaling up manually in the console, verified that we don't hang on forever trying to scale down
* Tested manually scaling up by bumping the allocated storage in the CR - still works

[contribution process]: https://git.io/fj2m9


Patch:
diff --git a/apis/database/v1beta1/rdsinstance_types.go b/apis/database/v1beta1/rdsinstance_types.go
index a8cffd077..7fb49fba0 100644
--- a/apis/database/v1beta1/rdsinstance_types.go
+++ b/apis/database/v1beta1/rdsinstance_types.go
@@ -522,6 +522,16 @@ type RDSInstanceParameters struct {
 	// +immutable
 	MasterPasswordSecretRef *xpv1.SecretKeySelector `json:"masterPasswordSecretRef,omitempty"`
 
+	// The upper limit to which Amazon RDS can automatically scale the storage of
+	// the DB instance.
+	//
+	// For more information about this setting, including limitations that apply
+	// to it, see Managing capacity automatically with Amazon RDS storage autoscaling
+	// (https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/USER_PIOPS.StorageTypes.html#USER_PIOPS.Autoscaling)
+	// in the Amazon RDS User Guide.
+	// +optional
+	MaxAllocatedStorage *int `json:"maxAllocatedStorage,omitempty"`
+
 	// MonitoringInterval is the interval, in seconds, between points when Enhanced Monitoring metrics
 	// are collected for the DB instance. To disable collecting Enhanced Monitoring
 	// metrics, specify 0. The default is 0.
@@ -855,6 +865,8 @@ const (
 	RDSInstanceStateBackingUp = "backing-up"
 	// The instance is being backed up, but is available
 	RDSInstanceStateConfiguringEnhancedMonitoring = "configuring-enhanced-monitoring"
+	// After you modify the storage size for a DB instance, the status of the DB instance is storage-optimization.
+	RDSInstanceStateStorageOptimization = "storage-optimization"
 	// The instance has failed and Amazon RDS can't recover it. Perform a point-in-time restore to the latest restorable time of the instance to recover the data.
 	RDSInstanceStateFailed = "failed"
 )
@@ -1110,6 +1122,9 @@ type RDSInstanceObservation struct {
 	// instance is accessed.
 	DBResourceID string `json:"dbResourceId,omitempty"`
 
+	// AllocatedStorage is the allocated storage size in gibibytes.
+	AllocatedStorage int `json:"allocatedStorage,omitempty"`
+
 	// DomainMemberships is the Active Directory Domain membership records associated with the DB instance.
 	DomainMemberships []DomainMembership `json:"domainMemberships,omitempty"`
 
diff --git a/apis/database/v1beta1/zz_generated.deepcopy.go b/apis/database/v1beta1/zz_generated.deepcopy.go
index 4c4641049..0f8d8ca62 100644
--- a/apis/database/v1beta1/zz_generated.deepcopy.go
+++ b/apis/database/v1beta1/zz_generated.deepcopy.go
@@ -643,6 +643,11 @@ func (in *RDSInstanceParameters) DeepCopyInto(out *RDSInstanceParameters) {
 		*out = new(v1.SecretKeySelector)
 		**out = **in
 	}
+	if in.MaxAllocatedStorage != nil {
+		in, out := &in.MaxAllocatedStorage, &out.MaxAllocatedStorage
+		*out = new(int)
+		**out = **in
+	}
 	if in.MonitoringInterval != nil {
 		in, out := &in.MonitoringInterval, &out.MonitoringInterval
 		*out = new(int)
diff --git a/package/crds/database.aws.crossplane.io_rdsinstances.yaml b/package/crds/database.aws.crossplane.io_rdsinstances.yaml
index afaad92d3..27f54e85a 100644
--- a/package/crds/database.aws.crossplane.io_rdsinstances.yaml
+++ b/package/crds/database.aws.crossplane.io_rdsinstances.yaml
@@ -451,6 +451,14 @@ spec:
                       First character must be a letter.    * Cannot be a reserved
                       word for the chosen database engine.'
                     type: string
+                  maxAllocatedStorage:
+                    description: "The upper limit to which Amazon RDS can automatically
+                      scale the storage of the DB instance. \n For more information
+                      about this setting, including limitations that apply to it,
+                      see Managing capacity automatically with Amazon RDS storage
+                      autoscaling (https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/USER_PIOPS.StorageTypes.html#USER_PIOPS.Autoscaling)
+                      in the Amazon RDS User Guide."
+                    type: integer
                   monitoringInterval:
                     description: 'MonitoringInterval is the interval, in seconds,
                       between points when Enhanced Monitoring metrics are collected
@@ -901,6 +909,10 @@ spec:
                 description: RDSInstanceObservation is the representation of the current
                   state that is observed.
                 properties:
+                  allocatedStorage:
+                    description: AllocatedStorage is the allocated storage size in
+                      gibibytes.
+                    type: integer
                   dbInstanceArn:
                     description: DBInstanceArn is the Amazon Resource Name (ARN) for
                       the DB instance.
diff --git a/pkg/clients/rds/rds.go b/pkg/clients/rds/rds.go
index a961f6cf2..2af4dbece 100644
--- a/pkg/clients/rds/rds.go
+++ b/pkg/clients/rds/rds.go
@@ -98,6 +98,7 @@ func GenerateCreateDBInstanceInput(name, password string, p *v1beta1.RDSInstance
 		LicenseModel:                       p.LicenseModel,
 		MasterUserPassword:                 awsclients.String(password),
 		MasterUsername:                     p.MasterUsername,
+		MaxAllocatedStorage:                awsclients.Int32Address(p.MaxAllocatedStorage),
 		MonitoringInterval:                 awsclients.Int32Address(p.MonitoringInterval),
 		MonitoringRoleArn:                  p.MonitoringRoleARN,
 		MultiAZ:                            p.MultiAZ,
@@ -257,6 +258,19 @@ func CreatePatch(in *rdstypes.DBInstance, target *v1beta1.RDSInstanceParameters)
 	currentParams := &v1beta1.RDSInstanceParameters{}
 	LateInitialize(currentParams, in)
 
+	// Don't attempt to scale down storage if autoscaling is enabled,
+	// and the current storage is larger than what was once
+	// requested. We still want to allow the user to manually scale
+	// the storage, so we only remove it if we're certain AWS will
+	// reject the value
+	if target.MaxAllocatedStorage != nil && aws.ToInt(target.AllocatedStorage) < aws.ToInt(currentParams.AllocatedStorage) {
+		// By making the values equal, CreateJSONPatch will exclude
+		// the field. It might seem more sensible to change the target
+		// object, but it's a pointer to the CR so we do not want to
+		// mutate it.
+		currentParams.AllocatedStorage = target.AllocatedStorage
+	}
+
 	jsonPatch, err := awsclients.CreateJSONPatch(currentParams, target)
 	if err != nil {
 		return nil, err
@@ -297,6 +311,7 @@ func GenerateModifyDBInstanceInput(name string, p *v1beta1.RDSInstanceParameters
 		EngineVersion:                      p.EngineVersion,
 		Iops:                               awsclients.Int32Address(p.IOPS),
 		LicenseModel:                       p.LicenseModel,
+		MaxAllocatedStorage:                awsclients.Int32Address(p.MaxAllocatedStorage),
 		MonitoringInterval:                 awsclients.Int32Address(p.MonitoringInterval),
 		MonitoringRoleArn:                  p.MonitoringRoleARN,
 		MultiAZ:                            p.MultiAZ,
@@ -333,6 +348,7 @@ func GenerateModifyDBInstanceInput(name string, p *v1beta1.RDSInstanceParameters
 // rds.DBInstance.
 func GenerateObservation(db rdstypes.DBInstance) v1beta1.RDSInstanceObservation { // nolint:gocyclo
 	o := v1beta1.RDSInstanceObservation{
+		AllocatedStorage:                      int(db.AllocatedStorage),
 		DBInstanceStatus:                      aws.ToString(db.DBInstanceStatus),
 		DBInstanceArn:                         aws.ToString(db.DBInstanceArn),
 		DBInstancePort:                        int(db.DbInstancePort),
@@ -497,6 +513,7 @@ func LateInitialize(in *v1beta1.RDSInstanceParameters, db *rdstypes.DBInstance)
 	in.KMSKeyID = awsclients.LateInitializeStringPtr(in.KMSKeyID, db.KmsKeyId)
 	in.LicenseModel = awsclients.LateInitializeStringPtr(in.LicenseModel, db.LicenseModel)
 	in.MasterUsername = awsclients.LateInitializeStringPtr(in.MasterUsername, db.MasterUsername)
+	in.MaxAllocatedStorage = awsclients.LateInitializeIntFrom32Ptr(in.MaxAllocatedStorage, db.MaxAllocatedStorage)
 	in.MonitoringInterval = awsclients.LateInitializeIntFrom32Ptr(in.MonitoringInterval, db.MonitoringInterval)
 	in.MonitoringRoleARN = awsclients.LateInitializeStringPtr(in.MonitoringRoleARN, db.MonitoringRoleArn)
 	in.MultiAZ = awsclients.LateInitializeBoolPtr(in.MultiAZ, awsclients.Bool(db.MultiAZ))
diff --git a/pkg/controller/database/rdsinstance.go b/pkg/controller/database/rdsinstance.go
index 294e2ac0a..653159fa2 100644
--- a/pkg/controller/database/rdsinstance.go
+++ b/pkg/controller/database/rdsinstance.go
@@ -126,7 +126,7 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex
 	cr.Status.AtProvider = rds.GenerateObservation(instance)
 
 	switch cr.Status.AtProvider.DBInstanceStatus {
-	case v1beta1.RDSInstanceStateAvailable, v1beta1.RDSInstanceStateModifying, v1beta1.RDSInstanceStateBackingUp, v1beta1.RDSInstanceStateConfiguringEnhancedMonitoring:
+	case v1beta1.RDSInstanceStateAvailable, v1beta1.RDSInstanceStateModifying, v1beta1.RDSInstanceStateBackingUp, v1beta1.RDSInstanceStateConfiguringEnhancedMonitoring, v1beta1.RDSInstanceStateStorageOptimization:
 		cr.Status.SetConditions(xpv1.Available())
 	case v1beta1.RDSInstanceStateCreating:
 		cr.Status.SetConditions(xpv1.Creating())


Test patch:
diff --git a/pkg/controller/database/rdsinstance_test.go b/pkg/controller/database/rdsinstance_test.go
index 60291fd53..6773c4c8d 100644
--- a/pkg/controller/database/rdsinstance_test.go
+++ b/pkg/controller/database/rdsinstance_test.go
@@ -109,6 +109,18 @@ func withDBInstanceStatus(s string) rdsModifier {
 	return func(r *v1beta1.RDSInstance) { r.Status.AtProvider.DBInstanceStatus = s }
 }
 
+func withAllocatedStorage(i int) rdsModifier {
+	return func(r *v1beta1.RDSInstance) { r.Spec.ForProvider.AllocatedStorage = &i }
+}
+
+func withMaxAllocatedStorage(i int) rdsModifier {
+	return func(r *v1beta1.RDSInstance) { r.Spec.ForProvider.MaxAllocatedStorage = &i }
+}
+
+func withStatusAllocatedStorage(i int) rdsModifier {
+	return func(r *v1beta1.RDSInstance) { r.Status.AtProvider.AllocatedStorage = i }
+}
+
 func withPasswordSecretRef(s xpv1.SecretKeySelector) rdsModifier {
 	return func(r *v1beta1.RDSInstance) { r.Spec.ForProvider.MasterPasswordSecretRef = &s }
 }
@@ -161,6 +173,37 @@ func TestObserve(t *testing.T) {
 				},
 			},
 		},
+		"AutoscaledStorageIsUpToDate": { // if aws scales storage up, we should still consider it up to date, even if initial storage size was provided
+			args: args{
+				rds: &fake.MockRDSClient{
+					MockDescribe: func(ctx context.Context, input *awsrds.DescribeDBInstancesInput, opts []func(*awsrds.Options)) (*awsrds.DescribeDBInstancesOutput, error) {
+						return &awsrds.DescribeDBInstancesOutput{
+							DBInstances: []awsrdstypes.DBInstance{
+								{
+									DBInstanceStatus:    aws.String(string(v1beta1.RDSInstanceStateAvailable)),
+									MaxAllocatedStorage: aws.Int32(100),
+									AllocatedStorage:    30,
+								},
+							},
+						}, nil
+					},
+				},
+				cr: instance(withMaxAllocatedStorage(100), withAllocatedStorage(20)),
+			},
+			want: want{
+				cr: instance(
+					withMaxAllocatedStorage(100),
+					withAllocatedStorage(20),
+					withStatusAllocatedStorage(30),
+					withConditions(xpv1.Available()),
+					withDBInstanceStatus(string(v1beta1.RDSInstanceStateAvailable))),
+				result: managed.ExternalObservation{
+					ResourceExists:    true,
+					ResourceUpToDate:  true,
+					ConnectionDetails: rds.GetConnectionDetails(v1beta1.RDSInstance{}),
+				},
+			},
+		},
 		"DeletingState": {
 			args: args{
 				rds: &fake.MockRDSClient{
@@ -517,6 +560,33 @@ func TestUpdate(t *testing.T) {
 				cr: instance(withTags(map[string]string{"foo": "bar"})),
 			},
 		},
+		"AutoscaleExcludeStorage": {
+			args: args{
+				rds: &fake.MockRDSClient{
+					MockModify: func(ctx context.Context, input *awsrds.ModifyDBInstanceInput, opts []func(*awsrds.Options)) (*awsrds.ModifyDBInstanceOutput, error) {
+						if input.AllocatedStorage != nil {
+							return &awsrds.ModifyDBInstanceOutput{}, errors.New("AllocatedStorage must not be set when on a modify request when AWS has autoscaled the storage")
+						}
+						return &awsrds.ModifyDBInstanceOutput{}, nil
+					},
+					MockDescribe: func(ctx context.Context, input *awsrds.DescribeDBInstancesInput, opts []func(*awsrds.Options)) (*awsrds.DescribeDBInstancesOutput, error) {
+						return &awsrds.DescribeDBInstancesOutput{
+							DBInstances: []awsrdstypes.DBInstance{{
+								MaxAllocatedStorage: aws.Int32(100),
+								AllocatedStorage:    30,
+							}},
+						}, nil
+					},
+					MockAddTags: func(ctx context.Context, input *awsrds.AddTagsToResourceInput, opts []func(*awsrds.Options)) (*awsrds.AddTagsToResourceOutput, error) {
+						return &awsrds.AddTagsToResourceOutput{}, nil
+					},
+				},
+				cr: instance(withMaxAllocatedStorage(100), withAllocatedStorage(20)),
+			},
+			want: want{
+				cr: instance(withMaxAllocatedStorage(100), withAllocatedStorage(20)),
+			},
+		},
 		"AlreadyModifying": {
 			args: args{
 				cr: instance(withDBInstanceStatus(v1beta1.RDSInstanceStateModifying)),