From 4cf806b8ac0b8ea474cbac0938569ab294108016 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Th=C3=B6mmes?= Date: Tue, 10 Dec 2019 16:05:07 +0100 Subject: [PATCH] Cherry picks from 0.10.1 (#361) * squash (#6174) (#6175) * Use prefix instead of regex for authority match in virtualservice (#6088) (#6183) * Use prefix instead of regex for authority match in virtualservice This patch changes to use prefix instead of regex for authority match in virtualservice. As described in https://github.com/knative/serving/issues/6058, Istio 1.4 introduced 100 bytes limitation for the regex. So, Knative service which has long service name or domain name, it hits the limit easily. To fix it, this patch uses `prefix` and stop using `regex`. Current regex in VirtualService should be able to replaced with Prefix. CURRENT: ``` regex: ^hello-example\.default\.example\.com(?::\d{1,5})?$ ``` AFTER: ``` prefix: hello-example.default.example.com ``` * Trim cluster local domain to match local --- .../ingress/resources/virtual_service.go | 25 +++---- .../ingress/resources/virtual_service_test.go | 14 ++-- pkg/reconciler/route/resources/service.go | 6 +- .../route/resources/service_test.go | 68 +++++++++++-------- test/e2e/service_to_service_test.go | 14 ++-- 5 files changed, 68 insertions(+), 59 deletions(-) diff --git a/pkg/reconciler/ingress/resources/virtual_service.go b/pkg/reconciler/ingress/resources/virtual_service.go index 14784b20ffbc..0a73d113ac81 100644 --- a/pkg/reconciler/ingress/resources/virtual_service.go +++ b/pkg/reconciler/ingress/resources/virtual_service.go @@ -20,7 +20,6 @@ import ( "crypto/md5" "encoding/json" "fmt" - "regexp" "strings" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -282,7 +281,8 @@ func makeMatch(host string, pathRegExp string, gateways sets.String) v1alpha3.HT match := v1alpha3.HTTPMatchRequest{ Gateways: gateways.List(), Authority: &istiov1alpha1.StringMatch{ - Regex: hostRegExp(host), + // Do not use Regex as Istio 1.4 or later has 100 bytes limitation. + Prefix: hostPrefix(host), }, } // Empty pathRegExp is considered match all path. We only need to @@ -295,29 +295,20 @@ func makeMatch(host string, pathRegExp string, gateways sets.String) v1alpha3.HT return match } -// Should only match 1..65535, but for simplicity it matches 0-99999. -const portMatch = `(?::\d{1,5})?` - -// hostRegExp returns an ECMAScript regular expression to match either host or host: -// for clusterLocalHost, we will also match the prefixes. -func hostRegExp(host string) string { +// hostPrefix returns an host to match either host or host:. +// For clusterLocalHost, it trims .svc. from the host to match short host. +func hostPrefix(host string) string { localDomainSuffix := ".svc." + network.GetClusterDomainName() if !strings.HasSuffix(host, localDomainSuffix) { - return exact(regexp.QuoteMeta(host) + portMatch) + return host } - prefix := regexp.QuoteMeta(strings.TrimSuffix(host, localDomainSuffix)) - clusterSuffix := regexp.QuoteMeta("." + network.GetClusterDomainName()) - svcSuffix := regexp.QuoteMeta(".svc") - return exact(prefix + optional(svcSuffix+optional(clusterSuffix)) + portMatch) -} - -func exact(regexp string) string { - return "^" + regexp + "$" + return strings.TrimSuffix(host, localDomainSuffix) } func optional(regexp string) string { return "(" + regexp + ")?" } + func getHosts(ia v1alpha1.IngressAccessor) sets.String { hosts := sets.NewString() for _, rule := range ia.GetSpec().Rules { diff --git a/pkg/reconciler/ingress/resources/virtual_service_test.go b/pkg/reconciler/ingress/resources/virtual_service_test.go index d385bf2b61bb..bb8ff346372e 100644 --- a/pkg/reconciler/ingress/resources/virtual_service_test.go +++ b/pkg/reconciler/ingress/resources/virtual_service_test.go @@ -227,7 +227,7 @@ func TestMakeMeshVirtualServiceSpec_CorrectRoutes(t *testing.T) { expected := []v1alpha3.HTTPRoute{{ Match: []v1alpha3.HTTPMatchRequest{{ URI: &istiov1alpha1.StringMatch{Regex: "^/pets/(.*?)?"}, - Authority: &istiov1alpha1.StringMatch{Regex: `^test-route\.test-ns(\.svc(\.cluster\.local)?)?(?::\d{1,5})?$`}, + Authority: &istiov1alpha1.StringMatch{Prefix: `test-route.test-ns`}, Gateways: []string{"mesh"}, }}, Route: []v1alpha3.HTTPRouteDestination{{ @@ -353,11 +353,11 @@ func TestMakeIngressVirtualServiceSpec_CorrectRoutes(t *testing.T) { expected := []v1alpha3.HTTPRoute{{ Match: []v1alpha3.HTTPMatchRequest{{ URI: &istiov1alpha1.StringMatch{Regex: "^/pets/(.*?)?"}, - Authority: &istiov1alpha1.StringMatch{Regex: `^domain\.com(?::\d{1,5})?$`}, + Authority: &istiov1alpha1.StringMatch{Prefix: `domain.com`}, Gateways: []string{"gateway.public"}, }, { URI: &istiov1alpha1.StringMatch{Regex: "^/pets/(.*?)?"}, - Authority: &istiov1alpha1.StringMatch{Regex: `^test-route\.test-ns(\.svc(\.cluster\.local)?)?(?::\d{1,5})?$`}, + Authority: &istiov1alpha1.StringMatch{Prefix: `test-route.test-ns`}, Gateways: []string{"gateway.private"}, }}, Route: []v1alpha3.HTTPRouteDestination{{ @@ -390,7 +390,7 @@ func TestMakeIngressVirtualServiceSpec_CorrectRoutes(t *testing.T) { }, { Match: []v1alpha3.HTTPMatchRequest{{ URI: &istiov1alpha1.StringMatch{Regex: "^/pets/(.*?)?"}, - Authority: &istiov1alpha1.StringMatch{Regex: `^v1\.domain\.com(?::\d{1,5})?$`}, + Authority: &istiov1alpha1.StringMatch{Prefix: `v1.domain.com`}, Gateways: []string{}, }}, Route: []v1alpha3.HTTPRouteDestination{{ @@ -443,10 +443,10 @@ func TestMakeVirtualServiceRoute_Vanilla(t *testing.T) { expected := v1alpha3.HTTPRoute{ Match: []v1alpha3.HTTPMatchRequest{{ Gateways: []string{"gateway-1"}, - Authority: &istiov1alpha1.StringMatch{Regex: `^a\.com(?::\d{1,5})?$`}, + Authority: &istiov1alpha1.StringMatch{Prefix: `a.com`}, }, { Gateways: []string{"gateway-1"}, - Authority: &istiov1alpha1.StringMatch{Regex: `^b\.org(?::\d{1,5})?$`}, + Authority: &istiov1alpha1.StringMatch{Prefix: `b.org`}, }}, Route: []v1alpha3.HTTPRouteDestination{{ Destination: v1alpha3.Destination{ @@ -495,7 +495,7 @@ func TestMakeVirtualServiceRoute_TwoTargets(t *testing.T) { expected := v1alpha3.HTTPRoute{ Match: []v1alpha3.HTTPMatchRequest{{ Gateways: []string{"knative-testing/gateway-1"}, - Authority: &istiov1alpha1.StringMatch{Regex: `^test\.org(?::\d{1,5})?$`}, + Authority: &istiov1alpha1.StringMatch{Prefix: `test.org`}, }}, Route: []v1alpha3.HTTPRouteDestination{{ Destination: v1alpha3.Destination{ diff --git a/pkg/reconciler/route/resources/service.go b/pkg/reconciler/route/resources/service.go index 5f7d5149636d..a8a7a0c13c6f 100644 --- a/pkg/reconciler/route/resources/service.go +++ b/pkg/reconciler/route/resources/service.go @@ -183,7 +183,11 @@ func makeServiceSpec(ingress netv1alpha1.IngressAccessor, isPrivate bool) (*core func GetDesiredServiceNames(ctx context.Context, route *v1alpha1.Route) (sets.String, error) { traffic := route.Spec.Traffic - names := sets.String{} + // We always want create the route with the service name. + // If the traffic stanza only contains revision targets, then + // this will not be added below, and as a consequence we'll create + // a public route to it. + names := sets.NewString(route.Name) for _, t := range traffic { serviceName, err := domains.HostnameFromTemplate(ctx, route.Name, t.Tag) diff --git a/pkg/reconciler/route/resources/service_test.go b/pkg/reconciler/route/resources/service_test.go index f039f6878dc6..2cfd47b7e815 100644 --- a/pkg/reconciler/route/resources/service_test.go +++ b/pkg/reconciler/route/resources/service_test.go @@ -381,36 +381,50 @@ func TestGetDesiredServiceNames(t *testing.T) { traffic RouteOption want sets.String wantErr bool - }{ - { - name: "no traffic defined", - }, - { - name: "only default traffic", - traffic: WithSpecTraffic(v1alpha1.TrafficTarget{TrafficTarget: v1.TrafficTarget{}}), - want: sets.NewString("myroute"), + }{{ + name: "no traffic defined", + want: sets.NewString("myroute"), + }, { + name: "only default traffic", + traffic: WithSpecTraffic(v1alpha1.TrafficTarget{TrafficTarget: v1.TrafficTarget{}}), + want: sets.NewString("myroute"), + }, { + name: "traffic targets with default and tags", + traffic: WithSpecTraffic(v1alpha1.TrafficTarget{ + TrafficTarget: v1.TrafficTarget{}, + }, v1alpha1.TrafficTarget{ + TrafficTarget: v1.TrafficTarget{ + Tag: "hello", + }, + }, v1alpha1.TrafficTarget{ + TrafficTarget: v1.TrafficTarget{ + Tag: "hello", + }, + }, v1alpha1.TrafficTarget{ + TrafficTarget: v1.TrafficTarget{ + Tag: "bye", + }, }, - { - name: "traffic targets with tag", - traffic: WithSpecTraffic(v1alpha1.TrafficTarget{ - TrafficTarget: v1.TrafficTarget{}, - }, v1alpha1.TrafficTarget{ - TrafficTarget: v1.TrafficTarget{ - Tag: "hello", - }, - }, v1alpha1.TrafficTarget{ - TrafficTarget: v1.TrafficTarget{ - Tag: "hello", - }, - }, v1alpha1.TrafficTarget{ - TrafficTarget: v1.TrafficTarget{ - Tag: "bye", - }, + ), + want: sets.NewString("myroute", "hello-myroute", "bye-myroute"), + }, { + name: "traffic targets with NO default and tags", + traffic: WithSpecTraffic(v1alpha1.TrafficTarget{ + TrafficTarget: v1.TrafficTarget{ + Tag: "hello", + }, + }, v1alpha1.TrafficTarget{ + TrafficTarget: v1.TrafficTarget{ + Tag: "hello", + }, + }, v1alpha1.TrafficTarget{ + TrafficTarget: v1.TrafficTarget{ + Tag: "bye", }, - ), - want: sets.NewString("myroute", "hello-myroute", "bye-myroute"), }, - } + ), + want: sets.NewString("myroute", "hello-myroute", "bye-myroute"), + }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { cfg := testConfig() diff --git a/test/e2e/service_to_service_test.go b/test/e2e/service_to_service_test.go index 601791d02a6b..36b8287ff584 100644 --- a/test/e2e/service_to_service_test.go +++ b/test/e2e/service_to_service_test.go @@ -28,14 +28,13 @@ import ( "knative.dev/pkg/test/ingress" "knative.dev/pkg/test/logstream" "knative.dev/pkg/test/spoof" + "knative.dev/serving/pkg/apis/autoscaling" + routeconfig "knative.dev/serving/pkg/reconciler/route/config" v1alph1testing "knative.dev/serving/pkg/testing/v1alpha1" "knative.dev/serving/test" v1a1test "knative.dev/serving/test/v1alpha1" corev1 "k8s.io/api/core/v1" - - "knative.dev/serving/pkg/apis/autoscaling" - routeconfig "knative.dev/serving/pkg/reconciler/route/config" ) const ( @@ -151,7 +150,8 @@ func testProxyToHelloworld(t *testing.T, clients *test.Clients, helloworldURL *u t.Fatalf("The httpproxy response '%s' is not equal to helloworld response '%s'.", string(response.Body), helloworldResponse) } - // As a final check (since we know they are both up), check that if we can access the helloworld app externally. + // As a final check (since we know they are both up), check that if we can + // (or cannot) access the helloworld app externally. response, err = sendRequest(t, clients, test.ServingFlags.ResolvableDomain, helloworldURL) if err != nil { if test.ServingFlags.ResolvableDomain { @@ -222,7 +222,7 @@ func TestServiceToServiceCall(t *testing.T) { t.Run(tc.name, func(t *testing.T) { cancel := logstream.Start(t) defer cancel() - testProxyToHelloworld(t, clients, helloworldURL, true, false) + testProxyToHelloworld(t, clients, helloworldURL, true /*inject*/, false /*accessible externally*/) }) } } @@ -256,7 +256,7 @@ func testSvcToSvcCallViaActivator(t *testing.T, clients *test.Clients, injectA b } // Send request to helloworld app via httpproxy service - testProxyToHelloworld(t, clients, resources.Route.Status.URL.URL(), injectA, false) + testProxyToHelloworld(t, clients, resources.Route.Status.URL.URL(), injectA, false /*accessible externally*/) } // Same test as TestServiceToServiceCall but before sending requests @@ -324,7 +324,7 @@ func TestCallToPublicService(t *testing.T) { t.Run(tc.name, func(t *testing.T) { cancel := logstream.Start(t) defer cancel() - testProxyToHelloworld(t, clients, tc.url, false, tc.accessibleExternally) + testProxyToHelloworld(t, clients, tc.url, false /*inject*/, tc.accessibleExternally) }) } }