diff --git a/internal/controller/nginx/config/maps.go b/internal/controller/nginx/config/maps.go index 2be256c776..ac903f1433 100644 --- a/internal/controller/nginx/config/maps.go +++ b/internal/controller/nginx/config/maps.go @@ -2,6 +2,7 @@ package config import ( "fmt" + "sort" "strings" gotemplate "text/template" @@ -185,7 +186,7 @@ func createAddHeadersMap(name string) shared.Map { // buildInferenceMaps creates maps for InferencePool Backends. func buildInferenceMaps(groups []dataplane.BackendGroup) []shared.Map { - inferenceMaps := make([]shared.Map, 0, len(groups)) + uniqueMaps := make(map[string]shared.Map) for _, group := range groups { for _, backend := range group.Backends { @@ -193,6 +194,13 @@ func buildInferenceMaps(groups []dataplane.BackendGroup) []shared.Map { continue } + backendVarName := strings.ReplaceAll(backend.UpstreamName, "-", "_") + mapKey := backendVarName // Use this as the key to detect duplicates + + // Skip if we've already processed this upstream + if _, exists := uniqueMaps[mapKey]; exists { + continue + } // Decide what the map must return when the picker didn’t set a value. var defaultResult string switch backend.EndpointPickerConfig.EndpointPickerRef.FailureMode { @@ -230,14 +238,26 @@ func buildInferenceMaps(groups []dataplane.BackendGroup) []shared.Map { Result: defaultResult, }) - backendVarName := strings.ReplaceAll(backend.UpstreamName, "-", "_") - - inferenceMaps = append(inferenceMaps, shared.Map{ + uniqueMaps[mapKey] = shared.Map{ Source: `$inference_workload_endpoint`, Variable: fmt.Sprintf("$inference_backend_%s", backendVarName), Parameters: params, - }) + } } } + + // Sort the map keys to ensure deterministic ordering + mapKeys := make([]string, 0, len(uniqueMaps)) + for key := range uniqueMaps { + mapKeys = append(mapKeys, key) + } + sort.Strings(mapKeys) + + // Build the result slice in sorted order + inferenceMaps := make([]shared.Map, 0, len(uniqueMaps)) + for _, key := range mapKeys { + inferenceMaps = append(inferenceMaps, uniqueMaps[key]) + } + return inferenceMaps } diff --git a/internal/controller/nginx/config/maps_test.go b/internal/controller/nginx/config/maps_test.go index bac84b0067..46fa1fca5a 100644 --- a/internal/controller/nginx/config/maps_test.go +++ b/internal/controller/nginx/config/maps_test.go @@ -397,55 +397,183 @@ func TestCreateStreamMapsWithEmpty(t *testing.T) { func TestBuildInferenceMaps(t *testing.T) { t.Parallel() - g := NewWithT(t) - group := dataplane.BackendGroup{ - Backends: []dataplane.Backend{ - { - UpstreamName: "upstream1", - EndpointPickerConfig: &dataplane.EndpointPickerConfig{ - NsName: "default", - EndpointPickerRef: &inference.EndpointPickerRef{ - FailureMode: inference.EndpointPickerFailClose, + tests := []struct { + expectedConfig map[string]struct { + failureMode inference.EndpointPickerFailureMode + defaultResult string + } + name string + backendGroups []dataplane.BackendGroup + expectedMaps int + }{ + { + name: "unique backends with different failure modes, result is ordered by upstream name", + backendGroups: []dataplane.BackendGroup{ + { + Backends: []dataplane.Backend{ + { + UpstreamName: "upstream2", + EndpointPickerConfig: &dataplane.EndpointPickerConfig{ + NsName: "default", + EndpointPickerRef: &inference.EndpointPickerRef{ + FailureMode: inference.EndpointPickerFailOpen, + }, + }, + }, + { + UpstreamName: "upstream1", + EndpointPickerConfig: &dataplane.EndpointPickerConfig{ + NsName: "default", + EndpointPickerRef: &inference.EndpointPickerRef{ + FailureMode: inference.EndpointPickerFailClose, + }, + }, + }, + { + UpstreamName: "upstream3", + EndpointPickerConfig: nil, + }, }, }, }, - { - UpstreamName: "upstream2", - EndpointPickerConfig: &dataplane.EndpointPickerConfig{ - NsName: "default", - EndpointPickerRef: &inference.EndpointPickerRef{ - FailureMode: inference.EndpointPickerFailOpen, + expectedMaps: 2, + expectedConfig: map[string]struct { + failureMode inference.EndpointPickerFailureMode + defaultResult string + }{ + "upstream1": { + failureMode: inference.EndpointPickerFailClose, + defaultResult: "invalid-backend-ref", + }, + "upstream2": { + failureMode: inference.EndpointPickerFailOpen, + defaultResult: "upstream2", + }, + }, + }, + { + name: "duplicate upstreams should be deduplicated", + backendGroups: []dataplane.BackendGroup{ + { + Backends: []dataplane.Backend{ + { + UpstreamName: "upstream1", + EndpointPickerConfig: &dataplane.EndpointPickerConfig{ + NsName: "default", + EndpointPickerRef: &inference.EndpointPickerRef{ + FailureMode: inference.EndpointPickerFailClose, + }, + }, + }, + { + UpstreamName: "upstream1", // Duplicate + EndpointPickerConfig: &dataplane.EndpointPickerConfig{ + NsName: "default", + EndpointPickerRef: &inference.EndpointPickerRef{ + FailureMode: inference.EndpointPickerFailClose, + }, + }, + }, + }, + }, + { + Backends: []dataplane.Backend{ + { + UpstreamName: "upstream1", // Another duplicate + EndpointPickerConfig: &dataplane.EndpointPickerConfig{ + NsName: "default", + EndpointPickerRef: &inference.EndpointPickerRef{ + FailureMode: inference.EndpointPickerFailClose, + }, + }, + }, + { + UpstreamName: "upstream2", + EndpointPickerConfig: &dataplane.EndpointPickerConfig{ + NsName: "default", + EndpointPickerRef: &inference.EndpointPickerRef{ + FailureMode: inference.EndpointPickerFailOpen, + }, + }, + }, }, }, }, - { - UpstreamName: "upstream3", - EndpointPickerConfig: nil, + expectedMaps: 2, // Only 2 unique upstreams + expectedConfig: map[string]struct { + failureMode inference.EndpointPickerFailureMode + defaultResult string + }{ + "upstream1": { + failureMode: inference.EndpointPickerFailClose, + defaultResult: "invalid-backend-ref", + }, + "upstream2": { + failureMode: inference.EndpointPickerFailOpen, + defaultResult: "upstream2", + }, }, }, + { + name: "no endpoint picker configs", + backendGroups: []dataplane.BackendGroup{ + { + Backends: []dataplane.Backend{ + { + UpstreamName: "upstream1", + EndpointPickerConfig: nil, + }, + { + UpstreamName: "upstream2", + EndpointPickerConfig: nil, + }, + }, + }, + }, + expectedMaps: 0, + }, } - maps := buildInferenceMaps([]dataplane.BackendGroup{group}) - g.Expect(maps).To(HaveLen(2)) - g.Expect(maps[0].Source).To(Equal("$inference_workload_endpoint")) - g.Expect(maps[0].Variable).To(Equal("$inference_backend_upstream1")) - g.Expect(maps[0].Parameters).To(HaveLen(3)) - g.Expect(maps[0].Parameters[0].Value).To(Equal("\"\"")) - g.Expect(maps[0].Parameters[0].Result).To(Equal("upstream1")) - g.Expect(maps[0].Parameters[1].Value).To(Equal("~.+")) - g.Expect(maps[0].Parameters[1].Result).To(Equal("$inference_workload_endpoint")) - g.Expect(maps[0].Parameters[2].Value).To(Equal("default")) - g.Expect(maps[0].Parameters[2].Result).To(Equal("invalid-backend-ref")) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + maps := buildInferenceMaps(tc.backendGroups) + g.Expect(maps).To(HaveLen(tc.expectedMaps)) - // Check the second map - g.Expect(maps[1].Source).To(Equal("$inference_workload_endpoint")) - g.Expect(maps[1].Variable).To(Equal("$inference_backend_upstream2")) - g.Expect(maps[1].Parameters).To(HaveLen(3)) - g.Expect(maps[1].Parameters[0].Value).To(Equal("\"\"")) - g.Expect(maps[1].Parameters[0].Result).To(Equal("upstream2")) - g.Expect(maps[1].Parameters[1].Value).To(Equal("~.+")) - g.Expect(maps[1].Parameters[1].Result).To(Equal("$inference_workload_endpoint")) - g.Expect(maps[1].Parameters[2].Value).To(Equal("default")) - g.Expect(maps[1].Parameters[2].Result).To(Equal("upstream2")) + // Verify each map has the correct structure + seenUpstreams := make(map[string]bool) + for _, m := range maps { + g.Expect(m.Source).To(Equal("$inference_workload_endpoint")) + g.Expect(m.Parameters).To(HaveLen(3)) + + // Extract upstream name from variable name + varName := strings.TrimPrefix(m.Variable, "$inference_backend_") + upstreamName := strings.ReplaceAll(varName, "_", "-") + + // Verify we haven't seen this upstream before (no duplicates) + g.Expect(seenUpstreams[upstreamName]).To(BeFalse(), "Duplicate upstream found: %s", upstreamName) + seenUpstreams[upstreamName] = true + + // Verify parameter structure + g.Expect(m.Parameters[0].Value).To(Equal("\"\"")) + g.Expect(m.Parameters[0].Result).To(Equal(upstreamName)) + g.Expect(m.Parameters[1].Value).To(Equal("~.+")) + g.Expect(m.Parameters[1].Result).To(Equal("$inference_workload_endpoint")) + g.Expect(m.Parameters[2].Value).To(Equal("default")) + + // Verify the default result matches expected failure mode + if expectedConfig, exists := tc.expectedConfig[upstreamName]; exists { + g.Expect(m.Parameters[2].Result).To(Equal(expectedConfig.defaultResult)) + } + } + + // Verify all expected upstreams are present + for expectedUpstream := range tc.expectedConfig { + g.Expect(seenUpstreams[expectedUpstream]).To(BeTrue(), "Expected upstream not found: %s", expectedUpstream) + } + }) + } } diff --git a/internal/controller/nginx/config/servers.go b/internal/controller/nginx/config/servers.go index 717664c87c..578cf5e2b2 100644 --- a/internal/controller/nginx/config/servers.go +++ b/internal/controller/nginx/config/servers.go @@ -9,6 +9,8 @@ import ( "strings" gotemplate "text/template" + "k8s.io/apimachinery/pkg/types" + "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/http" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/policies" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/shared" @@ -258,6 +260,7 @@ func extractMirrorTargetsWithPercentages(pathRules []dataplane.PathRule) map[str return mirrorTargets } +//nolint:lll /* There are several different flows of location blocks, depending on the user configuration. The following describes them, with basic location examples. @@ -281,12 +284,12 @@ location /coffee { js_content httpmatches.match; // chooses backend1 or backend2, and redirects to appropriate internal location } location /_ngf-internal-rule0-route0 { - internal; - proxy_pass http://backend1; + internal; + proxy_pass http://backend1; } location /_ngf-internal-rule1-route0 { - internal; - proxy_pass http://backend2; + internal; + proxy_pass http://backend2; } --------------- Inference extension, no HTTP matching conditions. @@ -295,12 +298,45 @@ External location calls inference NJS module. The module gets the AI endpoint to then redirects to the internal inference location that proxies to the backend. location /coffee { - set $epp_internal_path /_ngf-internal-rule0-route0-inference; - js_content epp.getEndpoint; // gets endpoint and redirects to /_ngf-internal-rule0-route0-inference + set $epp_internal_path /_ngf-internal-proxy-pass-rule0-route0-backend0-inference; + js_content epp.getEndpoint; // gets endpoint and redirects to /_ngf-internal-proxy-pass-rule0-route0-backend0-inference +} +location /_ngf-internal-proxy-pass-rule0-route0-backend0-inference { + internal; + proxy_pass http://$inference_backend_test_foo_80; +} +--------------- +Inference extension with multiple inference backends. + +location /coffee { + rewrite ^ $inference_backend_group_routeNS__routeName_rule0_pathRule0 last; +} + +location /_ngf-internal-proxy-pass-rule0-route0-backend0-inference { + internal; + proxy_pass http://$inference_backend_test_primary_pool_80; +} + +location /_ngf-internal-test_primary_pool_80-routeNS-routeName-routeRule0-pathRule0 { + internal; + set $epp_internal_path /_ngf-internal-proxy-pass-rule0-route0-backend0-inference; + js_content epp.getEndpoint; // gets endpoint and redirects to /_ngf-internal-proxy-pass-rule0-route0-backend0-inference } -location /_ngf-internal-rule0-route0-inference { - internal; - proxy_pass http://$inference-backend; + +location /_ngf-internal-proxy-pass-rule0-route0-backend1-inference { + internal; + proxy_pass http://$inference_backend_test_secondary_pool_80; +} + +location /_ngf-internal-test_secondary_pool_80-routeNS-routeName-routeRule0-pathRule0 { + internal; + set $epp_internal_path /_ngf-internal-proxy-pass-rule0-route0-backend1-inference; + js_content epp.getEndpoint; // gets endpoint and redirects to /_ngf-internal-proxy-pass-rule0-route0-backend1-inference +} + +split_clients $request_id $inference_backend_group_routeNS__routeName_rule0_pathRule0 { + 70.00% /_ngf-internal-test_primary_pool_80-routeNS-routeName-routeRule0-pathRule0; + 30.00% /_ngf-internal-test_secondary_pool_80-routeNS-routeName-routeRule0-pathRule0; } --------------- Inference extension with HTTP matching conditions. @@ -310,23 +346,60 @@ and which backend to use, then redirects to the internal inference location. The location calls the inference NJS module to get the AI endpoint to proxy to, then redirects to the internal location that proxies to the backend. -Note that the location path naming here is a little different than the previous example. -The final location that proxy_passes has the non-inference name to avoid too much refactoring -in the code, and the intermediate location has -inference in the name, whereas in the previous example -it was the final location that had -inference in the name. +location /coffee { + js_content httpmatches.match; // chooses backend and redirects to appropriate internal inference location +} +location /_ngf-internal-test_foo_80-routeNS-routeName-routeRule0-pathRule0 { + internal; + set $epp_internal_path /_ngf-internal-proxy-pass-rule0-route0-backend0-inference; + js_content epp.getEndpoint; // redirects to /_ngf-internal-proxy-pass-rule0-route0-backend0-inference +} +location /_ngf-internal-proxy-pass-rule0-route0-backend0-inference { + internal; + proxy_pass http://$inference_backend_test_foo_80; +} +--------------- +Inference extension with multiple backends with HTTP matching conditions. + +External location calls httpmatch NJS module. The module determines the HTTP request conditions that exist +and which backend to use, then redirects to an internal location which will rewrite to another internal inference +location based on a split clients variable. That internal inference location calls the inference NJS module +to get the AI endpoint to proxy to, then redirects to the internal location that proxies to the backend. location /coffee { - js_content httpmatches.match; // chooses backend and redirects to appropriate internal inference location + js_content httpmatches.match; // chooses backend and redirects to appropriate internal inference location } -location /_ngf-internal-rule0-route0-inference { - internal; - set $epp_internal_path /_ngf-internal-rule0-route0; - js_content epp.getEndpoint; // redirects to /_ngf-internal-rule0-route0 +location /_ngf-internal-split-clients-rule0-route0-inference { + internal; + rewrite ^ $inference_backend_group_routeNS__routeName_rule0_pathRule0 last; } -location /_ngf-internal-rule0-route0 { - internal; - proxy_pass http://$inference-backend; + +location /_ngf-internal-proxy-pass-rule0-route0-backend0-inference { + internal; + proxy_pass http://$inference_backend_test_primary_pool_80; +} + +location /_ngf-internal-test_primary_pool_80-routeNS-routeName-routeRule0-pathRule0 { + internal; + set $epp_internal_path /_ngf-internal-proxy-pass-rule0-route0-backend0-inference; + js_content epp.getEndpoint; // gets endpoint and redirects to /_ngf-internal-proxy-pass-rule0-route0-backend0-inference +} + +location /_ngf-internal-proxy-pass-rule0-route0-backend1-inference { + internal; + proxy_pass http://$inference_backend_test_secondary_pool_80; +} + +location /_ngf-internal-test_secondary_pool_80-routeNS-routeName-routeRule0-pathRule0 { + internal; + set $epp_internal_path /_ngf-internal-proxy-pass-rule0-route0-backend1-inference; + js_content epp.getEndpoint; // gets endpoint and redirects to /_ngf-internal-proxy-pass-rule0-route0-backend1-inference +} + +split_clients $request_id $inference_backend_group_routeNS__routeName_rule0_pathRule0 { + 70.00% /_ngf-internal-test_primary_pool_80-routeNS-routeName-routeRule0-pathRule0; + 30.00% /_ngf-internal-test_secondary_pool_80-routeNS-routeName-routeRule0-pathRule0; } */ @@ -441,46 +514,151 @@ func createInternalLocationsForRule( keepAliveCheck keepAliveChecker, mirrorPercentage *float64, ) ([]http.Location, []routeMatch) { - internalLocations := make([]http.Location, 0, len(rule.MatchRules)) + // Calculate the exact capacity needed + capacity := 0 + for _, r := range rule.MatchRules { + if !rule.HasInferenceBackends { + capacity++ // intLocation (always created for non-inference) + } else { + // For inference backends with matches + if len(r.BackendGroup.Backends) > 1 { + capacity++ // intSplitClientsLocation (created for multiple backends) + } + + capacity += len(r.BackendGroup.Backends) * 2 // intEPPLocation and intProxyPassLocation per backend + } + } + + internalLocations := make([]http.Location, 0, capacity) matches := make([]routeMatch, 0, len(rule.MatchRules)) + + // If there are multiple matches on a single route rule, they will share the same intEPPLocation and + // intProxyPassLocation. To avoid creating duplicates, we track the unique names here. + uniqueEPPNameMap := make(map[string]struct{}) + for matchRuleIdx, r := range rule.MatchRules { var intLocation http.Location var match routeMatch + skipMatch := false + if !rule.HasInferenceBackends { intLocation, match = initializeInternalMatchLocation(pathRuleIdx, matchRuleIdx, r.Match, rule.GRPC) + intLocation.Includes = createIncludesFromPolicyGenerateResult( + generator.GenerateForInternalLocation(rule.Policies), + ) + intLocation = updateLocation( + r, + rule, + intLocation, + port, + keepAliveCheck, + mirrorPercentage, + ) + internalLocations = append(internalLocations, intLocation) } else { - intLocation, match = initializeInternalMatchLocationWithInference(pathRuleIdx, matchRuleIdx, r.Match) - intInfLocation := initializeInternalInferenceRedirectLocation(pathRuleIdx, matchRuleIdx) - for _, b := range r.BackendGroup.Backends { + // If there are multiple inference backends, we need 4 locations: + // 1. (external location) external match location which redirects to split clients location + // 2. (intSplitClientsLocation) internal split clients location which rewrites to internal inference + // location based on SC variable + // 3. (intEPPLocation) internal inference location which calls the EPP NJS module to get endpoint and + // redirects to final internal location + // 4. (intProxyPassLocation) final internal inference location which proxy_passes to backend + // + // The match needs to point to the internal split clients location (intSplitClientsLocation) + // + // If there is only one inference backend, we need 3 locations: + // 1. (external location) external match location which redirects to internal inference location + // 2. (intEPPLocation) internal inference location which calls the EPP NJS module to get endpoint and + // redirects to final internal location + // 3. (intProxyPassLocation) final internal inference location which proxy_passes to backend + // + // The match needs to point to the internal inference location which calls the EPP NJS module (intEPPLocation) + + var intEPPLocation http.Location + for backendIdx, b := range r.BackendGroup.Backends { + intProxyPassLocation := initializeInternalInferenceProxyPassLocation(pathRuleIdx, matchRuleIdx, backendIdx) + intProxyPassLocation.Includes = createIncludesFromPolicyGenerateResult( + generator.GenerateForInternalLocation(rule.Policies), + ) + + // Since we are creating a separate intProxyPassLocation per backend, + // we need to update the rule to only have that backend for the location. + // This ensures the correct name gets generated to correlate with the split clients generation. + // If there is only one backend, this is effectively a no-op. + tempRule := dataplane.MatchRule{ + Source: r.Source, + Match: r.Match, + Filters: r.Filters, + BackendGroup: dataplane.BackendGroup{ + Source: r.BackendGroup.Source, + RuleIdx: r.BackendGroup.RuleIdx, + PathRuleIdx: r.BackendGroup.PathRuleIdx, + Backends: []dataplane.Backend{b}, // Only include the current backend + }, + } + intProxyPassLocation = updateLocation( + tempRule, + rule, + intProxyPassLocation, + port, + keepAliveCheck, + mirrorPercentage, + ) + + intEPPLocation = initializeInternalInferenceEPPLocation( + b, + r.BackendGroup.Source, + r.BackendGroup.RuleIdx, + r.BackendGroup.PathRuleIdx, + ) + intEPPLocation.Includes = createIncludesFromPolicyGenerateResult( + generator.GenerateForInternalLocation(rule.Policies), + ) + + mapKey := intEPPLocation.Path // Use this as the key to detect duplicates + + // The only time this happens is on a single route rule with multiple matches with the same path. + // In this case, we only need to create one set of internal locations, and can skip the duplicate matches. + if _, exists := uniqueEPPNameMap[mapKey]; exists { + skipMatch = true + break + } + uniqueEPPNameMap[mapKey] = struct{}{} + // we only append intEPPLocation and intProxyPassLocation once per unique intEPPLocation name + internalLocations = append(internalLocations, intProxyPassLocation) + if b.EndpointPickerConfig != nil && b.EndpointPickerConfig.EndpointPickerRef != nil { - eppRef := b.EndpointPickerConfig.EndpointPickerRef - var portNum int - if eppRef.Port != nil { - portNum = int(eppRef.Port.Number) - } - intInfLocation.EPPInternalPath = intLocation.Path - if b.EndpointPickerConfig.NsName != "" { - intInfLocation.EPPHost = string(eppRef.Name) + "." + b.EndpointPickerConfig.NsName - } else { - intInfLocation.EPPHost = string(eppRef.Name) - } - intInfLocation.EPPPort = portNum + eppHost, portNum := extractEPPConfig(b) + intEPPLocation = setLocationEPPConfig(intEPPLocation, intProxyPassLocation.Path, eppHost, portNum) + internalLocations = append(internalLocations, intEPPLocation) } } - internalLocations = append(internalLocations, intInfLocation) + + // skip adding match and creating split clients location if it's a duplicate intEPPLocation.Path + if skipMatch { + continue + } + + if len(r.BackendGroup.Backends) > 1 { + intSplitClientsLocation := initializeInternalInferenceSplitClientsLocation(pathRuleIdx, matchRuleIdx) + intSplitClientsLocation.Includes = createIncludesFromPolicyGenerateResult( + generator.GenerateForInternalLocation(rule.Policies), + ) + + splitClientsVariableName := createInferenceSplitClientsVariableName( + convertStringToSafeVariableName(r.BackendGroup.Name()), + ) + intSplitClientsLocation.Rewrites = append(intSplitClientsLocation.Rewrites, + fmt.Sprintf("^ $%s last", splitClientsVariableName)) + + internalLocations = append(internalLocations, intSplitClientsLocation) + + match = createRouteMatch(r.Match, intSplitClientsLocation.Path) + } else { + match = createRouteMatch(r.Match, intEPPLocation.Path) + } } - intLocation.Includes = createIncludesFromPolicyGenerateResult( - generator.GenerateForInternalLocation(rule.Policies), - ) - intLocation = updateLocation( - r, - rule, - intLocation, - port, - keepAliveCheck, - mirrorPercentage, - ) - internalLocations = append(internalLocations, intLocation) + matches = append(matches, match) } @@ -496,45 +674,132 @@ func createInferenceLocationsForRule( keepAliveCheck keepAliveChecker, mirrorPercentage *float64, ) []http.Location { - locs := make([]http.Location, 0, len(rule.MatchRules)+len(extLocations)) - for matchRuleIdx, r := range rule.MatchRules { - intLocation := initializeInternalInferenceLocation(pathRuleIdx, matchRuleIdx) - intLocation.Includes = createIncludesFromPolicyGenerateResult( - generator.GenerateForInternalLocation(rule.Policies), - ) - intLocation = updateLocation( - r, - rule, - intLocation, - port, - keepAliveCheck, - mirrorPercentage, - ) + capacity := len(extLocations) + + for _, r := range rule.MatchRules { for _, b := range r.BackendGroup.Backends { + capacity++ // intProxyPassLocation (always created) + + // intEPPLocation (created only for multiple backends with EPP config) + if len(r.BackendGroup.Backends) > 1 && + b.EndpointPickerConfig != nil && + b.EndpointPickerConfig.EndpointPickerRef != nil { + capacity++ // intEPPLocation + } + } + } + + locs := make([]http.Location, 0, capacity) + + // There will only be one rule.MatchRules, since if there are multiple, createInternalLocationsForRule + // would have been called instead. + for matchRuleIdx, r := range rule.MatchRules { + // If there are multiple inference backends, we need 3 locations: + // 1. (external location) external location which rewrites to the EPP internal location based on a + // split clients variable + // 2. (intEPPLocation) internal inference location which calls the EPP NJS module to get endpoint + // and redirects to final internal location + // 3. (intProxyPassLocation) final internal inference location which proxy_passes to backend + // + // If there is only one inference backend, we need 2 locations: + // 1. (external location) external location which calls the EPP NJS module to get endpoint and redirects + // to internal inference location + // 2. (intProxyPassLocation) final internal inference location which proxy_passes to backend + + if len(r.BackendGroup.Backends) > 1 { + splitClientsVariableName := createInferenceSplitClientsVariableName( + convertStringToSafeVariableName(r.BackendGroup.Name()), + ) + for i := range extLocations { + extLocations[i].Rewrites = append(extLocations[i].Rewrites, fmt.Sprintf("^ $%s last", splitClientsVariableName)) + extLocations[i].Type = http.ExternalLocationType + } + } + + for backendIdx, b := range r.BackendGroup.Backends { + intProxyPassLocation := initializeInternalInferenceProxyPassLocation(pathRuleIdx, matchRuleIdx, backendIdx) + intProxyPassLocation.Includes = createIncludesFromPolicyGenerateResult( + generator.GenerateForInternalLocation(rule.Policies), + ) + + // Since we are creating a separate intProxyPassLocation per backend, + // we need to update the rule to only have that backend for the location. + // This ensures the correct name gets generated to correlate with the split clients generation. + // If there is only one backend, this is effectively a no-op. + tempRule := dataplane.MatchRule{ + Source: r.Source, + Match: r.Match, + Filters: r.Filters, + BackendGroup: dataplane.BackendGroup{ + Source: r.BackendGroup.Source, + RuleIdx: r.BackendGroup.RuleIdx, + PathRuleIdx: r.BackendGroup.PathRuleIdx, + Backends: []dataplane.Backend{b}, // Only include the current backend + }, + } + intProxyPassLocation = updateLocation( + tempRule, + rule, + intProxyPassLocation, + port, + keepAliveCheck, + mirrorPercentage, + ) + locs = append(locs, intProxyPassLocation) + if b.EndpointPickerConfig != nil && b.EndpointPickerConfig.EndpointPickerRef != nil { - for i := range extLocations { - eppRef := b.EndpointPickerConfig.EndpointPickerRef - var portNum int - if eppRef.Port != nil { - portNum = int(eppRef.Port.Number) - } - extLocations[i].EPPInternalPath = intLocation.Path - if b.EndpointPickerConfig.NsName != "" { - extLocations[i].EPPHost = string(eppRef.Name) + "." + b.EndpointPickerConfig.NsName - } else { - extLocations[i].EPPHost = string(eppRef.Name) + eppHost, portNum := extractEPPConfig(b) + + if len(r.BackendGroup.Backends) > 1 { + intEPPLocation := initializeInternalInferenceEPPLocation( + b, + r.BackendGroup.Source, + r.BackendGroup.RuleIdx, + r.BackendGroup.PathRuleIdx, + ) + intEPPLocation.Includes = createIncludesFromPolicyGenerateResult( + generator.GenerateForInternalLocation(rule.Policies), + ) + intEPPLocation = setLocationEPPConfig(intEPPLocation, intProxyPassLocation.Path, eppHost, portNum) + locs = append(locs, intEPPLocation) + } else { + for i := range extLocations { + extLocations[i] = setLocationEPPConfig(extLocations[i], intProxyPassLocation.Path, eppHost, portNum) } - extLocations[i].EPPPort = portNum } } } - locs = append(locs, intLocation) } locs = append(locs, extLocations...) return locs } +func setLocationEPPConfig(location http.Location, eppInternalPath, eppHost string, eppPort int) http.Location { + location.EPPInternalPath = eppInternalPath + location.EPPHost = eppHost + location.EPPPort = eppPort + return location +} + +func extractEPPConfig(backend dataplane.Backend) (string, int) { + var eppHost string + var eppPort int + + eppRef := backend.EndpointPickerConfig.EndpointPickerRef + if eppRef.Port != nil { + eppPort = int(eppRef.Port.Number) + } + + if backend.EndpointPickerConfig.NsName != "" { + eppHost = string(eppRef.Name) + "." + backend.EndpointPickerConfig.NsName + } else { + eppHost = string(eppRef.Name) + } + + return eppHost, eppPort +} + func needsInternalLocationsForMatches(rule dataplane.PathRule) bool { if len(rule.MatchRules) > 1 { return true @@ -547,27 +812,79 @@ func needsInternalLocationsForMatches(rule dataplane.PathRule) bool { // for example, {/foo: {exact: {}, prefix: {}}}. type pathAndTypeMap map[string]map[dataplane.PathType]struct{} -// To calculate the maximum number of locations, we need to take into account the following: -// 1. Each match rule for a path rule will have one location. -// 2. Each path rule may have an additional location if it contains non-path-only matches. -// 3. Each prefix path rule may have an additional location if it doesn't contain trailing slash. -// 4. There may be an additional location for the default root path. -// 5. There may be an additional location per parent location for the inference extension. -// We also return a map of all paths and their types. func getMaxLocationCountAndPathMap(pathRules []dataplane.PathRule) (int, pathAndTypeMap) { - maxLocs := 1 + // To calculate the maximum number of locations, we need to take into account the following: + // 1. Each path rule will have at least one external location. + // 2. Each path rule may have an additional external location if it's a non-slashed prefix path. + // 3. There may be an additional location for the default root path. + // 4. For inference backends: + // - Single backend without matches: 2 locations (external EPP + internal proxy pass) + // - Single backend with matches: 3 locations (external redirect + internal EPP + internal proxy pass) + // - Multiple backends without matches: 1 external + (2 * numBackends) internal locations + // - Multiple backends with matches: 1 external + 1 split clients + (2 * numBackends) internal locations + // 5. For non-inference backends with matches: + // - Each match rule gets an internal location + // We also return a map of all paths and their types. + + maxLocs := 0 pathsAndTypes := make(pathAndTypeMap) + for _, rule := range pathRules { - maxLocs += (len(rule.MatchRules) * 2) + 2 + // External locations calculation + maxLocs++ // Base external location for the path + + // Add the path to the map if pathsAndTypes[rule.Path] == nil { - pathsAndTypes[rule.Path] = map[dataplane.PathType]struct{}{ - rule.PathType: {}, + pathsAndTypes[rule.Path] = make(map[dataplane.PathType]struct{}) + } + pathsAndTypes[rule.Path][rule.PathType] = struct{}{} + + // Check if we need an additional external location for non-slashed prefix paths + if isNonSlashedPrefixPath(rule.PathType, rule.Path) { + maxLocs++ // Additional external location for exact match + } + + // Determine if we need internal locations for matches + needsInternalMatches := needsInternalLocationsForMatches(rule) + + // Internal locations calculation + for _, matchRule := range rule.MatchRules { + if !rule.HasInferenceBackends { + // Non-inference backends with matches need internal locations + if needsInternalMatches { + maxLocs++ // Internal match location per match rule + } + } else { + // Inference backends calculation + numBackends := len(matchRule.BackendGroup.Backends) + + if needsInternalMatches { + // Has HTTP matching conditions + if numBackends > 1 { + // Multiple backends with matches: split clients + 2 locations per backend + maxLocs++ // Internal split clients location + maxLocs += numBackends * 2 // EPP + proxy pass per backend + } else { + // Single backend with matches: EPP + proxy pass + maxLocs += 2 + } + } else { + // No HTTP matching conditions + if numBackends > 1 { + // Multiple backends without matches: 2 locations per backend (no split clients for external) + maxLocs += numBackends * 2 // EPP + proxy pass per backend + } else { + // Single backend without matches: proxy pass only (external becomes EPP) + maxLocs++ // Just the internal proxy pass location + } + } } - } else { - pathsAndTypes[rule.Path][rule.PathType] = struct{}{} } } + // Add 1 for potential default root location + maxLocs++ + return maxLocs, pathsAndTypes } @@ -645,43 +962,71 @@ func initializeInternalMatchLocation( return createMatchLocation(path, grpc), createRouteMatch(match, path) } -// initializeInternalInferenceRedirectLocation initializes the internal inference location that is redirected to by -// an external HTTP matching location. This location then redirects to the final proxy_pass location. -func initializeInternalInferenceRedirectLocation(pathruleIdx, matchRuleIdx int) http.Location { +// initializeInternalInferenceEPPLocation initializes the internal inference EPP location. This location calls the +// inference njs module to get the correct endpoint for the request and redirects to the final internal location +// that does the proxy_pass to the backend. +func initializeInternalInferenceEPPLocation( + b dataplane.Backend, + source types.NamespacedName, + ruleIdx, + pathruleIdx int, +) http.Location { return http.Location{ - Path: inferencePath(pathruleIdx, matchRuleIdx), + // This path needs to be recreated in the split_clients directive generation to match correctly. + Path: generateInternalInferenceEPPLocationPath( + b.UpstreamName, + source, + ruleIdx, + pathruleIdx, + ), Type: http.InferenceInternalLocationType, } } -// initializeInternalMatchLocationWithInference initializes the internal location that is redirected to by -// an internal inference location, which was redirected to by the external HTTP matching location. -// This location will proxy_pass to the backend. -// The routeMatch is created with the inference internal location path, so that the HTTP match in the external -// location can redirect to the proper inference location, which then redirects to this location. -func initializeInternalMatchLocationWithInference( - pathruleIdx, - matchRuleIdx int, - match dataplane.Match, -) (http.Location, routeMatch) { - path := fmt.Sprintf("%s-rule%d-route%d", http.InternalRoutePathPrefix, pathruleIdx, matchRuleIdx) - grpc := false - - return createMatchLocation(path, grpc), createRouteMatch(match, inferencePath(pathruleIdx, matchRuleIdx)) +func generateInternalInferenceEPPLocationPath( + upstreamName string, + source types.NamespacedName, + ruleIdx int, + pathRuleIdx int, +) string { + return fmt.Sprintf( + "%s-%s-%s-%s-routeRule%d-pathRule%d", + http.InternalRoutePathPrefix, + upstreamName, + source.Namespace, + source.Name, + ruleIdx, + pathRuleIdx, + ) } -// initializeInternalInferenceLocation initializes the internal inference location that does the final +// initializeInternalInferenceProxyPassLocation initializes the internal inference location that does the final // proxy_pass to the inference backend. -// This is used when the external location redirects directly here, without any HTTP matching. -func initializeInternalInferenceLocation(pathruleIdx, matchRuleIdx int) http.Location { +func initializeInternalInferenceProxyPassLocation(pathruleIdx, matchRuleIdx, backendIdx int) http.Location { return http.Location{ - Path: inferencePath(pathruleIdx, matchRuleIdx), + Path: fmt.Sprintf( + "%s-proxy-pass-rule%d-route%d-backend%d-inference", + http.InternalRoutePathPrefix, + pathruleIdx, + matchRuleIdx, + backendIdx, + ), Type: http.InternalLocationType, } } -func inferencePath(pathruleIdx int, matchRuleIdx int) string { - return fmt.Sprintf("%s-rule%d-route%d-inference", http.InternalRoutePathPrefix, pathruleIdx, matchRuleIdx) +// initializeInternalInferenceSplitClientsLocation initializes the internal inference location that rewrites +// to a location determined by a split_clients variable. +func initializeInternalInferenceSplitClientsLocation(pathruleIdx, matchRuleIdx int) http.Location { + return http.Location{ + Path: fmt.Sprintf( + "%s-split-clients-rule%d-route%d-inference", + http.InternalRoutePathPrefix, + pathruleIdx, + matchRuleIdx, + ), + Type: http.InternalLocationType, + } } // updateLocation updates a location with any relevant configurations, like proxy_pass, filters, tls settings, etc. diff --git a/internal/controller/nginx/config/servers_template.go b/internal/controller/nginx/config/servers_template.go index 9575b77480..68a53d6a6b 100644 --- a/internal/controller/nginx/config/servers_template.go +++ b/internal/controller/nginx/config/servers_template.go @@ -104,7 +104,7 @@ server { {{- range $i := $l.Includes }} include {{ $i.Name }}; - {{- end -}} + {{- end }} {{ range $r := $l.Rewrites }} rewrite {{ $r }}; diff --git a/internal/controller/nginx/config/servers_test.go b/internal/controller/nginx/config/servers_test.go index 2e35224362..59f4303778 100644 --- a/internal/controller/nginx/config/servers_test.go +++ b/internal/controller/nginx/config/servers_test.go @@ -1433,7 +1433,7 @@ func TestCreateServers(t *testing.T) { }, { Path: "/_ngf-internal-rule1-route0", - ProxyPass: "http://$group_test__route1_rule1$request_uri", + ProxyPass: "http://$group_test__route1_rule1_pathRule0$request_uri", ProxySetHeaders: httpBaseHeaders, Type: http.InternalLocationType, Includes: internalIncludes, @@ -2447,53 +2447,136 @@ func TestCreateLocations_Includes(t *testing.T) { func TestCreateLocations_InferenceBackends(t *testing.T) { t.Parallel() - hrNsName := types.NamespacedName{Namespace: "test", Name: "route1"} + hrNsName := types.NamespacedName{Namespace: "testNS", Name: "routeName"} - fooGroup := dataplane.BackendGroup{ - Source: hrNsName, - RuleIdx: 0, - Backends: []dataplane.Backend{ - { - UpstreamName: "test_foo_80", - Valid: true, - Weight: 1, - EndpointPickerConfig: &dataplane.EndpointPickerConfig{ - EndpointPickerRef: &inference.EndpointPickerRef{ - Name: "test-epp", - Port: &inference.Port{ - Number: 80, - }, - }, - NsName: hrNsName.Namespace, + createInferenceBackend := func(upstreamName string, weight int32, eppName string) dataplane.Backend { + return dataplane.Backend{ + UpstreamName: upstreamName, + Valid: true, + Weight: weight, + EndpointPickerConfig: &dataplane.EndpointPickerConfig{ + EndpointPickerRef: &inference.EndpointPickerRef{ + Name: inference.ObjectName(eppName), + Port: &inference.Port{Number: 80}, }, + NsName: hrNsName.Namespace, }, - }, + } + } + + singleInferenceBackend := createInferenceBackend("test_foo_80", 1, "test-epp") + + multiInferencePrimaryBackend := createInferenceBackend("test_primary_pool_80", 70, "primary-pool") + multiInferenceSecondaryBackend := createInferenceBackend("test_secondary_pool_80", 30, "secondary-pool") + + createBackendGroup := func(backends []dataplane.Backend) dataplane.BackendGroup { + return dataplane.BackendGroup{ + Source: hrNsName, + Backends: backends, + } + } + + singleInferenceGroup := createBackendGroup([]dataplane.Backend{singleInferenceBackend}) + multipleInferenceGroup := createBackendGroup([]dataplane.Backend{ + multiInferencePrimaryBackend, + multiInferenceSecondaryBackend, + }) + + // setBackendGroupIndices returns a new PathRule with updated RuleIdx and PathRuleIdx in all BackendGroups + setBackendGroupIndices := func(pathRule dataplane.PathRule, ruleIdx int, pathRuleIdx int) dataplane.PathRule { + // Create a copy of the PathRule + newPathRule := pathRule + + // Create a copy of the MatchRules slice + newPathRule.MatchRules = make([]dataplane.MatchRule, len(pathRule.MatchRules)) + copy(newPathRule.MatchRules, pathRule.MatchRules) + + // Update the BackendGroup indices in the copied MatchRules + for matchRuleIdx := range newPathRule.MatchRules { + newPathRule.MatchRules[matchRuleIdx].BackendGroup.RuleIdx = ruleIdx + newPathRule.MatchRules[matchRuleIdx].BackendGroup.PathRuleIdx = pathRuleIdx + } + + return newPathRule } - pathRuleInferenceOnly := dataplane.PathRule{ - Path: "/inference", - PathType: dataplane.PathTypeExact, - HasInferenceBackends: true, - MatchRules: []dataplane.MatchRule{ + // Helper function to create path rules + createPathRule := func(path string, matchRules []dataplane.MatchRule) dataplane.PathRule { + return dataplane.PathRule{ + Path: path, + PathType: dataplane.PathTypeExact, + HasInferenceBackends: true, + MatchRules: matchRules, + } + } + + pathRuleInferenceOnly := createPathRule("/inference", []dataplane.MatchRule{ + { + Match: dataplane.Match{}, + BackendGroup: singleInferenceGroup, + }, + }) + + pathRuleInferenceWithMatch := createPathRule("/inference-match", []dataplane.MatchRule{ + { + Match: dataplane.Match{ + Method: helpers.GetPointer("POST"), + }, + BackendGroup: singleInferenceGroup, + }, + }) + + pathRuleMultipleInferenceBackends := createPathRule("/weighted-inference", []dataplane.MatchRule{ + { + Match: dataplane.Match{}, + BackendGroup: multipleInferenceGroup, + }, + }) + + pathRuleMultipleInferenceWithMatch := createPathRule("/weighted-inference-match", []dataplane.MatchRule{ + { + Match: dataplane.Match{ + Method: helpers.GetPointer("GET"), + }, + BackendGroup: multipleInferenceGroup, + }, + }) + + singleRouteMultipleMatchesSingleBackend := createPathRule( + "/inference-multiple-matches-single-backend", + []dataplane.MatchRule{ { Match: dataplane.Match{}, - BackendGroup: fooGroup, + BackendGroup: singleInferenceGroup, }, - }, - } + { + Match: dataplane.Match{}, + BackendGroup: singleInferenceGroup, + }, + }) - pathRuleInferenceWithMatch := dataplane.PathRule{ - Path: "/inference-match", - PathType: dataplane.PathTypeExact, - HasInferenceBackends: true, - MatchRules: []dataplane.MatchRule{ + singleRouteMultipleMatchesMultipleBackends := createPathRule( + "/inference-multiple-matches-multiple-backends", + []dataplane.MatchRule{ { - Match: dataplane.Match{ - Method: helpers.GetPointer("POST"), - }, - BackendGroup: fooGroup, + Match: dataplane.Match{}, + BackendGroup: multipleInferenceGroup, }, - }, + { + Match: dataplane.Match{}, + BackendGroup: multipleInferenceGroup, + }, + }) + + proxySetHeaders := []http.Header{ + {Name: "Host", Value: "$gw_api_compliant_host"}, + {Name: "X-Forwarded-For", Value: "$proxy_add_x_forwarded_for"}, + {Name: "X-Real-IP", Value: "$remote_addr"}, + {Name: "X-Forwarded-Proto", Value: "$scheme"}, + {Name: "X-Forwarded-Host", Value: "$host"}, + {Name: "X-Forwarded-Port", Value: "$server_port"}, + {Name: "Upgrade", Value: "$http_upgrade"}, + {Name: "Connection", Value: "$connection_upgrade"}, } tests := []struct { @@ -2507,25 +2590,16 @@ func TestCreateLocations_InferenceBackends(t *testing.T) { pathRules: []dataplane.PathRule{pathRuleInferenceOnly}, expLocs: []http.Location{ { - Path: "/_ngf-internal-rule0-route0-inference", - Type: http.InternalLocationType, - ProxyPass: "http://$inference_backend_test_foo_80$request_uri", - ProxySetHeaders: []http.Header{ - {Name: "Host", Value: "$gw_api_compliant_host"}, - {Name: "X-Forwarded-For", Value: "$proxy_add_x_forwarded_for"}, - {Name: "X-Real-IP", Value: "$remote_addr"}, - {Name: "X-Forwarded-Proto", Value: "$scheme"}, - {Name: "X-Forwarded-Host", Value: "$host"}, - {Name: "X-Forwarded-Port", Value: "$server_port"}, - {Name: "Upgrade", Value: "$http_upgrade"}, - {Name: "Connection", Value: "$connection_upgrade"}, - }, + Path: "/_ngf-internal-proxy-pass-rule0-route0-backend0-inference", + Type: http.InternalLocationType, + ProxyPass: "http://$inference_backend_test_foo_80$request_uri", + ProxySetHeaders: proxySetHeaders, }, { Path: "= /inference", Type: http.InferenceExternalLocationType, - EPPInternalPath: "/_ngf-internal-rule0-route0-inference", - EPPHost: "test-epp.test", + EPPInternalPath: "/_ngf-internal-proxy-pass-rule0-route0-backend0-inference", + EPPHost: "test-epp.testNS", EPPPort: 80, }, createDefaultRootLocation(), @@ -2542,32 +2616,313 @@ func TestCreateLocations_InferenceBackends(t *testing.T) { HTTPMatchKey: "1_0", }, { - Path: "/_ngf-internal-rule0-route0-inference", + Path: "/_ngf-internal-proxy-pass-rule0-route0-backend0-inference", + Type: http.InternalLocationType, + ProxyPass: "http://$inference_backend_test_foo_80$request_uri", + ProxySetHeaders: proxySetHeaders, + }, + { + Path: "/_ngf-internal-test_foo_80-testNS-routeName-routeRule0-pathRule0", Type: http.InferenceInternalLocationType, - EPPInternalPath: "/_ngf-internal-rule0-route0", - EPPHost: "test-epp.test", + EPPInternalPath: "/_ngf-internal-proxy-pass-rule0-route0-backend0-inference", + EPPHost: "test-epp.testNS", EPPPort: 80, }, + createDefaultRootLocation(), + }, + expMatches: httpMatchPairs{ + "1_0": { + {Method: "POST", RedirectPath: "/_ngf-internal-test_foo_80-testNS-routeName-routeRule0-pathRule0"}, + }, + }, + }, + { + name: "multiple weighted inference backends, no match conditions", + pathRules: []dataplane.PathRule{pathRuleMultipleInferenceBackends}, + expLocs: []http.Location{ { - Path: "/_ngf-internal-rule0-route0", - Type: http.InternalLocationType, - ProxyPass: "http://$inference_backend_test_foo_80$request_uri", - ProxySetHeaders: []http.Header{ - {Name: "Host", Value: "$gw_api_compliant_host"}, - {Name: "X-Forwarded-For", Value: "$proxy_add_x_forwarded_for"}, - {Name: "X-Real-IP", Value: "$remote_addr"}, - {Name: "X-Forwarded-Proto", Value: "$scheme"}, - {Name: "X-Forwarded-Host", Value: "$host"}, - {Name: "X-Forwarded-Port", Value: "$server_port"}, - {Name: "Upgrade", Value: "$http_upgrade"}, - {Name: "Connection", Value: "$connection_upgrade"}, - }, + Path: "/_ngf-internal-proxy-pass-rule0-route0-backend0-inference", + Type: http.InternalLocationType, + ProxyPass: "http://$inference_backend_test_primary_pool_80$request_uri", + ProxySetHeaders: proxySetHeaders, + }, + { + Path: "/_ngf-internal-test_primary_pool_80-testNS-routeName-routeRule0-pathRule0", + Type: http.InferenceInternalLocationType, + EPPInternalPath: "/_ngf-internal-proxy-pass-rule0-route0-backend0-inference", + EPPHost: "primary-pool.testNS", + EPPPort: 80, + }, + { + Path: "/_ngf-internal-proxy-pass-rule0-route0-backend1-inference", + Type: http.InternalLocationType, + ProxyPass: "http://$inference_backend_test_secondary_pool_80$request_uri", + ProxySetHeaders: proxySetHeaders, + }, + { + Path: "/_ngf-internal-test_secondary_pool_80-testNS-routeName-routeRule0-pathRule0", + Type: http.InferenceInternalLocationType, + EPPInternalPath: "/_ngf-internal-proxy-pass-rule0-route0-backend1-inference", + EPPHost: "secondary-pool.testNS", + EPPPort: 80, + }, + { + Path: "= /weighted-inference", + Type: http.ExternalLocationType, + Rewrites: []string{"^ $inference_backend_group_testNS__routeName_rule0_pathRule0 last"}, + }, + createDefaultRootLocation(), + }, + expMatches: httpMatchPairs{}, + }, + { + name: "single route multiple matches with single backend", + pathRules: []dataplane.PathRule{singleRouteMultipleMatchesSingleBackend}, + expLocs: []http.Location{ + { + Path: "= /inference-multiple-matches-single-backend", + Type: http.RedirectLocationType, + HTTPMatchKey: "1_0", + }, + { + Path: "/_ngf-internal-proxy-pass-rule0-route0-backend0-inference", + Type: http.InternalLocationType, + ProxyPass: "http://$inference_backend_test_foo_80$request_uri", + ProxySetHeaders: proxySetHeaders, + }, + { + Path: "/_ngf-internal-test_foo_80-testNS-routeName-routeRule0-pathRule0", + Type: http.InferenceInternalLocationType, + EPPInternalPath: "/_ngf-internal-proxy-pass-rule0-route0-backend0-inference", + EPPHost: "test-epp.testNS", + EPPPort: 80, }, createDefaultRootLocation(), }, expMatches: httpMatchPairs{ "1_0": { - {Method: "POST", RedirectPath: "/_ngf-internal-rule0-route0-inference"}, + {Any: true, RedirectPath: "/_ngf-internal-test_foo_80-testNS-routeName-routeRule0-pathRule0"}, + }, + }, + }, + { + name: "single route multiple matches with multiple backends", + pathRules: []dataplane.PathRule{singleRouteMultipleMatchesMultipleBackends}, + expLocs: []http.Location{ + { + Path: "= /inference-multiple-matches-multiple-backends", + Type: http.RedirectLocationType, + HTTPMatchKey: "1_0", + }, + { + Path: "/_ngf-internal-proxy-pass-rule0-route0-backend0-inference", + Type: http.InternalLocationType, + ProxyPass: "http://$inference_backend_test_primary_pool_80$request_uri", + ProxySetHeaders: proxySetHeaders, + }, + { + Path: "/_ngf-internal-test_primary_pool_80-testNS-routeName-routeRule0-pathRule0", + Type: http.InferenceInternalLocationType, + EPPInternalPath: "/_ngf-internal-proxy-pass-rule0-route0-backend0-inference", + EPPHost: "primary-pool.testNS", + EPPPort: 80, + }, + { + Path: "/_ngf-internal-proxy-pass-rule0-route0-backend1-inference", + Type: http.InternalLocationType, + ProxyPass: "http://$inference_backend_test_secondary_pool_80$request_uri", + ProxySetHeaders: proxySetHeaders, + }, + { + Path: "/_ngf-internal-test_secondary_pool_80-testNS-routeName-routeRule0-pathRule0", + Type: http.InferenceInternalLocationType, + EPPInternalPath: "/_ngf-internal-proxy-pass-rule0-route0-backend1-inference", + EPPHost: "secondary-pool.testNS", + EPPPort: 80, + }, + { + Path: "/_ngf-internal-split-clients-rule0-route0-inference", + Type: http.InternalLocationType, + Rewrites: []string{"^ $inference_backend_group_testNS__routeName_rule0_pathRule0 last"}, + }, + createDefaultRootLocation(), + }, + expMatches: httpMatchPairs{ + "1_0": { + {Any: true, RedirectPath: "/_ngf-internal-split-clients-rule0-route0-inference"}, + }, + }, + }, + { + name: "multiple weighted inference backends with match conditions", + pathRules: []dataplane.PathRule{pathRuleMultipleInferenceWithMatch}, + expLocs: []http.Location{ + { + Path: "= /weighted-inference-match", + Type: http.RedirectLocationType, + HTTPMatchKey: "1_0", + }, + { + Path: "/_ngf-internal-proxy-pass-rule0-route0-backend0-inference", + Type: http.InternalLocationType, + ProxyPass: "http://$inference_backend_test_primary_pool_80$request_uri", + ProxySetHeaders: proxySetHeaders, + }, + { + Path: "/_ngf-internal-test_primary_pool_80-testNS-routeName-routeRule0-pathRule0", + Type: http.InferenceInternalLocationType, + EPPInternalPath: "/_ngf-internal-proxy-pass-rule0-route0-backend0-inference", + EPPHost: "primary-pool.testNS", + EPPPort: 80, + }, + { + Path: "/_ngf-internal-proxy-pass-rule0-route0-backend1-inference", + Type: http.InternalLocationType, + ProxyPass: "http://$inference_backend_test_secondary_pool_80$request_uri", + ProxySetHeaders: proxySetHeaders, + }, + { + Path: "/_ngf-internal-test_secondary_pool_80-testNS-routeName-routeRule0-pathRule0", + Type: http.InferenceInternalLocationType, + EPPInternalPath: "/_ngf-internal-proxy-pass-rule0-route0-backend1-inference", + EPPHost: "secondary-pool.testNS", + EPPPort: 80, + }, + { + Path: "/_ngf-internal-split-clients-rule0-route0-inference", + Type: http.InternalLocationType, + Rewrites: []string{"^ $inference_backend_group_testNS__routeName_rule0_pathRule0 last"}, + }, + createDefaultRootLocation(), + }, + expMatches: httpMatchPairs{ + "1_0": { + {Method: "GET", RedirectPath: "/_ngf-internal-split-clients-rule0-route0-inference"}, + }, + }, + }, + { + name: "mixed multiple path rules with different inference configurations", + pathRules: []dataplane.PathRule{ + setBackendGroupIndices(pathRuleInferenceOnly, 0, 0), + setBackendGroupIndices(pathRuleInferenceWithMatch, 1, 1), + setBackendGroupIndices(pathRuleMultipleInferenceBackends, 2, 2), + setBackendGroupIndices(pathRuleMultipleInferenceWithMatch, 3, 3), + }, + expLocs: []http.Location{ + // 1. Single inference pool locations (rule index 0) + { + Path: "/_ngf-internal-proxy-pass-rule0-route0-backend0-inference", + Type: http.InternalLocationType, + ProxyPass: "http://$inference_backend_test_foo_80$request_uri", + ProxySetHeaders: proxySetHeaders, + }, + { + Path: "= /inference", + Type: http.InferenceExternalLocationType, + EPPInternalPath: "/_ngf-internal-proxy-pass-rule0-route0-backend0-inference", + EPPHost: "test-epp.testNS", + EPPPort: 80, + }, + // 2. Single inference pool with match (rule index 1) + { + Path: "= /inference-match", + Type: http.RedirectLocationType, + HTTPMatchKey: "1_1", + }, + { + Path: "/_ngf-internal-proxy-pass-rule1-route0-backend0-inference", + Type: http.InternalLocationType, + ProxyPass: "http://$inference_backend_test_foo_80$request_uri", + ProxySetHeaders: proxySetHeaders, + }, + { + Path: "/_ngf-internal-test_foo_80-testNS-routeName-routeRule1-pathRule1", + Type: http.InferenceInternalLocationType, + EPPInternalPath: "/_ngf-internal-proxy-pass-rule1-route0-backend0-inference", + EPPHost: "test-epp.testNS", + EPPPort: 80, + }, + // 3. Multiple inference pools, no match (rule index 2) + { + Path: "/_ngf-internal-proxy-pass-rule2-route0-backend0-inference", + Type: http.InternalLocationType, + ProxyPass: "http://$inference_backend_test_primary_pool_80$request_uri", + ProxySetHeaders: proxySetHeaders, + }, + { + Path: "/_ngf-internal-test_primary_pool_80-testNS-routeName-routeRule2-pathRule2", + Type: http.InferenceInternalLocationType, + EPPInternalPath: "/_ngf-internal-proxy-pass-rule2-route0-backend0-inference", + EPPHost: "primary-pool.testNS", + EPPPort: 80, + }, + { + Path: "/_ngf-internal-proxy-pass-rule2-route0-backend1-inference", + Type: http.InternalLocationType, + ProxyPass: "http://$inference_backend_test_secondary_pool_80$request_uri", + ProxySetHeaders: proxySetHeaders, + }, + { + Path: "/_ngf-internal-test_secondary_pool_80-testNS-routeName-routeRule2-pathRule2", + Type: http.InferenceInternalLocationType, + EPPInternalPath: "/_ngf-internal-proxy-pass-rule2-route0-backend1-inference", + EPPHost: "secondary-pool.testNS", + EPPPort: 80, + }, + { + Path: "= /weighted-inference", + Type: http.ExternalLocationType, + Rewrites: []string{"^ $inference_backend_group_testNS__routeName_rule2_pathRule2 last"}, + }, + // 4. Multiple inference pools with match (rule index 3) + { + Path: "= /weighted-inference-match", + Type: http.RedirectLocationType, + HTTPMatchKey: "1_3", + }, + { + Path: "/_ngf-internal-proxy-pass-rule3-route0-backend0-inference", + Type: http.InternalLocationType, + ProxyPass: "http://$inference_backend_test_primary_pool_80$request_uri", + ProxySetHeaders: proxySetHeaders, + }, + { + Path: "/_ngf-internal-test_primary_pool_80-testNS-routeName-routeRule3-pathRule3", + Type: http.InferenceInternalLocationType, + EPPInternalPath: "/_ngf-internal-proxy-pass-rule3-route0-backend0-inference", + EPPHost: "primary-pool.testNS", + EPPPort: 80, + }, + { + Path: "/_ngf-internal-proxy-pass-rule3-route0-backend1-inference", + Type: http.InternalLocationType, + ProxyPass: "http://$inference_backend_test_secondary_pool_80$request_uri", + ProxySetHeaders: proxySetHeaders, + }, + { + Path: "/_ngf-internal-test_secondary_pool_80-testNS-routeName-routeRule3-pathRule3", + Type: http.InferenceInternalLocationType, + EPPInternalPath: "/_ngf-internal-proxy-pass-rule3-route0-backend1-inference", + EPPHost: "secondary-pool.testNS", + EPPPort: 80, + }, + { + Path: "/_ngf-internal-split-clients-rule3-route0-inference", + Type: http.InternalLocationType, + Rewrites: []string{"^ $inference_backend_group_testNS__routeName_rule3_pathRule3 last"}, + }, + createDefaultRootLocation(), + }, + expMatches: httpMatchPairs{ + "1_1": { + {Method: "POST", RedirectPath: "/_ngf-internal-test_foo_80-testNS-routeName-routeRule1-pathRule1"}, + }, + "1_3": { + { + Method: "GET", + RedirectPath: "/_ngf-internal-split-clients-rule3-route0-inference", + }, }, }, }, @@ -3871,7 +4226,7 @@ func TestCreateProxyPass(t *testing.T) { inferenceBackend: true, }, { - expected: "http://$group_ns1__bg_rule0$request_uri", + expected: "http://$group_ns1__bg_rule0_pathRule0$request_uri", grp: dataplane.BackendGroup{ Source: types.NamespacedName{Namespace: "ns1", Name: "bg"}, Backends: []dataplane.Backend{ diff --git a/internal/controller/nginx/config/split_clients.go b/internal/controller/nginx/config/split_clients.go index 2c9febf6bc..35e7d4c2ab 100644 --- a/internal/controller/nginx/config/split_clients.go +++ b/internal/controller/nginx/config/split_clients.go @@ -6,6 +6,8 @@ import ( "strings" gotemplate "text/template" + "k8s.io/apimachinery/pkg/types" + "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/http" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/state/dataplane" "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/helpers" @@ -105,13 +107,20 @@ func createBackendGroupSplitClients(backendGroups []dataplane.BackendGroup) []ht splitClients := make([]http.SplitClient, 0, numSplits) for _, group := range backendGroups { + variableName := convertStringToSafeVariableName(group.Name()) + distributions := createBackendGroupSplitClientDistributions(group) if distributions == nil { continue } + if group.Backends[0].EndpointPickerConfig != nil { + // This is an inferencePool backend group, need to adjust the name. + variableName = createInferenceSplitClientsVariableName(variableName) + } + splitClients = append(splitClients, http.SplitClient{ - VariableName: convertStringToSafeVariableName(group.Name()), + VariableName: variableName, Distributions: distributions, }) } @@ -119,6 +128,10 @@ func createBackendGroupSplitClients(backendGroups []dataplane.BackendGroup) []ht return splitClients } +func createInferenceSplitClientsVariableName(groupName string) string { + return "inference_backend_" + groupName +} + func createBackendGroupSplitClientDistributions(group dataplane.BackendGroup) []http.SplitClientDistribution { if !backendGroupNeedsSplit(group) { return nil @@ -155,7 +168,7 @@ func createBackendGroupSplitClientDistributions(group dataplane.BackendGroup) [] distributions = append(distributions, http.SplitClientDistribution{ Percent: fmt.Sprintf("%.2f", percentage), - Value: getSplitClientValue(b), + Value: getSplitClientValue(b, group.Source, group.RuleIdx, group.PathRuleIdx), }) } @@ -165,14 +178,23 @@ func createBackendGroupSplitClientDistributions(group dataplane.BackendGroup) [] distributions = append(distributions, http.SplitClientDistribution{ Percent: fmt.Sprintf("%.2f", availablePercentage), - Value: getSplitClientValue(lastBackend), + Value: getSplitClientValue(lastBackend, group.Source, group.RuleIdx, group.PathRuleIdx), }) return distributions } -func getSplitClientValue(b dataplane.Backend) string { +func getSplitClientValue(b dataplane.Backend, source types.NamespacedName, ruleIdx, pathRuleIdx int) string { if b.Valid { + if b.EndpointPickerConfig != nil { + return generateInternalInferenceEPPLocationPath( + b.UpstreamName, + source, + ruleIdx, + pathRuleIdx, + ) + } + return b.UpstreamName } return invalidBackendRef diff --git a/internal/controller/nginx/config/split_clients_test.go b/internal/controller/nginx/config/split_clients_test.go index 29f54986f2..30c2a7cbfb 100644 --- a/internal/controller/nginx/config/split_clients_test.go +++ b/internal/controller/nginx/config/split_clients_test.go @@ -619,7 +619,7 @@ func TestBackendGroupCreateSplitClients(t *testing.T) { }, expSplitClients: []http.SplitClient{ { - VariableName: "group_test__hr_one_split_rule0", + VariableName: "group_test__hr_one_split_rule0_pathRule0", Distributions: []http.SplitClientDistribution{ { Percent: "50.00", @@ -632,7 +632,7 @@ func TestBackendGroupCreateSplitClients(t *testing.T) { }, }, { - VariableName: "group_test__hr_two_splits_rule0", + VariableName: "group_test__hr_two_splits_rule0_pathRule0", Distributions: []http.SplitClientDistribution{ { Percent: "50.00", @@ -645,7 +645,7 @@ func TestBackendGroupCreateSplitClients(t *testing.T) { }, }, { - VariableName: "group_test__hr_two_splits_rule1", + VariableName: "group_test__hr_two_splits_rule1_pathRule0", Distributions: []http.SplitClientDistribution{ { Percent: "33.33", @@ -834,10 +834,15 @@ func TestCreateBackendGroupSplitClientDistributions(t *testing.T) { func TestGetSplitClientValue(t *testing.T) { t.Parallel() + hrNsName := types.NamespacedName{Namespace: "test", Name: "hr"} + tests := []struct { - msg string - expValue string - backend dataplane.Backend + source types.NamespacedName + msg string + expValue string + backend dataplane.Backend + ruleIdx int + pathRuleIdx int }{ { msg: "valid backend", @@ -845,6 +850,8 @@ func TestGetSplitClientValue(t *testing.T) { UpstreamName: "valid", Valid: true, }, + source: hrNsName, + ruleIdx: 0, expValue: "valid", }, { @@ -853,6 +860,35 @@ func TestGetSplitClientValue(t *testing.T) { UpstreamName: "invalid", Valid: false, }, + source: hrNsName, + ruleIdx: 0, + expValue: invalidBackendRef, + }, + { + msg: "valid backend with endpoint picker config", + backend: dataplane.Backend{ + UpstreamName: "inference-backend", + Valid: true, + EndpointPickerConfig: &dataplane.EndpointPickerConfig{ + NsName: "test-namespace", + }, + }, + source: hrNsName, + ruleIdx: 2, + pathRuleIdx: 1, + expValue: "/_ngf-internal-inference-backend-test-hr-routeRule2-pathRule1", + }, + { + msg: "invalid backend with endpoint picker config", + backend: dataplane.Backend{ + UpstreamName: "invalid-inference-backend", + Valid: false, + EndpointPickerConfig: &dataplane.EndpointPickerConfig{ + NsName: "test-namespace", + }, + }, + source: hrNsName, + ruleIdx: 1, expValue: invalidBackendRef, }, } @@ -861,7 +897,7 @@ func TestGetSplitClientValue(t *testing.T) { t.Run(test.msg, func(t *testing.T) { t.Parallel() g := NewWithT(t) - result := getSplitClientValue(test.backend) + result := getSplitClientValue(test.backend, test.source, test.ruleIdx, test.pathRuleIdx) g.Expect(result).To(Equal(test.expValue)) }) } @@ -1092,7 +1128,7 @@ func TestBackendGroupName(t *testing.T) { Weight: 1, }, }, - expName: "group_test__hr_rule0", + expName: "group_test__hr_rule0_pathRule0", }, { msg: "multiple invalid backends", @@ -1108,7 +1144,7 @@ func TestBackendGroupName(t *testing.T) { Weight: 1, }, }, - expName: "group_test__hr_rule0", + expName: "group_test__hr_rule0_pathRule0", }, } diff --git a/internal/controller/state/dataplane/configuration.go b/internal/controller/state/dataplane/configuration.go index 26bc3c58ee..def60605b6 100644 --- a/internal/controller/state/dataplane/configuration.go +++ b/internal/controller/state/dataplane/configuration.go @@ -347,8 +347,9 @@ func buildCertBundles( func buildBackendGroups(servers []VirtualServer) []BackendGroup { type key struct { - nsname types.NamespacedName - ruleIdx int + nsname types.NamespacedName + ruleIdx int + pathRuleIdx int } // There can be duplicate backend groups if a route is attached to multiple listeners. @@ -361,8 +362,9 @@ func buildBackendGroups(servers []VirtualServer) []BackendGroup { group := mr.BackendGroup k := key{ - nsname: group.Source, - ruleIdx: group.RuleIdx, + nsname: group.Source, + ruleIdx: group.RuleIdx, + pathRuleIdx: group.PathRuleIdx, } uniqueGroups[k] = group @@ -694,6 +696,12 @@ func (hpr *hostPathRules) buildServers() []VirtualServer { return s.PathRules[i].PathType < s.PathRules[j].PathType }) + for pathRuleIdx := range s.PathRules { + for matchRuleIdx := range s.PathRules[pathRuleIdx].MatchRules { + s.PathRules[pathRuleIdx].MatchRules[matchRuleIdx].BackendGroup.PathRuleIdx = pathRuleIdx + } + } + servers = append(servers, s) } diff --git a/internal/controller/state/dataplane/configuration_test.go b/internal/controller/state/dataplane/configuration_test.go index 330a0cef79..b9c3bc27e8 100644 --- a/internal/controller/state/dataplane/configuration_test.go +++ b/internal/controller/state/dataplane/configuration_test.go @@ -401,6 +401,19 @@ func assertBuildConfiguration(g *WithT, result, expected Configuration) { } func TestBuildConfiguration(t *testing.T) { + // setPathRuleIdx creates a new BackendGroup with the specified PathRuleIdx + // This is needed because pathRuleIdx cannot be determined in createTestResources when + // the BackendGroup is created, but can only be determined when the VirtualServer's PathRules are + // being defined in the Configuration. + setPathRuleIdx := func(bg BackendGroup, pathRuleIdx int) BackendGroup { + return BackendGroup{ + Source: bg.Source, + RuleIdx: bg.RuleIdx, + PathRuleIdx: pathRuleIdx, + Backends: bg.Backends, + } + } + t.Parallel() fakeResolver := &resolverfakes.FakeServiceResolver{} @@ -669,7 +682,6 @@ func TestBuildConfiguration(t *testing.T) { "listener-8443", pathAndType{path: "/", pathType: prefix}, pathAndType{path: "/third", pathType: prefix}, ) - httpsHR8, expHTTPSHR8Groups, httpsRouteHR8 := createTestResources( "https-hr-8", "foo.example.com", @@ -1257,11 +1269,11 @@ func TestBuildConfiguration(t *testing.T) { PathType: PathTypePrefix, MatchRules: []MatchRule{ { - BackendGroup: expHR3Groups[0], + BackendGroup: setPathRuleIdx(expHR3Groups[0], 0), Source: &hr3.ObjectMeta, }, { - BackendGroup: expHR4Groups[1], + BackendGroup: setPathRuleIdx(expHR4Groups[1], 0), Source: &hr4.ObjectMeta, }, }, @@ -1271,7 +1283,7 @@ func TestBuildConfiguration(t *testing.T) { PathType: PathTypePrefix, MatchRules: []MatchRule{ { - BackendGroup: expHR4Groups[0], + BackendGroup: setPathRuleIdx(expHR4Groups[0], 1), Source: &hr4.ObjectMeta, }, }, @@ -1281,7 +1293,7 @@ func TestBuildConfiguration(t *testing.T) { PathType: PathTypePrefix, MatchRules: []MatchRule{ { - BackendGroup: expHR3Groups[1], + BackendGroup: setPathRuleIdx(expHR3Groups[1], 2), Source: &hr3.ObjectMeta, }, }, @@ -1300,11 +1312,11 @@ func TestBuildConfiguration(t *testing.T) { PathType: PathTypePrefix, MatchRules: []MatchRule{ { - BackendGroup: expHTTPSHR3Groups[0], + BackendGroup: setPathRuleIdx(expHTTPSHR3Groups[0], 0), Source: &httpsHR3.ObjectMeta, }, { - BackendGroup: expHTTPSHR4Groups[1], + BackendGroup: setPathRuleIdx(expHTTPSHR4Groups[1], 0), Source: &httpsHR4.ObjectMeta, }, }, @@ -1314,7 +1326,7 @@ func TestBuildConfiguration(t *testing.T) { PathType: PathTypePrefix, MatchRules: []MatchRule{ { - BackendGroup: expHTTPSHR4Groups[0], + BackendGroup: setPathRuleIdx(expHTTPSHR4Groups[0], 1), Source: &httpsHR4.ObjectMeta, }, }, @@ -1324,7 +1336,7 @@ func TestBuildConfiguration(t *testing.T) { PathType: PathTypePrefix, MatchRules: []MatchRule{ { - BackendGroup: expHTTPSHR3Groups[1], + BackendGroup: setPathRuleIdx(expHTTPSHR3Groups[1], 2), Source: &httpsHR3.ObjectMeta, }, }, @@ -1340,14 +1352,14 @@ func TestBuildConfiguration(t *testing.T) { }...) conf.Upstreams = append(conf.Upstreams, fooUpstream) conf.BackendGroups = []BackendGroup{ - expHR3Groups[0], - expHR3Groups[1], - expHR4Groups[0], - expHR4Groups[1], - expHTTPSHR3Groups[0], - expHTTPSHR3Groups[1], - expHTTPSHR4Groups[0], - expHTTPSHR4Groups[1], + setPathRuleIdx(expHR3Groups[0], 0), + setPathRuleIdx(expHR3Groups[1], 2), + setPathRuleIdx(expHR4Groups[0], 1), + setPathRuleIdx(expHR4Groups[1], 0), + setPathRuleIdx(expHTTPSHR3Groups[0], 0), + setPathRuleIdx(expHTTPSHR3Groups[1], 2), + setPathRuleIdx(expHTTPSHR4Groups[0], 1), + setPathRuleIdx(expHTTPSHR4Groups[1], 0), } return conf }), @@ -1417,7 +1429,7 @@ func TestBuildConfiguration(t *testing.T) { PathType: PathTypePrefix, MatchRules: []MatchRule{ { - BackendGroup: expHR3Groups[0], + BackendGroup: setPathRuleIdx(expHR3Groups[0], 0), Source: &hr3.ObjectMeta, }, }, @@ -1427,7 +1439,7 @@ func TestBuildConfiguration(t *testing.T) { PathType: PathTypePrefix, MatchRules: []MatchRule{ { - BackendGroup: expHR3Groups[1], + BackendGroup: setPathRuleIdx(expHR3Groups[1], 1), Source: &hr3.ObjectMeta, }, }, @@ -1447,7 +1459,7 @@ func TestBuildConfiguration(t *testing.T) { PathType: PathTypePrefix, MatchRules: []MatchRule{ { - BackendGroup: expHR8Groups[0], + BackendGroup: setPathRuleIdx(expHR8Groups[0], 0), Source: &hr8.ObjectMeta, }, }, @@ -1457,7 +1469,7 @@ func TestBuildConfiguration(t *testing.T) { PathType: PathTypePrefix, MatchRules: []MatchRule{ { - BackendGroup: expHR8Groups[1], + BackendGroup: setPathRuleIdx(expHR8Groups[1], 1), Source: &hr8.ObjectMeta, }, }, @@ -1476,7 +1488,7 @@ func TestBuildConfiguration(t *testing.T) { PathType: PathTypePrefix, MatchRules: []MatchRule{ { - BackendGroup: expHTTPSHR3Groups[0], + BackendGroup: setPathRuleIdx(expHTTPSHR3Groups[0], 0), Source: &httpsHR3.ObjectMeta, }, }, @@ -1486,7 +1498,7 @@ func TestBuildConfiguration(t *testing.T) { PathType: PathTypePrefix, MatchRules: []MatchRule{ { - BackendGroup: expHTTPSHR3Groups[1], + BackendGroup: setPathRuleIdx(expHTTPSHR3Groups[1], 1), Source: &httpsHR3.ObjectMeta, }, }, @@ -1512,7 +1524,7 @@ func TestBuildConfiguration(t *testing.T) { PathType: PathTypePrefix, MatchRules: []MatchRule{ { - BackendGroup: expHTTPSHR7Groups[0], + BackendGroup: setPathRuleIdx(expHTTPSHR7Groups[0], 0), Source: &httpsHR7.ObjectMeta, }, }, @@ -1522,7 +1534,7 @@ func TestBuildConfiguration(t *testing.T) { PathType: PathTypePrefix, MatchRules: []MatchRule{ { - BackendGroup: expHTTPSHR7Groups[1], + BackendGroup: setPathRuleIdx(expHTTPSHR7Groups[1], 1), Source: &httpsHR7.ObjectMeta, }, }, @@ -1538,14 +1550,14 @@ func TestBuildConfiguration(t *testing.T) { }...) conf.Upstreams = append(conf.Upstreams, fooUpstream) conf.BackendGroups = []BackendGroup{ - expHR3Groups[0], - expHR3Groups[1], - expHR8Groups[0], - expHR8Groups[1], - expHTTPSHR3Groups[0], - expHTTPSHR3Groups[1], - expHTTPSHR7Groups[0], - expHTTPSHR7Groups[1], + setPathRuleIdx(expHR3Groups[0], 0), + setPathRuleIdx(expHR3Groups[1], 1), + setPathRuleIdx(expHR8Groups[0], 0), + setPathRuleIdx(expHR8Groups[1], 1), + setPathRuleIdx(expHTTPSHR3Groups[0], 0), + setPathRuleIdx(expHTTPSHR3Groups[1], 1), + setPathRuleIdx(expHTTPSHR7Groups[0], 0), + setPathRuleIdx(expHTTPSHR7Groups[1], 1), } return conf }), @@ -1609,7 +1621,7 @@ func TestBuildConfiguration(t *testing.T) { MatchRules: []MatchRule{ { Source: &hr5.ObjectMeta, - BackendGroup: expHR5Groups[1], + BackendGroup: setPathRuleIdx(expHR5Groups[1], 1), Filters: HTTPFilters{ InvalidFilter: &InvalidHTTPFilter{}, }, @@ -1622,7 +1634,7 @@ func TestBuildConfiguration(t *testing.T) { }...) conf.SSLServers = []VirtualServer{} conf.Upstreams = []Upstream{fooUpstream} - conf.BackendGroups = []BackendGroup{expHR5Groups[0], expHR5Groups[1]} + conf.BackendGroups = []BackendGroup{expHR5Groups[0], setPathRuleIdx(expHR5Groups[1], 1)} conf.SSLKeyPairs = map[SSLKeyPairID]SSLKeyPair{} return conf }), @@ -1815,7 +1827,7 @@ func TestBuildConfiguration(t *testing.T) { PathType: PathTypePrefix, MatchRules: []MatchRule{ { - BackendGroup: expHR7Groups[0], + BackendGroup: setPathRuleIdx(expHR7Groups[0], 1), Source: &hr7.ObjectMeta, }, }, @@ -1826,7 +1838,7 @@ func TestBuildConfiguration(t *testing.T) { }...) conf.SSLServers = []VirtualServer{} conf.Upstreams = []Upstream{fooUpstream} - conf.BackendGroups = []BackendGroup{expHR7Groups[0], expHR7Groups[1]} + conf.BackendGroups = []BackendGroup{setPathRuleIdx(expHR7Groups[0], 1), expHR7Groups[1]} conf.SSLKeyPairs = map[SSLKeyPairID]SSLKeyPair{} return conf }), @@ -3323,7 +3335,7 @@ func TestBackendGroupName(t *testing.T) { t.Parallel() backendGroup := createBackendGroup("route1", 2, "foo", "bar") - expectedGroupName := "group_test__route1_rule2" + expectedGroupName := "group_test__route1_rule2_pathRule0" g := NewWithT(t) g.Expect(backendGroup.Name()).To(Equal(expectedGroupName)) diff --git a/internal/controller/state/dataplane/types.go b/internal/controller/state/dataplane/types.go index 1b0ccbde6a..9e1beeaa0f 100644 --- a/internal/controller/state/dataplane/types.go +++ b/internal/controller/state/dataplane/types.go @@ -311,6 +311,9 @@ type BackendGroup struct { Backends []Backend // RuleIdx is the index of the corresponding rule in the HTTPRoute. RuleIdx int + // PathRuleIdx is the index of the corresponding path rule when attached to a VirtualServer. + // BackendGroups attached to a MatchRule that have the same Path match will have the same PathRuleIdx. + PathRuleIdx int } // Name returns the name of the backend group. @@ -321,7 +324,7 @@ type BackendGroup struct { // The RuleIdx may change for a given rule if an update is made to the HTTPRoute, but it will always match the index // of the rule in the stored HTTPRoute. func (bg *BackendGroup) Name() string { - return fmt.Sprintf("group_%s__%s_rule%d", bg.Source.Namespace, bg.Source.Name, bg.RuleIdx) + return fmt.Sprintf("group_%s__%s_rule%d_pathRule%d", bg.Source.Namespace, bg.Source.Name, bg.RuleIdx, bg.PathRuleIdx) } // Backend represents a Backend for a routing rule. diff --git a/internal/controller/state/graph/httproute.go b/internal/controller/state/graph/httproute.go index 30b28f1800..1a4749b984 100644 --- a/internal/controller/state/graph/httproute.go +++ b/internal/controller/state/graph/httproute.go @@ -207,6 +207,21 @@ func processHTTPRouteRule( backendRefs := make([]RouteBackendRef, 0, len(specRule.BackendRefs)) + if checkForMixedBackendTypes(specRule, routeNamespace, inferencePools) { + err := field.Forbidden( + rulePath.Child("backendRefs"), + "mixing InferencePool and non-InferencePool backends in a rule is not supported", + ) + errors.invalid = append(errors.invalid, err) + + return RouteRule{ + ValidMatches: validMatches, + Matches: specRule.Matches, + Filters: routeFilters, + RouteBackendRefs: backendRefs, + }, errors + } + // rule.BackendRefs are validated separately because of their special requirements for _, b := range specRule.BackendRefs { var interfaceFilters []any @@ -225,18 +240,6 @@ func processHTTPRouteRule( // headless Service backend (that we created), so nginx config can be built properly. // Only do this if the InferencePool actually exists. if ok, key := inferencePoolBackend(b, routeNamespace, inferencePools); ok { - // 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 { - err := field.Forbidden( - rulePath.Child("backendRefs"), - "cannot use InferencePool backend when multiple backendRefs are specified in a single rule", - ) - errors.invalid = append(errors.invalid, err) - break - } - svcName := controller.CreateInferencePoolServiceName(string(b.Name)) rbr = RouteBackendRef{ IsInferencePool: true, @@ -643,3 +646,28 @@ func checkForUnsupportedHTTPFields(rule v1.HTTPRouteRule, rulePath *field.Path) return ruleErrors } + +// checkForMixedBackendTypes returns true if the rule contains a mix of +// InferencePool and non-InferencePool backends. +func checkForMixedBackendTypes( + specRule v1.HTTPRouteRule, + routeNamespace string, + inferencePools map[types.NamespacedName]*inference.InferencePool, +) bool { + var hasInferencePool, hasNonInferencePool bool + + for _, backendRef := range specRule.BackendRefs { + if ok, _ := inferencePoolBackend(backendRef, routeNamespace, inferencePools); ok { + hasInferencePool = true + } else { + hasNonInferencePool = true + } + + // Early exit if we find both types + if hasInferencePool && hasNonInferencePool { + return true + } + } + + return false +} diff --git a/internal/controller/state/graph/httproute_test.go b/internal/controller/state/graph/httproute_test.go index 828d17d749..ff59e84eba 100644 --- a/internal/controller/state/graph/httproute_test.go +++ b/internal/controller/state/graph/httproute_test.go @@ -1256,63 +1256,144 @@ func TestBuildHTTPRouteWithMirrorRoutes(t *testing.T) { func TestProcessHTTPRouteRule_InferencePoolWithMultipleBackendRefs(t *testing.T) { t.Parallel() - g := NewWithT(t) validator := &validationfakes.FakeHTTPFieldsValidator{} - inferencePoolName := "ipool" + inferencePoolName1 := "primary-pool" + inferencePoolName2 := "secondary-pool" routeNamespace := "test" inferencePools := map[types.NamespacedName]*inference.InferencePool{ - {Namespace: routeNamespace, Name: inferencePoolName}: {}, + {Namespace: routeNamespace, Name: inferencePoolName1}: {}, + {Namespace: routeNamespace, Name: inferencePoolName2}: {}, } - // BackendRef 1: InferencePool - backendRef1 := gatewayv1.HTTPBackendRef{ - BackendRef: gatewayv1.BackendRef{ - BackendObjectReference: gatewayv1.BackendObjectReference{ - Group: helpers.GetPointer[gatewayv1.Group](inferenceAPIGroup), - Kind: helpers.GetPointer[gatewayv1.Kind](kinds.InferencePool), - Name: gatewayv1.ObjectName(inferencePoolName), - Namespace: helpers.GetPointer(gatewayv1.Namespace(routeNamespace)), - }, - }, - } - // BackendRef 2: Service - backendRef2 := gatewayv1.HTTPBackendRef{ - BackendRef: gatewayv1.BackendRef{ - BackendObjectReference: gatewayv1.BackendObjectReference{ - Kind: helpers.GetPointer[gatewayv1.Kind](kinds.Service), - Name: "backend", + tests := []struct { + specRule gatewayv1.HTTPRouteRule + name string + expectErrorMsg string + expectedBackend int + expectValid bool + }{ + { + name: "multiple weighted InferencePool backends (valid)", + specRule: gatewayv1.HTTPRouteRule{ + Matches: []gatewayv1.HTTPRouteMatch{ + { + Path: &gatewayv1.HTTPPathMatch{ + Type: helpers.GetPointer(gatewayv1.PathMatchPathPrefix), + Value: helpers.GetPointer("/inference"), + }, + }, + }, + BackendRefs: []gatewayv1.HTTPBackendRef{ + { + BackendRef: gatewayv1.BackendRef{ + BackendObjectReference: gatewayv1.BackendObjectReference{ + Group: helpers.GetPointer[gatewayv1.Group](inferenceAPIGroup), + Kind: helpers.GetPointer[gatewayv1.Kind](kinds.InferencePool), + Name: gatewayv1.ObjectName(inferencePoolName1), + Namespace: helpers.GetPointer(gatewayv1.Namespace(routeNamespace)), + }, + Weight: helpers.GetPointer(int32(70)), + }, + }, + { + BackendRef: gatewayv1.BackendRef{ + BackendObjectReference: gatewayv1.BackendObjectReference{ + Group: helpers.GetPointer[gatewayv1.Group](inferenceAPIGroup), + Kind: helpers.GetPointer[gatewayv1.Kind](kinds.InferencePool), + Name: gatewayv1.ObjectName(inferencePoolName2), + Namespace: helpers.GetPointer(gatewayv1.Namespace(routeNamespace)), + }, + Weight: helpers.GetPointer(int32(30)), + }, + }, + }, }, + expectValid: true, + expectedBackend: 2, }, - } - - specRule := gatewayv1.HTTPRouteRule{ - Matches: []gatewayv1.HTTPRouteMatch{ - { - Path: &gatewayv1.HTTPPathMatch{ - Type: helpers.GetPointer(gatewayv1.PathMatchPathPrefix), - Value: helpers.GetPointer("/"), + { + name: "InferencePool mixed with Service backend (invalid)", + specRule: gatewayv1.HTTPRouteRule{ + Matches: []gatewayv1.HTTPRouteMatch{ + { + Path: &gatewayv1.HTTPPathMatch{ + Type: helpers.GetPointer(gatewayv1.PathMatchPathPrefix), + Value: helpers.GetPointer("/mixed"), + }, + }, + }, + BackendRefs: []gatewayv1.HTTPBackendRef{ + { + BackendRef: gatewayv1.BackendRef{ + BackendObjectReference: gatewayv1.BackendObjectReference{ + Group: helpers.GetPointer[gatewayv1.Group](inferenceAPIGroup), + Kind: helpers.GetPointer[gatewayv1.Kind](kinds.InferencePool), + Name: gatewayv1.ObjectName(inferencePoolName1), + Namespace: helpers.GetPointer(gatewayv1.Namespace(routeNamespace)), + }, + }, + }, + { + BackendRef: gatewayv1.BackendRef{ + BackendObjectReference: gatewayv1.BackendObjectReference{ + Kind: helpers.GetPointer[gatewayv1.Kind](kinds.Service), + Name: "service-backend", + }, + }, + }, }, }, + expectValid: false, + expectErrorMsg: "mixing InferencePool and non-InferencePool backends in a rule is not supported", + expectedBackend: 0, }, - BackendRefs: []gatewayv1.HTTPBackendRef{backendRef1, backendRef2}, } - rulePath := field.NewPath("spec").Child("rules").Index(0) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) - routeRule, errs := processHTTPRouteRule( - specRule, - rulePath, - validator, - nil, - inferencePools, - routeNamespace, - ) + rulePath := field.NewPath("spec").Child("rules").Index(0) - g.Expect(routeRule.RouteBackendRefs).To(BeEmpty()) - g.Expect(errs.invalid).To(HaveLen(1)) - errMsg := "cannot use InferencePool backend when multiple backendRefs are specified in a single rule" - g.Expect(errs.invalid[0].Error()).To(ContainSubstring(errMsg)) + routeRule, errs := processHTTPRouteRule( + tc.specRule, + rulePath, + validator, + nil, + inferencePools, + routeNamespace, + ) + + if tc.expectValid { + g.Expect(errs.invalid).To(BeEmpty()) + g.Expect(routeRule.RouteBackendRefs).To(HaveLen(tc.expectedBackend)) + + if tc.expectedBackend == 2 { + // Verify both backends are converted to services with weights + g.Expect(routeRule.RouteBackendRefs[0].IsInferencePool).To(BeTrue()) + g.Expect(routeRule.RouteBackendRefs[1].IsInferencePool).To(BeTrue()) + + // Verify service name conversion (primary-pool -> primary-pool-pool-svc) + g.Expect(string(routeRule.RouteBackendRefs[0].BackendRef.Name)).To(Equal("primary-pool-pool-svc")) + g.Expect(string(routeRule.RouteBackendRefs[1].BackendRef.Name)).To(Equal("secondary-pool-pool-svc")) + + // Verify weights are preserved + g.Expect(routeRule.RouteBackendRefs[0].BackendRef.Weight).To(Equal(helpers.GetPointer(int32(70)))) + g.Expect(routeRule.RouteBackendRefs[1].BackendRef.Weight).To(Equal(helpers.GetPointer(int32(30)))) + + // Verify kind is converted to Service + g.Expect(*routeRule.RouteBackendRefs[0].BackendRef.Kind).To(Equal(gatewayv1.Kind(kinds.Service))) + g.Expect(*routeRule.RouteBackendRefs[1].BackendRef.Kind).To(Equal(gatewayv1.Kind(kinds.Service))) + } + } else { + g.Expect(errs.invalid).To(HaveLen(1)) + g.Expect(errs.invalid[0].Error()).To(ContainSubstring(tc.expectErrorMsg)) + g.Expect(routeRule.RouteBackendRefs).To(HaveLen(tc.expectedBackend)) + } + }) + } } func TestValidateMatch(t *testing.T) { diff --git a/tests/Makefile b/tests/Makefile index d8be4d2be8..2b8dd4bb45 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -19,8 +19,8 @@ CONFORMANCE_PROFILES = $(STANDARD_CONFORMANCE_PROFILES) # by default we use the SKIP_TESTS_OPENSHIFT = HTTPRouteServiceTypes # Doesn't work on OpenShift due to security restrictions SKIP_TESTS = CEL_TEST_TARGET = -INFERENCE_SUPPORTED_FEATURES = GatewayFollowingEPPRouting,EppUnAvailableFailOpen,HTTPRouteInvalidInferencePoolRef,InferencePoolAccepted,HTTPRouteMultipleGatewaysDifferentPools,HTTPRouteMultipleRulesDifferentPools,InferencePoolHTTPRoutePortValidation,InferencePoolInvalidEPPService,InferencePoolResolvedRefsCondition -INFERENCE_SKIP_TESTS = GatewayWeightedAcrossTwoInferencePools +INFERENCE_SUPPORTED_FEATURES = GatewayFollowingEPPRouting,EppUnAvailableFailOpen,HTTPRouteInvalidInferencePoolRef,InferencePoolAccepted,HTTPRouteMultipleGatewaysDifferentPools,HTTPRouteMultipleRulesDifferentPools,InferencePoolHTTPRoutePortValidation,InferencePoolInvalidEPPService,InferencePoolResolvedRefsCondition,GatewayWeightedAcrossTwoInferencePools +INFERENCE_SKIP_TESTS = # Check if ENABLE_EXPERIMENTAL is true ifeq ($(ENABLE_EXPERIMENTAL),true) @@ -115,9 +115,9 @@ run-inference-conformance-tests: ## Run inference conformance tests .PHONY: cleanup-conformance-tests cleanup-conformance-tests: ## Clean up conformance tests fixtures - kubectl delete pod conformance + kubectl delete pod conformance --ignore-not-found kubectl delete pod conformance-inference --ignore-not-found - kubectl delete -f conformance/conformance-rbac.yaml + kubectl delete -f conformance/conformance-rbac.yaml --ignore-not-found .PHONY: reset-go-modules reset-go-modules: ## Reset the go modules changes