From 1022266b2c566f3d37f57276b1356e601c0cfe10 Mon Sep 17 00:00:00 2001 From: Tao Yi Date: Wed, 18 Oct 2023 15:54:46 +0800 Subject: [PATCH] feat(expression router): implement query parameter match of HTTPRoute (#4780) * feat: implement query parameter match of HTTPRoute * update CHANGELOG * update helm chart version * update httproute validation message * remove explanations about limit of Kong 3.4.1 and above * remove version check in validation * remove version cutoff * update tests * update comments * add Query param match to experimental conformance tests --- CHANGELOG.md | 2 ++ .../admission/validation/gateway/httproute.go | 11 ++++--- .../validation/gateway/httproute_test.go | 4 +-- internal/dataplane/parser/atc/field.go | 13 ++++++++ internal/dataplane/parser/atc/predicate.go | 10 ++++++ .../dataplane/parser/translate_httproute.go | 6 ++-- .../parser/translators/httproute_atc.go | 32 +++++++++++++++++++ .../parser/translators/httproute_atc_test.go | 8 +++++ internal/gatewayapi/aliases.go | 2 ++ internal/util/builder/httproutematch.go | 9 ++++++ test/conformance/gateway_conformance_test.go | 4 +-- .../gateway_expermimental_conformance_test.go | 1 + test/integration/httproute_test.go | 23 +++++++++++++ 13 files changed, 114 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a969bd1731..c35e6424d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -159,6 +159,8 @@ Adding a new version? You'll need three changes: - `--publish-service-udp` to `--ingress-service-udp` - `--publish-status-address-udp` to `--ingress-address-udp` [#4765](https://github.com/Kong/kubernetes-ingress-controller/pull/4765) +- Support Query Parameter matching of `HTTPRoute` when expression router enabled. + [#4780](https://github.com/Kong/kubernetes-ingress-controller/pull/4780) [KIC Annotations reference]: https://docs.konghq.com/kubernetes-ingress-controller/latest/references/annotations/ diff --git a/internal/admission/validation/gateway/httproute.go b/internal/admission/validation/gateway/httproute.go index 7ebb5237b9..3fa4627a57 100644 --- a/internal/admission/validation/gateway/httproute.go +++ b/internal/admission/validation/gateway/httproute.go @@ -33,7 +33,7 @@ func ValidateHTTPRoute( attachedGateways ...*gatewayapi.Gateway, ) (bool, string, error) { // validate that no unsupported features are in use - if err := validateHTTPRouteFeatures(httproute); err != nil { + if err := validateHTTPRouteFeatures(httproute, parserFeatures); err != nil { return false, "httproute spec did not pass validation", err } @@ -95,13 +95,14 @@ func validateHTTPRouteListener(listener *gatewayapi.Listener) error { // validateHTTPRouteFeatures checks for features that are not supported by this // HTTPRoute implementation and validates that the provided object is not using // any of those unsupported features. -func validateHTTPRouteFeatures(httproute *gatewayapi.HTTPRoute) error { +func validateHTTPRouteFeatures(httproute *gatewayapi.HTTPRoute, parserFeatures parser.FeatureFlags) error { for _, rule := range httproute.Spec.Rules { for _, match := range rule.Matches { - // We don't support query parameters matching rules yet - // TODO: https://github.com/Kong/kubernetes-ingress-controller/issues/3679 + // We support query parameters matching rules only with expression router. if len(match.QueryParams) != 0 { - return fmt.Errorf("queryparam matching is not yet supported for httproute") + if !parserFeatures.ExpressionRoutes { + return fmt.Errorf("queryparam matching is supported with expression router only") + } } } // We don't support any backendRef types except Kubernetes Services. diff --git a/internal/admission/validation/gateway/httproute_test.go b/internal/admission/validation/gateway/httproute_test.go index f69e7587ca..b2c0d43a05 100644 --- a/internal/admission/validation/gateway/httproute_test.go +++ b/internal/admission/validation/gateway/httproute_test.go @@ -204,7 +204,7 @@ func TestValidateHTTPRoute(t *testing.T) { err: fmt.Errorf("HTTPRoute not supported by listener http-alternate"), }, { - msg: "if an HTTPRoute is using queryparams matching it fails validation due to lack of support", + msg: "if an HTTPRoute is using queryparams matching it fails validation due to only supporting expression router", route: &gatewayapi.HTTPRoute{ ObjectMeta: metav1.ObjectMeta{ Namespace: corev1.NamespaceDefault, @@ -254,7 +254,7 @@ func TestValidateHTTPRoute(t *testing.T) { }}, valid: false, validationMsg: "httproute spec did not pass validation", - err: fmt.Errorf("queryparam matching is not yet supported for httproute"), + err: fmt.Errorf("queryparam matching is supported with expression router only"), }, { msg: "we don't support any group except core kubernetes for backendRefs", diff --git a/internal/dataplane/parser/atc/field.go b/internal/dataplane/parser/atc/field.go index afe1008cd8..e48f0abb33 100644 --- a/internal/dataplane/parser/atc/field.go +++ b/internal/dataplane/parser/atc/field.go @@ -80,3 +80,16 @@ func (f HTTPHeaderField) FieldType() FieldType { func (f HTTPHeaderField) String() string { return "http.headers." + strings.ToLower(strings.ReplaceAll(f.HeaderName, "-", "_")) } + +// HTTPQueryField extracts the value of an HTTP query parameter from the query string of the request. +type HTTPQueryField struct { + QueryParamName string +} + +func (f HTTPQueryField) FieldType() FieldType { + return FieldTypeString +} + +func (f HTTPQueryField) String() string { + return "http.queries." + f.QueryParamName +} diff --git a/internal/dataplane/parser/atc/predicate.go b/internal/dataplane/parser/atc/predicate.go index 65d4d6bb2f..797417ac3e 100644 --- a/internal/dataplane/parser/atc/predicate.go +++ b/internal/dataplane/parser/atc/predicate.go @@ -197,3 +197,13 @@ func NewPredicateTLSSNI(op BinaryOperator, value string) Predicate { value: StringLiteral(value), } } + +func NewPredicateHTTPQuery(key string, op BinaryOperator, value string) Predicate { + return Predicate{ + field: HTTPQueryField{ + QueryParamName: key, + }, + op: op, + value: StringLiteral(value), + } +} diff --git a/internal/dataplane/parser/translate_httproute.go b/internal/dataplane/parser/translate_httproute.go index 351f8d1ef1..ea08e64b1f 100644 --- a/internal/dataplane/parser/translate_httproute.go +++ b/internal/dataplane/parser/translate_httproute.go @@ -202,7 +202,7 @@ func GenerateKongRouteFromTranslation( ) } -// generateKongRoutesFromHTTPRouteMatches converts an HTTPRouteMatches to a slice of Kong Route objects. +// generateKongRoutesFromHTTPRouteMatches converts an HTTPRouteMatches to a slice of Kong Route objects with traditional routes. // This function assumes that the HTTPRouteMatches share the query params, headers and methods. func generateKongRoutesFromHTTPRouteMatches( routeName string, @@ -232,7 +232,9 @@ func generateKongRoutesFromHTTPRouteMatches( return []kongstate.Route{r}, nil } - // TODO: implement query param matches (https://github.com/Kong/kubernetes-ingress-controller/issues/2778) + // Kong supports query parameter match only with expression router. + // This function is only called for Kong with traditional/traditional compatible router, + // so we do not support query parameter match here. if len(matches[0].QueryParams) > 0 { return []kongstate.Route{}, translators.ErrRouteValidationQueryParamMatchesUnsupported } diff --git a/internal/dataplane/parser/translators/httproute_atc.go b/internal/dataplane/parser/translators/httproute_atc.go index e55b1cda63..e02cbfb53b 100644 --- a/internal/dataplane/parser/translators/httproute_atc.go +++ b/internal/dataplane/parser/translators/httproute_atc.go @@ -134,6 +134,11 @@ func generateMatcherFromHTTPRouteMatch(match gatewayapi.HTTPRouteMatch) atc.Matc matcher.And(headerMatcher) } + if len(match.QueryParams) > 0 { + queryMatcher := queryParamMatcherFromHTTPQueryParamMatches(match.QueryParams) + matcher.And(queryMatcher) + } + if match.Method != nil { method := *match.Method methodMatcher := methodMatcherFromMethods([]string{string(method)}) @@ -206,6 +211,33 @@ func headerMatcherFromHTTPHeaderMatches(headerMatches []gatewayapi.HTTPHeaderMat return atc.And(matchers...) } +func queryParamMatcherFromHTTPQueryParamMatch(queryParamMatch gatewayapi.HTTPQueryParamMatch) atc.Matcher { + matchType := gatewayapi.QueryParamMatchExact + if queryParamMatch.Type != nil { + matchType = *queryParamMatch.Type + } + switch matchType { + case gatewayapi.QueryParamMatchExact: + return atc.NewPredicateHTTPQuery(string(queryParamMatch.Name), atc.OpEqual, queryParamMatch.Value) + case gatewayapi.QueryParamMatchRegularExpression: + return atc.NewPredicateHTTPQuery(string(queryParamMatch.Name), atc.OpRegexMatch, queryParamMatch.Value) + } + return nil // should be unreachable +} + +func queryParamMatcherFromHTTPQueryParamMatches(queryParamMatches []gatewayapi.HTTPQueryParamMatch) atc.Matcher { + // Sort queryParamMatches by names to generate a stable output. + sort.Slice(queryParamMatches, func(i, j int) bool { + return string(queryParamMatches[i].Name) < string(queryParamMatches[j].Name) + }) + + matchers := make([]atc.Matcher, 0, len(queryParamMatches)) + for _, queryParamMatch := range queryParamMatches { + matchers = append(matchers, queryParamMatcherFromHTTPQueryParamMatch(queryParamMatch)) + } + return atc.And(matchers...) +} + func matchersFromParentHTTPRoute(hostnames []string, metaAnnotations map[string]string) []atc.Matcher { // translate hostnames. ret := []atc.Matcher{} diff --git a/internal/dataplane/parser/translators/httproute_atc_test.go b/internal/dataplane/parser/translators/httproute_atc_test.go index 963a4776fd..b097c8aee7 100644 --- a/internal/dataplane/parser/translators/httproute_atc_test.go +++ b/internal/dataplane/parser/translators/httproute_atc_test.go @@ -318,6 +318,14 @@ func TestGenerateMatcherFromHTTPRouteMatch(t *testing.T) { Build(), expression: `((http.path == "/prefix/0") || (http.path ^= "/prefix/0/")) && ((http.headers.hash ~ "[0-9A-Fa-f]{32}") && (http.headers.x_foo == "Bar"))`, }, + { + name: "prefix path match and multiple query params", + match: builder.NewHTTPRouteMatch().WithPathPrefix("/prefix/0"). + WithQueryParam("foo", "bar"). + WithQueryParamRegex("id", "[0-9a-z-]+"). + Build(), + expression: `((http.path == "/prefix/0") || (http.path ^= "/prefix/0/")) && ((http.queries.foo == "bar") && (http.queries.id ~ "[0-9a-z-]+"))`, + }, } for _, tc := range testCases { diff --git a/internal/gatewayapi/aliases.go b/internal/gatewayapi/aliases.go index fda3ce9efe..de058f2e77 100644 --- a/internal/gatewayapi/aliases.go +++ b/internal/gatewayapi/aliases.go @@ -140,6 +140,8 @@ const ( PathMatchExact = gatewayv1beta1.PathMatchExact PathMatchPathPrefix = gatewayv1beta1.PathMatchPathPrefix PathMatchRegularExpression = gatewayv1beta1.PathMatchRegularExpression + QueryParamMatchExact = gatewayv1beta1.QueryParamMatchExact + QueryParamMatchRegularExpression = gatewayv1beta1.QueryParamMatchRegularExpression RouteConditionAccepted = gatewayv1beta1.RouteConditionAccepted RouteConditionResolvedRefs = gatewayv1beta1.RouteConditionResolvedRefs RouteReasonAccepted = gatewayv1beta1.RouteReasonAccepted diff --git a/internal/util/builder/httproutematch.go b/internal/util/builder/httproutematch.go index 7b8ed96762..52b8f348cb 100644 --- a/internal/util/builder/httproutematch.go +++ b/internal/util/builder/httproutematch.go @@ -56,6 +56,15 @@ func (b *HTTPRouteMatchBuilder) WithQueryParam(name, value string) *HTTPRouteMat return b } +func (b *HTTPRouteMatchBuilder) WithQueryParamRegex(name, value string) *HTTPRouteMatchBuilder { + b.httpRouteMatch.QueryParams = append(b.httpRouteMatch.QueryParams, gatewayapi.HTTPQueryParamMatch{ + Type: lo.ToPtr(gatewayapi.QueryParamMatchRegularExpression), + Name: gatewayapi.HTTPHeaderName(name), + Value: value, + }) + return b +} + func (b *HTTPRouteMatchBuilder) WithMethod(method gatewayapi.HTTPMethod) *HTTPRouteMatchBuilder { b.httpRouteMatch.Method = &method return b diff --git a/test/conformance/gateway_conformance_test.go b/test/conformance/gateway_conformance_test.go index e4f82300ae..c874970e2f 100644 --- a/test/conformance/gateway_conformance_test.go +++ b/test/conformance/gateway_conformance_test.go @@ -25,8 +25,6 @@ var commonSkippedTests = []string{ // https://github.com/Kong/kubernetes-ingress-controller/issues/4563 tests.GatewayWithAttachedRoutesWithPort8080.ShortName, tests.HTTPRouteRedirectPortAndScheme.ShortName, - // https://github.com/Kong/kubernetes-ingress-controller/issues/3679 - tests.HTTPRouteQueryParamMatching.ShortName, // https://github.com/Kong/kubernetes-ingress-controller/issues/3681 tests.HTTPRouteRedirectPort.ShortName, // https://github.com/Kong/kubernetes-ingress-controller/issues/3682 @@ -62,6 +60,8 @@ var ( // cannot support the path > method > header precedence, // but no way to omit individual cases. tests.HTTPRouteMethodMatching.ShortName, + // only expression router supports query param matches + tests.HTTPRouteQueryParamMatching.ShortName, ) ) diff --git a/test/conformance/gateway_expermimental_conformance_test.go b/test/conformance/gateway_expermimental_conformance_test.go index 0929310858..f6d75a547a 100644 --- a/test/conformance/gateway_expermimental_conformance_test.go +++ b/test/conformance/gateway_expermimental_conformance_test.go @@ -43,6 +43,7 @@ func TestGatewayExperimentalConformance(t *testing.T) { SupportedFeatures: sets.New( suite.SupportHTTPRouteMethodMatching, suite.SupportHTTPResponseHeaderModification, + suite.SupportHTTPRouteQueryParamMatching, ), }, ConformanceProfiles: sets.New( diff --git a/test/integration/httproute_test.go b/test/integration/httproute_test.go index f2ff564b0e..2a59511cd2 100644 --- a/test/integration/httproute_test.go +++ b/test/integration/httproute_test.go @@ -13,6 +13,7 @@ import ( "github.com/kong/go-kong/kong" ktfkong "github.com/kong/kubernetes-testing-framework/pkg/clusters/addons/kong" "github.com/kong/kubernetes-testing-framework/pkg/utils/kubernetes/generators" + "github.com/samber/lo" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" @@ -196,6 +197,28 @@ func TestHTTPRouteEssentials(t *testing.T) { helpers.EventuallyGETPath(t, proxyURL, proxyURL.Host, "/", http.StatusOK, "httpbin.org", map[string]string{"Content-Type": "audio/mp3"}, ingressWait, waitTick) }) + t.Run("HTTPRoute query param match", func(t *testing.T) { + RunWhenKongExpressionRouter(t) //nolint:contextcheck + + httpRoute, err = gatewayClient.GatewayV1beta1().HTTPRoutes(ns.Name).Get(ctx, httpRoute.Name, metav1.GetOptions{}) + require.NoError(t, err) + + httpRoute.Spec.Rules[0].Matches = append(httpRoute.Spec.Rules[0].Matches, gatewayapi.HTTPRouteMatch{ + QueryParams: []gatewayapi.HTTPQueryParamMatch{ + { + Type: lo.ToPtr(gatewayapi.QueryParamMatchExact), + Name: "foo", + Value: "bar", + }, + }, + }) + httpRoute, err = gatewayClient.GatewayV1beta1().HTTPRoutes(ns.Name).Update(ctx, httpRoute, metav1.UpdateOptions{}) + require.NoError(t, err) + + t.Log("verifying HTTPRoute query param match") + helpers.EventuallyGETPath(t, proxyURL, proxyURL.Host, "/?foo=bar", http.StatusOK, "httpbin.org", nil, ingressWait, waitTick) + }) + t.Log("verifying that the HTTPRoute has the Condition 'Accepted' set to 'True'") require.Eventually(t, HTTPRouteMatchesAcceptedCallback(t, gatewayClient, httpRoute, true, gatewayapi.RouteReasonAccepted), statusWait, waitTick)