diff --git a/CHANGELOG.md b/CHANGELOG.md index 91590f7617..2440417aa0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -77,6 +77,13 @@ `sticky_sessions_cookie` is set when `algorithm` is `sticky-sessions`. [#3555](https://github.com/Kong/kong-operator/pull/3555) +### Changed + +- HybridGateway: generate one KongRoute per HTTPRouteMatch to honor Gateway API OR semantics across matches within a rule. + This enables the `HTTPRouteMatching` conformance test for Hybrid mode. + Note: routes count per rule may increase. + [#3577](https://github.com/Kong/kong-operator/pull/3577) + ### Fixes - Admission webhook now validates HTTPRoute regex patterns before sending diff --git a/controller/hybridgateway/converter/http_route.go b/controller/hybridgateway/converter/http_route.go index b229bc0a19..ac507d1756 100644 --- a/controller/hybridgateway/converter/http_route.go +++ b/controller/hybridgateway/converter/http_route.go @@ -425,23 +425,27 @@ func (c *httpRouteConverter) translate(ctx context.Context, logger logr.Logger) log.Debug(logger, "Successfully translated KongService resource", "service", serviceName) - // Build the KongRoute resource. - routePtr, err := kongroute.RouteForRule(ctx, logger, c.Client, c.route, rule, &pRef, cp, serviceName, hostnames) + // Build one KongRoute per match in the rule. + // Gateway API semantics require OR across matches within a rule + // and AND within a single match. Generating a route per match + // preserves the OR semantics for Hybrid Gateway. + routes, err := kongroute.RoutesForRule(ctx, logger, c.Client, c.route, rule, &pRef, cp, serviceName, hostnames) if err != nil { - log.Error(logger, err, "Failed to translate KongRoute resource, skipping rule", + log.Error(logger, err, "Failed to translate KongRoute resources for rule, skipping rule", "service", serviceName, "hostnames", hostnames) - translationErrors = append(translationErrors, fmt.Errorf("failed to translate KongRoute for rule: %w", err)) + translationErrors = append(translationErrors, fmt.Errorf("failed to translate KongRoutes for rule: %w", err)) continue } - routeName := routePtr.Name - c.outputStore = append(c.outputStore, routePtr) - log.Debug(logger, "Successfully translated KongRoute resource", - "route", routeName) + for _, r := range routes { + routeName := r.Name + c.outputStore = append(c.outputStore, r) + log.Debug(logger, "Successfully translated KongRoute resource", + "route", routeName) + } // Build the KongPlugin and KongPluginBinding resources. log.Debug(logger, "Processing filters for rule", - "kongRoute", routeName, "filterCount", len(rule.Filters)) for _, filter := range rule.Filters { @@ -456,30 +460,33 @@ func (c *httpRouteConverter) translate(ctx context.Context, logger logr.Logger) for _, plugin := range plugins { pluginName := plugin.Name c.outputStore = append(c.outputStore, &plugin) - // Create a KongPluginBinding to bind the KongPlugin to each KongRoute. - bindingPtr, err := pluginbinding.BindingForPluginAndRoute( - ctx, - logger, - c.Client, - c.route, - &pRef, - cp, - pluginName, - routeName, - ) - if err != nil { - log.Error(logger, err, "Failed to build KongPluginBinding resource, skipping binding", + // Create a KongPluginBinding to bind the KongPlugin to each KongRoute generated for the rule. + for _, r := range routes { + bindingPtr, err := pluginbinding.BindingForPluginAndRoute( + ctx, + logger, + c.Client, + c.route, + &pRef, + cp, + pluginName, + r.Name, + ) + if err != nil { + log.Error(logger, err, "Failed to build KongPluginBinding resource, skipping binding", + "plugin", pluginName, + "kongRoute", r.Name) + translationErrors = append(translationErrors, fmt.Errorf("failed to build KongPluginBinding for plugin %s: %w", pluginName, err)) + continue + } + bindingName := bindingPtr.Name + c.outputStore = append(c.outputStore, bindingPtr) + + log.Debug(logger, "Successfully translated KongPlugin and KongPluginBinding resources", "plugin", pluginName, - "kongRoute", routeName) - translationErrors = append(translationErrors, fmt.Errorf("failed to build KongPluginBinding for plugin %s: %w", pluginName, err)) - continue + "binding", bindingName, + "route", r.Name) } - bindingName := bindingPtr.Name - c.outputStore = append(c.outputStore, bindingPtr) - - log.Debug(logger, "Successfully translated KongPlugin and KongPluginBinding resources", - "plugin", pluginName, - "binding", bindingName) } } diff --git a/controller/hybridgateway/converter/http_route_converter_test.go b/controller/hybridgateway/converter/http_route_converter_test.go index 48b2b7bcbd..ba23237ac1 100644 --- a/controller/hybridgateway/converter/http_route_converter_test.go +++ b/controller/hybridgateway/converter/http_route_converter_test.go @@ -449,7 +449,7 @@ func TestHTTPRouteConverter_Translate(t *testing.T) { return newHTTPRouteConverter(route, fakeClient, false, "").(*httpRouteConverter) }, wantErr: true, - wantErrSub: "failed to translate KongRoute for rule", + wantErrSub: "failed to translate KongRoutes for rule", wantOutputs: outputCount{ upstreams: 1, services: 1, diff --git a/controller/hybridgateway/kongroute/route.go b/controller/hybridgateway/kongroute/route.go index 388d9d03e4..2cab323584 100644 --- a/controller/hybridgateway/kongroute/route.go +++ b/controller/hybridgateway/kongroute/route.go @@ -5,6 +5,7 @@ import ( "fmt" "github.com/go-logr/logr" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" @@ -18,30 +19,32 @@ import ( gwtypes "github.com/kong/kong-operator/v2/internal/types" ) -// RouteForRule creates or updates a KongRoute for the given HTTPRoute rule. +// RoutesForRule creates or updates one KongRoute per HTTPRouteMatch in the given rule. // -// The function performs the following operations: -// 1. Generates the KongRoute name using the namegen package -// 2. Checks if a KongRoute with that name already exists in the cluster -// 3. If it exists, updates the KongRoute -// 4. If it doesn't exist, creates a new KongRoute -// 5. Returns the KongRoute resource for use by the caller +// Gateway API semantics are: +// - Within a single HTTPRouteRule, entries in .Matches are ORed +// - Within a single HTTPRouteMatch, individual criteria (path/method/headers) are ANDed +// +// To faithfully represent this in Kong, we generate one KongRoute for each HTTPRouteMatch +// and attach only that match's criteria to the route. All routes point to the same KongService. +// This fixes Hybrid Gateway conformance failures such as HTTPRouteMatching, which includes +// rules that combine independent path-only and header-only matches. // // Parameters: // - ctx: The context for API calls and cancellation // - logger: Logger for structured logging // - cl: Kubernetes client for API operations -// - httpRoute: The HTTPRoute resource from which the KongRoute is derived -// - rule: The specific rule within the HTTPRoute from which the KongRoute is derived +// - httpRoute: The HTTPRoute resource from which the KongRoutes are derived +// - rule: The specific rule within the HTTPRoute // - pRef: The parent reference (Gateway) for the HTTPRoute -// - cp: The control plane reference for the KongRoute -// - serviceName: The name of the KongService this KongRoute should point to -// - hostnames: The hostnames for the KongRoute +// - cp: The control plane reference for the KongRoutes +// - serviceName: The name of the KongService these KongRoutes should point to +// - hostnames: The hostnames for the KongRoutes // // Returns: -// - kongRoute: The created or updated KongRoute resource +// - kongRoutes: The created or updated KongRoute resources (one per match) // - err: Any error that occurred during the process -func RouteForRule( +func RoutesForRule( ctx context.Context, logger logr.Logger, cl client.Client, @@ -51,41 +54,53 @@ func RouteForRule( cp *commonv1alpha1.ControlPlaneRef, serviceName string, hostnames []string, -) (kongRoute *configurationv1alpha1.KongRoute, err error) { - routeName := namegen.NewKongRouteName(httpRoute, cp, rule) - logger = logger.WithValues("kongroute", routeName) - log.Debug(logger, "Creating KongRoute for HTTPRoute rule") - - routeBuilder := builder.NewKongRoute(). - WithName(routeName). - WithNamespace(metadata.NamespaceFromParentRef(httpRoute, pRef)). - WithLabels(httpRoute, pRef). - WithAnnotations(httpRoute, pRef). - WithSpecName(routeName). - WithHosts(hostnames). - WithStripPath(metadata.ExtractStripPath(httpRoute.Annotations)). - WithPreserveHost(metadata.ExtractPreserveHost(httpRoute.Annotations)). - WithKongService(serviceName) +) (kongRoutes []*configurationv1alpha1.KongRoute, err error) { + // If the rule has no matches, create a single catch-all route. + // Kong requires at least one matcher; use "/" path to represent catch-all. + if len(rule.Matches) == 0 { + match := gatewayv1.HTTPRouteMatch{ + Path: &gatewayv1.HTTPPathMatch{Type: ptr.To(gatewayv1.PathMatchPathPrefix), Value: new("/")}, + } + rule.Matches = append(rule.Matches, match) + } - // Check if the rule contains a URLRewrite or RequestRedirect filter with ReplacePrefixMatch: - // if so, we need to set a capture group on the KongRoute paths. + // Check filters to determine if we need capture groups in paths. setCaptureGroup := needsCaptureGroup(rule) - // Add HTTPRoute matches - for _, match := range rule.Matches { - routeBuilder = routeBuilder.WithHTTPRouteMatch(match, setCaptureGroup) - } - newRoute, err := routeBuilder.Build() - if err != nil { - log.Error(logger, err, "Failed to build KongRoute resource") - return nil, fmt.Errorf("failed to build KongRoute %s: %w", routeName, err) - } + for i, match := range rule.Matches { + routeName := namegen.NewKongRouteNameForMatch(httpRoute, cp, match, i) + mLog := logger.WithValues("kongroute", routeName, "matchIndex", i) + log.Debug(mLog, "Creating KongRoute for HTTPRoute match") + + routeBuilder := builder.NewKongRoute(). + WithName(routeName). + WithNamespace(metadata.NamespaceFromParentRef(httpRoute, pRef)). + WithLabels(httpRoute, pRef). + WithAnnotations(httpRoute, pRef). + WithSpecName(routeName). + WithHosts(hostnames). + WithStripPath(metadata.ExtractStripPath(httpRoute.Annotations)). + WithPreserveHost(metadata.ExtractPreserveHost(httpRoute.Annotations)). + WithKongService(serviceName). + WithHTTPRouteMatch(match, setCaptureGroup) + + newRoute, buildErr := routeBuilder.Build() + if buildErr != nil { + log.Error(mLog, buildErr, "Failed to build KongRoute resource") + return nil, fmt.Errorf("failed to build KongRoute %s: %w", routeName, buildErr) + } + + if _, updErr := translator.VerifyAndUpdate(ctx, mLog, cl, &newRoute, httpRoute, true); updErr != nil { + return nil, updErr + } - if _, err = translator.VerifyAndUpdate(ctx, logger, cl, &newRoute, httpRoute, true); err != nil { - return nil, err + // Add to result slice as an explicit copy for clarity. + // Using DeepCopy expresses the intent that each match yields an + // independent KongRoute object. + kongRoutes = append(kongRoutes, newRoute.DeepCopy()) } - return &newRoute, nil + return kongRoutes, nil } // needsCaptureGroup checks if the given HTTPRoute rule requires a capture group diff --git a/controller/hybridgateway/kongroute/route_test.go b/controller/hybridgateway/kongroute/route_test.go index 5d8a86eae5..7f0424a092 100644 --- a/controller/hybridgateway/kongroute/route_test.go +++ b/controller/hybridgateway/kongroute/route_test.go @@ -15,12 +15,11 @@ import ( commonv1alpha1 "github.com/kong/kong-operator/v2/api/common/v1alpha1" configurationv1alpha1 "github.com/kong/kong-operator/v2/api/configuration/v1alpha1" - "github.com/kong/kong-operator/v2/controller/hybridgateway/builder" gwtypes "github.com/kong/kong-operator/v2/internal/types" "github.com/kong/kong-operator/v2/pkg/consts" ) -func TestRouteForRule(t *testing.T) { +func TestRoutesForRule(t *testing.T) { ctx := context.Background() logger := logr.Discard() @@ -43,15 +42,22 @@ func TestRouteForRule(t *testing.T) { }, } - // Create test rule + // Create test rule with two matches so that two KongRoutes are generated + prefix := gatewayv1.PathMatchPathPrefix rule := gwtypes.HTTPRouteRule{ Matches: []gatewayv1.HTTPRouteMatch{ { Path: &gatewayv1.HTTPPathMatch{ - Type: (*gatewayv1.PathMatchType)(new("PathPrefix")), + Type: &prefix, Value: new("/test"), }, }, + { + Headers: []gatewayv1.HTTPHeaderMatch{{ + Name: "X-Foo", + Value: "bar", + }}, + }, }, } @@ -75,13 +81,14 @@ func TestRouteForRule(t *testing.T) { serviceName string hostnames []string expectError bool - expectUpdate bool + expectRoutes int }{ { - name: "create new route", - serviceName: "test-service", - hostnames: []string{"example.com"}, - expectError: false, + name: "create new route", + serviceName: "test-service", + hostnames: []string{"example.com"}, + expectError: false, + expectRoutes: 2, }, } @@ -93,38 +100,43 @@ func TestRouteForRule(t *testing.T) { } fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() - result, err := RouteForRule(ctx, logger, fakeClient, httpRoute, rule, pRef, cpRef, tt.serviceName, tt.hostnames) + results, err := RoutesForRule(ctx, logger, fakeClient, httpRoute, rule, pRef, cpRef, tt.serviceName, tt.hostnames) if tt.expectError { assert.Error(t, err) - assert.Nil(t, result) + assert.Nil(t, results) return } require.NoError(t, err) - require.NotNil(t, result) - - // Verify the route has correct properties - assert.Equal(t, "test-namespace", result.Namespace) - assert.NotEmpty(t, result.Name) - assert.Equal(t, tt.hostnames, result.Spec.Hosts) - - // Verify service reference - if tt.serviceName != "" { - require.NotNil(t, result.Spec.ServiceRef) - assert.Equal(t, configurationv1alpha1.ServiceRefNamespacedRef, result.Spec.ServiceRef.Type) - require.NotNil(t, result.Spec.ServiceRef.NamespacedRef) - assert.Equal(t, tt.serviceName, result.Spec.ServiceRef.NamespacedRef.Name) - } - - // Verify HTTPRoute annotation - expectedAnnotation := httpRoute.Namespace + "/" + httpRoute.Name - assert.Contains(t, result.Annotations[consts.GatewayOperatorHybridRoutesAnnotation], expectedAnnotation) - - // Verify paths from rule matches - if len(rule.Matches) > 0 && rule.Matches[0].Path != nil { - paths, _ := builder.GenerateKongRoutePathFromHTTPRouteMatch(rule.Matches[0].Path, false) - assert.Subset(t, result.Spec.Paths, paths) + require.NotNil(t, results) + assert.Len(t, results, tt.expectRoutes) + + for _, result := range results { + // Verify the route has correct properties + assert.Equal(t, "test-namespace", result.Namespace) + assert.NotEmpty(t, result.Name) + assert.Equal(t, tt.hostnames, result.Spec.Hosts) + + // Verify service reference + if tt.serviceName != "" { + require.NotNil(t, result.Spec.ServiceRef) + assert.Equal(t, configurationv1alpha1.ServiceRefNamespacedRef, result.Spec.ServiceRef.Type) + require.NotNil(t, result.Spec.ServiceRef.NamespacedRef) + assert.Equal(t, tt.serviceName, result.Spec.ServiceRef.NamespacedRef.Name) + } + + // Verify HTTPRoute annotation + expectedAnnotation := httpRoute.Namespace + "/" + httpRoute.Name + assert.Contains(t, result.Annotations[consts.GatewayOperatorHybridRoutesAnnotation], expectedAnnotation) + + // Verify at least one path/header/method is set based on the match. + // For the first match with path, we expect Paths to be non-empty. + // For the second header-only match, Paths may be empty; Headers must be set. + if len(result.Spec.Paths) == 0 { + // header-only route + assert.Contains(t, result.Spec.Headers, "X-Foo") + } } }) } diff --git a/controller/hybridgateway/namegen/name.go b/controller/hybridgateway/namegen/name.go index 4a60051a20..e109874bb9 100644 --- a/controller/hybridgateway/namegen/name.go +++ b/controller/hybridgateway/namegen/name.go @@ -112,6 +112,22 @@ func NewKongRouteName(route *gwtypes.HTTPRoute, cp *commonv1alpha1.ControlPlaneR return newNameWithHashSuffix(readableElements, hashElements) } +// NewKongRouteNameForMatch generates a KongRoute name based on HTTPRoute, ControlPlaneRef, +// and a single HTTPRouteMatch. The optional index is included to avoid collisions when +// multiple matches are identical. +func NewKongRouteNameForMatch(route *gwtypes.HTTPRoute, cp *commonv1alpha1.ControlPlaneRef, match gatewayv1.HTTPRouteMatch, index int) string { + readableElements := []string{ + httpProcolPrefix, + route.Namespace + "-" + route.Name, + } + hashElements := []string{ + defaultCPPrefix + utils.Hash32(cp), + utils.Hash32(match), + fmt.Sprintf("m%02d", index), + } + return newNameWithHashSuffix(readableElements, hashElements) +} + // NewKongPluginName generates a KongPlugin name based on the HTTPRouteFilter passed as argument. func NewKongPluginName(filter gatewayv1.HTTPRouteFilter, namespace string, pluginName string) string { return newName(namespace, pluginName, utils.Hash32(filter)) diff --git a/test/conformance/skipped_tests_test.go b/test/conformance/skipped_tests_test.go index f0cfd26cef..3f6592e4ca 100644 --- a/test/conformance/skipped_tests_test.go +++ b/test/conformance/skipped_tests_test.go @@ -41,7 +41,6 @@ var skippedTestsForHybrid = []string{ tests.HTTPRouteHeaderMatching.ShortName, tests.HTTPRouteInvalidBackendRefUnknownKind.ShortName, tests.HTTPRouteMethodMatching.ShortName, - tests.HTTPRouteMatching.ShortName, tests.HTTPRouteMatchingAcrossRoutes.ShortName, tests.HTTPRoutePartiallyInvalidViaInvalidReferenceGrant.ShortName, tests.HTTPRoutePathMatchOrder.ShortName, diff --git a/test/e2e/chainsaw/hybridgateway/httproute-filters/chainsaw-test.yaml b/test/e2e/chainsaw/hybridgateway/httproute-filters/chainsaw-test.yaml index d4b5d1d62a..7438d57474 100644 --- a/test/e2e/chainsaw/hybridgateway/httproute-filters/chainsaw-test.yaml +++ b/test/e2e/chainsaw/hybridgateway/httproute-filters/chainsaw-test.yaml @@ -145,34 +145,101 @@ spec: value: - ($fqdn) # Step 11: Assert that the HTTPRoute is translated into a KongRoute resource (RequestHeaderModifier). - - name: Assert-kong-route-request-header-modifier - description: Assert that the KongRoute resource is created for the HTTPRoute with RequestHeaderModifier. + # Assert KongRoute for each match independently (per-match route generation). + - name: Assert-kong-route-request-header-modifier-get + description: Assert KongRoute for GET /get use: template: ../../common/_step_templates/assert-kongRoute.yaml bindings: - - name: route_hosts - value: ($fqdn) - name: route_paths - value: + value: - "~/get$" - "/get/" - - "~/post$" - - "~/put$" - - "~/patch$" - - "~/delete$" - - "~/status$" - - "/status/" - name: route_methods value: - "GET" + - name: strip_path + value: false + - name: resource_type + value: "kongservices.configuration.konghq.com" + + - name: Assert-kong-route-request-header-modifier-post + description: Assert KongRoute for POST /post + use: + template: ../../common/_step_templates/assert-kongRoute.yaml + bindings: + - name: route_paths + value: + - "~/post$" + - name: route_methods + value: - "POST" + - name: strip_path + value: false + - name: resource_type + value: "kongservices.configuration.konghq.com" + + - name: Assert-kong-route-request-header-modifier-put + description: Assert KongRoute for PUT /put + use: + template: ../../common/_step_templates/assert-kongRoute.yaml + bindings: + - name: route_paths + value: + - "~/put$" + - name: route_methods + value: - "PUT" + - name: strip_path + value: false + - name: resource_type + value: "kongservices.configuration.konghq.com" + + - name: Assert-kong-route-request-header-modifier-patch + description: Assert KongRoute for PATCH /patch + use: + template: ../../common/_step_templates/assert-kongRoute.yaml + bindings: + - name: route_paths + value: + - "~/patch$" + - name: route_methods + value: - "PATCH" + - name: strip_path + value: false + - name: resource_type + value: "kongservices.configuration.konghq.com" + + - name: Assert-kong-route-request-header-modifier-delete + description: Assert KongRoute for DELETE /delete + use: + template: ../../common/_step_templates/assert-kongRoute.yaml + bindings: + - name: route_paths + value: + - "~/delete$" + - name: route_methods + value: - "DELETE" - name: strip_path value: false - name: resource_type value: "kongservices.configuration.konghq.com" + + - name: Assert-kong-route-request-header-modifier-status + description: Assert KongRoute for any method /status prefix + use: + template: ../../common/_step_templates/assert-kongRoute.yaml + bindings: + - name: route_paths + value: + - "~/status$" + - "/status/" + - name: strip_path + value: false + - name: resource_type + value: "kongservices.configuration.konghq.com" # Step 12: Assert that the HTTPRoute backend is translated into a KongService resource (RequestHeaderModifier). - name: Assert-kong-service-request-header-modifier