Merge pull request #2148 from crossplane-contrib/backport-2146-to-rel…

…ease-0.51 [Backport release-0.51] fix(rdsinstance.database): do not send empty modify-calls
crossplane-contrib · Jan 23, 2025 · a5dad2f · a5dad2f
2 parents 286b475 + c8700ba
commit a5dad2f
Show file tree

Hide file tree

Showing 4 changed files with 24 additions and 8 deletions.
diff --git a/pkg/clients/database/rds.go b/pkg/clients/database/rds.go
@@ -887,6 +887,10 @@ func generateCloudWatchExportConfiguration(spec, current []string) *rdstypes.Clo
 		}
 	}
 
+	if len(toEnable) == 0 && len(toDisable) == 0 {
+		return nil
+	}
+
 	return &rdstypes.CloudwatchLogsExportConfiguration{
 		EnableLogTypes:  toEnable,
 		DisableLogTypes: toDisable,

diff --git a/pkg/clients/database/rds_test.go b/pkg/clients/database/rds_test.go
@@ -1368,10 +1368,6 @@ func TestGenerateModifyDBInstanceInput(t *testing.T) {
 			params: v1beta1.RDSInstanceParameters{},
 			want: rds.ModifyDBInstanceInput{
 				DBInstanceIdentifier: &emptyName,
-				CloudwatchLogsExportConfiguration: &rdstypes.CloudwatchLogsExportConfiguration{
-					DisableLogTypes: []string{},
-					EnableLogTypes:  []string{},
-				},
 			},
 		},
 	}

diff --git a/pkg/controller/database/rdsinstance/rdsinstance.go b/pkg/controller/database/rdsinstance/rdsinstance.go
@@ -31,6 +31,8 @@ import (
 	"github.com/crossplane/crossplane-runtime/pkg/password"
 	"github.com/crossplane/crossplane-runtime/pkg/reconciler/managed"
 	"github.com/crossplane/crossplane-runtime/pkg/resource"
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/pkg/errors"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -297,8 +299,20 @@ func (e *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext
 		modify.StorageThroughput = nil
 	}
 
-	if _, err = e.client.ModifyDBInstance(ctx, modify); err != nil {
-		return managed.ExternalUpdate{}, errorutils.Wrap(err, errModifyFailed)
+	// We only want to request a modification if we actually got more than ID and modify flags.
+	// Note: When ApplyImmediately is set to false, AWS rejects those empty calls with
+	// "cannot modify RDS instance: api error InalidParameterCombination: No modifications were requested"
+	// (but when set to true, AWS doesn't block them...)
+	diff := cmp.Diff(&awsrds.ModifyDBInstanceInput{}, modify, cmpopts.EquateEmpty(),
+		cmpopts.IgnoreUnexported(awsrds.ModifyDBInstanceInput{}),
+		cmpopts.IgnoreFields(awsrds.ModifyDBInstanceInput{}, "DBInstanceIdentifier"),
+		cmpopts.IgnoreFields(awsrds.ModifyDBInstanceInput{}, "AllowMajorVersionUpgrade"),
+		cmpopts.IgnoreFields(awsrds.ModifyDBInstanceInput{}, "ApplyImmediately"),
+	)
+	if diff != "" {
+		if _, err = e.client.ModifyDBInstance(ctx, modify); err != nil {
+			return managed.ExternalUpdate{}, errorutils.Wrap(err, errModifyFailed)
+		}
 	}
 
 	// Update tags if necessary

diff --git a/pkg/controller/database/rdsinstance/rdsinstance_test.go b/pkg/controller/database/rdsinstance/rdsinstance_test.go
@@ -857,10 +857,12 @@ func TestUpdate(t *testing.T) {
 						}, nil
 					},
 				},
-				cr: instance(),
+				cr: instance(
+					withEngineVersion(&engineVersion)),
 			},
 			want: want{
-				cr:  instance(),
+				cr: instance(
+					withEngineVersion(&engineVersion)),
 				err: errorutils.Wrap(errBoom, errModifyFailed),
 			},
 		},