Merge pull request #2073 from wotolom/docdb-dbcluster-allow-major-ver…

…sion-upgrade

feat(docdb): allow (major) version upgrades

Makefile

@@ -47,7 +47,7 @@ GOLANGCILINT_VERSION = 1.54.2
 
 UP_VERSION = v0.27.0
 UP_CHANNEL = stable
-UPTEST_VERSION = v0.12.0
+UPTEST_VERSION = v0.11.0
 
 -include build/makelib/k8s_tools.mk
 

apis/docdb/generator-config.yaml

@@ -1,3 +1,16 @@
+resources:
+  DBCluster:
+    fields:
+      AllowMajorVersionUpgrade:
+        from:
+          operation: ModifyDBCluster
+          path: AllowMajorVersionUpgrade
+      EngineVersion:
+        is_read_only: true
+        from:
+          operation: DescribeDBClusters
+          path: DBClusters.EngineVersion
+
 ignore:
   field_paths:
     - DescribeDBClusterParameterGroupsInput.DBClusterParameterGroupName

apis/docdb/v1alpha1/custom_types.go

@@ -31,6 +31,8 @@ const (
 	DocDBInstanceStateDeleting = "deleting"
 	// The instance is being modified.
 	DocDBInstanceStateModifying = "modifying"
+	// The instance is being upgraded.
+	DocDBInstanceStateUpgrading = "upgrading"
 	// 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.
 	DocDBInstanceStateFailed = "failed"
 )

package/crds/docdb.aws.crossplane.io_dbclusters.yaml

@@ -73,6 +73,15 @@ spec:
               forProvider:
                 description: DBClusterParameters defines the desired state of DBCluster
                 properties:
+                  allowMajorVersionUpgrade:
+                    description: |-
+                      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.
+                    type: boolean
                   applyImmediately:
                     description: |-
                       A value that specifies whether the changes in this request and any pending
@@ -984,6 +993,9 @@ spec:
                     description: Specifies the connection endpoint for the primary
                       instance of the cluster.
                     type: string
+                  engineVersion:
+                    description: Indicates the database engine version.
+                    type: string
                   hostedZoneID:
                     description: Specifies the ID that Amazon Route 53 assigns when
                       you create a hosted zone.

pkg/controller/docdb/dbcluster/setup.go

@@ -31,6 +31,7 @@ import (
 	"github.com/crossplane/crossplane-runtime/pkg/password"
 	"github.com/crossplane/crossplane-runtime/pkg/reconciler/managed"
 	"github.com/crossplane/crossplane-runtime/pkg/resource"
+	cpresource "github.com/crossplane/crossplane-runtime/pkg/resource"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -40,8 +41,10 @@ import (
 
 	svcapitypes "github.com/crossplane-contrib/provider-aws/apis/docdb/v1alpha1"
 	"github.com/crossplane-contrib/provider-aws/apis/v1alpha1"
+	"github.com/crossplane-contrib/provider-aws/pkg/controller/docdb/utils"
 	svcutils "github.com/crossplane-contrib/provider-aws/pkg/controller/docdb/utils"
 	"github.com/crossplane-contrib/provider-aws/pkg/features"
+	errorutils "github.com/crossplane-contrib/provider-aws/pkg/utils/errors"
 	"github.com/crossplane-contrib/provider-aws/pkg/utils/pointer"
 	custommanaged "github.com/crossplane-contrib/provider-aws/pkg/utils/reconciler/managed"
 )
@@ -121,6 +124,9 @@ func (e *hooks) postObserve(ctx context.Context, cr *svcapitypes.DBCluster, resp
 		return managed.ExternalObservation{}, err
 	}
 
+	cluster := resp.DBClusters[0]
+	cr.Status.AtProvider.EngineVersion = cluster.EngineVersion
+
 	obs.ConnectionDetails = getConnectionDetails(cr)
 
 	if !meta.WasDeleted(cr) {
@@ -177,6 +183,10 @@ func lateInitialize(cr *svcapitypes.DBClusterParameters, resp *svcsdk.DescribeDB
 func (e *hooks) isUpToDate(ctx context.Context, cr *svcapitypes.DBCluster, resp *svcsdk.DescribeDBClustersOutput) (bool, string, error) {
 	cluster := resp.DBClusters[0]
 
+	if pointer.StringValue(cluster.Status) == svcapitypes.DocDBInstanceStateModifying || pointer.StringValue(cluster.Status) == svcapitypes.DocDBInstanceStateUpgrading {
+		return true, "", nil
+	}
+
 	_, pwChanged, err := e.getPasswordFromRef(ctx, cr.Spec.ForProvider.MasterUserPasswordSecretRef, cr.Spec.WriteConnectionSecretToReference)
 	if err != nil || pwChanged {
 		return false, "", err
@@ -187,6 +197,7 @@ func (e *hooks) isUpToDate(ctx context.Context, cr *svcapitypes.DBCluster, resp
 		pointer.StringValue(cr.Spec.ForProvider.DBClusterParameterGroupName) != pointer.StringValue(cluster.DBClusterParameterGroup),
 		pointer.BoolValue(cr.Spec.ForProvider.DeletionProtection) != pointer.BoolValue(cluster.DeletionProtection),
 		!areSameElements(cr.Spec.ForProvider.EnableCloudwatchLogsExports, cluster.EnabledCloudwatchLogsExports),
+		!isEngineVersionUpToDate(cr, resp),
 		pointer.Int64Value(cr.Spec.ForProvider.Port) != pointer.Int64Value(cluster.Port),
 		pointer.StringValue(cr.Spec.ForProvider.PreferredBackupWindow) != pointer.StringValue(cluster.PreferredBackupWindow),
 		pointer.StringValue(cr.Spec.ForProvider.PreferredMaintenanceWindow) != pointer.StringValue(cluster.PreferredMaintenanceWindow):
@@ -211,14 +222,53 @@ func (e *hooks) preUpdate(ctx context.Context, cr *svcapitypes.DBCluster, obj *s
 	if pwchanged {
 		obj.MasterUserPassword = aws.String(pw)
 	}
+
+	// ModifyDBCluster() returns error, when trying to upgrade major (minor is fine) EngineVersion:
+	// "Cannot change VPC security group while doing a major version upgrade."
+	// therefore EngineVersion update is entirely done separately in postUpdate
+	obj.EngineVersion = nil
+	// In case of a custom DBClusterParameterGroup, AWS requires for a major version update that
+	// EngineVersion and DBClusterParameterGroupName are in the same ModifyDBCluster()-call
+	obj.DBClusterParameterGroupName = nil
+
 	return nil
 }
 
-func (e *hooks) postUpdate(_ context.Context, cr *svcapitypes.DBCluster, resp *svcsdk.ModifyDBClusterOutput, upd managed.ExternalUpdate, err error) (managed.ExternalUpdate, error) {
+func (e *hooks) postUpdate(ctx context.Context, cr *svcapitypes.DBCluster, resp *svcsdk.ModifyDBClusterOutput, upd managed.ExternalUpdate, err error) (managed.ExternalUpdate, error) {
 	if err != nil {
 		return managed.ExternalUpdate{}, err
 	}
 
+	input := GenerateDescribeDBClustersInput(cr)
+	// GenerateDescribeDBClustersInput returns an empty DescribeDBClustersInput
+	// and the function is generated by ack-generate, so we manually need to set the
+	// DBClusterIdentifier
+	input.DBClusterIdentifier = pointer.ToOrNilIfZeroValue(meta.GetExternalName(cr))
+	out, err := e.client.DescribeDBClustersWithContext(ctx, input)
+	if err != nil {
+		return managed.ExternalUpdate{}, errorutils.Wrap(cpresource.Ignore(IsNotFound, err), errDescribe)
+	}
+
+	needsEngineVersionUpdate := !isEngineVersionUpToDate(cr, out)
+	needsDBClusterParamGroupUpdate := pointer.StringValue(cr.Spec.ForProvider.DBClusterParameterGroupName) != pointer.StringValue(out.DBClusters[0].DBClusterParameterGroup)
+	needsPostUpdate := needsEngineVersionUpdate || needsDBClusterParamGroupUpdate
+
+	if needsPostUpdate {
+		modifyInput := &svcsdk.ModifyDBClusterInput{
+			DBClusterIdentifier:         pointer.ToOrNilIfZeroValue(meta.GetExternalName(cr)),
+			ApplyImmediately:            cr.Spec.ForProvider.ApplyImmediately,
+			DBClusterParameterGroupName: cr.Spec.ForProvider.DBClusterParameterGroupName,
+		}
+		if needsEngineVersionUpdate {
+			modifyInput.EngineVersion = cr.Spec.ForProvider.EngineVersion
+			modifyInput.AllowMajorVersionUpgrade = cr.Spec.ForProvider.AllowMajorVersionUpgrade
+		}
+
+		if _, err = e.client.ModifyDBClusterWithContext(ctx, modifyInput); err != nil {
+			return managed.ExternalUpdate{}, err
+		}
+	}
+
 	return upd, svcutils.UpdateTagsForResource(e.client, cr.Spec.ForProvider.Tags, resp.DBCluster.DBClusterArn)
 }
 
@@ -442,6 +492,22 @@ func areSameElements(a1, a2 []*string) bool {
 	return true
 }
 
+func isEngineVersionUpToDate(cr *svcapitypes.DBCluster, out *svcsdk.DescribeDBClustersOutput) bool {
+	// If EngineVersion is not set, AWS sets a default value,
+	// so we do not try to update in this case
+	if cr.Spec.ForProvider.EngineVersion != nil {
+		if out.DBClusters[0].EngineVersion == nil {
+			return false
+		}
+
+		// Upgrade is only necessary if the spec version is higher.
+		// Downgrades are not possible in pointer.
+		c := utils.CompareEngineVersions(*cr.Spec.ForProvider.EngineVersion, *out.DBClusters[0].EngineVersion)
+		return c <= 0
+	}
+	return true
+}
+
 func (e *hooks) getPasswordFromRef(ctx context.Context, in *xpv1.SecretKeySelector, out *xpv1.SecretReference) (newPwd string, changed bool, err error) {
 	if in == nil {
 		return "", false, nil