Make filters part of MatchRule

nginx · Sep 8, 2022 · 875b213 · 875b213
1 parent a30aa71
commit 875b213
Show file tree

Hide file tree

Showing 4 changed files with 174 additions and 203 deletions.
diff --git a/internal/nginx/config/generator.go b/internal/nginx/config/generator.go
@@ -126,15 +126,10 @@ func generate(virtualServer state.VirtualServer, serviceStore state.ServiceStore
 			// If it doesn't work as expected, such situation is silently handled below in findFirstFilters.
 			// Consider reporting an error. But that should be done in a separate validation layer.
 
-			firstFilters := findFirstFilters(r.GetFilters(), supportedFilters)
-
-			loc.Return = generateReturnValForRedirectFilter(
-				firstFilters[v1beta1.HTTPRouteFilterRequestRedirect].RequestRedirect,
-				listenerPort,
-			)
-
-			// ProxyPass is mutually exclusive with Return
-			if loc.Return == nil {
+			// RequestRedirect and proxying are mutually exclusive.
+			if r.Filters.RequestRedirect != nil {
+				loc.Return = generateReturnValForRedirectFilter(r.Filters.RequestRedirect, listenerPort)
+			} else {
 				address, err := getBackendAddress(r.Source.Spec.Rules[r.RuleIdx].BackendRefs, r.Source.Namespace, serviceStore)
 				if err != nil {
 					warnings.AddWarning(r.Source, err.Error())
@@ -174,25 +169,6 @@ func generateProxyPass(address string) string {
 	return "http://" + address
 }
 
-var supportedFilters = map[v1beta1.HTTPRouteFilterType]struct{}{
-	v1beta1.HTTPRouteFilterRequestRedirect: {},
-}
-
-func findFirstFilters(
-	filters []v1beta1.HTTPRouteFilter,
-	filterTypes map[v1beta1.HTTPRouteFilterType]struct{},
-) map[v1beta1.HTTPRouteFilterType]v1beta1.HTTPRouteFilter {
-
-	result := make(map[v1beta1.HTTPRouteFilterType]v1beta1.HTTPRouteFilter)
-	for i := len(filters) - 1; i >= 0; i-- {
-		if _, exist := filterTypes[filters[i].Type]; exist {
-			result[filters[i].Type] = filters[i]
-		}
-	}
-
-	return result
-}
-
 func generateReturnValForRedirectFilter(filter *v1beta1.HTTPRequestRedirectFilter, listenerPort int) *returnVal {
 	if filter == nil {
 		return nil

diff --git a/internal/nginx/config/generator_test.go b/internal/nginx/config/generator_test.go
@@ -221,14 +221,7 @@ func TestGenerate(t *testing.T) {
 							},
 						},
 					},
-					Filters: []v1beta1.HTTPRouteFilter{
-						{
-							Type: v1beta1.HTTPRouteFilterRequestRedirect,
-							RequestRedirect: &v1beta1.HTTPRequestRedirectFilter{
-								Hostname: (*v1beta1.PreciseHostname)(helpers.GetStringPointer("foo.example.com")),
-							},
-						},
-					},
+					// redirect is set in the corresponding state.MatchRule
 				},
 				{
 					// A match with a redirect with explicit port
@@ -239,15 +232,7 @@ func TestGenerate(t *testing.T) {
 							},
 						},
 					},
-					Filters: []v1beta1.HTTPRouteFilter{
-						{
-							Type: v1beta1.HTTPRouteFilterRequestRedirect,
-							RequestRedirect: &v1beta1.HTTPRequestRedirectFilter{
-								Hostname: (*v1beta1.PreciseHostname)(helpers.GetStringPointer("bar.example.com")),
-								Port:     (*v1beta1.PortNumber)(helpers.GetInt32Pointer(8080)),
-							},
-						},
-					},
+					// redirect is set in the corresponding state.MatchRule
 				},
 			},
 		},
@@ -342,6 +327,11 @@ func TestGenerate(t *testing.T) {
 							MatchIdx: 0,
 							RuleIdx:  3,
 							Source:   hr,
+							Filters: state.Filters{
+								RequestRedirect: &v1beta1.HTTPRequestRedirectFilter{
+									Hostname: (*v1beta1.PreciseHostname)(helpers.GetStringPointer("foo.example.com")),
+								},
+							},
 						},
 					},
 				},
@@ -352,6 +342,12 @@ func TestGenerate(t *testing.T) {
 							MatchIdx: 0,
 							RuleIdx:  4,
 							Source:   hr,
+							Filters: state.Filters{
+								RequestRedirect: &v1beta1.HTTPRequestRedirectFilter{
+									Hostname: (*v1beta1.PreciseHostname)(helpers.GetStringPointer("bar.example.com")),
+									Port:     (*v1beta1.PortNumber)(helpers.GetInt32Pointer(8080)),
+								},
+							},
 						},
 					},
 				},
@@ -476,95 +472,6 @@ func TestGenerateProxyPass(t *testing.T) {
 	}
 }
 
-func TestFindFirstFilters(t *testing.T) {
-	oneType := map[v1beta1.HTTPRouteFilterType]struct{}{
-		v1beta1.HTTPRouteFilterRequestRedirect: {},
-	}
-
-	twoTypes := map[v1beta1.HTTPRouteFilterType]struct{}{
-		v1beta1.HTTPRouteFilterRequestRedirect: {},
-		v1beta1.HTTPRouteFilterURLRewrite:      {},
-	}
-
-	redirect1 := v1beta1.HTTPRouteFilter{
-		Type: v1beta1.HTTPRouteFilterRequestRedirect,
-		RequestRedirect: &v1beta1.HTTPRequestRedirectFilter{
-			Hostname: (*v1beta1.PreciseHostname)(helpers.GetStringPointer("foo.example.com")),
-		},
-	}
-	redirect2 := v1beta1.HTTPRouteFilter{
-		Type: v1beta1.HTTPRouteFilterRequestRedirect,
-		RequestRedirect: &v1beta1.HTTPRequestRedirectFilter{
-			Hostname: (*v1beta1.PreciseHostname)(helpers.GetStringPointer("bar.example.com")),
-		},
-	}
-	rewrite1 := v1beta1.HTTPRouteFilter{
-		Type: v1beta1.HTTPRouteFilterURLRewrite,
-		URLRewrite: &v1beta1.HTTPURLRewriteFilter{
-			Hostname: (*v1beta1.PreciseHostname)(helpers.GetStringPointer("foo.example.com")),
-		},
-	}
-	rewrite2 := v1beta1.HTTPRouteFilter{
-		Type: v1beta1.HTTPRouteFilterURLRewrite,
-		URLRewrite: &v1beta1.HTTPURLRewriteFilter{
-			Hostname: (*v1beta1.PreciseHostname)(helpers.GetStringPointer("bar.example.com")),
-		},
-	}
-
-	oneTypeFilters := []v1beta1.HTTPRouteFilter{redirect1, redirect2}
-
-	twoTypesFilters := []v1beta1.HTTPRouteFilter{
-		redirect1,
-		rewrite1,
-		rewrite2,
-		redirect2,
-	}
-
-	tests := []struct {
-		filters     []v1beta1.HTTPRouteFilter
-		filterTypes map[v1beta1.HTTPRouteFilterType]struct{}
-		expected    map[v1beta1.HTTPRouteFilterType]v1beta1.HTTPRouteFilter
-		msg         string
-	}{
-		{
-			filters:     []v1beta1.HTTPRouteFilter{},
-			filterTypes: twoTypes,
-			expected:    map[v1beta1.HTTPRouteFilterType]v1beta1.HTTPRouteFilter{},
-			msg:         "no filters",
-		},
-		{
-			filters:     oneTypeFilters,
-			filterTypes: oneType,
-			expected: map[v1beta1.HTTPRouteFilterType]v1beta1.HTTPRouteFilter{
-				v1beta1.HTTPRouteFilterRequestRedirect: redirect1,
-			},
-			msg: "only one type",
-		},
-		{
-			filters:     twoTypesFilters,
-			filterTypes: map[v1beta1.HTTPRouteFilterType]struct{}{},
-			expected:    map[v1beta1.HTTPRouteFilterType]v1beta1.HTTPRouteFilter{},
-			msg:         "no supported type",
-		},
-		{
-			filters:     twoTypesFilters,
-			filterTypes: twoTypes,
-			expected: map[v1beta1.HTTPRouteFilterType]v1beta1.HTTPRouteFilter{
-				v1beta1.HTTPRouteFilterRequestRedirect: redirect1,
-				v1beta1.HTTPRouteFilterURLRewrite:      rewrite1,
-			},
-			msg: "two types two filters",
-		},
-	}
-
-	for _, test := range tests {
-		result := findFirstFilters(test.filters, test.filterTypes)
-		if diff := cmp.Diff(test.expected, result); diff != "" {
-			t.Errorf("findFirstFilters() mismatch '%s' (-want +got):\n%s", test.msg, diff)
-		}
-	}
-}
-
 func TestGenerateReturnValForRedirectFilter(t *testing.T) {
 	const listenerPort = 123
 

diff --git a/internal/state/configuration.go b/internal/state/configuration.go
@@ -44,6 +44,11 @@ type PathRule struct {
 	MatchRules []MatchRule
 }
 
+// Filters hold the filters for a MatchRule.
+type Filters struct {
+	RequestRedirect *v1beta1.HTTPRequestRedirectFilter
+}
+
 // MatchRule represents a routing rule. It corresponds directly to a Match in the HTTPRoute resource.
 // An HTTPRoute is guaranteed to have at least one rule with one match.
 // If no rule or match is specified by the user, the default rule {{path:{ type: "PathPrefix", value: "/"}}} is set by the schema.
@@ -53,19 +58,18 @@ type MatchRule struct {
 	// RuleIdx is the index of the corresponding rule in the HTTPRoute.
 	RuleIdx int
 	// Source is the corresponding HTTPRoute resource.
+	// FIXME(pleshakov): Consider referencing only the parts neeeded for the config generation rather than
+	// the entire resource.
 	Source *v1beta1.HTTPRoute
+	// Filters holds the filters for the MatchRule.
+	Filters Filters
 }
 
 // GetMatch returns the HTTPRouteMatch of the Route .
 func (r *MatchRule) GetMatch() v1beta1.HTTPRouteMatch {
 	return r.Source.Spec.Rules[r.RuleIdx].Matches[r.MatchIdx]
 }
 
-// GetFilters returns the filters for the MatchRule.
-func (r *MatchRule) GetFilters() []v1beta1.HTTPRouteFilter {
-	return r.Source.Spec.Rules[r.RuleIdx].Filters
-}
-
 // buildConfiguration builds the Configuration from the graph.
 // FIXME(pleshakov) For now we only handle paths with prefix matches. Handle exact and regex matches
 func buildConfiguration(graph *graph) Configuration {
@@ -158,6 +162,8 @@ func (b *virtualServerBuilder) upsertListener(l *listener) {
 		}
 
 		for i, rule := range r.Source.Spec.Rules {
+			filters := createFilters(rule.Filters)
+
 			for _, h := range hostnames {
 				for j, m := range rule.Matches {
 					path := getPath(m.Path)
@@ -171,6 +177,7 @@ func (b *virtualServerBuilder) upsertListener(l *listener) {
 						MatchIdx: j,
 						RuleIdx:  i,
 						Source:   r.Source,
+						Filters:  filters,
 					})
 
 					b.rulesPerHost[h][path] = rule
@@ -246,3 +253,18 @@ func getPath(path *v1beta1.HTTPPathMatch) string {
 	}
 	return *path.Value
 }
+
+func createFilters(filters []v1beta1.HTTPRouteFilter) Filters {
+	var result Filters
+
+	for _, f := range filters {
+		switch f.Type {
+		case v1beta1.HTTPRouteFilterRequestRedirect:
+			result.RequestRedirect = f.RequestRedirect
+			// using the first filter
+			return result
+		}
+	}
+
+	return result
+}