Skip to content
This repository has been archived by the owner on Dec 1, 2022. It is now read-only.

Commit

Permalink
Cherry picks from 0.10.1 (#361)
Browse files Browse the repository at this point in the history 
* squash (knative#6174) (knative#6175)

* Use prefix instead of regex for authority match in virtualservice (knative#6088) (knative#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 knative#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
  • Loading branch information
@markusthoemmes @openshift-merge-robot
markusthoemmes authored and openshift-merge-robot committed Dec 10, 2019
1 parent 1453e95 commit 4cf806b
Show file tree
Hide file tree
Showing 5 changed files with 68 additions and 59 deletions.
25 changes: 8 additions & 17 deletions pkg/reconciler/ingress/resources/virtual_service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"crypto/md5"
"encoding/json"
"fmt"
"regexp"
"strings"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -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
Expand All @@ -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:<any port>
// for clusterLocalHost, we will also match the prefixes.
func hostRegExp(host string) string {
// hostPrefix returns an host to match either host or host:<any port>.
// For clusterLocalHost, it trims .svc.<local domain> 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 {
Expand Down
14 changes: 7 additions & 7 deletions pkg/reconciler/ingress/resources/virtual_service_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{{
Expand Down Expand Up @@ -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{{
Expand Down Expand Up @@ -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{{
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand Down
6 changes: 5 additions & 1 deletion pkg/reconciler/route/resources/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
68 changes: 41 additions & 27 deletions pkg/reconciler/route/resources/service_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
14 changes: 7 additions & 7 deletions test/e2e/service_to_service_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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*/)
})
}
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
})
}
}

0 comments on commit 4cf806b

Please sign in to comment.