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/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..b638d5d04f 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.CookieBasedSessionPersistence, + }, + }, + { + 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.CookieBasedSessionPersistence, + }, + }, + { + 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.CookieBasedSessionPersistence, + }, + }, + } + + 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;": 7, + "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.CookieBasedSessionPersistence, + 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.CookieBasedSessionPersistence), + 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/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/dataplane/configuration.go b/internal/controller/state/dataplane/configuration.go index 26bc3c58ee..ea85d46627 100644 --- a/internal/controller/state/dataplane/configuration.go +++ b/internal/controller/state/dataplane/configuration.go @@ -783,6 +783,7 @@ func buildUpstreams( referencedServices, uniqueUpstreams, allowedAddressType, + br.SessionPersistence, ); upstream != nil { uniqueUpstreams[upstream.Name] = *upstream } @@ -818,6 +819,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 @@ -836,7 +838,6 @@ func buildUpstream( } var errMsg string - eps, err := resolveUpstreamEndpoints( ctx, logger, @@ -857,11 +858,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: CookieBasedSessionPersistence, + } + } + 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..dead623b49 100644 --- a/internal/controller/state/dataplane/configuration_test.go +++ b/internal/controller/state/dataplane/configuration_test.go @@ -2873,10 +2873,10 @@ func TestGetListenerHostname(t *testing.T) { } } -func refsToValidRules(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}, @@ -2981,44 +2981,66 @@ 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, + } + } - hr1Refs1 := createBackendRefs("baz", "", "") // empty service names should be ignored + hr1Refs0 := createBackendRefs(createSPConfig("foo-bar-sp"), "foo", "bar") - hr1Refs2 := createBackendRefs("invalid-for-gateway") + hr1Refs1 := createBackendRefs(nil, "baz", "", "") // empty service names should be ignored + + 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, "nil-endpoints") - hr2Refs1 := createBackendRefs("nil-endpoints") + hr3Refs0 := createBackendRefs(nil, "baz") // shouldn't duplicate baz upstream - hr3Refs0 := createBackendRefs("baz") // shouldn't duplicate baz upstream + hr4Refs0 := createBackendRefs(nil, "empty-endpoints", "") - hr4Refs0 := createBackendRefs("empty-endpoints", "") + hr4Refs1 := createBackendRefs(nil, "baz2") - hr4Refs1 := createBackendRefs("baz2") + hr5Refs0 := createBackendRefs(nil, "ipv6-endpoints") - hr5Refs0 := createBackendRefs("ipv6-endpoints") + nonExistingRefs := createBackendRefs(nil, "non-existing") - nonExistingRefs := createBackendRefs("non-existing") + invalidHRRefs := createBackendRefs(nil, "abc") - invalidHRRefs := createBackendRefs("abc") + refsWithPolicies := createBackendRefs(createSPConfig("policies-sp"), "policies") - refsWithPolicies := createBackendRefs("policies") + 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"}}: { @@ -3163,39 +3185,57 @@ func TestBuildUpstreams(t *testing.T) { expUpstreams := []Upstream{ { - Name: "test_bar_80", - Endpoints: barEndpoints, + Name: "test_bar_80_foo-bar-sp", + Endpoints: barEndpoints, + SessionPersistence: getExpectedConfig(), + }, + { + Name: "test_baz2_80", + Endpoints: baz2Endpoints, + SessionPersistence: SessionPersistenceConfig{}, }, { - Name: "test_baz2_80", - Endpoints: baz2Endpoints, + Name: "test_baz_80", + Endpoints: bazEndpoints, + SessionPersistence: SessionPersistenceConfig{}, }, { - Name: "test_baz_80", - Endpoints: bazEndpoints, + Name: "test_baz_80_foo-baz-sp", + Endpoints: bazEndpoints, + SessionPersistence: getExpectedConfig(), }, { - 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_foo-bar-sp", + Endpoints: fooEndpoints, + SessionPersistence: getExpectedConfig(), + }, + { + Name: "test_foo_80_foo-baz-sp", + Endpoints: fooEndpoints, + SessionPersistence: getExpectedConfig(), + }, + { + Name: "test_ipv6-endpoints_80", + Endpoints: ipv6Endpoints, + SessionPersistence: SessionPersistenceConfig{}, }, { Name: "test_nil-endpoints_80", Endpoints: nil, ErrorMsg: nilEndpointsErrMsg, }, + { - Name: "test_ipv6-endpoints_80", - Endpoints: ipv6Endpoints, - }, - { - Name: "test_policies_80", - Endpoints: policyEndpoints, - Policies: []policies.Policy{validPolicy1, validPolicy2}, + Name: "test_policies_80_policies-sp", + Endpoints: policyEndpoints, + Policies: []policies.Policy{validPolicy1, validPolicy2}, + SessionPersistence: getExpectedConfig(), }, } diff --git a/internal/controller/state/dataplane/types.go b/internal/controller/state/dataplane/types.go index 1b0ccbde6a..e6cb43f299 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,26 @@ 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 +} + +// SessionPersistenceType is the type of session persistence. +type SessionPersistenceType string + +const ( + // CookieBasedSessionPersistence indicates cookie-based session persistence. + CookieBasedSessionPersistence 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/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 ffd86ac6e0..c670ccc487 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 +// 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. func (g *Graph) IsReferenced(resourceType ngftypes.ObjectType, nsname types.NamespacedName) bool { switch obj := resourceType.(type) { @@ -208,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 { @@ -228,7 +236,7 @@ func BuildGraph( processedGwClasses.Winner, processedNginxProxies, state.CRDMetadata, - experimentalEnabled, + featureFlags.Experimental, ) secretResolver := newSecretResolver(state.Secrets) @@ -242,7 +250,7 @@ func BuildGraph( gc, refGrantResolver, processedNginxProxies, - experimentalEnabled, + featureFlags.Experimental, ) processedBackendTLSPolicies := processBackendTLSPolicies( @@ -261,6 +269,7 @@ func BuildGraph( gws, processedSnippetsFilters, state.InferencePools, + 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 ed0e39082f..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{ @@ -196,8 +201,9 @@ func TestBuildGraph(t *testing.T) { createValidRuleWithBackendRefsAndFilters := func( matches []gatewayv1.HTTPRouteMatch, routeType RouteType, + sessionPersistence *SessionPersistenceConfig, ) RouteRule { - rule := createValidRuleWithBackendRefs(matches) + rule := createValidRuleWithBackendRefs(matches, sessionPersistence) rule.Filters = RouteRuleFilters{ Filters: []Filter{ { @@ -333,14 +339,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 +396,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 +881,15 @@ func TestBuildGraph(t *testing.T) { } } + 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, @@ -885,7 +917,7 @@ func TestBuildGraph(t *testing.T) { }, Spec: L7RouteSpec{ Hostnames: hr1.Spec.Hostnames, - Rules: []RouteRule{createValidRuleWithBackendRefsAndFilters(routeMatches, RouteTypeHTTP)}, + Rules: []RouteRule{createValidRuleWithBackendRefsAndFilters(routeMatches, RouteTypeHTTP, getExpectedSPConfig)}, }, Policies: []*Policy{processedRoutePolicy}, Conditions: []conditions.Condition{ @@ -1049,6 +1081,13 @@ func TestBuildGraph(t *testing.T) { }, } + expectedSPgr := &SessionPersistenceConfig{ + Name: "session-persistence-grpcroute", + SessionType: gatewayv1.CookieBasedSessionPersistence, + Expiry: "30m", + Valid: true, + Idx: "gr_test_0", + } routeGR := &L7Route{ RouteType: RouteTypeGRPC, Valid: true, @@ -1077,7 +1116,7 @@ func TestBuildGraph(t *testing.T) { Spec: L7RouteSpec{ Hostnames: gr.Spec.Hostnames, Rules: []RouteRule{ - createValidRuleWithBackendRefsAndFilters(routeMatches, RouteTypeGRPC), + createValidRuleWithBackendRefsAndFilters(routeMatches, RouteTypeGRPC, expectedSPgr), }, }, } @@ -1109,7 +1148,7 @@ func TestBuildGraph(t *testing.T) { }, Spec: L7RouteSpec{ Hostnames: hr3.Spec.Hostnames, - Rules: []RouteRule{createValidRuleWithBackendRefs(routeMatches)}, + Rules: []RouteRule{createValidRuleWithBackendRefs(routeMatches, nil)}, }, } @@ -1448,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", }, { @@ -1475,6 +1515,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,12 +1534,15 @@ func TestBuildGraph(t *testing.T) { }, }, validation.Validators{ - HTTPFieldsValidator: &validationfakes.FakeHTTPFieldsValidator{}, + HTTPFieldsValidator: createAllValidValidator(), GenericValidator: &validationfakes.FakeGenericValidator{}, 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 5d6e7489e3..15611f2a8b 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 FeatureFlags, ) *L7Route { r := &L7Route{ Source: ghr, @@ -52,10 +53,16 @@ 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, ) r.Spec.Rules = rules @@ -71,6 +78,7 @@ func buildGRPCMirrorRoutes( route *v1.GRPCRoute, gateways map[types.NamespacedName]*Gateway, snippetsFilters map[types.NamespacedName]*SnippetsFilter, + featureFlags FeatureFlags, ) { for idx, rule := range l7route.Spec.Rules { if rule.Filters.Valid { @@ -107,6 +115,7 @@ func buildGRPCMirrorRoutes( tmpMirrorRoute, gateways, snippetsFilters, + featureFlags, ) if mirrorRoute != nil { @@ -157,15 +166,17 @@ func removeGRPCMirrorFilters(filters []v1.GRPCRouteFilter) []v1.GRPCRouteFilter func processGRPCRouteRule( specRule v1.GRPCRouteRule, - rulePath *field.Path, + ruleIdx int, validator validation.HTTPFieldsValidator, resolveExtRefFunc resolveExtRefFilter, + grpcRouteNsName types.NamespacedName, + featureFlags FeatureFlags, ) (RouteRule, routeRuleErrors) { - var errors routeRuleErrors - + rulePath := field.NewPath("spec").Child("rules").Index(ruleIdx) validMatches := true - unsupportedFieldsErrors := checkForUnsupportedGRPCFields(specRule, rulePath) + var errors routeRuleErrors + unsupportedFieldsErrors := checkForUnsupportedGRPCFields(specRule, rulePath, featureFlags) if len(unsupportedFieldsErrors) > 0 { errors.warn = append(errors.warn, unsupportedFieldsErrors...) } @@ -189,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 @@ -201,8 +228,9 @@ func processGRPCRouteRule( } } rbr := RouteBackendRef{ - BackendRef: b.BackendRef, - Filters: interfaceFilters, + BackendRef: b.BackendRef, + Filters: interfaceFilters, + SessionPersistence: sp, } backendRefs = append(backendRefs, rbr) } @@ -235,6 +263,8 @@ func processGRPCRouteRules( specRules []v1.GRPCRouteRule, validator validation.HTTPFieldsValidator, resolveExtRefFunc resolveExtRefFilter, + grpcRouteNsName types.NamespacedName, + featureFlags FeatureFlags, ) (rules []RouteRule, valid bool, conds []conditions.Condition) { rules = make([]RouteRule, len(specRules)) @@ -243,14 +273,14 @@ 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, ) if rr.ValidMatches && rr.Filters.Valid { @@ -259,7 +289,7 @@ func processGRPCRouteRules( allRulesErrors = allRulesErrors.append(errors) - rules[i] = rr + rules[ruleIdx] = rr } conds = make([]conditions.Condition, 0, 2) @@ -455,7 +485,11 @@ func validateGRPCHeaderMatch( return allErrs } -func checkForUnsupportedGRPCFields(rule v1.GRPCRouteRule, rulePath *field.Path) field.ErrorList { +func checkForUnsupportedGRPCFields( + rule v1.GRPCRouteRule, + rulePath *field.Path, + featureFlags FeatureFlags, +) field.ErrorList { var ruleErrors field.ErrorList if rule.Name != nil { @@ -464,10 +498,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..c94d650e62 100644 --- a/internal/controller/state/graph/grpcroute_test.go +++ b/internal/controller/state/graph/grpcroute_test.go @@ -20,38 +20,56 @@ 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, }, }, } + + if sp != nil { + return v1.GRPCRouteRule{ + Matches: matches, + SessionPersistence: sp, + BackendRefs: backendRef, + } + } + + return v1.GRPCRouteRule{ + Matches: matches, + } } func createGRPCHeadersMatch(headerType, headerName, headerValue string) v1.GRPCRouteRule { - return v1.GRPCRouteRule{ - Matches: []v1.GRPCRouteMatch{ - { - Headers: []v1.GRPCHeaderMatch{ - { - Type: (*v1.GRPCHeaderMatchType)(&headerType), - Name: v1.GRPCHeaderName(headerName), - Value: headerValue, - }, + matches := []v1.GRPCRouteMatch{ + { + Headers: []v1.GRPCHeaderMatch{ + { + Type: (*v1.GRPCHeaderMatchType)(&headerType), + Name: v1.GRPCHeaderName(headerName), + Value: headerValue, }, }, }, } + + return v1.GRPCRouteRule{ + Matches: matches, + } } func createGRPCRoute( @@ -114,11 +132,33 @@ 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)), + }, + } + + 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{grRuleWithFilters}) + gr := createGRPCRoute("gr-1", gwNsName.Name, "example.com", []v1.GRPCRouteRule{grRuleWithFiltersAndSessionPersistence}) grWrongGateway := createGRPCRoute("gr-2", "some-gateway", "example.com", []v1.GRPCRouteRule{}) @@ -193,8 +233,21 @@ func TestBuildGRPCRoutes(t *testing.T) { }, }, }, - ValidMatches: true, - RouteBackendRefs: []RouteBackendRef{}, + 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", + }, + }, + }, }, }, }, @@ -209,7 +262,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 +282,17 @@ func TestBuildGRPCRoutes(t *testing.T) { }, }, } - routes := buildRoutesForGateways( - validator, + createAllValidValidator(), map[types.NamespacedName]*v1.HTTPRoute{}, grRoutes, test.gateways, snippetsFilters, nil, + FeatureFlags{ + Plus: true, + Experimental: true, + }, ) g.Expect(helpers.Diff(test.expected, routes)).To(BeEmpty()) }) @@ -255,13 +315,40 @@ 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"), + AbsoluteTimeout: helpers.GetPointer(v1.Duration("10h")), + Type: helpers.GetPointer(v1.CookieBasedSessionPersistence), + CookieConfig: &v1.CookieConfig{ + LifetimeType: helpers.GetPointer((v1.PermanentCookieLifetimeType)), + }, + } - methodMatchRule := createGRPCMethodMatch("myService", "myMethod", "Exact") + methodMatchRule := createGRPCMethodMatch( + "myService", + "myMethod", + "Exact", + spMethod, + []v1.GRPCBackendRef{grpcBackendRef}, + ) headersMatchRule := createGRPCHeadersMatch("Exact", "MyHeader", "SomeValue") - methodMatchEmptyFields := createGRPCMethodMatch("", "", "") - methodMatchInvalidFields := createGRPCMethodMatch("service{}", "method{}", "Exact") - methodMatchNilType := createGRPCMethodMatch("myService", "myMethod", "nilType") + methodMatchEmptyFields := createGRPCMethodMatch("", "", "", nil, nil) + methodMatchInvalidFields := createGRPCMethodMatch("service{}", "method{}", "Exact", nil, nil) + methodMatchNilType := createGRPCMethodMatch("myService", "myMethod", "nilType", nil, nil) headersMatchInvalid := createGRPCHeadersMatch("", "MyHeader", "SomeValue") headersMatchEmptyType := v1.GRPCRouteRule{ @@ -284,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, @@ -396,7 +470,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,7 +485,7 @@ func TestBuildGRPCRoute(t *testing.T) { []v1.GRPCRouteRule{grInvalidFilterRule}, ) - grValidFilterRule := createGRPCMethodMatch("myService", "myMethod", "Exact") + grValidFilterRule := createGRPCMethodMatch("myService", "myMethod", "Exact", nil, nil) grValidHeaderMatch := createGRPCHeadersMatch("RegularExpression", "MyHeader", "headers-[a-z]+") validSnippetsFilterRef := &v1.LocalObjectReference{ Group: ngfAPIv1alpha1.GroupName, @@ -451,7 +525,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 +544,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 +563,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 +589,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 +632,16 @@ func TestBuildGRPCRoute(t *testing.T) { }, } + 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, @@ -582,8 +664,19 @@ func TestBuildGRPCRoute(t *testing.T) { Valid: true, Filters: []Filter{}, }, - Matches: ConvertGRPCMatches(grBoth.Spec.Rules[0].Matches), - RouteBackendRefs: []RouteBackendRef{}, + 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", + }, + }, + }, }, { ValidMatches: true, @@ -597,10 +690,12 @@ func TestBuildGRPCRoute(t *testing.T) { }, }, }, - 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 +727,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 +768,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 +809,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 +851,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 +860,7 @@ func TestBuildGRPCRoute(t *testing.T) { name: "invalid route with duplicate sectionName", }, { - validator: createAllValidValidator(), + validator: createAllValidValidator(&durationSP), gr: grOneInvalid, expected: &L7Route{ Source: grOneInvalid, @@ -793,8 +888,19 @@ func TestBuildGRPCRoute(t *testing.T) { Valid: true, Filters: []Filter{}, }, - Matches: ConvertGRPCMatches(grOneInvalid.Spec.Rules[0].Matches), - RouteBackendRefs: []RouteBackendRef{}, + 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", + }, + }, + }, }, { ValidMatches: false, @@ -808,10 +914,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 +957,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 +995,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 +1032,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 +1071,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 +1100,7 @@ func TestBuildGRPCRoute(t *testing.T) { name: "invalid hostname", }, { - validator: createAllValidValidator(), + validator: createAllValidValidator(nil), gr: grInvalidSnippetsFilter, expected: &L7Route{ Source: grInvalidSnippetsFilter, @@ -1030,7 +1138,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 +1177,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 +1220,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 +1255,7 @@ func TestBuildGRPCRoute(t *testing.T) { name: "valid route with unsupported field", }, { - validator: createAllValidValidator(), + validator: createAllValidValidator(nil), gr: grInvalidWithUnsupportedField, expected: &L7Route{ RouteType: RouteTypeGRPC, @@ -1199,7 +1307,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, + FeatureFlags{ + Plus: test.plus, + Experimental: test.experimental, + }, + ) g.Expect(helpers.Diff(test.expected, route)).To(BeEmpty()) }) } @@ -1351,11 +1468,21 @@ func TestBuildGRPCRouteWithMirrorRoutes(t *testing.T) { g := NewWithT(t) + featureFlags := FeatureFlags{ + 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,7 +1493,7 @@ 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 @@ -1523,7 +1650,7 @@ func TestProcessGRPCRouteRule_UnsupportedFields(t *testing.T) { Type: helpers.GetPointer(v1.SessionPersistenceType("unsupported-session-persistence")), }), }, - expectedErrors: 2, + expectedErrors: 3, }, } @@ -1536,7 +1663,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, + FeatureFlags{ + Plus: false, + Experimental: false, + }, + ) if len(unsupportedFieldsErrors) > 0 { errors.warn = append(errors.warn, unsupportedFieldsErrors...) } @@ -1555,6 +1689,8 @@ func TestProcessGRPCRouteRules_UnsupportedFields(t *testing.T) { expectedConds []conditions.Condition expectedWarns int expectedValid bool + plusEnabled bool + experimental bool }{ { name: "No unsupported fields", @@ -1582,17 +1718,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 { @@ -1600,10 +1774,20 @@ 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, + grpcRouteNsName, + FeatureFlags{ + 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..c20c753d70 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 FeatureFlags, ) *L7Route { r := &L7Route{ Source: ghr, @@ -63,12 +64,17 @@ 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, ) r.Spec.Rules = rules @@ -84,6 +90,7 @@ func buildHTTPMirrorRoutes( route *v1.HTTPRoute, gateways map[types.NamespacedName]*Gateway, snippetsFilters map[types.NamespacedName]*SnippetsFilter, + featureFlags FeatureFlags, ) { for idx, rule := range l7route.Spec.Rules { if rule.Filters.Valid { @@ -121,6 +128,7 @@ func buildHTTPMirrorRoutes( gateways, snippetsFilters, nil, + featureFlags, ) if mirrorRoute != nil { @@ -171,15 +179,17 @@ 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, + routeNsName types.NamespacedName, + featureFlags FeatureFlags, ) (RouteRule, routeRuleErrors) { - var errors routeRuleErrors + rulePath := field.NewPath("spec").Child("rules").Index(ruleIdx) - unsupportedFieldsErrors := checkForUnsupportedHTTPFields(specRule, rulePath) + var errors routeRuleErrors + unsupportedFieldsErrors := checkForUnsupportedHTTPFields(specRule, rulePath, featureFlags) if len(unsupportedFieldsErrors) > 0 { errors.warn = append(errors.warn, unsupportedFieldsErrors...) } @@ -202,13 +212,63 @@ func processHTTPRouteRule( validator, resolveExtRefFunc, ) - errors = errors.append(filterErrors) - backendRefs := make([]RouteBackendRef, 0, len(specRule.BackendRefs)) + 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 { + 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 +} + +func getBackendRefs( + 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(routeRule.BackendRefs)) // rule.BackendRefs are validated separately because of their special requirements - for _, b := range specRule.BackendRefs { + for _, b := range routeRule.BackendRefs { var interfaceFilters []any if len(b.Filters) > 0 { interfaceFilters = make([]any, 0, len(b.Filters)) @@ -218,7 +278,8 @@ func processHTTPRouteRule( } rbr := RouteBackendRef{ - BackendRef: b.BackendRef, + BackendRef: b.BackendRef, + SessionPersistence: sp, } // If route specifies an InferencePool backend, we need to convert it to its associated @@ -228,7 +289,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(routeRule.BackendRefs) > 1 { err := field.Forbidden( rulePath.Child("backendRefs"), "cannot use InferencePool backend when multiple backendRefs are specified in a single rule", @@ -256,28 +317,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( @@ -285,7 +325,8 @@ func processHTTPRouteRules( validator validation.HTTPFieldsValidator, resolveExtRefFunc resolveExtRefFilter, inferencePools map[types.NamespacedName]*inference.InferencePool, - routeNamespace string, + routeNsName types.NamespacedName, + featureFlags FeatureFlags, ) (rules []RouteRule, valid bool, conds []conditions.Condition) { rules = make([]RouteRule, len(specRules)) @@ -294,16 +335,15 @@ 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, ) if rr.ValidMatches && rr.Filters.Valid { @@ -312,7 +352,7 @@ func processHTTPRouteRules( allRulesErrors = allRulesErrors.append(errors) - rules[i] = rr + rules[ruleIdx] = rr } conds = make([]conditions.Condition, 0, 2) @@ -608,7 +648,11 @@ func validateFilterRewrite( return allErrs } -func checkForUnsupportedHTTPFields(rule v1.HTTPRouteRule, rulePath *field.Path) field.ErrorList { +func checkForUnsupportedHTTPFields( + rule v1.HTTPRouteRule, + rulePath *field.Path, + featureFlags FeatureFlags, +) field.ErrorList { var ruleErrors field.ErrorList if rule.Name != nil { @@ -629,10 +673,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..f522ff30d6 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{ @@ -130,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() @@ -161,8 +197,17 @@ func TestBuildHTTPRoutes(t *testing.T) { RequestRedirect: &gatewayv1.HTTPRequestRedirectFilter{}, } - addFilterToPath(hr, "/", snippetsFilterRef) - addFilterToPath(hr, "/", requestRedirectFilter) + spConfig := &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, spConfig) + addElementsToPath(hr, "/", requestRedirectFilter, nil) hrWrongGateway := createHTTPRoute("hr-2", "some-gateway", "example.com", "/") @@ -238,7 +283,7 @@ func TestBuildHTTPRoutes(t *testing.T) { }, }, Matches: hr.Spec.Rules[0].Matches, - RouteBackendRefs: []RouteBackendRef{expRouteBackendRef}, + RouteBackendRefs: []RouteBackendRef{getExpRouteBackendRefForPath("/", "hr-1_test_0")}, }, }, }, @@ -253,7 +298,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 +320,16 @@ func TestBuildHTTPRoutes(t *testing.T) { } routes := buildRoutesForGateways( - validator, + createAllValidValidator(), hrRoutes, map[types.NamespacedName]*gatewayv1.GRPCRoute{}, test.gateways, snippetsFilters, nil, + FeatureFlags{ + Plus: true, + Experimental: true, + }, ) g.Expect(helpers.Diff(test.expected, routes)).To(BeEmpty()) }) @@ -305,13 +358,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.PermanentCookieLifetimeType)), + }, + } // 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 +390,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 +401,12 @@ func TestBuildHTTPRoute(t *testing.T) { "/filter", "/", ) - addFilterToPath(hrDroppedInvalidMatchesAndInvalidFilters, "/filter", invalidFilter) + addElementsToPath(hrDroppedInvalidMatchesAndInvalidFilters, "/filter", invalidFilter, sp) // 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, sp) + addElementsToPath(hrDroppedInvalidFilters, "/", invalidFilter, sp) // route with duplicate section names hrDuplicateSectionName := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/") @@ -364,7 +425,7 @@ func TestBuildHTTPRoute(t *testing.T) { Name: "sf", }, } - addFilterToPath(hrValidSnippetsFilter, "/filter", validSnippetsFilterExtRef) + addElementsToPath(hrValidSnippetsFilter, "/filter", validSnippetsFilterExtRef, sp) // route with invalid snippets filter extension ref hrInvalidSnippetsFilter := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/filter") @@ -376,7 +437,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 +449,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", "/") @@ -402,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{ @@ -423,16 +487,30 @@ func TestBuildHTTPRoute(t *testing.T) { } return nil }, + ValidateDurationStub: func(_ string) (string, error) { + return "1h", nil + }, + } + + 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 - hr *gatewayv1.HTTPRoute - expected *L7Route - name string + validator *validationfakes.FakeHTTPFieldsValidator + hr *gatewayv1.HTTPRoute + expected *L7Route + name string + plus, experimental bool }{ { - validator: &validationfakes.FakeHTTPFieldsValidator{}, + validator: createHTTPValidValidator(sp.AbsoluteTimeout), hr: hr, expected: &L7Route{ RouteType: RouteTypeHTTP, @@ -465,12 +543,14 @@ func TestBuildHTTPRoute(t *testing.T) { Filters: convertHTTPRouteFilters(hr.Spec.Rules[1].Filters), }, Matches: hr.Spec.Rules[1].Matches, - RouteBackendRefs: []RouteBackendRef{expRouteBackendRef}, + RouteBackendRefs: []RouteBackendRef{getExpRouteBackendRefForPath("/filter", "hr_test_1")}, }, }, }, }, - name: "normal case", + plus: true, + experimental: true, + name: "normal case", }, { validator: &validationfakes.FakeHTTPFieldsValidator{}, @@ -705,7 +785,6 @@ func TestBuildHTTPRoute(t *testing.T) { }, name: "dropped invalid rule with invalid matches", }, - { validator: validatorInvalidFieldsInRule, hr: hrDroppedInvalidMatchesAndInvalidFilters, @@ -749,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, @@ -763,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, @@ -796,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, @@ -805,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, @@ -847,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, @@ -1053,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{}, @@ -1108,8 +1195,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, + FeatureFlags{ + Plus: test.plus, + Experimental: test.experimental, + }, + ) g.Expect(helpers.Diff(test.expected, route)).To(BeEmpty()) }) } @@ -1151,8 +1247,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 +1336,23 @@ func TestBuildHTTPRouteWithMirrorRoutes(t *testing.T) { g := NewWithT(t) + featureFlags := FeatureFlags{ + 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()) @@ -1259,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 @@ -1271,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)), }, }, } @@ -1297,15 +1405,18 @@ 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, + routeNsName, + FeatureFlags{ + Plus: false, + Experimental: false, + }, ) g.Expect(routeRule.RouteBackendRefs).To(BeEmpty()) @@ -1920,7 +2031,7 @@ func TestUnsupportedFieldsErrors(t *testing.T) { Type: helpers.GetPointer(gatewayv1.SessionPersistenceType("unsupported-session-persistence")), }), }, - expectedErrors: 4, + expectedErrors: 5, }, } @@ -1932,7 +2043,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, + FeatureFlags{ + Plus: false, + Experimental: false, + }, + ) if len(unsupportedFieldsErrors) > 0 { errors.warn = append(errors.warn, unsupportedFieldsErrors...) } @@ -1944,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 @@ -1952,6 +2073,8 @@ func TestProcessHTTPRouteRules_UnsupportedFields(t *testing.T) { expectedConds []conditions.Condition expectedWarns int expectedValid bool + plusEnabled bool + experimental bool }{ { name: "No unsupported fields", @@ -1983,7 +2106,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 +2115,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 { @@ -2007,7 +2169,11 @@ func TestProcessHTTPRouteRules_UnsupportedFields(t *testing.T) { validation.SkipValidator{}, nil, nil, - routeNamespace, + routeNsName, + FeatureFlags{ + Plus: test.plusEnabled, + Experimental: test.experimental, + }, ) g.Expect(valid).To(Equal(test.expectedValid)) 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 c6bf225f43..b65bde0c52 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. @@ -169,12 +170,30 @@ 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. 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 + // 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 +} + // CreateRouteKey takes a client.Object and creates a RouteKey. func CreateRouteKey(obj client.Object) RouteKey { nsName := types.NamespacedName{ @@ -256,6 +275,7 @@ func buildRoutesForGateways( gateways map[types.NamespacedName]*Gateway, snippetsFilters map[types.NamespacedName]*SnippetsFilter, inferencePools map[types.NamespacedName]*inference.InferencePool, + featureFlags FeatureFlags, ) map[RouteKey]*L7Route { if len(gateways) == 0 { return nil @@ -264,7 +284,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 +292,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 +304,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 +1170,165 @@ 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, + 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 "" + } +} diff --git a/internal/controller/state/graph/route_common_test.go b/internal/controller/state/graph/route_common_test.go index 52ff2ca2af..94e0f247da 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,448 @@ 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)) + }) + } +} 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 }