Merge pull request #43 from echen-98/maintenance-window

fix update of maintenance window

Author: kumargauravsharma

apis/v1alpha1/ack-generate-metadata.yaml

@@ -1,14 +1,14 @@
 ack_generate_info:
-  build_date: "2021-07-16T03:54:07Z"
+  build_date: "2021-07-16T19:55:47Z"
   build_hash: 7832e9aa4a48565302cd440f4cdf2267f04adfed
   go_version: go1.15.2 darwin/amd64
   version: v0.5.0
 api_directory_checksum: 04701e412e7e4597466c1d56571be2c5de2b1e27
 api_version: v1alpha1
 aws_sdk_go_version: ""
 generator_config_info:
-  file_checksum: cc29e8be7c6f65ef2f8db75fc3752adbf18590df
+  file_checksum: 821cc69bbfdafc0bb1302bb609f7b92774448303
   original_file_name: generator.yaml
 last_modification:
   reason: API generation
-  timestamp: 2021-07-16 03:54:15.20368 +0000 UTC
+  timestamp: 2021-07-16 19:55:56.542005 +0000 UTC

apis/v1alpha1/generator.yaml

@@ -69,6 +69,8 @@ resources:
         template_path: hooks/sdk_delete_post_request.go.tpl
       sdk_update_post_build_request:
         template_path: hooks/sdk_update_post_build_request.go.tpl
+      delta_post_compare:
+        code: "filterDelta(delta, a, b)"
   Snapshot:
     update_conditions_custom_method_name: CustomUpdateConditions
     exceptions:

generator.yaml

@@ -69,6 +69,8 @@ resources:
         template_path: hooks/sdk_delete_post_request.go.tpl
       sdk_update_post_build_request:
         template_path: hooks/sdk_update_post_build_request.go.tpl
+      delta_post_compare:
+        code: "filterDelta(delta, a, b)"
   Snapshot:
     update_conditions_custom_method_name: CustomUpdateConditions
     exceptions:

pkg/common/delta.go

@@ -0,0 +1,42 @@
+// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License"). You may
+// not use this file except in compliance with the License. A copy of the
+// License is located at
+//
+//     http://aws.amazon.com/apache2.0/
+//
+// or in the "license" file accompanying this file. This file is distributed
+// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+// express or implied. See the License for the specific language governing
+// permissions and limitations under the License.
+
+package common
+
+import ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"
+
+// remove the Difference corresponding to the given subject from the delta struct
+//TODO: ideally this would have a common implementation in compare/delta.go
+func RemoveFromDelta(
+	delta *ackcompare.Delta,
+	subject string,
+) {
+	// copy slice
+	differences := delta.Differences
+
+	// identify index of Difference to remove
+	//TODO: this could require a stricter Path.Equals down the road
+	var i *int = nil
+	for j, diff := range differences {
+		if diff.Path.Contains(subject) {
+			i = &j
+			break
+		}
+	}
+
+	// if found, create a new slice and replace the original
+	if i != nil {
+		differences = append(differences[:*i], differences[*i+1:]...)
+		delta.Differences = differences
+	}
+}

pkg/resource/replication_group/custom_update_api.go

@@ -84,13 +84,13 @@ func (rm *resourceManager) CustomModifyReplicationGroup(
 	if desired.ko.Spec.AutomaticFailoverEnabled != nil && *desired.ko.Spec.AutomaticFailoverEnabled == false {
 		latestAutomaticFailoverEnabled := latest.ko.Status.AutomaticFailover != nil && *latest.ko.Status.AutomaticFailover == "enabled"
 		if latestAutomaticFailoverEnabled != *desired.ko.Spec.AutomaticFailoverEnabled {
-			return rm.modifyReplicationGroup(ctx, desired, latest)
+			return rm.modifyReplicationGroup(ctx, desired, latest, delta)
 		}
 	}
 	if desired.ko.Spec.MultiAZEnabled != nil && *desired.ko.Spec.MultiAZEnabled == false {
 		latestMultiAZEnabled := latest.ko.Status.MultiAZ != nil && *latest.ko.Status.MultiAZ == "enabled"
 		if latestMultiAZEnabled != *desired.ko.Spec.MultiAZEnabled {
-			return rm.modifyReplicationGroup(ctx, desired, latest)
+			return rm.modifyReplicationGroup(ctx, desired, latest, delta)
 		}
 	}
 
@@ -107,7 +107,7 @@ func (rm *resourceManager) CustomModifyReplicationGroup(
 		return rm.updateShardConfiguration(ctx, desired, latest)
 	}
 
-	return rm.modifyReplicationGroup(ctx, desired, latest)
+	return rm.modifyReplicationGroup(ctx, desired, latest, delta)
 }
 
 // modifyReplicationGroup updates replication group
@@ -118,6 +118,7 @@ func (rm *resourceManager) modifyReplicationGroup(
 	ctx context.Context,
 	desired *resource,
 	latest *resource,
+	delta *ackcompare.Delta,
 ) (*resource, error) {
 	// Method currently handles SecurityGroupIDs, EngineVersion
 	// Avoid making unnecessary DescribeCacheCluster API call if both fields are nil in spec.
@@ -134,8 +135,8 @@ func (rm *resourceManager) modifyReplicationGroup(
 
 	// SecurityGroupIds, EngineVersion
 	if rm.securityGroupIdsDiffer(desired, latest, latestCacheCluster) ||
-		rm.engineVersionsDiffer(desired, latest) {
-		input := rm.newModifyReplicationGroupRequestPayload(desired, latest, latestCacheCluster)
+		delta.DifferentAt("Spec.EngineVersion") {
+		input := rm.newModifyReplicationGroupRequestPayload(desired, latest, latestCacheCluster, delta)
 		resp, respErr := rm.sdkapi.ModifyReplicationGroupWithContext(ctx, input)
 		rm.metrics.RecordAPICall("UPDATE", "ModifyReplicationGroup", respErr)
 		if respErr != nil {
@@ -648,6 +649,7 @@ func (rm *resourceManager) newModifyReplicationGroupRequestPayload(
 	desired *resource,
 	latest *resource,
 	latestCacheCluster *svcsdk.CacheCluster,
+	delta *ackcompare.Delta,
 ) *svcsdk.ModifyReplicationGroupInput {
 	input := &svcsdk.ModifyReplicationGroupInput{}
 
@@ -667,37 +669,14 @@ func (rm *resourceManager) newModifyReplicationGroupRequestPayload(
 		input.SetSecurityGroupIds(ids)
 	}
 
-	if rm.engineVersionsDiffer(desired, latest) &&
+	if delta.DifferentAt("Spec.EngineVersion") &&
 		desired.ko.Spec.EngineVersion != nil {
 		input.SetEngineVersion(*desired.ko.Spec.EngineVersion)
 	}
 
 	return input
 }
 
-/*
-engineVersionsDiffer returns true if the desired engine version is different
-from the latest observed engine version, and false if they differ or if
-the desired EngineVersion is nil
-*/
-func (rm *resourceManager) engineVersionsDiffer(
-	desired *resource,
-	latest *resource,
-) bool {
-	if desired.ko.Spec.EngineVersion == nil {
-		return false
-	}
-
-	latestEV := ""
-	if latest.ko.Spec.EngineVersion != nil {
-		latestEV = *latest.ko.Spec.EngineVersion
-	}
-
-	return *desired.ko.Spec.EngineVersion != latestEV
-
-	//TODO: should Delta be used in this function?
-}
-
 // This method copies the data from given replicationGroup by populating it into copy of supplied resource
 // and returns it.
 func (rm *resourceManager) provideUpdatedResource(

pkg/resource/replication_group/custom_update_api_test.go

@@ -942,57 +942,3 @@ func provideCacheClusterSecurityGroups(IDs ...string) []*svcsdk.SecurityGroupMem
 	}
 	return securityGroups
 }
-
-// TestEngineVersionsDiffer tests scenarios to check if desired, latest (from cache cluster)
-// Engine Version configuration differs.
-func TestEngineVersionsDiffer(t *testing.T) {
-	assert := assert.New(t)
-	require := require.New(t)
-	// setup
-	rm := provideResourceManager()
-	// Tests
-	t.Run("NoDiff=NoSpec_NoStatus", func(t *testing.T) {
-		desiredRG := provideResource()
-		latestRG := provideResource()
-		require.Nil(desiredRG.ko.Spec.EngineVersion)
-		require.Nil(latestRG.ko.Spec.EngineVersion)
-		differ := rm.engineVersionsDiffer(desiredRG, latestRG)
-		assert.False(differ)
-	})
-	t.Run("NoDiff=OnlyDesiredNil", func(t *testing.T) {
-		desiredRG := provideResource()
-		latestRG := provideResource()
-		latestEV := "test-engine-version"
-		latestRG.ko.Spec.EngineVersion = &latestEV
-		require.Nil(desiredRG.ko.Spec.EngineVersion)
-		require.NotNil(latestRG.ko.Spec.EngineVersion)
-		differ := rm.engineVersionsDiffer(desiredRG, latestRG)
-		assert.False(differ)
-	})
-	t.Run("NoDiff=Desired_Latest_Match", func(t *testing.T) {
-		desiredRG := provideResource()
-		latestRG := provideResource()
-		latestEV := "test-engine-version"
-		desiredRG.ko.Spec.EngineVersion = &latestEV
-		latestRG.ko.Spec.EngineVersion = &latestEV
-		require.NotNil(desiredRG.ko.Spec.EngineVersion)
-		require.NotNil(latestRG.ko.Spec.EngineVersion)
-		differ := rm.engineVersionsDiffer(desiredRG, latestRG)
-		assert.False(differ)
-	})
-	t.Run("Diff=Desired_Latest_Mismatch", func(t *testing.T) {
-		desiredRG := provideResource()
-		latestRG := provideResource()
-		desiredEV := "desired-test-engine-version"
-		latestEV := "latest-test-engine-version"
-
-		desiredRG.ko.Spec.EngineVersion = &desiredEV
-		latestRG.ko.Spec.EngineVersion = &latestEV
-
-		require.NotNil(desiredRG.ko.Spec.EngineVersion)
-		require.NotNil(latestRG.ko.Spec.EngineVersion)
-		require.NotEqual(*desiredRG.ko.Spec.EngineVersion, *latestRG.ko.Spec.EngineVersion)
-		differ := rm.engineVersionsDiffer(desiredRG, latestRG)
-		assert.True(differ)
-	})
-}

pkg/resource/replication_group/delta.go

@@ -0,0 +1,64 @@
+// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License"). You may
+// not use this file except in compliance with the License. A copy of the
+// License is located at
+//
+//     http://aws.amazon.com/apache2.0/
+//
+// or in the "license" file accompanying this file. This file is distributed
+// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+// express or implied. See the License for the specific language governing
+// permissions and limitations under the License.
+
+package replication_group
+
+import (
+	"strings"
+	"regexp"
+
+	ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"
+
+	"github.com/aws-controllers-k8s/elasticache-controller/pkg/common"
+)
+
+// remove non-meaningful differences from delta
+func filterDelta(
+	delta *ackcompare.Delta,
+	desired *resource,
+	latest *resource,
+) {
+
+	if delta.DifferentAt("Spec.EngineVersion") {
+		if desired.ko.Spec.EngineVersion != nil && latest.ko.Spec.EngineVersion != nil {
+			if engineVersionsMatch(*desired.ko.Spec.EngineVersion, *latest.ko.Spec.EngineVersion) {
+				common.RemoveFromDelta(delta, "Spec.EngineVersion")
+			}
+		}
+		// TODO: handle the case of a nil difference (especially when desired EV is nil)
+	}
+}
+
+// returns true if desired and latest engine versions match and false otherwise
+// precondition: both desiredEV and latestEV are non-nil
+// this handles the case where only the major EV is specified, e.g. "6.x" (or similar), but the latest
+//   version shows the minor version, e.g. "6.0.5"
+func engineVersionsMatch(
+	desiredEV string,
+	latestEV string,
+) bool {
+	if desiredEV == latestEV {
+		return true
+	}
+
+	// if the last character of desiredEV is "x", only check for a major version match
+	last := len(desiredEV) - 1
+	if desiredEV[last:] == "x" {
+		// cut off the "x" and replace all occurrences of '.' with '\.' (as '.' is a special regex character)
+		desired := strings.Replace(desiredEV[:last], ".", "\\.", -1)
+		r, _ := regexp.Compile(desired + ".*")
+		return r.MatchString(latestEV)
+	}
+
+	return false
+}

pkg/resource/replication_group/delta_util.go

@@ -0,0 +1,26 @@
+// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License"). You may
+// not use this file except in compliance with the License. A copy of the
+// License is located at
+//
+//     http://aws.amazon.com/apache2.0/
+//
+// or in the "license" file accompanying this file. This file is distributed
+// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+// express or implied. See the License for the specific language governing
+// permissions and limitations under the License.
+
+package replication_group
+
+import "testing"
+import "github.com/stretchr/testify/require"
+
+func TestEngineVersionsMatch(t *testing.T) {
+	require := require.New(t)
+
+	require.True(engineVersionsMatch("6.x", "6.0.5"))
+	require.False(engineVersionsMatch("13.x", "6.0.6"))
+	require.True(engineVersionsMatch("5.0.3", "5.0.3"))
+	require.False(engineVersionsMatch("5.0.3", "5.0.4"))
+}

pkg/resource/replication_group/delta_util_test.go

@@ -42,9 +42,13 @@ func (rm *resourceManager) updateSpecFields(
 	latestCacheCluster, err := rm.describeCacheCluster(ctx, resource)
 	if err == nil && latestCacheCluster != nil {
 		setEngineVersion(latestCacheCluster, resource)
+		setMaintenanceWindow(latestCacheCluster, resource)
 	}
 }
 
+//TODO: for all the fields here, reevaluate if the latest observed state should always be populated,
+// even if the corresponding field was not specified in desired
+
 // if ReplicasPerNodeGroup was given in desired.Spec, update ko.Spec with the latest observed value
 func setReplicasPerNodeGroup(
 	respRG *svcsdk.ReplicationGroup,
@@ -72,3 +76,15 @@ func setEngineVersion(
 		*ko.Spec.EngineVersion = *latestCacheCluster.EngineVersion
 	}
 }
+
+// update maintenance window (if non-nil in API response) regardless of whether it was specified in desired
+func setMaintenanceWindow(
+	latestCacheCluster *svcsdk.CacheCluster,
+	resource *resource,
+) {
+	ko := resource.ko
+	if latestCacheCluster.PreferredMaintenanceWindow != nil {
+		pmw := *latestCacheCluster.PreferredMaintenanceWindow
+		ko.Spec.PreferredMaintenanceWindow = &pmw
+	}
+}