Fix cluster logging config comparison to prevent false deltas#202
Fix cluster logging config comparison to prevent false deltas#202a-hilaly wants to merge 1 commit intoaws-controllers-k8s:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: a-hilaly The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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
a-hilaly
force-pushed
the
fix/logging-config
branch
from
e97fe64 to
34f64d1
Compare
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@a-hilaly: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
| } | ||
| } | ||
|
|
||
| // Disabled types: known EKS log types that the user did not enable. |
There was a problem hiding this comment.
What about logs the user explicitly disabled? If EKS add a new LogType that isn't listed here this will prevent the user from disabling it if it ever became enabled.
There was a problem hiding this comment.
Instead of using a hardcoded list could we use the values from latest to get the most recent list of LogTypes? The full set of log types to disable would be (latest disabled - user enable + user disabled).
2 participants
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
customPreComparethat only comparesthe 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
UpdateClusterConfigAPI to avoidInvalidParameterException"No changes needed" errors.Also remove the
LoggingNoChangesErrorswallow logic that was maskingthe root cause, and clean up the now unused generated newLogging
helper from
sdk.goand its template.Fixes aws-controllers-k8s/community#2781
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.