chore: separate validation and translation for HTTPRoute (#4886)

randmonkey · web-flow · commit 41ab6ed1dc7d · 2023-10-20T22:24:11.000+08:00
* separte validation and translation for HTTPRoute

* update tests
diff --git a/internal/dataplane/parser/translate_httproute.go b/internal/dataplane/parser/translate_httproute.go
@@ -30,12 +30,22 @@ func (p *Parser) ingressRulesFromHTTPRoutes() ingressRules {
 		return result
 	}
 
+	httpRoutesToTranslate := make([]*gatewayapi.HTTPRoute, 0, len(httpRouteList))
+	for _, httproute := range httpRouteList {
+		// Validate each HTTPRoute before translating and register translation failures if an HTTPRoute is invalid.
+		if err := validateHTTPRoute(httproute, p.featureFlags); err != nil {
+			p.registerTranslationFailure(fmt.Sprintf("HTTPRoute can't be routed: %v", err), httproute)
+			continue
+		}
+		httpRoutesToTranslate = append(httpRoutesToTranslate, httproute)
+	}
+
 	if p.featureFlags.ExpressionRoutes {
-		p.ingressRulesFromHTTPRoutesUsingExpressionRoutes(httpRouteList, &result)
+		p.ingressRulesFromHTTPRoutesUsingExpressionRoutes(httpRoutesToTranslate, &result)
 		return result
 	}
 
-	for _, httproute := range httpRouteList {
+	for _, httproute := range httpRoutesToTranslate {
 		if err := p.ingressRulesFromHTTPRoute(&result, httproute); err != nil {
 			p.registerTranslationFailure(fmt.Sprintf("HTTPRoute can't be routed: %s", err), httproute)
 		} else {
@@ -51,9 +61,6 @@ func (p *Parser) ingressRulesFromHTTPRoutes() ingressRules {
 // ingressRulesFromHTTPRoute validates and generates a set of proto-Kong routes (ingress rules) from an HTTPRoute.
 // If multiple rules in the HTTPRoute use the same Service, it combines them into a single Kong route.
 func (p *Parser) ingressRulesFromHTTPRoute(result *ingressRules, httproute *gatewayapi.HTTPRoute) error {
-	if err := validateHTTPRoute(httproute); err != nil {
-		return fmt.Errorf("validation failed : %w", err)
-	}
 	for _, kongServiceTranslation := range translators.TranslateHTTPRoute(httproute) {
 		// HTTPRoute uses a wrapper HTTPBackendRef to add optional filters to its BackendRefs
 		backendRefs := httpBackendRefsToBackendRefs(kongServiceTranslation.BackendRefs)
@@ -82,7 +89,7 @@ func (p *Parser) ingressRulesFromHTTPRoute(result *ingressRules, httproute *gate
 	return nil
 }
 
-func validateHTTPRoute(httproute *gatewayapi.HTTPRoute) error {
+func validateHTTPRoute(httproute *gatewayapi.HTTPRoute, featureFlags FeatureFlags) error {
 	spec := httproute.Spec
 
 	// validation for HTTPRoutes will happen at a higher layer, but in spite of that we run
@@ -93,6 +100,18 @@ func validateHTTPRoute(httproute *gatewayapi.HTTPRoute) error {
 		return translators.ErrRouteValidationNoRules
 	}
 
+	// Kong supports query parameter match only with expression router,
+	// so we return error when query param match is specified and expression router is not enabled in the parser.
+	if !featureFlags.ExpressionRoutes {
+		for _, rule := range spec.Rules {
+			for _, match := range rule.Matches {
+				if len(match.QueryParams) > 0 {
+					return translators.ErrRouteValidationQueryParamMatchesUnsupported
+				}
+			}
+		}
+	}
+
 	return nil
 }
 
@@ -105,10 +124,6 @@ func (p *Parser) ingressRulesFromHTTPRoutesUsingExpressionRoutes(httpRoutes []*g
 	// first, split HTTPRoutes by hostnames and matches.
 	splitHTTPRouteMatches := []translators.SplitHTTPRouteMatch{}
 	for _, httproute := range httpRoutes {
-		if err := validateHTTPRoute(httproute); err != nil {
-			p.registerTranslationFailure(fmt.Sprintf("HTTPRoute can't be routed: %s", err), httproute)
-			continue
-		}
 		splitHTTPRouteMatches = append(splitHTTPRouteMatches, translators.SplitHTTPRoute(httproute)...)
 	}
 	// assign priorities to split HTTPRoutes.
@@ -232,13 +247,6 @@ func generateKongRoutesFromHTTPRouteMatches(
 		return []kongstate.Route{r}, nil
 	}
 
-	// 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
-	}
-
 	r := generateKongstateHTTPRoute(routeName, ingressObjectInfo, hostnames)
 	r.Tags = tags
 
diff --git a/internal/dataplane/parser/translate_httproute_test.go b/internal/dataplane/parser/translate_httproute_test.go
@@ -1,7 +1,6 @@
 package parser
 
 import (
-	"strings"
 	"testing"
 
 	"github.com/go-logr/zapr"
@@ -41,6 +40,120 @@ type testCaseIngressRulesFromHTTPRoutes struct {
 	errs     []error
 }
 
+func TestValidateHTTPRoute(t *testing.T) {
+	testCases := []struct {
+		name             string
+		httpRoute        *gatewayapi.HTTPRoute
+		expressionRoutes bool
+		expectedError    error
+	}{
+		{
+			name: "valid HTTPRoute should pass the validation",
+			httpRoute: &gatewayapi.HTTPRoute{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "basic-httproute",
+					Namespace: corev1.NamespaceDefault,
+				},
+				Spec: gatewayapi.HTTPRouteSpec{
+					CommonRouteSpec: commonRouteSpecMock("fake-gateway-1"),
+					Hostnames: []gatewayapi.Hostname{
+						"konghq.com",
+						"www.konghq.com",
+					},
+					Rules: []gatewayapi.HTTPRouteRule{{
+						BackendRefs: []gatewayapi.HTTPBackendRef{
+							builder.NewHTTPBackendRef("fake-service").WithPort(80).Build(),
+						},
+					}},
+				},
+			},
+			expressionRoutes: false,
+			expectedError:    nil,
+		},
+		{
+			name: "HTTPRoute with no rules should not pass the validation",
+			httpRoute: &gatewayapi.HTTPRoute{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "httproute-no-rule",
+					Namespace: corev1.NamespaceDefault,
+				},
+				Spec: gatewayapi.HTTPRouteSpec{
+					CommonRouteSpec: commonRouteSpecMock("fake-gateway-1"),
+					Hostnames: []gatewayapi.Hostname{
+						"konghq.com",
+						"www.konghq.com",
+					},
+				},
+			},
+			expressionRoutes: false,
+			expectedError:    translators.ErrRouteValidationNoRules,
+		},
+		{
+			name: "HTTPRoute with query param match should pass validation with expression routes",
+			httpRoute: &gatewayapi.HTTPRoute{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "httproute-query-param-match",
+					Namespace: corev1.NamespaceDefault,
+				},
+				Spec: gatewayapi.HTTPRouteSpec{
+					CommonRouteSpec: commonRouteSpecMock("fake-gateway-1"),
+					Hostnames: []gatewayapi.Hostname{
+						"konghq.com",
+						"www.konghq.com",
+					},
+					Rules: []gatewayapi.HTTPRouteRule{{
+						BackendRefs: []gatewayapi.HTTPBackendRef{
+							builder.NewHTTPBackendRef("fake-service").WithPort(80).Build(),
+						},
+						Matches: builder.NewHTTPRouteMatch().WithQueryParam("foo", "bar").ToSlice(),
+					}},
+				},
+			},
+			expressionRoutes: true,
+			expectedError:    nil,
+		},
+		{
+			name: "HTTPRoute with query param match should not pass validation when expression routes disabled",
+			httpRoute: &gatewayapi.HTTPRoute{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "httproute-query-param-match",
+					Namespace: corev1.NamespaceDefault,
+				},
+				Spec: gatewayapi.HTTPRouteSpec{
+					CommonRouteSpec: commonRouteSpecMock("fake-gateway-1"),
+					Hostnames: []gatewayapi.Hostname{
+						"konghq.com",
+						"www.konghq.com",
+					},
+					Rules: []gatewayapi.HTTPRouteRule{{
+						BackendRefs: []gatewayapi.HTTPBackendRef{
+							builder.NewHTTPBackendRef("fake-service").WithPort(80).Build(),
+						},
+						Matches: builder.NewHTTPRouteMatch().WithQueryParam("foo", "bar").ToSlice(),
+					}},
+				},
+			},
+			expressionRoutes: false,
+			expectedError:    translators.ErrRouteValidationQueryParamMatchesUnsupported,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			featureFlags := FeatureFlags{
+				ExpressionRoutes: tc.expressionRoutes,
+			}
+			err := validateHTTPRoute(tc.httpRoute, featureFlags)
+			if tc.expectedError == nil {
+				require.NoError(t, err, "should pass the validation")
+			} else {
+				require.ErrorIs(t, err, tc.expectedError, "should return expected error")
+			}
+		})
+	}
+}
+
 func TestIngressRulesFromHTTPRoutes(t *testing.T) {
 	fakestore, err := store.NewFakeStore(store.FakeObjects{})
 	require.NoError(t, err)
@@ -257,58 +370,6 @@ func TestIngressRulesFromHTTPRoutes(t *testing.T) {
 				}
 			},
 		},
-		{
-			msg: "an HTTPRoute with no rules can't be routed",
-			routes: []*gatewayapi.HTTPRoute{{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "basic-httproute",
-					Namespace: corev1.NamespaceDefault,
-				},
-				Spec: gatewayapi.HTTPRouteSpec{
-					CommonRouteSpec: commonRouteSpecMock("fake-gateway"),
-				},
-			}},
-			expected: func(routes []*gatewayapi.HTTPRoute) ingressRules {
-				return ingressRules{
-					SecretNameToSNIs:      newSecretNameToSNIs(),
-					ServiceNameToParent:   map[string]client.Object{},
-					ServiceNameToServices: make(map[string]kongstate.Service),
-				}
-			},
-			errs: []error{
-				translators.ErrRouteValidationNoRules,
-			},
-		},
-		{
-			msg: "an HTTPRoute with queryParam matches is not yet supported",
-			routes: []*gatewayapi.HTTPRoute{{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "basic-httproute",
-					Namespace: corev1.NamespaceDefault,
-				},
-				Spec: gatewayapi.HTTPRouteSpec{
-					CommonRouteSpec: commonRouteSpecMock("fake-gateway"),
-					Rules: []gatewayapi.HTTPRouteRule{{
-						Matches: []gatewayapi.HTTPRouteMatch{
-							builder.NewHTTPRouteMatch().WithQueryParam("username", "kong").Build(),
-						},
-						BackendRefs: []gatewayapi.HTTPBackendRef{
-							builder.NewHTTPBackendRef("fake-service").WithPort(80).Build(),
-						},
-					}},
-				},
-			}},
-			expected: func(routes []*gatewayapi.HTTPRoute) ingressRules {
-				return ingressRules{
-					SecretNameToSNIs:      newSecretNameToSNIs(),
-					ServiceNameToParent:   map[string]client.Object{},
-					ServiceNameToServices: make(map[string]kongstate.Service),
-				}
-			},
-			errs: []error{
-				translators.ErrRouteValidationQueryParamMatchesUnsupported,
-			},
-		},
 		{
 			msg: "an HTTPRoute with regex path matches is supported",
 			routes: []*gatewayapi.HTTPRoute{{
@@ -1487,17 +1548,11 @@ func TestIngressRulesFromHTTPRoutesUsingExpressionRoutes(t *testing.T) {
 	parser.featureFlags.ExpressionRoutes = true
 	httpRouteTypeMeta := metav1.TypeMeta{Kind: "HTTPRoute", APIVersion: gatewayv1beta1.GroupVersion.String()}
 
-	newResourceFailure := func(reason string, objects ...client.Object) failures.ResourceFailure {
-		failure, _ := failures.NewResourceFailure(reason, objects...)
-		return failure
-	}
-
 	testCases := []struct {
 		name                 string
 		httpRoutes           []*gatewayapi.HTTPRoute
 		expectedKongServices []kongstate.Service
 		expectedKongRoutes   map[string][]kongstate.Route
-		expectedFailures     []failures.ResourceFailure
 	}{
 		{
 			name: "single HTTPRoute with no hostname and multiple matches",
@@ -1744,78 +1799,6 @@ func TestIngressRulesFromHTTPRoutesUsingExpressionRoutes(t *testing.T) {
 				},
 			},
 		},
-		{
-			name: "multiple HTTPRoutes with translation failures",
-			httpRoutes: []*gatewayapi.HTTPRoute{
-				{
-					TypeMeta: httpRouteTypeMeta,
-					ObjectMeta: metav1.ObjectMeta{
-						Namespace: "default",
-						Name:      "httproute-no-host-no-rule",
-					},
-					Spec: gatewayapi.HTTPRouteSpec{
-						Hostnames: []gatewayapi.Hostname{"no-rule.example"},
-					},
-				},
-				{
-					TypeMeta: httpRouteTypeMeta,
-					ObjectMeta: metav1.ObjectMeta{
-						Namespace: "default",
-						Name:      "httproute-1",
-					},
-					Spec: gatewayapi.HTTPRouteSpec{
-						Hostnames: []gatewayapi.Hostname{
-							"foo.com",
-						},
-						Rules: []gatewayapi.HTTPRouteRule{
-							{
-								Matches: []gatewayapi.HTTPRouteMatch{
-									builder.NewHTTPRouteMatch().WithPathExact("/v1/foo").Build(),
-								},
-								BackendRefs: []gatewayapi.HTTPBackendRef{
-									builder.NewHTTPBackendRef("service1").WithPort(80).Build(),
-								},
-							},
-						},
-					},
-				},
-			},
-			expectedKongServices: []kongstate.Service{
-				{
-					Service: kong.Service{
-						Name: kong.String("httproute.default.httproute-1.foo.com.0"),
-					},
-					Backends: []kongstate.ServiceBackend{
-						{
-							Name:    "service1",
-							PortDef: kongstate.PortDef{Mode: kongstate.PortModeByNumber, Number: int32(80)},
-						},
-					},
-				},
-			},
-			expectedKongRoutes: map[string][]kongstate.Route{
-				"httproute.default.httproute-1.foo.com.0": {
-					{
-						Route: kong.Route{
-							Name:         kong.String("httproute.default.httproute-1.foo.com.0.0"),
-							Expression:   kong.String(`(http.host == "foo.com") && (http.path == "/v1/foo")`),
-							PreserveHost: kong.Bool(true),
-						},
-						Plugins:          []kong.Plugin{},
-						ExpressionRoutes: true,
-					},
-				},
-			},
-			expectedFailures: []failures.ResourceFailure{
-				newResourceFailure(translators.ErrRouteValidationNoRules.Error(), &gatewayapi.HTTPRoute{
-					TypeMeta: httpRouteTypeMeta,
-					ObjectMeta: metav1.ObjectMeta{
-						Namespace: "default",
-						Name:      "httproute-no-host-no-rule",
-					},
-				}),
-			},
-		},
 	}
 
 	for _, tc := range testCases {
@@ -1847,15 +1830,6 @@ func TestIngressRulesFromHTTPRoutesUsingExpressionRoutes(t *testing.T) {
 					require.Equal(t, *expectedRoute.Expression, *r.Expression)
 				}
 			}
-			// check translation failures
-			translationFailures := failureCollector.PopResourceFailures()
-			require.Equal(t, len(tc.expectedFailures), len(translationFailures))
-			for _, expectedTranslationFailure := range tc.expectedFailures {
-				expectedFailureMessage := expectedTranslationFailure.Message()
-				require.True(t, lo.ContainsBy(translationFailures, func(failure failures.ResourceFailure) bool {
-					return strings.Contains(failure.Message(), expectedFailureMessage)
-				}))
-			}
 		})
 	}
 }
