diff --git a/changelogs/unreleased/4197-sunjayBhatia-small.md b/changelogs/unreleased/4197-sunjayBhatia-small.md new file mode 100644 index 00000000000..9a789709703 --- /dev/null +++ b/changelogs/unreleased/4197-sunjayBhatia-small.md @@ -0,0 +1,2 @@ +Very long (~100 characters) Ingress path prefix matches should now no longer be rejected by Envoy. +See [this issue](https://github.com/projectcontour/contour/issues/4191) for context. diff --git a/internal/envoy/v3/route.go b/internal/envoy/v3/route.go index a1ef05efed1..6bade5dbc01 100644 --- a/internal/envoy/v3/route.go +++ b/internal/envoy/v3/route.go @@ -59,7 +59,7 @@ func RouteAuthzContext(settings map[string]string) *any.Any { ) } -const prefixPathMatchSegmentRegex = `((\/).*)?` +const prefixPathMatchSegmentRegex = `(?:[\/].*)*` var _ = regexp.MustCompile(prefixPathMatchSegmentRegex) @@ -69,7 +69,9 @@ func RouteMatch(route *dag.Route) *envoy_route_v3.RouteMatch { case *dag.RegexMatchCondition: return &envoy_route_v3.RouteMatch{ PathSpecifier: &envoy_route_v3.RouteMatch_SafeRegex{ - SafeRegex: SafeRegexMatch(c.Regex), + // Add an anchor since we at the very least have a / as a string literal prefix. + // Reduces regex program size so Envoy doesn't reject long prefix matches. + SafeRegex: SafeRegexMatch("^" + c.Regex), }, Headers: headerMatcher(route.HeaderMatchConditions), } @@ -78,7 +80,9 @@ func RouteMatch(route *dag.Route) *envoy_route_v3.RouteMatch { case dag.PrefixMatchSegment: return &envoy_route_v3.RouteMatch{ PathSpecifier: &envoy_route_v3.RouteMatch_SafeRegex{ - SafeRegex: SafeRegexMatch(regexp.QuoteMeta(c.Prefix) + prefixPathMatchSegmentRegex), + // Add anchor since we have a string literal prefix. + // Reduces regex program size so Envoy doesn't reject long prefix matches. + SafeRegex: SafeRegexMatch("^" + regexp.QuoteMeta(c.Prefix) + prefixPathMatchSegmentRegex), }, Headers: headerMatcher(route.HeaderMatchConditions), } diff --git a/internal/envoy/v3/route_test.go b/internal/envoy/v3/route_test.go index cd1cf17c251..34f4f47facb 100644 --- a/internal/envoy/v3/route_test.go +++ b/internal/envoy/v3/route_test.go @@ -1145,7 +1145,7 @@ func TestRouteMatch(t *testing.T) { }, want: &envoy_route_v3.RouteMatch{ PathSpecifier: &envoy_route_v3.RouteMatch_SafeRegex{ - SafeRegex: SafeRegexMatch(`/foo((\/).*)?`), + SafeRegex: SafeRegexMatch(`^/foo(?:[\/].*)*`), }, }, }, @@ -1158,7 +1158,7 @@ func TestRouteMatch(t *testing.T) { }, want: &envoy_route_v3.RouteMatch{ PathSpecifier: &envoy_route_v3.RouteMatch_SafeRegex{ - SafeRegex: SafeRegexMatch(`/foo\.bar((\/).*)?`), + SafeRegex: SafeRegexMatch(`^/foo\.bar(?:[\/].*)*`), }, }, }, @@ -1185,7 +1185,9 @@ func TestRouteMatch(t *testing.T) { // note, unlike header conditions this is not a quoted regex because // the value comes directly from the Ingress.Paths.Path value which // is permitted to be a bare regex. - SafeRegex: SafeRegexMatch("/v.1/*"), + // We add an anchor since we should always have a / prefix to reduce + // complexity. + SafeRegex: SafeRegexMatch("^/v.1/*"), }, }, }, diff --git a/internal/featuretests/v3/envoy.go b/internal/featuretests/v3/envoy.go index 15cc0b180d5..1e16a026b9a 100644 --- a/internal/featuretests/v3/envoy.go +++ b/internal/featuretests/v3/envoy.go @@ -125,7 +125,7 @@ func routeSegmentPrefix(prefix string) *envoy_route_v3.RouteMatch { EngineType: &matcher.RegexMatcher_GoogleRe2{ GoogleRe2: &matcher.RegexMatcher_GoogleRE2{}, }, - Regex: regexp.QuoteMeta(prefix) + `((\/).*)?`, + Regex: "^" + regexp.QuoteMeta(prefix) + `(?:[\/].*)*`, }, }, } diff --git a/internal/xdscache/v3/route_test.go b/internal/xdscache/v3/route_test.go index e7468b38a26..fbe33bf56e9 100644 --- a/internal/xdscache/v3/route_test.go +++ b/internal/xdscache/v3/route_test.go @@ -3311,7 +3311,7 @@ func routeRegex(regex string, headers ...dag.HeaderMatchCondition) *envoy_route_ func routePrefixIngress(prefix string, headers ...dag.HeaderMatchCondition) *envoy_route_v3.RouteMatch { return envoy_v3.RouteMatch(&dag.Route{ PathMatchCondition: &dag.RegexMatchCondition{ - Regex: regexp.QuoteMeta(prefix) + `((\/).*)?`, + Regex: regexp.QuoteMeta(prefix) + `(?:[\/].*)*`, }, HeaderMatchConditions: headers, }) diff --git a/test/e2e/ingress/ingress_test.go b/test/e2e/ingress/ingress_test.go index 828bb51452a..8b2117ea0d2 100644 --- a/test/e2e/ingress/ingress_test.go +++ b/test/e2e/ingress/ingress_test.go @@ -181,6 +181,8 @@ var _ = Describe("Ingress", func() { testBackendTLS(namespace) }) }) + + f.NamespacedTest("long-path-match", testLongPathMatch) }) func pathTypePtr(val networkingv1.PathType) *networkingv1.PathType { diff --git a/test/e2e/ingress/long_path_match_test.go b/test/e2e/ingress/long_path_match_test.go new file mode 100644 index 00000000000..1da6a1b4a58 --- /dev/null +++ b/test/e2e/ingress/long_path_match_test.go @@ -0,0 +1,113 @@ +// Copyright Project Contour Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build e2e +// +build e2e + +package ingress + +import ( + "context" + "net/http" + "strings" + + . "github.com/onsi/ginkgo" + "github.com/projectcontour/contour/test/e2e" + "github.com/stretchr/testify/require" + networkingv1 "k8s.io/api/networking/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func testLongPathMatch(namespace string) { + Specify("long path matches should be properly programmed", func() { + f.Fixtures.Echo.Deploy(namespace, "echo") + + // Just on the edge, should be RE2 program size 101 before regex optimizations. + longPrefixMatch := "/" + strings.Repeat("a", 82) + reallyLongPrefixMatch := "/" + strings.Repeat("b", 500) + longRegexMatch := "/" + strings.Repeat("c", 200) + ".*" + + i := &networkingv1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace, + Name: "long-patch-match", + }, + Spec: networkingv1.IngressSpec{ + Rules: []networkingv1.IngressRule{ + { + Host: "long-patch-match.ingress.projectcontour.io", + IngressRuleValue: networkingv1.IngressRuleValue{ + HTTP: &networkingv1.HTTPIngressRuleValue{ + Paths: []networkingv1.HTTPIngressPath{ + { + PathType: pathTypePtr(networkingv1.PathTypePrefix), + Path: longPrefixMatch, + Backend: networkingv1.IngressBackend{ + Service: &networkingv1.IngressServiceBackend{ + Name: "echo", + Port: networkingv1.ServiceBackendPort{ + Number: 80, + }, + }, + }, + }, + { + PathType: pathTypePtr(networkingv1.PathTypePrefix), + Path: reallyLongPrefixMatch, + Backend: networkingv1.IngressBackend{ + Service: &networkingv1.IngressServiceBackend{ + Name: "echo", + Port: networkingv1.ServiceBackendPort{ + Number: 80, + }, + }, + }, + }, + { + PathType: pathTypePtr(networkingv1.PathTypeImplementationSpecific), + Path: longRegexMatch, + Backend: networkingv1.IngressBackend{ + Service: &networkingv1.IngressServiceBackend{ + Name: "echo", + Port: networkingv1.ServiceBackendPort{ + Number: 80, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + } + require.NoError(f.T(), f.Client.Create(context.TODO(), i)) + + cases := []string{ + longPrefixMatch, + reallyLongPrefixMatch, + // Cut off end .* when making request. + longRegexMatch[:len(longRegexMatch)-2], + } + for _, path := range cases { + res, ok := f.HTTP.RequestUntil(&e2e.HTTPRequestOpts{ + Host: i.Spec.Rules[0].Host, + Path: path, + Condition: e2e.HasStatusCode(http.StatusOK), + }) + require.NotNil(f.T(), res, "request never succeeded") + require.Truef(f.T(), ok, "expected %d response code, got %d", http.StatusOK, res.StatusCode) + } + }) +}