From ca568636aa0c95d4c61e1784e659f7af1abda360 Mon Sep 17 00:00:00 2001 From: Yi Tao Date: Thu, 21 Mar 2024 16:30:18 +0800 Subject: [PATCH 1/3] feat: implement translator to translate path segment matching --- internal/annotations/annotations.go | 5 ++ .../dataplane/translator/atc/predicate.go | 2 + .../translator/subtranslator/ingress_atc.go | 36 +++++++++++- .../subtranslator/ingress_atc_test.go | 56 +++++++++++++++++-- 4 files changed, 92 insertions(+), 7 deletions(-) diff --git a/internal/annotations/annotations.go b/internal/annotations/annotations.go index 37795eddf1..dac0416b62 100644 --- a/internal/annotations/annotations.go +++ b/internal/annotations/annotations.go @@ -56,6 +56,7 @@ const ( ResponseBuffering = "/response-buffering" HostAliasesKey = "/host-aliases" RegexPrefixKey = "/regex-prefix" + SegmentPrefixKey = "/segment-prefix" ConnectTimeoutKey = "/connect-timeout" WriteTimeoutKey = "/write-timeout" ReadTimeoutKey = "/read-timeout" @@ -212,6 +213,10 @@ func ExtractRegexPrefix(anns map[string]string) string { return anns[AnnotationPrefix+RegexPrefixKey] } +func ExtractSegmentPrefix(anns map[string]string) string { + return anns[AnnotationPrefix+SegmentPrefixKey] +} + // HasServiceUpstreamAnnotation returns true if the annotation // ingress.kubernetes.io/service-upstream is set to "true" in anns. func HasServiceUpstreamAnnotation(anns map[string]string) bool { diff --git a/internal/dataplane/translator/atc/predicate.go b/internal/dataplane/translator/atc/predicate.go index 43313860d0..62f3b8a0e1 100644 --- a/internal/dataplane/translator/atc/predicate.go +++ b/internal/dataplane/translator/atc/predicate.go @@ -105,6 +105,8 @@ type Predicate struct { value Literal } +var _ Matcher = Predicate{} + // Expression returns a string representation of a Predicate. func (p Predicate) Expression() string { lhs := p.field.String() diff --git a/internal/dataplane/translator/subtranslator/ingress_atc.go b/internal/dataplane/translator/subtranslator/ingress_atc.go index cc834d5a9b..cb2d2d8b90 100644 --- a/internal/dataplane/translator/subtranslator/ingress_atc.go +++ b/internal/dataplane/translator/subtranslator/ingress_atc.go @@ -76,8 +76,10 @@ func (m *ingressTranslationMeta) translateIntoKongExpressionRoute() *kongstate.R if pathRegexPrefix == "" { pathRegexPrefix = ControllerPathRegexPrefix } + pathSegmentPrefix := annotations.ExtractSegmentPrefix(ingressAnnotations) + for _, path := range m.paths { - pathMatchers = append(pathMatchers, pathMatcherFromIngressPath(path, pathRegexPrefix)) + pathMatchers = append(pathMatchers, pathMatcherFromIngressPath(path, pathRegexPrefix, pathSegmentPrefix)) } routeMatcher.And(atc.Or(pathMatchers...)) @@ -108,7 +110,7 @@ func (m *ingressTranslationMeta) translateIntoKongExpressionRoute() *kongstate.R } // pathMatcherFromIngressPath translate ingress path into matcher to match the path. -func pathMatcherFromIngressPath(httpIngressPath netv1.HTTPIngressPath, regexPathPrefix string) atc.Matcher { +func pathMatcherFromIngressPath(httpIngressPath netv1.HTTPIngressPath, regexPathPrefix string, segmentPathPrefix string) atc.Matcher { switch *httpIngressPath.PathType { // Prefix paths. case netv1.PathTypePrefix: @@ -145,12 +147,42 @@ func pathMatcherFromIngressPath(httpIngressPath netv1.HTTPIngressPath, regexPath } return atc.NewPredicateHTTPPath(atc.OpRegexMatch, regex) } + // segment match path. + if segmentPathPrefix != "" && strings.HasPrefix(httpIngressPath.Path, segmentPathPrefix) { + segmentMatchingPath := strings.TrimPrefix(httpIngressPath.Path, segmentPathPrefix) + return pathSegmentMatcherFromIngressMatch(segmentMatchingPath) + } return atc.NewPredicateHTTPPath(atc.OpPrefixMatch, httpIngressPath.Path) } return nil } +func pathSegmentMatcherFromIngressMatch(path string) atc.Matcher { + 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 == "*" || segment == "**" { + continue + } + segment = strings.ReplaceAll(segment, "\\*", "*") + predicates = append(predicates, atc.NewPredicateHTTPPathSingleSegment(index, atc.OpEqual, segment)) + } + + return atc.And(predicates...) +} + // 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/internal/dataplane/translator/subtranslator/ingress_atc_test.go b/internal/dataplane/translator/subtranslator/ingress_atc_test.go index 5c213cd5dd..2dfff0576e 100644 --- a/internal/dataplane/translator/subtranslator/ingress_atc_test.go +++ b/internal/dataplane/translator/subtranslator/ingress_atc_test.go @@ -618,10 +618,11 @@ func TestEncodeIngressRoutePriorityFromTraits(t *testing.T) { func TestPathMatcherFromIngressPath(t *testing.T) { testCases := []struct { - name string - path netv1.HTTPIngressPath - regexPrefix string - expression string + name string + path netv1.HTTPIngressPath + regexPrefix string + segmentPrefix string + expression string }{ { name: "simple prefix match", @@ -679,6 +680,51 @@ func TestPathMatcherFromIngressPath(t *testing.T) { }, expression: `http.path ~ "^/"`, }, + { + name: "segment match without wildcard", + path: netv1.HTTPIngressPath{ + Path: "/!/api/foo", + PathType: &pathTypeImplementationSpecific, + }, + segmentPrefix: "/!", + expression: `(http.path.segments.len == 2) && (http.path.segments.0 == "api") && (http.path.segments.1 == "foo")`, + }, + { + name: "segment match with wildcard", + path: netv1.HTTPIngressPath{ + Path: "/!/api/foo/*/bar", + PathType: &pathTypeImplementationSpecific, + }, + segmentPrefix: "/!", + expression: `(http.path.segments.len == 4) && (http.path.segments.0 == "api") && (http.path.segments.1 == "foo") && (http.path.segments.3 == "bar")`, + }, + { + name: "segments match with trailing **", + path: netv1.HTTPIngressPath{ + Path: "/SEG_PREFIX/SEG_PREFIX/**", + PathType: &pathTypeImplementationSpecific, + }, + segmentPrefix: "/SEG_PREFIX", + expression: `(http.path.segments.len >= 1) && (http.path.segments.0 == "SEG_PREFIX")`, + }, + { + name: "segment match with a literal * not representing wildcard", + path: netv1.HTTPIngressPath{ + Path: "/!/api/*/foo/\\*/bar", + PathType: &pathTypeImplementationSpecific, + }, + segmentPrefix: "/!", + expression: `(http.path.segments.len == 5) && (http.path.segments.0 == "api") && (http.path.segments.2 == "foo") && (http.path.segments.3 == "*") && (http.path.segments.4 == "bar")`, + }, + { + name: "segments match with only a **", + path: netv1.HTTPIngressPath{ + Path: "/!/**", + PathType: &pathTypeImplementationSpecific, + }, + segmentPrefix: "/!", + expression: `http.path.segments.len >= 0`, + }, { name: "empty implementation specific (non-regex) match", path: netv1.HTTPIngressPath{ @@ -696,7 +742,7 @@ func TestPathMatcherFromIngressPath(t *testing.T) { if regexPrefix == "" { regexPrefix = ControllerPathRegexPrefix } - matcher := pathMatcherFromIngressPath(tc.path, regexPrefix) + matcher := pathMatcherFromIngressPath(tc.path, regexPrefix, tc.segmentPrefix) require.Equal(t, tc.expression, matcher.Expression()) }) } From 73f44e9c8212490b97a8dcf5a17adc2d433861e7 Mon Sep 17 00:00:00 2001 From: Yi Tao Date: Mon, 25 Mar 2024 17:23:28 +0800 Subject: [PATCH 2/3] add checks and validations for path segment match --- .../admission/validation/ingress/ingress.go | 18 ++++++++ .../translator/subtranslator/ingress.go | 6 ++- .../translator/subtranslator/ingress_atc.go | 45 ++++++++++++------- .../subtranslator/ingress_atc_test.go | 22 ++++++++- internal/versions/versions.go | 6 +++ 5 files changed, 79 insertions(+), 18 deletions(-) diff --git a/internal/admission/validation/ingress/ingress.go b/internal/admission/validation/ingress/ingress.go index 9b7a8a3d4b..4470ba085d 100644 --- a/internal/admission/validation/ingress/ingress.go +++ b/internal/admission/validation/ingress/ingress.go @@ -10,6 +10,7 @@ import ( netv1 "k8s.io/api/networking/v1" "github.com/kong/kubernetes-ingress-controller/v3/internal/admission/validation" + "github.com/kong/kubernetes-ingress-controller/v3/internal/annotations" "github.com/kong/kubernetes-ingress-controller/v3/internal/dataplane/failures" "github.com/kong/kubernetes-ingress-controller/v3/internal/dataplane/translator" "github.com/kong/kubernetes-ingress-controller/v3/internal/dataplane/translator/subtranslator" @@ -37,6 +38,9 @@ func ValidateIngress( if err := validation.ValidateRouteSourceAnnotations(ingress); err != nil { return false, fmt.Sprintf("Ingress has invalid Kong annotations: %s", err), nil } + if err := ValidatePathSegmentMatchPrefix(ingress, translatorFeatures); err != nil { + return false, fmt.Sprintf("Ingress has invalid specification of path segment matches: %s", err), nil + } for _, kg := range ingressToKongRoutesForValidation(translatorFeatures, ingress, failuresCollector, storer) { kg := kg @@ -87,3 +91,17 @@ func ingressToKongRoutesForValidation( } return kongRoutes } + +func ValidatePathSegmentMatchPrefix(ingress *netv1.Ingress, translatorFeatures translator.FeatureFlags) error { + segmentPrefix := annotations.ExtractSegmentPrefix(ingress.Annotations) + if segmentPrefix == "" { + return nil + } + if !strings.HasPrefix(segmentPrefix, "/") || len(segmentPrefix) < 2 { + return fmt.Errorf("invalid segment match prefix %s: must be started with '/' and has at least 2 characters", segmentPrefix) + } + if !translatorFeatures.ExpressionRoutes { + return fmt.Errorf("path segment match is only supported when expression routes enabled") + } + return nil +} diff --git a/internal/dataplane/translator/subtranslator/ingress.go b/internal/dataplane/translator/subtranslator/ingress.go index c5a8460621..f982f9d36c 100644 --- a/internal/dataplane/translator/subtranslator/ingress.go +++ b/internal/dataplane/translator/subtranslator/ingress.go @@ -230,7 +230,11 @@ func (i *ingressTranslationIndex) Translate() map[string]kongstate.Service { } if i.featureFlags.ExpressionRoutes { - route := meta.translateIntoKongExpressionRoute() + route, err := meta.translateIntoKongExpressionRoute() + if err != nil { + i.failuresCollector.PushResourceFailure(fmt.Sprintf("failed to translate to expression based route: %s", err), meta.parentIngress) + continue + } kongStateService.Routes = append(kongStateService.Routes, *route) } else { route := meta.translateIntoKongRoute() diff --git a/internal/dataplane/translator/subtranslator/ingress_atc.go b/internal/dataplane/translator/subtranslator/ingress_atc.go index cb2d2d8b90..832b89d7b0 100644 --- a/internal/dataplane/translator/subtranslator/ingress_atc.go +++ b/internal/dataplane/translator/subtranslator/ingress_atc.go @@ -1,6 +1,7 @@ package subtranslator import ( + "fmt" "regexp" "sort" "strings" @@ -28,7 +29,7 @@ var ( const IngressDefaultBackendPriority RoutePriorityType = 0 -func (m *ingressTranslationMeta) translateIntoKongExpressionRoute() *kongstate.Route { +func (m *ingressTranslationMeta) translateIntoKongExpressionRoute() (*kongstate.Route, error) { // '_' is not allowed in a host, so use '_' to replace a possible occurrence of '*' since '*' is not allowed in Kong. ingressHost := strings.ReplaceAll(m.ingressHost, "*", "_") routeName := m.backend.intoKongRouteName(k8stypes.NamespacedName{ @@ -77,9 +78,16 @@ func (m *ingressTranslationMeta) translateIntoKongExpressionRoute() *kongstate.R pathRegexPrefix = ControllerPathRegexPrefix } pathSegmentPrefix := annotations.ExtractSegmentPrefix(ingressAnnotations) + if pathRegexPrefix == pathSegmentPrefix { + return nil, fmt.Errorf("cannot use the same prefix %s for regex match and segment match", pathRegexPrefix) + } for _, path := range m.paths { - pathMatchers = append(pathMatchers, pathMatcherFromIngressPath(path, pathRegexPrefix, pathSegmentPrefix)) + pathMatcher, err := pathMatcherFromIngressPath(path, pathRegexPrefix, pathSegmentPrefix) + if err != nil { + return nil, err + } + pathMatchers = append(pathMatchers, pathMatcher) } routeMatcher.And(atc.Or(pathMatchers...)) @@ -106,34 +114,34 @@ func (m *ingressTranslationMeta) translateIntoKongExpressionRoute() *kongstate.R priority := calculateExpressionRoutePriority(m.paths, pathRegexPrefix, m.ingressHost, ingressAnnotations) atc.ApplyExpression(&route.Route, routeMatcher, priority) - return route + return route, nil } // pathMatcherFromIngressPath translate ingress path into matcher to match the path. -func pathMatcherFromIngressPath(httpIngressPath netv1.HTTPIngressPath, regexPathPrefix string, segmentPathPrefix string) atc.Matcher { +func pathMatcherFromIngressPath(httpIngressPath netv1.HTTPIngressPath, regexPathPrefix string, segmentPathPrefix string) (atc.Matcher, error) { switch *httpIngressPath.PathType { // Prefix paths. case netv1.PathTypePrefix: base := strings.Trim(httpIngressPath.Path, "/") if base == "" { // empty string in prefix path matches prefix "/". - return atc.NewPredicateHTTPPath(atc.OpPrefixMatch, "/") + return atc.NewPredicateHTTPPath(atc.OpPrefixMatch, "/"), nil } return atc.Or( // otherwise, match //* or /. atc.NewPredicateHTTPPath(atc.OpEqual, "/"+base), atc.NewPredicateHTTPPath(atc.OpPrefixMatch, "/"+base+"/"), - ) + ), nil // Exact paths. case netv1.PathTypeExact: relative := strings.TrimLeft(httpIngressPath.Path, "/") - return atc.NewPredicateHTTPPath(atc.OpEqual, "/"+relative) + return atc.NewPredicateHTTPPath(atc.OpEqual, "/"+relative), nil // Implementation Specific match. treat it as regex match if it begins with a regex prefix (/~ by default), // otherwise generate a prefix match. case netv1.PathTypeImplementationSpecific: // empty path. matches prefix "/" to match any path. if httpIngressPath.Path == "" { - return atc.NewPredicateHTTPPath(atc.OpPrefixMatch, "/") + return atc.NewPredicateHTTPPath(atc.OpPrefixMatch, "/"), nil } // regex match. if regexPathPrefix != "" && strings.HasPrefix(httpIngressPath.Path, regexPathPrefix) { @@ -145,20 +153,20 @@ func pathMatcherFromIngressPath(httpIngressPath netv1.HTTPIngressPath, regexPath if !strings.HasPrefix(regex, "^") { regex = "^" + regex } - return atc.NewPredicateHTTPPath(atc.OpRegexMatch, regex) + return atc.NewPredicateHTTPPath(atc.OpRegexMatch, regex), nil } // segment match path. if segmentPathPrefix != "" && strings.HasPrefix(httpIngressPath.Path, segmentPathPrefix) { segmentMatchingPath := strings.TrimPrefix(httpIngressPath.Path, segmentPathPrefix) return pathSegmentMatcherFromIngressMatch(segmentMatchingPath) } - return atc.NewPredicateHTTPPath(atc.OpPrefixMatch, httpIngressPath.Path) + return atc.NewPredicateHTTPPath(atc.OpPrefixMatch, httpIngressPath.Path), nil } - - return nil + // Should not reach here. + return nil, nil } -func pathSegmentMatcherFromIngressMatch(path string) atc.Matcher { +func pathSegmentMatcherFromIngressMatch(path string) (atc.Matcher, error) { path = strings.Trim(path, "/") segments := strings.Split(path, "/") predicates := make([]atc.Matcher, 0) @@ -173,14 +181,21 @@ func pathSegmentMatcherFromIngressMatch(path string) atc.Matcher { predicates = append(predicates, segmentLenPredicate) for index, segment := range segments { - if segment == "*" || segment == "**" { + 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...) + return atc.And(predicates...), nil } // headerMatcherFromHeaders generates matcher to match headers in HTTP requests. diff --git a/internal/dataplane/translator/subtranslator/ingress_atc_test.go b/internal/dataplane/translator/subtranslator/ingress_atc_test.go index 2dfff0576e..5a15bb32a1 100644 --- a/internal/dataplane/translator/subtranslator/ingress_atc_test.go +++ b/internal/dataplane/translator/subtranslator/ingress_atc_test.go @@ -623,6 +623,8 @@ func TestPathMatcherFromIngressPath(t *testing.T) { regexPrefix string segmentPrefix string expression string + expectError bool + errorContains string }{ { name: "simple prefix match", @@ -725,6 +727,16 @@ func TestPathMatcherFromIngressPath(t *testing.T) { segmentPrefix: "/!", expression: `http.path.segments.len >= 0`, }, + { + name: "segment match with ** in the middle segments should be invalid", + path: netv1.HTTPIngressPath{ + Path: "/!/a/**/b", + PathType: &pathTypeImplementationSpecific, + }, + segmentPrefix: "/!", + expectError: true, + errorContains: "'**' can only appear on the last segment", + }, { name: "empty implementation specific (non-regex) match", path: netv1.HTTPIngressPath{ @@ -742,8 +754,14 @@ func TestPathMatcherFromIngressPath(t *testing.T) { if regexPrefix == "" { regexPrefix = ControllerPathRegexPrefix } - matcher := pathMatcherFromIngressPath(tc.path, regexPrefix, tc.segmentPrefix) - require.Equal(t, tc.expression, matcher.Expression()) + matcher, err := pathMatcherFromIngressPath(tc.path, regexPrefix, tc.segmentPrefix) + if !tc.expectError { + require.NoError(t, err) + require.Equal(t, tc.expression, matcher.Expression()) + } else { + require.Error(t, err) + require.Contains(t, err.Error(), tc.errorContains) + } }) } } diff --git a/internal/versions/versions.go b/internal/versions/versions.go index 3c190d1693..03fca4fcd3 100644 --- a/internal/versions/versions.go +++ b/internal/versions/versions.go @@ -9,3 +9,9 @@ var KICv3VersionCutoff = semver.Version{Major: 3, Minor: 4, Patch: 1} // DeckFileFormatVersion is the version of the decK file format used by KIC everywhere. const DeckFileFormatVersion = "3.0" + +var ( + HTTPPathSegmentMatchVersionCutoff = semver.Version{Major: 3, Minor: 6, Patch: 0} + HTTPPathSegmentMatchMinPatchVersion3_5 = semver.Version{Major: 3, Minor: 5, Patch: 1} + HTTPPathSegmentMatchMinPatchVersion3_4 = semver.Version{Major: 3, Minor: 4, Patch: 3} +) From e3c4e97fee6cb7367a3e69209d2ad1a747f44c6e Mon Sep 17 00:00:00 2001 From: Yi Tao Date: Fri, 29 Mar 2024 15:09:31 +0800 Subject: [PATCH 3/3] implement segment match for httproute and add integration tests --- .../translator/subtranslator/atc_utils.go | 36 ++++++++ .../translator/subtranslator/httproute_atc.go | 44 ++++++---- .../subtranslator/httproute_atc_test.go | 4 +- .../translator/subtranslator/ingress_atc.go | 34 +------- test/integration/httproute_test.go | 84 +++++++++++++++++++ test/integration/ingress_test.go | 50 +++++++++++ 6 files changed, 203 insertions(+), 49 deletions(-) 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..2cd894ceee 100644 --- a/test/integration/httproute_test.go +++ b/test/integration/httproute_test.go @@ -675,3 +675,87 @@ 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) { + ctx := context.Background() + RunWhenKongVersion(t, ">=3.6.0", "Path segment match is only supported in Kong 3.6.0+ with expression router") + RunWhenKongExpressionRouter(ctx, t) + + 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..2200352e3d 100644 --- a/test/integration/ingress_test.go +++ b/test/integration/ingress_test.go @@ -1204,6 +1204,56 @@ func TestIngressRewriteURI(t *testing.T) { require.NoError(t, <-backgroundTestError, "for Ingress without rewrite run in background") } +func TestIngressPathSegmentMatch(t *testing.T) { + ctx := context.Background() + RunWhenKongVersion(t, ">=3.6.0", "Path segment match is only supported in Kong 3.6.0+ with expression router") + RunWhenKongExpressionRouter(ctx, t) + + 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) + // Change pathTypes of paths to `ImplementationSpecific` to specify the paths to segment matches. + 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 {