From d44ebda924bb5488b19248593e04617e2b39b2be Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Mon, 17 Nov 2025 18:55:39 -0700 Subject: [PATCH 1/2] add session persistence support for NGINX plus users --- .../controller/nginx/config/http/config.go | 9 + internal/controller/nginx/config/upstreams.go | 18 + .../nginx/config/upstreams_template.go | 6 + .../controller/nginx/config/upstreams_test.go | 242 +++++- .../nginx/config/validation/common.go | 61 ++ .../nginx/config/validation/common_test.go | 28 + .../nginx/config/validation/framework_test.go | 42 ++ .../nginx/config/validation/http_validator.go | 1 + .../controller/state/conditions/conditions.go | 15 + .../state/dataplane/configuration.go | 27 +- .../state/dataplane/configuration_test.go | 157 +++- internal/controller/state/dataplane/types.go | 21 + internal/controller/state/graph/graph.go | 11 + internal/controller/state/graph/graph_test.go | 49 +- internal/controller/state/graph/grpcroute.go | 52 +- .../controller/state/graph/grpcroute_test.go | 490 ++++++++++-- internal/controller/state/graph/httproute.go | 109 ++- .../controller/state/graph/httproute_test.go | 270 ++++++- .../controller/state/graph/route_common.go | 260 ++++++- .../state/graph/route_common_test.go | 704 ++++++++++++++++++ .../fake_httpfields_validator.go | 77 ++ .../controller/state/validation/validator.go | 2 + 22 files changed, 2466 insertions(+), 185 deletions(-) diff --git a/internal/controller/nginx/config/http/config.go b/internal/controller/nginx/config/http/config.go index cd4d565965..b5d7565982 100644 --- a/internal/controller/nginx/config/http/config.go +++ b/internal/controller/nginx/config/http/config.go @@ -120,6 +120,7 @@ const ( // Upstream holds all configuration for an HTTP upstream. type Upstream struct { + SessionPersistence UpstreamSessionPersistence Name string ZoneSize string // format: 512k, 1m StateFile string @@ -129,6 +130,14 @@ type Upstream struct { Servers []UpstreamServer } +// UpstreamSessionPersistence holds the session persistence configuration for an upstream. +type UpstreamSessionPersistence struct { + Name string + Expiry string + Path string + SessionType string +} + // UpstreamKeepAlive holds the keepalive configuration for an HTTP upstream. type UpstreamKeepAlive struct { Time string diff --git a/internal/controller/nginx/config/upstreams.go b/internal/controller/nginx/config/upstreams.go index 7708ba1d97..0ab0a8234c 100644 --- a/internal/controller/nginx/config/upstreams.go +++ b/internal/controller/nginx/config/upstreams.go @@ -147,6 +147,7 @@ func (g GeneratorImpl) createUpstream( processor upstreamsettings.Processor, ) http.Upstream { var stateFile string + var sp http.UpstreamSessionPersistence upstreamPolicySettings := processor.Process(up.Policies) zoneSize := ossZoneSize @@ -157,6 +158,8 @@ func (g GeneratorImpl) createUpstream( if !upstreamHasResolveServers(up) { stateFile = fmt.Sprintf("%s/%s.conf", stateDir, up.Name) } + + sp = getSessionPersistenceConfiguration(up.SessionPersistence) } if upstreamPolicySettings.ZoneSize != "" { @@ -212,6 +215,7 @@ func (g GeneratorImpl) createUpstream( Servers: upstreamServers, KeepAlive: upstreamPolicySettings.KeepAlive, LoadBalancingMethod: chosenLBMethod, + SessionPersistence: sp, } } @@ -236,3 +240,17 @@ func upstreamHasResolveServers(upstream dataplane.Upstream) bool { } return false } + +// getSessionPersistenceConfiguration gets the session persistence configuration for an upstream. +// Supported only for NGINX Plus and cookie-based type. +func getSessionPersistenceConfiguration(sp dataplane.SessionPersistenceConfig) http.UpstreamSessionPersistence { + if sp.Name == "" { + return http.UpstreamSessionPersistence{} + } + return http.UpstreamSessionPersistence{ + Name: sp.Name, + Expiry: sp.Expiry, + Path: sp.Path, + SessionType: string(sp.SessionType), + } +} diff --git a/internal/controller/nginx/config/upstreams_template.go b/internal/controller/nginx/config/upstreams_template.go index b3af0cad2e..9f42702422 100644 --- a/internal/controller/nginx/config/upstreams_template.go +++ b/internal/controller/nginx/config/upstreams_template.go @@ -17,6 +17,12 @@ upstream {{ $u.Name }} { zone {{ $u.Name }} {{ $u.ZoneSize }}; {{ end -}} + {{ if $u.SessionPersistence.Name -}} + sticky {{ $u.SessionPersistence.SessionType }} {{ $u.SessionPersistence.Name }} + {{- if $u.SessionPersistence.Expiry }} expires={{ $u.SessionPersistence.Expiry }}{{- end }} + {{- if $u.SessionPersistence.Path }} path={{ $u.SessionPersistence.Path }}{{- end }}; + {{ end -}} + {{- if $u.StateFile }} state {{ $u.StateFile }}; {{- else }} diff --git a/internal/controller/nginx/config/upstreams_test.go b/internal/controller/nginx/config/upstreams_test.go index f387fa4ed9..2e61d858a7 100644 --- a/internal/controller/nginx/config/upstreams_test.go +++ b/internal/controller/nginx/config/upstreams_test.go @@ -19,9 +19,11 @@ import ( "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/helpers" ) -func TestExecuteUpstreams(t *testing.T) { +func TestExecuteUpstreams_NginxOSS(t *testing.T) { t.Parallel() - gen := GeneratorImpl{} + gen := GeneratorImpl{ + plus: false, + } stateUpstreams := []dataplane.Upstream{ { Name: "up1", @@ -102,9 +104,14 @@ func TestExecuteUpstreams(t *testing.T) { "keepalive_requests 1;": 1, "keepalive_time 5s;": 1, "keepalive_timeout 10s;": 1, - "zone up5-usp 2m;": 1, "ip_hash;": 1, + "zone up1 512k;": 1, + "zone up2 512k;": 1, + "zone up3 512k;": 1, + "zone up4-ipv6 512k;": 1, + "zone up5-usp 2m;": 1, + "random two least_conn;": 4, } @@ -125,6 +132,200 @@ func TestExecuteUpstreams(t *testing.T) { } } +func TestExecuteUpstreams_NginxPlus(t *testing.T) { + t.Parallel() + gen := GeneratorImpl{ + plus: true, + } + stateUpstreams := []dataplane.Upstream{ + { + Name: "up1", + Endpoints: []resolver.Endpoint{ + { + Address: "10.0.0.0", + Port: 80, + Resolve: true, + }, + }, + }, + { + Name: "up2", + Endpoints: []resolver.Endpoint{ + { + Address: "11.0.0.0", + Port: 80, + Resolve: true, + }, + { + Address: "11.0.0.1", + Port: 80, + Resolve: true, + }, + { + Address: "11.0.0.2", + Port: 80, + }, + }, + }, + { + Name: "up3-ipv6", + Endpoints: []resolver.Endpoint{ + { + Address: "2001:db8::1", + Port: 80, + IPv6: true, + Resolve: true, + }, + }, + }, + { + Name: "up4-ipv6", + Endpoints: []resolver.Endpoint{ + { + Address: "2001:db8::2", + Port: 80, + IPv6: true, + Resolve: true, + }, + { + Address: "2001:db8::3", + Port: 80, + IPv6: true, + }, + }, + }, + { + Name: "up5", + Endpoints: []resolver.Endpoint{}, + }, + { + Name: "up6-usp-with-sp", + Endpoints: []resolver.Endpoint{ + { + Address: "12.0.0.1", + Port: 80, + Resolve: true, + }, + }, + Policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + ZoneSize: helpers.GetPointer[ngfAPI.Size]("2m"), + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Connections: helpers.GetPointer(int32(1)), + Requests: helpers.GetPointer(int32(1)), + Time: helpers.GetPointer[ngfAPI.Duration]("5s"), + Timeout: helpers.GetPointer[ngfAPI.Duration]("10s"), + }), + LoadBalancingMethod: helpers.GetPointer(ngfAPI.LoadBalancingTypeIPHash), + }, + }, + }, + SessionPersistence: dataplane.SessionPersistenceConfig{ + Name: "session-persistence", + Expiry: "30m", + Path: "/session", + SessionType: dataplane.SessionPersistenceCookie, + }, + }, + { + Name: "up7-with-sp", + Endpoints: []resolver.Endpoint{ + { + Address: "12.0.0.2", + Port: 80, + Resolve: true, + }, + }, + SessionPersistence: dataplane.SessionPersistenceConfig{ + Name: "session-persistence", + Expiry: "100h", + Path: "/v1/users", + SessionType: dataplane.SessionPersistenceCookie, + }, + }, + { + Name: "up8-with-sp-expiry-and-path-empty", + Endpoints: []resolver.Endpoint{ + { + Address: "12.0.0.3", + Port: 80, + Resolve: true, + }, + }, + SessionPersistence: dataplane.SessionPersistenceConfig{ + Name: "session-persistence", + SessionType: dataplane.SessionPersistenceCookie, + }, + }, + } + + expectedSubStrings := map[string]int{ + "upstream up1": 1, + "upstream up2": 1, + "upstream up3-ipv6": 1, + "upstream up4-ipv6": 1, + "upstream up5": 1, + "upstream up6-usp-with-sp": 1, + "upstream up7-with-sp": 1, + "upstream up8-with-sp-expiry-and-path-empty": 1, + "upstream invalid-backend-ref": 1, + + "random two least_conn;": 6, + "ip_hash;": 1, + + "zone up1 1m;": 1, + "zone up2 1m;": 1, + "zone up3-ipv6 1m;": 1, + "zone up4-ipv6 1m;": 1, + "zone up5 1m;": 1, + "zone up6-usp-with-sp 2m;": 1, + "zone up7-with-sp 1m;": 1, + "zone up8-with-sp-expiry-and-path-empty 1m;": 1, + + "sticky cookie session-persistence expires=30m path=/session;": 1, + "sticky cookie session-persistence expires=100h path=/v1/users;": 1, + "sticky cookie session-persistence;": 1, + + "keepalive 1;": 1, + "keepalive_requests 1;": 1, + "keepalive_time 5s;": 1, + "keepalive_timeout 10s;": 1, + + "server 10.0.0.0:80 resolve;": 1, + "server 11.0.0.0:80 resolve;": 1, + "server 11.0.0.1:80 resolve;": 1, + "server 11.0.0.2:80;": 1, + "server [2001:db8::1]:80 resolve;": 1, + "server [2001:db8::2]:80 resolve;": 1, + "server [2001:db8::3]:80;": 1, + "server 12.0.0.1:80 resolve;": 1, + "server 12.0.0.2:80 resolve;": 1, + "server 12.0.0.3:80 resolve;": 1, + "server unix:/var/run/nginx/nginx-500-server.sock;": 1, + } + + upstreams := gen.createUpstreams(stateUpstreams, upstreamsettings.NewProcessor()) + + upstreamResults := executeUpstreams(upstreams) + g := NewWithT(t) + g.Expect(upstreamResults).To(HaveLen(1)) + g.Expect(upstreamResults[0].dest).To(Equal(httpConfigFile)) + + nginxUpstreams := string(upstreamResults[0].data) + for expSubString, expectedCount := range expectedSubStrings { + actualCount := strings.Count(nginxUpstreams, expSubString) + g.Expect(actualCount).To( + Equal(expectedCount), + fmt.Sprintf("substring %q expected %d occurrence(s), got %d", expSubString, expectedCount, actualCount), + ) + } +} + func TestCreateUpstreams(t *testing.T) { t.Parallel() gen := GeneratorImpl{} @@ -711,6 +912,41 @@ func TestCreateUpstreamPlus(t *testing.T) { LoadBalancingMethod: defaultLBMethod, }, }, + { + msg: "session persistence config with endpoints", + stateUpstream: dataplane.Upstream{ + Name: "sp-with-endpoints", + Endpoints: []resolver.Endpoint{ + { + Address: "10.0.0.2", + Port: 80, + }, + }, + SessionPersistence: dataplane.SessionPersistenceConfig{ + Name: "session-persistence", + Expiry: "45m", + SessionType: dataplane.SessionPersistenceCookie, + Path: "/app", + }, + }, + expectedUpstream: http.Upstream{ + Name: "sp-with-endpoints", + ZoneSize: plusZoneSize, + StateFile: stateDir + "/sp-with-endpoints.conf", + Servers: []http.UpstreamServer{ + { + Address: "10.0.0.2:80", + }, + }, + LoadBalancingMethod: defaultLBMethod, + SessionPersistence: http.UpstreamSessionPersistence{ + Name: "session-persistence", + Expiry: "45m", + SessionType: string(dataplane.SessionPersistenceCookie), + Path: "/app", + }, + }, + }, } for _, test := range tests { diff --git a/internal/controller/nginx/config/validation/common.go b/internal/controller/nginx/config/validation/common.go index 7bff3e3e65..9e6366a3b4 100644 --- a/internal/controller/nginx/config/validation/common.go +++ b/internal/controller/nginx/config/validation/common.go @@ -5,6 +5,7 @@ import ( "fmt" "regexp" "strings" + "time" k8svalidation "k8s.io/apimachinery/pkg/util/validation" ) @@ -162,3 +163,63 @@ func validatePathInRegexMatch(path string) error { return nil } + +type HTTPDurationValidator struct{} + +func (d HTTPDurationValidator) ValidateDuration(duration string) (string, error) { + return d.validateDurationCanBeConvertedToNginxFormat(duration) +} + +// validateDurationCanBeConvertedToNginxFormat parses a Gateway API duration and returns a single-unit, +// NGINX-friendly duration that matches `^[0-9]{1,4}(ms|s|m|h)?$` +// The conversion rules are: +// - duration must be > 0 +// - ceil to the next millisecond +// - choose the smallest unit (ms→s→m→h) whose ceil value fits in 1–4 digits +// - always include a unit suffix +func (d HTTPDurationValidator) validateDurationCanBeConvertedToNginxFormat(in string) (string, error) { + td, err := time.ParseDuration(in) + if err != nil { + return "", fmt.Errorf("invalid duration: %w", err) + } + if td <= 0 { + return "", errors.New("duration must be > 0") + } + + ns := td.Nanoseconds() + ceilDivision := func(a, b int64) int64 { + return (a + b - 1) / b + } + + totalMS := ceilDivision(ns, int64(time.Millisecond)) + + type unit struct { + suffix string + step int64 + } + + units := []unit{ + {"ms", 1}, + {"s", 1000}, + {"m", 60 * 1000}, + {"h", 60 * 60 * 1000}, + } + + const maxValue = 9999 + var out string + for _, u := range units { + v := ceilDivision(totalMS, u.step) + if v >= 1 && v <= maxValue { + out = fmt.Sprintf("%d%s", v, u.suffix) + break + } + } + if out == "" { + return "", fmt.Errorf("duration is too large for NGINX format (exceeds %dh)", maxValue) + } + + if !durationStringFmtRegexp.MatchString(out) { + return "", fmt.Errorf("computed duration %q does not match NGINX format", out) + } + return out, nil +} diff --git a/internal/controller/nginx/config/validation/common_test.go b/internal/controller/nginx/config/validation/common_test.go index a61c550336..677f4edcf9 100644 --- a/internal/controller/nginx/config/validation/common_test.go +++ b/internal/controller/nginx/config/validation/common_test.go @@ -153,3 +153,31 @@ func TestValidatePathInRegexMatch(t *testing.T) { `/foo/(?P[0-9]+)`, ) } + +func TestValidateDurationCanBeConvertedToNginxFormat(t *testing.T) { + t.Parallel() + validator := HTTPDurationValidator{} + + testValidValuesForResultValidator[string, string]( + t, + validator.validateDurationCanBeConvertedToNginxFormat, + resultTestCase[string, string]{input: "1ms", expected: "1ms"}, + resultTestCase[string, string]{input: "1.1ms", expected: "2ms"}, + resultTestCase[string, string]{input: "100s", expected: "100s"}, + resultTestCase[string, string]{input: "1m", expected: "60s"}, + resultTestCase[string, string]{input: "1h", expected: "3600s"}, + resultTestCase[string, string]{input: "9999s", expected: "9999s"}, + resultTestCase[string, string]{input: "10000s", expected: "167m"}, + ) + + testInvalidValuesForResultValidator[string, string]( + t, + validator.validateDurationCanBeConvertedToNginxFormat, + "", + "foo", + "0s", + "-1s", + "1000000h", // too large + "9999h1s", // just over max + ) +} diff --git a/internal/controller/nginx/config/validation/framework_test.go b/internal/controller/nginx/config/validation/framework_test.go index 7e0ff5d0ce..a9ceaaf117 100644 --- a/internal/controller/nginx/config/validation/framework_test.go +++ b/internal/controller/nginx/config/validation/framework_test.go @@ -149,3 +149,45 @@ func TestGetSortedKeysAsString(t *testing.T) { result := getSortedKeysAsString(values) g.Expect(result).To(Equal(expected)) } + +type resultValidatorFunc[T configValue, R any] func(v T) (R, error) + +type resultTestCase[T configValue, R any] struct { + input T + expected R +} + +func testInvalidValuesForResultValidator[T configValue, R any]( + t *testing.T, + f resultValidatorFunc[T, R], + values ...T, +) { + t.Helper() + runValidatorTests( + t, + func(g *WithT, v T) { + _, err := f(v) + g.Expect(err).To(HaveOccurred(), createFailureMessage(v)) + }, + "invalid_value", + values..., + ) +} + +func testValidValuesForResultValidator[T configValue, R any]( + t *testing.T, + f resultValidatorFunc[T, R], + cases ...resultTestCase[T, R], +) { + t.Helper() + for i, tc := range cases { + name := fmt.Sprintf("test-case=%d", i) + + t.Run(name, func(t *testing.T) { + g := NewWithT(t) + got, err := f(tc.input) + g.Expect(err).ToNot(HaveOccurred(), createFailureMessage(tc.input)) + g.Expect(got).To(Equal(tc.expected), createFailureMessage(tc.input)) + }) + } +} diff --git a/internal/controller/nginx/config/validation/http_validator.go b/internal/controller/nginx/config/validation/http_validator.go index 4837113458..779682bf4e 100644 --- a/internal/controller/nginx/config/validation/http_validator.go +++ b/internal/controller/nginx/config/validation/http_validator.go @@ -13,6 +13,7 @@ type HTTPValidator struct { HTTPURLRewriteValidator HTTPHeaderValidator HTTPPathValidator + HTTPDurationValidator } func (HTTPValidator) SkipValidation() bool { return false } diff --git a/internal/controller/state/conditions/conditions.go b/internal/controller/state/conditions/conditions.go index 8e186fe1a4..e677b6be18 100644 --- a/internal/controller/state/conditions/conditions.go +++ b/internal/controller/state/conditions/conditions.go @@ -42,6 +42,10 @@ const ( // not yet supported. RouteReasonUnsupportedField v1.RouteConditionReason = "UnsupportedField" + // RouteReasonInvalidSessionPersistence is used with the "Accepted" (true) condition when the Route + // contains invalid session persistence configuration. + RouteReasonInvalidSessionPersistence v1.RouteConditionReason = "InvalidSessionPersistence" + // RouteReasonInvalidGateway is used with the "Accepted" (false) condition when the Gateway the Route // references is invalid. RouteReasonInvalidGateway v1.RouteConditionReason = "InvalidGateway" @@ -393,6 +397,17 @@ func NewRouteAcceptedUnsupportedField(msg string) Condition { } } +// NewRouteAcceptedInvalidSessionPersistenceConfiguration returns a Condition that indicates that the +// Route is accepted but invalid session persistence configuration was ignored. +func NewRouteAcceptedInvalidSessionPersistenceConfiguration(msg string) Condition { + return Condition{ + Type: string(v1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: string(RouteReasonInvalidSessionPersistence), + Message: fmt.Sprintf("Session Persistence configuration ignored: %s", msg), + } +} + // NewRoutePartiallyInvalid returns a Condition that indicates that the Route contains a combination // of both valid and invalid rules. // diff --git a/internal/controller/state/dataplane/configuration.go b/internal/controller/state/dataplane/configuration.go index 26bc3c58ee..3e20be893c 100644 --- a/internal/controller/state/dataplane/configuration.go +++ b/internal/controller/state/dataplane/configuration.go @@ -774,6 +774,12 @@ func buildUpstreams( } for _, br := range rule.BackendRefs { + var spForThisBackend *graph.SessionPersistenceConfig + // only set session persistence for regular backends + if rule.SessionPersistence != nil && !br.IsMirrorBackend && !br.IsInferencePool { + spForThisBackend = rule.SessionPersistence + } + if upstream := buildUpstream( ctx, logger, @@ -783,6 +789,7 @@ func buildUpstreams( referencedServices, uniqueUpstreams, allowedAddressType, + spForThisBackend, ); upstream != nil { uniqueUpstreams[upstream.Name] = *upstream } @@ -818,6 +825,7 @@ func buildUpstream( referencedServices map[types.NamespacedName]*graph.ReferencedService, uniqueUpstreams map[string]Upstream, allowedAddressType []discoveryV1.AddressType, + sessionPersistence *graph.SessionPersistenceConfig, ) *Upstream { if !br.Valid { return nil @@ -857,11 +865,22 @@ func buildUpstream( upstreamPolicies = buildPolicies(gateway, graphSvc.Policies) } + var sp SessionPersistenceConfig + if sessionPersistence != nil { + sp = SessionPersistenceConfig{ + Name: sessionPersistence.Name, + Expiry: sessionPersistence.Expiry, + Path: sessionPersistence.Path, + SessionType: SessionPersistenceCookie, + } + } + return &Upstream{ - Name: upstreamName, - Endpoints: eps, - ErrorMsg: errMsg, - Policies: upstreamPolicies, + Name: upstreamName, + Endpoints: eps, + ErrorMsg: errMsg, + Policies: upstreamPolicies, + SessionPersistence: sp, } } diff --git a/internal/controller/state/dataplane/configuration_test.go b/internal/controller/state/dataplane/configuration_test.go index 330a0cef79..7d5a5e2d5b 100644 --- a/internal/controller/state/dataplane/configuration_test.go +++ b/internal/controller/state/dataplane/configuration_test.go @@ -2873,14 +2873,18 @@ func TestGetListenerHostname(t *testing.T) { } } -func refsToValidRules(refs ...[]graph.BackendRef) []graph.RouteRule { +func refsToValidRules( + sessionPersistence *graph.SessionPersistenceConfig, + refs ...[]graph.BackendRef, +) []graph.RouteRule { rules := make([]graph.RouteRule, 0, len(refs)) for _, ref := range refs { rules = append(rules, graph.RouteRule{ - ValidMatches: true, - Filters: graph.RouteRuleFilters{Valid: true}, - BackendRefs: ref, + ValidMatches: true, + Filters: graph.RouteRuleFilters{Valid: true}, + BackendRefs: ref, + SessionPersistence: sessionPersistence, }) } @@ -3020,23 +3024,56 @@ func TestBuildUpstreams(t *testing.T) { refsWithPolicies := createBackendRefs("policies") + mirrorRefs := createBackendRefs("mirror-service") + mirrorRefs[0].IsMirrorBackend = true + + inferenceRefs := createBackendRefs("inference-service") + inferenceRefs[0].IsInferencePool = true + + mixedRefs := []graph.BackendRef{ + { + SvcNsName: types.NamespacedName{Namespace: "test", Name: "regular-service"}, + ServicePort: apiv1.ServicePort{Port: 80}, + Valid: true, + }, + { + SvcNsName: types.NamespacedName{Namespace: "test", Name: "mirror-service"}, + ServicePort: apiv1.ServicePort{Port: 80}, + Valid: true, + IsMirrorBackend: true, + }, + { + SvcNsName: types.NamespacedName{Namespace: "test", Name: "inference-service"}, + ServicePort: apiv1.ServicePort{Port: 80}, + Valid: true, + IsInferencePool: true, + }, + } + + spConfig := &graph.SessionPersistenceConfig{ + Name: "multiple-backends-sp", + SessionType: v1.CookieBasedSessionPersistence, + Expiry: "24h", + Path: "/", + } + routes := map[graph.RouteKey]*graph.L7Route{ {NamespacedName: types.NamespacedName{Name: "hr1", Namespace: "test"}}: { Valid: true, Spec: graph.L7RouteSpec{ - Rules: refsToValidRules(hr1Refs0, hr1Refs1, hr1Refs2), + Rules: refsToValidRules(spConfig, hr1Refs0, hr1Refs1, hr1Refs2), }, }, {NamespacedName: types.NamespacedName{Name: "hr2", Namespace: "test"}}: { Valid: true, Spec: graph.L7RouteSpec{ - Rules: refsToValidRules(hr2Refs0, hr2Refs1), + Rules: refsToValidRules(nil, hr2Refs0, hr2Refs1), }, }, {NamespacedName: types.NamespacedName{Name: "hr3", Namespace: "test"}}: { Valid: true, Spec: graph.L7RouteSpec{ - Rules: refsToValidRules(hr3Refs0), + Rules: refsToValidRules(spConfig, hr3Refs0), }, }, } @@ -3045,7 +3082,7 @@ func TestBuildUpstreams(t *testing.T) { {NamespacedName: types.NamespacedName{Name: "hr4", Namespace: "test"}}: { Valid: true, Spec: graph.L7RouteSpec{ - Rules: refsToValidRules(hr4Refs0, hr4Refs1), + Rules: refsToValidRules(nil, hr4Refs0, hr4Refs1), }, }, } @@ -3054,7 +3091,7 @@ func TestBuildUpstreams(t *testing.T) { {NamespacedName: types.NamespacedName{Name: "hr4", Namespace: "test"}}: { Valid: true, Spec: graph.L7RouteSpec{ - Rules: refsToValidRules(hr5Refs0, hr2Refs1), + Rules: refsToValidRules(nil, hr5Refs0, hr2Refs1), }, }, } @@ -3063,7 +3100,7 @@ func TestBuildUpstreams(t *testing.T) { {NamespacedName: types.NamespacedName{Name: "non-existing", Namespace: "test"}}: { Valid: true, Spec: graph.L7RouteSpec{ - Rules: refsToValidRules(nonExistingRefs), + Rules: refsToValidRules(spConfig, nonExistingRefs), }, }, } @@ -3072,7 +3109,7 @@ func TestBuildUpstreams(t *testing.T) { {NamespacedName: types.NamespacedName{Name: "invalid", Namespace: "test"}}: { Valid: false, Spec: graph.L7RouteSpec{ - Rules: refsToValidRules(invalidHRRefs), + Rules: refsToValidRules(nil, invalidHRRefs), }, }, } @@ -3081,7 +3118,28 @@ func TestBuildUpstreams(t *testing.T) { {NamespacedName: types.NamespacedName{Name: "policies", Namespace: "test"}}: { Valid: true, Spec: graph.L7RouteSpec{ - Rules: refsToValidRules(refsWithPolicies), + Rules: refsToValidRules(spConfig, refsWithPolicies), + }, + }, + } + + routesWithMirrorAndInference := map[graph.RouteKey]*graph.L7Route{ + {NamespacedName: types.NamespacedName{Name: "hr-mirror", Namespace: "test"}}: { + Valid: true, + Spec: graph.L7RouteSpec{ + Rules: refsToValidRules(spConfig, mirrorRefs), + }, + }, + {NamespacedName: types.NamespacedName{Name: "hr-inference", Namespace: "test"}}: { + Valid: true, + Spec: graph.L7RouteSpec{ + Rules: refsToValidRules(spConfig, inferenceRefs), + }, + }, + {NamespacedName: types.NamespacedName{Name: "hr-mixed", Namespace: "test"}}: { + Valid: true, + Spec: graph.L7RouteSpec{ + Rules: refsToValidRules(spConfig, mixedRefs), }, }, } @@ -3124,6 +3182,11 @@ func TestBuildUpstreams(t *testing.T) { Valid: true, Routes: routesWithPolicies, }, + { + Name: "listener-6", + Valid: true, + Routes: routesWithMirrorAndInference, + }, }, } @@ -3156,32 +3219,47 @@ func TestBuildUpstreams(t *testing.T) { }, }, }, + {Name: "mirror-service", Namespace: "test"}: {}, + {Name: "inference-service", Namespace: "test"}: {}, + {Name: "regular-service", Namespace: "test"}: {}, } emptyEndpointsErrMsg := "empty endpoints error" nilEndpointsErrMsg := "nil endpoints error" + expectedSPConfig := SessionPersistenceConfig{ + Name: "multiple-backends-sp", + SessionType: SessionPersistenceCookie, + Expiry: "24h", + Path: "/", + } + expUpstreams := []Upstream{ { - Name: "test_bar_80", - Endpoints: barEndpoints, + Name: "test_bar_80", + Endpoints: barEndpoints, + SessionPersistence: expectedSPConfig, }, { - Name: "test_baz2_80", - Endpoints: baz2Endpoints, + Name: "test_baz2_80", + Endpoints: baz2Endpoints, + SessionPersistence: SessionPersistenceConfig{}, }, { - Name: "test_baz_80", - Endpoints: bazEndpoints, + Name: "test_baz_80", + Endpoints: bazEndpoints, + SessionPersistence: expectedSPConfig, }, { - Name: "test_empty-endpoints_80", - Endpoints: []resolver.Endpoint{}, - ErrorMsg: emptyEndpointsErrMsg, + Name: "test_empty-endpoints_80", + Endpoints: []resolver.Endpoint{}, + ErrorMsg: emptyEndpointsErrMsg, + SessionPersistence: SessionPersistenceConfig{}, }, { - Name: "test_foo_80", - Endpoints: fooEndpoints, + Name: "test_foo_80", + Endpoints: fooEndpoints, + SessionPersistence: expectedSPConfig, }, { Name: "test_nil-endpoints_80", @@ -3189,13 +3267,30 @@ func TestBuildUpstreams(t *testing.T) { ErrorMsg: nilEndpointsErrMsg, }, { - Name: "test_ipv6-endpoints_80", - Endpoints: ipv6Endpoints, + Name: "test_ipv6-endpoints_80", + Endpoints: ipv6Endpoints, + SessionPersistence: SessionPersistenceConfig{}, + }, + { + Name: "test_policies_80", + Endpoints: policyEndpoints, + Policies: []policies.Policy{validPolicy1, validPolicy2}, + SessionPersistence: expectedSPConfig, + }, + { + Name: "test_mirror-service_80", + Endpoints: []resolver.Endpoint{{Address: "17.0.0.0", Port: 80}}, + SessionPersistence: SessionPersistenceConfig{}, + }, + { + Name: "test_inference-service_80", + Endpoints: []resolver.Endpoint{{Address: "18.0.0.0", Port: 80}}, + SessionPersistence: SessionPersistenceConfig{}, }, { - Name: "test_policies_80", - Endpoints: policyEndpoints, - Policies: []policies.Policy{validPolicy1, validPolicy2}, + Name: "test_regular-service_80", + Endpoints: []resolver.Endpoint{{Address: "19.0.0.0", Port: 80}}, + SessionPersistence: expectedSPConfig, }, } @@ -3228,6 +3323,12 @@ func TestBuildUpstreams(t *testing.T) { return ipv6Endpoints, nil case "policies": return policyEndpoints, nil + case "mirror-service": + return []resolver.Endpoint{{Address: "17.0.0.0", Port: 80}}, nil + case "inference-service": + return []resolver.Endpoint{{Address: "18.0.0.0", Port: 80}}, nil + case "regular-service": + return []resolver.Endpoint{{Address: "19.0.0.0", Port: 80}}, nil default: return nil, fmt.Errorf("unexpected service %s", svcNsName.Name) } diff --git a/internal/controller/state/dataplane/types.go b/internal/controller/state/dataplane/types.go index 1b0ccbde6a..2068197f2a 100644 --- a/internal/controller/state/dataplane/types.go +++ b/internal/controller/state/dataplane/types.go @@ -112,6 +112,8 @@ type Layer4VirtualServer struct { // Upstream is a pool of endpoints to be load balanced. type Upstream struct { + // SessionPersistence holds the session persistence configuration for the upstream. + SessionPersistence SessionPersistenceConfig // Name is the name of the Upstream. Will be unique for each service/port combination. Name string // ErrorMsg contains the error message if the Upstream is invalid. @@ -122,6 +124,25 @@ type Upstream struct { Policies []policies.Policy } +// SessionPersistenceConfig holds the session persistence configuration for an upstream. +type SessionPersistenceConfig struct { + // SessionType is the type of session persistence. + SessionType SessionPersistenceType + // Name is the name of the session. + Name string + // Expiry is the expiration time of the session. + Expiry string + // Path is the path of the for which session is applied. + Path string +} + +type SessionPersistenceType string + +const ( + // SessionPersistenceCookie indicates cookie-based session persistence. + SessionPersistenceCookie SessionPersistenceType = "cookie" +) + // SSL is the SSL configuration for a server. type SSL struct { // KeyPairID is the ID of the corresponding SSLKeyPair for the server. diff --git a/internal/controller/state/graph/graph.go b/internal/controller/state/graph/graph.go index ffd86ac6e0..b46fc32126 100644 --- a/internal/controller/state/graph/graph.go +++ b/internal/controller/state/graph/graph.go @@ -92,6 +92,14 @@ type NginxReloadResult struct { // ProtectedPorts are the ports that may not be configured by a listener with a descriptive name of each port. type ProtectedPorts map[int32]string +// flags hold the configuration flags for building the Graph. +type flags struct { + // plus indicates whether NGINX Plus features are enabled. + plus bool + // experimental indicates whether experimental features are enabled. + experimental bool +} + // IsReferenced returns true if the Graph references the resource. func (g *Graph) IsReferenced(resourceType ngftypes.ObjectType, nsname types.NamespacedName) bool { switch obj := resourceType.(type) { @@ -254,6 +262,8 @@ func BuildGraph( processedSnippetsFilters := processSnippetsFilters(state.SnippetsFilters) + // secrets map only gets populated if plus is enabled + plusEnabled := len(plusSecrets) > 0 routes := buildRoutesForGateways( validators.HTTPFieldsValidator, state.HTTPRoutes, @@ -261,6 +271,7 @@ func BuildGraph( gws, processedSnippetsFilters, state.InferencePools, + flags{plus: plusEnabled, experimental: experimentalEnabled}, ) referencedInferencePools := buildReferencedInferencePools(routes, gws, state.InferencePools, state.Services) diff --git a/internal/controller/state/graph/graph_test.go b/internal/controller/state/graph/graph_test.go index ed0e39082f..e9b6f5633b 100644 --- a/internal/controller/state/graph/graph_test.go +++ b/internal/controller/state/graph/graph_test.go @@ -196,6 +196,7 @@ func TestBuildGraph(t *testing.T) { createValidRuleWithBackendRefsAndFilters := func( matches []gatewayv1.HTTPRouteMatch, routeType RouteType, + sessionPersistence *SessionPersistenceConfig, ) RouteRule { rule := createValidRuleWithBackendRefs(matches) rule.Filters = RouteRuleFilters{ @@ -213,6 +214,10 @@ func TestBuildGraph(t *testing.T) { Valid: true, } + if sessionPersistence != nil { + rule.SessionPersistence = sessionPersistence + } + return rule } @@ -333,14 +338,23 @@ func TestBuildGraph(t *testing.T) { } } + spConfig := &gatewayv1.SessionPersistence{ + SessionName: helpers.GetPointer("session-persistence-httproute"), + Type: helpers.GetPointer(gatewayv1.CookieBasedSessionPersistence), + AbsoluteTimeout: helpers.GetPointer(gatewayv1.Duration("30m")), + CookieConfig: &gatewayv1.CookieConfig{ + LifetimeType: helpers.GetPointer(gatewayv1.PermanentCookieLifetimeType), + }, + } hr1 := createRoute("hr-1", "gateway-1", "listener-80-1") - addFilterToPath( + addElementsToPath( hr1, "/", gatewayv1.HTTPRouteFilter{ Type: gatewayv1.HTTPRouteFilterExtensionRef, ExtensionRef: refSnippetsFilterExtensionRef, }, + spConfig, ) hr2 := createRoute("hr-2", "wrong-gateway", "listener-80-1") @@ -381,6 +395,14 @@ func TestBuildGraph(t *testing.T) { ExtensionRef: refSnippetsFilterExtensionRef, }, }, + SessionPersistence: &gatewayv1.SessionPersistence{ + SessionName: helpers.GetPointer("session-persistence-grpcroute"), + Type: helpers.GetPointer(gatewayv1.CookieBasedSessionPersistence), + AbsoluteTimeout: helpers.GetPointer(gatewayv1.Duration("30m")), + CookieConfig: &gatewayv1.CookieConfig{ + LifetimeType: helpers.GetPointer(gatewayv1.PermanentCookieLifetimeType), + }, + }, }, }, }, @@ -858,6 +880,13 @@ func TestBuildGraph(t *testing.T) { } } + expectedSP := &SessionPersistenceConfig{ + Name: "session-persistence-httproute", + SessionType: gatewayv1.CookieBasedSessionPersistence, + Expiry: "30m", + Valid: true, + Path: "/", + } routeHR1 := &L7Route{ RouteType: RouteTypeHTTP, Valid: true, @@ -885,7 +914,7 @@ func TestBuildGraph(t *testing.T) { }, Spec: L7RouteSpec{ Hostnames: hr1.Spec.Hostnames, - Rules: []RouteRule{createValidRuleWithBackendRefsAndFilters(routeMatches, RouteTypeHTTP)}, + Rules: []RouteRule{createValidRuleWithBackendRefsAndFilters(routeMatches, RouteTypeHTTP, expectedSP)}, }, Policies: []*Policy{processedRoutePolicy}, Conditions: []conditions.Condition{ @@ -1049,6 +1078,12 @@ func TestBuildGraph(t *testing.T) { }, } + expectedSPgr := &SessionPersistenceConfig{ + Name: "session-persistence-grpcroute", + SessionType: gatewayv1.CookieBasedSessionPersistence, + Expiry: "30m", + Valid: true, + } routeGR := &L7Route{ RouteType: RouteTypeGRPC, Valid: true, @@ -1077,7 +1112,7 @@ func TestBuildGraph(t *testing.T) { Spec: L7RouteSpec{ Hostnames: gr.Spec.Hostnames, Rules: []RouteRule{ - createValidRuleWithBackendRefsAndFilters(routeMatches, RouteTypeGRPC), + createValidRuleWithBackendRefsAndFilters(routeMatches, RouteTypeGRPC, expectedSPgr), }, }, } @@ -1475,6 +1510,12 @@ func TestBuildGraph(t *testing.T) { fakePolicyValidator := &validationfakes.FakePolicyValidator{} + createAllValidValidator := func() *validationfakes.FakeHTTPFieldsValidator { + v := &validationfakes.FakeHTTPFieldsValidator{} + v.ValidateDurationReturns("30m", nil) + return v + } + result := BuildGraph( test.store, controllerName, @@ -1488,7 +1529,7 @@ func TestBuildGraph(t *testing.T) { }, }, validation.Validators{ - HTTPFieldsValidator: &validationfakes.FakeHTTPFieldsValidator{}, + HTTPFieldsValidator: createAllValidValidator(), GenericValidator: &validationfakes.FakeGenericValidator{}, PolicyValidator: fakePolicyValidator, }, diff --git a/internal/controller/state/graph/grpcroute.go b/internal/controller/state/graph/grpcroute.go index 5d6e7489e3..022193efa4 100644 --- a/internal/controller/state/graph/grpcroute.go +++ b/internal/controller/state/graph/grpcroute.go @@ -21,6 +21,7 @@ func buildGRPCRoute( ghr *v1.GRPCRoute, gws map[types.NamespacedName]*Gateway, snippetsFilters map[types.NamespacedName]*SnippetsFilter, + featureFlags flags, ) *L7Route { r := &L7Route{ Source: ghr, @@ -56,6 +57,7 @@ func buildGRPCRoute( ghr.Spec.Rules, validator, getSnippetsFilterResolverForNamespace(snippetsFilters, r.Source.GetNamespace()), + featureFlags, ) r.Spec.Rules = rules @@ -71,6 +73,7 @@ func buildGRPCMirrorRoutes( route *v1.GRPCRoute, gateways map[types.NamespacedName]*Gateway, snippetsFilters map[types.NamespacedName]*SnippetsFilter, + featureFlags flags, ) { for idx, rule := range l7route.Spec.Rules { if rule.Filters.Valid { @@ -107,6 +110,7 @@ func buildGRPCMirrorRoutes( tmpMirrorRoute, gateways, snippetsFilters, + featureFlags, ) if mirrorRoute != nil { @@ -160,12 +164,13 @@ func processGRPCRouteRule( rulePath *field.Path, validator validation.HTTPFieldsValidator, resolveExtRefFunc resolveExtRefFilter, + featureFlags flags, ) (RouteRule, routeRuleErrors) { var errors routeRuleErrors validMatches := true - unsupportedFieldsErrors := checkForUnsupportedGRPCFields(specRule, rulePath) + unsupportedFieldsErrors := checkForUnsupportedGRPCFields(specRule, rulePath, featureFlags) if len(unsupportedFieldsErrors) > 0 { errors.warn = append(errors.warn, unsupportedFieldsErrors...) } @@ -223,11 +228,27 @@ func processGRPCRouteRule( } } + var sp *SessionPersistenceConfig + if specRule.SessionPersistence != nil { + spConfig, spErrors := processSessionPersistenceConfig( + specRule.SessionPersistence, + specRule.Matches, + rulePath.Child("sessionPersistence"), + validator, + ) + errors = errors.append(spErrors) + + if spConfig != nil && spConfig.Valid { + sp = spConfig + } + } + return RouteRule{ - ValidMatches: validMatches, - Matches: ConvertGRPCMatches(specRule.Matches), - Filters: routeFilters, - RouteBackendRefs: backendRefs, + ValidMatches: validMatches, + Matches: ConvertGRPCMatches(specRule.Matches), + Filters: routeFilters, + RouteBackendRefs: backendRefs, + SessionPersistence: sp, }, errors } @@ -235,6 +256,7 @@ func processGRPCRouteRules( specRules []v1.GRPCRouteRule, validator validation.HTTPFieldsValidator, resolveExtRefFunc resolveExtRefFilter, + featureFlags flags, ) (rules []RouteRule, valid bool, conds []conditions.Condition) { rules = make([]RouteRule, len(specRules)) @@ -251,6 +273,7 @@ func processGRPCRouteRules( rulePath, validator, resolveExtRefFunc, + featureFlags, ) if rr.ValidMatches && rr.Filters.Valid { @@ -270,6 +293,11 @@ func processGRPCRouteRules( conds = append(conds, conditions.NewRouteAcceptedUnsupportedField(allRulesErrors.warn.ToAggregate().Error())) } + spConflictErrors := handleSessionPersistenceConflicts(rules) + if spConflictErrors != nil { + conds = append(conds, conditions.NewRouteAcceptedInvalidSessionPersistenceConfiguration(spConflictErrors.Error())) + } + if len(allRulesErrors.invalid) > 0 { msg := allRulesErrors.invalid.ToAggregate().Error() @@ -455,7 +483,7 @@ func validateGRPCHeaderMatch( return allErrs } -func checkForUnsupportedGRPCFields(rule v1.GRPCRouteRule, rulePath *field.Path) field.ErrorList { +func checkForUnsupportedGRPCFields(rule v1.GRPCRouteRule, rulePath *field.Path, featureFlags flags) field.ErrorList { var ruleErrors field.ErrorList if rule.Name != nil { @@ -464,10 +492,18 @@ func checkForUnsupportedGRPCFields(rule v1.GRPCRouteRule, rulePath *field.Path) "Name", )) } - if rule.SessionPersistence != nil { + + if !featureFlags.plus && rule.SessionPersistence != nil { + ruleErrors = append(ruleErrors, field.Forbidden( + rulePath.Child("sessionPersistence"), + "SessionPersistence is only supported in NGINX Plus. This configuration will be ignored.", + )) + } + + if !featureFlags.experimental && rule.SessionPersistence != nil { ruleErrors = append(ruleErrors, field.Forbidden( rulePath.Child("sessionPersistence"), - "SessionPersistence", + "SessionPersistence is only supported in experimental mode.", )) } diff --git a/internal/controller/state/graph/grpcroute_test.go b/internal/controller/state/graph/grpcroute_test.go index 032b6a891f..04f677e7ef 100644 --- a/internal/controller/state/graph/grpcroute_test.go +++ b/internal/controller/state/graph/grpcroute_test.go @@ -20,38 +20,63 @@ import ( "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/kinds" ) -func createGRPCMethodMatch(serviceName, methodName, methodType string) v1.GRPCRouteRule { +func createGRPCMethodMatch( + serviceName, + methodName, + methodType string, + sp *v1.SessionPersistence, + backendRef []v1.GRPCBackendRef, +) v1.GRPCRouteRule { var mt *v1.GRPCMethodMatchType if methodType != "nilType" { mt = (*v1.GRPCMethodMatchType)(&methodType) } - return v1.GRPCRouteRule{ - Matches: []v1.GRPCRouteMatch{ - { - Method: &v1.GRPCMethodMatch{ - Type: mt, - Service: &serviceName, - Method: &methodName, - }, + matches := []v1.GRPCRouteMatch{ + { + Method: &v1.GRPCMethodMatch{ + Type: mt, + Service: &serviceName, + Method: &methodName, }, }, } -} -func createGRPCHeadersMatch(headerType, headerName, headerValue string) v1.GRPCRouteRule { + if sp != nil { + return v1.GRPCRouteRule{ + Matches: matches, + SessionPersistence: sp, + BackendRefs: backendRef, + } + } + return v1.GRPCRouteRule{ - Matches: []v1.GRPCRouteMatch{ - { - Headers: []v1.GRPCHeaderMatch{ - { - Type: (*v1.GRPCHeaderMatchType)(&headerType), - Name: v1.GRPCHeaderName(headerName), - Value: headerValue, - }, + Matches: matches, + } +} + +func createGRPCHeadersMatch(headerType, headerName, headerValue string, sp *v1.SessionPersistence) v1.GRPCRouteRule { + matches := []v1.GRPCRouteMatch{ + { + Headers: []v1.GRPCHeaderMatch{ + { + Type: (*v1.GRPCHeaderMatchType)(&headerType), + Name: v1.GRPCHeaderName(headerName), + Value: headerValue, }, }, }, } + + if sp != nil { + return v1.GRPCRouteRule{ + Matches: matches, + SessionPersistence: sp, + } + } + + return v1.GRPCRouteRule{ + Matches: matches, + } } func createGRPCRoute( @@ -114,11 +139,21 @@ func TestBuildGRPCRoutes(t *testing.T) { RequestHeaderModifier: &v1.HTTPHeaderFilter{}, } - grRuleWithFilters := v1.GRPCRouteRule{ - Filters: []v1.GRPCRouteFilter{snippetsFilterRef, requestHeaderFilter}, + sessionPersistenceConfig := v1.SessionPersistence{ + SessionName: helpers.GetPointer("grpc-route-session"), + AbsoluteTimeout: helpers.GetPointer(v1.Duration("10m")), + Type: helpers.GetPointer(v1.CookieBasedSessionPersistence), + CookieConfig: &v1.CookieConfig{ + LifetimeType: helpers.GetPointer((v1.PermanentCookieLifetimeType)), + }, + } + + grRuleWithFiltersAndSessionPersistence := v1.GRPCRouteRule{ + Filters: []v1.GRPCRouteFilter{snippetsFilterRef, requestHeaderFilter}, + SessionPersistence: &sessionPersistenceConfig, } - gr := createGRPCRoute("gr-1", gwNsName.Name, "example.com", []v1.GRPCRouteRule{grRuleWithFilters}) + gr := createGRPCRoute("gr-1", gwNsName.Name, "example.com", []v1.GRPCRouteRule{grRuleWithFiltersAndSessionPersistence}) grWrongGateway := createGRPCRoute("gr-2", "some-gateway", "example.com", []v1.GRPCRouteRule{}) @@ -193,6 +228,12 @@ func TestBuildGRPCRoutes(t *testing.T) { }, }, }, + SessionPersistence: &SessionPersistenceConfig{ + Valid: true, + Name: *sessionPersistenceConfig.SessionName, + SessionType: *sessionPersistenceConfig.Type, + Expiry: "10m", + }, ValidMatches: true, RouteBackendRefs: []RouteBackendRef{}, }, @@ -209,7 +250,11 @@ func TestBuildGRPCRoutes(t *testing.T) { }, } - validator := &validationfakes.FakeHTTPFieldsValidator{} + createAllValidValidator := func() *validationfakes.FakeHTTPFieldsValidator { + v := &validationfakes.FakeHTTPFieldsValidator{} + v.ValidateDurationReturns("10m", nil) + return v + } for _, test := range tests { t.Run(test.name, func(t *testing.T) { @@ -225,14 +270,17 @@ func TestBuildGRPCRoutes(t *testing.T) { }, }, } - routes := buildRoutesForGateways( - validator, + createAllValidValidator(), map[types.NamespacedName]*v1.HTTPRoute{}, grRoutes, test.gateways, snippetsFilters, nil, + flags{ + plus: true, + experimental: true, + }, ) g.Expect(helpers.Diff(test.expected, routes)).To(BeEmpty()) }) @@ -256,13 +304,31 @@ func TestBuildGRPCRoute(t *testing.T) { } gatewayNsName := client.ObjectKeyFromObject(gw.Source) - methodMatchRule := createGRPCMethodMatch("myService", "myMethod", "Exact") - headersMatchRule := createGRPCHeadersMatch("Exact", "MyHeader", "SomeValue") + spMethod := &v1.SessionPersistence{ + SessionName: helpers.GetPointer("grpc-method-session"), + AbsoluteTimeout: helpers.GetPointer(v1.Duration("10h")), + Type: helpers.GetPointer(v1.CookieBasedSessionPersistence), + CookieConfig: &v1.CookieConfig{ + LifetimeType: helpers.GetPointer((v1.PermanentCookieLifetimeType)), + }, + } + + spHeader := &v1.SessionPersistence{ + SessionName: helpers.GetPointer("grpc-header-session"), + AbsoluteTimeout: helpers.GetPointer(v1.Duration("10h")), + Type: helpers.GetPointer(v1.CookieBasedSessionPersistence), + CookieConfig: &v1.CookieConfig{ + LifetimeType: helpers.GetPointer((v1.SessionCookieLifetimeType)), + }, + } + + methodMatchRule := createGRPCMethodMatch("myService", "myMethod", "Exact", spMethod, nil) + headersMatchRule := createGRPCHeadersMatch("Exact", "MyHeader", "SomeValue", spHeader) - methodMatchEmptyFields := createGRPCMethodMatch("", "", "") - methodMatchInvalidFields := createGRPCMethodMatch("service{}", "method{}", "Exact") - methodMatchNilType := createGRPCMethodMatch("myService", "myMethod", "nilType") - headersMatchInvalid := createGRPCHeadersMatch("", "MyHeader", "SomeValue") + methodMatchEmptyFields := createGRPCMethodMatch("", "", "", nil, nil) + methodMatchInvalidFields := createGRPCMethodMatch("service{}", "method{}", "Exact", nil, nil) + methodMatchNilType := createGRPCMethodMatch("myService", "myMethod", "nilType", nil, nil) + headersMatchInvalid := createGRPCHeadersMatch("", "MyHeader", "SomeValue", nil) headersMatchEmptyType := v1.GRPCRouteRule{ Matches: []v1.GRPCRouteMatch{ @@ -396,7 +462,7 @@ func TestBuildGRPCRoute(t *testing.T) { grDuplicateSectionName.Spec.ParentRefs[0], ) - grInvalidFilterRule := createGRPCMethodMatch("myService", "myMethod", "Exact") + grInvalidFilterRule := createGRPCMethodMatch("myService", "myMethod", "Exact", nil, nil) grInvalidFilterRule.Filters = []v1.GRPCRouteFilter{ { @@ -411,8 +477,8 @@ func TestBuildGRPCRoute(t *testing.T) { []v1.GRPCRouteRule{grInvalidFilterRule}, ) - grValidFilterRule := createGRPCMethodMatch("myService", "myMethod", "Exact") - grValidHeaderMatch := createGRPCHeadersMatch("RegularExpression", "MyHeader", "headers-[a-z]+") + grValidFilterRule := createGRPCMethodMatch("myService", "myMethod", "Exact", nil, nil) + grValidHeaderMatch := createGRPCHeadersMatch("RegularExpression", "MyHeader", "headers-[a-z]+", nil) validSnippetsFilterRef := &v1.LocalObjectReference{ Group: ngfAPIv1alpha1.GroupName, Kind: kinds.SnippetsFilter, @@ -451,7 +517,7 @@ func TestBuildGRPCRoute(t *testing.T) { ) // route with invalid snippets filter extension ref - grInvalidSnippetsFilterRule := createGRPCMethodMatch("myService", "myMethod", "Exact") + grInvalidSnippetsFilterRule := createGRPCMethodMatch("myService", "myMethod", "Exact", nil, nil) grInvalidSnippetsFilterRule.Filters = []v1.GRPCRouteFilter{ { Type: v1.GRPCRouteFilterExtensionRef, @@ -470,7 +536,7 @@ func TestBuildGRPCRoute(t *testing.T) { ) // route with unresolvable snippets filter extension ref - grUnresolvableSnippetsFilterRule := createGRPCMethodMatch("myService", "myMethod", "Exact") + grUnresolvableSnippetsFilterRule := createGRPCMethodMatch("myService", "myMethod", "Exact", nil, nil) grUnresolvableSnippetsFilterRule.Filters = []v1.GRPCRouteFilter{ { Type: v1.GRPCRouteFilterExtensionRef, @@ -489,7 +555,7 @@ func TestBuildGRPCRoute(t *testing.T) { ) // route with two invalid snippets filter extensions refs: (1) invalid group (2) unresolvable - grInvalidAndUnresolvableSnippetsFilterRule := createGRPCMethodMatch("myService", "myMethod", "Exact") + grInvalidAndUnresolvableSnippetsFilterRule := createGRPCMethodMatch("myService", "myMethod", "Exact", nil, nil) grInvalidAndUnresolvableSnippetsFilterRule.Filters = []v1.GRPCRouteFilter{ { Type: v1.GRPCRouteFilterExtensionRef, @@ -515,9 +581,15 @@ func TestBuildGRPCRoute(t *testing.T) { []v1.GRPCRouteRule{grInvalidAndUnresolvableSnippetsFilterRule}, ) - createAllValidValidator := func() *validationfakes.FakeHTTPFieldsValidator { + createAllValidValidator := func(duration *v1.Duration) *validationfakes.FakeHTTPFieldsValidator { v := &validationfakes.FakeHTTPFieldsValidator{} v.ValidateMethodInMatchReturns(true, nil) + + if duration == nil { + v.ValidateDurationReturns("", nil) + } else { + v.ValidateDurationReturns(string(*duration), nil) + } return v } @@ -552,14 +624,75 @@ func TestBuildGRPCRoute(t *testing.T) { }, } + backendRefOne := []v1.GRPCBackendRef{ + { + BackendRef: v1.BackendRef{ + BackendObjectReference: v1.BackendObjectReference{ + Kind: helpers.GetPointer[v1.Kind]("Service"), + Name: "service-one", + Namespace: helpers.GetPointer[v1.Namespace]("test"), + Port: helpers.GetPointer[v1.PortNumber](80), + }, + }, + }, + } + + backendRefTwo := []v1.GRPCBackendRef{ + { + BackendRef: v1.BackendRef{ + BackendObjectReference: v1.BackendObjectReference{ + Kind: helpers.GetPointer[v1.Kind]("Service"), + Name: "service-two", + Namespace: helpers.GetPointer[v1.Namespace]("test"), + Port: helpers.GetPointer[v1.PortNumber](80), + }, + }, + }, + } + + spDifferentSameBackend := &v1.SessionPersistence{ + SessionName: helpers.GetPointer("grpc-method-session-diff"), + AbsoluteTimeout: helpers.GetPointer(v1.Duration("5h")), + Type: helpers.GetPointer(v1.CookieBasedSessionPersistence), + CookieConfig: &v1.CookieConfig{ + LifetimeType: helpers.GetPointer((v1.PermanentCookieLifetimeType)), + }, + } + + methodMatchWithSP := createGRPCMethodMatch("myService", "myMethod", "Exact", spMethod, backendRefOne) + methodMatchWithSameSPSameBackend := createGRPCMethodMatch("myService", "myMethod", "Exact", spMethod, backendRefOne) + + methodMatchWithPSameBackend := createGRPCMethodMatch( + "myService", + "myMethod", + "Exact", + spDifferentSameBackend, + backendRefTwo, + ) + methodMatchWithDiffSPSameBackend := createGRPCMethodMatch("myService", "myMethod", "Exact", spMethod, backendRefTwo) + + grpcRouteSPConflict := createGRPCRoute( + "gr-same-backend", + gatewayNsName.Name, + "example.com", + []v1.GRPCRouteRule{ + methodMatchWithSP, + methodMatchWithSameSPSameBackend, + methodMatchWithPSameBackend, + methodMatchWithDiffSPSameBackend, + }, + ) + + durationSP := v1.Duration("10h") tests := []struct { - validator *validationfakes.FakeHTTPFieldsValidator - gr *v1.GRPCRoute - expected *L7Route - name string + validator *validationfakes.FakeHTTPFieldsValidator + gr *v1.GRPCRoute + expected *L7Route + name string + plus, experimental bool }{ { - validator: createAllValidValidator(), + validator: createAllValidValidator(&durationSP), gr: grBoth, expected: &L7Route{ RouteType: RouteTypeGRPC, @@ -584,6 +717,12 @@ func TestBuildGRPCRoute(t *testing.T) { }, Matches: ConvertGRPCMatches(grBoth.Spec.Rules[0].Matches), RouteBackendRefs: []RouteBackendRef{}, + SessionPersistence: &SessionPersistenceConfig{ + Valid: true, + Name: "grpc-method-session", + SessionType: v1.CookieBasedSessionPersistence, + Expiry: "10h", + }, }, { ValidMatches: true, @@ -593,14 +732,21 @@ func TestBuildGRPCRoute(t *testing.T) { }, Matches: ConvertGRPCMatches(grBoth.Spec.Rules[1].Matches), RouteBackendRefs: []RouteBackendRef{}, + SessionPersistence: &SessionPersistenceConfig{ + Valid: true, + Name: "grpc-header-session", + SessionType: v1.CookieBasedSessionPersistence, + }, }, }, }, }, - name: "normal case with both", + plus: true, + experimental: true, + name: "normal case with both", }, { - validator: createAllValidValidator(), + validator: createAllValidValidator(nil), gr: grEmptyMatch, expected: &L7Route{ RouteType: RouteTypeGRPC, @@ -632,7 +778,7 @@ func TestBuildGRPCRoute(t *testing.T) { name: "valid rule with empty match", }, { - validator: createAllValidValidator(), + validator: createAllValidValidator(nil), gr: grValidFilter, expected: &L7Route{ RouteType: RouteTypeGRPC, @@ -673,7 +819,7 @@ func TestBuildGRPCRoute(t *testing.T) { name: "valid path rule, headers with filters", }, { - validator: createAllValidValidator(), + validator: createAllValidValidator(nil), gr: grInvalidMatchesEmptyMethodFields, expected: &L7Route{ RouteType: RouteTypeGRPC, @@ -714,7 +860,7 @@ func TestBuildGRPCRoute(t *testing.T) { }, { validator: func() *validationfakes.FakeHTTPFieldsValidator { - validator := createAllValidValidator() + validator := createAllValidValidator(nil) validator.ValidatePathInMatchReturns(errors.New("invalid path value")) return validator }(), @@ -756,7 +902,7 @@ func TestBuildGRPCRoute(t *testing.T) { name: "invalid matches with invalid method fields", }, { - validator: createAllValidValidator(), + validator: createAllValidValidator(nil), gr: grDuplicateSectionName, expected: &L7Route{ RouteType: RouteTypeGRPC, @@ -765,7 +911,7 @@ func TestBuildGRPCRoute(t *testing.T) { name: "invalid route with duplicate sectionName", }, { - validator: createAllValidValidator(), + validator: createAllValidValidator(&durationSP), gr: grOneInvalid, expected: &L7Route{ Source: grOneInvalid, @@ -795,6 +941,12 @@ func TestBuildGRPCRoute(t *testing.T) { }, Matches: ConvertGRPCMatches(grOneInvalid.Spec.Rules[0].Matches), RouteBackendRefs: []RouteBackendRef{}, + SessionPersistence: &SessionPersistenceConfig{ + Valid: true, + Name: "grpc-method-session", + SessionType: v1.CookieBasedSessionPersistence, + Expiry: "10h", + }, }, { ValidMatches: false, @@ -808,10 +960,12 @@ func TestBuildGRPCRoute(t *testing.T) { }, }, }, - name: "invalid headers and valid method", + plus: true, + experimental: true, + name: "invalid headers and valid method", }, { - validator: createAllValidValidator(), + validator: createAllValidValidator(nil), gr: grInvalidHeadersInvalidType, expected: &L7Route{ Source: grInvalidHeadersInvalidType, @@ -849,7 +1003,7 @@ func TestBuildGRPCRoute(t *testing.T) { name: "invalid headers with invalid type", }, { - validator: createAllValidValidator(), + validator: createAllValidValidator(nil), gr: grInvalidHeadersEmptyType, expected: &L7Route{ Source: grInvalidHeadersEmptyType, @@ -887,7 +1041,7 @@ func TestBuildGRPCRoute(t *testing.T) { name: "invalid headers with no header type specified", }, { - validator: createAllValidValidator(), + validator: createAllValidValidator(nil), gr: grInvalidMatchesNilMethodType, expected: &L7Route{ Source: grInvalidMatchesNilMethodType, @@ -924,7 +1078,7 @@ func TestBuildGRPCRoute(t *testing.T) { name: "invalid method with nil type", }, { - validator: createAllValidValidator(), + validator: createAllValidValidator(nil), gr: grInvalidFilter, expected: &L7Route{ Source: grInvalidFilter, @@ -963,13 +1117,13 @@ func TestBuildGRPCRoute(t *testing.T) { name: "invalid filter", }, { - validator: createAllValidValidator(), + validator: createAllValidValidator(nil), gr: grNotNGF, expected: nil, name: "not NGF route", }, { - validator: createAllValidValidator(), + validator: createAllValidValidator(nil), gr: grInvalidHostname, expected: &L7Route{ Source: grInvalidHostname, @@ -992,7 +1146,7 @@ func TestBuildGRPCRoute(t *testing.T) { name: "invalid hostname", }, { - validator: createAllValidValidator(), + validator: createAllValidValidator(nil), gr: grInvalidSnippetsFilter, expected: &L7Route{ Source: grInvalidSnippetsFilter, @@ -1030,7 +1184,7 @@ func TestBuildGRPCRoute(t *testing.T) { name: "invalid snippet filter extension ref", }, { - validator: createAllValidValidator(), + validator: createAllValidValidator(nil), gr: grUnresolvableSnippetsFilter, expected: &L7Route{ Source: grUnresolvableSnippetsFilter, @@ -1069,7 +1223,7 @@ func TestBuildGRPCRoute(t *testing.T) { name: "unresolvable snippet filter extension ref", }, { - validator: createAllValidValidator(), + validator: createAllValidValidator(nil), gr: grInvalidAndUnresolvableSnippetsFilter, expected: &L7Route{ Source: grInvalidAndUnresolvableSnippetsFilter, @@ -1112,7 +1266,7 @@ func TestBuildGRPCRoute(t *testing.T) { name: "one invalid and one unresolvable snippet filter extension ref", }, { - validator: createAllValidValidator(), + validator: createAllValidValidator(nil), gr: grValidWithUnsupportedField, expected: &L7Route{ RouteType: RouteTypeGRPC, @@ -1147,7 +1301,7 @@ func TestBuildGRPCRoute(t *testing.T) { name: "valid route with unsupported field", }, { - validator: createAllValidValidator(), + validator: createAllValidValidator(nil), gr: grInvalidWithUnsupportedField, expected: &L7Route{ RouteType: RouteTypeGRPC, @@ -1185,6 +1339,126 @@ func TestBuildGRPCRoute(t *testing.T) { }, name: "invalid route with unsupported field", }, + { + validator: createAllValidValidator(nil), + gr: grpcRouteSPConflict, + expected: &L7Route{ + RouteType: RouteTypeGRPC, + Source: grpcRouteSPConflict, + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: CreateParentRefGateway(gw), + SectionName: grpcRouteSPConflict.Spec.ParentRefs[0].SectionName, + }, + }, + Valid: true, + Attachable: true, + Spec: L7RouteSpec{ + Hostnames: grpcRouteSPConflict.Spec.Hostnames, + Rules: []RouteRule{ + { + ValidMatches: true, + Filters: RouteRuleFilters{ + Valid: true, + Filters: []Filter{}, + }, + Matches: ConvertGRPCMatches(grpcRouteSPConflict.Spec.Rules[0].Matches), + RouteBackendRefs: []RouteBackendRef{ + { + BackendRef: v1.BackendRef{ + BackendObjectReference: v1.BackendObjectReference{ + Name: "service-one", + Kind: helpers.GetPointer(v1.Kind("Service")), + Namespace: helpers.GetPointer(v1.Namespace("test")), + Port: helpers.GetPointer(v1.PortNumber(80)), + }, + }, + }, + }, + SessionPersistence: &SessionPersistenceConfig{ + Valid: true, + Name: "grpc-method-session", + SessionType: v1.CookieBasedSessionPersistence, + }, + }, + { + ValidMatches: true, + Filters: RouteRuleFilters{ + Valid: true, + Filters: []Filter{}, + }, + Matches: ConvertGRPCMatches(grpcRouteSPConflict.Spec.Rules[1].Matches), + RouteBackendRefs: []RouteBackendRef{ + { + BackendRef: v1.BackendRef{ + BackendObjectReference: v1.BackendObjectReference{ + Name: "service-one", + Kind: helpers.GetPointer(v1.Kind("Service")), + Namespace: helpers.GetPointer(v1.Namespace("test")), + Port: helpers.GetPointer(v1.PortNumber(80)), + }, + }, + }, + }, + SessionPersistence: &SessionPersistenceConfig{ + Valid: true, + Name: "grpc-method-session", + SessionType: v1.CookieBasedSessionPersistence, + }, + }, + { + ValidMatches: true, + Filters: RouteRuleFilters{ + Valid: true, + Filters: []Filter{}, + }, + Matches: ConvertGRPCMatches(grpcRouteSPConflict.Spec.Rules[2].Matches), + RouteBackendRefs: []RouteBackendRef{ + { + BackendRef: v1.BackendRef{ + BackendObjectReference: v1.BackendObjectReference{ + Name: "service-two", + Kind: helpers.GetPointer(v1.Kind("Service")), + Namespace: helpers.GetPointer(v1.Namespace("test")), + Port: helpers.GetPointer(v1.PortNumber(80)), + }, + }, + }, + }, + }, + { + ValidMatches: true, + Filters: RouteRuleFilters{ + Valid: true, + Filters: []Filter{}, + }, + Matches: ConvertGRPCMatches(grpcRouteSPConflict.Spec.Rules[3].Matches), + RouteBackendRefs: []RouteBackendRef{ + { + BackendRef: v1.BackendRef{ + BackendObjectReference: v1.BackendObjectReference{ + Name: "service-two", + Kind: helpers.GetPointer(v1.Kind("Service")), + Namespace: helpers.GetPointer(v1.Namespace("test")), + Port: helpers.GetPointer(v1.PortNumber(80)), + }, + }, + }, + }, + }, + }, + }, + Conditions: []conditions.Condition{ + conditions.NewRouteAcceptedInvalidSessionPersistenceConfiguration( + "for backendRefs test/service-two:80 due to conflicting configuration across multiple rules", + ), + }, + }, + name: "route has multiple rules with same backend but different session persistence", + plus: true, + experimental: true, + }, } gws := map[types.NamespacedName]*Gateway{ @@ -1199,7 +1473,16 @@ func TestBuildGRPCRoute(t *testing.T) { snippetsFilters := map[types.NamespacedName]*SnippetsFilter{ {Namespace: "test", Name: "sf"}: {Valid: true}, } - route := buildGRPCRoute(test.validator, test.gr, gws, snippetsFilters) + route := buildGRPCRoute( + test.validator, + test.gr, + gws, + snippetsFilters, + flags{ + plus: test.plus, + experimental: test.experimental, + }, + ) g.Expect(helpers.Diff(test.expected, route)).To(BeEmpty()) }) } @@ -1351,11 +1634,21 @@ func TestBuildGRPCRouteWithMirrorRoutes(t *testing.T) { g := NewWithT(t) + featureFlags := flags{ + plus: false, + experimental: false, + } + routes := map[RouteKey]*L7Route{} - l7route := buildGRPCRoute(validator, gr, gateways, snippetsFilters) + l7route := buildGRPCRoute( + validator, + gr, + gateways, + snippetsFilters, + featureFlags, + ) g.Expect(l7route).NotTo(BeNil()) - - buildGRPCMirrorRoutes(routes, l7route, gr, gateways, snippetsFilters) + buildGRPCMirrorRoutes(routes, l7route, gr, gateways, snippetsFilters, featureFlags) obj, ok := expectedMirrorRoute.Source.(*v1.GRPCRoute) g.Expect(ok).To(BeTrue()) @@ -1366,11 +1659,11 @@ func TestBuildGRPCRouteWithMirrorRoutes(t *testing.T) { func TestConvertGRPCMatches(t *testing.T) { t.Parallel() - methodMatch := createGRPCMethodMatch("myService", "myMethod", "Exact").Matches + methodMatch := createGRPCMethodMatch("myService", "myMethod", "Exact", nil, nil).Matches - headersMatch := createGRPCHeadersMatch("Exact", "MyHeader", "SomeValue").Matches + headersMatch := createGRPCHeadersMatch("Exact", "MyHeader", "SomeValue", nil).Matches - headerMatchRegularExp := createGRPCHeadersMatch("RegularExpression", "HeaderRegex", "headers-[a-z]+").Matches + headerMatchRegularExp := createGRPCHeadersMatch("RegularExpression", "HeaderRegex", "headers-[a-z]+", nil).Matches expectedHTTPMatches := []v1.HTTPRouteMatch{ { @@ -1523,7 +1816,7 @@ func TestProcessGRPCRouteRule_UnsupportedFields(t *testing.T) { Type: helpers.GetPointer(v1.SessionPersistenceType("unsupported-session-persistence")), }), }, - expectedErrors: 2, + expectedErrors: 3, }, } @@ -1536,7 +1829,14 @@ func TestProcessGRPCRouteRule_UnsupportedFields(t *testing.T) { var errors routeRuleErrors // Wrap the rule in GRPCRouteRuleWrapper - unsupportedFieldsErrors := checkForUnsupportedGRPCFields(test.specRule, rulePath) + unsupportedFieldsErrors := checkForUnsupportedGRPCFields( + test.specRule, + rulePath, + flags{ + plus: false, + experimental: false, + }, + ) if len(unsupportedFieldsErrors) > 0 { errors.warn = append(errors.warn, unsupportedFieldsErrors...) } @@ -1555,6 +1855,8 @@ func TestProcessGRPCRouteRules_UnsupportedFields(t *testing.T) { expectedConds []conditions.Condition expectedWarns int expectedValid bool + plusEnabled bool + experimental bool }{ { name: "No unsupported fields", @@ -1582,17 +1884,55 @@ func TestProcessGRPCRouteRules_UnsupportedFields(t *testing.T) { { Name: helpers.GetPointer[v1.SectionName]("unsupported-name"), SessionPersistence: helpers.GetPointer(v1.SessionPersistence{ - Type: helpers.GetPointer(v1.SessionPersistenceType("unsupported-session-persistence")), + Type: helpers.GetPointer(v1.CookieBasedSessionPersistence), + SessionName: helpers.GetPointer("session_id"), }), }, }, expectedValid: true, expectedConds: []conditions.Condition{ conditions.NewRouteAcceptedUnsupportedField("[spec.rules[0].name: Forbidden: Name, " + - "spec.rules[0].sessionPersistence: Forbidden: SessionPersistence]"), + "spec.rules[0].sessionPersistence: Forbidden: " + + "SessionPersistence is only supported in NGINX Plus. This configuration will be ignored.]"), }, + experimental: true, + plusEnabled: false, expectedWarns: 2, }, + { + name: "Session persistence unsupported with experimental disabled", + specRules: []v1.GRPCRouteRule{ + { + SessionPersistence: helpers.GetPointer(v1.SessionPersistence{ + Type: helpers.GetPointer(v1.CookieBasedSessionPersistence), + SessionName: helpers.GetPointer("session_id"), + }), + }, + }, + expectedValid: true, + expectedConds: []conditions.Condition{ + conditions.NewRouteAcceptedUnsupportedField("spec.rules[0].sessionPersistence: Forbidden: " + + "SessionPersistence is only supported in experimental mode."), + }, + expectedWarns: 1, + plusEnabled: true, + experimental: false, + }, + { + name: "Session Persistence supported with Plus enabled and experimental enabled", + specRules: []v1.GRPCRouteRule{ + { + SessionPersistence: helpers.GetPointer(v1.SessionPersistence{ + Type: helpers.GetPointer(v1.CookieBasedSessionPersistence), + SessionName: helpers.GetPointer("session_id"), + }), + }, + }, + expectedValid: true, + plusEnabled: true, + experimental: true, + expectedWarns: 0, + }, } for _, test := range tests { @@ -1604,6 +1944,10 @@ func TestProcessGRPCRouteRules_UnsupportedFields(t *testing.T) { test.specRules, validation.SkipValidator{}, nil, + flags{ + plus: test.plusEnabled, + experimental: test.experimental, + }, ) g.Expect(valid).To(Equal(test.expectedValid)) diff --git a/internal/controller/state/graph/httproute.go b/internal/controller/state/graph/httproute.go index a8ee8ab158..a2b78bc31b 100644 --- a/internal/controller/state/graph/httproute.go +++ b/internal/controller/state/graph/httproute.go @@ -31,6 +31,7 @@ func buildHTTPRoute( gws map[types.NamespacedName]*Gateway, snippetsFilters map[types.NamespacedName]*SnippetsFilter, inferencePools map[types.NamespacedName]*inference.InferencePool, + featureFlags flags, ) *L7Route { r := &L7Route{ Source: ghr, @@ -69,6 +70,7 @@ func buildHTTPRoute( getSnippetsFilterResolverForNamespace(snippetsFilters, r.Source.GetNamespace()), inferencePools, r.Source.GetNamespace(), + featureFlags, ) r.Spec.Rules = rules @@ -84,6 +86,7 @@ func buildHTTPMirrorRoutes( route *v1.HTTPRoute, gateways map[types.NamespacedName]*Gateway, snippetsFilters map[types.NamespacedName]*SnippetsFilter, + featureFlags flags, ) { for idx, rule := range l7route.Spec.Rules { if rule.Filters.Valid { @@ -121,6 +124,7 @@ func buildHTTPMirrorRoutes( gateways, snippetsFilters, nil, + featureFlags, ) if mirrorRoute != nil { @@ -176,10 +180,11 @@ func processHTTPRouteRule( resolveExtRefFunc resolveExtRefFilter, inferencePools map[types.NamespacedName]*inference.InferencePool, routeNamespace string, + featureFlags flags, ) (RouteRule, routeRuleErrors) { var errors routeRuleErrors - unsupportedFieldsErrors := checkForUnsupportedHTTPFields(specRule, rulePath) + unsupportedFieldsErrors := checkForUnsupportedHTTPFields(specRule, rulePath, featureFlags) if len(unsupportedFieldsErrors) > 0 { errors.warn = append(errors.warn, unsupportedFieldsErrors...) } @@ -202,13 +207,63 @@ func processHTTPRouteRule( validator, resolveExtRefFunc, ) - errors = errors.append(filterErrors) - backendRefs := make([]RouteBackendRef, 0, len(specRule.BackendRefs)) + backendRefs, backendRefErrors := getBackendRefs(specRule.BackendRefs, routeNamespace, inferencePools, rulePath) + errors = errors.append(backendRefErrors) + + if routeFilters.Valid { + for i, filter := range routeFilters.Filters { + if filter.RequestMirror == nil { + continue + } + + rbr := RouteBackendRef{ + BackendRef: v1.BackendRef{ + BackendObjectReference: filter.RequestMirror.BackendRef, + }, + MirrorBackendIdx: helpers.GetPointer(i), + } + backendRefs = append(backendRefs, rbr) + } + } + + var sp *SessionPersistenceConfig + if specRule.SessionPersistence != nil { + spConfig, spErrors := processSessionPersistenceConfig( + specRule.SessionPersistence, + specRule.Matches, + rulePath.Child("sessionPersistence"), + validator, + ) + + errors = errors.append(spErrors) + + if spConfig != nil && spConfig.Valid { + sp = spConfig + } + } + + return RouteRule{ + ValidMatches: validMatches, + Matches: specRule.Matches, + Filters: routeFilters, + RouteBackendRefs: backendRefs, + SessionPersistence: sp, + }, errors +} + +func getBackendRefs( + routeBackendRefs []v1.HTTPBackendRef, + routeNamespace string, + inferencePools map[types.NamespacedName]*inference.InferencePool, + rulePath *field.Path, +) ([]RouteBackendRef, routeRuleErrors) { + var errors routeRuleErrors + backendRefs := make([]RouteBackendRef, 0, len(routeBackendRefs)) // rule.BackendRefs are validated separately because of their special requirements - for _, b := range specRule.BackendRefs { + for _, b := range routeBackendRefs { var interfaceFilters []any if len(b.Filters) > 0 { interfaceFilters = make([]any, 0, len(b.Filters)) @@ -228,7 +283,7 @@ func processHTTPRouteRule( // We don't support traffic splitting at the Route level for // InferencePool backends, so if there's more than one backendRef, and one of them // is an InferencePool, we mark the rule as invalid. - if len(specRule.BackendRefs) > 1 { + if len(routeBackendRefs) > 1 { err := field.Forbidden( rulePath.Child("backendRefs"), "cannot use InferencePool backend when multiple backendRefs are specified in a single rule", @@ -256,28 +311,7 @@ func processHTTPRouteRule( backendRefs = append(backendRefs, rbr) } - if routeFilters.Valid { - for i, filter := range routeFilters.Filters { - if filter.RequestMirror == nil { - continue - } - - rbr := RouteBackendRef{ - BackendRef: v1.BackendRef{ - BackendObjectReference: filter.RequestMirror.BackendRef, - }, - MirrorBackendIdx: helpers.GetPointer(i), - } - backendRefs = append(backendRefs, rbr) - } - } - - return RouteRule{ - ValidMatches: validMatches, - Matches: specRule.Matches, - Filters: routeFilters, - RouteBackendRefs: backendRefs, - }, errors + return backendRefs, errors } func processHTTPRouteRules( @@ -286,6 +320,7 @@ func processHTTPRouteRules( resolveExtRefFunc resolveExtRefFilter, inferencePools map[types.NamespacedName]*inference.InferencePool, routeNamespace string, + featureFlags flags, ) (rules []RouteRule, valid bool, conds []conditions.Condition) { rules = make([]RouteRule, len(specRules)) @@ -304,6 +339,7 @@ func processHTTPRouteRules( resolveExtRefFunc, inferencePools, routeNamespace, + featureFlags, ) if rr.ValidMatches && rr.Filters.Valid { @@ -324,6 +360,11 @@ func processHTTPRouteRules( conds = append(conds, conditions.NewRouteAcceptedUnsupportedField(allRulesErrors.warn.ToAggregate().Error())) } + spConflictErrors := handleSessionPersistenceConflicts(rules) + if spConflictErrors != nil { + conds = append(conds, conditions.NewRouteAcceptedInvalidSessionPersistenceConfiguration(spConflictErrors.Error())) + } + if len(allRulesErrors.invalid) > 0 { msg := allRulesErrors.invalid.ToAggregate().Error() @@ -608,7 +649,7 @@ func validateFilterRewrite( return allErrs } -func checkForUnsupportedHTTPFields(rule v1.HTTPRouteRule, rulePath *field.Path) field.ErrorList { +func checkForUnsupportedHTTPFields(rule v1.HTTPRouteRule, rulePath *field.Path, featureFlags flags) field.ErrorList { var ruleErrors field.ErrorList if rule.Name != nil { @@ -629,10 +670,18 @@ func checkForUnsupportedHTTPFields(rule v1.HTTPRouteRule, rulePath *field.Path) "Retry", )) } - if rule.SessionPersistence != nil { + + if !featureFlags.plus && rule.SessionPersistence != nil { + ruleErrors = append(ruleErrors, field.Forbidden( + rulePath.Child("sessionPersistence"), + "SessionPersistence is only supported in NGINX Plus. This configuration will be ignored.", + )) + } + + if !featureFlags.experimental && rule.SessionPersistence != nil { ruleErrors = append(ruleErrors, field.Forbidden( rulePath.Child("sessionPersistence"), - "SessionPersistence", + "SessionPersistence is only supported in experimental mode.", )) } diff --git a/internal/controller/state/graph/httproute_test.go b/internal/controller/state/graph/httproute_test.go index 01bd75ed9d..d672562a10 100644 --- a/internal/controller/state/graph/httproute_test.go +++ b/internal/controller/state/graph/httproute_test.go @@ -59,6 +59,7 @@ func createHTTPRoute( BackendObjectReference: gatewayv1.BackendObjectReference{ Kind: helpers.GetPointer[gatewayv1.Kind](kinds.Service), Name: "backend", + Port: helpers.GetPointer[gatewayv1.PortNumber](80), }, }, Filters: []gatewayv1.HTTPRouteFilter{ @@ -92,7 +93,12 @@ func createHTTPRoute( } } -func addFilterToPath(hr *gatewayv1.HTTPRoute, path string, filter gatewayv1.HTTPRouteFilter) { +func addElementsToPath( + hr *gatewayv1.HTTPRoute, + path string, + filter gatewayv1.HTTPRouteFilter, + sp *gatewayv1.SessionPersistence, +) { for i := range hr.Spec.Rules { for _, match := range hr.Spec.Rules[i].Matches { if match.Path == nil { @@ -100,6 +106,10 @@ func addFilterToPath(hr *gatewayv1.HTTPRoute, path string, filter gatewayv1.HTTP } if *match.Path.Value == path { hr.Spec.Rules[i].Filters = append(hr.Spec.Rules[i].Filters, filter) + + if sp != nil { + hr.Spec.Rules[i].SessionPersistence = sp + } } } } @@ -110,6 +120,7 @@ var expRouteBackendRef = RouteBackendRef{ BackendObjectReference: gatewayv1.BackendObjectReference{ Kind: helpers.GetPointer[gatewayv1.Kind](kinds.Service), Name: "backend", + Port: helpers.GetPointer[gatewayv1.PortNumber](80), }, }, Filters: []any{ @@ -161,8 +172,17 @@ func TestBuildHTTPRoutes(t *testing.T) { RequestRedirect: &gatewayv1.HTTPRequestRedirectFilter{}, } - addFilterToPath(hr, "/", snippetsFilterRef) - addFilterToPath(hr, "/", requestRedirectFilter) + sessionPersistenceConfig := &gatewayv1.SessionPersistence{ + SessionName: helpers.GetPointer("http-route-session"), + AbsoluteTimeout: helpers.GetPointer(gatewayv1.Duration("1h")), + Type: helpers.GetPointer(gatewayv1.CookieBasedSessionPersistence), + CookieConfig: &gatewayv1.CookieConfig{ + LifetimeType: helpers.GetPointer((gatewayv1.PermanentCookieLifetimeType)), + }, + } + + addElementsToPath(hr, "/", snippetsFilterRef, nil) + addElementsToPath(hr, "/", requestRedirectFilter, sessionPersistenceConfig) hrWrongGateway := createHTTPRoute("hr-2", "some-gateway", "example.com", "/") @@ -239,6 +259,13 @@ func TestBuildHTTPRoutes(t *testing.T) { }, Matches: hr.Spec.Rules[0].Matches, RouteBackendRefs: []RouteBackendRef{expRouteBackendRef}, + SessionPersistence: &SessionPersistenceConfig{ + Valid: true, + Name: *sessionPersistenceConfig.SessionName, + SessionType: *sessionPersistenceConfig.Type, + Expiry: "1h", + Path: "/", + }, }, }, }, @@ -253,7 +280,11 @@ func TestBuildHTTPRoutes(t *testing.T) { }, } - validator := &validationfakes.FakeHTTPFieldsValidator{} + createAllValidValidator := func() *validationfakes.FakeHTTPFieldsValidator { + v := &validationfakes.FakeHTTPFieldsValidator{} + v.ValidateDurationReturns("1h", nil) + return v + } for _, test := range tests { t.Run(test.name, func(t *testing.T) { @@ -271,12 +302,16 @@ func TestBuildHTTPRoutes(t *testing.T) { } routes := buildRoutesForGateways( - validator, + createAllValidValidator(), hrRoutes, map[types.NamespacedName]*gatewayv1.GRPCRoute{}, test.gateways, snippetsFilters, nil, + flags{ + plus: true, + experimental: true, + }, ) g.Expect(helpers.Diff(test.expected, routes)).To(BeEmpty()) }) @@ -305,13 +340,21 @@ func TestBuildHTTPRoute(t *testing.T) { hrValidWithUnsupportedField := createHTTPRoute("hr-valid-unsupported", gatewayNsName.Name, "example.com", "/") hrValidWithUnsupportedField.Spec.Rules[0].Name = helpers.GetPointer[gatewayv1.SectionName]("unsupported-name") + sp := &gatewayv1.SessionPersistence{ + SessionName: helpers.GetPointer("http-route-session"), + AbsoluteTimeout: helpers.GetPointer(gatewayv1.Duration("1h")), + Type: helpers.GetPointer(gatewayv1.CookieBasedSessionPersistence), + CookieConfig: &gatewayv1.CookieConfig{ + LifetimeType: helpers.GetPointer((gatewayv1.SessionCookieLifetimeType)), + }, + } // route with valid filter validFilter := gatewayv1.HTTPRouteFilter{ Type: gatewayv1.HTTPRouteFilterRequestRedirect, RequestRedirect: &gatewayv1.HTTPRequestRedirectFilter{}, } hr := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/", "/filter") - addFilterToPath(hr, "/filter", validFilter) + addElementsToPath(hr, "/filter", validFilter, sp) // invalid routes without filters hrInvalidHostname := createHTTPRoute("hr", gatewayNsName.Name, "", "/") @@ -329,7 +372,7 @@ func TestBuildHTTPRoute(t *testing.T) { }, } hrInvalidFilters := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/filter") - addFilterToPath(hrInvalidFilters, "/filter", invalidFilter) + addElementsToPath(hrInvalidFilters, "/filter", invalidFilter, nil) // route with invalid matches and filters hrDroppedInvalidMatchesAndInvalidFilters := createHTTPRoute( @@ -340,12 +383,12 @@ func TestBuildHTTPRoute(t *testing.T) { "/filter", "/", ) - addFilterToPath(hrDroppedInvalidMatchesAndInvalidFilters, "/filter", invalidFilter) + addElementsToPath(hrDroppedInvalidMatchesAndInvalidFilters, "/filter", invalidFilter, nil) // route with both invalid and valid filters in the same rule hrDroppedInvalidFilters := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/filter", "/") - addFilterToPath(hrDroppedInvalidFilters, "/filter", validFilter) - addFilterToPath(hrDroppedInvalidFilters, "/", invalidFilter) + addElementsToPath(hrDroppedInvalidFilters, "/filter", validFilter, nil) + addElementsToPath(hrDroppedInvalidFilters, "/", invalidFilter, nil) // route with duplicate section names hrDuplicateSectionName := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/") @@ -364,7 +407,7 @@ func TestBuildHTTPRoute(t *testing.T) { Name: "sf", }, } - addFilterToPath(hrValidSnippetsFilter, "/filter", validSnippetsFilterExtRef) + addElementsToPath(hrValidSnippetsFilter, "/filter", validSnippetsFilterExtRef, nil) // route with invalid snippets filter extension ref hrInvalidSnippetsFilter := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/filter") @@ -376,7 +419,7 @@ func TestBuildHTTPRoute(t *testing.T) { Name: "sf", }, } - addFilterToPath(hrInvalidSnippetsFilter, "/filter", invalidSnippetsFilterExtRef) + addElementsToPath(hrInvalidSnippetsFilter, "/filter", invalidSnippetsFilterExtRef, nil) // route with unresolvable snippets filter extension ref hrUnresolvableSnippetsFilter := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/filter") @@ -388,12 +431,12 @@ func TestBuildHTTPRoute(t *testing.T) { Name: "does-not-exist", }, } - addFilterToPath(hrUnresolvableSnippetsFilter, "/filter", unresolvableSnippetsFilterExtRef) + addElementsToPath(hrUnresolvableSnippetsFilter, "/filter", unresolvableSnippetsFilterExtRef, nil) // route with two invalid snippets filter extensions refs: (1) invalid group (2) unresolvable hrInvalidAndUnresolvableSnippetsFilter := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/filter") - addFilterToPath(hrInvalidAndUnresolvableSnippetsFilter, "/filter", invalidSnippetsFilterExtRef) - addFilterToPath(hrInvalidAndUnresolvableSnippetsFilter, "/filter", unresolvableSnippetsFilterExtRef) + addElementsToPath(hrInvalidAndUnresolvableSnippetsFilter, "/filter", invalidSnippetsFilterExtRef, nil) + addElementsToPath(hrInvalidAndUnresolvableSnippetsFilter, "/filter", unresolvableSnippetsFilterExtRef, nil) // routes with an inference pool backend hrInferencePool := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/") @@ -425,11 +468,32 @@ func TestBuildHTTPRoute(t *testing.T) { }, } + hrWithMultipleRulesDifferentSP := createHTTPRoute( + "hr-multi-rules-sp", + gatewayNsName.Name, + "example.com", + "/rule1", + "/rule2", + ) + + spDifferent := &gatewayv1.SessionPersistence{ + SessionName: helpers.GetPointer("session-different"), + AbsoluteTimeout: helpers.GetPointer(gatewayv1.Duration("2h")), + Type: helpers.GetPointer(gatewayv1.CookieBasedSessionPersistence), + CookieConfig: &gatewayv1.CookieConfig{ + LifetimeType: helpers.GetPointer((gatewayv1.PermanentCookieLifetimeType)), + }, + } + + addElementsToPath(hrWithMultipleRulesDifferentSP, "/rule1", validFilter, sp) + addElementsToPath(hrWithMultipleRulesDifferentSP, "/rule2", validFilter, spDifferent) + tests := []struct { - validator *validationfakes.FakeHTTPFieldsValidator - hr *gatewayv1.HTTPRoute - expected *L7Route - name string + validator *validationfakes.FakeHTTPFieldsValidator + hr *gatewayv1.HTTPRoute + expected *L7Route + name string + plus, experimental bool }{ { validator: &validationfakes.FakeHTTPFieldsValidator{}, @@ -466,11 +530,19 @@ func TestBuildHTTPRoute(t *testing.T) { }, Matches: hr.Spec.Rules[1].Matches, RouteBackendRefs: []RouteBackendRef{expRouteBackendRef}, + SessionPersistence: &SessionPersistenceConfig{ + Valid: true, + Name: *sp.SessionName, + SessionType: *sp.Type, + Path: "/filter", + }, }, }, }, }, - name: "normal case", + plus: true, + experimental: true, + name: "normal case", }, { validator: &validationfakes.FakeHTTPFieldsValidator{}, @@ -705,7 +777,6 @@ func TestBuildHTTPRoute(t *testing.T) { }, name: "dropped invalid rule with invalid matches", }, - { validator: validatorInvalidFieldsInRule, hr: hrDroppedInvalidMatchesAndInvalidFilters, @@ -1091,6 +1162,66 @@ func TestBuildHTTPRoute(t *testing.T) { }, name: "route with an inference pool backend that doesn't exist", }, + { + validator: &validationfakes.FakeHTTPFieldsValidator{}, + hr: hrWithMultipleRulesDifferentSP, + expected: &L7Route{ + RouteType: RouteTypeHTTP, + Source: hrWithMultipleRulesDifferentSP, + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: CreateParentRefGateway(gw), + SectionName: hrWithMultipleRulesDifferentSP.Spec.ParentRefs[0].SectionName, + }, + }, + Valid: true, + Attachable: true, + Spec: L7RouteSpec{ + Hostnames: hrWithMultipleRulesDifferentSP.Spec.Hostnames, + Rules: []RouteRule{ + { + ValidMatches: true, + Filters: RouteRuleFilters{ + Valid: true, + Filters: []Filter{ + { + RouteType: RouteTypeHTTP, + FilterType: FilterRequestRedirect, + RequestRedirect: &gatewayv1.HTTPRequestRedirectFilter{}, + }, + }, + }, + Matches: hrWithMultipleRulesDifferentSP.Spec.Rules[0].Matches, + RouteBackendRefs: []RouteBackendRef{expRouteBackendRef}, + }, + { + ValidMatches: true, + Filters: RouteRuleFilters{ + Valid: true, + Filters: []Filter{ + { + RouteType: RouteTypeHTTP, + FilterType: FilterRequestRedirect, + RequestRedirect: &gatewayv1.HTTPRequestRedirectFilter{}, + }, + }, + }, + Matches: hrWithMultipleRulesDifferentSP.Spec.Rules[1].Matches, + RouteBackendRefs: []RouteBackendRef{expRouteBackendRef}, + }, + }, + }, + Conditions: []conditions.Condition{ + conditions.NewRouteAcceptedInvalidSessionPersistenceConfiguration( + "for backendRefs default/backend:80 due to conflicting configuration across multiple rules", + ), + }, + }, + plus: true, + experimental: true, + name: "route with multiple rules with different session persistence configs", + }, } gws := map[types.NamespacedName]*Gateway{ @@ -1108,8 +1239,17 @@ func TestBuildHTTPRoute(t *testing.T) { inferencePools := map[types.NamespacedName]*inference.InferencePool{ {Namespace: "test", Name: "ipool"}: {}, } - - route := buildHTTPRoute(test.validator, test.hr, gws, snippetsFilters, inferencePools) + route := buildHTTPRoute( + test.validator, + test.hr, + gws, + snippetsFilters, + inferencePools, + flags{ + plus: test.plus, + experimental: test.experimental, + }, + ) g.Expect(helpers.Diff(test.expected, route)).To(BeEmpty()) }) } @@ -1151,8 +1291,8 @@ func TestBuildHTTPRouteWithMirrorRoutes(t *testing.T) { }, } hr := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/mirror") - addFilterToPath(hr, "/mirror", mirrorFilter) - addFilterToPath(hr, "/mirror", urlRewriteFilter) + addElementsToPath(hr, "/mirror", mirrorFilter, nil) + addElementsToPath(hr, "/mirror", urlRewriteFilter, nil) // Expected mirror route expectedMirrorRoute := &L7Route{ @@ -1240,11 +1380,23 @@ func TestBuildHTTPRouteWithMirrorRoutes(t *testing.T) { g := NewWithT(t) + featureFlags := flags{ + plus: false, + experimental: false, + } + routes := map[RouteKey]*L7Route{} - l7route := buildHTTPRoute(validator, hr, gateways, snippetsFilters, nil) + l7route := buildHTTPRoute( + validator, + hr, + gateways, + snippetsFilters, + nil, + featureFlags, + ) g.Expect(l7route).NotTo(BeNil()) - buildHTTPMirrorRoutes(routes, l7route, hr, gateways, snippetsFilters) + buildHTTPMirrorRoutes(routes, l7route, hr, gateways, snippetsFilters, featureFlags) obj, ok := expectedMirrorRoute.Source.(*gatewayv1.HTTPRoute) g.Expect(ok).To(BeTrue()) @@ -1306,6 +1458,10 @@ func TestProcessHTTPRouteRule_InferencePoolWithMultipleBackendRefs(t *testing.T) nil, inferencePools, routeNamespace, + flags{ + plus: false, + experimental: false, + }, ) g.Expect(routeRule.RouteBackendRefs).To(BeEmpty()) @@ -1920,7 +2076,7 @@ func TestUnsupportedFieldsErrors(t *testing.T) { Type: helpers.GetPointer(gatewayv1.SessionPersistenceType("unsupported-session-persistence")), }), }, - expectedErrors: 4, + expectedErrors: 5, }, } @@ -1932,7 +2088,14 @@ func TestUnsupportedFieldsErrors(t *testing.T) { rulePath := field.NewPath("spec").Child("rules") var errors routeRuleErrors - unsupportedFieldsErrors := checkForUnsupportedHTTPFields(test.specRule, rulePath) + unsupportedFieldsErrors := checkForUnsupportedHTTPFields( + test.specRule, + rulePath, + flags{ + plus: false, + experimental: false, + }, + ) if len(unsupportedFieldsErrors) > 0 { errors.warn = append(errors.warn, unsupportedFieldsErrors...) } @@ -1952,6 +2115,8 @@ func TestProcessHTTPRouteRules_UnsupportedFields(t *testing.T) { expectedConds []conditions.Condition expectedWarns int expectedValid bool + plusEnabled bool + experimental bool }{ { name: "No unsupported fields", @@ -1983,7 +2148,8 @@ func TestProcessHTTPRouteRules_UnsupportedFields(t *testing.T) { }), Retry: helpers.GetPointer(gatewayv1.HTTPRouteRetry{Attempts: helpers.GetPointer(3)}), SessionPersistence: helpers.GetPointer(gatewayv1.SessionPersistence{ - Type: helpers.GetPointer(gatewayv1.SessionPersistenceType("unsupported-session-persistence")), + Type: helpers.GetPointer(gatewayv1.CookieBasedSessionPersistence), + SessionName: helpers.GetPointer("session_id"), }), }, }, @@ -1991,10 +2157,48 @@ func TestProcessHTTPRouteRules_UnsupportedFields(t *testing.T) { expectedConds: []conditions.Condition{ conditions.NewRouteAcceptedUnsupportedField("[spec.rules[0].name: Forbidden: Name, spec.rules[0].timeouts: " + "Forbidden: Timeouts, spec.rules[0].retry: Forbidden: Retry, " + - "spec.rules[0].sessionPersistence: Forbidden: SessionPersistence]"), + "spec.rules[0].sessionPersistence: Forbidden: " + + "SessionPersistence is only supported in NGINX Plus. This configuration will be ignored.]"), }, + experimental: true, + plusEnabled: false, expectedWarns: 4, }, + { + name: "Session persistence unsupported with experimental disabled", + specRules: []gatewayv1.HTTPRouteRule{ + { + SessionPersistence: helpers.GetPointer(gatewayv1.SessionPersistence{ + Type: helpers.GetPointer(gatewayv1.CookieBasedSessionPersistence), + SessionName: helpers.GetPointer("session_id"), + }), + }, + }, + expectedValid: true, + expectedConds: []conditions.Condition{ + conditions.NewRouteAcceptedUnsupportedField("spec.rules[0].sessionPersistence: Forbidden: " + + "SessionPersistence is only supported in experimental mode."), + }, + expectedWarns: 1, + plusEnabled: true, + experimental: false, + }, + { + name: "SessionPersistence field with Plus enabled and experimental enabled", + specRules: []gatewayv1.HTTPRouteRule{ + { + SessionPersistence: helpers.GetPointer(gatewayv1.SessionPersistence{ + Type: helpers.GetPointer(gatewayv1.CookieBasedSessionPersistence), + SessionName: helpers.GetPointer("session_id"), + }), + }, + }, + expectedValid: true, + expectedConds: nil, + expectedWarns: 0, + plusEnabled: true, + experimental: true, + }, } for _, test := range tests { @@ -2008,6 +2212,10 @@ func TestProcessHTTPRouteRules_UnsupportedFields(t *testing.T) { nil, nil, routeNamespace, + flags{ + plus: test.plusEnabled, + experimental: test.experimental, + }, ) g.Expect(valid).To(Equal(test.expectedValid)) diff --git a/internal/controller/state/graph/route_common.go b/internal/controller/state/graph/route_common.go index c6bf225f43..bc5b435884 100644 --- a/internal/controller/state/graph/route_common.go +++ b/internal/controller/state/graph/route_common.go @@ -23,6 +23,7 @@ import ( const ( wildcardHostname = "~^" inferenceAPIGroup = "inference.networking.k8s.io" + defaultNamespace = "default" ) // ParentRef describes a reference to a parent in a Route. @@ -147,6 +148,8 @@ type L7RouteSpec struct { } type RouteRule struct { + // SessionPersistence holds the session persistence configuration for the route rule. + SessionPersistence *SessionPersistenceConfig // Matches define the predicate used to match requests to a given action. Matches []v1.HTTPRouteMatch // RouteBackendRefs are a wrapper for v1.BackendRef and any BackendRef filters from the HTTPRoute or GRPCRoute. @@ -175,6 +178,19 @@ type RouteBackendRef struct { IsInferencePool bool } +type SessionPersistenceConfig struct { + // Name is the name of the session. + Name string + // Expiry determines the expiry time of the session. + Expiry string + // SessionType is the type of session persistence. + SessionType v1.SessionPersistenceType + // Path is the path for which the session persistence is allowed. + Path string + // Valid indicates if the session persistence configuration is valid. + Valid bool +} + // CreateRouteKey takes a client.Object and creates a RouteKey. func CreateRouteKey(obj client.Object) RouteKey { nsName := types.NamespacedName{ @@ -256,6 +272,7 @@ func buildRoutesForGateways( gateways map[types.NamespacedName]*Gateway, snippetsFilters map[types.NamespacedName]*SnippetsFilter, inferencePools map[types.NamespacedName]*inference.InferencePool, + featureFlags flags, ) map[RouteKey]*L7Route { if len(gateways) == 0 { return nil @@ -264,7 +281,7 @@ func buildRoutesForGateways( routes := make(map[RouteKey]*L7Route) for _, route := range httpRoutes { - r := buildHTTPRoute(validator, route, gateways, snippetsFilters, inferencePools) + r := buildHTTPRoute(validator, route, gateways, snippetsFilters, inferencePools, featureFlags) if r == nil { continue } @@ -272,11 +289,11 @@ func buildRoutesForGateways( routes[CreateRouteKey(route)] = r // if this route has a RequestMirror filter, build a duplicate route for the mirror - buildHTTPMirrorRoutes(routes, r, route, gateways, snippetsFilters) + buildHTTPMirrorRoutes(routes, r, route, gateways, snippetsFilters, featureFlags) } for _, route := range grpcRoutes { - r := buildGRPCRoute(validator, route, gateways, snippetsFilters) + r := buildGRPCRoute(validator, route, gateways, snippetsFilters, featureFlags) if r == nil { continue } @@ -284,7 +301,7 @@ func buildRoutesForGateways( routes[CreateRouteKey(route)] = r // if this route has a RequestMirror filter, build a duplicate route for the mirror - buildGRPCMirrorRoutes(routes, r, route, gateways, snippetsFilters) + buildGRPCMirrorRoutes(routes, r, route, gateways, snippetsFilters, featureFlags) } return routes @@ -1150,3 +1167,238 @@ func routeKeyForKind(kind v1.Kind, nsname types.NamespacedName) RouteKey { return key } + +// processSessionPersistenceConfig processes the session persistence configuration. +func processSessionPersistenceConfig[T any]( + sp *v1.SessionPersistence, + routeMatches []T, + rulePath *field.Path, + validator validation.HTTPFieldsValidator, +) (*SessionPersistenceConfig, routeRuleErrors) { + var spConfig SessionPersistenceConfig + expiry, errors := validateSessionPersistenceConfig(sp, rulePath, validator) + + if len(errors.warn) > 0 { + errors.warn = append(errors.warn, field.Invalid( + rulePath, + rulePath.String(), + "session persistence is ignored because there are errors in the configuration", + )) + spConfig.Valid = false + return &spConfig, errors + } + + var cookieLifetimeType v1.CookieLifetimeType + if sp.CookieConfig != nil && sp.CookieConfig.LifetimeType != nil { + cookieLifetimeType = *sp.CookieConfig.LifetimeType + } + + if sp.AbsoluteTimeout != nil { + if cookieLifetimeType == v1.SessionCookieLifetimeType { + expiry = "" + } + } + + var path string + switch rm := any(routeMatches).(type) { + case []v1.HTTPRouteMatch: + path = deriveCookiePathForHTTPMatches(rm) + case []v1.GRPCRouteMatch: + path = "" + default: + panic("unsupported route match type") + } + + spConfig = SessionPersistenceConfig{ + Valid: true, + Name: *sp.SessionName, + SessionType: *sp.Type, + Path: path, + Expiry: expiry, + } + + return &spConfig, errors +} + +// validateSessionPersistenceConfig validates the session persistence configuration. +// Returns warnings for any invalid session persistence configuration. +// but that does not make the route associated with it invalid. +func validateSessionPersistenceConfig( + sp *v1.SessionPersistence, + path *field.Path, + validator validation.HTTPFieldsValidator, +) (string, routeRuleErrors) { + if sp == nil { + return "", routeRuleErrors{} + } + + var errors routeRuleErrors + if sp.SessionName == nil { + errors.warn = append(errors.warn, field.Required(path.Child("sessionName"), "sessionName cannot be empty")) + } + + if sp.Type != nil && *sp.Type != v1.CookieBasedSessionPersistence { + errors.warn = append(errors.warn, field.NotSupported( + path.Child("type"), + sp.Type, + []string{string(v1.CookieBasedSessionPersistence)}, + )) + } + + if sp.IdleTimeout != nil { + errors.warn = append(errors.warn, field.Forbidden( + path.Child("idleTimeout"), + "IdleTimeout", + )) + } + + var timeout string + if sp.AbsoluteTimeout != nil { + if absoluteTimeout, err := validator.ValidateDuration(string(*sp.AbsoluteTimeout)); err != nil { + errors.warn = append(errors.warn, field.Invalid( + path.Child("absoluteTimeout"), + sp.AbsoluteTimeout, + err.Error(), + )) + } else { + timeout = absoluteTimeout + } + } + + return timeout, errors +} + +func deriveCookiePathForHTTPMatches(matches []v1.HTTPRouteMatch) string { + paths := make([]string, 0, len(matches)) + for _, match := range matches { + path := getCookiePath(match) + paths = append(paths, path) + } + + return longestCommonPathPrefix(paths) +} + +// longestCommonPathPrefix returns the longest common path prefix of the given +// paths. +// Examples: +// +// ["/foo/bar", "/foo/baz"] -> "/foo" +// ["/foo/bar", "/foo/bar/b"] -> "/foo/bar" +// ["/foo", "/bar"] -> "" +// [] -> "" +func longestCommonPathPrefix(paths []string) string { + if len(paths) == 0 { + return "" + } + if len(paths) == 1 { + return paths[0] + } + + commonSegs := strings.Split(paths[0], "/") + for _, p := range paths[1:] { + segs := strings.Split(p, "/") + i := 0 + limit := len(commonSegs) + if len(segs) < limit { + limit = len(segs) + } + for i < limit && commonSegs[i] == segs[i] { + i++ + } + // truncate commonSegs to the common prefix + commonSegs = commonSegs[:i] + if len(commonSegs) == 0 { + return "" + } + } + + return strings.Join(commonSegs, "/") +} + +func getCookiePath(match v1.HTTPRouteMatch) string { + pathType := *match.Path.Type + + switch pathType { + case v1.PathMatchExact, v1.PathMatchPathPrefix: + return *match.Path.Value + default: + return "" + } +} + +// handleSessionPersistenceConflicts enforces: +// 1. For each backend (ns/name:port), all rules that configure SessionPersistence +// for that backend must have the same SessionPersistenceConfig. +// 2. If multiple rules configure different session persistence configs for the same backend, it +// is cleared on all those rules for that backend and a warning is emitted. +func handleSessionPersistenceConflicts(rules []RouteRule) error { + if len(rules) == 0 { + return nil + } + + backendToRuleIdxs := make(map[string][]int) + spPerBackendRef := make(map[string]*SessionPersistenceConfig) + + for ri, rule := range rules { + sp := rule.SessionPersistence + if sp == nil { + continue + } + + for _, rbr := range rule.RouteBackendRefs { + key := createBackendRefKey(rbr.BackendRef) + backendToRuleIdxs[key] = append(backendToRuleIdxs[key], ri) + + if existing, ok := spPerBackendRef[key]; !ok { + spPerBackendRef[key] = sp + } else if !equalSessionPersistenceConfig(existing, sp) { + spPerBackendRef[key] = nil + } + } + } + + hadConflict := false + conflictingBackends := make([]string, 0) + for backendKey, sp := range spPerBackendRef { + if sp != nil { + continue + } + + hadConflict = true + conflictingBackends = append(conflictingBackends, backendKey) + for _, ri := range backendToRuleIdxs[backendKey] { + rules[ri].SessionPersistence = nil + } + } + + if !hadConflict { + return nil + } + + return fmt.Errorf("for backendRefs %s due to conflicting configuration across multiple rules", + strings.Join(conflictingBackends, ", "), + ) +} + +func equalSessionPersistenceConfig(a, b *SessionPersistenceConfig) bool { + if a == nil || b == nil { + return a == b + } + return a.Name == b.Name && + a.SessionType == b.SessionType && + a.Expiry == b.Expiry && + a.Path == b.Path +} + +func createBackendRefKey(backendRef v1.BackendRef) string { + var port int32 + if backendRef.Port != nil { + port = *backendRef.Port + } + + ns := defaultNamespace + if backendRef.Namespace != nil { + ns = string(*backendRef.Namespace) + } + return fmt.Sprintf("%s/%s:%d", ns, backendRef.Name, port) +} diff --git a/internal/controller/state/graph/route_common_test.go b/internal/controller/state/graph/route_common_test.go index 52ff2ca2af..e91dd2e865 100644 --- a/internal/controller/state/graph/route_common_test.go +++ b/internal/controller/state/graph/route_common_test.go @@ -15,6 +15,7 @@ import ( "sigs.k8s.io/gateway-api/apis/v1alpha2" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/state/conditions" + "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/state/validation/validationfakes" "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/helpers" "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/kinds" ) @@ -3867,3 +3868,706 @@ func TestFindAttachableListenersWithPort(t *testing.T) { }) } } + +func TestProcessSessionPersistenceConfiguration(t *testing.T) { + t.Parallel() + + createAllValidValidator := func(duration *gatewayv1.Duration) *validationfakes.FakeHTTPFieldsValidator { + v := &validationfakes.FakeHTTPFieldsValidator{} + if duration == nil { + v.ValidateDurationReturns("", nil) + } else { + v.ValidateDurationReturns(string(*duration), nil) + } + return v + } + + sessionPersistencePath := field.NewPath("sessionPersistence") + tests := []struct { + name string + sessionPersistence *gatewayv1.SessionPersistence + expectedResult SessionPersistenceConfig + expectedErrors routeRuleErrors + httpRouteMatches []gatewayv1.HTTPRouteMatch + grpcRouteMatches []gatewayv1.GRPCRouteMatch + }{ + { + name: "session persistence has errors in configuration", + sessionPersistence: &gatewayv1.SessionPersistence{ + Type: helpers.GetPointer(gatewayv1.CookieBasedSessionPersistence), + }, + expectedErrors: routeRuleErrors{ + warn: field.ErrorList{ + field.Required(sessionPersistencePath.Child("sessionName"), "sessionName cannot be empty"), + field.Invalid( + sessionPersistencePath, + sessionPersistencePath.String(), + "session persistence is ignored because there are errors in the configuration", + ), + }, + }, + expectedResult: SessionPersistenceConfig{ + Valid: false, + }, + httpRouteMatches: []gatewayv1.HTTPRouteMatch{ + { + Path: &gatewayv1.HTTPPathMatch{ + Type: helpers.GetPointer(gatewayv1.PathMatchExact), + Value: helpers.GetPointer("/coffee"), + }, + }, + }, + }, + { + name: "when lifetime type of a cookie is Session, timeout is set to 0", + sessionPersistence: &gatewayv1.SessionPersistence{ + SessionName: helpers.GetPointer("session-persistence"), + Type: helpers.GetPointer(gatewayv1.CookieBasedSessionPersistence), + AbsoluteTimeout: helpers.GetPointer(gatewayv1.Duration("20m")), + CookieConfig: &gatewayv1.CookieConfig{ + LifetimeType: helpers.GetPointer(gatewayv1.SessionCookieLifetimeType), + }, + }, + expectedResult: SessionPersistenceConfig{ + Valid: true, + SessionType: gatewayv1.CookieBasedSessionPersistence, + Name: "session-persistence", + Path: "/tea", + }, + httpRouteMatches: []gatewayv1.HTTPRouteMatch{ + { + Path: &gatewayv1.HTTPPathMatch{ + Type: helpers.GetPointer(gatewayv1.PathMatchExact), + Value: helpers.GetPointer("/tea"), + }, + }, + }, + }, + { + name: "valid session persistence configuration for HTTPRoute", + sessionPersistence: &gatewayv1.SessionPersistence{ + SessionName: helpers.GetPointer("session-persistence-http"), + Type: helpers.GetPointer(gatewayv1.CookieBasedSessionPersistence), + AbsoluteTimeout: helpers.GetPointer(gatewayv1.Duration("1h")), + CookieConfig: &gatewayv1.CookieConfig{ + LifetimeType: helpers.GetPointer(gatewayv1.PermanentCookieLifetimeType), + }, + }, + expectedResult: SessionPersistenceConfig{ + Valid: true, + SessionType: gatewayv1.CookieBasedSessionPersistence, + Name: "session-persistence-http", + Expiry: "1h", + Path: "/app/v1", + }, + httpRouteMatches: []gatewayv1.HTTPRouteMatch{ + { + Path: &gatewayv1.HTTPPathMatch{ + Type: helpers.GetPointer(gatewayv1.PathMatchExact), + Value: helpers.GetPointer("/app/v1/users/"), + }, + }, + { + Path: &gatewayv1.HTTPPathMatch{ + Type: helpers.GetPointer(gatewayv1.PathMatchPathPrefix), + Value: helpers.GetPointer("/app/v1/latte/"), + }, + }, + { + Path: &gatewayv1.HTTPPathMatch{ + Type: helpers.GetPointer(gatewayv1.PathMatchExact), + Value: helpers.GetPointer("/app/v1/tea"), + }, + }, + { + Path: &gatewayv1.HTTPPathMatch{ + Type: helpers.GetPointer(gatewayv1.PathMatchPathPrefix), + Value: helpers.GetPointer("/app/v1/coffee"), + }, + }, + }, + }, + { + name: "valid session persistence configuration for GRPCRoute", + sessionPersistence: &gatewayv1.SessionPersistence{ + SessionName: helpers.GetPointer("session-persistence-grpc"), + Type: helpers.GetPointer(gatewayv1.CookieBasedSessionPersistence), + AbsoluteTimeout: helpers.GetPointer(gatewayv1.Duration("30m")), + CookieConfig: &gatewayv1.CookieConfig{ + LifetimeType: helpers.GetPointer(gatewayv1.PermanentCookieLifetimeType), + }, + }, + expectedResult: SessionPersistenceConfig{ + Valid: true, + SessionType: gatewayv1.CookieBasedSessionPersistence, + Name: "session-persistence-grpc", + Expiry: "30m", + }, + grpcRouteMatches: []gatewayv1.GRPCRouteMatch{ + { + Method: &gatewayv1.GRPCMethodMatch{ + Type: helpers.GetPointer(gatewayv1.GRPCMethodMatchExact), + Service: helpers.GetPointer("mymethod.user"), + }, + }, + { + Method: &gatewayv1.GRPCMethodMatch{ + Type: helpers.GetPointer(gatewayv1.GRPCMethodMatchExact), + Service: helpers.GetPointer("mymethod.coffee"), + }, + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + var result *SessionPersistenceConfig + var errors routeRuleErrors + if test.httpRouteMatches != nil { + result, errors = processSessionPersistenceConfig( + test.sessionPersistence, + test.httpRouteMatches, + sessionPersistencePath, + createAllValidValidator(test.sessionPersistence.AbsoluteTimeout), + ) + } + + if test.grpcRouteMatches != nil { + result, errors = processSessionPersistenceConfig( + test.sessionPersistence, + test.grpcRouteMatches, + sessionPersistencePath, + createAllValidValidator(test.sessionPersistence.AbsoluteTimeout), + ) + } + + g.Expect(result).To(HaveValue(Equal(test.expectedResult))) + g.Expect(errors).To(Equal(test.expectedErrors)) + }) + } +} + +func TestValidateSessionPersistence(t *testing.T) { + t.Parallel() + + createAllValidValidator := func() *validationfakes.FakeHTTPFieldsValidator { + v := &validationfakes.FakeHTTPFieldsValidator{} + v.ValidateDurationReturns("", nil) + return v + } + + createInvalidDurationValidator := func() *validationfakes.FakeHTTPFieldsValidator { + v := &validationfakes.FakeHTTPFieldsValidator{} + v.ValidateDurationReturns("", errors.New("invalid duration format")) + return v + } + + sessionPersistencePath := field.NewPath("sessionPersistence") + tests := []struct { + sessionPersistence *gatewayv1.SessionPersistence + validator *validationfakes.FakeHTTPFieldsValidator + name string + expectedErrors routeRuleErrors + }{ + { + name: "session persistence returns error for empty name", + sessionPersistence: &gatewayv1.SessionPersistence{ + Type: helpers.GetPointer(gatewayv1.CookieBasedSessionPersistence), + }, + expectedErrors: routeRuleErrors{ + warn: field.ErrorList{ + field.Required(sessionPersistencePath.Child("sessionName"), "sessionName cannot be empty"), + }, + }, + validator: createAllValidValidator(), + }, + { + name: "session persistence returns error for invalid type", + sessionPersistence: &gatewayv1.SessionPersistence{ + SessionName: helpers.GetPointer("session-persistence"), + Type: helpers.GetPointer(gatewayv1.HeaderBasedSessionPersistence), + }, + expectedErrors: routeRuleErrors{ + warn: field.ErrorList{ + field.NotSupported( + sessionPersistencePath.Child("type"), + helpers.GetPointer(gatewayv1.HeaderBasedSessionPersistence), + []string{string(gatewayv1.CookieBasedSessionPersistence)}, + ), + }, + }, + validator: createAllValidValidator(), + }, + { + name: "session persistence returns error when idleTimeout is specified", + sessionPersistence: &gatewayv1.SessionPersistence{ + SessionName: helpers.GetPointer("session-persistence"), + Type: helpers.GetPointer(gatewayv1.CookieBasedSessionPersistence), + IdleTimeout: helpers.GetPointer(gatewayv1.Duration("10m")), + }, + expectedErrors: routeRuleErrors{ + warn: field.ErrorList{ + field.Forbidden( + sessionPersistencePath.Child("idleTimeout"), + "IdleTimeout", + ), + }, + }, + validator: createAllValidValidator(), + }, + { + name: "session persistence returns error when absoluteTimeout is invalid", + sessionPersistence: &gatewayv1.SessionPersistence{ + SessionName: helpers.GetPointer("session-persistence"), + Type: helpers.GetPointer(gatewayv1.CookieBasedSessionPersistence), + AbsoluteTimeout: helpers.GetPointer(gatewayv1.Duration("invalid-duration")), + }, + expectedErrors: routeRuleErrors{ + warn: field.ErrorList{ + field.Invalid( + sessionPersistencePath.Child("absoluteTimeout"), + helpers.GetPointer(gatewayv1.Duration("invalid-duration")), + "invalid duration format", + ), + }, + }, + validator: createInvalidDurationValidator(), + }, + { + name: "valid session persistence returns no errors", + sessionPersistence: &gatewayv1.SessionPersistence{ + SessionName: helpers.GetPointer("session-persistence"), + Type: helpers.GetPointer(gatewayv1.CookieBasedSessionPersistence), + AbsoluteTimeout: helpers.GetPointer(gatewayv1.Duration("30m")), + CookieConfig: &gatewayv1.CookieConfig{ + LifetimeType: helpers.GetPointer(gatewayv1.PermanentCookieLifetimeType), + }, + }, + validator: createAllValidValidator(), + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + _, errors := validateSessionPersistenceConfig( + test.sessionPersistence, + field.NewPath("sessionPersistence"), + test.validator, + ) + g.Expect(errors).To(Equal(test.expectedErrors)) + }) + } +} + +func TestGetCookiePath(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + expectedPath string + matches []gatewayv1.HTTPRouteMatch + }{ + { + name: "no matches returns empty path", + matches: []gatewayv1.HTTPRouteMatch{}, + expectedPath: "", + }, + { + name: "single match with type Exact returns that path", + matches: []gatewayv1.HTTPRouteMatch{ + { + Path: &gatewayv1.HTTPPathMatch{ + Type: helpers.GetPointer(gatewayv1.PathMatchExact), + Value: helpers.GetPointer("/app/users"), + }, + }, + }, + expectedPath: "/app/users", + }, + { + name: "single match with type Prefix returns that path", + matches: []gatewayv1.HTTPRouteMatch{ + { + Path: &gatewayv1.HTTPPathMatch{ + Type: helpers.GetPointer(gatewayv1.PathMatchPathPrefix), + Value: helpers.GetPointer("/app/orders"), + }, + }, + }, + expectedPath: "/app/orders", + }, + { + name: "single match with type Regular Expression returns empty path", + matches: []gatewayv1.HTTPRouteMatch{ + { + Path: &gatewayv1.HTTPPathMatch{ + Type: helpers.GetPointer(gatewayv1.PathMatchRegularExpression), + Value: helpers.GetPointer("/app/[a-z]+/orders"), + }, + }, + }, + expectedPath: "", + }, + { + name: "multiple matches with all three types of matches returns empty path", + matches: []gatewayv1.HTTPRouteMatch{ + { + Path: &gatewayv1.HTTPPathMatch{ + Type: helpers.GetPointer(gatewayv1.PathMatchRegularExpression), + Value: helpers.GetPointer("/app/[a-z]+/orders"), + }, + }, + { + Path: &gatewayv1.HTTPPathMatch{ + Type: helpers.GetPointer(gatewayv1.PathMatchPathPrefix), + Value: helpers.GetPointer("/app/users/login"), + }, + }, + { + Path: &gatewayv1.HTTPPathMatch{ + Type: helpers.GetPointer(gatewayv1.PathMatchExact), + Value: helpers.GetPointer("/app/users/"), + }, + }, + }, + expectedPath: "", + }, + { + name: "multiple matches with all predefine path types Exact and PathPrefix " + + "returns longest common prefix", + matches: []gatewayv1.HTTPRouteMatch{ + { + Path: &gatewayv1.HTTPPathMatch{ + Type: helpers.GetPointer(gatewayv1.PathMatchPathPrefix), + Value: helpers.GetPointer("/app/users/profile/"), + }, + }, + { + Path: &gatewayv1.HTTPPathMatch{ + Type: helpers.GetPointer(gatewayv1.PathMatchPathPrefix), + Value: helpers.GetPointer("/app/users/login"), + }, + }, + { + Path: &gatewayv1.HTTPPathMatch{ + Type: helpers.GetPointer(gatewayv1.PathMatchExact), + Value: helpers.GetPointer("/app/users/orders/"), + }, + }, + { + Path: &gatewayv1.HTTPPathMatch{ + Type: helpers.GetPointer(gatewayv1.PathMatchPathPrefix), + Value: helpers.GetPointer("/app/users/history"), + }, + }, + }, + expectedPath: "/app/users", + }, + { + name: "multiple matches with all predefine path types Exact and PathPrefix " + + "returns empty path when there is no common prefix", + matches: []gatewayv1.HTTPRouteMatch{ + { + Path: &gatewayv1.HTTPPathMatch{ + Type: helpers.GetPointer(gatewayv1.PathMatchExact), + Value: helpers.GetPointer("/app/v1"), + }, + }, + { + Path: &gatewayv1.HTTPPathMatch{ + Type: helpers.GetPointer(gatewayv1.PathMatchPathPrefix), + Value: helpers.GetPointer("/coffee/latte/"), + }, + }, + { + Path: &gatewayv1.HTTPPathMatch{ + Type: helpers.GetPointer(gatewayv1.PathMatchExact), + Value: helpers.GetPointer("/coffee/espresso"), + }, + }, + { + Path: &gatewayv1.HTTPPathMatch{ + Type: helpers.GetPointer(gatewayv1.PathMatchPathPrefix), + Value: helpers.GetPointer("/tea/green"), + }, + }, + }, + expectedPath: "", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + result := deriveCookiePathForHTTPMatches(test.matches) + g.Expect(result).To(Equal(test.expectedPath)) + }) + } +} + +func TestHandleSessionPersistenceConflict_RouteRules(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + routeRuleNoConflict := []RouteRule{ + { + SessionPersistence: &SessionPersistenceConfig{ + Valid: true, + SessionType: gatewayv1.CookieBasedSessionPersistence, + Name: "session-1", + Expiry: "30m", + }, + RouteBackendRefs: []RouteBackendRef{ + { + BackendRef: gatewayv1.BackendRef{ + BackendObjectReference: gatewayv1.BackendObjectReference{ + Name: "backend-1", + Port: helpers.GetPointer[gatewayv1.PortNumber](8080), + Namespace: helpers.GetPointer(gatewayv1.Namespace("default")), + }, + }, + }, + }, + }, + { + SessionPersistence: &SessionPersistenceConfig{ + Valid: true, + SessionType: gatewayv1.CookieBasedSessionPersistence, + Name: "session-2", + Expiry: "1h", + }, + RouteBackendRefs: []RouteBackendRef{ + { + BackendRef: gatewayv1.BackendRef{ + BackendObjectReference: gatewayv1.BackendObjectReference{ + Name: "backend-2", + Port: helpers.GetPointer[gatewayv1.PortNumber](8080), + Namespace: helpers.GetPointer(gatewayv1.Namespace("default")), + }, + }, + }, + }, + }, + } + + errNoConflict := handleSessionPersistenceConflicts(routeRuleNoConflict) + g.Expect(errNoConflict).ToNot(HaveOccurred()) + + // backend-1 has same session persistence config across multiple rules - no conflict + // backend-2 has different session persistence config across multiple rules - conflict + routeRuleWithConflict := []RouteRule{ + { + SessionPersistence: &SessionPersistenceConfig{ + Valid: true, + SessionType: gatewayv1.CookieBasedSessionPersistence, + Name: "session-1", + Expiry: "30m", + }, + RouteBackendRefs: []RouteBackendRef{ + { + BackendRef: gatewayv1.BackendRef{ + BackendObjectReference: gatewayv1.BackendObjectReference{ + Name: "backend-1", + Port: helpers.GetPointer[gatewayv1.PortNumber](8080), + Namespace: helpers.GetPointer(gatewayv1.Namespace("default")), + }, + }, + }, + }, + }, + { + SessionPersistence: &SessionPersistenceConfig{ + Valid: true, + SessionType: gatewayv1.CookieBasedSessionPersistence, + Name: "session-1", + Expiry: "30m", + }, + RouteBackendRefs: []RouteBackendRef{ + { + BackendRef: gatewayv1.BackendRef{ + BackendObjectReference: gatewayv1.BackendObjectReference{ + Name: "backend-1", + Port: helpers.GetPointer[gatewayv1.PortNumber](8080), + Namespace: helpers.GetPointer(gatewayv1.Namespace("default")), + }, + }, + }, + }, + }, + { + SessionPersistence: &SessionPersistenceConfig{ + Valid: true, + SessionType: gatewayv1.CookieBasedSessionPersistence, + Name: "session-3", + Expiry: "24h", + }, + RouteBackendRefs: []RouteBackendRef{ + { + BackendRef: gatewayv1.BackendRef{ + BackendObjectReference: gatewayv1.BackendObjectReference{ + Name: "backend-2", + Port: helpers.GetPointer[gatewayv1.PortNumber](8080), + Namespace: helpers.GetPointer(gatewayv1.Namespace("default")), + }, + }, + }, + }, + }, + { + SessionPersistence: &SessionPersistenceConfig{ + Valid: true, + SessionType: gatewayv1.CookieBasedSessionPersistence, + Name: "session-4", + Expiry: "20h", + }, + RouteBackendRefs: []RouteBackendRef{ + { + BackendRef: gatewayv1.BackendRef{ + BackendObjectReference: gatewayv1.BackendObjectReference{ + Name: "backend-2", + Port: helpers.GetPointer[gatewayv1.PortNumber](8080), + Namespace: helpers.GetPointer(gatewayv1.Namespace("default")), + }, + }, + }, + }, + }, + { + SessionPersistence: &SessionPersistenceConfig{ + Valid: true, + SessionType: gatewayv1.CookieBasedSessionPersistence, + Name: "session-5", + Expiry: "20h", + }, + RouteBackendRefs: []RouteBackendRef{ + { + BackendRef: gatewayv1.BackendRef{ + BackendObjectReference: gatewayv1.BackendObjectReference{ + Name: "backend-3", + Port: helpers.GetPointer[gatewayv1.PortNumber](8080), + Namespace: helpers.GetPointer(gatewayv1.Namespace("default")), + }, + }, + }, + }, + }, + } + + errWithConflict := handleSessionPersistenceConflicts(routeRuleWithConflict) + g.Expect(errWithConflict).To(HaveOccurred()) + g.Expect(errWithConflict.Error()).To(ContainSubstring( + "for backendRefs default/backend-2:8080 due to conflicting configuration across multiple rules", + )) +} + +func TestEqualSessionPersistence(t *testing.T) { + t.Parallel() + + tests := []struct { + sp1 *SessionPersistenceConfig + sp2 *SessionPersistenceConfig + name string + expected bool + }{ + { + name: "both nil session persistence are equal", + sp1: nil, + sp2: nil, + expected: true, + }, + { + name: "one nil session persistence is not equal", + sp1: &SessionPersistenceConfig{ + Valid: true, + SessionType: gatewayv1.CookieBasedSessionPersistence, + Name: "session-1", + Expiry: "30m", + }, + sp2: nil, + expected: false, + }, + { + name: "different session names are not equal", + sp1: &SessionPersistenceConfig{ + Valid: true, + SessionType: gatewayv1.CookieBasedSessionPersistence, + Name: "session-1", + Expiry: "30m", + }, + sp2: &SessionPersistenceConfig{ + Valid: true, + SessionType: gatewayv1.CookieBasedSessionPersistence, + Name: "session-2", + Expiry: "30m", + }, + expected: false, + }, + { + name: "different expiry durations are not equal", + sp1: &SessionPersistenceConfig{ + Valid: true, + SessionType: gatewayv1.CookieBasedSessionPersistence, + Name: "session-1", + Expiry: "30m", + }, + sp2: &SessionPersistenceConfig{ + Valid: true, + SessionType: gatewayv1.CookieBasedSessionPersistence, + Name: "session-1", + Expiry: "1h", + }, + expected: false, + }, + { + name: "different session types are not equal", + sp1: &SessionPersistenceConfig{ + Valid: true, + SessionType: gatewayv1.CookieBasedSessionPersistence, + Name: "session-1", + Expiry: "30m", + }, + sp2: &SessionPersistenceConfig{ + Valid: true, + SessionType: gatewayv1.HeaderBasedSessionPersistence, + Name: "session-1", + Expiry: "30m", + }, + expected: false, + }, + { + name: "same session persistence are equal", + sp1: &SessionPersistenceConfig{ + Valid: true, + SessionType: gatewayv1.CookieBasedSessionPersistence, + Name: "session-1", + Expiry: "30m", + }, + sp2: &SessionPersistenceConfig{ + Valid: true, + SessionType: gatewayv1.CookieBasedSessionPersistence, + Name: "session-1", + Expiry: "30m", + }, + expected: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + result := equalSessionPersistenceConfig(test.sp1, test.sp2) + g.Expect(result).To(Equal(test.expected)) + }) + } +} diff --git a/internal/controller/state/validation/validationfakes/fake_httpfields_validator.go b/internal/controller/state/validation/validationfakes/fake_httpfields_validator.go index cd5ff2d8f7..f82c40610d 100644 --- a/internal/controller/state/validation/validationfakes/fake_httpfields_validator.go +++ b/internal/controller/state/validation/validationfakes/fake_httpfields_validator.go @@ -18,6 +18,19 @@ type FakeHTTPFieldsValidator struct { skipValidationReturnsOnCall map[int]struct { result1 bool } + ValidateDurationStub func(string) (string, error) + validateDurationMutex sync.RWMutex + validateDurationArgsForCall []struct { + arg1 string + } + validateDurationReturns struct { + result1 string + result2 error + } + validateDurationReturnsOnCall map[int]struct { + result1 string + result2 error + } ValidateFilterHeaderNameStub func(string) error validateFilterHeaderNameMutex sync.RWMutex validateFilterHeaderNameArgsForCall []struct { @@ -235,6 +248,70 @@ func (fake *FakeHTTPFieldsValidator) SkipValidationReturnsOnCall(i int, result1 }{result1} } +func (fake *FakeHTTPFieldsValidator) ValidateDuration(arg1 string) (string, error) { + fake.validateDurationMutex.Lock() + ret, specificReturn := fake.validateDurationReturnsOnCall[len(fake.validateDurationArgsForCall)] + fake.validateDurationArgsForCall = append(fake.validateDurationArgsForCall, struct { + arg1 string + }{arg1}) + stub := fake.ValidateDurationStub + fakeReturns := fake.validateDurationReturns + fake.recordInvocation("ValidateDuration", []interface{}{arg1}) + fake.validateDurationMutex.Unlock() + if stub != nil { + return stub(arg1) + } + if specificReturn { + return ret.result1, ret.result2 + } + return fakeReturns.result1, fakeReturns.result2 +} + +func (fake *FakeHTTPFieldsValidator) ValidateDurationCallCount() int { + fake.validateDurationMutex.RLock() + defer fake.validateDurationMutex.RUnlock() + return len(fake.validateDurationArgsForCall) +} + +func (fake *FakeHTTPFieldsValidator) ValidateDurationCalls(stub func(string) (string, error)) { + fake.validateDurationMutex.Lock() + defer fake.validateDurationMutex.Unlock() + fake.ValidateDurationStub = stub +} + +func (fake *FakeHTTPFieldsValidator) ValidateDurationArgsForCall(i int) string { + fake.validateDurationMutex.RLock() + defer fake.validateDurationMutex.RUnlock() + argsForCall := fake.validateDurationArgsForCall[i] + return argsForCall.arg1 +} + +func (fake *FakeHTTPFieldsValidator) ValidateDurationReturns(result1 string, result2 error) { + fake.validateDurationMutex.Lock() + defer fake.validateDurationMutex.Unlock() + fake.ValidateDurationStub = nil + fake.validateDurationReturns = struct { + result1 string + result2 error + }{result1, result2} +} + +func (fake *FakeHTTPFieldsValidator) ValidateDurationReturnsOnCall(i int, result1 string, result2 error) { + fake.validateDurationMutex.Lock() + defer fake.validateDurationMutex.Unlock() + fake.ValidateDurationStub = nil + if fake.validateDurationReturnsOnCall == nil { + fake.validateDurationReturnsOnCall = make(map[int]struct { + result1 string + result2 error + }) + } + fake.validateDurationReturnsOnCall[i] = struct { + result1 string + result2 error + }{result1, result2} +} + func (fake *FakeHTTPFieldsValidator) ValidateFilterHeaderName(arg1 string) error { fake.validateFilterHeaderNameMutex.Lock() ret, specificReturn := fake.validateFilterHeaderNameReturnsOnCall[len(fake.validateFilterHeaderNameArgsForCall)] diff --git a/internal/controller/state/validation/validator.go b/internal/controller/state/validation/validator.go index 5c1bd54e4e..44f2f268e9 100644 --- a/internal/controller/state/validation/validator.go +++ b/internal/controller/state/validation/validator.go @@ -37,6 +37,7 @@ type HTTPFieldsValidator interface { ValidateFilterHeaderName(name string) error ValidateFilterHeaderValue(value string) error ValidatePath(path string) error + ValidateDuration(duration string) (string, error) } // GenericValidator validates any generic values from NGF API resources from the perspective of a data-plane. @@ -83,3 +84,4 @@ func (SkipValidator) ValidateHostname(string) error { return n func (SkipValidator) ValidateFilterHeaderName(string) error { return nil } func (SkipValidator) ValidateFilterHeaderValue(string) error { return nil } func (SkipValidator) ValidatePath(string) error { return nil } +func (SkipValidator) ValidateDuration(string) (string, error) { return "", nil } From 864ddd8927714f9545ada7c5b8c8413fced06f1b Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Thu, 20 Nov 2025 14:11:54 -0700 Subject: [PATCH 2/2] add sp config to backend ref --- internal/controller/manager.go | 11 +- .../controller/nginx/config/upstreams_test.go | 12 +- internal/controller/state/change_processor.go | 6 +- .../controller/state/conditions/conditions.go | 15 - .../state/dataplane/configuration.go | 11 +- .../state/dataplane/configuration_test.go | 203 ++++------ internal/controller/state/dataplane/types.go | 5 +- .../controller/state/graph/backend_refs.go | 14 +- .../state/graph/backend_refs_test.go | 36 +- internal/controller/state/graph/graph.go | 22 +- internal/controller/state/graph/graph_test.go | 38 +- internal/controller/state/graph/grpcroute.go | 90 ++--- .../controller/state/graph/grpcroute_test.go | 352 +++++------------- internal/controller/state/graph/httproute.go | 103 ++--- .../controller/state/graph/httproute_test.go | 232 +++++------- .../state/graph/multiple_gateways_test.go | 8 +- .../controller/state/graph/route_common.go | 90 +---- .../state/graph/route_common_test.go | 258 ------------- 18 files changed, 475 insertions(+), 1031 deletions(-) diff --git a/internal/controller/manager.go b/internal/controller/manager.go index fe3c387d2e..8b8c00b607 100644 --- a/internal/controller/manager.go +++ b/internal/controller/manager.go @@ -140,10 +140,13 @@ func StartManager(cfg config.Config) error { GenericValidator: genericValidator, PolicyValidator: policyManager, }, - EventRecorder: recorder, - MustExtractGVK: mustExtractGVK, - PlusSecrets: plusSecrets, - ExperimentalFeatures: cfg.ExperimentalFeatures, + EventRecorder: recorder, + MustExtractGVK: mustExtractGVK, + PlusSecrets: plusSecrets, + FeatureFlags: graph.FeatureFlags{ + Plus: cfg.Plus, + Experimental: cfg.ExperimentalFeatures, + }, }) var handlerCollector handlerMetricsCollector = collectors.NewControllerNoopCollector() diff --git a/internal/controller/nginx/config/upstreams_test.go b/internal/controller/nginx/config/upstreams_test.go index 2e61d858a7..b638d5d04f 100644 --- a/internal/controller/nginx/config/upstreams_test.go +++ b/internal/controller/nginx/config/upstreams_test.go @@ -229,7 +229,7 @@ func TestExecuteUpstreams_NginxPlus(t *testing.T) { Name: "session-persistence", Expiry: "30m", Path: "/session", - SessionType: dataplane.SessionPersistenceCookie, + SessionType: dataplane.CookieBasedSessionPersistence, }, }, { @@ -245,7 +245,7 @@ func TestExecuteUpstreams_NginxPlus(t *testing.T) { Name: "session-persistence", Expiry: "100h", Path: "/v1/users", - SessionType: dataplane.SessionPersistenceCookie, + SessionType: dataplane.CookieBasedSessionPersistence, }, }, { @@ -259,7 +259,7 @@ func TestExecuteUpstreams_NginxPlus(t *testing.T) { }, SessionPersistence: dataplane.SessionPersistenceConfig{ Name: "session-persistence", - SessionType: dataplane.SessionPersistenceCookie, + SessionType: dataplane.CookieBasedSessionPersistence, }, }, } @@ -275,7 +275,7 @@ func TestExecuteUpstreams_NginxPlus(t *testing.T) { "upstream up8-with-sp-expiry-and-path-empty": 1, "upstream invalid-backend-ref": 1, - "random two least_conn;": 6, + "random two least_conn;": 7, "ip_hash;": 1, "zone up1 1m;": 1, @@ -925,7 +925,7 @@ func TestCreateUpstreamPlus(t *testing.T) { SessionPersistence: dataplane.SessionPersistenceConfig{ Name: "session-persistence", Expiry: "45m", - SessionType: dataplane.SessionPersistenceCookie, + SessionType: dataplane.CookieBasedSessionPersistence, Path: "/app", }, }, @@ -942,7 +942,7 @@ func TestCreateUpstreamPlus(t *testing.T) { SessionPersistence: http.UpstreamSessionPersistence{ Name: "session-persistence", Expiry: "45m", - SessionType: string(dataplane.SessionPersistenceCookie), + SessionType: string(dataplane.CookieBasedSessionPersistence), Path: "/app", }, }, diff --git a/internal/controller/state/change_processor.go b/internal/controller/state/change_processor.go index d661903b8c..74bef98779 100644 --- a/internal/controller/state/change_processor.go +++ b/internal/controller/state/change_processor.go @@ -64,8 +64,8 @@ type ChangeProcessorConfig struct { GatewayCtlrName string // GatewayClassName is the name of the GatewayClass resource. GatewayClassName string - // ExperimentalFeatures indicates if experimental features are enabled. - ExperimentalFeatures bool + // FeaturesFlags holds the feature flags for building the Graph. + FeatureFlags graph.FeatureFlags } // ChangeProcessorImpl is an implementation of ChangeProcessor. @@ -278,7 +278,7 @@ func (c *ChangeProcessorImpl) Process() *graph.Graph { c.cfg.PlusSecrets, c.cfg.Validators, c.cfg.Logger, - c.cfg.ExperimentalFeatures, + c.cfg.FeatureFlags, ) return c.latestGraph diff --git a/internal/controller/state/conditions/conditions.go b/internal/controller/state/conditions/conditions.go index e677b6be18..8e186fe1a4 100644 --- a/internal/controller/state/conditions/conditions.go +++ b/internal/controller/state/conditions/conditions.go @@ -42,10 +42,6 @@ const ( // not yet supported. RouteReasonUnsupportedField v1.RouteConditionReason = "UnsupportedField" - // RouteReasonInvalidSessionPersistence is used with the "Accepted" (true) condition when the Route - // contains invalid session persistence configuration. - RouteReasonInvalidSessionPersistence v1.RouteConditionReason = "InvalidSessionPersistence" - // RouteReasonInvalidGateway is used with the "Accepted" (false) condition when the Gateway the Route // references is invalid. RouteReasonInvalidGateway v1.RouteConditionReason = "InvalidGateway" @@ -397,17 +393,6 @@ func NewRouteAcceptedUnsupportedField(msg string) Condition { } } -// NewRouteAcceptedInvalidSessionPersistenceConfiguration returns a Condition that indicates that the -// Route is accepted but invalid session persistence configuration was ignored. -func NewRouteAcceptedInvalidSessionPersistenceConfiguration(msg string) Condition { - return Condition{ - Type: string(v1.RouteConditionAccepted), - Status: metav1.ConditionTrue, - Reason: string(RouteReasonInvalidSessionPersistence), - Message: fmt.Sprintf("Session Persistence configuration ignored: %s", msg), - } -} - // NewRoutePartiallyInvalid returns a Condition that indicates that the Route contains a combination // of both valid and invalid rules. // diff --git a/internal/controller/state/dataplane/configuration.go b/internal/controller/state/dataplane/configuration.go index 3e20be893c..ea85d46627 100644 --- a/internal/controller/state/dataplane/configuration.go +++ b/internal/controller/state/dataplane/configuration.go @@ -774,12 +774,6 @@ func buildUpstreams( } for _, br := range rule.BackendRefs { - var spForThisBackend *graph.SessionPersistenceConfig - // only set session persistence for regular backends - if rule.SessionPersistence != nil && !br.IsMirrorBackend && !br.IsInferencePool { - spForThisBackend = rule.SessionPersistence - } - if upstream := buildUpstream( ctx, logger, @@ -789,7 +783,7 @@ func buildUpstreams( referencedServices, uniqueUpstreams, allowedAddressType, - spForThisBackend, + br.SessionPersistence, ); upstream != nil { uniqueUpstreams[upstream.Name] = *upstream } @@ -844,7 +838,6 @@ func buildUpstream( } var errMsg string - eps, err := resolveUpstreamEndpoints( ctx, logger, @@ -871,7 +864,7 @@ func buildUpstream( Name: sessionPersistence.Name, Expiry: sessionPersistence.Expiry, Path: sessionPersistence.Path, - SessionType: SessionPersistenceCookie, + SessionType: CookieBasedSessionPersistence, } } diff --git a/internal/controller/state/dataplane/configuration_test.go b/internal/controller/state/dataplane/configuration_test.go index 7d5a5e2d5b..dead623b49 100644 --- a/internal/controller/state/dataplane/configuration_test.go +++ b/internal/controller/state/dataplane/configuration_test.go @@ -2873,18 +2873,14 @@ func TestGetListenerHostname(t *testing.T) { } } -func refsToValidRules( - sessionPersistence *graph.SessionPersistenceConfig, - refs ...[]graph.BackendRef, -) []graph.RouteRule { - rules := make([]graph.RouteRule, 0, len(refs)) +func refsToValidRules(backendRefs ...[]graph.BackendRef) []graph.RouteRule { + rules := make([]graph.RouteRule, 0, len(backendRefs)) - for _, ref := range refs { + for _, ref := range backendRefs { rules = append(rules, graph.RouteRule{ - ValidMatches: true, - Filters: graph.RouteRuleFilters{Valid: true}, - BackendRefs: ref, - SessionPersistence: sessionPersistence, + ValidMatches: true, + Filters: graph.RouteRuleFilters{Valid: true}, + BackendRefs: ref, }) } @@ -2985,95 +2981,84 @@ func TestBuildUpstreams(t *testing.T) { }, } - createBackendRefs := func(serviceNames ...string) []graph.BackendRef { + createBackendRefs := func(sp *graph.SessionPersistenceConfig, serviceNames ...string) []graph.BackendRef { var backends []graph.BackendRef for _, name := range serviceNames { backends = append(backends, graph.BackendRef{ - SvcNsName: types.NamespacedName{Namespace: "test", Name: name}, - ServicePort: apiv1.ServicePort{Port: 80}, - Valid: name != "", + SvcNsName: types.NamespacedName{Namespace: "test", Name: name}, + ServicePort: apiv1.ServicePort{Port: 80}, + Valid: name != "", + SessionPersistence: sp, }) } return backends } - hr1Refs0 := createBackendRefs("foo", "bar") + createSPConfig := func(idx string) *graph.SessionPersistenceConfig { + return &graph.SessionPersistenceConfig{ + Name: "session-persistence", + SessionType: v1.CookieBasedSessionPersistence, + Expiry: "24h", + Path: "/", + Valid: true, + Idx: idx, + } + } + + hr1Refs0 := createBackendRefs(createSPConfig("foo-bar-sp"), "foo", "bar") - hr1Refs1 := createBackendRefs("baz", "", "") // empty service names should be ignored + hr1Refs1 := createBackendRefs(nil, "baz", "", "") // empty service names should be ignored - hr1Refs2 := createBackendRefs("invalid-for-gateway") + hr1Refs2 := createBackendRefs(nil, "invalid-for-gateway") hr1Refs2[0].InvalidForGateways = map[types.NamespacedName]conditions.Condition{ {Namespace: "test", Name: "gateway"}: {}, } - hr2Refs0 := createBackendRefs("foo", "baz") // shouldn't duplicate foo and baz upstream + // should duplicate foo upstream because it has a different SP config + hr2Refs0 := createBackendRefs(createSPConfig("foo-baz-sp"), "foo", "baz") - hr2Refs1 := createBackendRefs("nil-endpoints") + hr2Refs1 := createBackendRefs(nil, "nil-endpoints") - hr3Refs0 := createBackendRefs("baz") // shouldn't duplicate baz upstream + hr3Refs0 := createBackendRefs(nil, "baz") // shouldn't duplicate baz upstream - hr4Refs0 := createBackendRefs("empty-endpoints", "") + hr4Refs0 := createBackendRefs(nil, "empty-endpoints", "") - hr4Refs1 := createBackendRefs("baz2") + hr4Refs1 := createBackendRefs(nil, "baz2") - hr5Refs0 := createBackendRefs("ipv6-endpoints") + hr5Refs0 := createBackendRefs(nil, "ipv6-endpoints") - nonExistingRefs := createBackendRefs("non-existing") + nonExistingRefs := createBackendRefs(nil, "non-existing") - invalidHRRefs := createBackendRefs("abc") + invalidHRRefs := createBackendRefs(nil, "abc") - refsWithPolicies := createBackendRefs("policies") - - mirrorRefs := createBackendRefs("mirror-service") - mirrorRefs[0].IsMirrorBackend = true - - inferenceRefs := createBackendRefs("inference-service") - inferenceRefs[0].IsInferencePool = true - - mixedRefs := []graph.BackendRef{ - { - SvcNsName: types.NamespacedName{Namespace: "test", Name: "regular-service"}, - ServicePort: apiv1.ServicePort{Port: 80}, - Valid: true, - }, - { - SvcNsName: types.NamespacedName{Namespace: "test", Name: "mirror-service"}, - ServicePort: apiv1.ServicePort{Port: 80}, - Valid: true, - IsMirrorBackend: true, - }, - { - SvcNsName: types.NamespacedName{Namespace: "test", Name: "inference-service"}, - ServicePort: apiv1.ServicePort{Port: 80}, - Valid: true, - IsInferencePool: true, - }, - } + refsWithPolicies := createBackendRefs(createSPConfig("policies-sp"), "policies") - spConfig := &graph.SessionPersistenceConfig{ - Name: "multiple-backends-sp", - SessionType: v1.CookieBasedSessionPersistence, - Expiry: "24h", - Path: "/", + getExpectedConfig := func() SessionPersistenceConfig { + return SessionPersistenceConfig{ + Name: "session-persistence", + SessionType: CookieBasedSessionPersistence, + Expiry: "24h", + Path: "/", + } } routes := map[graph.RouteKey]*graph.L7Route{ {NamespacedName: types.NamespacedName{Name: "hr1", Namespace: "test"}}: { Valid: true, Spec: graph.L7RouteSpec{ - Rules: refsToValidRules(spConfig, hr1Refs0, hr1Refs1, hr1Refs2), + Rules: refsToValidRules(hr1Refs0, hr1Refs1, hr1Refs2), }, }, {NamespacedName: types.NamespacedName{Name: "hr2", Namespace: "test"}}: { Valid: true, Spec: graph.L7RouteSpec{ - Rules: refsToValidRules(nil, hr2Refs0, hr2Refs1), + Rules: refsToValidRules(hr2Refs0, hr2Refs1), }, }, {NamespacedName: types.NamespacedName{Name: "hr3", Namespace: "test"}}: { Valid: true, Spec: graph.L7RouteSpec{ - Rules: refsToValidRules(spConfig, hr3Refs0), + Rules: refsToValidRules(hr3Refs0), }, }, } @@ -3082,7 +3067,7 @@ func TestBuildUpstreams(t *testing.T) { {NamespacedName: types.NamespacedName{Name: "hr4", Namespace: "test"}}: { Valid: true, Spec: graph.L7RouteSpec{ - Rules: refsToValidRules(nil, hr4Refs0, hr4Refs1), + Rules: refsToValidRules(hr4Refs0, hr4Refs1), }, }, } @@ -3091,7 +3076,7 @@ func TestBuildUpstreams(t *testing.T) { {NamespacedName: types.NamespacedName{Name: "hr4", Namespace: "test"}}: { Valid: true, Spec: graph.L7RouteSpec{ - Rules: refsToValidRules(nil, hr5Refs0, hr2Refs1), + Rules: refsToValidRules(hr5Refs0, hr2Refs1), }, }, } @@ -3100,7 +3085,7 @@ func TestBuildUpstreams(t *testing.T) { {NamespacedName: types.NamespacedName{Name: "non-existing", Namespace: "test"}}: { Valid: true, Spec: graph.L7RouteSpec{ - Rules: refsToValidRules(spConfig, nonExistingRefs), + Rules: refsToValidRules(nonExistingRefs), }, }, } @@ -3109,7 +3094,7 @@ func TestBuildUpstreams(t *testing.T) { {NamespacedName: types.NamespacedName{Name: "invalid", Namespace: "test"}}: { Valid: false, Spec: graph.L7RouteSpec{ - Rules: refsToValidRules(nil, invalidHRRefs), + Rules: refsToValidRules(invalidHRRefs), }, }, } @@ -3118,28 +3103,7 @@ func TestBuildUpstreams(t *testing.T) { {NamespacedName: types.NamespacedName{Name: "policies", Namespace: "test"}}: { Valid: true, Spec: graph.L7RouteSpec{ - Rules: refsToValidRules(spConfig, refsWithPolicies), - }, - }, - } - - routesWithMirrorAndInference := map[graph.RouteKey]*graph.L7Route{ - {NamespacedName: types.NamespacedName{Name: "hr-mirror", Namespace: "test"}}: { - Valid: true, - Spec: graph.L7RouteSpec{ - Rules: refsToValidRules(spConfig, mirrorRefs), - }, - }, - {NamespacedName: types.NamespacedName{Name: "hr-inference", Namespace: "test"}}: { - Valid: true, - Spec: graph.L7RouteSpec{ - Rules: refsToValidRules(spConfig, inferenceRefs), - }, - }, - {NamespacedName: types.NamespacedName{Name: "hr-mixed", Namespace: "test"}}: { - Valid: true, - Spec: graph.L7RouteSpec{ - Rules: refsToValidRules(spConfig, mixedRefs), + Rules: refsToValidRules(refsWithPolicies), }, }, } @@ -3182,11 +3146,6 @@ func TestBuildUpstreams(t *testing.T) { Valid: true, Routes: routesWithPolicies, }, - { - Name: "listener-6", - Valid: true, - Routes: routesWithMirrorAndInference, - }, }, } @@ -3219,26 +3178,16 @@ func TestBuildUpstreams(t *testing.T) { }, }, }, - {Name: "mirror-service", Namespace: "test"}: {}, - {Name: "inference-service", Namespace: "test"}: {}, - {Name: "regular-service", Namespace: "test"}: {}, } emptyEndpointsErrMsg := "empty endpoints error" nilEndpointsErrMsg := "nil endpoints error" - expectedSPConfig := SessionPersistenceConfig{ - Name: "multiple-backends-sp", - SessionType: SessionPersistenceCookie, - Expiry: "24h", - Path: "/", - } - expUpstreams := []Upstream{ { - Name: "test_bar_80", + Name: "test_bar_80_foo-bar-sp", Endpoints: barEndpoints, - SessionPersistence: expectedSPConfig, + SessionPersistence: getExpectedConfig(), }, { Name: "test_baz2_80", @@ -3248,7 +3197,12 @@ func TestBuildUpstreams(t *testing.T) { { Name: "test_baz_80", Endpoints: bazEndpoints, - SessionPersistence: expectedSPConfig, + SessionPersistence: SessionPersistenceConfig{}, + }, + { + Name: "test_baz_80_foo-baz-sp", + Endpoints: bazEndpoints, + SessionPersistence: getExpectedConfig(), }, { Name: "test_empty-endpoints_80", @@ -3257,14 +3211,14 @@ func TestBuildUpstreams(t *testing.T) { SessionPersistence: SessionPersistenceConfig{}, }, { - Name: "test_foo_80", + Name: "test_foo_80_foo-bar-sp", Endpoints: fooEndpoints, - SessionPersistence: expectedSPConfig, + SessionPersistence: getExpectedConfig(), }, { - Name: "test_nil-endpoints_80", - Endpoints: nil, - ErrorMsg: nilEndpointsErrMsg, + Name: "test_foo_80_foo-baz-sp", + Endpoints: fooEndpoints, + SessionPersistence: getExpectedConfig(), }, { Name: "test_ipv6-endpoints_80", @@ -3272,25 +3226,16 @@ func TestBuildUpstreams(t *testing.T) { SessionPersistence: SessionPersistenceConfig{}, }, { - Name: "test_policies_80", - Endpoints: policyEndpoints, - Policies: []policies.Policy{validPolicy1, validPolicy2}, - SessionPersistence: expectedSPConfig, - }, - { - Name: "test_mirror-service_80", - Endpoints: []resolver.Endpoint{{Address: "17.0.0.0", Port: 80}}, - SessionPersistence: SessionPersistenceConfig{}, - }, - { - Name: "test_inference-service_80", - Endpoints: []resolver.Endpoint{{Address: "18.0.0.0", Port: 80}}, - SessionPersistence: SessionPersistenceConfig{}, + Name: "test_nil-endpoints_80", + Endpoints: nil, + ErrorMsg: nilEndpointsErrMsg, }, + { - Name: "test_regular-service_80", - Endpoints: []resolver.Endpoint{{Address: "19.0.0.0", Port: 80}}, - SessionPersistence: expectedSPConfig, + Name: "test_policies_80_policies-sp", + Endpoints: policyEndpoints, + Policies: []policies.Policy{validPolicy1, validPolicy2}, + SessionPersistence: getExpectedConfig(), }, } @@ -3323,12 +3268,6 @@ func TestBuildUpstreams(t *testing.T) { return ipv6Endpoints, nil case "policies": return policyEndpoints, nil - case "mirror-service": - return []resolver.Endpoint{{Address: "17.0.0.0", Port: 80}}, nil - case "inference-service": - return []resolver.Endpoint{{Address: "18.0.0.0", Port: 80}}, nil - case "regular-service": - return []resolver.Endpoint{{Address: "19.0.0.0", Port: 80}}, nil default: return nil, fmt.Errorf("unexpected service %s", svcNsName.Name) } diff --git a/internal/controller/state/dataplane/types.go b/internal/controller/state/dataplane/types.go index 2068197f2a..e6cb43f299 100644 --- a/internal/controller/state/dataplane/types.go +++ b/internal/controller/state/dataplane/types.go @@ -136,11 +136,12 @@ type SessionPersistenceConfig struct { Path string } +// SessionPersistenceType is the type of session persistence. type SessionPersistenceType string const ( - // SessionPersistenceCookie indicates cookie-based session persistence. - SessionPersistenceCookie SessionPersistenceType = "cookie" + // CookieBasedSessionPersistence indicates cookie-based session persistence. + CookieBasedSessionPersistence SessionPersistenceType = "cookie" ) // SSL is the SSL configuration for a server. diff --git a/internal/controller/state/graph/backend_refs.go b/internal/controller/state/graph/backend_refs.go index 24da9013a4..740c3b6b76 100644 --- a/internal/controller/state/graph/backend_refs.go +++ b/internal/controller/state/graph/backend_refs.go @@ -35,6 +35,8 @@ type BackendRef struct { // condition. Certain NginxProxy configurations may result in a backend not being valid for some Gateways, // but not others. InvalidForGateways map[types.NamespacedName]conditions.Condition + // SessionPersistence is the SessionPersistenceConfig of the backendRef. + SessionPersistence *SessionPersistenceConfig // SvcNsName is the NamespacedName of the Service referenced by the backendRef. SvcNsName types.NamespacedName // ServicePort is the ServicePort of the Service which is referenced by the backendRef. @@ -55,7 +57,16 @@ func (b BackendRef) ServicePortReference() string { if !b.Valid { return "" } - return fmt.Sprintf("%s_%s_%d", b.SvcNsName.Namespace, b.SvcNsName.Name, b.ServicePort.Port) + return b.buildUpstreamNameForBackendRef() +} + +func (b BackendRef) buildUpstreamNameForBackendRef() string { + base := fmt.Sprintf("%s_%s_%d", b.SvcNsName.Namespace, b.SvcNsName.Name, b.ServicePort.Port) + if b.SessionPersistence == nil || b.SessionPersistence.Name == "" { + return base + } + + return fmt.Sprintf("%s_%s", base, b.SessionPersistence.Idx) } func addBackendRefsToRouteRules( @@ -314,6 +325,7 @@ func createBackendRef( IsInferencePool: ref.IsInferencePool, InvalidForGateways: invalidForGateways, EndpointPickerConfig: ref.EndpointPickerConfig, + SessionPersistence: ref.SessionPersistence, } return backendRef, conds diff --git a/internal/controller/state/graph/backend_refs_test.go b/internal/controller/state/graph/backend_refs_test.go index d45db3be12..d9b9604994 100644 --- a/internal/controller/state/graph/backend_refs_test.go +++ b/internal/controller/state/graph/backend_refs_test.go @@ -1410,6 +1410,14 @@ func TestCreateBackend(t *testing.T) { }, } + expectedSPConfig := SessionPersistenceConfig{ + Name: "test-persistence", + Idx: "test-persistence-idx", + Valid: true, + Expiry: "10m", + Path: "/test-path", + } + tests := []struct { nginxProxySpec *EffectiveNginxProxy name string @@ -1428,8 +1436,9 @@ func TestCreateBackend(t *testing.T) { Weight: 5, Valid: true, InvalidForGateways: map[types.NamespacedName]conditions.Condition{}, + SessionPersistence: &expectedSPConfig, }, - expectedServicePortReference: "test_service1_80", + expectedServicePortReference: "test_service1_80_test-persistence-idx", expectedConditions: nil, name: "normal case", }, @@ -1446,8 +1455,9 @@ func TestCreateBackend(t *testing.T) { Weight: 1, Valid: true, InvalidForGateways: map[types.NamespacedName]conditions.Condition{}, + SessionPersistence: &expectedSPConfig, }, - expectedServicePortReference: "test_service1_80", + expectedServicePortReference: "test_service1_80_test-persistence-idx", expectedConditions: nil, name: "normal with nil weight", }, @@ -1531,8 +1541,9 @@ func TestCreateBackend(t *testing.T) { `The Service configured with IPv4 family but NginxProxy is configured with IPv6`, ), }, + SessionPersistence: &expectedSPConfig, }, - expectedServicePortReference: "test_service1_80", + expectedServicePortReference: "test_service1_80_test-persistence-idx", nginxProxySpec: &EffectiveNginxProxy{IPFamily: helpers.GetPointer(ngfAPIv1alpha2.IPv6)}, expectedConditions: nil, name: "service IPFamily doesn't match NginxProxy IPFamily", @@ -1551,8 +1562,9 @@ func TestCreateBackend(t *testing.T) { Valid: true, BackendTLSPolicy: &btp, InvalidForGateways: map[types.NamespacedName]conditions.Condition{}, + SessionPersistence: &expectedSPConfig, }, - expectedServicePortReference: "test_service2_80", + expectedServicePortReference: "test_service2_80_test-persistence-idx", expectedConditions: nil, name: "normal case with policy", }, @@ -1595,8 +1607,9 @@ func TestCreateBackend(t *testing.T) { "ExternalName service requires DNS resolver configuration in Gateway's NginxProxy", ), }, + SessionPersistence: &expectedSPConfig, }, - expectedServicePortReference: "test_external-service_80", + expectedServicePortReference: "test_external-service_80_test-persistence-idx", expectedConditions: nil, name: "ExternalName service without DNS resolver", }, @@ -1620,8 +1633,9 @@ func TestCreateBackend(t *testing.T) { Weight: 5, Valid: true, InvalidForGateways: map[types.NamespacedName]conditions.Condition{}, + SessionPersistence: &expectedSPConfig, }, - expectedServicePortReference: "test_external-service_80", + expectedServicePortReference: "test_external-service_80_test-persistence-idx", expectedConditions: nil, name: "ExternalName service with DNS resolver", }, @@ -1642,8 +1656,9 @@ func TestCreateBackend(t *testing.T) { "ExternalName service requires DNS resolver configuration in Gateway's NginxProxy", ), }, + SessionPersistence: &expectedSPConfig, }, - expectedServicePortReference: "test_external-service_80", + expectedServicePortReference: "test_external-service_80_test-persistence-idx", expectedConditions: nil, name: "ExternalName service with multiple gateways - mixed DNS resolver config", }, @@ -1728,6 +1743,13 @@ func TestCreateBackend(t *testing.T) { IsInferencePool: false, BackendRef: test.ref.BackendRef, Filters: []any{}, + SessionPersistence: &SessionPersistenceConfig{ + Name: "test-persistence", + Idx: "test-persistence-idx", + Valid: true, + Expiry: "10m", + Path: "/test-path", + }, } route := &L7Route{ RouteType: RouteTypeHTTP, diff --git a/internal/controller/state/graph/graph.go b/internal/controller/state/graph/graph.go index b46fc32126..c670ccc487 100644 --- a/internal/controller/state/graph/graph.go +++ b/internal/controller/state/graph/graph.go @@ -92,12 +92,12 @@ type NginxReloadResult struct { // ProtectedPorts are the ports that may not be configured by a listener with a descriptive name of each port. type ProtectedPorts map[int32]string -// flags hold the configuration flags for building the Graph. -type flags struct { - // plus indicates whether NGINX Plus features are enabled. - plus bool - // experimental indicates whether experimental features are enabled. - experimental bool +// FeatureFlags hold the feature flags for building the Graph. +type FeatureFlags struct { + // Plus indicates whether NGINX Plus features are enabled. + Plus bool + // Experimental indicates whether experimental features are enabled. + Experimental bool } // IsReferenced returns true if the Graph references the resource. @@ -216,7 +216,7 @@ func BuildGraph( plusSecrets map[types.NamespacedName][]PlusSecretFile, validators validation.Validators, logger logr.Logger, - experimentalEnabled bool, + featureFlags FeatureFlags, ) *Graph { processedGwClasses, gcExists := processGatewayClasses(state.GatewayClasses, gcName, controllerName) if gcExists && processedGwClasses.Winner == nil { @@ -236,7 +236,7 @@ func BuildGraph( processedGwClasses.Winner, processedNginxProxies, state.CRDMetadata, - experimentalEnabled, + featureFlags.Experimental, ) secretResolver := newSecretResolver(state.Secrets) @@ -250,7 +250,7 @@ func BuildGraph( gc, refGrantResolver, processedNginxProxies, - experimentalEnabled, + featureFlags.Experimental, ) processedBackendTLSPolicies := processBackendTLSPolicies( @@ -262,8 +262,6 @@ func BuildGraph( processedSnippetsFilters := processSnippetsFilters(state.SnippetsFilters) - // secrets map only gets populated if plus is enabled - plusEnabled := len(plusSecrets) > 0 routes := buildRoutesForGateways( validators.HTTPFieldsValidator, state.HTTPRoutes, @@ -271,7 +269,7 @@ func BuildGraph( gws, processedSnippetsFilters, state.InferencePools, - flags{plus: plusEnabled, experimental: experimentalEnabled}, + featureFlags, ) referencedInferencePools := buildReferencedInferencePools(routes, gws, state.InferencePools, state.Services) diff --git a/internal/controller/state/graph/graph_test.go b/internal/controller/state/graph/graph_test.go index e9b6f5633b..4c0111fdda 100644 --- a/internal/controller/state/graph/graph_test.go +++ b/internal/controller/state/graph/graph_test.go @@ -165,7 +165,10 @@ func TestBuildGraph(t *testing.T) { }, } - createValidRuleWithBackendRefs := func(matches []gatewayv1.HTTPRouteMatch) RouteRule { + createValidRuleWithBackendRefs := func( + matches []gatewayv1.HTTPRouteMatch, + sessionPersistence *SessionPersistenceConfig, + ) RouteRule { refs := []BackendRef{ { SvcNsName: types.NamespacedName{Namespace: "service", Name: "foo"}, @@ -174,11 +177,13 @@ func TestBuildGraph(t *testing.T) { Weight: 1, BackendTLSPolicy: &btp, InvalidForGateways: map[types.NamespacedName]conditions.Condition{}, + SessionPersistence: sessionPersistence, }, } rbrs := []RouteBackendRef{ { - BackendRef: commonGWBackendRef, + BackendRef: commonGWBackendRef, + SessionPersistence: sessionPersistence, }, } return RouteRule{ @@ -198,7 +203,7 @@ func TestBuildGraph(t *testing.T) { routeType RouteType, sessionPersistence *SessionPersistenceConfig, ) RouteRule { - rule := createValidRuleWithBackendRefs(matches) + rule := createValidRuleWithBackendRefs(matches, sessionPersistence) rule.Filters = RouteRuleFilters{ Filters: []Filter{ { @@ -214,10 +219,6 @@ func TestBuildGraph(t *testing.T) { Valid: true, } - if sessionPersistence != nil { - rule.SessionPersistence = sessionPersistence - } - return rule } @@ -880,13 +881,15 @@ func TestBuildGraph(t *testing.T) { } } - expectedSP := &SessionPersistenceConfig{ + getExpectedSPConfig := &SessionPersistenceConfig{ Name: "session-persistence-httproute", SessionType: gatewayv1.CookieBasedSessionPersistence, Expiry: "30m", Valid: true, Path: "/", + Idx: "hr-1_test_0", } + routeHR1 := &L7Route{ RouteType: RouteTypeHTTP, Valid: true, @@ -914,7 +917,7 @@ func TestBuildGraph(t *testing.T) { }, Spec: L7RouteSpec{ Hostnames: hr1.Spec.Hostnames, - Rules: []RouteRule{createValidRuleWithBackendRefsAndFilters(routeMatches, RouteTypeHTTP, expectedSP)}, + Rules: []RouteRule{createValidRuleWithBackendRefsAndFilters(routeMatches, RouteTypeHTTP, getExpectedSPConfig)}, }, Policies: []*Policy{processedRoutePolicy}, Conditions: []conditions.Condition{ @@ -1083,6 +1086,7 @@ func TestBuildGraph(t *testing.T) { SessionType: gatewayv1.CookieBasedSessionPersistence, Expiry: "30m", Valid: true, + Idx: "gr_test_0", } routeGR := &L7Route{ RouteType: RouteTypeGRPC, @@ -1144,7 +1148,7 @@ func TestBuildGraph(t *testing.T) { }, Spec: L7RouteSpec{ Hostnames: hr3.Spec.Hostnames, - Rules: []RouteRule{createValidRuleWithBackendRefs(routeMatches)}, + Rules: []RouteRule{createValidRuleWithBackendRefs(routeMatches, nil)}, }, } @@ -1483,15 +1487,16 @@ func TestBuildGraph(t *testing.T) { } tests := []struct { - store ClusterState - expected *Graph - name string - experimentalEnabled bool + store ClusterState + expected *Graph + name string + plus, experimentalEnabled bool }{ { store: createStateWithGatewayClass(normalGC), expected: createExpectedGraphWithGatewayClass(normalGC), experimentalEnabled: true, + plus: true, name: "normal case", }, { @@ -1534,7 +1539,10 @@ func TestBuildGraph(t *testing.T) { PolicyValidator: fakePolicyValidator, }, logr.Discard(), - test.experimentalEnabled, + FeatureFlags{ + Experimental: test.experimentalEnabled, + Plus: test.plus, + }, ) g.Expect(helpers.Diff(test.expected, result)).To(BeEmpty()) diff --git a/internal/controller/state/graph/grpcroute.go b/internal/controller/state/graph/grpcroute.go index 022193efa4..15611f2a8b 100644 --- a/internal/controller/state/graph/grpcroute.go +++ b/internal/controller/state/graph/grpcroute.go @@ -21,7 +21,7 @@ func buildGRPCRoute( ghr *v1.GRPCRoute, gws map[types.NamespacedName]*Gateway, snippetsFilters map[types.NamespacedName]*SnippetsFilter, - featureFlags flags, + featureFlags FeatureFlags, ) *L7Route { r := &L7Route{ Source: ghr, @@ -53,10 +53,15 @@ func buildGRPCRoute( r.Spec.Hostnames = ghr.Spec.Hostnames r.Attachable = true + grpcRouteNsName := types.NamespacedName{ + Namespace: ghr.GetNamespace(), + Name: ghr.GetName(), + } rules, valid, conds := processGRPCRouteRules( ghr.Spec.Rules, validator, getSnippetsFilterResolverForNamespace(snippetsFilters, r.Source.GetNamespace()), + grpcRouteNsName, featureFlags, ) @@ -73,7 +78,7 @@ func buildGRPCMirrorRoutes( route *v1.GRPCRoute, gateways map[types.NamespacedName]*Gateway, snippetsFilters map[types.NamespacedName]*SnippetsFilter, - featureFlags flags, + featureFlags FeatureFlags, ) { for idx, rule := range l7route.Spec.Rules { if rule.Filters.Valid { @@ -161,15 +166,16 @@ func removeGRPCMirrorFilters(filters []v1.GRPCRouteFilter) []v1.GRPCRouteFilter func processGRPCRouteRule( specRule v1.GRPCRouteRule, - rulePath *field.Path, + ruleIdx int, validator validation.HTTPFieldsValidator, resolveExtRefFunc resolveExtRefFilter, - featureFlags flags, + grpcRouteNsName types.NamespacedName, + featureFlags FeatureFlags, ) (RouteRule, routeRuleErrors) { - var errors routeRuleErrors - + rulePath := field.NewPath("spec").Child("rules").Index(ruleIdx) validMatches := true + var errors routeRuleErrors unsupportedFieldsErrors := checkForUnsupportedGRPCFields(specRule, rulePath, featureFlags) if len(unsupportedFieldsErrors) > 0 { errors.warn = append(errors.warn, unsupportedFieldsErrors...) @@ -194,6 +200,22 @@ func processGRPCRouteRule( errors = errors.append(filterErrors) + var sp *SessionPersistenceConfig + if specRule.SessionPersistence != nil { + spConfig, spErrors := processSessionPersistenceConfig( + specRule.SessionPersistence, + specRule.Matches, + rulePath.Child("sessionPersistence"), + validator, + ) + errors = errors.append(spErrors) + + if spConfig != nil && spConfig.Valid { + spConfig.Idx = getSessionPersistenceKey(ruleIdx, grpcRouteNsName) + sp = spConfig + } + } + backendRefs := make([]RouteBackendRef, 0, len(specRule.BackendRefs)) // rule.BackendRefs are validated separately because of their special requirements @@ -206,8 +228,9 @@ func processGRPCRouteRule( } } rbr := RouteBackendRef{ - BackendRef: b.BackendRef, - Filters: interfaceFilters, + BackendRef: b.BackendRef, + Filters: interfaceFilters, + SessionPersistence: sp, } backendRefs = append(backendRefs, rbr) } @@ -228,27 +251,11 @@ func processGRPCRouteRule( } } - var sp *SessionPersistenceConfig - if specRule.SessionPersistence != nil { - spConfig, spErrors := processSessionPersistenceConfig( - specRule.SessionPersistence, - specRule.Matches, - rulePath.Child("sessionPersistence"), - validator, - ) - errors = errors.append(spErrors) - - if spConfig != nil && spConfig.Valid { - sp = spConfig - } - } - return RouteRule{ - ValidMatches: validMatches, - Matches: ConvertGRPCMatches(specRule.Matches), - Filters: routeFilters, - RouteBackendRefs: backendRefs, - SessionPersistence: sp, + ValidMatches: validMatches, + Matches: ConvertGRPCMatches(specRule.Matches), + Filters: routeFilters, + RouteBackendRefs: backendRefs, }, errors } @@ -256,7 +263,8 @@ func processGRPCRouteRules( specRules []v1.GRPCRouteRule, validator validation.HTTPFieldsValidator, resolveExtRefFunc resolveExtRefFilter, - featureFlags flags, + grpcRouteNsName types.NamespacedName, + featureFlags FeatureFlags, ) (rules []RouteRule, valid bool, conds []conditions.Condition) { rules = make([]RouteRule, len(specRules)) @@ -265,14 +273,13 @@ func processGRPCRouteRules( atLeastOneValid bool ) - for i, rule := range specRules { - rulePath := field.NewPath("spec").Child("rules").Index(i) - + for ruleIdx, rule := range specRules { rr, errors := processGRPCRouteRule( rule, - rulePath, + ruleIdx, validator, resolveExtRefFunc, + grpcRouteNsName, featureFlags, ) @@ -282,7 +289,7 @@ func processGRPCRouteRules( allRulesErrors = allRulesErrors.append(errors) - rules[i] = rr + rules[ruleIdx] = rr } conds = make([]conditions.Condition, 0, 2) @@ -293,11 +300,6 @@ func processGRPCRouteRules( conds = append(conds, conditions.NewRouteAcceptedUnsupportedField(allRulesErrors.warn.ToAggregate().Error())) } - spConflictErrors := handleSessionPersistenceConflicts(rules) - if spConflictErrors != nil { - conds = append(conds, conditions.NewRouteAcceptedInvalidSessionPersistenceConfiguration(spConflictErrors.Error())) - } - if len(allRulesErrors.invalid) > 0 { msg := allRulesErrors.invalid.ToAggregate().Error() @@ -483,7 +485,11 @@ func validateGRPCHeaderMatch( return allErrs } -func checkForUnsupportedGRPCFields(rule v1.GRPCRouteRule, rulePath *field.Path, featureFlags flags) field.ErrorList { +func checkForUnsupportedGRPCFields( + rule v1.GRPCRouteRule, + rulePath *field.Path, + featureFlags FeatureFlags, +) field.ErrorList { var ruleErrors field.ErrorList if rule.Name != nil { @@ -493,14 +499,14 @@ func checkForUnsupportedGRPCFields(rule v1.GRPCRouteRule, rulePath *field.Path, )) } - if !featureFlags.plus && rule.SessionPersistence != nil { + if !featureFlags.Plus && rule.SessionPersistence != nil { ruleErrors = append(ruleErrors, field.Forbidden( rulePath.Child("sessionPersistence"), "SessionPersistence is only supported in NGINX Plus. This configuration will be ignored.", )) } - if !featureFlags.experimental && rule.SessionPersistence != nil { + if !featureFlags.Experimental && rule.SessionPersistence != nil { ruleErrors = append(ruleErrors, field.Forbidden( rulePath.Child("sessionPersistence"), "SessionPersistence is only supported in experimental mode.", diff --git a/internal/controller/state/graph/grpcroute_test.go b/internal/controller/state/graph/grpcroute_test.go index 04f677e7ef..c94d650e62 100644 --- a/internal/controller/state/graph/grpcroute_test.go +++ b/internal/controller/state/graph/grpcroute_test.go @@ -54,7 +54,7 @@ func createGRPCMethodMatch( } } -func createGRPCHeadersMatch(headerType, headerName, headerValue string, sp *v1.SessionPersistence) v1.GRPCRouteRule { +func createGRPCHeadersMatch(headerType, headerName, headerValue string) v1.GRPCRouteRule { matches := []v1.GRPCRouteMatch{ { Headers: []v1.GRPCHeaderMatch{ @@ -67,13 +67,6 @@ func createGRPCHeadersMatch(headerType, headerName, headerValue string, sp *v1.S }, } - if sp != nil { - return v1.GRPCRouteRule{ - Matches: matches, - SessionPersistence: sp, - } - } - return v1.GRPCRouteRule{ Matches: matches, } @@ -148,9 +141,21 @@ func TestBuildGRPCRoutes(t *testing.T) { }, } + grpcBackendRef := v1.GRPCBackendRef{ + BackendRef: v1.BackendRef{ + BackendObjectReference: v1.BackendObjectReference{ + Kind: helpers.GetPointer[v1.Kind]("Service"), + Name: "grpc-service", + Namespace: helpers.GetPointer[v1.Namespace]("test"), + Port: helpers.GetPointer[v1.PortNumber](80), + }, + }, + } + grRuleWithFiltersAndSessionPersistence := v1.GRPCRouteRule{ Filters: []v1.GRPCRouteFilter{snippetsFilterRef, requestHeaderFilter}, SessionPersistence: &sessionPersistenceConfig, + BackendRefs: []v1.GRPCBackendRef{grpcBackendRef}, } gr := createGRPCRoute("gr-1", gwNsName.Name, "example.com", []v1.GRPCRouteRule{grRuleWithFiltersAndSessionPersistence}) @@ -228,14 +233,21 @@ func TestBuildGRPCRoutes(t *testing.T) { }, }, }, - SessionPersistence: &SessionPersistenceConfig{ - Valid: true, - Name: *sessionPersistenceConfig.SessionName, - SessionType: *sessionPersistenceConfig.Type, - Expiry: "10m", + ValidMatches: true, + RouteBackendRefs: []RouteBackendRef{ + { + BackendRef: v1.BackendRef{ + BackendObjectReference: grpcBackendRef.BackendObjectReference, + }, + SessionPersistence: &SessionPersistenceConfig{ + Valid: true, + Name: *sessionPersistenceConfig.SessionName, + SessionType: *sessionPersistenceConfig.Type, + Expiry: "10m", + Idx: "gr-1_test_0", + }, + }, }, - ValidMatches: true, - RouteBackendRefs: []RouteBackendRef{}, }, }, }, @@ -277,9 +289,9 @@ func TestBuildGRPCRoutes(t *testing.T) { test.gateways, snippetsFilters, nil, - flags{ - plus: true, - experimental: true, + FeatureFlags{ + Plus: true, + Experimental: true, }, ) g.Expect(helpers.Diff(test.expected, routes)).To(BeEmpty()) @@ -303,6 +315,18 @@ func TestBuildGRPCRoute(t *testing.T) { }, } gatewayNsName := client.ObjectKeyFromObject(gw.Source) + backendRef := v1.BackendRef{ + BackendObjectReference: v1.BackendObjectReference{ + Kind: helpers.GetPointer[v1.Kind]("Service"), + Name: "service1", + Namespace: helpers.GetPointer[v1.Namespace]("test"), + Port: helpers.GetPointer[v1.PortNumber](80), + }, + } + + grpcBackendRef := v1.GRPCBackendRef{ + BackendRef: backendRef, + } spMethod := &v1.SessionPersistence{ SessionName: helpers.GetPointer("grpc-method-session"), @@ -313,22 +337,19 @@ func TestBuildGRPCRoute(t *testing.T) { }, } - spHeader := &v1.SessionPersistence{ - SessionName: helpers.GetPointer("grpc-header-session"), - AbsoluteTimeout: helpers.GetPointer(v1.Duration("10h")), - Type: helpers.GetPointer(v1.CookieBasedSessionPersistence), - CookieConfig: &v1.CookieConfig{ - LifetimeType: helpers.GetPointer((v1.SessionCookieLifetimeType)), - }, - } - - methodMatchRule := createGRPCMethodMatch("myService", "myMethod", "Exact", spMethod, nil) - headersMatchRule := createGRPCHeadersMatch("Exact", "MyHeader", "SomeValue", spHeader) + methodMatchRule := createGRPCMethodMatch( + "myService", + "myMethod", + "Exact", + spMethod, + []v1.GRPCBackendRef{grpcBackendRef}, + ) + headersMatchRule := createGRPCHeadersMatch("Exact", "MyHeader", "SomeValue") methodMatchEmptyFields := createGRPCMethodMatch("", "", "", nil, nil) methodMatchInvalidFields := createGRPCMethodMatch("service{}", "method{}", "Exact", nil, nil) methodMatchNilType := createGRPCMethodMatch("myService", "myMethod", "nilType", nil, nil) - headersMatchInvalid := createGRPCHeadersMatch("", "MyHeader", "SomeValue", nil) + headersMatchInvalid := createGRPCHeadersMatch("", "MyHeader", "SomeValue") headersMatchEmptyType := v1.GRPCRouteRule{ Matches: []v1.GRPCRouteMatch{ @@ -350,19 +371,6 @@ func TestBuildGRPCRoute(t *testing.T) { []v1.GRPCRouteRule{methodMatchRule, headersMatchRule}, ) - backendRef := v1.BackendRef{ - BackendObjectReference: v1.BackendObjectReference{ - Kind: helpers.GetPointer[v1.Kind]("Service"), - Name: "service1", - Namespace: helpers.GetPointer[v1.Namespace]("test"), - Port: helpers.GetPointer[v1.PortNumber](80), - }, - } - - grpcBackendRef := v1.GRPCBackendRef{ - BackendRef: backendRef, - } - grEmptyMatch := createGRPCRoute( "gr-1", gatewayNsName.Name, @@ -478,7 +486,7 @@ func TestBuildGRPCRoute(t *testing.T) { ) grValidFilterRule := createGRPCMethodMatch("myService", "myMethod", "Exact", nil, nil) - grValidHeaderMatch := createGRPCHeadersMatch("RegularExpression", "MyHeader", "headers-[a-z]+", nil) + grValidHeaderMatch := createGRPCHeadersMatch("RegularExpression", "MyHeader", "headers-[a-z]+") validSnippetsFilterRef := &v1.LocalObjectReference{ Group: ngfAPIv1alpha1.GroupName, Kind: kinds.SnippetsFilter, @@ -624,65 +632,6 @@ func TestBuildGRPCRoute(t *testing.T) { }, } - backendRefOne := []v1.GRPCBackendRef{ - { - BackendRef: v1.BackendRef{ - BackendObjectReference: v1.BackendObjectReference{ - Kind: helpers.GetPointer[v1.Kind]("Service"), - Name: "service-one", - Namespace: helpers.GetPointer[v1.Namespace]("test"), - Port: helpers.GetPointer[v1.PortNumber](80), - }, - }, - }, - } - - backendRefTwo := []v1.GRPCBackendRef{ - { - BackendRef: v1.BackendRef{ - BackendObjectReference: v1.BackendObjectReference{ - Kind: helpers.GetPointer[v1.Kind]("Service"), - Name: "service-two", - Namespace: helpers.GetPointer[v1.Namespace]("test"), - Port: helpers.GetPointer[v1.PortNumber](80), - }, - }, - }, - } - - spDifferentSameBackend := &v1.SessionPersistence{ - SessionName: helpers.GetPointer("grpc-method-session-diff"), - AbsoluteTimeout: helpers.GetPointer(v1.Duration("5h")), - Type: helpers.GetPointer(v1.CookieBasedSessionPersistence), - CookieConfig: &v1.CookieConfig{ - LifetimeType: helpers.GetPointer((v1.PermanentCookieLifetimeType)), - }, - } - - methodMatchWithSP := createGRPCMethodMatch("myService", "myMethod", "Exact", spMethod, backendRefOne) - methodMatchWithSameSPSameBackend := createGRPCMethodMatch("myService", "myMethod", "Exact", spMethod, backendRefOne) - - methodMatchWithPSameBackend := createGRPCMethodMatch( - "myService", - "myMethod", - "Exact", - spDifferentSameBackend, - backendRefTwo, - ) - methodMatchWithDiffSPSameBackend := createGRPCMethodMatch("myService", "myMethod", "Exact", spMethod, backendRefTwo) - - grpcRouteSPConflict := createGRPCRoute( - "gr-same-backend", - gatewayNsName.Name, - "example.com", - []v1.GRPCRouteRule{ - methodMatchWithSP, - methodMatchWithSameSPSameBackend, - methodMatchWithPSameBackend, - methodMatchWithDiffSPSameBackend, - }, - ) - durationSP := v1.Duration("10h") tests := []struct { validator *validationfakes.FakeHTTPFieldsValidator @@ -715,13 +664,18 @@ func TestBuildGRPCRoute(t *testing.T) { Valid: true, Filters: []Filter{}, }, - Matches: ConvertGRPCMatches(grBoth.Spec.Rules[0].Matches), - RouteBackendRefs: []RouteBackendRef{}, - SessionPersistence: &SessionPersistenceConfig{ - Valid: true, - Name: "grpc-method-session", - SessionType: v1.CookieBasedSessionPersistence, - Expiry: "10h", + Matches: ConvertGRPCMatches(grBoth.Spec.Rules[0].Matches), + RouteBackendRefs: []RouteBackendRef{ + { + BackendRef: grpcBackendRef.BackendRef, + SessionPersistence: &SessionPersistenceConfig{ + Valid: true, + Name: "grpc-method-session", + SessionType: v1.CookieBasedSessionPersistence, + Expiry: "10h", + Idx: "gr-1_test_0", + }, + }, }, }, { @@ -732,11 +686,6 @@ func TestBuildGRPCRoute(t *testing.T) { }, Matches: ConvertGRPCMatches(grBoth.Spec.Rules[1].Matches), RouteBackendRefs: []RouteBackendRef{}, - SessionPersistence: &SessionPersistenceConfig{ - Valid: true, - Name: "grpc-header-session", - SessionType: v1.CookieBasedSessionPersistence, - }, }, }, }, @@ -939,13 +888,18 @@ func TestBuildGRPCRoute(t *testing.T) { Valid: true, Filters: []Filter{}, }, - Matches: ConvertGRPCMatches(grOneInvalid.Spec.Rules[0].Matches), - RouteBackendRefs: []RouteBackendRef{}, - SessionPersistence: &SessionPersistenceConfig{ - Valid: true, - Name: "grpc-method-session", - SessionType: v1.CookieBasedSessionPersistence, - Expiry: "10h", + Matches: ConvertGRPCMatches(grOneInvalid.Spec.Rules[0].Matches), + RouteBackendRefs: []RouteBackendRef{ + { + BackendRef: grpcBackendRef.BackendRef, + SessionPersistence: &SessionPersistenceConfig{ + Valid: true, + Name: "grpc-method-session", + SessionType: v1.CookieBasedSessionPersistence, + Expiry: "10h", + Idx: "gr-1_test_0", + }, + }, }, }, { @@ -1339,126 +1293,6 @@ func TestBuildGRPCRoute(t *testing.T) { }, name: "invalid route with unsupported field", }, - { - validator: createAllValidValidator(nil), - gr: grpcRouteSPConflict, - expected: &L7Route{ - RouteType: RouteTypeGRPC, - Source: grpcRouteSPConflict, - ParentRefs: []ParentRef{ - { - Idx: 0, - Gateway: CreateParentRefGateway(gw), - SectionName: grpcRouteSPConflict.Spec.ParentRefs[0].SectionName, - }, - }, - Valid: true, - Attachable: true, - Spec: L7RouteSpec{ - Hostnames: grpcRouteSPConflict.Spec.Hostnames, - Rules: []RouteRule{ - { - ValidMatches: true, - Filters: RouteRuleFilters{ - Valid: true, - Filters: []Filter{}, - }, - Matches: ConvertGRPCMatches(grpcRouteSPConflict.Spec.Rules[0].Matches), - RouteBackendRefs: []RouteBackendRef{ - { - BackendRef: v1.BackendRef{ - BackendObjectReference: v1.BackendObjectReference{ - Name: "service-one", - Kind: helpers.GetPointer(v1.Kind("Service")), - Namespace: helpers.GetPointer(v1.Namespace("test")), - Port: helpers.GetPointer(v1.PortNumber(80)), - }, - }, - }, - }, - SessionPersistence: &SessionPersistenceConfig{ - Valid: true, - Name: "grpc-method-session", - SessionType: v1.CookieBasedSessionPersistence, - }, - }, - { - ValidMatches: true, - Filters: RouteRuleFilters{ - Valid: true, - Filters: []Filter{}, - }, - Matches: ConvertGRPCMatches(grpcRouteSPConflict.Spec.Rules[1].Matches), - RouteBackendRefs: []RouteBackendRef{ - { - BackendRef: v1.BackendRef{ - BackendObjectReference: v1.BackendObjectReference{ - Name: "service-one", - Kind: helpers.GetPointer(v1.Kind("Service")), - Namespace: helpers.GetPointer(v1.Namespace("test")), - Port: helpers.GetPointer(v1.PortNumber(80)), - }, - }, - }, - }, - SessionPersistence: &SessionPersistenceConfig{ - Valid: true, - Name: "grpc-method-session", - SessionType: v1.CookieBasedSessionPersistence, - }, - }, - { - ValidMatches: true, - Filters: RouteRuleFilters{ - Valid: true, - Filters: []Filter{}, - }, - Matches: ConvertGRPCMatches(grpcRouteSPConflict.Spec.Rules[2].Matches), - RouteBackendRefs: []RouteBackendRef{ - { - BackendRef: v1.BackendRef{ - BackendObjectReference: v1.BackendObjectReference{ - Name: "service-two", - Kind: helpers.GetPointer(v1.Kind("Service")), - Namespace: helpers.GetPointer(v1.Namespace("test")), - Port: helpers.GetPointer(v1.PortNumber(80)), - }, - }, - }, - }, - }, - { - ValidMatches: true, - Filters: RouteRuleFilters{ - Valid: true, - Filters: []Filter{}, - }, - Matches: ConvertGRPCMatches(grpcRouteSPConflict.Spec.Rules[3].Matches), - RouteBackendRefs: []RouteBackendRef{ - { - BackendRef: v1.BackendRef{ - BackendObjectReference: v1.BackendObjectReference{ - Name: "service-two", - Kind: helpers.GetPointer(v1.Kind("Service")), - Namespace: helpers.GetPointer(v1.Namespace("test")), - Port: helpers.GetPointer(v1.PortNumber(80)), - }, - }, - }, - }, - }, - }, - }, - Conditions: []conditions.Condition{ - conditions.NewRouteAcceptedInvalidSessionPersistenceConfiguration( - "for backendRefs test/service-two:80 due to conflicting configuration across multiple rules", - ), - }, - }, - name: "route has multiple rules with same backend but different session persistence", - plus: true, - experimental: true, - }, } gws := map[types.NamespacedName]*Gateway{ @@ -1478,9 +1312,9 @@ func TestBuildGRPCRoute(t *testing.T) { test.gr, gws, snippetsFilters, - flags{ - plus: test.plus, - experimental: test.experimental, + FeatureFlags{ + Plus: test.plus, + Experimental: test.experimental, }, ) g.Expect(helpers.Diff(test.expected, route)).To(BeEmpty()) @@ -1634,9 +1468,9 @@ func TestBuildGRPCRouteWithMirrorRoutes(t *testing.T) { g := NewWithT(t) - featureFlags := flags{ - plus: false, - experimental: false, + featureFlags := FeatureFlags{ + Plus: false, + Experimental: false, } routes := map[RouteKey]*L7Route{} @@ -1661,9 +1495,9 @@ func TestConvertGRPCMatches(t *testing.T) { t.Parallel() methodMatch := createGRPCMethodMatch("myService", "myMethod", "Exact", nil, nil).Matches - headersMatch := createGRPCHeadersMatch("Exact", "MyHeader", "SomeValue", nil).Matches + headersMatch := createGRPCHeadersMatch("Exact", "MyHeader", "SomeValue").Matches - headerMatchRegularExp := createGRPCHeadersMatch("RegularExpression", "HeaderRegex", "headers-[a-z]+", nil).Matches + headerMatchRegularExp := createGRPCHeadersMatch("RegularExpression", "HeaderRegex", "headers-[a-z]+").Matches expectedHTTPMatches := []v1.HTTPRouteMatch{ { @@ -1832,9 +1666,9 @@ func TestProcessGRPCRouteRule_UnsupportedFields(t *testing.T) { unsupportedFieldsErrors := checkForUnsupportedGRPCFields( test.specRule, rulePath, - flags{ - plus: false, - experimental: false, + FeatureFlags{ + Plus: false, + Experimental: false, }, ) if len(unsupportedFieldsErrors) > 0 { @@ -1940,13 +1774,19 @@ func TestProcessGRPCRouteRules_UnsupportedFields(t *testing.T) { t.Parallel() g := NewWithT(t) + grpcRouteNsName := types.NamespacedName{ + Namespace: "test", + Name: "grpc-route", + } + _, valid, conds := processGRPCRouteRules( test.specRules, validation.SkipValidator{}, nil, - flags{ - plus: test.plusEnabled, - experimental: test.experimental, + grpcRouteNsName, + FeatureFlags{ + Plus: test.plusEnabled, + Experimental: test.experimental, }, ) diff --git a/internal/controller/state/graph/httproute.go b/internal/controller/state/graph/httproute.go index a2b78bc31b..c20c753d70 100644 --- a/internal/controller/state/graph/httproute.go +++ b/internal/controller/state/graph/httproute.go @@ -31,7 +31,7 @@ func buildHTTPRoute( gws map[types.NamespacedName]*Gateway, snippetsFilters map[types.NamespacedName]*SnippetsFilter, inferencePools map[types.NamespacedName]*inference.InferencePool, - featureFlags flags, + featureFlags FeatureFlags, ) *L7Route { r := &L7Route{ Source: ghr, @@ -64,12 +64,16 @@ func buildHTTPRoute( r.Spec.Hostnames = ghr.Spec.Hostnames r.Attachable = true + nsName := types.NamespacedName{ + Name: ghr.GetName(), + Namespace: ghr.GetNamespace(), + } rules, valid, conds := processHTTPRouteRules( ghr.Spec.Rules, validator, getSnippetsFilterResolverForNamespace(snippetsFilters, r.Source.GetNamespace()), inferencePools, - r.Source.GetNamespace(), + nsName, featureFlags, ) @@ -86,7 +90,7 @@ func buildHTTPMirrorRoutes( route *v1.HTTPRoute, gateways map[types.NamespacedName]*Gateway, snippetsFilters map[types.NamespacedName]*SnippetsFilter, - featureFlags flags, + featureFlags FeatureFlags, ) { for idx, rule := range l7route.Spec.Rules { if rule.Filters.Valid { @@ -175,15 +179,16 @@ func removeHTTPMirrorFilters(filters []v1.HTTPRouteFilter) []v1.HTTPRouteFilter func processHTTPRouteRule( specRule v1.HTTPRouteRule, - rulePath *field.Path, + ruleIdx int, validator validation.HTTPFieldsValidator, resolveExtRefFunc resolveExtRefFilter, inferencePools map[types.NamespacedName]*inference.InferencePool, - routeNamespace string, - featureFlags flags, + routeNsName types.NamespacedName, + featureFlags FeatureFlags, ) (RouteRule, routeRuleErrors) { - var errors routeRuleErrors + rulePath := field.NewPath("spec").Child("rules").Index(ruleIdx) + var errors routeRuleErrors unsupportedFieldsErrors := checkForUnsupportedHTTPFields(specRule, rulePath, featureFlags) if len(unsupportedFieldsErrors) > 0 { errors.warn = append(errors.warn, unsupportedFieldsErrors...) @@ -209,7 +214,23 @@ func processHTTPRouteRule( ) errors = errors.append(filterErrors) - backendRefs, backendRefErrors := getBackendRefs(specRule.BackendRefs, routeNamespace, inferencePools, rulePath) + var sp *SessionPersistenceConfig + if specRule.SessionPersistence != nil { + spConfig, spErrors := processSessionPersistenceConfig( + specRule.SessionPersistence, + specRule.Matches, + rulePath.Child("sessionPersistence"), + validator, + ) + errors = errors.append(spErrors) + + if spConfig != nil && spConfig.Valid { + spConfig.Idx = getSessionPersistenceKey(ruleIdx, routeNsName) + sp = spConfig + } + } + + backendRefs, backendRefErrors := getBackendRefs(specRule, routeNsName.Namespace, inferencePools, rulePath, sp) errors = errors.append(backendRefErrors) if routeFilters.Valid { @@ -228,42 +249,26 @@ func processHTTPRouteRule( } } - var sp *SessionPersistenceConfig - if specRule.SessionPersistence != nil { - spConfig, spErrors := processSessionPersistenceConfig( - specRule.SessionPersistence, - specRule.Matches, - rulePath.Child("sessionPersistence"), - validator, - ) - - errors = errors.append(spErrors) - - if spConfig != nil && spConfig.Valid { - sp = spConfig - } - } - return RouteRule{ - ValidMatches: validMatches, - Matches: specRule.Matches, - Filters: routeFilters, - RouteBackendRefs: backendRefs, - SessionPersistence: sp, + ValidMatches: validMatches, + Matches: specRule.Matches, + Filters: routeFilters, + RouteBackendRefs: backendRefs, }, errors } func getBackendRefs( - routeBackendRefs []v1.HTTPBackendRef, + routeRule v1.HTTPRouteRule, routeNamespace string, inferencePools map[types.NamespacedName]*inference.InferencePool, rulePath *field.Path, + sp *SessionPersistenceConfig, ) ([]RouteBackendRef, routeRuleErrors) { var errors routeRuleErrors - backendRefs := make([]RouteBackendRef, 0, len(routeBackendRefs)) + backendRefs := make([]RouteBackendRef, 0, len(routeRule.BackendRefs)) // rule.BackendRefs are validated separately because of their special requirements - for _, b := range routeBackendRefs { + for _, b := range routeRule.BackendRefs { var interfaceFilters []any if len(b.Filters) > 0 { interfaceFilters = make([]any, 0, len(b.Filters)) @@ -273,7 +278,8 @@ func getBackendRefs( } rbr := RouteBackendRef{ - BackendRef: b.BackendRef, + BackendRef: b.BackendRef, + SessionPersistence: sp, } // If route specifies an InferencePool backend, we need to convert it to its associated @@ -283,7 +289,7 @@ func getBackendRefs( // We don't support traffic splitting at the Route level for // InferencePool backends, so if there's more than one backendRef, and one of them // is an InferencePool, we mark the rule as invalid. - if len(routeBackendRefs) > 1 { + if len(routeRule.BackendRefs) > 1 { err := field.Forbidden( rulePath.Child("backendRefs"), "cannot use InferencePool backend when multiple backendRefs are specified in a single rule", @@ -319,8 +325,8 @@ func processHTTPRouteRules( validator validation.HTTPFieldsValidator, resolveExtRefFunc resolveExtRefFilter, inferencePools map[types.NamespacedName]*inference.InferencePool, - routeNamespace string, - featureFlags flags, + routeNsName types.NamespacedName, + featureFlags FeatureFlags, ) (rules []RouteRule, valid bool, conds []conditions.Condition) { rules = make([]RouteRule, len(specRules)) @@ -329,16 +335,14 @@ func processHTTPRouteRules( atLeastOneValid bool ) - for i, rule := range specRules { - rulePath := field.NewPath("spec").Child("rules").Index(i) - + for ruleIdx, rule := range specRules { rr, errors := processHTTPRouteRule( rule, - rulePath, + ruleIdx, validator, resolveExtRefFunc, inferencePools, - routeNamespace, + routeNsName, featureFlags, ) @@ -348,7 +352,7 @@ func processHTTPRouteRules( allRulesErrors = allRulesErrors.append(errors) - rules[i] = rr + rules[ruleIdx] = rr } conds = make([]conditions.Condition, 0, 2) @@ -360,11 +364,6 @@ func processHTTPRouteRules( conds = append(conds, conditions.NewRouteAcceptedUnsupportedField(allRulesErrors.warn.ToAggregate().Error())) } - spConflictErrors := handleSessionPersistenceConflicts(rules) - if spConflictErrors != nil { - conds = append(conds, conditions.NewRouteAcceptedInvalidSessionPersistenceConfiguration(spConflictErrors.Error())) - } - if len(allRulesErrors.invalid) > 0 { msg := allRulesErrors.invalid.ToAggregate().Error() @@ -649,7 +648,11 @@ func validateFilterRewrite( return allErrs } -func checkForUnsupportedHTTPFields(rule v1.HTTPRouteRule, rulePath *field.Path, featureFlags flags) field.ErrorList { +func checkForUnsupportedHTTPFields( + rule v1.HTTPRouteRule, + rulePath *field.Path, + featureFlags FeatureFlags, +) field.ErrorList { var ruleErrors field.ErrorList if rule.Name != nil { @@ -671,14 +674,14 @@ func checkForUnsupportedHTTPFields(rule v1.HTTPRouteRule, rulePath *field.Path, )) } - if !featureFlags.plus && rule.SessionPersistence != nil { + if !featureFlags.Plus && rule.SessionPersistence != nil { ruleErrors = append(ruleErrors, field.Forbidden( rulePath.Child("sessionPersistence"), "SessionPersistence is only supported in NGINX Plus. This configuration will be ignored.", )) } - if !featureFlags.experimental && rule.SessionPersistence != nil { + if !featureFlags.Experimental && rule.SessionPersistence != nil { ruleErrors = append(ruleErrors, field.Forbidden( rulePath.Child("sessionPersistence"), "SessionPersistence is only supported in experimental mode.", diff --git a/internal/controller/state/graph/httproute_test.go b/internal/controller/state/graph/httproute_test.go index d672562a10..f522ff30d6 100644 --- a/internal/controller/state/graph/httproute_test.go +++ b/internal/controller/state/graph/httproute_test.go @@ -141,6 +141,31 @@ func createInferencePoolBackend(name, namespace string) gatewayv1.BackendRef { } } +func getExpRouteBackendRefForPath(path string, spIdx string) RouteBackendRef { + return RouteBackendRef{ + BackendRef: gatewayv1.BackendRef{ + BackendObjectReference: gatewayv1.BackendObjectReference{ + Kind: helpers.GetPointer[gatewayv1.Kind](kinds.Service), + Name: "backend", + Port: helpers.GetPointer[gatewayv1.PortNumber](80), + }, + }, + Filters: []any{ + gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterExtensionRef, + }, + }, + SessionPersistence: &SessionPersistenceConfig{ + Valid: true, + Name: "http-route-session", + SessionType: gatewayv1.CookieBasedSessionPersistence, + Expiry: "1h", + Path: path, + Idx: spIdx, + }, + } +} + func TestBuildHTTPRoutes(t *testing.T) { t.Parallel() @@ -172,7 +197,7 @@ func TestBuildHTTPRoutes(t *testing.T) { RequestRedirect: &gatewayv1.HTTPRequestRedirectFilter{}, } - sessionPersistenceConfig := &gatewayv1.SessionPersistence{ + spConfig := &gatewayv1.SessionPersistence{ SessionName: helpers.GetPointer("http-route-session"), AbsoluteTimeout: helpers.GetPointer(gatewayv1.Duration("1h")), Type: helpers.GetPointer(gatewayv1.CookieBasedSessionPersistence), @@ -181,8 +206,8 @@ func TestBuildHTTPRoutes(t *testing.T) { }, } - addElementsToPath(hr, "/", snippetsFilterRef, nil) - addElementsToPath(hr, "/", requestRedirectFilter, sessionPersistenceConfig) + addElementsToPath(hr, "/", snippetsFilterRef, spConfig) + addElementsToPath(hr, "/", requestRedirectFilter, nil) hrWrongGateway := createHTTPRoute("hr-2", "some-gateway", "example.com", "/") @@ -258,14 +283,7 @@ func TestBuildHTTPRoutes(t *testing.T) { }, }, Matches: hr.Spec.Rules[0].Matches, - RouteBackendRefs: []RouteBackendRef{expRouteBackendRef}, - SessionPersistence: &SessionPersistenceConfig{ - Valid: true, - Name: *sessionPersistenceConfig.SessionName, - SessionType: *sessionPersistenceConfig.Type, - Expiry: "1h", - Path: "/", - }, + RouteBackendRefs: []RouteBackendRef{getExpRouteBackendRefForPath("/", "hr-1_test_0")}, }, }, }, @@ -308,9 +326,9 @@ func TestBuildHTTPRoutes(t *testing.T) { test.gateways, snippetsFilters, nil, - flags{ - plus: true, - experimental: true, + FeatureFlags{ + Plus: true, + Experimental: true, }, ) g.Expect(helpers.Diff(test.expected, routes)).To(BeEmpty()) @@ -345,7 +363,7 @@ func TestBuildHTTPRoute(t *testing.T) { AbsoluteTimeout: helpers.GetPointer(gatewayv1.Duration("1h")), Type: helpers.GetPointer(gatewayv1.CookieBasedSessionPersistence), CookieConfig: &gatewayv1.CookieConfig{ - LifetimeType: helpers.GetPointer((gatewayv1.SessionCookieLifetimeType)), + LifetimeType: helpers.GetPointer((gatewayv1.PermanentCookieLifetimeType)), }, } // route with valid filter @@ -383,12 +401,12 @@ func TestBuildHTTPRoute(t *testing.T) { "/filter", "/", ) - addElementsToPath(hrDroppedInvalidMatchesAndInvalidFilters, "/filter", invalidFilter, nil) + addElementsToPath(hrDroppedInvalidMatchesAndInvalidFilters, "/filter", invalidFilter, sp) // route with both invalid and valid filters in the same rule hrDroppedInvalidFilters := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/filter", "/") - addElementsToPath(hrDroppedInvalidFilters, "/filter", validFilter, nil) - addElementsToPath(hrDroppedInvalidFilters, "/", invalidFilter, nil) + addElementsToPath(hrDroppedInvalidFilters, "/filter", validFilter, sp) + addElementsToPath(hrDroppedInvalidFilters, "/", invalidFilter, sp) // route with duplicate section names hrDuplicateSectionName := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/") @@ -407,7 +425,7 @@ func TestBuildHTTPRoute(t *testing.T) { Name: "sf", }, } - addElementsToPath(hrValidSnippetsFilter, "/filter", validSnippetsFilterExtRef, nil) + addElementsToPath(hrValidSnippetsFilter, "/filter", validSnippetsFilterExtRef, sp) // route with invalid snippets filter extension ref hrInvalidSnippetsFilter := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/filter") @@ -445,6 +463,9 @@ func TestBuildHTTPRoute(t *testing.T) { BackendRef: createInferencePoolBackend("ipool", gatewayNsName.Namespace), }, } + + // session persistence should not be added for inference pool backends + hrInferencePool.Spec.Rules[0].SessionPersistence = sp // route with an inference pool backend that does not exist hrInferencePoolDoesNotExist := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/") hrInferencePoolDoesNotExist.Spec.Rules[0].BackendRefs = []gatewayv1.HTTPBackendRef{ @@ -466,27 +487,20 @@ func TestBuildHTTPRoute(t *testing.T) { } return nil }, - } - - hrWithMultipleRulesDifferentSP := createHTTPRoute( - "hr-multi-rules-sp", - gatewayNsName.Name, - "example.com", - "/rule1", - "/rule2", - ) - - spDifferent := &gatewayv1.SessionPersistence{ - SessionName: helpers.GetPointer("session-different"), - AbsoluteTimeout: helpers.GetPointer(gatewayv1.Duration("2h")), - Type: helpers.GetPointer(gatewayv1.CookieBasedSessionPersistence), - CookieConfig: &gatewayv1.CookieConfig{ - LifetimeType: helpers.GetPointer((gatewayv1.PermanentCookieLifetimeType)), + ValidateDurationStub: func(_ string) (string, error) { + return "1h", nil }, } - addElementsToPath(hrWithMultipleRulesDifferentSP, "/rule1", validFilter, sp) - addElementsToPath(hrWithMultipleRulesDifferentSP, "/rule2", validFilter, spDifferent) + createHTTPValidValidator := func(duration *gatewayv1.Duration) *validationfakes.FakeHTTPFieldsValidator { + v := &validationfakes.FakeHTTPFieldsValidator{} + if duration == nil { + v.ValidateDurationReturns("", nil) + } else { + v.ValidateDurationReturns(string(*duration), nil) + } + return v + } tests := []struct { validator *validationfakes.FakeHTTPFieldsValidator @@ -496,7 +510,7 @@ func TestBuildHTTPRoute(t *testing.T) { plus, experimental bool }{ { - validator: &validationfakes.FakeHTTPFieldsValidator{}, + validator: createHTTPValidValidator(sp.AbsoluteTimeout), hr: hr, expected: &L7Route{ RouteType: RouteTypeHTTP, @@ -529,13 +543,7 @@ func TestBuildHTTPRoute(t *testing.T) { Filters: convertHTTPRouteFilters(hr.Spec.Rules[1].Filters), }, Matches: hr.Spec.Rules[1].Matches, - RouteBackendRefs: []RouteBackendRef{expRouteBackendRef}, - SessionPersistence: &SessionPersistenceConfig{ - Valid: true, - Name: *sp.SessionName, - SessionType: *sp.Type, - Path: "/filter", - }, + RouteBackendRefs: []RouteBackendRef{getExpRouteBackendRefForPath("/filter", "hr_test_1")}, }, }, }, @@ -820,7 +828,7 @@ func TestBuildHTTPRoute(t *testing.T) { hrDroppedInvalidMatchesAndInvalidFilters.Spec.Rules[1].Filters, ), }, - RouteBackendRefs: []RouteBackendRef{expRouteBackendRef}, + RouteBackendRefs: []RouteBackendRef{getExpRouteBackendRefForPath("/filter", "hr_test_1")}, }, { ValidMatches: true, @@ -834,7 +842,9 @@ func TestBuildHTTPRoute(t *testing.T) { }, }, }, - name: "dropped invalid rule with invalid filters and invalid rule with invalid matches", + plus: true, + experimental: true, + name: "dropped invalid rule with invalid filters and invalid rule with invalid matches", }, { validator: validatorInvalidFieldsInRule, @@ -867,7 +877,7 @@ func TestBuildHTTPRoute(t *testing.T) { Filters: convertHTTPRouteFilters(hrDroppedInvalidFilters.Spec.Rules[0].Filters), Valid: true, }, - RouteBackendRefs: []RouteBackendRef{expRouteBackendRef}, + RouteBackendRefs: []RouteBackendRef{getExpRouteBackendRefForPath("/filter", "hr_test_0")}, }, { ValidMatches: true, @@ -876,12 +886,14 @@ func TestBuildHTTPRoute(t *testing.T) { Filters: convertHTTPRouteFilters(hrDroppedInvalidFilters.Spec.Rules[1].Filters), Valid: false, }, - RouteBackendRefs: []RouteBackendRef{expRouteBackendRef}, + RouteBackendRefs: []RouteBackendRef{getExpRouteBackendRefForPath("/", "hr_test_1")}, }, }, }, }, - name: "dropped invalid rule with invalid filters", + plus: true, + experimental: true, + name: "dropped invalid rule with invalid filters", }, { validator: validatorInvalidFieldsInRule, @@ -918,12 +930,14 @@ func TestBuildHTTPRoute(t *testing.T) { }, Valid: true, }, - RouteBackendRefs: []RouteBackendRef{expRouteBackendRef}, + RouteBackendRefs: []RouteBackendRef{getExpRouteBackendRefForPath("/filter", "hr_test_0")}, }, }, }, }, - name: "rule with valid snippets filter extension ref filter", + plus: true, + experimental: true, + name: "rule with valid snippets filter extension ref filter", }, { validator: validatorInvalidFieldsInRule, @@ -1124,7 +1138,9 @@ func TestBuildHTTPRoute(t *testing.T) { }, }, }, - name: "route with an inference pool backend gets converted to service", + plus: true, + experimental: true, + name: "route with an inference pool backend gets converted to service", }, { validator: &validationfakes.FakeHTTPFieldsValidator{}, @@ -1162,66 +1178,6 @@ func TestBuildHTTPRoute(t *testing.T) { }, name: "route with an inference pool backend that doesn't exist", }, - { - validator: &validationfakes.FakeHTTPFieldsValidator{}, - hr: hrWithMultipleRulesDifferentSP, - expected: &L7Route{ - RouteType: RouteTypeHTTP, - Source: hrWithMultipleRulesDifferentSP, - ParentRefs: []ParentRef{ - { - Idx: 0, - Gateway: CreateParentRefGateway(gw), - SectionName: hrWithMultipleRulesDifferentSP.Spec.ParentRefs[0].SectionName, - }, - }, - Valid: true, - Attachable: true, - Spec: L7RouteSpec{ - Hostnames: hrWithMultipleRulesDifferentSP.Spec.Hostnames, - Rules: []RouteRule{ - { - ValidMatches: true, - Filters: RouteRuleFilters{ - Valid: true, - Filters: []Filter{ - { - RouteType: RouteTypeHTTP, - FilterType: FilterRequestRedirect, - RequestRedirect: &gatewayv1.HTTPRequestRedirectFilter{}, - }, - }, - }, - Matches: hrWithMultipleRulesDifferentSP.Spec.Rules[0].Matches, - RouteBackendRefs: []RouteBackendRef{expRouteBackendRef}, - }, - { - ValidMatches: true, - Filters: RouteRuleFilters{ - Valid: true, - Filters: []Filter{ - { - RouteType: RouteTypeHTTP, - FilterType: FilterRequestRedirect, - RequestRedirect: &gatewayv1.HTTPRequestRedirectFilter{}, - }, - }, - }, - Matches: hrWithMultipleRulesDifferentSP.Spec.Rules[1].Matches, - RouteBackendRefs: []RouteBackendRef{expRouteBackendRef}, - }, - }, - }, - Conditions: []conditions.Condition{ - conditions.NewRouteAcceptedInvalidSessionPersistenceConfiguration( - "for backendRefs default/backend:80 due to conflicting configuration across multiple rules", - ), - }, - }, - plus: true, - experimental: true, - name: "route with multiple rules with different session persistence configs", - }, } gws := map[types.NamespacedName]*Gateway{ @@ -1245,9 +1201,9 @@ func TestBuildHTTPRoute(t *testing.T) { gws, snippetsFilters, inferencePools, - flags{ - plus: test.plus, - experimental: test.experimental, + FeatureFlags{ + Plus: test.plus, + Experimental: test.experimental, }, ) g.Expect(helpers.Diff(test.expected, route)).To(BeEmpty()) @@ -1380,9 +1336,9 @@ func TestBuildHTTPRouteWithMirrorRoutes(t *testing.T) { g := NewWithT(t) - featureFlags := flags{ - plus: false, - experimental: false, + featureFlags := FeatureFlags{ + Plus: false, + Experimental: false, } routes := map[RouteKey]*L7Route{} @@ -1411,9 +1367,9 @@ func TestProcessHTTPRouteRule_InferencePoolWithMultipleBackendRefs(t *testing.T) validator := &validationfakes.FakeHTTPFieldsValidator{} inferencePoolName := "ipool" - routeNamespace := "test" + routeNsName := types.NamespacedName{Namespace: "test", Name: "hr"} inferencePools := map[types.NamespacedName]*inference.InferencePool{ - {Namespace: routeNamespace, Name: inferencePoolName}: {}, + {Namespace: routeNsName.Namespace, Name: inferencePoolName}: {}, } // BackendRef 1: InferencePool @@ -1423,7 +1379,7 @@ func TestProcessHTTPRouteRule_InferencePoolWithMultipleBackendRefs(t *testing.T) Group: helpers.GetPointer[gatewayv1.Group](inferenceAPIGroup), Kind: helpers.GetPointer[gatewayv1.Kind](kinds.InferencePool), Name: gatewayv1.ObjectName(inferencePoolName), - Namespace: helpers.GetPointer(gatewayv1.Namespace(routeNamespace)), + Namespace: helpers.GetPointer(gatewayv1.Namespace(routeNsName.Namespace)), }, }, } @@ -1449,18 +1405,17 @@ func TestProcessHTTPRouteRule_InferencePoolWithMultipleBackendRefs(t *testing.T) BackendRefs: []gatewayv1.HTTPBackendRef{backendRef1, backendRef2}, } - rulePath := field.NewPath("spec").Child("rules").Index(0) - + ruleIdx := 0 routeRule, errs := processHTTPRouteRule( specRule, - rulePath, + ruleIdx, validator, nil, inferencePools, - routeNamespace, - flags{ - plus: false, - experimental: false, + routeNsName, + FeatureFlags{ + Plus: false, + Experimental: false, }, ) @@ -2091,9 +2046,9 @@ func TestUnsupportedFieldsErrors(t *testing.T) { unsupportedFieldsErrors := checkForUnsupportedHTTPFields( test.specRule, rulePath, - flags{ - plus: false, - experimental: false, + FeatureFlags{ + Plus: false, + Experimental: false, }, ) if len(unsupportedFieldsErrors) > 0 { @@ -2107,7 +2062,10 @@ func TestUnsupportedFieldsErrors(t *testing.T) { func TestProcessHTTPRouteRules_UnsupportedFields(t *testing.T) { t.Parallel() - routeNamespace := "test" + routeNsName := types.NamespacedName{ + Namespace: "test", + Name: "route", + } tests := []struct { name string @@ -2211,10 +2169,10 @@ func TestProcessHTTPRouteRules_UnsupportedFields(t *testing.T) { validation.SkipValidator{}, nil, nil, - routeNamespace, - flags{ - plus: test.plusEnabled, - experimental: test.experimental, + routeNsName, + FeatureFlags{ + Plus: test.plusEnabled, + Experimental: test.experimental, }, ) diff --git a/internal/controller/state/graph/multiple_gateways_test.go b/internal/controller/state/graph/multiple_gateways_test.go index c20fd98516..3d027661df 100644 --- a/internal/controller/state/graph/multiple_gateways_test.go +++ b/internal/controller/state/graph/multiple_gateways_test.go @@ -409,7 +409,9 @@ func Test_MultipleGateways_WithNginxProxy(t *testing.T) { PolicyValidator: fakePolicyValidator, }, logr.Discard(), - experimentalFeaturesEnabled, + FeatureFlags{ + Experimental: experimentalFeaturesEnabled, + }, ) g.Expect(helpers.Diff(test.expGraph, result)).To(BeEmpty()) @@ -899,7 +901,9 @@ func Test_MultipleGateways_WithListeners(t *testing.T) { PolicyValidator: fakePolicyValidator, }, logr.Discard(), - experimentalFeaturesEnabled, + FeatureFlags{ + Experimental: experimentalFeaturesEnabled, + }, ) g.Expect(helpers.Diff(test.expGraph, result)).To(BeEmpty()) diff --git a/internal/controller/state/graph/route_common.go b/internal/controller/state/graph/route_common.go index bc5b435884..b65bde0c52 100644 --- a/internal/controller/state/graph/route_common.go +++ b/internal/controller/state/graph/route_common.go @@ -148,8 +148,6 @@ type L7RouteSpec struct { } type RouteRule struct { - // SessionPersistence holds the session persistence configuration for the route rule. - SessionPersistence *SessionPersistenceConfig // Matches define the predicate used to match requests to a given action. Matches []v1.HTTPRouteMatch // RouteBackendRefs are a wrapper for v1.BackendRef and any BackendRef filters from the HTTPRoute or GRPCRoute. @@ -172,6 +170,9 @@ type RouteBackendRef struct { // EndpointPickerConfig is the configuration for the EndpointPicker, if this backendRef is for an InferencePool. EndpointPickerConfig EndpointPickerConfig + // SessionPersistence holds the session persistence configuration for the route rule. + SessionPersistence *SessionPersistenceConfig + Filters []any // IsInferencePool indicates if this backend is an InferencePool disguised as a Service. @@ -187,6 +188,8 @@ type SessionPersistenceConfig struct { SessionType v1.SessionPersistenceType // Path is the path for which the session persistence is allowed. Path string + // Idx is the unique identifier for this configuration in the route rule. + Idx string // Valid indicates if the session persistence configuration is valid. Valid bool } @@ -272,7 +275,7 @@ func buildRoutesForGateways( gateways map[types.NamespacedName]*Gateway, snippetsFilters map[types.NamespacedName]*SnippetsFilter, inferencePools map[types.NamespacedName]*inference.InferencePool, - featureFlags flags, + featureFlags FeatureFlags, ) map[RouteKey]*L7Route { if len(gateways) == 0 { return nil @@ -1168,6 +1171,10 @@ func routeKeyForKind(kind v1.Kind, nsname types.NamespacedName) RouteKey { return key } +func getSessionPersistenceKey(ruleIdx int, routeNsName types.NamespacedName) string { + return fmt.Sprintf("%s_%s_%d", routeNsName.Name, routeNsName.Namespace, ruleIdx) +} + // processSessionPersistenceConfig processes the session persistence configuration. func processSessionPersistenceConfig[T any]( sp *v1.SessionPersistence, @@ -1325,80 +1332,3 @@ func getCookiePath(match v1.HTTPRouteMatch) string { return "" } } - -// handleSessionPersistenceConflicts enforces: -// 1. For each backend (ns/name:port), all rules that configure SessionPersistence -// for that backend must have the same SessionPersistenceConfig. -// 2. If multiple rules configure different session persistence configs for the same backend, it -// is cleared on all those rules for that backend and a warning is emitted. -func handleSessionPersistenceConflicts(rules []RouteRule) error { - if len(rules) == 0 { - return nil - } - - backendToRuleIdxs := make(map[string][]int) - spPerBackendRef := make(map[string]*SessionPersistenceConfig) - - for ri, rule := range rules { - sp := rule.SessionPersistence - if sp == nil { - continue - } - - for _, rbr := range rule.RouteBackendRefs { - key := createBackendRefKey(rbr.BackendRef) - backendToRuleIdxs[key] = append(backendToRuleIdxs[key], ri) - - if existing, ok := spPerBackendRef[key]; !ok { - spPerBackendRef[key] = sp - } else if !equalSessionPersistenceConfig(existing, sp) { - spPerBackendRef[key] = nil - } - } - } - - hadConflict := false - conflictingBackends := make([]string, 0) - for backendKey, sp := range spPerBackendRef { - if sp != nil { - continue - } - - hadConflict = true - conflictingBackends = append(conflictingBackends, backendKey) - for _, ri := range backendToRuleIdxs[backendKey] { - rules[ri].SessionPersistence = nil - } - } - - if !hadConflict { - return nil - } - - return fmt.Errorf("for backendRefs %s due to conflicting configuration across multiple rules", - strings.Join(conflictingBackends, ", "), - ) -} - -func equalSessionPersistenceConfig(a, b *SessionPersistenceConfig) bool { - if a == nil || b == nil { - return a == b - } - return a.Name == b.Name && - a.SessionType == b.SessionType && - a.Expiry == b.Expiry && - a.Path == b.Path -} - -func createBackendRefKey(backendRef v1.BackendRef) string { - var port int32 - if backendRef.Port != nil { - port = *backendRef.Port - } - - ns := defaultNamespace - if backendRef.Namespace != nil { - ns = string(*backendRef.Namespace) - } - return fmt.Sprintf("%s/%s:%d", ns, backendRef.Name, port) -} diff --git a/internal/controller/state/graph/route_common_test.go b/internal/controller/state/graph/route_common_test.go index e91dd2e865..94e0f247da 100644 --- a/internal/controller/state/graph/route_common_test.go +++ b/internal/controller/state/graph/route_common_test.go @@ -4313,261 +4313,3 @@ func TestGetCookiePath(t *testing.T) { }) } } - -func TestHandleSessionPersistenceConflict_RouteRules(t *testing.T) { - t.Parallel() - g := NewWithT(t) - - routeRuleNoConflict := []RouteRule{ - { - SessionPersistence: &SessionPersistenceConfig{ - Valid: true, - SessionType: gatewayv1.CookieBasedSessionPersistence, - Name: "session-1", - Expiry: "30m", - }, - RouteBackendRefs: []RouteBackendRef{ - { - BackendRef: gatewayv1.BackendRef{ - BackendObjectReference: gatewayv1.BackendObjectReference{ - Name: "backend-1", - Port: helpers.GetPointer[gatewayv1.PortNumber](8080), - Namespace: helpers.GetPointer(gatewayv1.Namespace("default")), - }, - }, - }, - }, - }, - { - SessionPersistence: &SessionPersistenceConfig{ - Valid: true, - SessionType: gatewayv1.CookieBasedSessionPersistence, - Name: "session-2", - Expiry: "1h", - }, - RouteBackendRefs: []RouteBackendRef{ - { - BackendRef: gatewayv1.BackendRef{ - BackendObjectReference: gatewayv1.BackendObjectReference{ - Name: "backend-2", - Port: helpers.GetPointer[gatewayv1.PortNumber](8080), - Namespace: helpers.GetPointer(gatewayv1.Namespace("default")), - }, - }, - }, - }, - }, - } - - errNoConflict := handleSessionPersistenceConflicts(routeRuleNoConflict) - g.Expect(errNoConflict).ToNot(HaveOccurred()) - - // backend-1 has same session persistence config across multiple rules - no conflict - // backend-2 has different session persistence config across multiple rules - conflict - routeRuleWithConflict := []RouteRule{ - { - SessionPersistence: &SessionPersistenceConfig{ - Valid: true, - SessionType: gatewayv1.CookieBasedSessionPersistence, - Name: "session-1", - Expiry: "30m", - }, - RouteBackendRefs: []RouteBackendRef{ - { - BackendRef: gatewayv1.BackendRef{ - BackendObjectReference: gatewayv1.BackendObjectReference{ - Name: "backend-1", - Port: helpers.GetPointer[gatewayv1.PortNumber](8080), - Namespace: helpers.GetPointer(gatewayv1.Namespace("default")), - }, - }, - }, - }, - }, - { - SessionPersistence: &SessionPersistenceConfig{ - Valid: true, - SessionType: gatewayv1.CookieBasedSessionPersistence, - Name: "session-1", - Expiry: "30m", - }, - RouteBackendRefs: []RouteBackendRef{ - { - BackendRef: gatewayv1.BackendRef{ - BackendObjectReference: gatewayv1.BackendObjectReference{ - Name: "backend-1", - Port: helpers.GetPointer[gatewayv1.PortNumber](8080), - Namespace: helpers.GetPointer(gatewayv1.Namespace("default")), - }, - }, - }, - }, - }, - { - SessionPersistence: &SessionPersistenceConfig{ - Valid: true, - SessionType: gatewayv1.CookieBasedSessionPersistence, - Name: "session-3", - Expiry: "24h", - }, - RouteBackendRefs: []RouteBackendRef{ - { - BackendRef: gatewayv1.BackendRef{ - BackendObjectReference: gatewayv1.BackendObjectReference{ - Name: "backend-2", - Port: helpers.GetPointer[gatewayv1.PortNumber](8080), - Namespace: helpers.GetPointer(gatewayv1.Namespace("default")), - }, - }, - }, - }, - }, - { - SessionPersistence: &SessionPersistenceConfig{ - Valid: true, - SessionType: gatewayv1.CookieBasedSessionPersistence, - Name: "session-4", - Expiry: "20h", - }, - RouteBackendRefs: []RouteBackendRef{ - { - BackendRef: gatewayv1.BackendRef{ - BackendObjectReference: gatewayv1.BackendObjectReference{ - Name: "backend-2", - Port: helpers.GetPointer[gatewayv1.PortNumber](8080), - Namespace: helpers.GetPointer(gatewayv1.Namespace("default")), - }, - }, - }, - }, - }, - { - SessionPersistence: &SessionPersistenceConfig{ - Valid: true, - SessionType: gatewayv1.CookieBasedSessionPersistence, - Name: "session-5", - Expiry: "20h", - }, - RouteBackendRefs: []RouteBackendRef{ - { - BackendRef: gatewayv1.BackendRef{ - BackendObjectReference: gatewayv1.BackendObjectReference{ - Name: "backend-3", - Port: helpers.GetPointer[gatewayv1.PortNumber](8080), - Namespace: helpers.GetPointer(gatewayv1.Namespace("default")), - }, - }, - }, - }, - }, - } - - errWithConflict := handleSessionPersistenceConflicts(routeRuleWithConflict) - g.Expect(errWithConflict).To(HaveOccurred()) - g.Expect(errWithConflict.Error()).To(ContainSubstring( - "for backendRefs default/backend-2:8080 due to conflicting configuration across multiple rules", - )) -} - -func TestEqualSessionPersistence(t *testing.T) { - t.Parallel() - - tests := []struct { - sp1 *SessionPersistenceConfig - sp2 *SessionPersistenceConfig - name string - expected bool - }{ - { - name: "both nil session persistence are equal", - sp1: nil, - sp2: nil, - expected: true, - }, - { - name: "one nil session persistence is not equal", - sp1: &SessionPersistenceConfig{ - Valid: true, - SessionType: gatewayv1.CookieBasedSessionPersistence, - Name: "session-1", - Expiry: "30m", - }, - sp2: nil, - expected: false, - }, - { - name: "different session names are not equal", - sp1: &SessionPersistenceConfig{ - Valid: true, - SessionType: gatewayv1.CookieBasedSessionPersistence, - Name: "session-1", - Expiry: "30m", - }, - sp2: &SessionPersistenceConfig{ - Valid: true, - SessionType: gatewayv1.CookieBasedSessionPersistence, - Name: "session-2", - Expiry: "30m", - }, - expected: false, - }, - { - name: "different expiry durations are not equal", - sp1: &SessionPersistenceConfig{ - Valid: true, - SessionType: gatewayv1.CookieBasedSessionPersistence, - Name: "session-1", - Expiry: "30m", - }, - sp2: &SessionPersistenceConfig{ - Valid: true, - SessionType: gatewayv1.CookieBasedSessionPersistence, - Name: "session-1", - Expiry: "1h", - }, - expected: false, - }, - { - name: "different session types are not equal", - sp1: &SessionPersistenceConfig{ - Valid: true, - SessionType: gatewayv1.CookieBasedSessionPersistence, - Name: "session-1", - Expiry: "30m", - }, - sp2: &SessionPersistenceConfig{ - Valid: true, - SessionType: gatewayv1.HeaderBasedSessionPersistence, - Name: "session-1", - Expiry: "30m", - }, - expected: false, - }, - { - name: "same session persistence are equal", - sp1: &SessionPersistenceConfig{ - Valid: true, - SessionType: gatewayv1.CookieBasedSessionPersistence, - Name: "session-1", - Expiry: "30m", - }, - sp2: &SessionPersistenceConfig{ - Valid: true, - SessionType: gatewayv1.CookieBasedSessionPersistence, - Name: "session-1", - Expiry: "30m", - }, - expected: true, - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - t.Parallel() - g := NewWithT(t) - - result := equalSessionPersistenceConfig(test.sp1, test.sp2) - g.Expect(result).To(Equal(test.expected)) - }) - } -}