From 5f4ec240af1122f883fda71be8dd4839302acd32 Mon Sep 17 00:00:00 2001 From: Mattia Lavacca Date: Thu, 12 Oct 2023 18:49:14 +0200 Subject: [PATCH 1/4] feat: HTTPRoute extensionRef filter converted onto plugin Signed-off-by: Mattia Lavacca --- .../dataplane/parser/translate_httproute.go | 6 +-- .../dataplane/parser/translators/httproute.go | 46 ++++++++++++++++--- .../parser/translators/httproute_atc.go | 10 ++-- .../parser/translators/httproute_test.go | 35 ++++++++++++-- internal/gatewayapi/aliases.go | 1 + 5 files changed, 76 insertions(+), 22 deletions(-) diff --git a/internal/dataplane/parser/translate_httproute.go b/internal/dataplane/parser/translate_httproute.go index 23190ee5c2..25d4a3aaf4 100644 --- a/internal/dataplane/parser/translate_httproute.go +++ b/internal/dataplane/parser/translate_httproute.go @@ -270,8 +270,7 @@ func generateKongRoutesFromHTTPRouteMatches( // if the redirect filter has not been set, we still need to set the route plugins if !hasRedirectFilter { - plugins := translators.GeneratePluginsFromHTTPRouteFilters(filters, "", tags) - r.Plugins = append(r.Plugins, plugins...) + translators.ConvertFiltersToPlugins(&r, filters, "", tags) routes = []kongstate.Route{r} } @@ -320,8 +319,7 @@ func getRoutesFromMatches( } // generate kong plugins from rule.filters - plugins := translators.GeneratePluginsFromHTTPRouteFilters(filters, path, tags) - matchRoute.Plugins = append(matchRoute.Plugins, plugins...) + translators.ConvertFiltersToPlugins(matchRoute, filters, path, tags) routes = append(routes, *route) } else { diff --git a/internal/dataplane/parser/translators/httproute.go b/internal/dataplane/parser/translators/httproute.go index cd7a78ff9c..9f131e2a81 100644 --- a/internal/dataplane/parser/translators/httproute.go +++ b/internal/dataplane/parser/translators/httproute.go @@ -9,6 +9,8 @@ import ( "github.com/kong/go-kong/kong" + "github.com/kong/kubernetes-ingress-controller/v2/internal/annotations" + "github.com/kong/kubernetes-ingress-controller/v2/internal/dataplane/kongstate" "github.com/kong/kubernetes-ingress-controller/v2/internal/gatewayapi" ) @@ -350,14 +352,35 @@ func mustMarshalJSON[T any](val T) string { return string(key) } -// GeneratePluginsFromHTTPRouteFilters converts HTTPRouteFilter into Kong plugins. +func ConvertFiltersToPlugins(route *kongstate.Route, filters []gatewayapi.HTTPRouteFilter, path string, tags []*string) { + plugins, pluginAnnotation := generatePluginsFromHTTPRouteFilters(filters, path, tags) + if route.Plugins == nil { + route.Plugins = make([]kong.Plugin, 0) + } + route.Plugins = append(route.Plugins, plugins...) + if len(pluginAnnotation) > 0 { + if route.Ingress.Annotations == nil { + route.Ingress.Annotations = make(map[string]string) + } + if _, ok := route.Ingress.Annotations[annotations.AnnotationPrefix+annotations.PluginsKey]; !ok { + route.Ingress.Annotations[annotations.AnnotationPrefix+annotations.PluginsKey] = pluginAnnotation + } else { + route.Ingress.Annotations[annotations.AnnotationPrefix+annotations.PluginsKey] = fmt.Sprintf("%s,%s", + route.Ingress.Annotations[annotations.AnnotationPrefix+annotations.PluginsKey], + pluginAnnotation) + } + } +} + +// generatePluginsFromHTTPRouteFilters converts HTTPRouteFilter into Kong plugins. // path is the parameter to be used by the redirect plugin, to perform redirection. -func GeneratePluginsFromHTTPRouteFilters(filters []gatewayapi.HTTPRouteFilter, path string, tags []*string) []kong.Plugin { +func generatePluginsFromHTTPRouteFilters(filters []gatewayapi.HTTPRouteFilter, path string, tags []*string) ([]kong.Plugin, string) { kongPlugins := make([]kong.Plugin, 0) if len(filters) == 0 { - return kongPlugins + return kongPlugins, "" } + var pluginsAnnotation string for _, filter := range filters { switch filter.Type { case gatewayapi.HTTPRouteFilterRequestHeaderModifier: @@ -369,8 +392,15 @@ func GeneratePluginsFromHTTPRouteFilters(filters []gatewayapi.HTTPRouteFilter, p case gatewayapi.HTTPRouteFilterResponseHeaderModifier: kongPlugins = append(kongPlugins, generateResponseHeaderModifierKongPlugin(filter.ResponseHeaderModifier)) - case gatewayapi.HTTPRouteFilterExtensionRef, - gatewayapi.HTTPRouteFilterRequestMirror, + case gatewayapi.HTTPRouteFilterExtensionRef: + plugin := generateExtensionRefKongPlugin(filter.ExtensionRef) + if len(pluginsAnnotation) > 0 { + pluginsAnnotation = fmt.Sprintf("%s,%s", pluginsAnnotation, plugin) + } else { + pluginsAnnotation = plugin + } + + case gatewayapi.HTTPRouteFilterRequestMirror, gatewayapi.HTTPRouteFilterURLRewrite: // not supported } @@ -381,7 +411,7 @@ func GeneratePluginsFromHTTPRouteFilters(filters []gatewayapi.HTTPRouteFilter, p p.Tags = tags } - return kongPlugins + return kongPlugins, pluginsAnnotation } // generateRequestRedirectKongPlugin generates configurations of plugins to satisfy the specification @@ -429,6 +459,10 @@ func generateRequestRedirectKongPlugin(modifier *gatewayapi.HTTPRequestRedirectF return plugins } +func generateExtensionRefKongPlugin(modifier *gatewayapi.LocalObjectReference) string { + return string(modifier.Name) +} + // generateRequestHeaderModifierKongPlugin converts a gatewayapi.HTTPRequestHeaderFilter into a // kong.Plugin of type request-transformer. func generateRequestHeaderModifierKongPlugin(modifier *gatewayapi.HTTPHeaderFilter) kong.Plugin { diff --git a/internal/dataplane/parser/translators/httproute_atc.go b/internal/dataplane/parser/translators/httproute_atc.go index aba0fcbb7a..0bef5cf02d 100644 --- a/internal/dataplane/parser/translators/httproute_atc.go +++ b/internal/dataplane/parser/translators/httproute_atc.go @@ -66,8 +66,7 @@ func GenerateKongExpressionRoutesFromHTTPRouteMatches( atc.ApplyExpression(&r.Route, routeMatcher, 1) // generate plugins. - plugins := GeneratePluginsFromHTTPRouteFilters(translation.Filters, "", tags) - r.Plugins = plugins + ConvertFiltersToPlugins(&r, translation.Filters, "", tags) return []kongstate.Route{r}, nil } @@ -104,9 +103,7 @@ func generateKongExpressionRoutesWithRequestRedirectFilter( if match.Path != nil && match.Path.Value != nil { path = *match.Path.Value } - plugins := GeneratePluginsFromHTTPRouteFilters(translation.Filters, path, tags) - matchRoute.Plugins = plugins - + ConvertFiltersToPlugins(&matchRoute, translation.Filters, path, tags) routes = append(routes, matchRoute) } return routes, nil @@ -593,8 +590,7 @@ func KongExpressionRouteFromHTTPRouteMatchWithPriority( path = *match.Match.Path.Value } - plugins := GeneratePluginsFromHTTPRouteFilters(rule.Filters, path, tags) - r.Plugins = plugins + ConvertFiltersToPlugins(&r, rule.Filters, path, tags) } return r diff --git a/internal/dataplane/parser/translators/httproute_test.go b/internal/dataplane/parser/translators/httproute_test.go index 0451222818..e1115af9b7 100644 --- a/internal/dataplane/parser/translators/httproute_test.go +++ b/internal/dataplane/parser/translators/httproute_test.go @@ -12,10 +12,11 @@ import ( func TestGeneratePluginsFromHTTPRouteFilters(t *testing.T) { testCases := []struct { - name string - filters []gatewayapi.HTTPRouteFilter - path string - expectedPlugins []kong.Plugin + name string + filters []gatewayapi.HTTPRouteFilter + path string + expectedPlugins []kong.Plugin + expectedPluginsAnnotation string }{ { name: "no filters", @@ -153,13 +154,37 @@ func TestGeneratePluginsFromHTTPRouteFilters(t *testing.T) { }, }, }, + { + name: "extension-refs", + filters: []gatewayapi.HTTPRouteFilter{ + { + Type: gatewayapi.HTTPRouteFilterExtensionRef, + ExtensionRef: &gatewayapi.LocalObjectReference{ + Group: gatewayapi.Group("configuration.konghq.com/v1"), + Kind: gatewayapi.Kind("KongPlugin"), + Name: "plugin1", + }, + }, + { + Type: gatewayapi.HTTPRouteFilterExtensionRef, + ExtensionRef: &gatewayapi.LocalObjectReference{ + Group: gatewayapi.Group("configuration.konghq.com/v1"), + Kind: gatewayapi.Kind("KongPlugin"), + Name: "plugin2", + }, + }, + }, + expectedPluginsAnnotation: "plugin1,plugin2", + expectedPlugins: []kong.Plugin{}, + }, } for _, tc := range testCases { tc := tc - plugins := GeneratePluginsFromHTTPRouteFilters(tc.filters, tc.path, nil) t.Run(tc.name, func(t *testing.T) { + plugins, pluginsAnnotation := generatePluginsFromHTTPRouteFilters(tc.filters, tc.path, nil) require.Equal(t, tc.expectedPlugins, plugins) + require.Equal(t, tc.expectedPluginsAnnotation, pluginsAnnotation) }) } } diff --git a/internal/gatewayapi/aliases.go b/internal/gatewayapi/aliases.go index 5213293947..3dfb39a5ae 100644 --- a/internal/gatewayapi/aliases.go +++ b/internal/gatewayapi/aliases.go @@ -40,6 +40,7 @@ type ( HTTPRouteList = gatewayv1.HTTPRouteList HTTPRouteMatch = gatewayv1.HTTPRouteMatch HTTPRouteRule = gatewayv1.HTTPRouteRule + LocalObjectReference = gatewayv1.LocalObjectReference HTTPRouteSpec = gatewayv1.HTTPRouteSpec HTTPRouteStatus = gatewayv1.HTTPRouteStatus Hostname = gatewayv1.Hostname From 5851d22e20aeb52e0ff99dfd749e7cff4fd4ab3c Mon Sep 17 00:00:00 2001 From: Mattia Lavacca Date: Mon, 23 Oct 2023 16:48:06 +0200 Subject: [PATCH 2/4] address review's comments Signed-off-by: Mattia Lavacca --- CHANGELOG.md | 5 +- .../dataplane/parser/translate_httproute.go | 24 ++++++--- .../parser/translate_httproute_test.go | 11 ---- .../dataplane/parser/translators/httproute.go | 50 +++++++++++++------ .../parser/translators/httproute_atc.go | 18 ++++--- .../parser/translators/httproute_atc_test.go | 3 -- .../parser/translators/httproute_test.go | 47 ++++++++++++++--- 7 files changed, 109 insertions(+), 49 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dd44482298..bbe352d9d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -159,7 +159,6 @@ Adding a new version? You'll need three changes: version `gateway.networking.k8s.io/v1`. [#4935](https://github.com/Kong/kubernetes-ingress-controller/pull/4935) - ### Added - Added support for expression-based Kong routes for `TLSRoute`. This requires @@ -174,6 +173,10 @@ Adding a new version? You'll need three changes: [#4762](https://github.com/Kong/kubernetes-ingress-controller/pull/4762) - Support Query Parameter matching of `HTTPRoute` when expression router enabled. [#4780](https://github.com/Kong/kubernetes-ingress-controller/pull/4780) +- Support `ExtensionRef` HTTPRoute filter. It is now possibile to set a KongPlugin + reference in the `HTTPRoute`s' `ExtensionRef` filter field. + [#4838](https://github.com/Kong/kubernetes-ingress-controller/pull/4838) + [KIC Annotations reference]: https://docs.konghq.com/kubernetes-ingress-controller/latest/references/annotations/ diff --git a/internal/dataplane/parser/translate_httproute.go b/internal/dataplane/parser/translate_httproute.go index 25d4a3aaf4..2b964beb80 100644 --- a/internal/dataplane/parser/translate_httproute.go +++ b/internal/dataplane/parser/translate_httproute.go @@ -266,11 +266,16 @@ func generateKongRoutesFromHTTPRouteMatches( return filter.Type == gatewayapi.HTTPRouteFilterRequestRedirect }) - routes := getRoutesFromMatches(matches, &r, filters, tags, hasRedirectFilter) + routes, err := getRoutesFromMatches(matches, &r, filters, tags, hasRedirectFilter) + if err != nil { + return nil, err + } // if the redirect filter has not been set, we still need to set the route plugins if !hasRedirectFilter { - translators.ConvertFiltersToPlugins(&r, filters, "", tags) + if err := translators.SetRoutePlugins(&r, filters, "", tags); err != nil { + return nil, err + } routes = []kongstate.Route{r} } @@ -284,7 +289,7 @@ func getRoutesFromMatches( filters []gatewayapi.HTTPRouteFilter, tags []*string, hasRedirectFilter bool, -) []kongstate.Route { +) ([]kongstate.Route, error) { seenMethods := make(map[string]struct{}) routes := make([]kongstate.Route, 0) @@ -319,7 +324,9 @@ func getRoutesFromMatches( } // generate kong plugins from rule.filters - translators.ConvertFiltersToPlugins(matchRoute, filters, path, tags) + if err := translators.SetRoutePlugins(matchRoute, filters, path, tags); err != nil { + return nil, err + } routes = append(routes, *route) } else { @@ -342,7 +349,7 @@ func getRoutesFromMatches( } } } - return routes + return routes, nil } func generateKongRoutePathFromHTTPRouteMatch(match gatewayapi.HTTPRouteMatch) []string { @@ -426,9 +433,14 @@ func (p *Parser) ingressRulesFromSplitHTTPRouteMatchWithPriority( return err } + additionalRoutes, err := translators.KongExpressionRouteFromHTTPRouteMatchWithPriority(httpRouteMatchWithPriority) + if err != nil { + return err + } + kongService.Routes = append( kongService.Routes, - translators.KongExpressionRouteFromHTTPRouteMatchWithPriority(httpRouteMatchWithPriority), + *additionalRoutes, ) // cache the service to avoid duplicates in further loop iterations rules.ServiceNameToServices[serviceName] = kongService diff --git a/internal/dataplane/parser/translate_httproute_test.go b/internal/dataplane/parser/translate_httproute_test.go index 00a46665e3..ee5b6adc4d 100644 --- a/internal/dataplane/parser/translate_httproute_test.go +++ b/internal/dataplane/parser/translate_httproute_test.go @@ -1599,7 +1599,6 @@ func TestIngressRulesFromHTTPRoutesUsingExpressionRoutes(t *testing.T) { Expression: kong.String(`http.path == "/v1/foo"`), PreserveHost: kong.Bool(true), }, - Plugins: []kong.Plugin{}, ExpressionRoutes: true, }, { @@ -1608,7 +1607,6 @@ func TestIngressRulesFromHTTPRoutesUsingExpressionRoutes(t *testing.T) { Expression: kong.String(`http.path == "/v1/barr"`), PreserveHost: kong.Bool(true), }, - Plugins: []kong.Plugin{}, ExpressionRoutes: true, }, }, @@ -1703,7 +1701,6 @@ func TestIngressRulesFromHTTPRoutesUsingExpressionRoutes(t *testing.T) { Expression: kong.String(`(http.host == "foo.com") && (http.path == "/v1/foo")`), PreserveHost: kong.Bool(true), }, - Plugins: []kong.Plugin{}, ExpressionRoutes: true, }, }, @@ -1714,7 +1711,6 @@ func TestIngressRulesFromHTTPRoutesUsingExpressionRoutes(t *testing.T) { Expression: kong.String(`(http.host =^ ".bar.com") && (http.path == "/v1/foo")`), PreserveHost: kong.Bool(true), }, - Plugins: []kong.Plugin{}, ExpressionRoutes: true, }, }, @@ -1725,7 +1721,6 @@ func TestIngressRulesFromHTTPRoutesUsingExpressionRoutes(t *testing.T) { Expression: kong.String(`(http.host == "foo.com") && (http.path == "/v1/barr")`), PreserveHost: kong.Bool(true), }, - Plugins: []kong.Plugin{}, ExpressionRoutes: true, }, }, @@ -1736,7 +1731,6 @@ func TestIngressRulesFromHTTPRoutesUsingExpressionRoutes(t *testing.T) { Expression: kong.String(`(http.host =^ ".bar.com") && (http.path == "/v1/barr")`), PreserveHost: kong.Bool(true), }, - Plugins: []kong.Plugin{}, ExpressionRoutes: true, }, }, @@ -1793,7 +1787,6 @@ func TestIngressRulesFromHTTPRoutesUsingExpressionRoutes(t *testing.T) { Expression: kong.String(`(http.host == "foo.com") && (tls.sni == "foo.com") && (http.path == "/v1/foo")`), PreserveHost: kong.Bool(true), }, - Plugins: []kong.Plugin{}, ExpressionRoutes: true, }, }, @@ -1896,7 +1889,6 @@ func TestIngressRulesFromSplitHTTPRouteMatchWithPriority(t *testing.T) { StripPath: kong.Bool(false), Priority: kong.Int(1024), }, - Plugins: []kong.Plugin{}, ExpressionRoutes: true, }, }, @@ -2037,7 +2029,6 @@ func TestIngressRulesFromSplitHTTPRouteMatchWithPriority(t *testing.T) { StripPath: kong.Bool(false), Priority: kong.Int(1024), }, - Plugins: []kong.Plugin{}, ExpressionRoutes: true, }, }, @@ -2090,7 +2081,6 @@ func TestIngressRulesFromSplitHTTPRouteMatchWithPriority(t *testing.T) { StripPath: kong.Bool(false), Priority: kong.Int(1024), }, - Plugins: []kong.Plugin{}, ExpressionRoutes: true, }, }, @@ -2140,7 +2130,6 @@ func TestIngressRulesFromSplitHTTPRouteMatchWithPriority(t *testing.T) { StripPath: kong.Bool(false), Priority: kong.Int(1024), }, - Plugins: []kong.Plugin{}, ExpressionRoutes: true, }, }, diff --git a/internal/dataplane/parser/translators/httproute.go b/internal/dataplane/parser/translators/httproute.go index 9f131e2a81..618768c5d1 100644 --- a/internal/dataplane/parser/translators/httproute.go +++ b/internal/dataplane/parser/translators/httproute.go @@ -352,12 +352,21 @@ func mustMarshalJSON[T any](val T) string { return string(key) } -func ConvertFiltersToPlugins(route *kongstate.Route, filters []gatewayapi.HTTPRouteFilter, path string, tags []*string) { - plugins, pluginAnnotation := generatePluginsFromHTTPRouteFilters(filters, path, tags) - if route.Plugins == nil { - route.Plugins = make([]kong.Plugin, 0) +// SetRoutePlugins converts HTTPRouteFilter into Kong plugins. The plugins are set into the given kongstate.Route. +// The plugins can be set in two different ways: +// - Direct conversion from the respective HTTPRouteFilter. +// - ExtensionRef to plugins annotation from the ExtensionRef filter. +func SetRoutePlugins(route *kongstate.Route, filters []gatewayapi.HTTPRouteFilter, path string, tags []*string) error { + plugins, pluginAnnotation, err := generatePluginsFromHTTPRouteFilters(filters, path, tags) + if err != nil { + return err + } + if len(plugins) > 0 { + if route.Plugins == nil { + route.Plugins = make([]kong.Plugin, 0) + } + route.Plugins = append(route.Plugins, plugins...) } - route.Plugins = append(route.Plugins, plugins...) if len(pluginAnnotation) > 0 { if route.Ingress.Annotations == nil { route.Ingress.Annotations = make(map[string]string) @@ -370,17 +379,21 @@ func ConvertFiltersToPlugins(route *kongstate.Route, filters []gatewayapi.HTTPRo pluginAnnotation) } } + return nil } // generatePluginsFromHTTPRouteFilters converts HTTPRouteFilter into Kong plugins. // path is the parameter to be used by the redirect plugin, to perform redirection. -func generatePluginsFromHTTPRouteFilters(filters []gatewayapi.HTTPRouteFilter, path string, tags []*string) ([]kong.Plugin, string) { +// It returns two values: +// - A set of plugins generated by the conversion of all the provided filters, excluding ExtensionRefs. +// - A plugins annotation value, generated by the ExtensionRef filter. +func generatePluginsFromHTTPRouteFilters(filters []gatewayapi.HTTPRouteFilter, path string, tags []*string) ([]kong.Plugin, string, error) { kongPlugins := make([]kong.Plugin, 0) if len(filters) == 0 { - return kongPlugins, "" + return kongPlugins, "", nil } - var pluginsAnnotation string + var pluginsAnnotation strings.Builder for _, filter := range filters { switch filter.Type { case gatewayapi.HTTPRouteFilterRequestHeaderModifier: @@ -393,16 +406,20 @@ func generatePluginsFromHTTPRouteFilters(filters []gatewayapi.HTTPRouteFilter, p kongPlugins = append(kongPlugins, generateResponseHeaderModifierKongPlugin(filter.ResponseHeaderModifier)) case gatewayapi.HTTPRouteFilterExtensionRef: - plugin := generateExtensionRefKongPlugin(filter.ExtensionRef) - if len(pluginsAnnotation) > 0 { - pluginsAnnotation = fmt.Sprintf("%s,%s", pluginsAnnotation, plugin) + plugin, err := generateExtensionRefKongPlugin(filter.ExtensionRef) + if err != nil { + return nil, "", err + } + if len(pluginsAnnotation.String()) > 0 { + fmt.Fprintf(&pluginsAnnotation, ",%s", plugin) } else { - pluginsAnnotation = plugin + pluginsAnnotation.WriteString(plugin) } case gatewayapi.HTTPRouteFilterRequestMirror, gatewayapi.HTTPRouteFilterURLRewrite: // not supported + return nil, "", fmt.Errorf("httpFilter %s unsupported", filter.Type) } } for _, p := range kongPlugins { @@ -411,7 +428,7 @@ func generatePluginsFromHTTPRouteFilters(filters []gatewayapi.HTTPRouteFilter, p p.Tags = tags } - return kongPlugins, pluginsAnnotation + return kongPlugins, pluginsAnnotation.String(), nil } // generateRequestRedirectKongPlugin generates configurations of plugins to satisfy the specification @@ -459,8 +476,11 @@ func generateRequestRedirectKongPlugin(modifier *gatewayapi.HTTPRequestRedirectF return plugins } -func generateExtensionRefKongPlugin(modifier *gatewayapi.LocalObjectReference) string { - return string(modifier.Name) +func generateExtensionRefKongPlugin(modifier *gatewayapi.LocalObjectReference) (string, error) { + if modifier.Group != "configuration.konghq.com" || modifier.Kind != "KongPlugin" { + return "", fmt.Errorf("plugin %s/%s unsupported", modifier.Group, modifier.Kind) + } + return string(modifier.Name), nil } // generateRequestHeaderModifierKongPlugin converts a gatewayapi.HTTPRequestHeaderFilter into a diff --git a/internal/dataplane/parser/translators/httproute_atc.go b/internal/dataplane/parser/translators/httproute_atc.go index 0bef5cf02d..ef1a109e24 100644 --- a/internal/dataplane/parser/translators/httproute_atc.go +++ b/internal/dataplane/parser/translators/httproute_atc.go @@ -66,7 +66,9 @@ func GenerateKongExpressionRoutesFromHTTPRouteMatches( atc.ApplyExpression(&r.Route, routeMatcher, 1) // generate plugins. - ConvertFiltersToPlugins(&r, translation.Filters, "", tags) + if err := SetRoutePlugins(&r, translation.Filters, "", tags); err != nil { + return nil, err + } return []kongstate.Route{r}, nil } @@ -103,7 +105,9 @@ func generateKongExpressionRoutesWithRequestRedirectFilter( if match.Path != nil && match.Path.Value != nil { path = *match.Path.Value } - ConvertFiltersToPlugins(&matchRoute, translation.Filters, path, tags) + if err := SetRoutePlugins(&matchRoute, translation.Filters, path, tags); err != nil { + return nil, err + } routes = append(routes, matchRoute) } return routes, nil @@ -537,7 +541,7 @@ func compareSplitHTTPRouteMatchesRelativePriority(match1, match2 SplitHTTPRouteM // based kong route with assigned priority. func KongExpressionRouteFromHTTPRouteMatchWithPriority( httpRouteMatchWithPriority SplitHTTPRouteMatchToKongRoutePriority, -) kongstate.Route { +) (*kongstate.Route, error) { match := httpRouteMatchWithPriority.Match httproute := httpRouteMatchWithPriority.Match.Source tags := util.GenerateTagsForObject(httproute) @@ -556,7 +560,7 @@ func KongExpressionRouteFromHTTPRouteMatchWithPriority( match.MatchIndex, ) - r := kongstate.Route{ + r := &kongstate.Route{ Route: kong.Route{ Name: kong.String(routeName), PreserveHost: kong.Bool(true), @@ -590,10 +594,12 @@ func KongExpressionRouteFromHTTPRouteMatchWithPriority( path = *match.Match.Path.Value } - ConvertFiltersToPlugins(&r, rule.Filters, path, tags) + if err := SetRoutePlugins(r, rule.Filters, path, tags); err != nil { + return nil, err + } } - return r + return r, nil } // KongServiceNameFromSplitHTTPRouteMatch generates service name from split HTTPRoute match. diff --git a/internal/dataplane/parser/translators/httproute_atc_test.go b/internal/dataplane/parser/translators/httproute_atc_test.go index b097c8aee7..14c927ca2d 100644 --- a/internal/dataplane/parser/translators/httproute_atc_test.go +++ b/internal/dataplane/parser/translators/httproute_atc_test.go @@ -79,7 +79,6 @@ func TestGenerateKongExpressionRoutesFromHTTPRouteMatches(t *testing.T) { Expression: kong.String(`(http.path == "/prefix") || (http.path ^= "/prefix/")`), Priority: kong.Int(1), }, - Plugins: []kong.Plugin{}, ExpressionRoutes: true, }, }, @@ -101,7 +100,6 @@ func TestGenerateKongExpressionRoutesFromHTTPRouteMatches(t *testing.T) { Expression: kong.String(`((http.path == "/prefix") || (http.path ^= "/prefix/")) || ((http.path == "/exact") && (http.method == "GET"))`), Priority: kong.Int(1), }, - Plugins: []kong.Plugin{}, ExpressionRoutes: true, }, }, @@ -244,7 +242,6 @@ func TestGenerateKongExpressionRoutesFromHTTPRouteMatches(t *testing.T) { Expression: kong.String(`((http.path == "/prefix/0") || (http.path ^= "/prefix/0/")) && (http.host == "a.foo.com") && (tls.sni == "a.foo.com")`), Priority: kong.Int(1), }, - Plugins: []kong.Plugin{}, ExpressionRoutes: true, }, }, diff --git a/internal/dataplane/parser/translators/httproute_test.go b/internal/dataplane/parser/translators/httproute_test.go index e1115af9b7..6a6ad935b1 100644 --- a/internal/dataplane/parser/translators/httproute_test.go +++ b/internal/dataplane/parser/translators/httproute_test.go @@ -1,6 +1,7 @@ package translators import ( + "errors" "testing" "github.com/kong/go-kong/kong" @@ -17,6 +18,7 @@ func TestGeneratePluginsFromHTTPRouteFilters(t *testing.T) { path string expectedPlugins []kong.Plugin expectedPluginsAnnotation string + expectedErr error }{ { name: "no filters", @@ -24,7 +26,7 @@ func TestGeneratePluginsFromHTTPRouteFilters(t *testing.T) { expectedPlugins: []kong.Plugin{}, }, { - name: "request header modifier", + name: "request header modifier filter", filters: []gatewayapi.HTTPRouteFilter{ { Type: gatewayapi.HTTPRouteFilterRequestHeaderModifier, @@ -74,7 +76,7 @@ func TestGeneratePluginsFromHTTPRouteFilters(t *testing.T) { }, }, { - name: "request redirect modifier", + name: "request redirect modifier filter", filters: []gatewayapi.HTTPRouteFilter{ { Type: gatewayapi.HTTPRouteFilterRequestRedirect, @@ -105,7 +107,7 @@ func TestGeneratePluginsFromHTTPRouteFilters(t *testing.T) { }, }, { - name: "response header modifier", + name: "response header modifier filter", filters: []gatewayapi.HTTPRouteFilter{ { Type: gatewayapi.HTTPRouteFilterResponseHeaderModifier, @@ -155,12 +157,12 @@ func TestGeneratePluginsFromHTTPRouteFilters(t *testing.T) { }, }, { - name: "extension-refs", + name: "valid extensionrefs filters", filters: []gatewayapi.HTTPRouteFilter{ { Type: gatewayapi.HTTPRouteFilterExtensionRef, ExtensionRef: &gatewayapi.LocalObjectReference{ - Group: gatewayapi.Group("configuration.konghq.com/v1"), + Group: gatewayapi.Group("configuration.konghq.com"), Kind: gatewayapi.Kind("KongPlugin"), Name: "plugin1", }, @@ -168,7 +170,7 @@ func TestGeneratePluginsFromHTTPRouteFilters(t *testing.T) { { Type: gatewayapi.HTTPRouteFilterExtensionRef, ExtensionRef: &gatewayapi.LocalObjectReference{ - Group: gatewayapi.Group("configuration.konghq.com/v1"), + Group: gatewayapi.Group("configuration.konghq.com"), Kind: gatewayapi.Kind("KongPlugin"), Name: "plugin2", }, @@ -177,12 +179,43 @@ func TestGeneratePluginsFromHTTPRouteFilters(t *testing.T) { expectedPluginsAnnotation: "plugin1,plugin2", expectedPlugins: []kong.Plugin{}, }, + { + name: "invalid extensionrefs filter group", + filters: []gatewayapi.HTTPRouteFilter{ + { + Type: gatewayapi.HTTPRouteFilterExtensionRef, + ExtensionRef: &gatewayapi.LocalObjectReference{ + Group: gatewayapi.Group("wrong.group"), + Kind: gatewayapi.Kind("KongPlugin"), + Name: "plugin1", + }, + }, + }, + expectedPluginsAnnotation: "", + expectedErr: errors.New("plugin wrong.group/KongPlugin unsupported"), + }, + { + name: "invalid extensionrefs filter kind", + filters: []gatewayapi.HTTPRouteFilter{ + { + Type: gatewayapi.HTTPRouteFilterExtensionRef, + ExtensionRef: &gatewayapi.LocalObjectReference{ + Group: gatewayapi.Group("configuration.konghq.com"), + Kind: gatewayapi.Kind("WrongKind"), + Name: "plugin1", + }, + }, + }, + expectedPluginsAnnotation: "", + expectedErr: errors.New("plugin configuration.konghq.com/WrongKind unsupported"), + }, } for _, tc := range testCases { tc := tc t.Run(tc.name, func(t *testing.T) { - plugins, pluginsAnnotation := generatePluginsFromHTTPRouteFilters(tc.filters, tc.path, nil) + plugins, pluginsAnnotation, err := generatePluginsFromHTTPRouteFilters(tc.filters, tc.path, nil) + require.Equal(t, tc.expectedErr, err) require.Equal(t, tc.expectedPlugins, plugins) require.Equal(t, tc.expectedPluginsAnnotation, pluginsAnnotation) }) From ddb9a650ce0e23f4edab784905d7d1ac9395a01c Mon Sep 17 00:00:00 2001 From: Mattia Lavacca Date: Tue, 24 Oct 2023 15:19:54 +0200 Subject: [PATCH 3/4] Apply suggestions from code review Co-authored-by: Jakub Warczarek --- internal/dataplane/parser/translators/httproute.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/internal/dataplane/parser/translators/httproute.go b/internal/dataplane/parser/translators/httproute.go index 618768c5d1..9568596b0e 100644 --- a/internal/dataplane/parser/translators/httproute.go +++ b/internal/dataplane/parser/translators/httproute.go @@ -362,20 +362,18 @@ func SetRoutePlugins(route *kongstate.Route, filters []gatewayapi.HTTPRouteFilte return err } if len(plugins) > 0 { - if route.Plugins == nil { - route.Plugins = make([]kong.Plugin, 0) - } route.Plugins = append(route.Plugins, plugins...) } if len(pluginAnnotation) > 0 { if route.Ingress.Annotations == nil { route.Ingress.Annotations = make(map[string]string) } - if _, ok := route.Ingress.Annotations[annotations.AnnotationPrefix+annotations.PluginsKey]; !ok { - route.Ingress.Annotations[annotations.AnnotationPrefix+annotations.PluginsKey] = pluginAnnotation + const pluginAnnotationKey = annotations.AnnotationPrefix + annotations.PluginsKey + if _, ok := route.Ingress.Annotations[pluginAnnotationKey]; !ok { + route.Ingress.Annotations[pluginAnnotationKey] = pluginAnnotation } else { - route.Ingress.Annotations[annotations.AnnotationPrefix+annotations.PluginsKey] = fmt.Sprintf("%s,%s", - route.Ingress.Annotations[annotations.AnnotationPrefix+annotations.PluginsKey], + route.Ingress.Annotations[pluginAnnotationKey] = fmt.Sprintf("%s,%s", + route.Ingress.Annotations[pluginAnnotationKey], pluginAnnotation) } } From b19b29468f16ef3101e5e0b6eb86b2da7d42bee2 Mon Sep 17 00:00:00 2001 From: Mattia Lavacca Date: Tue, 24 Oct 2023 17:20:25 +0200 Subject: [PATCH 4/4] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Patryk Małek --- CHANGELOG.md | 1 - internal/dataplane/parser/translators/httproute.go | 9 +++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bbe352d9d3..e8f7f19966 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -177,7 +177,6 @@ Adding a new version? You'll need three changes: reference in the `HTTPRoute`s' `ExtensionRef` filter field. [#4838](https://github.com/Kong/kubernetes-ingress-controller/pull/4838) - [KIC Annotations reference]: https://docs.konghq.com/kubernetes-ingress-controller/latest/references/annotations/ ## 2.12.1 diff --git a/internal/dataplane/parser/translators/httproute.go b/internal/dataplane/parser/translators/httproute.go index 9568596b0e..95b12e921b 100644 --- a/internal/dataplane/parser/translators/httproute.go +++ b/internal/dataplane/parser/translators/httproute.go @@ -361,9 +361,7 @@ func SetRoutePlugins(route *kongstate.Route, filters []gatewayapi.HTTPRouteFilte if err != nil { return err } - if len(plugins) > 0 { - route.Plugins = append(route.Plugins, plugins...) - } + route.Plugins = append(route.Plugins, plugins...) if len(pluginAnnotation) > 0 { if route.Ingress.Annotations == nil { route.Ingress.Annotations = make(map[string]string) @@ -409,7 +407,10 @@ func generatePluginsFromHTTPRouteFilters(filters []gatewayapi.HTTPRouteFilter, p return nil, "", err } if len(pluginsAnnotation.String()) > 0 { - fmt.Fprintf(&pluginsAnnotation, ",%s", plugin) + _, err := pluginsAnnotation.WriteString("," + plugin) + if err != nil { + return nil, "", err + } } else { pluginsAnnotation.WriteString(plugin) }