Merge pull request #1598 from wotolom/rdsinstane-allow-major-version-…

…upgrade

Fixes #1330

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:

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

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

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

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

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

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")
 		}

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)