Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions changelog/v1.22.0-beta13/routeoptions-per-route-clone-oom.yaml
Original file line number Diff line number Diff line change
@@ -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.
4 changes: 4 additions & 0 deletions projects/gateway2/extensions/extensions.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions projects/gateway2/proxy_syncer/proxy_syncer.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)...)
Expand Down
Original file line number Diff line number Diff line change
@@ -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"
Expand Down Expand Up @@ -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")
})
})
Loading