From 34f64d19a4a664b0d6ae1adf2a50e7bddda9cd51 Mon Sep 17 00:00:00 2001 From: Amine Date: Mon, 23 Feb 2026 20:35:00 -0800 Subject: [PATCH] Fix cluster logging config comparison to prevent false deltas The EKS API always returns both enabled and disabled log type entries in its response, but users typically only specify enabled log types in their manifests. The generated structural delta comparison treated this representation mismatch as a real diff, causing the controller to repeatedly attempt no-op logging updates. This blocked all other updates (including version upgrades) from being processed, since the logging handler always returned early. Disable the generated delta for the Logging field and replace it with a custom semantic comparison in `customPreCompare` that only compares the effective set of enabled log types using `SliceStringPEqual`. When a real logging change is detected, send the full 5-type config (both enabled and disabled) to the `UpdateClusterConfig` API to avoid `InvalidParameterException` "No changes needed" errors. Also remove the `LoggingNoChangesError` swallow logic that was masking the root cause, and clean up the now unused generated newLogging helper from `sdk.go` and its template. Fixes aws-controllers-k8s/community#2781 --- apis/v1alpha1/ack-generate-metadata.yaml | 8 +- apis/v1alpha1/generator.yaml | 5 +- generator.yaml | 5 +- pkg/resource/cluster/delta.go | 13 +-- pkg/resource/cluster/hook.go | 103 ++++++++++++++++++-- pkg/resource/cluster/sdk.go | 31 ------ templates/hooks/cluster/sdk_file_end.go.tpl | 2 +- test/e2e/tests/test_cluster.py | 18 ++-- 8 files changed, 116 insertions(+), 69 deletions(-) diff --git a/apis/v1alpha1/ack-generate-metadata.yaml b/apis/v1alpha1/ack-generate-metadata.yaml index e5bf553b..b94cea4b 100755 --- a/apis/v1alpha1/ack-generate-metadata.yaml +++ b/apis/v1alpha1/ack-generate-metadata.yaml @@ -1,13 +1,13 @@ ack_generate_info: - build_date: "2026-02-19T00:10:17Z" - build_hash: af61bc1478baf08196fb6e2c81590518a754e45e + build_date: "2026-02-24T04:42:15Z" + build_hash: b0103744b8b2c48e9f8938ff6c2bf3b95ad95406 go_version: go1.25.6 - version: v0.57.0-9-gaf61bc1 + version: v0.57.0-14-gb010374 api_directory_checksum: df9ec56e987fdc93ed2e7f55c55cf0bc3372b109 api_version: v1alpha1 aws_sdk_go_version: v1.40.1 generator_config_info: - file_checksum: a0b97e79bcd3136d63cafd2a8b98e4b6b5a52c15 + file_checksum: 01b155972bea16ddf80b42849a78ca34f00f52eb original_file_name: generator.yaml last_modification: reason: API generation diff --git a/apis/v1alpha1/generator.yaml b/apis/v1alpha1/generator.yaml index 342cdad6..1f6c33b0 100644 --- a/apis/v1alpha1/generator.yaml +++ b/apis/v1alpha1/generator.yaml @@ -179,6 +179,9 @@ resources: BootstrapSelfManagedAddons: compare: is_ignored: true + Logging: + compare: + is_ignored: true exceptions: errors: 404: @@ -197,7 +200,7 @@ resources: - ValidationError hooks: delta_pre_compare: - code: customPreCompare(a, b) + code: customPreCompare(delta, a, b) sdk_create_post_set_output: template_path: hooks/cluster/sdk_create_post_set_output.go.tpl sdk_read_one_post_set_output: diff --git a/generator.yaml b/generator.yaml index 342cdad6..1f6c33b0 100644 --- a/generator.yaml +++ b/generator.yaml @@ -179,6 +179,9 @@ resources: BootstrapSelfManagedAddons: compare: is_ignored: true + Logging: + compare: + is_ignored: true exceptions: errors: 404: @@ -197,7 +200,7 @@ resources: - ValidationError hooks: delta_pre_compare: - code: customPreCompare(a, b) + code: customPreCompare(delta, a, b) sdk_create_post_set_output: template_path: hooks/cluster/sdk_create_post_set_output.go.tpl sdk_read_one_post_set_output: diff --git a/pkg/resource/cluster/delta.go b/pkg/resource/cluster/delta.go index ce160f9a..28e7cd6e 100644 --- a/pkg/resource/cluster/delta.go +++ b/pkg/resource/cluster/delta.go @@ -41,7 +41,7 @@ func newResourceDelta( delta.Add("", a, b) return delta } - customPreCompare(a, b) + customPreCompare(delta, a, b) if ackcompare.HasNilDifference(a.ko.Spec.AccessConfig, b.ko.Spec.AccessConfig) { delta.Add("Spec.AccessConfig", a.ko.Spec.AccessConfig, b.ko.Spec.AccessConfig) @@ -122,17 +122,6 @@ func newResourceDelta( } } } - if ackcompare.HasNilDifference(a.ko.Spec.Logging, b.ko.Spec.Logging) { - delta.Add("Spec.Logging", a.ko.Spec.Logging, b.ko.Spec.Logging) - } else if a.ko.Spec.Logging != nil && b.ko.Spec.Logging != nil { - if len(a.ko.Spec.Logging.ClusterLogging) != len(b.ko.Spec.Logging.ClusterLogging) { - delta.Add("Spec.Logging.ClusterLogging", a.ko.Spec.Logging.ClusterLogging, b.ko.Spec.Logging.ClusterLogging) - } else if len(a.ko.Spec.Logging.ClusterLogging) > 0 { - if !equality.Semantic.Equalities.DeepEqual(a.ko.Spec.Logging.ClusterLogging, b.ko.Spec.Logging.ClusterLogging) { - delta.Add("Spec.Logging.ClusterLogging", a.ko.Spec.Logging.ClusterLogging, b.ko.Spec.Logging.ClusterLogging) - } - } - } if ackcompare.HasNilDifference(a.ko.Spec.Name, b.ko.Spec.Name) { delta.Add("Spec.Name", a.ko.Spec.Name, b.ko.Spec.Name) } else if a.ko.Spec.Name != nil && b.ko.Spec.Name != nil { diff --git a/pkg/resource/cluster/hook.go b/pkg/resource/cluster/hook.go index 100f1f69..37c17da6 100644 --- a/pkg/resource/cluster/hook.go +++ b/pkg/resource/cluster/hook.go @@ -37,10 +37,6 @@ import ( "github.com/aws-controllers-k8s/eks-controller/pkg/util" ) -const ( - LoggingNoChangesError = "No changes needed for the logging config provided" -) - // Taken from the list of cluster statuses on the boto3 documentation // https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/eks.html#EKS.Client.describe_cluster const ( @@ -185,12 +181,38 @@ func (rm *resourceManager) clusterInUse(ctx context.Context, r *resource) (bool, } func customPreCompare( + delta *ackcompare.Delta, a *resource, b *resource, ) { if a.ko.Spec.UpgradePolicy == nil && b.ko.Spec.UpgradePolicy != nil { a.ko.Spec.UpgradePolicy = b.ko.Spec.UpgradePolicy.DeepCopy() } + // Custom logging comparison. The EKS API always returns both enabled and + // disabled log type entries, but users may only specify enabled ones. A + // missing enabled list means everything is disabled. We compare the + // effective set of enabled log types to avoid false positives. + desiredEnabled := enabledLogTypes(a) + latestEnabled := enabledLogTypes(b) + if !ackcompare.SliceStringPEqual(desiredEnabled, latestEnabled) { + delta.Add("Spec.Logging", a.ko.Spec.Logging, b.ko.Spec.Logging) + } +} + +// enabledLogTypes returns the list of log type strings that are enabled in the +// resource's logging config. A nil logging config or missing entries means +// nothing is enabled. +func enabledLogTypes(r *resource) []*string { + var enabled []*string + if r.ko.Spec.Logging == nil { + return enabled + } + for _, ls := range r.ko.Spec.Logging.ClusterLogging { + if ls.Enabled != nil && *ls.Enabled { + enabled = append(enabled, ls.Types...) + } + } + return enabled } func (rm *resourceManager) customUpdate( @@ -249,15 +271,12 @@ func (rm *resourceManager) customUpdate( if err := rm.updateConfigLogging(ctx, desired); err != nil { awsErr, ok := extractAWSError(err) - // The API responds with an error if there were no changes applied - if !ok || awsErr.Message != LoggingNoChangesError { - return nil, err - } - // Check to see if we've raced an async update call and need to requeue if ok && awsErr.Code == "ResourceInUseException" { return nil, requeueAfterAsyncUpdate() } + + return nil, err } return returnClusterUpdating(updatedRes) } @@ -472,6 +491,15 @@ func (rm *resourceManager) updateVersion( return nil } +// allEKSLogTypes is the complete set of EKS control plane log types. +var allEKSLogTypes = []svcsdktypes.LogType{ + svcsdktypes.LogTypeApi, + svcsdktypes.LogTypeAudit, + svcsdktypes.LogTypeAuthenticator, + svcsdktypes.LogTypeControllerManager, + svcsdktypes.LogTypeScheduler, +} + func (rm *resourceManager) updateConfigLogging( ctx context.Context, r *resource, @@ -481,7 +509,7 @@ func (rm *resourceManager) updateConfigLogging( defer exit(err) input := &svcsdk.UpdateClusterConfigInput{ Name: r.ko.Spec.Name, - Logging: rm.newLogging(r), + Logging: newFullLogging(r), } _, err = rm.sdkapi.UpdateClusterConfig(ctx, input) @@ -493,6 +521,61 @@ func (rm *resourceManager) updateConfigLogging( return nil } +// newFullLogging builds a complete EKS logging config with all log types +// explicitly set to enabled or disabled. The EKS API expects the full config; +// sending only enabled types causes "No changes needed" errors when the +// effective state hasn't changed. +// +// All user-specified enabled types are passed through to the API (including +// unknown ones, so that EKS can validate and reject them). The disabled set +// is built from the known EKS log types minus whatever the user enabled. +func newFullLogging(r *resource) *svcsdktypes.Logging { + // Collect the set of log types the user wants enabled. + enabled := make(map[svcsdktypes.LogType]bool) + var enabledTypes []svcsdktypes.LogType + if r.ko.Spec.Logging != nil { + for _, ls := range r.ko.Spec.Logging.ClusterLogging { + if ls.Enabled != nil && *ls.Enabled { + for _, t := range ls.Types { + if t != nil { + lt := svcsdktypes.LogType(*t) + if !enabled[lt] { + enabled[lt] = true + enabledTypes = append(enabledTypes, lt) + } + } + } + } + } + } + + // Disabled types: known EKS log types that the user did not enable. + var disabledTypes []svcsdktypes.LogType + for _, lt := range allEKSLogTypes { + if !enabled[lt] { + disabledTypes = append(disabledTypes, lt) + } + } + + var clusterLogging []svcsdktypes.LogSetup + if len(enabledTypes) > 0 { + clusterLogging = append(clusterLogging, svcsdktypes.LogSetup{ + Enabled: aws.Bool(true), + Types: enabledTypes, + }) + } + if len(disabledTypes) > 0 { + clusterLogging = append(clusterLogging, svcsdktypes.LogSetup{ + Enabled: aws.Bool(false), + Types: disabledTypes, + }) + } + + return &svcsdktypes.Logging{ + ClusterLogging: clusterLogging, + } +} + func newAccessConfig(r *resource) *svcsdktypes.UpdateAccessConfigRequest { cfg := &svcsdktypes.UpdateAccessConfigRequest{} if r.ko.Spec.AccessConfig != nil { diff --git a/pkg/resource/cluster/sdk.go b/pkg/resource/cluster/sdk.go index e39dde3b..2ae88c52 100644 --- a/pkg/resource/cluster/sdk.go +++ b/pkg/resource/cluster/sdk.go @@ -1183,37 +1183,6 @@ func (rm *resourceManager) terminalAWSError(err error) bool { } } -// newLogging returns a Logging object -// with each the field set by the resource's corresponding spec field. -func (rm *resourceManager) newLogging( - r *resource, -) *svcsdktypes.Logging { - res := &svcsdktypes.Logging{} - - if r.ko.Spec.Logging.ClusterLogging != nil { - resf0 := []svcsdktypes.LogSetup{} - for _, resf0iter := range r.ko.Spec.Logging.ClusterLogging { - resf0elem := &svcsdktypes.LogSetup{} - if resf0iter.Enabled != nil { - resf0elem.Enabled = resf0iter.Enabled - } - if resf0iter.Types != nil { - resf0elemf1 := []svcsdktypes.LogType{} - for _, resf0elemf1iter := range resf0iter.Types { - var resf0elemf1elem string - resf0elemf1elem = string(*resf0elemf1iter) - resf0elemf1 = append(resf0elemf1, svcsdktypes.LogType(resf0elemf1elem)) - } - resf0elem.Types = resf0elemf1 - } - resf0 = append(resf0, *resf0elem) - } - res.ClusterLogging = resf0 - } - - return res -} - // newVpcConfigRequest returns a VpcConfigRequest object // with each the field set by the resource's corresponding spec field. func (rm *resourceManager) newVpcConfigRequest( diff --git a/templates/hooks/cluster/sdk_file_end.go.tpl b/templates/hooks/cluster/sdk_file_end.go.tpl index 486d9070..94013abd 100644 --- a/templates/hooks/cluster/sdk_file_end.go.tpl +++ b/templates/hooks/cluster/sdk_file_end.go.tpl @@ -3,7 +3,7 @@ {{/* Find the structure field within the operation */}} {{- range $fieldName, $field := $CRD.SpecFields -}} -{{- if (or (eq $field.Path "Logging") (eq $field.Path "ResourcesVPCConfig")) }} +{{- if (eq $field.Path "ResourcesVPCConfig") }} {{- $shapeName := $field.ShapeRef.ShapeName }} diff --git a/test/e2e/tests/test_cluster.py b/test/e2e/tests/test_cluster.py index cb713027..83380d52 100644 --- a/test/e2e/tests/test_cluster.py +++ b/test/e2e/tests/test_cluster.py @@ -233,7 +233,10 @@ def test_create_update_delete_cluster(self, eks_client, simple_cluster): aws_res = eks_client.describe_cluster(name=cluster_name) assert sorted(aws_res["cluster"]["resourcesVpcConfig"]["subnetIds"]) == sorted(subnets_ids) - # Update the logging fields + # Update the logging fields specifying only enabled types. The + # controller must handle partial specs correctly — the EKS API + # returns both enabled and disabled entries, but users should not + # be required to list disabled types explicitly. updates = { "spec": { "logging": { @@ -242,10 +245,6 @@ def test_create_update_delete_cluster(self, eks_client, simple_cluster): "enabled": True, "types": ["api"] }, - { - "enabled": False, - "types": ["audit", "authenticator", "controllerManager", "scheduler"] - }, ] }, } @@ -257,10 +256,11 @@ def test_create_update_delete_cluster(self, eks_client, simple_cluster): wait_for_cluster_active(eks_client, cluster_name) aws_res = eks_client.describe_cluster(name=cluster_name) - assert len(aws_res["cluster"]["logging"]["clusterLogging"]) > 0 - logging = aws_res["cluster"]["logging"]["clusterLogging"][0] - assert logging["enabled"] == True - assert logging["types"] == ["api"] + enabled_types = [] + for log_setup in aws_res["cluster"]["logging"]["clusterLogging"]: + if log_setup.get("enabled"): + enabled_types.extend(log_setup["types"]) + assert enabled_types == ["api"] # Update the AccessConfig field updates = {