diff --git a/changelog/v1.22.0-beta13/routeoptions-per-route-clone-oom.yaml b/changelog/v1.22.0-beta13/routeoptions-per-route-clone-oom.yaml new file mode 100644 index 00000000000..4a597f8b978 --- /dev/null +++ b/changelog/v1.22.0-beta13/routeoptions-per-route-clone-oom.yaml @@ -0,0 +1,33 @@ +changelog: + - type: FIX + issueLink: https://github.com/solo-io/solo-projects/issues/8802 + resolvesIssue: false + description: >- + Add an opt-in optimization for excessive control plane memory usage in the Kubernetes Gateway + API translation when many routes reference the same RouteOption. Set the GG_ROUTE_OPTION_INTERNING + environment variable on the gloo control plane to a truthy value to enable it (disabled by + default, in which case translation behaves exactly as before). When enabled, translation no + longer deep-copies the attached RouteOption's options (including potentially large transformation + templates) for every route rule on every translation cycle: each unique RouteOption is + deep-copied exactly once per translation pass and that interned copy's sub-messages are shared + by every route referencing it, so memory scales with the number of unique RouteOptions instead + of the number of translated routes. The informer cache itself is never reachable from translation + output. The merged options remain a distinct per-route message, so translation plugins can still + set route-level fields safely; they must not mutate nested option messages in place. + - type: FIX + issueLink: https://github.com/solo-io/solo-projects/issues/8802 + resolvesIssue: false + description: >- + The transformation plugin no longer writes the resolved escapeCharacters inheritance + (template -> staged -> Settings) back into the input transformation template. The input + template can be shared across every route referencing the same RouteOption, so the + write-back could leak the resolved value across routes and mask later changes to the + Settings-level default. + - type: FIX + issueLink: https://github.com/solo-io/solo-projects/issues/8802 + resolvesIssue: false + description: >- + The Kubernetes Gateway API header modifier filter no longer writes into an existing + HeaderManipulation message on the route's options in place; it copies the message before + applying the filter. The existing message can be shared with other routes referencing the + same RouteOption when parent filters are re-applied to delegated child routes. diff --git a/projects/gateway2/extensions/extensions.go b/projects/gateway2/extensions/extensions.go index ea6153780c3..341e705ed7a 100644 --- a/projects/gateway2/extensions/extensions.go +++ b/projects/gateway2/extensions/extensions.go @@ -21,6 +21,10 @@ import ( // which have Enterprise variants. type K8sGatewayExtensions interface { // CreatePluginRegistry exposes the plugins supported by this implementation. + // + // Implementations must build a fresh registry on every call: plugins carry per-pass state + // (e.g. the routeoptions plugin's status cache and its query's interned RouteOption copies, + // see rtoptquery.NewQuery) and must not be reused across translation passes. CreatePluginRegistry(context.Context) registry.PluginRegistry // GetTranslator allows an extension to provide custom translation for diff --git a/projects/gateway2/proxy_syncer/proxy_syncer.go b/projects/gateway2/proxy_syncer/proxy_syncer.go index 54bf5691c17..f0c1ab0f3b9 100644 --- a/projects/gateway2/proxy_syncer/proxy_syncer.go +++ b/projects/gateway2/proxy_syncer/proxy_syncer.go @@ -692,6 +692,8 @@ func (s *ProxySyncer) buildProxy(ctx context.Context, gw *gwv1.Gateway) *glooPro stopwatch := statsutils.NewTranslatorStopWatch("ProxySyncer") stopwatch.Start() + // a fresh registry per pass: plugins carry per-pass state and must not be reused across + // translation passes (see K8sGatewayExtensions.CreatePluginRegistry) pluginRegistry := s.k8sGwExtensions.CreatePluginRegistry(ctx) rm := reports.NewReportMap() r := reports.NewReporter(&rm) diff --git a/projects/gateway2/translator/plugins/headermodifier/header_modifier_plugin.go b/projects/gateway2/translator/plugins/headermodifier/header_modifier_plugin.go index 10ea450a70c..66b591abff0 100644 --- a/projects/gateway2/translator/plugins/headermodifier/header_modifier_plugin.go +++ b/projects/gateway2/translator/plugins/headermodifier/header_modifier_plugin.go @@ -53,10 +53,7 @@ func (p *plugin) applyRequestFilter( if config == nil { return errors.Errorf("RequestHeaderModifier filter supplied does not define requestHeaderModifier") } - headerManipulation := outputRoute.GetOptions().GetHeaderManipulation() - if headerManipulation == nil { - headerManipulation = &headers.HeaderManipulation{} - } + headerManipulation := cloneHeaderManipulation(outputRoute.GetOptions().GetHeaderManipulation()) headerManipulation.RequestHeadersToAdd = requestHeadersToAdd(config.Add, config.Set) headerManipulation.RequestHeadersToRemove = config.Remove outputRoute.GetOptions().HeaderManipulation = headerManipulation @@ -70,16 +67,25 @@ func (p *plugin) applyResponseFilter( if config == nil { return errors.Errorf("Response filter supplied does not define requestHeaderModifier") } - headerManipulation := outputRoute.GetOptions().GetHeaderManipulation() - if headerManipulation == nil { - headerManipulation = &headers.HeaderManipulation{} - } + headerManipulation := cloneHeaderManipulation(outputRoute.GetOptions().GetHeaderManipulation()) headerManipulation.ResponseHeadersToAdd = responseHeadersToAdd(config.Add, config.Set) headerManipulation.ResponseHeadersToRemove = config.Remove outputRoute.GetOptions().HeaderManipulation = headerManipulation return nil } +// cloneHeaderManipulation returns a deep copy of hm (or a fresh message if nil) for the filter +// writes above to land on. The existing HeaderManipulation must never be written to in place: +// route options share their sub-messages with every route referencing the same RouteOption +// (solo-io/solo-projects#8802), and parent filters re-applied to delegated child routes reach +// this plugin with options already populated from RouteOptions. +func cloneHeaderManipulation(hm *headers.HeaderManipulation) *headers.HeaderManipulation { + if hm == nil { + return &headers.HeaderManipulation{} + } + return hm.Clone().(*headers.HeaderManipulation) +} + func requestHeadersToAdd(add []gwv1.HTTPHeader, set []gwv1.HTTPHeader) []*core.HeaderValueOption { envoyHeaders := make([]*core.HeaderValueOption, 0, len(add)+len(set)) envoyHeaders = append(envoyHeaders, translateHeaders(add, true)...) diff --git a/projects/gateway2/translator/plugins/headermodifier/header_modifier_plugin_test.go b/projects/gateway2/translator/plugins/headermodifier/header_modifier_plugin_test.go index 8f69fadf3bf..3952f88a2fa 100644 --- a/projects/gateway2/translator/plugins/headermodifier/header_modifier_plugin_test.go +++ b/projects/gateway2/translator/plugins/headermodifier/header_modifier_plugin_test.go @@ -1,8 +1,13 @@ package headermodifier_test import ( + "context" + "github.com/golang/protobuf/ptypes/wrappers" . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "google.golang.org/protobuf/proto" + "github.com/solo-io/gloo/projects/gateway2/translator/plugins" "github.com/solo-io/gloo/projects/gateway2/translator/plugins/filtertests" "github.com/solo-io/gloo/projects/gateway2/translator/plugins/headermodifier" @@ -121,3 +126,95 @@ var _ = DescribeTable( }, ), ) + +var _ = Describe("HeaderModifierPlugin mutation safety", func() { + It("does not mutate a HeaderManipulation already present on the route options", func() { + // A route's options sub-messages can be shared with every other route referencing the + // same RouteOption (solo-io/solo-projects#8802). Parent filters re-applied to delegated + // child routes reach this plugin with options already populated from RouteOptions, so it + // must never write into the existing HeaderManipulation in place. + shared := &headers.HeaderManipulation{ + RequestHeadersToAdd: []*core.HeaderValueOption{ + { + HeaderOption: &core.HeaderValueOption_Header{ + Header: &core.HeaderValue{Key: "from-route-option", Value: "original"}, + }, + Append: &wrappers.BoolValue{Value: true}, + }, + }, + ResponseHeadersToRemove: []string{"x-strip-response"}, + } + snapshot := proto.Clone(shared).(*headers.HeaderManipulation) + + outputRoute := &v1.Route{Options: &v1.RouteOptions{HeaderManipulation: shared}} + rtCtx := &plugins.RouteContext{ + HTTPRoute: &gwv1.HTTPRoute{}, + Rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{{ + Type: gwv1.HTTPRouteFilterRequestHeaderModifier, + RequestHeaderModifier: &gwv1.HTTPHeaderFilter{ + Add: []gwv1.HTTPHeader{{Name: "foo", Value: "bar"}}, + }, + }}, + }, + } + + err := headermodifier.NewPlugin().ApplyRoutePlugin(context.Background(), rtCtx, outputRoute) + Expect(err).NotTo(HaveOccurred()) + + // the filter's headers land on the route... + result := outputRoute.GetOptions().GetHeaderManipulation() + Expect(result.GetRequestHeadersToAdd()).To(HaveLen(1)) + Expect(result.GetRequestHeadersToAdd()[0].GetHeader().GetKey()).To(Equal("foo")) + // ...fields the filter does not touch survive from the original message... + Expect(result.GetResponseHeadersToRemove()).To(ConsistOf("x-strip-response")) + // ...and the original stays untouched. + Expect(proto.Equal(shared, snapshot)).To(BeTrue(), + "ApplyRoutePlugin mutated a HeaderManipulation shared with other routes") + }) + + It("does not mutate a HeaderManipulation already present on the route options (response filter)", func() { + // applyResponseFilter is an independent write path from applyRequestFilter; it needs its + // own pin so neither can regress to writing into the shared message in place. + shared := &headers.HeaderManipulation{ + RequestHeadersToAdd: []*core.HeaderValueOption{ + { + HeaderOption: &core.HeaderValueOption_Header{ + Header: &core.HeaderValue{Key: "from-route-option", Value: "original"}, + }, + Append: &wrappers.BoolValue{Value: true}, + }, + }, + ResponseHeadersToRemove: []string{"x-strip-response"}, + } + snapshot := proto.Clone(shared).(*headers.HeaderManipulation) + + outputRoute := &v1.Route{Options: &v1.RouteOptions{HeaderManipulation: shared}} + rtCtx := &plugins.RouteContext{ + HTTPRoute: &gwv1.HTTPRoute{}, + Rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{{ + Type: gwv1.HTTPRouteFilterResponseHeaderModifier, + ResponseHeaderModifier: &gwv1.HTTPHeaderFilter{ + Add: []gwv1.HTTPHeader{{Name: "foo", Value: "bar"}}, + }, + }}, + }, + } + + err := headermodifier.NewPlugin().ApplyRoutePlugin(context.Background(), rtCtx, outputRoute) + Expect(err).NotTo(HaveOccurred()) + + // the filter's headers land on the route... + result := outputRoute.GetOptions().GetHeaderManipulation() + Expect(result.GetResponseHeadersToAdd()).To(HaveLen(1)) + Expect(result.GetResponseHeadersToAdd()[0].GetHeader().GetKey()).To(Equal("foo")) + // ...fields the filter does not touch survive from the original message (the request + // side here: the response filter legitimately overwrites the response-side fields)... + Expect(result.GetRequestHeadersToAdd()).To(HaveLen(1)) + Expect(result.GetRequestHeadersToAdd()[0].GetHeader().GetKey()).To(Equal("from-route-option")) + // ...and the original stays untouched. + Expect(proto.Equal(shared, snapshot)).To(BeTrue(), + "ApplyRoutePlugin mutated a HeaderManipulation shared with other routes") + }) +}) diff --git a/projects/gateway2/translator/plugins/routeoptions/query/query.go b/projects/gateway2/translator/plugins/routeoptions/query/query.go index 1380bb46515..347d98ab3ca 100644 --- a/projects/gateway2/translator/plugins/routeoptions/query/query.go +++ b/projects/gateway2/translator/plugins/routeoptions/query/query.go @@ -6,18 +6,18 @@ import ( "github.com/hashicorp/go-multierror" "github.com/rotisserie/eris" apierrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" gwv1 "sigs.k8s.io/gateway-api/apis/v1" + "github.com/solo-io/gloo/pkg/utils/envutils" sologatewayv1 "github.com/solo-io/gloo/projects/gateway/pkg/api/v1" solokubev1 "github.com/solo-io/gloo/projects/gateway/pkg/api/v1/kube/apis/gateway.solo.io/v1" - gwquery "github.com/solo-io/gloo/projects/gateway2/query" utils "github.com/solo-io/gloo/projects/gateway2/translator/plugins/utils" gwutils "github.com/solo-io/gloo/projects/gateway2/utils" + "github.com/solo-io/gloo/projects/gloo/constants" gloov1 "github.com/solo-io/gloo/projects/gloo/pkg/api/v1" glooutils "github.com/solo-io/gloo/projects/gloo/pkg/utils" "github.com/solo-io/solo-kit/pkg/api/v1/resources/core" @@ -28,6 +28,14 @@ var routeOptionGK = schema.GroupKind{ Kind: sologatewayv1.RouteOptionGVK.Kind, } +// RouteOptionInterningEnabled gates the per-pass RouteOption interning memory optimization +// (solo-io/solo-projects#8802). When false (the default), each attached RouteOption is deep-copied +// per route rule as before; set GG_ROUTE_OPTION_INTERNING truthy to share one interned copy per +// unique RouteOption across all routes in a translation pass and skip the cache's per-call deep +// copies. Read once at init and captured per query in NewQuery so a pass cannot flip mid-translation; +// exported so tests can toggle it directly. +var RouteOptionInterningEnabled = envutils.IsEnvTruthy(constants.GlooGatewayRouteOptionInterningEnv) + type RouteOptionQueries interface { // GetRouteOptionForRouteRule returns the RouteOption attached to the given route and rule. // @@ -40,50 +48,158 @@ type RouteOptionQueries interface { // resource is considered for the merge. // // It returns the merged RouteOption, a list of sources corresponding to the merge, and an error if one occurs. + // + // MEMORY/MUTABILITY CONTRACT: by default each attached RouteOption is deep-copied per route + // rule (via the cache-backed client and ShallowMergeRouteOptions), matching historical behavior. + // + // When GG_ROUTE_OPTION_INTERNING is enabled (see RouteOptionInterningEnabled), the query + // instead avoids deep-copying potentially large RouteOptions for every route rule on every + // translation (solo-io/solo-projects#8802): the lookups pass client.UnsafeDisableDeepCopy and + // the query interns one deep copy per unique RouteOption for its own lifetime (a fresh query is + // constructed per translation pass and retained with that pass's output; see NewQuery). In that + // mode the merged RouteOption shares its options sub-messages with the interned copy, never with + // the informer cache, so the cache cannot be corrupted through the merged result. The merged + // options are always a distinct top-level message, so callers may reassign its top-level fields; + // when interning is enabled they must not mutate nested messages, slices, or maps reachable from + // it, since those are shared with every other route referencing the same RouteOption in this pass. GetRouteOptionForRouteRule( ctx context.Context, route types.NamespacedName, rule *gwv1.HTTPRouteRule, - gwQueries gwquery.GatewayQueries, ) (*solokubev1.RouteOption, []*gloov1.SourceMetadata_SourceRef, error) } type routeOptionQueries struct { c client.Client + + // intern captures RouteOptionInterningEnabled at construction so the behavior is stable for + // the query's lifetime (one translation pass) and cannot flip mid-translation. + intern bool + + // interned holds this query's one deep copy per unique RouteOption (only used when intern is + // true). An entry is replaced when the cached RouteOption's resourceVersion moves, so a lookup + // can never be served stale options and the map stays bounded by the number of RouteOptions + // even if the query outlives the translation pass it was built for. The interned copies keep + // the informer + // cache unreachable from translation output while still sharing one copy across all routes + // that reference the same RouteOption (solo-io/solo-projects#8802). Not safe for concurrent + // use: route plugins run sequentially within a pass. + interned map[types.NamespacedName]internedRouteOption +} + +// internedRouteOption is one RouteOption's deep-copied options plus the resourceVersion they +// were copied at. +type internedRouteOption struct { + resourceVersion string + options *gloov1.RouteOptions } +// NewQuery returns a RouteOptionQueries meant to live for a single translation pass: the proxy +// syncer builds a fresh plugin registry — and with it a fresh query — per pass, and retains it +// with that pass's output for status syncing. The query's interned RouteOption copies are +// retained along with it, which is what bounds translation memory at one copy per unique +// RouteOption per pass instead of one per route (solo-io/solo-projects#8802). func NewQuery(c client.Client) RouteOptionQueries { - return &routeOptionQueries{c} + return &routeOptionQueries{ + c: c, + intern: RouteOptionInterningEnabled, + interned: map[types.NamespacedName]internedRouteOption{}, + } +} + +// internedOptions returns this query's private deep copy of the RouteOption's options, cloning +// on first sight or when the cached object's resourceVersion has moved since the copy was +// taken. opt is shared with the informer cache (the lookups disable deep copies) and is only +// ever read, never written to. +func (r *routeOptionQueries) internedOptions(opt *solokubev1.RouteOption) *gloov1.RouteOptions { + src := opt.Spec.GetOptions() + if src == nil { + return nil + } + key := types.NamespacedName{Namespace: opt.GetNamespace(), Name: opt.GetName()} + if entry, ok := r.interned[key]; ok && entry.resourceVersion == opt.GetResourceVersion() { + return entry.options + } + copied := src.Clone().(*gloov1.RouteOptions) + r.interned[key] = internedRouteOption{ + resourceVersion: opt.GetResourceVersion(), + options: copied, + } + return copied } func (r *routeOptionQueries) GetRouteOptionForRouteRule( ctx context.Context, route types.NamespacedName, rule *gwv1.HTTPRouteRule, - gwQueries gwquery.GatewayQueries, ) (*solokubev1.RouteOption, []*gloov1.SourceMetadata_SourceRef, error) { var sources []*gloov1.SourceMetadata_SourceRef merged := &solokubev1.RouteOption{} - filterAttachments, err := lookupFilterAttachments(ctx, route, rule, gwQueries) - if err != nil { - return nil, nil, err - } - for _, opt := range filterAttachments { + // mergeAttachment folds a single RouteOption attachment into the accumulated `merged` result, + // recording it as a source if any of its fields were used. + // + // When interning is enabled the merge reads from the query's interned copy of each attachment, + // never from the cache-shared object itself, and the first attachment seeds `merged` with a + // shallow copy (sharing the interned copy's 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. + // + // When interning is disabled the merge reads the cache-backed client's per-call deep copies + // directly and ShallowMergeRouteOptions deep-clones the first source, matching historical + // behavior. + mergeAttachment := func(opt *solokubev1.RouteOption) { + if !r.intern { + optionUsed := false + merged.Spec.Options, optionUsed = glooutils.ShallowMergeRouteOptions(merged.Spec.GetOptions(), opt.Spec.GetOptions()) + if optionUsed { + sources = append(sources, routeOptionToSourceRef(opt)) + } + return + } + + options := r.internedOptions(opt) + if options == nil { + return + } optionUsed := false - merged.Spec.Options, optionUsed = glooutils.ShallowMergeRouteOptions(merged.Spec.GetOptions(), opt.Spec.GetOptions()) + if merged.Spec.GetOptions() == nil { + merged.Spec.Options = glooutils.ShallowCopyRouteOptions(options) + optionUsed = true + } else { + merged.Spec.Options, optionUsed = glooutils.ShallowMergeRouteOptions(merged.Spec.GetOptions(), options) + } if optionUsed { sources = append(sources, routeOptionToSourceRef(opt)) } } - var list solokubev1.RouteOptionList - if err := r.c.List( - ctx, - &list, + filterAttachments, err := r.lookupFilterAttachments(ctx, route, rule) + if err != nil { + return nil, nil, err + } + for _, opt := range filterAttachments { + mergeAttachment(opt) + } + + listOpts := []client.ListOption{ client.MatchingFieldsSelector{Selector: fields.OneTermEqualSelector(RouteOptionTargetField, route.String())}, client.InNamespace(route.Namespace), - ); err != nil { + } + if r.intern { + // Skip the client's per-call deep copy out of the cache: the merge only ever reads this + // query's interned copies (see internedOptions), so a per-call copy would be pure + // allocation churn thrown away after each lookup of a query that runs for every route + // rule on every translation (solo-io/solo-projects#8802). The returned objects are + // shared with the cache and are only read, never written. + listOpts = append(listOpts, client.UnsafeDisableDeepCopy) + } + + var list solokubev1.RouteOptionList + if err := r.c.List(ctx, &list, listOpts...); err != nil { return nil, nil, err } @@ -97,11 +213,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 @@ -114,12 +226,12 @@ func nilOptionIfEmpty(opt *solokubev1.RouteOption) *solokubev1.RouteOption { return opt } -// lookupFilterAttachments returns the RouteOptions attached to the route via ExtensionRef filters on the route's rule -func lookupFilterAttachments( +// lookupFilterAttachments returns the RouteOptions attached to the route via ExtensionRef filters on the route's rule. +// ExtensionRefs are local object references, so the lookup is always in the route's namespace. +func (r *routeOptionQueries) lookupFilterAttachments( ctx context.Context, route types.NamespacedName, rule *gwv1.HTTPRouteRule, - gwQueries gwquery.GatewayQueries, ) ([]*solokubev1.RouteOption, error) { if rule == nil { return nil, nil @@ -130,11 +242,24 @@ func lookupFilterAttachments( return nil, nil } + var getOpts []client.GetOption + if r.intern { + // Shared with the cache and only read, never written, same as the List in + // GetRouteOptionForRouteRule; the merge reads only this query's interned copies + // (see internedOptions). + getOpts = append(getOpts, client.UnsafeDisableDeepCopy) + } + var out []*solokubev1.RouteOption var multiErr *multierror.Error - extLookup := extensionRefLookup{namespace: route.Namespace} for _, filter := range filters { - routeOption, err := utils.GetExtensionRefObjFrom[*solokubev1.RouteOption](ctx, extLookup, gwQueries, filter.ExtensionRef) + routeOption := &solokubev1.RouteOption{} + err := r.c.Get( + ctx, + types.NamespacedName{Namespace: route.Namespace, Name: string(filter.ExtensionRef.Name)}, + routeOption, + getOpts..., + ) if err != nil { // If the filter is not found, report a specific error so that it can reflect more // clearly on the status of the HTTPRoute. @@ -151,21 +276,6 @@ func lookupFilterAttachments( return out, multiErr.ErrorOrNil() } -type extensionRefLookup struct { - namespace string -} - -func (e extensionRefLookup) GroupKind() (metav1.GroupKind, error) { - return metav1.GroupKind{ - Group: routeOptionGK.Group, - Kind: routeOptionGK.Kind, - }, nil -} - -func (e extensionRefLookup) Namespace() string { - return e.namespace -} - func errFilterNotFound(namespace string, filter *gwv1.HTTPRouteFilter) error { return eris.Errorf( "extensionRef '%s' of type %s.%s in namespace '%s' not found", diff --git a/projects/gateway2/translator/plugins/routeoptions/query/query_sharing_test.go b/projects/gateway2/translator/plugins/routeoptions/query/query_sharing_test.go new file mode 100644 index 00000000000..b1b5ec10f06 --- /dev/null +++ b/projects/gateway2/translator/plugins/routeoptions/query/query_sharing_test.go @@ -0,0 +1,334 @@ +package query_test + +import ( + "context" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/types/known/wrapperspb" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + "github.com/solo-io/gloo/pkg/schemes" + solokubev1 "github.com/solo-io/gloo/projects/gateway/pkg/api/v1/kube/apis/gateway.solo.io/v1" + "github.com/solo-io/gloo/projects/gateway2/translator/plugins/routeoptions/query" +) + +// recordingClient wraps a client.Client to capture the RouteOption objects it returns and the +// options each call was made with. It lets tests pin the contracts that keep translation heap +// bounded when many routes reference the same RouteOption (solo-io/solo-projects#8802): +// - RouteOption lookups must not deep-copy out of the cache on every call, +// - the query must deep-copy each unique RouteOption exactly once (the interned copy), and +// - the merged result must share the interned copy's sub-messages — never the client's +// objects, so the informer cache stays unreachable from translation output. +type recordingClient struct { + client.Client + + returned []*solokubev1.RouteOption + getOpts [][]client.GetOption + listOpts [][]client.ListOption +} + +func (r *recordingClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + err := r.Client.Get(ctx, key, obj, opts...) + if ro, ok := obj.(*solokubev1.RouteOption); ok { + r.getOpts = append(r.getOpts, opts) + if err == nil { + r.returned = append(r.returned, ro) + } + } + return err +} + +func (r *recordingClient) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + err := r.Client.List(ctx, list, opts...) + if rol, ok := list.(*solokubev1.RouteOptionList); ok { + r.listOpts = append(r.listOpts, opts) + if err == nil { + for i := range rol.Items { + r.returned = append(r.returned, &rol.Items[i]) + } + } + } + return err +} + +func disablesDeepCopyOnList(opts []client.ListOption) bool { + lo := &client.ListOptions{} + lo.ApplyOptions(opts) + return lo.UnsafeDisableDeepCopy != nil && *lo.UnsafeDisableDeepCopy +} + +func disablesDeepCopyOnGet(opts []client.GetOption) bool { + gOpts := &client.GetOptions{} + gOpts.ApplyOptions(opts) + return gOpts.UnsafeDisableDeepCopy != nil && *gOpts.UnsafeDisableDeepCopy +} + +var _ = Describe("Query no-clone contract", func() { + var ( + ctx context.Context + builder *fake.ClientBuilder + origInterning bool + ) + + BeforeEach(func() { + ctx = context.Background() + // These specs pin the interning behavior, which is opt-in via GG_ROUTE_OPTION_INTERNING. + origInterning = query.RouteOptionInterningEnabled + query.RouteOptionInterningEnabled = true + builder = fake.NewClientBuilder().WithScheme(schemes.GatewayScheme()) + query.IterateIndices(func(o client.Object, f string, fun client.IndexerFunc) error { + builder.WithIndex(o, f, fun) + return nil + }) + }) + + AfterEach(func() { + query.RouteOptionInterningEnabled = origInterning + }) + + It("does not deep-copy targetRef-attached RouteOptions on every lookup", func() { + hr := httpRoute() + rec := &recordingClient{Client: builder.WithObjects(hr, attachedRouteOption()).Build()} + q := query.NewQuery(rec) + + _, _, err := q.GetRouteOptionForRouteRule(ctx, types.NamespacedName{Namespace: hr.GetNamespace(), Name: hr.GetName()}, nil) + Expect(err).NotTo(HaveOccurred()) + + Expect(rec.listOpts).ToNot(BeEmpty()) + for _, opts := range rec.listOpts { + Expect(disablesDeepCopyOnList(opts)).To(BeTrue(), + "RouteOption List must pass client.UnsafeDisableDeepCopy: the merge reads only the query's "+ + "interned copies, so a per-call deep copy out of the cache is pure allocation churn on a "+ + "query that runs per route rule per translation (solo-io/solo-projects#8802)") + } + }) + + It("does not deep-copy extensionRef-attached RouteOptions on every lookup", func() { + hr := httpRouteWithFilters() + rec := &recordingClient{Client: builder.WithObjects(hr, attachedRouteOption1(), attachedRouteOption2()).Build()} + q := query.NewQuery(rec) + + _, _, err := q.GetRouteOptionForRouteRule(ctx, types.NamespacedName{Namespace: hr.GetNamespace(), Name: hr.GetName()}, &hr.Spec.Rules[0]) + Expect(err).NotTo(HaveOccurred()) + + Expect(rec.getOpts).To(HaveLen(2)) + for _, opts := range rec.getOpts { + Expect(disablesDeepCopyOnGet(opts)).To(BeTrue(), + "RouteOption Get must pass client.UnsafeDisableDeepCopy: the merge reads only the query's "+ + "interned copies, so a per-call deep copy out of the cache is pure allocation churn on a "+ + "query that runs per route rule per translation (solo-io/solo-projects#8802)") + } + }) + + It("merges every source through interned copies, shared across route rules", func() { + hr := httpRouteWithFilters() + rec := &recordingClient{Client: builder.WithObjects(hr, attachedRouteOption1(), attachedRouteOption2(), attachedRouteOption3()).Build()} + q := query.NewQuery(rec) + + nn := types.NamespacedName{Namespace: hr.GetNamespace(), Name: hr.GetName()} + merged, sources, err := q.GetRouteOptionForRouteRule(ctx, nn, &hr.Spec.Rules[0]) + Expect(err).NotTo(HaveOccurred()) + Expect(merged).NotTo(BeNil()) + Expect(sources).To(HaveLen(3)) + + byName := map[string]*solokubev1.RouteOption{} + for _, ro := range rec.returned { + byName[ro.GetName()] = ro + } + + // Highest priority source (first extensionRef) wins Faults; lower priority sources + // augment with the fields unset in higher priority ones. Each merged field must carry the + // winning source's value without aliasing the client's (cache-shared) objects. + Expect(proto.Equal(merged.Spec.GetOptions().GetFaults(), byName["good-policy"].Spec.GetOptions().GetFaults())).To(BeTrue()) + Expect(proto.Equal(merged.Spec.GetOptions().GetPrefixRewrite(), byName["good-policy2"].Spec.GetOptions().GetPrefixRewrite())).To(BeTrue()) + Expect(proto.Equal(merged.Spec.GetOptions().GetTimeout(), byName["good-policy3"].Spec.GetOptions().GetTimeout())).To(BeTrue()) + Expect(merged.Spec.GetOptions().GetFaults()).NotTo(BeIdenticalTo(byName["good-policy"].Spec.GetOptions().GetFaults())) + Expect(merged.Spec.GetOptions().GetPrefixRewrite()).NotTo(BeIdenticalTo(byName["good-policy2"].Spec.GetOptions().GetPrefixRewrite())) + Expect(merged.Spec.GetOptions().GetTimeout()).NotTo(BeIdenticalTo(byName["good-policy3"].Spec.GetOptions().GetTimeout())) + + // A second route rule referencing the same RouteOptions must share the interned copies + // rather than clone again: one deep copy per unique RouteOption per pass. + merged2, _, err := q.GetRouteOptionForRouteRule(ctx, nn, &hr.Spec.Rules[0]) + Expect(err).NotTo(HaveOccurred()) + Expect(merged2.Spec.GetOptions().GetFaults()).To(BeIdenticalTo(merged.Spec.GetOptions().GetFaults())) + Expect(merged2.Spec.GetOptions().GetPrefixRewrite()).To(BeIdenticalTo(merged.Spec.GetOptions().GetPrefixRewrite())) + Expect(merged2.Spec.GetOptions().GetTimeout()).To(BeIdenticalTo(merged.Spec.GetOptions().GetTimeout())) + }) + + It("never exposes the client's objects through the merged result", func() { + hr := httpRoute() + rec := &recordingClient{Client: builder.WithObjects(hr, attachedRouteOption()).Build()} + q := query.NewQuery(rec) + + merged, _, err := q.GetRouteOptionForRouteRule(ctx, types.NamespacedName{Namespace: hr.GetNamespace(), Name: hr.GetName()}, nil) + Expect(err).NotTo(HaveOccurred()) + Expect(merged).NotTo(BeNil()) + Expect(rec.returned).To(HaveLen(1)) + + // The lookups disable deep copies, so the returned objects stand in for the informer + // cache: nothing reachable from the merged result may alias them. The merge must be fed + // from the query's own per-pass interned copy, so that a nested mutation downstream can + // at worst contaminate this pass's output, never the cache itself. + Expect(merged.Spec.GetOptions().GetFaults()).NotTo(BeIdenticalTo(rec.returned[0].Spec.GetOptions().GetFaults()), + "merged options must not alias the objects returned by the cache-backed client") + Expect(proto.Equal(merged.Spec.GetOptions(), rec.returned[0].Spec.GetOptions())).To(BeTrue()) + }) + + It("shares one interned copy across all routes referencing the same RouteOption", func() { + hr := httpRoute() + rec := &recordingClient{Client: builder.WithObjects(hr, attachedRouteOption()).Build()} + q := query.NewQuery(rec) + + nn := types.NamespacedName{Namespace: hr.GetNamespace(), Name: hr.GetName()} + merged1, _, err := q.GetRouteOptionForRouteRule(ctx, nn, nil) + Expect(err).NotTo(HaveOccurred()) + merged2, _, err := q.GetRouteOptionForRouteRule(ctx, nn, nil) + Expect(err).NotTo(HaveOccurred()) + + // One deep copy per unique RouteOption per query lifetime (= per translation pass): + // route rules referencing the same RouteOption must share the interned copy's + // sub-messages instead of each receiving a private clone — per-route clones are what + // caused the OOM in #8802. + Expect(merged1.Spec.GetOptions().GetFaults()).To(BeIdenticalTo(merged2.Spec.GetOptions().GetFaults())) + // ...while the top-level options message stays distinct per route, so route plugins can + // keep reassigning top-level fields without affecting other routes. + Expect(merged1.Spec.GetOptions()).NotTo(BeIdenticalTo(merged2.Spec.GetOptions())) + }) + + It("does not share interned copies across queries", func() { + hr := httpRoute() + c := builder.WithObjects(hr, attachedRouteOption()).Build() + + nn := types.NamespacedName{Namespace: hr.GetNamespace(), Name: hr.GetName()} + merged1, _, err := query.NewQuery(c).GetRouteOptionForRouteRule(ctx, nn, nil) + Expect(err).NotTo(HaveOccurred()) + merged2, _, err := query.NewQuery(c).GetRouteOptionForRouteRule(ctx, nn, nil) + Expect(err).NotTo(HaveOccurred()) + + // Each translation pass constructs its own query (via the per-pass plugin registry), so + // interned copies never leak across passes; each pass's copies are retained only as long + // as that pass's output. + Expect(merged1.Spec.GetOptions().GetFaults()).NotTo(BeIdenticalTo(merged2.Spec.GetOptions().GetFaults())) + }) + + It("does not serve a stale interned copy after the RouteOption is updated", func() { + hr := httpRoute() + c := builder.WithObjects(hr, attachedRouteOption()).Build() + q := query.NewQuery(c) + nn := types.NamespacedName{Namespace: hr.GetNamespace(), Name: hr.GetName()} + + merged1, _, err := q.GetRouteOptionForRouteRule(ctx, nn, nil) + Expect(err).NotTo(HaveOccurred()) + + // Update the RouteOption through the same client; the fake client bumps its + // resourceVersion, just as the informer cache hands the query a newer object when a + // watch event lands mid-pass. + updated := &solokubev1.RouteOption{} + Expect(c.Get(ctx, types.NamespacedName{Namespace: "default", Name: "good-policy"}, updated)).To(Succeed()) + updated.Spec.GetOptions().PrefixRewrite = wrapperspb.String("/updated") + Expect(c.Update(ctx, updated)).To(Succeed()) + + merged2, _, err := q.GetRouteOptionForRouteRule(ctx, nn, nil) + Expect(err).NotTo(HaveOccurred()) + + // The intern map must replace its copy when the resourceVersion moves rather than serve + // the stale one. + Expect(merged2.Spec.GetOptions().GetPrefixRewrite().GetValue()).To(Equal("/updated"), + "a lookup after an update must reflect the updated RouteOption, not a stale interned copy") + Expect(merged2.Spec.GetOptions().GetFaults()).NotTo(BeIdenticalTo(merged1.Spec.GetOptions().GetFaults()), + "the updated RouteOption must get its own interned copy") + }) + + It("does not mutate the RouteOption objects returned by the client", func() { + hr := httpRouteWithFilters() + rec := &recordingClient{Client: builder.WithObjects(hr, attachedRouteOption1(), attachedRouteOption2(), attachedRouteOption3()).Build()} + q := query.NewQuery(rec) + + _, _, err := q.GetRouteOptionForRouteRule(ctx, types.NamespacedName{Namespace: hr.GetNamespace(), Name: hr.GetName()}, &hr.Spec.Rules[0]) + Expect(err).NotTo(HaveOccurred()) + + // With deep copies disabled the returned objects are shared with the underlying cache, + // so the merge must never write into them. Compare against freshly constructed fixtures. + fixtures := map[string]*solokubev1.RouteOption{ + "good-policy": attachedRouteOption1(), + "good-policy2": attachedRouteOption2(), + "good-policy3": attachedRouteOption3(), + } + Expect(rec.returned).NotTo(BeEmpty()) + for _, ro := range rec.returned { + fixture, ok := fixtures[ro.GetName()] + Expect(ok).To(BeTrue(), "unexpected RouteOption %q returned by the client", ro.GetName()) + Expect(proto.Equal(ro.Spec.GetOptions(), fixture.Spec.GetOptions())).To(BeTrue(), + "the merge mutated RouteOption %q, which is shared with the cache", ro.GetName()) + } + }) +}) + +var _ = Describe("Query interning disabled (default)", func() { + var ( + ctx context.Context + builder *fake.ClientBuilder + origInterning bool + ) + + BeforeEach(func() { + ctx = context.Background() + // Default behavior: interning is opt-in, so off here. + origInterning = query.RouteOptionInterningEnabled + query.RouteOptionInterningEnabled = false + builder = fake.NewClientBuilder().WithScheme(schemes.GatewayScheme()) + query.IterateIndices(func(o client.Object, f string, fun client.IndexerFunc) error { + builder.WithIndex(o, f, fun) + return nil + }) + }) + + AfterEach(func() { + query.RouteOptionInterningEnabled = origInterning + }) + + It("does not pass UnsafeDisableDeepCopy on lookups", func() { + hr := httpRouteWithFilters() + rec := &recordingClient{Client: builder.WithObjects(hr, attachedRouteOption1(), attachedRouteOption2()).Build()} + q := query.NewQuery(rec) + + _, _, err := q.GetRouteOptionForRouteRule(ctx, types.NamespacedName{Namespace: hr.GetNamespace(), Name: hr.GetName()}, &hr.Spec.Rules[0]) + Expect(err).NotTo(HaveOccurred()) + + // With interning off the cache-backed client must deep-copy each lookup (the historical, + // safe behavior): the merge reads the returned objects directly, so they must not be + // cache-shared. + Expect(rec.getOpts).To(HaveLen(2)) + for _, opts := range rec.getOpts { + Expect(disablesDeepCopyOnGet(opts)).To(BeFalse(), + "with interning disabled, RouteOption Get must not pass client.UnsafeDisableDeepCopy") + } + Expect(rec.listOpts).ToNot(BeEmpty()) + for _, opts := range rec.listOpts { + Expect(disablesDeepCopyOnList(opts)).To(BeFalse(), + "with interning disabled, RouteOption List must not pass client.UnsafeDisableDeepCopy") + } + }) + + It("deep-copies per lookup instead of sharing across route rules", func() { + hr := httpRoute() + rec := &recordingClient{Client: builder.WithObjects(hr, attachedRouteOption()).Build()} + q := query.NewQuery(rec) + + nn := types.NamespacedName{Namespace: hr.GetNamespace(), Name: hr.GetName()} + merged1, _, err := q.GetRouteOptionForRouteRule(ctx, nn, nil) + Expect(err).NotTo(HaveOccurred()) + merged2, _, err := q.GetRouteOptionForRouteRule(ctx, nn, nil) + Expect(err).NotTo(HaveOccurred()) + + // The merged output is still correct... + Expect(merged1.Spec.GetOptions().GetFaults().GetAbort().GetHttpStatus()).To(BeEquivalentTo(500)) + // ...but each route rule gets its own copy (no cross-route sharing) — the historical + // behavior the flag falls back to. + Expect(merged1.Spec.GetOptions().GetFaults()).NotTo(BeIdenticalTo(merged2.Spec.GetOptions().GetFaults())) + }) +}) diff --git a/projects/gateway2/translator/plugins/routeoptions/query/query_test.go b/projects/gateway2/translator/plugins/routeoptions/query/query_test.go index a0d65bb5711..b4124b00443 100644 --- a/projects/gateway2/translator/plugins/routeoptions/query/query_test.go +++ b/projects/gateway2/translator/plugins/routeoptions/query/query_test.go @@ -14,7 +14,6 @@ import ( sologatewayv1 "github.com/solo-io/gloo/projects/gateway/pkg/api/v1" solokubev1 "github.com/solo-io/gloo/projects/gateway/pkg/api/v1/kube/apis/gateway.solo.io/v1" "github.com/solo-io/gloo/projects/gateway2/translator/plugins/routeoptions/query" - "github.com/solo-io/gloo/projects/gateway2/translator/testutils" "github.com/solo-io/gloo/projects/gateway2/wellknown" v1 "github.com/solo-io/gloo/projects/gloo/pkg/api/v1" "github.com/solo-io/gloo/projects/gloo/pkg/api/v1/options/faultinjection" @@ -52,9 +51,8 @@ var _ = Describe("Query", func() { fakeClient := builder.WithObjects(deps...).Build() query := query.NewQuery(fakeClient) - gwQuery := testutils.BuildGatewayQueriesWithClient(fakeClient) - rtOpt, sources, err := query.GetRouteOptionForRouteRule(ctx, hrNsName, nil, gwQuery) + rtOpt, sources, err := query.GetRouteOptionForRouteRule(ctx, hrNsName, nil) Expect(err).NotTo(HaveOccurred()) Expect(rtOpt).ToNot(BeNil()) @@ -76,9 +74,8 @@ var _ = Describe("Query", func() { fakeClient := builder.WithObjects(deps...).Build() query := query.NewQuery(fakeClient) - gwQuery := testutils.BuildGatewayQueriesWithClient(fakeClient) - rtOpt, sources, err := query.GetRouteOptionForRouteRule(ctx, hrNsName, nil, gwQuery) + rtOpt, sources, err := query.GetRouteOptionForRouteRule(ctx, hrNsName, nil) Expect(err).NotTo(HaveOccurred()) Expect(rtOpt).To(BeNil()) @@ -100,9 +97,8 @@ var _ = Describe("Query", func() { fakeClient := builder.WithObjects(deps...).Build() query := query.NewQuery(fakeClient) - gwQuery := testutils.BuildGatewayQueriesWithClient(fakeClient) - rtOpt, sources, err := query.GetRouteOptionForRouteRule(ctx, hrNsName, nil, gwQuery) + rtOpt, sources, err := query.GetRouteOptionForRouteRule(ctx, hrNsName, nil) Expect(err).NotTo(HaveOccurred()) Expect(rtOpt).ToNot(BeNil()) @@ -124,9 +120,8 @@ var _ = Describe("Query", func() { fakeClient := builder.WithObjects(deps...).Build() query := query.NewQuery(fakeClient) - gwQuery := testutils.BuildGatewayQueriesWithClient(fakeClient) - rtOpt, sources, err := query.GetRouteOptionForRouteRule(ctx, hrNsName, nil, gwQuery) + rtOpt, sources, err := query.GetRouteOptionForRouteRule(ctx, hrNsName, nil) Expect(err).NotTo(HaveOccurred()) Expect(rtOpt).To(BeNil()) @@ -151,9 +146,8 @@ var _ = Describe("Query", func() { fakeClient := builder.WithObjects(deps...).Build() query := query.NewQuery(fakeClient) - gwQuery := testutils.BuildGatewayQueriesWithClient(fakeClient) - rtOpt, sources, err := query.GetRouteOptionForRouteRule(ctx, hrNsName, &hr.Spec.Rules[0], gwQuery) + rtOpt, sources, err := query.GetRouteOptionForRouteRule(ctx, hrNsName, &hr.Spec.Rules[0]) Expect(err).NotTo(HaveOccurred()) Expect(rtOpt).ToNot(BeNil()) diff --git a/projects/gateway2/translator/plugins/routeoptions/route_options_plugin.go b/projects/gateway2/translator/plugins/routeoptions/route_options_plugin.go index 1be60048d84..fa4d750879c 100644 --- a/projects/gateway2/translator/plugins/routeoptions/route_options_plugin.go +++ b/projects/gateway2/translator/plugins/routeoptions/route_options_plugin.go @@ -26,7 +26,6 @@ import ( "github.com/solo-io/gloo/projects/gateway2/reports" "github.com/solo-io/gloo/projects/gateway2/translator/plugins" rtoptquery "github.com/solo-io/gloo/projects/gateway2/translator/plugins/routeoptions/query" - "github.com/solo-io/gloo/projects/gateway2/translator/plugins/utils" "github.com/solo-io/gloo/projects/gateway2/translator/routeutils" "github.com/solo-io/gloo/projects/gateway2/wellknown" "github.com/solo-io/gloo/projects/gloo/pkg/api/grpc/validation" @@ -297,20 +296,15 @@ func (p *plugin) handleAttachment( ctx, types.NamespacedName{Name: routeCtx.HTTPRoute.Name, Namespace: routeCtx.HTTPRoute.Namespace}, routeCtx.Rule, - p.gwQueries, ) if err != nil { contextutils.LoggerFrom(ctx).Errorf("error getting RouteOptions for Route: %v", err) - switch { - case errors.Is(err, utils.ErrTypesNotEqual): - default: - routeCtx.Reporter.SetCondition(reports.RouteCondition{ - Type: gwv1.RouteConditionResolvedRefs, - Status: metav1.ConditionFalse, - Reason: gwv1.RouteReasonBackendNotFound, - Message: err.Error(), - }) - } + routeCtx.Reporter.SetCondition(reports.RouteCondition{ + Type: gwv1.RouteConditionResolvedRefs, + Status: metav1.ConditionFalse, + Reason: gwv1.RouteReasonBackendNotFound, + Message: err.Error(), + }) return nil, nil, nil, err } if attachedOption == nil || attachedOption.Spec.GetOptions() == nil { diff --git a/projects/gateway2/translator/plugins/routeoptions/route_options_plugin_mutation_test.go b/projects/gateway2/translator/plugins/routeoptions/route_options_plugin_mutation_test.go new file mode 100644 index 00000000000..1ae9f663e48 --- /dev/null +++ b/projects/gateway2/translator/plugins/routeoptions/route_options_plugin_mutation_test.go @@ -0,0 +1,154 @@ +package routeoptions + +import ( + "context" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/types/known/wrapperspb" + + solokubev1 "github.com/solo-io/gloo/projects/gateway/pkg/api/v1/kube/apis/gateway.solo.io/v1" + gwquery "github.com/solo-io/gloo/projects/gateway2/query" + "github.com/solo-io/gloo/projects/gateway2/translator/plugins" + rtoptquery "github.com/solo-io/gloo/projects/gateway2/translator/plugins/routeoptions/query" + "github.com/solo-io/gloo/projects/gateway2/translator/testutils" + "github.com/solo-io/gloo/projects/gateway2/wellknown" + v1 "github.com/solo-io/gloo/projects/gloo/pkg/api/v1" + "github.com/solo-io/gloo/projects/gloo/pkg/api/v1/options/headers" + "github.com/solo-io/gloo/projects/gloo/pkg/api/v1/options/shadowing" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// trackingClient wraps a client.Client and captures every RouteOption object it hands back, so +// tests can assert that route translation never mutates them. Since the RouteOption queries pass +// client.UnsafeDisableDeepCopy, the objects returned in production are shared with the informer +// cache: any in-place mutation of their nested messages would corrupt the cache and leak config +// across every route referencing the same RouteOption (solo-io/solo-projects#8802). +type trackingClient struct { + client.Client + + returned []*solokubev1.RouteOption +} + +func (t *trackingClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + err := t.Client.Get(ctx, key, obj, opts...) + if ro, ok := obj.(*solokubev1.RouteOption); ok && err == nil { + t.returned = append(t.returned, ro) + } + return err +} + +func (t *trackingClient) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + err := t.Client.List(ctx, list, opts...) + if rol, ok := list.(*solokubev1.RouteOptionList); ok && err == nil { + for i := range rol.Items { + t.returned = append(t.returned, &rol.Items[i]) + } + } + return err +} + +var _ = Describe("RouteOptionsPlugin cache mutation guard", func() { + var ( + ctx context.Context + rec *trackingClient + p *plugin + origInterning bool + ) + + // expectReturnedUnchanged compares every RouteOption the client handed out against a freshly + // constructed fixture: any difference means translation wrote into an object that production + // shares with the informer cache. + expectReturnedUnchanged := func() { + GinkgoHelper() + fixtures := map[string]*solokubev1.RouteOption{ + "filter-policy": routeOption(), + "policy": attachedRouteOption(), + } + Expect(rec.returned).NotTo(BeEmpty()) + for _, ro := range rec.returned { + fixture, ok := fixtures[ro.GetName()] + Expect(ok).To(BeTrue(), "unexpected RouteOption %q returned by the client", ro.GetName()) + Expect(proto.Equal(ro.Spec.GetOptions(), fixture.Spec.GetOptions())).To(BeTrue(), + "route translation mutated RouteOption %q in place; with deep copies disabled this "+ + "corrupts the shared client cache", ro.GetName()) + } + } + + BeforeEach(func() { + ctx = context.Background() + // This guard protects the interning (cache-sharing) path, which is opt-in via + // GG_ROUTE_OPTION_INTERNING; enable it so NewQuery captures it. + origInterning = rtoptquery.RouteOptionInterningEnabled + rtoptquery.RouteOptionInterningEnabled = true + deps := []client.Object{routeOption(), attachedRouteOption()} + fakeClient := testutils.BuildIndexedFakeClient(deps, gwquery.IterateIndices, rtoptquery.IterateIndices) + rec = &trackingClient{Client: fakeClient} + gwQueries := testutils.BuildGatewayQueriesWithClient(rec) + p = NewPlugin(gwQueries, rec, nil, nil) + }) + + AfterEach(func() { + rtoptquery.RouteOptionInterningEnabled = origInterning + }) + + It("merges attachments and existing route options without touching the returned RouteOptions", func() { + // The output route already carries options written by the builtin filter plugins that run + // before the routeoptions plugin (e.g. headermodifier); the merge must fold these into a + // per-route struct, never into the shared RouteOption objects. + outputRoute := &v1.Route{ + Options: &v1.RouteOptions{ + HeaderManipulation: &headers.HeaderManipulation{ + RequestHeadersToRemove: []string{"x-remove-me"}, + }, + }, + } + rtCtx := &plugins.RouteContext{ + HTTPRoute: routeWithFilter(), + Rule: routeRuleWithExtRef(), + } + + Expect(p.ApplyRoutePlugin(ctx, rtCtx, outputRoute)).To(Succeed()) + + // Sanity: the merge produced the expected combination (extensionRef policy wins Faults, + // pre-existing options are preserved). + Expect(outputRoute.GetOptions().GetFaults().GetAbort().GetHttpStatus()).To(BeEquivalentTo(500)) + Expect(outputRoute.GetOptions().GetHeaderManipulation().GetRequestHeadersToRemove()).To(ContainElement("x-remove-me")) + + // The merged options must be a distinct top-level struct, not one of the returned objects'. + for _, ro := range rec.returned { + Expect(outputRoute.GetOptions()).NotTo(BeIdenticalTo(ro.Spec.GetOptions())) + } + + // Plugins that run after routeoptions (urlrewrite, mirror, ...) reassign top-level fields + // of the merged options; simulate them and verify the shared objects stay untouched. + outputRoute.GetOptions().PrefixRewrite = wrapperspb.String("/rewritten") + outputRoute.GetOptions().Shadowing = &shadowing.RouteShadowing{Percentage: 50} + + expectReturnedUnchanged() + }) + + It("does not touch the returned RouteOptions when a delegated child overrides parent options", func() { + hr := routeWithFilter() + hr.Annotations = map[string]string{ + wellknown.PolicyOverrideAnnotation: "*", + } + // Simulate a delegated child route whose own options are allowed to override the parent's. + outputRoute := &v1.Route{ + Options: &v1.RouteOptions{ + PrefixRewrite: wrapperspb.String("/child-rewrite"), + }, + } + rtCtx := &plugins.RouteContext{ + HTTPRoute: hr, + Rule: routeRuleWithExtRef(), + } + + Expect(p.ApplyRoutePlugin(ctx, rtCtx, outputRoute)).To(Succeed()) + + Expect(outputRoute.GetOptions().GetPrefixRewrite().GetValue()).To(Equal("/child-rewrite")) + + expectReturnedUnchanged() + }) +}) diff --git a/projects/gateway2/translator/plugins/routeoptions/route_options_plugin_test.go b/projects/gateway2/translator/plugins/routeoptions/route_options_plugin_test.go index be8501ce95e..68520255e1e 100644 --- a/projects/gateway2/translator/plugins/routeoptions/route_options_plugin_test.go +++ b/projects/gateway2/translator/plugins/routeoptions/route_options_plugin_test.go @@ -96,7 +96,11 @@ var _ = Describe("RouteOptionsPlugin", func() { plugin := NewPlugin(gwQueries, fakeClient, routeOptionCollection, statusReporter) rtCtx := &plugins.RouteContext{ - HTTPRoute: &gwv1.HTTPRoute{}, + HTTPRoute: &gwv1.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + }, Rule: &gwv1.HTTPRouteRule{ Filters: []gwv1.HTTPRouteFilter{{ Type: gwv1.HTTPRouteFilterExtensionRef, @@ -337,7 +341,11 @@ var _ = Describe("RouteOptionsPlugin", func() { plugin := NewPlugin(gwQueries, fakeClient, routeOptionCollection, statusReporter) rtCtx := &plugins.RouteContext{ - HTTPRoute: &gwv1.HTTPRoute{}, + HTTPRoute: &gwv1.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + }, Rule: &gwv1.HTTPRouteRule{ Filters: []gwv1.HTTPRouteFilter{{ Type: gwv1.HTTPRouteFilterExtensionRef, diff --git a/projects/gloo/constants/gloo_gateway.go b/projects/gloo/constants/gloo_gateway.go index bbbc7bed0ba..dd7c44f9fa8 100644 --- a/projects/gloo/constants/gloo_gateway.go +++ b/projects/gloo/constants/gloo_gateway.go @@ -2,4 +2,10 @@ package constants const ( GlooGatewayEnableK8sGwControllerEnv = "GG_K8S_GW_CONTROLLER" + + // GlooGatewayRouteOptionInterningEnv, when truthy, enables per-pass interning of RouteOptions + // in the Kubernetes Gateway API translation: each unique RouteOption is deep-copied once per + // translation pass and shared across every route referencing it, instead of being deep-copied + // per route rule (solo-io/solo-projects#8802). Defaults to off (per-route deep copy). + GlooGatewayRouteOptionInterningEnv = "GG_ROUTE_OPTION_INTERNING" ) diff --git a/projects/gloo/pkg/plugins/transformation/plugin.go b/projects/gloo/pkg/plugins/transformation/plugin.go index 1d2114e4a04..883239beee7 100644 --- a/projects/gloo/pkg/plugins/transformation/plugin.go +++ b/projects/gloo/pkg/plugins/transformation/plugin.go @@ -395,6 +395,11 @@ func TranslateTransformation(glooTransform *transformation.Transformation, } case *transformation.Transformation_TransformationTemplate: { + // resolve the template -> staged -> settings inheritance into a local and pass it + // down instead of writing it back into the input template: the input is nested in + // route/vhost options whose sub-messages can be shared across every route + // referencing the same RouteOption (solo-io/solo-projects#8802), so a write-back + // would corrupt shared state and mask later changes to the Settings-level default. escapeCharacters := typedTransformation.TransformationTemplate.GetEscapeCharacters() if escapeCharacters == nil { escapeCharacters = stagedEscapeCharacters @@ -402,9 +407,8 @@ func TranslateTransformation(glooTransform *transformation.Transformation, if escapeCharacters == nil { escapeCharacters = settingsEscapeCharacters } - typedTransformation.TransformationTemplate.EscapeCharacters = escapeCharacters - transformationType, err := translateTransformationTemplate(typedTransformation) + transformationType, err := translateTransformationTemplate(typedTransformation, escapeCharacters) if err != nil { return nil, err } @@ -430,7 +434,10 @@ func translateHeaderBodyTransform(in *transformation.Transformation_HeaderBodyTr return out } -func translateTransformationTemplate(in *transformation.Transformation_TransformationTemplate) (*envoytransformation.Transformation_TransformationTemplate, error) { +// escapeCharacters is the template -> staged -> settings inheritance resolved by +// TranslateTransformation, passed explicitly because the (potentially shared) input template +// must not be written to. +func translateTransformationTemplate(in *transformation.Transformation_TransformationTemplate, escapeCharacters *wrapperspb.BoolValue) (*envoytransformation.Transformation_TransformationTemplate, error) { out := &envoytransformation.Transformation_TransformationTemplate{} inTemplate := in.TransformationTemplate outTemplate := &envoytransformation.TransformationTemplate{ @@ -438,7 +445,7 @@ func translateTransformationTemplate(in *transformation.Transformation_Transform HeadersToRemove: inTemplate.GetHeadersToRemove(), IgnoreErrorOnParse: inTemplate.GetIgnoreErrorOnParse(), ParseBodyBehavior: envoytransformation.TransformationTemplate_RequestBodyParse(inTemplate.GetParseBodyBehavior()), - EscapeCharacters: inTemplate.GetEscapeCharacters().GetValue(), // the inheritance is handled in TranslateTransformation + EscapeCharacters: escapeCharacters.GetValue(), } if len(inTemplate.GetExtractors()) > 0 { diff --git a/projects/gloo/pkg/plugins/transformation/plugin_test.go b/projects/gloo/pkg/plugins/transformation/plugin_test.go index aa6deefe525..297530957be 100644 --- a/projects/gloo/pkg/plugins/transformation/plugin_test.go +++ b/projects/gloo/pkg/plugins/transformation/plugin_test.go @@ -105,6 +105,28 @@ var _ = Describe("Plugin", func() { }) + It("does not mutate the input transformation when resolving escape characters", func() { + input := &transformation.Transformation{ + TransformationType: &transformation.Transformation_TransformationTemplate{ + TransformationTemplate: &transformation.TransformationTemplate{ + HeadersToRemove: []string{"x-remove-me"}, + }, + }, + } + snapshot := input.Clone().(*transformation.Transformation) + + output, err := TranslateTransformation(input, nil, &wrapperspb.BoolValue{Value: true}) + Expect(err).NotTo(HaveOccurred()) + // the resolved escape characters land on the envoy output... + Expect(output.GetTransformationTemplate().GetEscapeCharacters()).To(BeTrue()) + // ...but the input must stay untouched: route options share their sub-messages across + // every route referencing the same RouteOption (solo-io/solo-projects#8802), so + // writing the resolved value back into the input template would corrupt shared state + // and mask later changes to the Settings-level default. + Expect(skMatchers.MatchProto(snapshot).Match(input)).To(BeTrue(), + "TranslateTransformation mutated its input transformation") + }) + It("throws error on unsupported transformation type", func() { // Xslt Transformation is enterprise-only input := &transformation.Transformation{ 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_benchmark_test.go b/projects/gloo/pkg/utils/merge_benchmark_test.go new file mode 100644 index 00000000000..a3e162a905f --- /dev/null +++ b/projects/gloo/pkg/utils/merge_benchmark_test.go @@ -0,0 +1,78 @@ +package utils_test + +import ( + "fmt" + "testing" + + v1 "github.com/solo-io/gloo/projects/gloo/pkg/api/v1" + "github.com/solo-io/gloo/projects/gloo/pkg/api/v1/options/transformation" + "github.com/solo-io/gloo/projects/gloo/pkg/utils" +) + +// These benchmarks compare the per-route cost of the two ways a RouteOption attachment can seed +// the merged options during gateway2 translation (see GetRouteOptionForRouteRule): +// +// - Clone: the pre-#8802 behavior, deep-copying the entire options tree for every route rule on +// every translation. For transformation-heavy RouteOptions shared by many routes this +// dominated translation heap (~31% of a 14GB heap for one user). +// - ShallowCopyRouteOptions: the current behavior, allocating one top-level message per route +// and sharing the sub-messages. +// +// Proto deep-copy cost scales with the number of messages and map/slice entries (string bytes are +// shared), so the fixture carries many header templates and dynamic metadata values like the +// user templates in solo-io/solo-projects#8802. + +func BenchmarkRouteOptionsDeepClone(b *testing.B) { + src := largeTransformationRouteOptions() + b.ReportAllocs() + for b.Loop() { + if out := src.Clone().(*v1.RouteOptions); out == nil { + b.Fatal("expected clone") + } + } +} + +func BenchmarkShallowCopyRouteOptions(b *testing.B) { + src := largeTransformationRouteOptions() + b.ReportAllocs() + for b.Loop() { + if out := utils.ShallowCopyRouteOptions(src); out == nil { + b.Fatal("expected copy") + } + } +} + +func largeTransformationRouteOptions() *v1.RouteOptions { + headers := make(map[string]*transformation.InjaTemplate, 1000) + for i := range 1000 { + headers[fmt.Sprintf("x-generated-header-%d", i)] = &transformation.InjaTemplate{ + Text: fmt.Sprintf(`{{ request_header("x-input-%d") }}`, i), + } + } + metadataValues := make([]*transformation.TransformationTemplate_DynamicMetadataValue, 0, 500) + for i := range 500 { + metadataValues = append(metadataValues, &transformation.TransformationTemplate_DynamicMetadataValue{ + MetadataNamespace: "io.solo.benchmark", + Key: fmt.Sprintf("key-%d", i), + Value: &transformation.InjaTemplate{Text: fmt.Sprintf("value-%d", i)}, + }) + } + return &v1.RouteOptions{ + StagedTransformations: &transformation.TransformationStages{ + Regular: &transformation.RequestResponseTransformations{ + RequestTransforms: []*transformation.RequestMatch{ + { + RequestTransformation: &transformation.Transformation{ + TransformationType: &transformation.Transformation_TransformationTemplate{ + TransformationTemplate: &transformation.TransformationTemplate{ + Headers: headers, + DynamicMetadataValues: metadataValues, + }, + }, + }, + }, + }, + }, + }, + } +} 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")) + }) + }) })