From 226faec0bccbd4197363678831d3ac34fa6a6931 Mon Sep 17 00:00:00 2001 From: Charel Baum Date: Fri, 9 Dec 2022 23:21:11 +0100 Subject: [PATCH 1/2] feat(rds): instance + cluster allowMajorVersionUpgrade Signed-off-by: Charel Baum --- apis/rds/generator-config.yaml | 12 ++ apis/rds/v1alpha1/custom_types.go | 16 ++- apis/rds/v1alpha1/zz_db_cluster.go | 8 ++ apis/rds/v1alpha1/zz_db_instance.go | 10 ++ apis/rds/v1alpha1/zz_generated.deepcopy.go | 10 ++ examples/rds/db-aurora-cluster.yaml | 4 +- examples/rds/db-instance.yaml | 3 +- .../rds.aws.crossplane.io_dbclusters.yaml | 12 +- .../rds.aws.crossplane.io_dbinstances.yaml | 18 ++- pkg/controller/rds/dbcluster/setup.go | 73 ++++++++--- pkg/controller/rds/dbcluster/setup_test.go | 6 +- .../rds/dbcluster/zz_conversions.go | 3 + pkg/controller/rds/dbinstance/setup.go | 115 ++++++++++++++---- .../rds/dbinstance/zz_conversions.go | 3 + 14 files changed, 243 insertions(+), 50 deletions(-) diff --git a/apis/rds/generator-config.yaml b/apis/rds/generator-config.yaml index 5aa281ebf6..8620ad523a 100644 --- a/apis/rds/generator-config.yaml +++ b/apis/rds/generator-config.yaml @@ -7,6 +7,18 @@ operations: operation_type: Delete resources: + DBInstance: + fields: + AllowMajorVersionUpgrade: + from: + operation: ModifyDBInstance + path: AllowMajorVersionUpgrade + DBCluster: + fields: + AllowMajorVersionUpgrade: + from: + operation: ModifyDBCluster + path: AllowMajorVersionUpgrade DBInstanceRoleAssociation: exceptions: errors: diff --git a/apis/rds/v1alpha1/custom_types.go b/apis/rds/v1alpha1/custom_types.go index 5f0dd6a902..450beb9944 100644 --- a/apis/rds/v1alpha1/custom_types.go +++ b/apis/rds/v1alpha1/custom_types.go @@ -196,7 +196,9 @@ type CustomDBClusterParameters struct { // Constraints: Must contain from 8 to 41 characters. Required. MasterUserPasswordSecretRef *xpv1.SecretKeySelector `json:"masterUserPasswordSecretRef"` - // A list of EC2 VPC security groups to associate with this DB cluster. + // A list of VPC security groups that the DB cluster will belong to. + // + // Valid for: Aurora DB clusters and Multi-AZ DB clusters VPCSecurityGroupIDs []string `json:"vpcSecurityGroupIDs,omitempty"` // VPCSecurityGroupIDRefs are references to VPCSecurityGroups used to set @@ -539,7 +541,17 @@ type CustomDBInstanceParameters struct { // +optional SkipFinalSnapshot bool `json:"skipFinalSnapshot,omitempty"` - // A list of EC2 VPC security groups to associate with this DB instance. + // A list of Amazon EC2 VPC security groups to authorize on this DB instance. + // This change is asynchronously applied as soon as possible. + // + // This setting doesn't apply to RDS Custom. + // + // Amazon Aurora + // Not applicable. The associated list of EC2 VPC security groups is managed + // by the DB cluster. For more information, see ModifyDBCluster. + // + // Constraints: + // * If supplied, must match existing VpcSecurityGroupIds. VPCSecurityGroupIDs []string `json:"vpcSecurityGroupIDs,omitempty"` // VPCSecurityGroupIDRefs are references to VPCSecurityGroups used to set diff --git a/apis/rds/v1alpha1/zz_db_cluster.go b/apis/rds/v1alpha1/zz_db_cluster.go index c714ae3d76..590b29409d 100644 --- a/apis/rds/v1alpha1/zz_db_cluster.go +++ b/apis/rds/v1alpha1/zz_db_cluster.go @@ -36,6 +36,14 @@ type DBClusterParameters struct { // // Valid for: Multi-AZ DB clusters only AllocatedStorage *int64 `json:"allocatedStorage,omitempty"` + // A value that indicates whether major version upgrades are allowed. + // + // Constraints: You must allow major version upgrades when specifying a value + // for the EngineVersion parameter that is a different major version than the + // DB cluster's current version. + // + // Valid for: Aurora DB clusters only + AllowMajorVersionUpgrade *bool `json:"allowMajorVersionUpgrade,omitempty"` // A value that indicates whether minor engine upgrades are applied automatically // to the DB cluster during the maintenance window. By default, minor engine // upgrades are applied automatically. diff --git a/apis/rds/v1alpha1/zz_db_instance.go b/apis/rds/v1alpha1/zz_db_instance.go index c0d7a7ffce..9941f11cd1 100644 --- a/apis/rds/v1alpha1/zz_db_instance.go +++ b/apis/rds/v1alpha1/zz_db_instance.go @@ -105,6 +105,16 @@ type DBInstanceParameters struct { // be an integer from 20 to 1024. Web and Express editions: Must be an integer // from 20 to 1024. AllocatedStorage *int64 `json:"allocatedStorage,omitempty"` + // A value that indicates whether major version upgrades are allowed. Changing + // this parameter doesn't result in an outage and the change is asynchronously + // applied as soon as possible. + // + // This setting doesn't apply to RDS Custom. + // + // Constraints: Major version upgrades must be allowed when specifying a value + // for the EngineVersion parameter that is a different major version than the + // DB instance's current version. + AllowMajorVersionUpgrade *bool `json:"allowMajorVersionUpgrade,omitempty"` // A value that indicates whether minor engine upgrades are applied automatically // to the DB instance during the maintenance window. By default, minor engine // upgrades are applied automatically. diff --git a/apis/rds/v1alpha1/zz_generated.deepcopy.go b/apis/rds/v1alpha1/zz_generated.deepcopy.go index 2ccc0ffa8b..a1cee4cb5c 100644 --- a/apis/rds/v1alpha1/zz_generated.deepcopy.go +++ b/apis/rds/v1alpha1/zz_generated.deepcopy.go @@ -1328,6 +1328,11 @@ func (in *DBClusterParameters) DeepCopyInto(out *DBClusterParameters) { *out = new(int64) **out = **in } + if in.AllowMajorVersionUpgrade != nil { + in, out := &in.AllowMajorVersionUpgrade, &out.AllowMajorVersionUpgrade + *out = new(bool) + **out = **in + } if in.AutoMinorVersionUpgrade != nil { in, out := &in.AutoMinorVersionUpgrade, &out.AutoMinorVersionUpgrade *out = new(bool) @@ -2870,6 +2875,11 @@ func (in *DBInstanceParameters) DeepCopyInto(out *DBInstanceParameters) { *out = new(int64) **out = **in } + if in.AllowMajorVersionUpgrade != nil { + in, out := &in.AllowMajorVersionUpgrade, &out.AllowMajorVersionUpgrade + *out = new(bool) + **out = **in + } if in.AutoMinorVersionUpgrade != nil { in, out := &in.AutoMinorVersionUpgrade, &out.AutoMinorVersionUpgrade *out = new(bool) diff --git a/examples/rds/db-aurora-cluster.yaml b/examples/rds/db-aurora-cluster.yaml index e87f6a5f6d..6840576d85 100644 --- a/examples/rds/db-aurora-cluster.yaml +++ b/examples/rds/db-aurora-cluster.yaml @@ -6,7 +6,9 @@ spec: forProvider: region: us-east-1 engine: aurora-mysql - masterUsername: admin + allowMajorVersionUpgrade: true # unset per default (Note: dbClusterParameterGroup with correct dbParameterClusterGroupFamily may needed, before majorVersion upgrade possible) + # for majorVersion upgrade via Cluster - depending on the setup - instances may need adjustments: before (e.g. supported instanceClass) or after (e.g. matching dbParameterGroup) the upgrade + masterUsername: adminuser masterUserPasswordSecretRef: name: example-aurora-mysql-cluster namespace: crossplane-system diff --git a/examples/rds/db-instance.yaml b/examples/rds/db-instance.yaml index 9d8cc2236c..336cf78667 100644 --- a/examples/rds/db-instance.yaml +++ b/examples/rds/db-instance.yaml @@ -9,10 +9,11 @@ spec: autoMinorVersionUpgrade: true autogeneratePassword: true backupRetentionPeriod: 14 - dbInstanceClass: db.t2.micro + dbInstanceClass: db.t3.micro # needs to support engine and -version (see AWS Docs: https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/Concepts.DBInstanceClass.html#Concepts.DBInstanceClass.Support) dbName: example engine: postgres engineVersion: "12.9" + allowMajorVersionUpgrade: true # unset per default (Note: supported dbInstanceClass and dbParameterGroup with correct dbParameterGroupFamily needed, before majorVersion upgrade possible; applyImmediately matters) masterUsername: adminuser masterUserPasswordSecretRef: key: password diff --git a/package/crds/rds.aws.crossplane.io_dbclusters.yaml b/package/crds/rds.aws.crossplane.io_dbclusters.yaml index a7d26f15b0..dc873fa9f9 100644 --- a/package/crds/rds.aws.crossplane.io_dbclusters.yaml +++ b/package/crds/rds.aws.crossplane.io_dbclusters.yaml @@ -68,6 +68,13 @@ spec: DB clusters only" format: int64 type: integer + allowMajorVersionUpgrade: + description: "A value that indicates whether major version upgrades + are allowed. \n Constraints: You must allow major version upgrades + when specifying a value for the EngineVersion parameter that + is a different major version than the DB cluster's current version. + \n Valid for: Aurora DB clusters only" + type: boolean applyImmediately: description: "A value that indicates whether the modifications in this request and any pending modifications are asynchronously @@ -1075,8 +1082,9 @@ spec: type: object type: object vpcSecurityGroupIDs: - description: A list of EC2 VPC security groups to associate with - this DB cluster. + description: "A list of VPC security groups that the DB cluster + will belong to. \n Valid for: Aurora DB clusters and Multi-AZ + DB clusters" items: type: string type: array diff --git a/package/crds/rds.aws.crossplane.io_dbinstances.yaml b/package/crds/rds.aws.crossplane.io_dbinstances.yaml index e5c743eb44..d01e92c765 100644 --- a/package/crds/rds.aws.crossplane.io_dbinstances.yaml +++ b/package/crds/rds.aws.crossplane.io_dbinstances.yaml @@ -105,6 +105,15 @@ spec: and Express editions: Must be an integer from 20 to 1024." format: int64 type: integer + allowMajorVersionUpgrade: + description: "A value that indicates whether major version upgrades + are allowed. Changing this parameter doesn't result in an outage + and the change is asynchronously applied as soon as possible. + \n This setting doesn't apply to RDS Custom. \n Constraints: + Major version upgrades must be allowed when specifying a value + for the EngineVersion parameter that is a different major version + than the DB instance's current version." + type: boolean applyImmediately: description: "A value that indicates whether the modifications in this request and any pending modifications are asynchronously @@ -1284,8 +1293,13 @@ spec: type: object type: object vpcSecurityGroupIDs: - description: A list of EC2 VPC security groups to associate with - this DB instance. + description: "A list of Amazon EC2 VPC security groups to authorize + on this DB instance. This change is asynchronously applied as + soon as possible. \n This setting doesn't apply to RDS Custom. + \n Amazon Aurora Not applicable. The associated list of EC2 + VPC security groups is managed by the DB cluster. For more information, + see ModifyDBCluster. \n Constraints: * If supplied, must match + existing VpcSecurityGroupIds." items: type: string type: array diff --git a/pkg/controller/rds/dbcluster/setup.go b/pkg/controller/rds/dbcluster/setup.go index a1d4fbda59..8aea868a22 100644 --- a/pkg/controller/rds/dbcluster/setup.go +++ b/pkg/controller/rds/dbcluster/setup.go @@ -95,12 +95,16 @@ func (e *custom) postObserve(ctx context.Context, cr *svcapitypes.DBCluster, res return managed.ExternalObservation{}, err } switch aws.StringValue(resp.DBClusters[0].Status) { - case "available", "modifying": + case "available", "storage-optimization", "backing-up": cr.SetConditions(xpv1.Available()) - case "deleting", "stopped", "stopping": - cr.SetConditions(xpv1.Unavailable()) + case "modifying": + cr.SetConditions(xpv1.Available().WithMessage("DB Cluster is " + aws.StringValue(resp.DBClusters[0].Status) + ", availability may vary")) + case "deleting": + cr.SetConditions(xpv1.Deleting()) case "creating": cr.SetConditions(xpv1.Creating()) + default: + cr.SetConditions(xpv1.Unavailable().WithMessage("DB Cluster is " + aws.StringValue(resp.DBClusters[0].Status))) } obs.ConnectionDetails = managed.ConnectionDetails{ @@ -527,7 +531,7 @@ func generateRestoreDBClusterToPointInTimeInput(cr *svcapitypes.DBCluster) *svcs func isUpToDate(cr *svcapitypes.DBCluster, out *svcsdk.DescribeDBClustersOutput) (bool, error) { // nolint:gocyclo status := aws.StringValue(out.DBClusters[0].Status) - if status == "modifying" || status == "upgrading" || status == "configuring-iam-database-auth" || status == "migrating" || status == "prepairing-data-migration" { + if status == "modifying" || status == "upgrading" || status == "configuring-iam-database-auth" || status == "migrating" || status == "prepairing-data-migration" || status == "creating" { return true, nil } @@ -567,7 +571,12 @@ func isUpToDate(cr *svcapitypes.DBCluster, out *svcsdk.DescribeDBClustersOutput) return false, nil } - if !isVPCSecurityGroupIDsUpToDate(cr, out) { + if !areVPCSecurityGroupIDsUpToDate(cr, out) { + return false, nil + } + + if cr.Spec.ForProvider.DBClusterParameterGroupName != nil && + aws.StringValue(cr.Spec.ForProvider.DBClusterParameterGroupName) != aws.StringValue(out.DBClusters[0].DBClusterParameterGroup) { return false, nil } @@ -653,14 +662,20 @@ func isPortUpToDate(cr *svcapitypes.DBCluster, out *svcsdk.DescribeDBClustersOut return true } -func isVPCSecurityGroupIDsUpToDate(cr *svcapitypes.DBCluster, out *svcsdk.DescribeDBClustersOutput) bool { - // AWS uses "sg-563ab33d" which ich really restrictive as the default, and it seems to use it even when it is - // patched (with "required") - might be race condition. Anyway with checking if there is a diff we can rectify and - // even make it configurable after creation. +func areVPCSecurityGroupIDsUpToDate(cr *svcapitypes.DBCluster, out *svcsdk.DescribeDBClustersOutput) bool { + // AWS uses the default SG which is really restrictive, and it seems to use it even when it is + // patched (with "required") - might be race condition. Anyway with checking if there is a diff + // we can rectify and even make it configurable after creation. - actualGroups := out.DBClusters[0].VpcSecurityGroups desiredIDs := cr.Spec.ForProvider.VPCSecurityGroupIDs + // if user is fine with default SG (removing all SGs is not possible, AWS will keep last set SGs) + if len(desiredIDs) == 0 { + return true + } + + actualGroups := out.DBClusters[0].VpcSecurityGroups + if len(desiredIDs) != len(actualGroups) { return false } @@ -680,6 +695,20 @@ func preUpdate(_ context.Context, cr *svcapitypes.DBCluster, obj *svcsdk.ModifyD obj.DBClusterIdentifier = aws.String(meta.GetExternalName(cr)) obj.ApplyImmediately = cr.Spec.ForProvider.ApplyImmediately + if cr.Spec.ForProvider.VPCSecurityGroupIDs != nil { + obj.VpcSecurityGroupIds = make([]*string, len(cr.Spec.ForProvider.VPCSecurityGroupIDs)) + for i, v := range cr.Spec.ForProvider.VPCSecurityGroupIDs { + obj.VpcSecurityGroupIds[i] = aws.String(v) + } + } + + // ModifyDBCluster() returns error, when trying to upgrade major (minor is fine) EngineVersion: + // "Cannot change VPC security group while doing a major version upgrade." + // even when the provided VPCSecurityGroupIDs are upToDate... + // therefore EngineVersion update is entirely done separately in postUpdate + // Note: strangely ModifyDBInstance does not seem to behave this way + obj.EngineVersion = nil + return nil } @@ -688,6 +717,24 @@ func (u *updater) postUpdate(ctx context.Context, cr *svcapitypes.DBCluster, obj input := GenerateDescribeDBClustersInput(cr) resp, err := u.client.DescribeDBClustersWithContext(ctx, input) + if err != nil { + return managed.ExternalUpdate{}, aws.Wrap(cpresource.Ignore(IsNotFound, err), errDescribe) + } + + if !isEngineVersionUpToDate(cr, resp) { + // AWS does not want major EngineVersion upgrades in a ModifyDBCluster()-request + // that contains any value in VPCSecurityGroupIDs. + // Therefore doing here a separate call for EngineVersion changes only + _, err := u.client.ModifyDBClusterWithContext(ctx, &svcsdk.ModifyDBClusterInput{ + DBClusterIdentifier: aws.String(meta.GetExternalName(cr)), + EngineVersion: cr.Spec.ForProvider.EngineVersion, + AllowMajorVersionUpgrade: cr.Spec.ForProvider.AllowMajorVersionUpgrade, + ApplyImmediately: cr.Spec.ForProvider.ApplyImmediately, + }) + if err != nil { + return managed.ExternalUpdate{}, err + } + } tags := resp.DBClusters[0].TagList @@ -699,11 +746,7 @@ func (u *updater) postUpdate(ctx context.Context, cr *svcapitypes.DBCluster, obj return managed.ExternalUpdate{}, err } } - if err != nil { - if err != nil { - return managed.ExternalUpdate{}, aws.Wrap(cpresource.Ignore(IsNotFound, err), errDescribe) - } - } + if !isPreferredMaintenanceWindowUpToDate(cr, resp) { return upd, errors.New("PreferredMaintenanceWindow not matching aws data") } diff --git a/pkg/controller/rds/dbcluster/setup_test.go b/pkg/controller/rds/dbcluster/setup_test.go index 935478acab..6e01d43876 100644 --- a/pkg/controller/rds/dbcluster/setup_test.go +++ b/pkg/controller/rds/dbcluster/setup_test.go @@ -61,7 +61,7 @@ func TestIsVPCSecurityGroupIDsUpToDate(t *testing.T) { isUpToDate: false, }, }, - "DesiredEmpty": { + "DesiredBeingManged": { // AWS default or managed by DBCluster args: args{ cr: &svcapitypes.DBCluster{ Spec: svcapitypes.DBClusterSpec{ @@ -88,7 +88,7 @@ func TestIsVPCSecurityGroupIDsUpToDate(t *testing.T) { }, }, want: want{ - isUpToDate: false, + isUpToDate: true, }, }, "ActualEmpty": { @@ -178,7 +178,7 @@ func TestIsVPCSecurityGroupIDsUpToDate(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { - isUpToDate := isVPCSecurityGroupIDsUpToDate(tc.args.cr, tc.args.out) + isUpToDate := areVPCSecurityGroupIDsUpToDate(tc.args.cr, tc.args.out) if diff := cmp.Diff(tc.want.isUpToDate, isUpToDate); diff != "" { t.Errorf("r: -want, +got:\n%s", diff) diff --git a/pkg/controller/rds/dbcluster/zz_conversions.go b/pkg/controller/rds/dbcluster/zz_conversions.go index 8f975fd9f3..5b63918bd4 100644 --- a/pkg/controller/rds/dbcluster/zz_conversions.go +++ b/pkg/controller/rds/dbcluster/zz_conversions.go @@ -715,6 +715,9 @@ func GenerateModifyDBClusterInput(cr *svcapitypes.DBCluster) *svcsdk.ModifyDBClu if cr.Spec.ForProvider.AllocatedStorage != nil { res.SetAllocatedStorage(*cr.Spec.ForProvider.AllocatedStorage) } + if cr.Spec.ForProvider.AllowMajorVersionUpgrade != nil { + res.SetAllowMajorVersionUpgrade(*cr.Spec.ForProvider.AllowMajorVersionUpgrade) + } if cr.Spec.ForProvider.AutoMinorVersionUpgrade != nil { res.SetAutoMinorVersionUpgrade(*cr.Spec.ForProvider.AutoMinorVersionUpgrade) } diff --git a/pkg/controller/rds/dbinstance/setup.go b/pkg/controller/rds/dbinstance/setup.go index 38da740201..5c5d9716ba 100644 --- a/pkg/controller/rds/dbinstance/setup.go +++ b/pkg/controller/rds/dbinstance/setup.go @@ -4,7 +4,7 @@ import ( "context" "encoding/json" "log" - + "sort" "strconv" "strings" "time" @@ -31,7 +31,6 @@ import ( svcapitypes "github.com/crossplane-contrib/provider-aws/apis/rds/v1alpha1" "github.com/crossplane-contrib/provider-aws/apis/v1alpha1" aws "github.com/crossplane-contrib/provider-aws/pkg/clients" - awsclient "github.com/crossplane-contrib/provider-aws/pkg/clients" dbinstance "github.com/crossplane-contrib/provider-aws/pkg/clients/rds" "github.com/crossplane-contrib/provider-aws/pkg/features" ) @@ -132,18 +131,18 @@ func (e *custom) preCreate(ctx context.Context, cr *svcapitypes.DBInstance, obj case "S3": _, err := e.client.RestoreDBInstanceFromS3WithContext(ctx, dbinstance.GenerateRestoreDBInstanceFromS3Input(meta.GetExternalName(cr), pw, &cr.Spec.ForProvider)) if err != nil { - return awsclient.Wrap(err, errS3RestoreFailed) + return aws.Wrap(err, errS3RestoreFailed) } case "Snapshot": _, err := e.client.RestoreDBInstanceFromDBSnapshotWithContext(ctx, dbinstance.GenerateRestoreDBInstanceFromSnapshotInput(meta.GetExternalName(cr), &cr.Spec.ForProvider)) if err != nil { - return awsclient.Wrap(err, errSnapshotRestoreFailed) + return aws.Wrap(err, errSnapshotRestoreFailed) } case "PointInTime": _, err := e.client.RestoreDBInstanceToPointInTimeWithContext(ctx, dbinstance.GenerateRestoreDBInstanceToPointInTimeInput(meta.GetExternalName(cr), &cr.Spec.ForProvider)) if err != nil { - return awsclient.Wrap(err, errPointInTimeRestoreFailed) + return aws.Wrap(err, errPointInTimeRestoreFailed) } default: return errors.New(errUnknownRestoreSource) @@ -186,6 +185,13 @@ func (e *custom) preUpdate(ctx context.Context, cr *svcapitypes.DBInstance, obj obj.MasterUserPassword = aws.String(pw) } + if cr.Spec.ForProvider.VPCSecurityGroupIDs != nil { + obj.VpcSecurityGroupIds = make([]*string, len(cr.Spec.ForProvider.VPCSecurityGroupIDs)) + for i, v := range cr.Spec.ForProvider.VPCSecurityGroupIDs { + obj.VpcSecurityGroupIds[i] = aws.String(v) + } + } + return nil } @@ -218,13 +224,18 @@ func (e *custom) postObserve(ctx context.Context, cr *svcapitypes.DBInstance, re if err != nil { return managed.ExternalObservation{}, err } + switch aws.StringValue(resp.DBInstances[0].DBInstanceStatus) { - case "available", "modifying": + case "available", "configuring-enhanced-monitoring", "storage-optimization", "backing-up": cr.SetConditions(xpv1.Available()) - case "deleting", "stopped", "stopping": - cr.SetConditions(xpv1.Unavailable()) + case "modifying": + cr.SetConditions(xpv1.Available().WithMessage("DB Instance is " + aws.StringValue(resp.DBInstances[0].DBInstanceStatus) + ", availability may vary")) + case "deleting": + cr.SetConditions(xpv1.Deleting()) case "creating": cr.SetConditions(xpv1.Creating()) + default: + cr.SetConditions(xpv1.Unavailable().WithMessage("DB Instance is " + aws.StringValue(resp.DBInstances[0].DBInstanceStatus))) } obs.ConnectionDetails, _ = e.assembleConnectionDetails(ctx, cr) @@ -238,6 +249,8 @@ func lateInitialize(in *svcapitypes.DBInstanceParameters, out *svcsdk.DescribeDB in.Engine = aws.LateInitializeStringPtr(in.Engine, db.Engine) in.DBClusterIdentifier = aws.LateInitializeStringPtr(in.DBClusterIdentifier, db.DBClusterIdentifier) + // if the instance belongs to a cluster, these fields should not be lateinit, + // to allow the user to manage these via the cluster if in.DBClusterIdentifier == nil { in.AllocatedStorage = aws.LateInitializeInt64Ptr(in.AllocatedStorage, db.AllocatedStorage) in.BackupRetentionPeriod = aws.LateInitializeInt64Ptr(in.BackupRetentionPeriod, db.BackupRetentionPeriod) @@ -254,6 +267,20 @@ func lateInitialize(in *svcapitypes.DBInstanceParameters, out *svcsdk.DescribeDB if strings.HasPrefix(aws.StringValue(db.EngineVersion), aws.StringValue(in.EngineVersion)) { in.EngineVersion = db.EngineVersion } + if in.DBParameterGroupName == nil { + for i := range db.DBParameterGroups { + if db.DBParameterGroups[i].DBParameterGroupName != nil { + in.DBParameterGroupName = db.DBParameterGroups[i].DBParameterGroupName + break + } + } + } + if len(in.VPCSecurityGroupIDs) == 0 && len(db.VpcSecurityGroups) != 0 { + in.VPCSecurityGroupIDs = make([]string, len(db.VpcSecurityGroups)) + for i, val := range db.VpcSecurityGroups { + in.VPCSecurityGroupIDs[i] = aws.StringValue(val.VpcSecurityGroupId) + } + } } in.AutoMinorVersionUpgrade = aws.LateInitializeBoolPtr(in.AutoMinorVersionUpgrade, db.AutoMinorVersionUpgrade) in.AvailabilityZone = aws.LateInitializeStringPtr(in.AvailabilityZone, db.AvailabilityZone) @@ -304,20 +331,6 @@ func lateInitialize(in *svcapitypes.DBInstanceParameters, out *svcsdk.DescribeDB } } } - if len(in.VPCSecurityGroupIDs) == 0 && len(db.VpcSecurityGroups) != 0 { - in.VPCSecurityGroupIDs = make([]string, len(db.VpcSecurityGroups)) - for i, val := range db.VpcSecurityGroups { - in.VPCSecurityGroupIDs[i] = aws.StringValue(val.VpcSecurityGroupId) - } - } - if in.DBParameterGroupName == nil { - for i := range db.DBParameterGroups { - if db.DBParameterGroups[i].DBParameterGroupName != nil { - in.DBParameterGroupName = db.DBParameterGroups[i].DBParameterGroupName - break - } - } - } return nil } @@ -339,7 +352,7 @@ func (e *custom) isUpToDate(cr *svcapitypes.DBInstance, out *svcsdk.DescribeDBIn // This could be matured a bit more for specific statuses, such as not allowing storage changes // when the status is "storage-optimization" status := aws.StringValue(out.DBInstances[0].DBInstanceStatus) - if status == "modifying" || status == "upgrading" { + if status == "modifying" || status == "upgrading" || status == "rebooting" || status == "creating" { return true, nil } @@ -374,9 +387,15 @@ func (e *custom) isUpToDate(cr *svcapitypes.DBInstance, out *svcsdk.DescribeDBIn } + vpcSGsChanged := !areVPCSecurityGroupIDsUpToDate(cr, db) + + dbParameterGroupChanged := !isDBParameterGroupNameUpToDate(cr, db) + diff := cmp.Diff(&svcapitypes.DBInstanceParameters{}, patch, cmpopts.EquateEmpty(), cmpopts.IgnoreTypes(&xpv1.Reference{}, &xpv1.Selector{}, []xpv1.Reference{}), cmpopts.IgnoreFields(svcapitypes.DBInstanceParameters{}, "Region"), + cmpopts.IgnoreFields(svcapitypes.DBInstanceParameters{}, "AllowMajorVersionUpgrade"), + cmpopts.IgnoreFields(svcapitypes.DBInstanceParameters{}, "DBParameterGroupName"), cmpopts.IgnoreFields(svcapitypes.DBInstanceParameters{}, "EngineVersion"), cmpopts.IgnoreFields(svcapitypes.DBInstanceParameters{}, "Tags"), cmpopts.IgnoreFields(svcapitypes.DBInstanceParameters{}, "SkipFinalSnapshot"), @@ -387,9 +406,10 @@ func (e *custom) isUpToDate(cr *svcapitypes.DBInstance, out *svcsdk.DescribeDBIn cmpopts.IgnoreFields(svcapitypes.DBInstanceParameters{}, "PreferredBackupWindow"), cmpopts.IgnoreFields(svcapitypes.CustomDBInstanceParameters{}, "ApplyImmediately"), cmpopts.IgnoreFields(svcapitypes.CustomDBInstanceParameters{}, "RestoreFrom"), + cmpopts.IgnoreFields(svcapitypes.CustomDBInstanceParameters{}, "VPCSecurityGroupIDs"), ) - if diff == "" && !maintenanceWindowChanged && !backupWindowChanged && !pwChanged && !versionChanged { + if diff == "" && !maintenanceWindowChanged && !backupWindowChanged && !pwChanged && !versionChanged && !vpcSGsChanged && !dbParameterGroupChanged { return true, nil } @@ -458,6 +478,53 @@ func compareTimeRanges(format string, expectedWindow *string, actualWindow *stri return false, nil } +func areVPCSecurityGroupIDsUpToDate(cr *svcapitypes.DBInstance, out *svcsdk.DBInstance) bool { + desiredIDs := cr.Spec.ForProvider.VPCSecurityGroupIDs + + // if user is fine with default SG or lets DBCluster manage it + // (removing all SGs is not possible, AWS will keep last set SGs) + if len(desiredIDs) == 0 { + return false + } + + actualGroups := out.VpcSecurityGroups + + if len(desiredIDs) != len(actualGroups) { + return true + } + + actualIDs := make([]string, 0, len(actualGroups)) + for _, grp := range actualGroups { + actualIDs = append(actualIDs, *grp.VpcSecurityGroupId) + } + + sort.Strings(desiredIDs) + sort.Strings(actualIDs) + + return cmp.Equal(desiredIDs, actualIDs) +} + +func isDBParameterGroupNameUpToDate(cr *svcapitypes.DBInstance, out *svcsdk.DBInstance) bool { + desiredGroup := cr.Spec.ForProvider.DBParameterGroupName + + // if user is fine with default DBParameterGroup or lets DBCluster manage it + if desiredGroup == nil { + return true + } + + actualGroups := out.DBParameterGroups + + for _, grp := range actualGroups { + + if aws.StringValue(grp.DBParameterGroupName) == aws.StringValue(desiredGroup) { + return true + } + + } + + return false +} + func filterList(cr *svcapitypes.DBInstance, obj *svcsdk.DescribeDBInstancesOutput) *svcsdk.DescribeDBInstancesOutput { resp := &svcsdk.DescribeDBInstancesOutput{} for _, dbInstance := range obj.DBInstances { diff --git a/pkg/controller/rds/dbinstance/zz_conversions.go b/pkg/controller/rds/dbinstance/zz_conversions.go index b987058a3d..0129fae715 100644 --- a/pkg/controller/rds/dbinstance/zz_conversions.go +++ b/pkg/controller/rds/dbinstance/zz_conversions.go @@ -897,6 +897,9 @@ func GenerateModifyDBInstanceInput(cr *svcapitypes.DBInstance) *svcsdk.ModifyDBI if cr.Spec.ForProvider.AllocatedStorage != nil { res.SetAllocatedStorage(*cr.Spec.ForProvider.AllocatedStorage) } + if cr.Spec.ForProvider.AllowMajorVersionUpgrade != nil { + res.SetAllowMajorVersionUpgrade(*cr.Spec.ForProvider.AllowMajorVersionUpgrade) + } if cr.Spec.ForProvider.AutoMinorVersionUpgrade != nil { res.SetAutoMinorVersionUpgrade(*cr.Spec.ForProvider.AutoMinorVersionUpgrade) } From 9fc98b3300f5ed8544add5fdeb8e9277de67af3b Mon Sep 17 00:00:00 2001 From: Charel Baum Date: Tue, 20 Dec 2022 10:30:11 +0100 Subject: [PATCH 2/2] fix(dbinstance): correct return values Signed-off-by: Charel Baum --- pkg/controller/rds/dbinstance/setup.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/controller/rds/dbinstance/setup.go b/pkg/controller/rds/dbinstance/setup.go index 5c5d9716ba..7e2a098f8a 100644 --- a/pkg/controller/rds/dbinstance/setup.go +++ b/pkg/controller/rds/dbinstance/setup.go @@ -484,13 +484,13 @@ func areVPCSecurityGroupIDsUpToDate(cr *svcapitypes.DBInstance, out *svcsdk.DBIn // if user is fine with default SG or lets DBCluster manage it // (removing all SGs is not possible, AWS will keep last set SGs) if len(desiredIDs) == 0 { - return false + return true } actualGroups := out.VpcSecurityGroups if len(desiredIDs) != len(actualGroups) { - return true + return false } actualIDs := make([]string, 0, len(actualGroups))