fix: Equals mutation-test harness for IR types; fix Listener and GatewayExtension equality#14248
Merged
andy-fong merged 8 commits intoJun 23, 2026
Conversation
gateway-bot
added
kind/cleanup
puertomontt
added
the
work in progress
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a generic mutation-test harness to enforce that IR Equals() implementations detect changes to every exported field (preventing missed KRT updates/stale Envoy config), applies it to several core IR types, and fixes GatewayExtension.Equals to include ObjectSource in comparisons.
Changes:
- Added
test/testutils/equalstestharness to validateEquals()field coverage via mutation cases plus a reflection-based completeness check. - Added harness self-tests (black-box + white-box) to ensure the completeness logic works as intended.
- Added IR-level harness tests for
PolicyAtt,AttachedPolicies,Gateway,ListenerSet,BackendRefIR, andGatewayExtension, and fixedGatewayExtension.Equalsto compareObjectSource.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/testutils/equalstest/equalstest.go | Adds the generic mutation/completeness test harness for Equals() implementations. |
| test/testutils/equalstest/equalstest_test.go | Black-box self-tests verifying harness behavior (correct equals, exemptions, fixture sanity). |
| test/testutils/equalstest/equalstest_internal_test.go | White-box tests for embedded-field flattening/completeness helper behavior. |
| pkg/pluginsdk/ir/gatewayextension.go | Fixes GatewayExtension.Equals to include ObjectSource comparison. |
| pkg/pluginsdk/ir/equality_harness_test.go | Applies the harness to core IR types to enforce Equals() coverage going forward. |
Adds a generic test harness (test/testutils/equalstest) that verifies every exported field of an IR type is covered by a mutation case that proves Equals() detects the change. Any uncovered, non-exempt field fails the test, so adding a field without updating Equals becomes a build-time failure instead of a silent stale-config bug. Applies the harness to six core IR types: PolicyAtt, AttachedPolicies, Gateway, ListenerSet, BackendRefIR, and GatewayExtension. Also fixes GatewayExtension.Equals to include the embedded ObjectSource comparison, which was previously missing (the harness-driven case exposed this gap before the fix was applied). Listener is intentionally excluded per plan 002 scope; plan 003 will add Listener coverage once Listener.Equals is rewritten. Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
puertomontt
force-pushed
the
advisor/002-equals-mutation-harness-v2
branch
from
June 13, 2026 01:24
2d9a241 to
1916ac6
Compare
reflect.TypeOf(*new(T)) returns nil for interface type parameters (nil-pointer panic on Kind()) and rejects pointer-to-struct types. reflect.TypeFor[T]() resolves the static type directly; dereference pointers one level so *Struct IR types can use the harness. Signed-off-by: omar <[email protected]>
Comment on lines
+583
to
+586
| []string{"Name", "Hostname", "Port", "Protocol", "TLS", "AllowedRoutes"}, // embedded gwv1.Listener fields covered by "Listener" case | ||
| // Suppress the gk field from the AttachedPolicies map closure; the | ||
| // "AttachedPolicies" case above exercises it at the struct level. | ||
| ) |
Typed informers leave TypeMeta empty, so a Gateway parent built with TypeMeta set (e.g. in tests) would never compare equal to the same parent from an informer, causing permanent spurious recomputation. Normalize empty GVKs to wellknown.GatewayGVK before comparing. ListenerSet is not normalized since the same Go type serves both the standard and legacy XListenerSet GVKs. Signed-off-by: omar <[email protected]>
The file header claimed Listener coverage was deferred, but this branch rewrites Listener.Equals and adds TestHarnessListenerEquals. Signed-off-by: omar <[email protected]>
puertomontt
changed the title
puertomontt
removed
the
work in progress
andy-fong
reviewed
Jun 17, 2026
| func (c Listener) Equals(in Listener) bool { | ||
| return reflect.DeepEqual(c, in) | ||
| return reflect.DeepEqual(c.Listener, in.Listener) && // plain gwv1 API struct, no protos | ||
| parentRefEquals(c.Parent, in.Parent) && |
Contributor
There was a problem hiding this comment.
I have a similar fix for this in https://github.com/kgateway-dev/kgateway/pull/14265/changes#diff-8613fbbe4c4f3ee612be6e867ac7fdd6464fb168ca0ffc40bdcdad28db7fd385R425 where I decided to repeat the versionEqual() that the parent already did just in case Listener is added to it's own KRT collection in the future and the Equals() is still correct. The parentRefEquals here will not detect value changes in the parent if this function is not called from the parent's Equals(). Currently, this function only get called from it's parent's Equals() though.
Replace the identity-based (GVK + namespace/name) parent comparison with the versionEquals approach from PR kgateway-dev#14265 so status-only writes to the parent Gateway/ListenerSet (which bump resourceVersion but not generation) do not trigger spurious re-translations. Removes the now-unused parentRefEquals/parentGVK helpers and updates the equality harness tests to match the new semantics. Signed-off-by: omar <[email protected]>
| baseHarnessGateway, | ||
| func(a, b Gateway) bool { return a.Equals(b) }, | ||
| cases, | ||
| []string{"Group", "Kind", "Namespace", "Name"}, // embedded ObjectSource fields covered by "ObjectSource" case |
| baseHarnessListenerSet, | ||
| func(a, b ListenerSet) bool { return a.Equals(b) }, | ||
| cases, | ||
| []string{"Group", "Kind", "Namespace", "Name"}, // embedded ObjectSource fields covered by "ObjectSource" case |
Comment on lines
+488
to
+489
| []string{"Group", "Kind", "Namespace", "Name"}, // embedded ObjectSource fields | ||
| ) |
| baseHarnessFullListener, | ||
| func(a, b Listener) bool { return a.Equals(b) }, | ||
| cases, | ||
| []string{"Name", "Hostname", "Port", "Protocol", "TLS", "AllowedRoutes"}, // embedded gwv1.Listener fields covered by "Listener" case |
Comment on lines
+425
to
+426
| // TestHarnessGatewayExtensionEquals drives the ObjectSource fix in Step 4. | ||
| // The "ObjectSource" mutation case will FAIL until Step 4 adds the comparison. |
Comment on lines
+588
to
+589
| // Suppress the gk field from the AttachedPolicies map closure; the | ||
| // "AttachedPolicies" case above exercises it at the struct level. |
andy-fong
approved these changes
Jun 23, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jun 23, 2026
chandler-solo
pushed a commit
to chandler-solo/kgateway
that referenced
this pull request
Jul 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
IR types consumed by KRT collections must implement
Equals()covering all fields; a missed field can silently suppress updates and leave stale Envoy config. Today nothing enforces coverage when a field is added to an existing IR type.This PR has two parts:
1. Equals mutation-test harness. Adds a generic harness (
test/testutils/equalstest) that verifies every exported field of an IR type is covered by a mutation case provingEquals()detects the change. Any uncovered, non-exempt field fails the test, so adding a field without updatingEqualsbecomes a build-time failure instead of a silent stale-config bug. The harness supports struct and pointer-to-struct type parameters viareflect.TypeFor, flattens embedded structs one level for coverage checks, and is applied to seven core IR types:PolicyAtt,AttachedPolicies,Gateway,Listener,ListenerSet,BackendRefIR, andGatewayExtension.2. Equals fixes the harness and follow-up review drove out.
GatewayExtension.Equalsnow includes the embeddedObjectSourcecomparison.Listener.Equalsno longer uses whole-structreflect.DeepEqual, which compared theParent client.Objectby content and made parent metadata churn (for example,resourceVersionbumps from status writes) dirty every listener. It now compares the embeddedgwv1.Listener,AttachedPolicies, andPolicyAncestorRefexplicitly, and comparesParentwithversionEquals. That keeps status-only parent writes from triggering unnecessary re-translation while still detecting meaningful parent version changes ifListener.Equalsis called directly in the future.Change Type
/kind fix
/kind cleanup
Changelog
Additional Notes
The harness completeness check is exercised through its real code path in
equalstest_internal_test.go, so a regression in field-coverage detection also fails tests. The Listener parent comparison follows theversionEqualsapproach discussed in #14265.