diff --git a/internal/dataplane/translator/subtranslator/atc_utils.go b/internal/dataplane/translator/subtranslator/atc_utils.go index 94b4882b49..a5e9b4ef89 100644 --- a/internal/dataplane/translator/subtranslator/atc_utils.go +++ b/internal/dataplane/translator/subtranslator/atc_utils.go @@ -1,6 +1,7 @@ package subtranslator import ( + "fmt" "strings" "github.com/kong/kubernetes-ingress-controller/v3/internal/dataplane/translator/atc" @@ -62,3 +63,38 @@ func hostMatcherFromHosts(hosts []string) atc.Matcher { } return atc.Or(matchers...) } + +// pathSegmentMatcher returns matcher to match path segment rules. +// a '*' represents any non-empty string in the segment. +// a '**' which can only appear in the last segment represents any path, including empty. +func pathSegmentMatcherFromPath(path string) (atc.Matcher, error) { + path = strings.Trim(path, "/") + segments := strings.Split(path, "/") + predicates := make([]atc.Matcher, 0) + + numSegments := len(segments) + var segmentLenPredicate atc.Predicate + if segments[numSegments-1] == "**" { + segmentLenPredicate = atc.NewPredicateHTTPPathSegmentLength(atc.OpGreaterEqual, numSegments-1) + } else { + segmentLenPredicate = atc.NewPredicateHTTPPathSegmentLength(atc.OpEqual, numSegments) + } + predicates = append(predicates, segmentLenPredicate) + + for index, segment := range segments { + if segment == "**" { + if index != numSegments-1 { + return nil, fmt.Errorf("'**' can only appear on the last segment") + } + continue + } + if segment == "*" { + continue + } + + segment = strings.ReplaceAll(segment, "\\*", "*") + predicates = append(predicates, atc.NewPredicateHTTPPathSingleSegment(index, atc.OpEqual, segment)) + } + + return atc.And(predicates...), nil +} diff --git a/internal/dataplane/translator/subtranslator/httproute_atc.go b/internal/dataplane/translator/subtranslator/httproute_atc.go index 03248ece57..af1eb58f16 100644 --- a/internal/dataplane/translator/subtranslator/httproute_atc.go +++ b/internal/dataplane/translator/subtranslator/httproute_atc.go @@ -57,7 +57,8 @@ func GenerateKongExpressionRoutesFromHTTPRouteMatches( } // if we do not need to generate a kong route for each match, we OR matchers from all matches together. - routeMatcher := atc.And(atc.Or(generateMatchersFromHTTPRouteMatches(translation.Matches)...)) + matchers, _ := generateMatchersFromHTTPRouteMatches(translation.Matches, ingressObjectInfo.Annotations) + routeMatcher := atc.And(atc.Or(matchers...)) // Add matcher from parent httproute (hostnames, SNIs) to be ANDed with the matcher from match. matchersFromParent := matchersFromParentHTTPRoute(hostnames, ingressObjectInfo.Annotations) for _, matcher := range matchersFromParent { @@ -91,7 +92,8 @@ func generateKongExpressionRoutesWithRequestRedirectFilter( ExpressionRoutes: true, } // generate matcher for this HTTPRoute Match. - matcher := atc.And(generateMatcherFromHTTPRouteMatch(match)) + matcherFromMatch, _ := generateMatcherFromHTTPRouteMatch(match, ingressObjectInfo.Annotations) + matcher := atc.And(matcherFromMatch) // add matcher from parent httproute (hostnames, protocols, SNIs) to be ANDed with the matcher from match. matchersFromParent := matchersFromParentHTTPRoute(hostnames, ingressObjectInfo.Annotations) @@ -113,20 +115,28 @@ func generateKongExpressionRoutesWithRequestRedirectFilter( return routes, nil } -func generateMatchersFromHTTPRouteMatches(matches []gatewayapi.HTTPRouteMatch) []atc.Matcher { +func generateMatchersFromHTTPRouteMatches(matches []gatewayapi.HTTPRouteMatch, routeAnnotations map[string]string) ([]atc.Matcher, []error) { ret := make([]atc.Matcher, 0, len(matches)) + errs := make([]error, 0) for _, match := range matches { - matcher := generateMatcherFromHTTPRouteMatch(match) + matcher, err := generateMatcherFromHTTPRouteMatch(match, routeAnnotations) + if err != nil { + errs = append(errs, err) + } ret = append(ret, matcher) } - return ret + return ret, errs } -func generateMatcherFromHTTPRouteMatch(match gatewayapi.HTTPRouteMatch) atc.Matcher { +func generateMatcherFromHTTPRouteMatch(match gatewayapi.HTTPRouteMatch, routeAnnotations map[string]string) (atc.Matcher, error) { matcher := atc.And() if match.Path != nil { - pathMatcher := pathMatcherFromHTTPPathMatch(match.Path) + segmentPrefix := annotations.ExtractSegmentPrefix(routeAnnotations) + pathMatcher, err := pathMatcherFromHTTPPathMatch(match.Path, segmentPrefix) + if err != nil { + return nil, err + } matcher.And(pathMatcher) } @@ -145,7 +155,7 @@ func generateMatcherFromHTTPRouteMatch(match gatewayapi.HTTPRouteMatch) atc.Matc methodMatcher := methodMatcherFromMethods([]string{string(method)}) matcher.And(methodMatcher) } - return matcher + return matcher, nil } func appendRegexBeginIfNotExist(regex string) string { @@ -155,17 +165,17 @@ func appendRegexBeginIfNotExist(regex string) string { return regex } -func pathMatcherFromHTTPPathMatch(pathMatch *gatewayapi.HTTPPathMatch) atc.Matcher { +func pathMatcherFromHTTPPathMatch(pathMatch *gatewayapi.HTTPPathMatch, segmentPrefix string) (atc.Matcher, error) { path := "" if pathMatch.Value != nil { path = *pathMatch.Value } switch *pathMatch.Type { case gatewayapi.PathMatchExact: - return atc.NewPredicateHTTPPath(atc.OpEqual, path) + return atc.NewPredicateHTTPPath(atc.OpEqual, path), nil case gatewayapi.PathMatchPathPrefix: if path == "" || path == "/" { - return atc.NewPredicateHTTPPath(atc.OpPrefixMatch, "/") + return atc.NewPredicateHTTPPath(atc.OpPrefixMatch, "/"), nil } // if path ends with /, we should remove the trailing / because it should be ignored: // https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io/v1.PathMatchType @@ -173,15 +183,18 @@ func pathMatcherFromHTTPPathMatch(pathMatch *gatewayapi.HTTPPathMatch) atc.Match return atc.Or( atc.NewPredicateHTTPPath(atc.OpEqual, path), atc.NewPredicateHTTPPath(atc.OpPrefixMatch, path+"/"), - ) + ), nil case gatewayapi.PathMatchRegularExpression: + if segmentPrefix != "" && strings.HasPrefix(path, segmentPrefix) { + return pathSegmentMatcherFromPath(strings.TrimPrefix(path, segmentPrefix)) + } // TODO: for compatibility with kong traditional routes, here we append the ^ prefix to match the path from beginning. // Could we allow the regex to match any part of the path? // https://github.com/Kong/kubernetes-ingress-controller/issues/3983 - return atc.NewPredicateHTTPPath(atc.OpRegexMatch, appendRegexBeginIfNotExist(path)) + return atc.NewPredicateHTTPPath(atc.OpRegexMatch, appendRegexBeginIfNotExist(path)), nil } - return nil // should be unreachable + return nil, fmt.Errorf("path match type %s not supported", *pathMatch.Type) // should be unreachable } func headerMatcherFromHTTPHeaderMatch(headerMatch gatewayapi.HTTPHeaderMatch) atc.Matcher { @@ -575,7 +588,8 @@ func KongExpressionRouteFromHTTPRouteMatchWithPriority( hostnames := []string{match.Hostname} matchers := matchersFromParentHTTPRoute(hostnames, httproute.Annotations) // generate ATC matcher from split HTTPRouteMatch itself. - matchers = append(matchers, generateMatcherFromHTTPRouteMatch(match.Match)) + matcherFromMatch, _ := generateMatcherFromHTTPRouteMatch(match.Match, httproute.Annotations) + matchers = append(matchers, matcherFromMatch) atc.ApplyExpression(&r.Route, atc.And(matchers...), httpRouteMatchWithPriority.Priority) diff --git a/internal/dataplane/translator/subtranslator/httproute_atc_test.go b/internal/dataplane/translator/subtranslator/httproute_atc_test.go index be070b7c42..a204ec2ad4 100644 --- a/internal/dataplane/translator/subtranslator/httproute_atc_test.go +++ b/internal/dataplane/translator/subtranslator/httproute_atc_test.go @@ -328,7 +328,9 @@ func TestGenerateMatcherFromHTTPRouteMatch(t *testing.T) { for _, tc := range testCases { tc := tc t.Run(tc.name, func(t *testing.T) { - require.Equal(t, tc.expression, generateMatcherFromHTTPRouteMatch(tc.match).Expression()) + matcher, err := generateMatcherFromHTTPRouteMatch(tc.match, map[string]string{}) + require.NoError(t, err) + require.Equal(t, tc.expression, matcher.Expression()) }) } } diff --git a/internal/dataplane/translator/subtranslator/ingress_atc.go b/internal/dataplane/translator/subtranslator/ingress_atc.go index 832b89d7b0..c9c7b65c32 100644 --- a/internal/dataplane/translator/subtranslator/ingress_atc.go +++ b/internal/dataplane/translator/subtranslator/ingress_atc.go @@ -158,7 +158,7 @@ func pathMatcherFromIngressPath(httpIngressPath netv1.HTTPIngressPath, regexPath // segment match path. if segmentPathPrefix != "" && strings.HasPrefix(httpIngressPath.Path, segmentPathPrefix) { segmentMatchingPath := strings.TrimPrefix(httpIngressPath.Path, segmentPathPrefix) - return pathSegmentMatcherFromIngressMatch(segmentMatchingPath) + return pathSegmentMatcherFromPath(segmentMatchingPath) } return atc.NewPredicateHTTPPath(atc.OpPrefixMatch, httpIngressPath.Path), nil } @@ -166,38 +166,6 @@ func pathMatcherFromIngressPath(httpIngressPath netv1.HTTPIngressPath, regexPath return nil, nil } -func pathSegmentMatcherFromIngressMatch(path string) (atc.Matcher, error) { - path = strings.Trim(path, "/") - segments := strings.Split(path, "/") - predicates := make([]atc.Matcher, 0) - - numSegments := len(segments) - var segmentLenPredicate atc.Predicate - if segments[numSegments-1] == "**" { - segmentLenPredicate = atc.NewPredicateHTTPPathSegmentLength(atc.OpGreaterEqual, numSegments-1) - } else { - segmentLenPredicate = atc.NewPredicateHTTPPathSegmentLength(atc.OpEqual, numSegments) - } - predicates = append(predicates, segmentLenPredicate) - - for index, segment := range segments { - if segment == "**" { - if index != numSegments-1 { - return nil, fmt.Errorf("'**' can only appear on the last segment") - } - continue - } - if segment == "*" { - continue - } - - segment = strings.ReplaceAll(segment, "\\*", "*") - predicates = append(predicates, atc.NewPredicateHTTPPathSingleSegment(index, atc.OpEqual, segment)) - } - - return atc.And(predicates...), nil -} - // headerMatcherFromHeaders generates matcher to match headers in HTTP requests. func headerMatcherFromHeaders(headers map[string][]string) atc.Matcher { matchers := make([]atc.Matcher, 0, len(headers)) diff --git a/test/integration/httproute_test.go b/test/integration/httproute_test.go index d61a65c729..3e523410c2 100644 --- a/test/integration/httproute_test.go +++ b/test/integration/httproute_test.go @@ -675,3 +675,88 @@ func TestHTTPRouteFilterHosts(t *testing.T) { t.Logf("test host matched in httproute, but not in listeners") require.False(t, testGetByHost(t, "another.specific.io")) } + +func TestHTTPRoutePathSegmentMatch(t *testing.T) { + RunWhenKongVersion(t, ">=3.6.0", "Path segment match is only supported in Kong 3.6.0+ with expression router") + RunWhenKongExpressionRouter(t) + + ctx := context.Background() + + ns, cleaner := helpers.Setup(ctx, t, env) + + t.Log("getting a gateway client") + gatewayClient, err := gatewayclient.NewForConfig(env.Cluster().Config()) + require.NoError(t, err) + + t.Log("deploying a new gatewayClass") + gatewayClassName := uuid.NewString() + gwc, err := helpers.DeployGatewayClass(ctx, gatewayClient, gatewayClassName) + require.NoError(t, err) + cleaner.Add(gwc) + + t.Log("deploying a new gateway") + gatewayName := uuid.NewString() + gateway, err := helpers.DeployGateway(ctx, gatewayClient, ns.Name, gatewayClassName, func(gw *gatewayapi.Gateway) { + gw.Name = gatewayName + }) + require.NoError(t, err) + cleaner.Add(gateway) + + t.Log("deploying a minimal HTTP container deployment to test Ingress routes") + container := generators.NewContainer("httpbin", test.HTTPBinImage, test.HTTPBinPort) + deployment := generators.NewDeploymentForContainer(container) + deployment, err = env.Cluster().Client().AppsV1().Deployments(ns.Name).Create(ctx, deployment, metav1.CreateOptions{}) + require.NoError(t, err) + + t.Logf("exposing deployment %s via service", deployment.Name) + service := generators.NewServiceForDeployment(deployment, corev1.ServiceTypeLoadBalancer) + _, err = env.Cluster().Client().CoreV1().Services(ns.Name).Create(ctx, service, metav1.CreateOptions{}) + require.NoError(t, err) + + t.Logf("creating an httproute to access deployment %s via kong", deployment.Name) + httpPort := gatewayapi.PortNumber(80) + httpRoute := &gatewayapi.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Name: uuid.NewString(), + Annotations: map[string]string{ + annotations.AnnotationPrefix + annotations.SegmentPrefixKey: "/#", + }, + }, + Spec: gatewayapi.HTTPRouteSpec{ + CommonRouteSpec: gatewayapi.CommonRouteSpec{ + ParentRefs: []gatewayapi.ParentReference{{ + Name: gatewayapi.ObjectName(gateway.Name), + }}, + }, + Rules: []gatewayapi.HTTPRouteRule{{ + Matches: []gatewayapi.HTTPRouteMatch{ + { + Path: &gatewayapi.HTTPPathMatch{ + Type: lo.ToPtr(gatewayapi.PathMatchRegularExpression), + Value: lo.ToPtr("/#/status/*"), + }, + }, + }, + BackendRefs: []gatewayapi.HTTPBackendRef{{ + BackendRef: gatewayapi.BackendRef{ + BackendObjectReference: gatewayapi.BackendObjectReference{ + Name: gatewayapi.ObjectName(service.Name), + Port: &httpPort, + Kind: util.StringToGatewayAPIKindPtr("Service"), + }, + }, + }}, + }}, + }, + } + _, err = gatewayClient.GatewayV1().HTTPRoutes(ns.Name).Create(ctx, httpRoute, metav1.CreateOptions{}) + require.NoError(t, err) + cleaner.Add(httpRoute) + + // paths that should match /status/* + helpers.EventuallyGETPath(t, proxyHTTPURL, proxyHTTPURL.Host, "/status/200", http.StatusOK, "", emptyHeaderSet, ingressWait, waitTick) + helpers.EventuallyGETPath(t, proxyHTTPURL, proxyHTTPURL.Host, "/status/204", http.StatusNoContent, "", emptyHeaderSet, ingressWait, waitTick) + helpers.EventuallyGETPath(t, proxyHTTPURL, proxyHTTPURL.Host, "/status/404", http.StatusNotFound, "", emptyHeaderSet, ingressWait, waitTick) + // paths that should not match + helpers.EventuallyGETPath(t, proxyHTTPURL, proxyHTTPURL.Host, "/status/200/aaa", http.StatusNotFound, "no Route matched", emptyHeaderSet, ingressWait, waitTick) +} diff --git a/test/integration/ingress_test.go b/test/integration/ingress_test.go index 9610d4246d..122145b202 100644 --- a/test/integration/ingress_test.go +++ b/test/integration/ingress_test.go @@ -1204,6 +1204,53 @@ func TestIngressRewriteURI(t *testing.T) { require.NoError(t, <-backgroundTestError, "for Ingress without rewrite run in background") } +func TestIngressPathSegmentMatch(t *testing.T) { + ctx := context.Background() + + ns, cleaner := helpers.Setup(ctx, t, env) + + t.Log("deploying a minimal HTTP container deployment to test Ingress routes") + container := generators.NewContainer("httpbin", test.HTTPBinImage, test.HTTPBinPort) + deployment := generators.NewDeploymentForContainer(container) + deployment, err := env.Cluster().Client().AppsV1().Deployments(ns.Name).Create(ctx, deployment, metav1.CreateOptions{}) + require.NoError(t, err) + cleaner.Add(deployment) + + t.Logf("exposing deployment %s via service", deployment.Name) + service := generators.NewServiceForDeployment(deployment, corev1.ServiceTypeLoadBalancer) + service, err = env.Cluster().Client().CoreV1().Services(ns.Name).Create(ctx, service, metav1.CreateOptions{}) + require.NoError(t, err) + cleaner.Add(service) + + t.Logf("creating an ingress for service %s with fixed host", service.Name) + ingress := generators.NewIngressForService("/#/status/*", map[string]string{ + annotations.AnnotationPrefix + annotations.SegmentPrefixKey: "/#", + }, service) + ingress.Spec.IngressClassName = kong.String(consts.IngressClass) + for i, rule := range ingress.Spec.Rules { + for j := range rule.HTTP.Paths { + ingress.Spec.Rules[i].HTTP.Paths[j].PathType = lo.ToPtr(netv1.PathTypeImplementationSpecific) + } + } + require.NoError(t, clusters.DeployIngress(ctx, env.Cluster(), ns.Name, ingress)) + cleaner.Add(ingress) + + require.Eventually(t, func() bool { + lbstatus, err := clusters.GetIngressLoadbalancerStatus(ctx, env.Cluster(), ns.Name, ingress) + if err != nil { + return false + } + return len(lbstatus.Ingress) > 0 + }, statusWait, waitTick) + + // paths that should match /status/* + helpers.EventuallyGETPath(t, proxyHTTPURL, proxyHTTPURL.Host, "/status/200", http.StatusOK, "", emptyHeaderSet, ingressWait, waitTick) + helpers.EventuallyGETPath(t, proxyHTTPURL, proxyHTTPURL.Host, "/status/204", http.StatusNoContent, "", emptyHeaderSet, ingressWait, waitTick) + helpers.EventuallyGETPath(t, proxyHTTPURL, proxyHTTPURL.Host, "/status/404", http.StatusNotFound, "", emptyHeaderSet, ingressWait, waitTick) + // paths that should not match + helpers.EventuallyGETPath(t, proxyHTTPURL, proxyHTTPURL.Host, "/status/200/aaa", http.StatusNotFound, "no Route matched", emptyHeaderSet, ingressWait, waitTick) +} + // setIngressClassNameWithRetry changes Ingress.Spec.IngressClassName to specified value // and retries if update conflict happens. func setIngressClassNameWithRetry(ctx context.Context, namespace string, ingress *netv1.Ingress, ingressClassName *string) error {