diff --git a/apis/v1alpha1/ack-generate-metadata.yaml b/apis/v1alpha1/ack-generate-metadata.yaml index 026912cf..01c315d5 100755 --- a/apis/v1alpha1/ack-generate-metadata.yaml +++ b/apis/v1alpha1/ack-generate-metadata.yaml @@ -1,5 +1,5 @@ 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 @@ -7,8 +7,8 @@ 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 diff --git a/apis/v1alpha1/generator.yaml b/apis/v1alpha1/generator.yaml index 5aedf654..39b1cd53 100644 --- a/apis/v1alpha1/generator.yaml +++ b/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: diff --git a/generator.yaml b/generator.yaml index 5aedf654..39b1cd53 100644 --- a/generator.yaml +++ b/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: diff --git a/pkg/common/delta.go b/pkg/common/delta.go new file mode 100644 index 00000000..7f367d00 --- /dev/null +++ b/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 + } +} diff --git a/pkg/resource/replication_group/custom_update_api.go b/pkg/resource/replication_group/custom_update_api.go index b4e021f6..002107ea 100644 --- a/pkg/resource/replication_group/custom_update_api.go +++ b/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,7 +669,7 @@ 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) } @@ -675,29 +677,6 @@ func (rm *resourceManager) newModifyReplicationGroupRequestPayload( 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( diff --git a/pkg/resource/replication_group/custom_update_api_test.go b/pkg/resource/replication_group/custom_update_api_test.go index 26a8cfa3..665c041e 100644 --- a/pkg/resource/replication_group/custom_update_api_test.go +++ b/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) - }) -} diff --git a/pkg/resource/replication_group/delta.go b/pkg/resource/replication_group/delta.go index 5a54a87d..006bc50a 100644 --- a/pkg/resource/replication_group/delta.go +++ b/pkg/resource/replication_group/delta.go @@ -223,5 +223,6 @@ func newResourceDelta( delta.Add("Spec.UserGroupIDs", a.ko.Spec.UserGroupIDs, b.ko.Spec.UserGroupIDs) } + filterDelta(delta, a, b) return delta } diff --git a/pkg/resource/replication_group/delta_util.go b/pkg/resource/replication_group/delta_util.go new file mode 100644 index 00000000..9b712620 --- /dev/null +++ b/pkg/resource/replication_group/delta_util.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 +} diff --git a/pkg/resource/replication_group/delta_util_test.go b/pkg/resource/replication_group/delta_util_test.go new file mode 100644 index 00000000..a1eaafdb --- /dev/null +++ b/pkg/resource/replication_group/delta_util_test.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")) +} diff --git a/pkg/resource/replication_group/post_set_output.go b/pkg/resource/replication_group/post_set_output.go index 6a5dadd4..fc150f54 100644 --- a/pkg/resource/replication_group/post_set_output.go +++ b/pkg/resource/replication_group/post_set_output.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 + } +} \ No newline at end of file diff --git a/pkg/resource/replication_group/testdata/replication_group/cr/rg_cmd_create_completed.yaml b/pkg/resource/replication_group/testdata/replication_group/cr/rg_cmd_create_completed.yaml index 2dd5fe09..02fe778a 100644 --- a/pkg/resource/replication_group/testdata/replication_group/cr/rg_cmd_create_completed.yaml +++ b/pkg/resource/replication_group/testdata/replication_group/cr/rg_cmd_create_completed.yaml @@ -6,6 +6,7 @@ spec: cacheNodeType: cache.t3.micro engine: redis numNodeGroups: 1 + preferredMaintenanceWindow: "wed:08:00-wed:09:00" replicasPerNodeGroup: 1 replicationGroupDescription: cluster-mode disabled RG replicationGroupID: rg-cmd diff --git a/pkg/resource/user/delta_util.go b/pkg/resource/user/delta_util.go index 783a3bdf..d9ad1425 100644 --- a/pkg/resource/user/delta_util.go +++ b/pkg/resource/user/delta_util.go @@ -14,6 +14,7 @@ package user import ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare" +import "github.com/aws-controllers-k8s/elasticache-controller/pkg/common" // remove differences which are not meaningful (i.e. ones that don't warrant a call to rm.Update) func filterDelta( @@ -26,33 +27,7 @@ func filterDelta( if delta.DifferentAt("Spec.AccessString") { if *desired.ko.Spec.AccessString == *desired.ko.Status.LastRequestedAccessString { - removeFromDelta(delta, "Spec.AccessString") + common.RemoveFromDelta(delta, "Spec.AccessString") } } } - -// 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 - } -} diff --git a/test/e2e/tests/test_replicationgroup.py b/test/e2e/tests/test_replicationgroup.py index 735dd57c..1d721ec1 100644 --- a/test/e2e/tests/test_replicationgroup.py +++ b/test/e2e/tests/test_replicationgroup.py @@ -157,6 +157,29 @@ def rg_cmd_update(rg_cmd_update_input, make_replication_group, rg_deletion_waite rg_deletion_waiter.wait(ReplicationGroupId=input_dict["RG_ID"]) +@pytest.fixture(scope="module") +def rg_update_pmw_input(make_rg_name): + return { + "RG_ID": make_rg_name("rg-update-pmw"), + "ENGINE_VERSION": "6.x", + "NUM_NODE_GROUPS": "1", + "REPLICAS_PER_NODE_GROUP": "1" + } + + +@pytest.fixture(scope="module") +def rg_update_pmw(rg_update_pmw_input, make_replication_group, rg_deletion_waiter): + input_dict = rg_update_pmw_input + + (reference, resource) = make_replication_group("replicationgroup_cmd_update", input_dict, input_dict['RG_ID']) + yield reference, resource + + # teardown + k8s.delete_custom_resource(reference) + sleep(DEFAULT_WAIT_SECS) + rg_deletion_waiter.wait(ReplicationGroupId=input_dict['RG_ID']) + + @pytest.fixture(scope="module") def rg_deletion_input(make_rg_name): return { @@ -186,7 +209,7 @@ def test_rg_cmd_fromsnapshot(self, rg_cmd_fromsnapshot): (reference, _) = rg_cmd_fromsnapshot assert k8s.wait_on_condition(reference, "ACK.ResourceSynced", "True", wait_periods=30) - # test update behavior of controller; this test can be changed to include multiple chained updates + # test update behavior of controller (engine version and replica count) def test_rg_cmd_update(self, rg_cmd_update_input, rg_cmd_update): (reference, _) = rg_cmd_update assert k8s.wait_on_condition(reference, "ACK.ResourceSynced", "True", wait_periods=30) @@ -232,6 +255,23 @@ def test_rg_cmd_update(self, rg_cmd_update_input, rg_cmd_update): assert cc is not None assert cc['EngineVersion'] == desired_engine_version + # test that controller can update preferred maintenance window + def test_rg_update_pmw(self, rg_update_pmw_input, rg_update_pmw): + # wait for resource to sync and retrieve initial PMW + (reference, _) = rg_update_pmw + assert k8s.wait_on_condition(reference, "ACK.ResourceSynced", "True", wait_periods=30) + + # update, wait for resource to sync + desired_pmw = 'sun:23:39-mon:02:24' + patch = {"spec": {"preferredMaintenanceWindow": desired_pmw}} + _ = k8s.patch_custom_resource(reference, patch) + sleep(DEFAULT_WAIT_SECS) + assert k8s.wait_on_condition(reference, "ACK.ResourceSynced", "True", wait_periods=5) # should be immediate + + # assert new state + cc = retrieve_cache_cluster(rg_update_pmw_input['RG_ID']) + assert cc['PreferredMaintenanceWindow'] == desired_pmw + def test_rg_auth_token(self, rg_auth_token, secrets): (reference, _) = rg_auth_token assert k8s.wait_on_condition(reference, "ACK.ResourceSynced", "True", wait_periods=30)