From f7712a045b6a4c2177279cfd6adeda6ae27de325 Mon Sep 17 00:00:00 2001 From: omar Date: Mon, 1 Jun 2026 18:22:00 -0400 Subject: [PATCH] fix(gateway2): share RouteOptions sub-messages instead of deep-cloning per route GetRouteOptionForRouteRule deep-cloned the first RouteOption attachment for every translated route via ShallowMergeRouteOptions' dst==nil branch. When many routes reference the same RouteOption (esp. ones carrying large transformation templates), each route received its own deep copy of identical config, which dominated translation heap (~31% / 4.3GB of a customer's 14GB heap). Add ShallowCopyRouteOptions, which copies only the top-level RouteOptions fields and shares the immutable sub-messages by pointer. This is consistent with the existing dst!=nil merge branch, which already shares src's fields. Each route still gets a distinct top-level message, so route plugins that reassign top-level fields (urlrewrite, headermodifier, mirror) remain isolated; they must not mutate the shared sub-messages in place. Memory now scales with the number of unique RouteOptions rather than the number of routes. --- .../plugins/routeoptions/query/query.go | 36 +++++++++++----- projects/gloo/pkg/utils/merge.go | 32 +++++++++++++++ projects/gloo/pkg/utils/merge_test.go | 41 +++++++++++++++++++ 3 files changed, 99 insertions(+), 10 deletions(-) diff --git a/projects/gateway2/translator/plugins/routeoptions/query/query.go b/projects/gateway2/translator/plugins/routeoptions/query/query.go index 1380bb46515..6d4016b5c72 100644 --- a/projects/gateway2/translator/plugins/routeoptions/query/query.go +++ b/projects/gateway2/translator/plugins/routeoptions/query/query.go @@ -65,16 +65,36 @@ func (r *routeOptionQueries) GetRouteOptionForRouteRule( var sources []*gloov1.SourceMetadata_SourceRef merged := &solokubev1.RouteOption{} + // mergeAttachment folds a single RouteOption attachment into the accumulated `merged` result, + // recording it as a source if any of its fields were used. + // + // The first attachment seeds `merged` with a shallow copy (sharing the attachment's immutable + // sub-messages by pointer) rather than a deep clone. Deep-cloning the first attachment per route + // is what dominated translation heap, since every route referencing the same RouteOption received + // its own deep copy of identical (and often large) transformation templates. `merged.Spec.Options` + // is a distinct top-level message per route, so downstream route plugins can still reassign its + // top-level fields safely; they must not mutate the shared sub-messages in place. + mergeAttachment := func(opt *solokubev1.RouteOption) { + optionUsed := false + if merged.Spec.GetOptions() == nil { + if src := opt.Spec.GetOptions(); src != nil { + merged.Spec.Options = glooutils.ShallowCopyRouteOptions(src) + optionUsed = true + } + } else { + merged.Spec.Options, optionUsed = glooutils.ShallowMergeRouteOptions(merged.Spec.GetOptions(), opt.Spec.GetOptions()) + } + if optionUsed { + sources = append(sources, routeOptionToSourceRef(opt)) + } + } + filterAttachments, err := lookupFilterAttachments(ctx, route, rule, gwQueries) if err != nil { return nil, nil, err } for _, opt := range filterAttachments { - optionUsed := false - merged.Spec.Options, optionUsed = glooutils.ShallowMergeRouteOptions(merged.Spec.GetOptions(), opt.Spec.GetOptions()) - if optionUsed { - sources = append(sources, routeOptionToSourceRef(opt)) - } + mergeAttachment(opt) } var list solokubev1.RouteOptionList @@ -97,11 +117,7 @@ func (r *routeOptionQueries) GetRouteOptionForRouteRule( } gwutils.SortByCreationTime(out) for _, opt := range out { - optionUsed := false - merged.Spec.Options, optionUsed = glooutils.ShallowMergeRouteOptions(merged.Spec.GetOptions(), opt.Spec.GetOptions()) - if optionUsed { - sources = append(sources, routeOptionToSourceRef(opt)) - } + mergeAttachment(opt) } return nilOptionIfEmpty(merged), sources, nil diff --git a/projects/gloo/pkg/utils/merge.go b/projects/gloo/pkg/utils/merge.go index 39370a0a83c..95f235b7de6 100644 --- a/projects/gloo/pkg/utils/merge.go +++ b/projects/gloo/pkg/utils/merge.go @@ -106,6 +106,38 @@ func ShallowMergeListenerOptions(dst, src *v1.ListenerOptions) (*v1.ListenerOpti return dst, overwrote } +// ShallowCopyRouteOptions returns a new RouteOptions whose top-level fields point at the +// same sub-messages as src, without deep-copying them. +// +// It is the single-argument analogue of the dst==nil case of ShallowMergeRouteOptions: that +// case deep-clones src on every call, which (for routes carrying large transformation +// templates) dominates translation heap because every translated route receives its own deep +// copy of an identical RouteOption. This helper instead shares the immutable sub-messages by +// pointer, which is consistent with how ShallowMergeRouteOptions already shares src's fields +// into a non-nil dst. +// +// The returned RouteOptions is a distinct top-level message, so callers may freely reassign its +// top-level fields (as the route plugins do) without affecting src. Callers must NOT mutate the +// shared sub-messages in place. +func ShallowCopyRouteOptions(src *v1.RouteOptions) *v1.RouteOptions { + if src == nil { + return nil + } + + out := &v1.RouteOptions{} + outValue, srcValue := reflect.ValueOf(out).Elem(), reflect.ValueOf(src).Elem() + for i := range srcValue.NumField() { + dstField, srcField := outValue.Field(i), srcValue.Field(i) + // CanSet is false for the unexported proto-internal fields (state, sizeCache, + // unknownFields), so the loop copies only the exported message/scalar fields. + if dstField.CanSet() { + dstField.Set(srcField) + } + } + + return out +} + // ShallowMergeRouteOptions merges the top-level fields of src into dst. // The fields in dst that have non-zero values will not be overwritten. // It performs a shallow merge of top-level fields only. diff --git a/projects/gloo/pkg/utils/merge_test.go b/projects/gloo/pkg/utils/merge_test.go index 9bb450df189..994b376f3d4 100644 --- a/projects/gloo/pkg/utils/merge_test.go +++ b/projects/gloo/pkg/utils/merge_test.go @@ -58,4 +58,45 @@ var _ = Describe("Merge", func() { Expect(actual).To(Equal(expected)) Expect(overwrote).To(BeTrue()) }) + + Describe("ShallowCopyRouteOptions", func() { + It("returns nil for a nil source", func() { + Expect(ShallowCopyRouteOptions(nil)).To(BeNil()) + }) + + It("copies top-level fields by value without deep-copying sub-messages", func() { + src := &v1.RouteOptions{ + PrefixRewrite: &wrappers.StringValue{Value: "rewrite-me"}, + Retries: &retries.RetryPolicy{ + RetryOn: "5XX", + NumRetries: 3, + }, + } + + out := ShallowCopyRouteOptions(src) + + // The copy is a distinct top-level message that compares equal by value. + Expect(out).NotTo(BeIdenticalTo(src)) + Expect(out).To(Equal(src)) + + // Sub-messages are shared by pointer rather than deep-cloned: this is the + // allocation saving that keeps translation heap bounded when many routes + // reference the same RouteOption. + Expect(out.GetRetries()).To(BeIdenticalTo(src.GetRetries())) + Expect(out.GetPrefixRewrite()).To(BeIdenticalTo(src.GetPrefixRewrite())) + }) + + It("isolates top-level field reassignment on the copy from the source", func() { + src := &v1.RouteOptions{ + PrefixRewrite: &wrappers.StringValue{Value: "original"}, + } + + out := ShallowCopyRouteOptions(src) + // Route plugins reassign top-level fields on the merged options; this must not + // leak back into the shared source RouteOption. + out.PrefixRewrite = &wrappers.StringValue{Value: "changed"} + + Expect(src.GetPrefixRewrite().GetValue()).To(Equal("original")) + }) + }) })