fix: compare pointer-bearing IR fields by value in Equals (HttpListenerPolicyIr, directResponse)#14331
Merged
puertomontt merged 4 commits intoJul 1, 2026
Conversation
…yIr.Equals HttpListenerPolicyIr.Equals compared the serverHeaderTransformation pointers by identity (!=) instead of by value, so two IRs with equal transformation values but distinct pointers compared unequal, triggering spurious re-translations. Use cmputils.PointerValsEqual instead. Add an equals harness test covering every field of HttpListenerPolicyIr, modeled on pkg/pluginsdk/ir/equality_harness_test.go. Since all fields are unexported, extend the equalstest harness with a non-breaking IncludeUnexported() option so the completeness check still fails when a field is added without a matching Equals comparison. Signed-off-by: omar <[email protected]>
gateway-bot
added
do-not-merge/description-invalid
kind/fix
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes an Equals correctness bug in the listenerpolicy HTTP PolicyIR so semantically identical serverHeaderTransformation values no longer compare unequal due to pointer identity, preventing unnecessary listener re-translations. It also strengthens equality regression coverage by adding a field-by-field equality harness for HttpListenerPolicyIr, and extends the shared equalstest harness to support completeness checks for unexported fields.
Changes:
- Fix
HttpListenerPolicyIr.Equalsto compareserverHeaderTransformationby value usingcmputils.PointerValsEqual. - Add a comprehensive equality harness test for
HttpListenerPolicyIrthat mutates each field and includes a reflexivity guard for the pointer-identity regression. - Extend
test/testutils/equalstest.Runwith anIncludeUnexported()option and add internal tests for that behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
pkg/kgateway/extensions2/plugins/listenerpolicy/http.go |
Compares serverHeaderTransformation pointer values by value rather than identity in Equals. |
pkg/kgateway/extensions2/plugins/listenerpolicy/equality_harness_test.go |
Adds a harness to ensure HttpListenerPolicyIr.Equals detects changes for every field (including reflexivity). |
test/testutils/equalstest/equalstest.go |
Adds Option support and IncludeUnexported() to make completeness checking work for types with unexported fields. |
test/testutils/equalstest/equalstest_internal_test.go |
Adds coverage validating default exported-only behavior and the new include-unexported behavior. |
Comments suppressed due to low confidence (1)
test/testutils/equalstest/equalstest.go:109
- The completeness-check failure message always says "exported field(s)", but Run can now be configured to include unexported fields via IncludeUnexported(). When that option is used, this message becomes misleading and can slow down debugging when a new unexported field is added without a Case.
if len(missing) > 0 {
t.Errorf(
"completeness check failed for %s: exported field(s) %v are neither covered by a mutation Case nor listed as exempt — add a Case or add the field name to exempt",
typeName(typ),
missing,
)
}
Comment on lines
52
to
+55
| // exportedFields emits both "Embedded" (flattened) and "EmbeddedBase" (the | ||
| // embedding name itself). With covered containing only "Embedded", | ||
| // "EmbeddedBase" must appear as uncovered. | ||
| missing := uncoveredFields(typ, covered, exempt) | ||
| missing := uncoveredFields(typ, covered, exempt, false) |
Comment on lines
63
to
65
| // Cover every name that exportedFields produces for internalFixture. | ||
| covered := map[string]bool{"Plain": true, "Embedded": true, "EmbeddedBase": true} | ||
| exempt := map[string]bool{} |
Signed-off-by: omar <[email protected]>
puertomontt
enabled auto-merge
June 30, 2026 15:14
directResponse.Equals compared d.spec == d2.spec, but DirectResponseSpec has pointer fields (Body, BodyFormat) so struct == compares those by pointer identity. Since the IR is rebuilt from a freshly decoded object on every recompute, equal specs get distinct pointers and Equals spuriously reported inequality, triggering needless re-translation. Compare by value with reflect.DeepEqual, matching how the core IR package compares plain (non-proto) API structs. Add an equals harness test; its reflexivity check guards the regression. Signed-off-by: omar <[email protected]>
puertomontt
changed the title
Signed-off-by: omar <[email protected]>
jenshu
approved these changes
Jul 1, 2026
puertomontt
added a commit
to puertomontt/kgateway
that referenced
this pull request
Jul 1, 2026
…erPolicyIr, directResponse) (kgateway-dev#14331) [v2.3.x backport] (kgateway-dev#14342) Signed-off-by: omar <[email protected]>
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
Fixes two
Equalsimplementations that compared pointer-bearing fields by pointer identity instead of by value. Because these IRs are rebuilt from freshly-decoded objects on every recompute, equal content gets distinct pointers, soEqualsspuriously reports inequality and triggers needless re-translation.1.
HttpListenerPolicyIr.Equals(listenerpolicy/http.go)serverHeaderTransformationwas compared with!=(pointer identity) rather than by value. Switched tocmputils.PointerValsEqual, matching the other pointer fields in the method.Also added an
Equalsharness test covering every field ofHttpListenerPolicyIr, modeled onpkg/pluginsdk/ir/equality_harness_test.go. The harness'sreflexivitysubtest (base().Equals(base())over two independent sets of pointers) directly guards the fixed bug.Because all fields of
HttpListenerPolicyIrare unexported, the harness's completeness check (which fails when a field is added without updatingEquals) would otherwise enforce nothing. So this extends the sharedequalstestharness with a non-breaking variadicIncludeUnexported()option; existing callers are unaffected.2.
directResponse.Equals(directresponse/direct_response_plugin.go)Found while auditing every
Equalsmethod across the plugin packages for the same bug class.directResponse.Equalsdidd.spec == d2.spec, butDirectResponseSpechas pointer fields (Body *string,BodyFormat *shared.BodyFormat), so struct==compared those by pointer identity. Switched to value comparison withreflect.DeepEqual—DirectResponseSpecis a plain (non-proto) API type, consistent with how the core IR package compares such structs (pkg/pluginsdk/ir/gatewayextension.go,backend.go). Added a harness test whose reflexivity check guards the regression.Changes
pkg/kgateway/extensions2/plugins/listenerpolicy/http.go: usecmputils.PointerValsEqualforserverHeaderTransformation.pkg/kgateway/extensions2/plugins/listenerpolicy/equality_harness_test.go: new harness test, one mutation case per field (28 fields) plus reflexivity/symmetry/completeness.pkg/kgateway/extensions2/plugins/directresponse/direct_response_plugin.go: compare spec by value.pkg/kgateway/extensions2/plugins/directresponse/equality_harness_test.go: new harness test.test/testutils/equalstest/equalstest.go: addIncludeUnexported()option (variadic, backwards compatible).test/testutils/equalstest/equalstest_internal_test.go: tests for the new option.Change Type
/kind fix
Changelog
Additional Notes
Verified each harness catches its regression: temporarily reverting either fix makes the corresponding
reflexivitysubtest fail ("Equals(base(), base()) returned false"), then pass once restored.Audit also surfaced (reviewed, intentionally not changed): a deliberate
+krtEqualsTodoonTrafficPolicyGatewayExtensionIR.Name, an existingreflect.DeepEqualonFilterStage(behaviorally correct), and a latent (currently unreachable) nil-deref inoauthIR.Equals. The other ~38Equalsmethods are clean.