Skip to content

Commit

Permalink
feat(docdb): allow (major) version upgrades
Browse files Browse the repository at this point in the history
Signed-off-by: Charel Baum (external expert on behalf of DB InfraGO AG) <[email protected]>
  • Loading branch information
Charel Baum (external expert on behalf of DB InfraGO AG) committed Jul 4, 2024
1 parent d17c9da commit 1a2c379
Show file tree
Hide file tree
Showing 8 changed files with 259 additions and 1 deletion.
8 changes: 8 additions & 0 deletions apis/docdb/generator-config.yaml
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
resources:
DBCluster:
fields:
AllowMajorVersionUpgrade:
from:
operation: ModifyDBCluster
path: AllowMajorVersionUpgrade

ignore:
field_paths:
- DescribeDBClusterParameterGroupsInput.DBClusterParameterGroupName
Expand Down
6 changes: 6 additions & 0 deletions apis/docdb/v1alpha1/zz_db_cluster.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions apis/docdb/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions package/crds/docdb.aws.crossplane.io_dbclusters.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
61 changes: 60 additions & 1 deletion pkg/controller/docdb/dbcluster/setup.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
)
Expand Down Expand Up @@ -187,6 +190,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):
Expand All @@ -211,14 +215,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)
}

Expand Down Expand Up @@ -442,6 +485,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
Expand Down
3 changes: 3 additions & 0 deletions pkg/controller/docdb/dbcluster/zz_conversions.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

85 changes: 85 additions & 0 deletions pkg/controller/docdb/utils/engine_version.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
package utils

import (
"strconv"
"strings"
)

// EngineVersion represents an AWS DocDB engine version.
type EngineVersion []any

// ParseEngineVersion from a raw string.
func ParseEngineVersion(raw string) EngineVersion {
split := strings.Split(raw, ".")

v := make(EngineVersion, len(split))
for i, s := range split {
d, err := strconv.Atoi(s)
if err != nil {
v[i] = s
} else {
v[i] = d
}
}
return v
}

const (
compareIsHigher = 1
compareIsEqual = 0
compareIsLower = -1
)

// Compare returns a positive value if v is represents a higher version number
// than other. A negative value is returned if other is higher than v.
// It returns 0 if both are considered equal.
func (v EngineVersion) Compare(other EngineVersion) int {
if other == nil {
return compareIsHigher
}

for i := 0; i < len(v); i++ {
a := v.get(i)
b := other.get(i)
c := compareVersionComponents(a, b)
if c != 0 {
return c
}
}
return compareIsEqual
}

func compareVersionComponents(a, b any) int {
if a == b {
return compareIsEqual
}
if b == nil {
return compareIsHigher
}
aI, aIsInt := a.(int)
bI, bIsInt := b.(int)
if aIsInt {
if bIsInt {
return aI - bI
}
return compareIsHigher
}
if bIsInt {
return compareIsLower
}
return compareIsEqual // We cannot decide if both are strings.
}

func (v EngineVersion) get(i int) any {
if i >= 0 && i < len(v) {
return v[i]
}
return nil
}

// CompareEngineVersions is a shortcut to compare two engine versions.
func CompareEngineVersions(a, b string) int {
av := ParseEngineVersion(a)
bv := ParseEngineVersion(b)
return av.Compare(bv)
}
83 changes: 83 additions & 0 deletions pkg/controller/docdb/utils/engine_version_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
package utils

import (
"testing"

"github.com/google/go-cmp/cmp"
)

func TestEngineVersionIsHigherOrEqual(t *testing.T) {
type args struct {
spec string
current string
}

type want struct {
result int
}

cases := map[string]struct {
args
want
}{
"DocDBIsEqual": {
args: args{
spec: "5.0.0",
current: "5.0.0",
},
want: want{
result: 0,
},
},
"DocDBIsEqual2": {
args: args{
spec: "5.0",
current: "5.0.0",
},
want: want{
result: 0,
},
},
"DocDBIsHigher": {
args: args{
spec: "5.0",
current: "4.0.0",
},
want: want{
result: 1,
},
},
"DocDBIsLower": {
args: args{
spec: "4.0.0",
current: "5.0.0",
},
want: want{
result: -1,
},
},
}

for name, tc := range cases {
t.Run(name, func(t *testing.T) {
specV := ParseEngineVersion(tc.args.spec)
curV := ParseEngineVersion(tc.args.current)

res := specV.Compare(curV)
resSign := sign(res)
if diff := cmp.Diff(tc.want.result, resSign); diff != "" {
t.Errorf("r: -want, +got:\n%q\n%q\n%s", tc.args.spec, tc.args.current, diff)
}
})
}
}

func sign(x int) int {
if x < 0 {
return -1
}
if x > 0 {
return 1
}
return 0
}

0 comments on commit 1a2c379

Please sign in to comment.