From 638e38ea5a728e1c3e7b1c5bcda9fbd025e87c52 Mon Sep 17 00:00:00 2001 From: Shashank Ram Date: Mon, 25 Nov 2024 14:29:10 -0800 Subject: [PATCH] gateway2/delegation: fix cycle detection There's a bug in how cycles are detected when evaluating a delegation chain, because of which a multi-level delegation tree with the same child route being referenced by multiple parent HTTP routes is broken. This change fixes the detection of cycles during the evaluation of the delegation chain. --- Testing done: Added a new translator unit test which fails without the fix. Signed-off-by: Shashank Ram --- changelog/v1.18.0-rc3/fix-10379.yaml | 11 ++ projects/gateway2/query/httproute.go | 8 +- .../translator/gateway_translator_test.go | 26 ++-- .../inputs/delegation/bug-10379.yaml | 127 ++++++++++++++++++ .../outputs/delegation/bug-10379.yaml | 53 ++++++++ 5 files changed, 216 insertions(+), 9 deletions(-) create mode 100644 changelog/v1.18.0-rc3/fix-10379.yaml create mode 100644 projects/gateway2/translator/testutils/inputs/delegation/bug-10379.yaml create mode 100644 projects/gateway2/translator/testutils/outputs/delegation/bug-10379.yaml diff --git a/changelog/v1.18.0-rc3/fix-10379.yaml b/changelog/v1.18.0-rc3/fix-10379.yaml new file mode 100644 index 00000000000..5621728b06e --- /dev/null +++ b/changelog/v1.18.0-rc3/fix-10379.yaml @@ -0,0 +1,11 @@ +changelog: + - type: FIX + issueLink: https://github.com/k8sgateway/k8sgateway/issues/10379 + resolvesIssue: false + description: | + There's a bug in how cycles are detected when evaluating a + delegation chain, because of which a multi-level delegation + tree with the same child route being referenced by multiple + parent HTTP routes is broken. This change fixes the detection + of cycles during the evaluation of the delegation chain. + diff --git a/projects/gateway2/query/httproute.go b/projects/gateway2/query/httproute.go index 9c787d1c297..1fca5e20ea3 100644 --- a/projects/gateway2/query/httproute.go +++ b/projects/gateway2/query/httproute.go @@ -245,7 +245,13 @@ func (r *gatewayQueries) getDelegatedChildren( visited = sets.New[types.NamespacedName]() } parentRef := namespacedName(parent) + // `visited` is used to detect cyclic references to routes in the delegation chain. + // It is important to remove the route from the set once all its children have been evaluated + // in the recursion stack, because a route may have multiple parents that have the same ancestor: + // e.g., A -> B1, A -> B2, B1 -> C, B2 -> C. So in this case, even though C is visited twice, + // the delegation chain is valid as it is evaluated only once for each parent. visited.Insert(parentRef) + defer visited.Delete(parentRef) children := NewBackendMap[[]*RouteInfo]() for _, parentRule := range parent.Spec.Rules { @@ -264,7 +270,7 @@ func (r *gatewayQueries) getDelegatedChildren( for _, childRoute := range referencedRoutes { childRef := namespacedName(&childRoute) if visited.Has(childRef) { - err := fmt.Errorf("ignoring child route %s for parent %s: %w", parentRef, childRef, ErrCyclicReference) + err := fmt.Errorf("ignoring child route %s for parent %s: %w", childRef, parentRef, ErrCyclicReference) children.AddError(backendRef.BackendObjectReference, err) // Don't resolve invalid child route continue diff --git a/projects/gateway2/translator/gateway_translator_test.go b/projects/gateway2/translator/gateway_translator_test.go index e54b53c643a..24a861e2568 100644 --- a/projects/gateway2/translator/gateway_translator_test.go +++ b/projects/gateway2/translator/gateway_translator_test.go @@ -51,7 +51,8 @@ var _ = DescribeTable("Basic GatewayTranslator Tests", gwNN: types.NamespacedName{ Namespace: "default", Name: "example-gateway", - }}), + }, + }), Entry( "https gateway with basic routing", translatorTestCase{ @@ -60,7 +61,8 @@ var _ = DescribeTable("Basic GatewayTranslator Tests", gwNN: types.NamespacedName{ Namespace: "default", Name: "example-gateway", - }}), + }, + }), Entry( "http gateway with multiple listeners on the same port", translatorTestCase{ @@ -69,7 +71,8 @@ var _ = DescribeTable("Basic GatewayTranslator Tests", gwNN: types.NamespacedName{ Namespace: "default", Name: "http", - }}), + }, + }), Entry( "https gateway with multiple listeners on the same port", translatorTestCase{ @@ -78,7 +81,8 @@ var _ = DescribeTable("Basic GatewayTranslator Tests", gwNN: types.NamespacedName{ Namespace: "default", Name: "http", - }}), + }, + }), Entry( "http gateway with multiple routing rules and HeaderModifier filter", translatorTestCase{ @@ -87,7 +91,8 @@ var _ = DescribeTable("Basic GatewayTranslator Tests", gwNN: types.NamespacedName{ Namespace: "default", Name: "gw", - }}), + }, + }), Entry( "http gateway with lambda destination", translatorTestCase{ @@ -96,7 +101,8 @@ var _ = DescribeTable("Basic GatewayTranslator Tests", gwNN: types.NamespacedName{ Namespace: "default", Name: "gw", - }}), + }, + }), Entry( "http gateway with azure destination", translatorTestCase{ @@ -105,7 +111,8 @@ var _ = DescribeTable("Basic GatewayTranslator Tests", gwNN: types.NamespacedName{ Namespace: "default", Name: "gw", - }}), + }, + }), Entry( "gateway with correctly sorted routes", translatorTestCase{ @@ -114,7 +121,8 @@ var _ = DescribeTable("Basic GatewayTranslator Tests", gwNN: types.NamespacedName{ Namespace: "infra", Name: "example-gateway", - }}), + }, + }), Entry( "httproute with missing backend reports correctly", translatorTestCase{ @@ -300,4 +308,6 @@ var _ = DescribeTable("Route Delegation translator", Entry("RouteOptions multi level inheritance with child override", "route_options_multi_level_inheritance_override_ok.yaml"), Entry("RouteOptions filter override merge", "route_options_filter_override_merge.yaml"), Entry("Child route matcher does not match parent", "bug-6621.yaml"), + // https://github.com/k8sgateway/k8sgateway/issues/10379 + Entry("Multi-level multiple parents delegation", "bug-10379.yaml"), ) diff --git a/projects/gateway2/translator/testutils/inputs/delegation/bug-10379.yaml b/projects/gateway2/translator/testutils/inputs/delegation/bug-10379.yaml new file mode 100644 index 00000000000..ac074c256d0 --- /dev/null +++ b/projects/gateway2/translator/testutils/inputs/delegation/bug-10379.yaml @@ -0,0 +1,127 @@ +apiVersion: gateway.networking.k8s.io/v1 +kind: Gateway +metadata: + name: example-gateway + namespace: infra +spec: + gatewayClassName: example-gateway-class + listeners: + - name: http + protocol: HTTP + port: 80 +--- +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: + name: api-example-com + namespace: infra + labels: + app: apis +spec: + parentRefs: + - name: example-gateway + hostnames: + - "api.example.com" + rules: + - matches: + - path: + type: PathPrefix + value: /api1 + backendRefs: + - group: gateway.networking.k8s.io + kind: HTTPRoute + name: apiproduct-1 + namespace: default + - matches: + - path: + type: PathPrefix + value: /api2 + backendRefs: + - group: gateway.networking.k8s.io + kind: HTTPRoute + name: apiproduct-2 + namespace: default +--- +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: + name: apiproduct-1 + namespace: default + labels: + app: apis + annotations: + delegation.gateway.solo.io/inherit-parent-matcher: "true" +spec: + rules: + - matches: + - path: + type: PathPrefix + value: / + filters: + - type: URLRewrite + urlRewrite: + path: + type: ReplacePrefixMatch + replacePrefixMatch: / + backendRefs: + - group: gateway.networking.k8s.io + kind: HTTPRoute + name: httpbin + namespace: default +--- +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: + name: apiproduct-2 + namespace: default + labels: + app: apis + annotations: + delegation.gateway.solo.io/inherit-parent-matcher: "true" +spec: + rules: + - matches: + - path: + type: PathPrefix + value: / + filters: + - type: URLRewrite + urlRewrite: + path: + type: ReplacePrefixMatch + replacePrefixMatch: / + backendRefs: + - group: gateway.networking.k8s.io + kind: HTTPRoute + name: httpbin + namespace: default +--- +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: + name: httpbin + namespace: default + labels: + app: apis + annotations: + delegation.gateway.solo.io/inherit-parent-matcher: "true" +spec: + rules: + - matches: + - path: + type: PathPrefix + value: / + backendRefs: + - name: httpbin + namespace: default + port: 8000 +--- +apiVersion: v1 +kind: Service +metadata: + name: httpbin + namespace: default +spec: + ports: + - protocol: TCP + port: 8000 diff --git a/projects/gateway2/translator/testutils/outputs/delegation/bug-10379.yaml b/projects/gateway2/translator/testutils/outputs/delegation/bug-10379.yaml new file mode 100644 index 00000000000..8769e60937e --- /dev/null +++ b/projects/gateway2/translator/testutils/outputs/delegation/bug-10379.yaml @@ -0,0 +1,53 @@ +--- +listeners: +- aggregateListener: + httpFilterChains: + - matcher: {} + virtualHostRefs: + - http~api_example_com + httpResources: + virtualHosts: + http~api_example_com: + domains: + - api.example.com + name: http~api_example_com + routes: + - matchers: + - prefix: /api1 + name: httproute-httpbin-default-0-0 + options: + regexRewrite: + pattern: + regex: ^/api1\/* + substitution: / + routeAction: + single: + kube: + port: 8000 + ref: + name: httpbin + namespace: default + - matchers: + - prefix: /api2 + name: httproute-httpbin-default-0-0 + options: + regexRewrite: + pattern: + regex: ^/api2\/* + substitution: / + routeAction: + single: + kube: + port: 8000 + ref: + name: httpbin + namespace: default + bindAddress: '::' + bindPort: 8080 + name: http +metadata: + labels: + created_by: gloo-kube-gateway-api + gateway_namespace: infra + name: infra-example-gateway + namespace: gloo-system