Skip to content

Commit

Permalink
Correct route precedence logic for k8s ingress (#36847)
Browse files Browse the repository at this point in the history 
* correct route precedence logic for k8s ingress

* add unit tests

* fix lint
  • Loading branch information
@dddddai
dddddai authored Jan 20, 2022
1 parent b9cb218 commit 42a66dc
Show file tree
Hide file tree
Showing 8 changed files with 230 additions and 68 deletions.
58 changes: 45 additions & 13 deletions pilot/pkg/config/kube/ingress/conversion.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (

meshconfig "istio.io/api/mesh/v1alpha1"
networking "istio.io/api/networking/v1alpha3"
"istio.io/istio/pilot/pkg/features"
"istio.io/istio/pilot/pkg/serviceregistry/kube"
"istio.io/istio/pkg/config"
"istio.io/istio/pkg/config/constants"
Expand Down Expand Up @@ -150,13 +151,11 @@ func ConvertIngressVirtualService(ingress v1beta1.Ingress, domainSuffix string,
host = "*"
}
virtualService := &networking.VirtualService{
Hosts: []string{},
Hosts: []string{host},
Gateways: []string{fmt.Sprintf("%s/%s-%s-%s", ingressNamespace, ingress.Name, constants.IstioIngressGatewayName, ingress.Namespace)},
}

virtualService.Hosts = []string{host}

httpRoutes := make([]*networking.HTTPRoute, 0)
httpRoutes := make([]*networking.HTTPRoute, 0, len(rule.HTTP.Paths))
for _, httpPath := range rule.HTTP.Paths {
httpMatch := &networking.HTTPMatchRequest{}
if httpPath.PathType != nil {
Expand Down Expand Up @@ -216,19 +215,36 @@ func ConvertIngressVirtualService(ingress v1beta1.Ingress, domainSuffix string,
if f {
vs := old.Spec.(*networking.VirtualService)
vs.Http = append(vs.Http, httpRoutes...)
if features.LegacyIngressBehavior {
sort.SliceStable(vs.Http, func(i, j int) bool {
r1 := vs.Http[i].Match[0].GetUri()
r2 := vs.Http[j].Match[0].GetUri()
_, r1Ex := r1.GetMatchType().(*networking.StringMatch_Exact)
_, r2Ex := r2.GetMatchType().(*networking.StringMatch_Exact)
// TODO: default at the end
if r1Ex && !r2Ex {
return true
}
return false
})
}
} else {
ingressByHost[host] = &virtualServiceConfig
}

if !features.LegacyIngressBehavior {
// sort routes to meet ingress route precedence requirements
// see https://kubernetes.io/docs/concepts/services-networking/ingress/#multiple-matches
vs := ingressByHost[host].Spec.(*networking.VirtualService)
sort.SliceStable(vs.Http, func(i, j int) bool {
r1 := vs.Http[i].Match[0].GetUri()
r2 := vs.Http[j].Match[0].GetUri()
_, r1Ex := r1.GetMatchType().(*networking.StringMatch_Exact)
_, r2Ex := r2.GetMatchType().(*networking.StringMatch_Exact)
r1Len, r1Ex := getMatchURILength(vs.Http[i].Match[0])
r2Len, r2Ex := getMatchURILength(vs.Http[j].Match[0])
// TODO: default at the end
if r1Ex && !r2Ex {
return true
if r1Len == r2Len {
return r1Ex && !r2Ex
}
return false
return r1Len > r2Len
})
} else {
ingressByHost[host] = &virtualServiceConfig
}
}

Expand All @@ -240,6 +256,22 @@ func ConvertIngressVirtualService(ingress v1beta1.Ingress, domainSuffix string,
}
}

// getMatchURILength returns the length of matching path, and whether the match type is EXACT
func getMatchURILength(match *networking.HTTPMatchRequest) (length int, exact bool) {
uri := match.GetUri()
switch uri.GetMatchType().(type) {
case *networking.StringMatch_Exact:
return len(uri.GetExact()), true
case *networking.StringMatch_Prefix:
return len(uri.GetPrefix()), false
case *networking.StringMatch_Regex:
// trim the regex suffix
return len(uri.GetRegex()) - len(prefixMatchRegex), false
}
// should not happen
return -1, false
}

func ingressBackendToHTTPRoute(backend *v1beta1.IngressBackend, namespace string, domainSuffix string,
serviceLister listerv1.ServiceLister) *networking.HTTPRoute {
if backend == nil {
Expand Down
40 changes: 39 additions & 1 deletion pilot/pkg/config/kube/ingress/conversion_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,10 @@ func readConfig(t *testing.T, filename string) ([]runtime.Object, error) {
func TestConversion(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

prefix := v1beta1.PathTypePrefix
exact := v1beta1.PathTypeExact

ingress := v1beta1.Ingress{
ObjectMeta: metaV1.ObjectMeta{
Namespace: "mock", // goes into backend full name
Expand All @@ -153,6 +157,14 @@ func TestConversion(t *testing.T) {
ServicePort: intstr.IntOrString{IntVal: 8000},
},
},
{
Path: "/test/foo",
PathType: &prefix,
Backend: v1beta1.IngressBackend{
ServiceName: "foo",
ServicePort: intstr.IntOrString{IntVal: 8000},
},
},
},
},
},
Expand Down Expand Up @@ -210,6 +222,22 @@ func TestConversion(t *testing.T) {
ServicePort: intstr.IntOrString{IntVal: 8000},
},
},
{
Path: "/test/foo/bar",
PathType: &prefix,
Backend: v1beta1.IngressBackend{
ServiceName: "foo",
ServicePort: intstr.IntOrString{IntVal: 8000},
},
},
{
Path: "/test/foo/bar",
PathType: &exact,
Backend: v1beta1.IngressBackend{
ServiceName: "foo",
ServicePort: intstr.IntOrString{IntVal: 8000},
},
},
},
},
},
Expand All @@ -226,16 +254,26 @@ func TestConversion(t *testing.T) {
t.Error("VirtualServices, expected 3 got ", len(cfgs))
}

expectedLength := [5]int{13, 13, 9, 6, 5}
expectedExact := [5]bool{true, false, false, true, true}

for n, cfg := range cfgs {
vs := cfg.Spec.(*networking.VirtualService)

if n == "my.host.com" {
if vs.Hosts[0] != "my.host.com" {
t.Error("Unexpected host", vs)
}
if len(vs.Http) != 2 {
if len(vs.Http) != 5 {
t.Error("Unexpected rules", vs.Http)
}
for i, route := range vs.Http {
length, exact := getMatchURILength(route.Match[0])
if length != expectedLength[i] || exact != expectedExact[i] {
t.Errorf("Unexpected rule at idx:%d, want {length:%d, exact:%v}, got {length:%d, exact: %v}",
i, expectedLength[i], expectedExact[i], length, exact)
}
}
}
}
}
Expand Down
40 changes: 20 additions & 20 deletions pilot/pkg/config/kube/ingress/testdata/simple.yaml.golden
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,92 +29,92 @@ spec:
http:
- match:
- uri:
exact: /path
exact: /sub/path/
route:
- destination:
host: service1.ns.svc.mydomain
port:
number: 4200
number: 4202
weight: 100
- match:
- uri:
exact: /sub/path
exact: /sub/path/
route:
- destination:
host: service1.ns.svc.mydomain
port:
number: 4201
number: 4207
weight: 100
- match:
- uri:
exact: /sub/path/
exact: /sub/path
route:
- destination:
host: service1.ns.svc.mydomain
port:
number: 4202
number: 4201
weight: 100
- match:
- uri:
prefix: /regex1
exact: /sub/path
route:
- destination:
host: service1.ns.svc.mydomain
port:
number: 4203
number: 4206
weight: 100
- match:
- uri:
exact: /regex2*
regex: /sub/path((\/).*)?
route:
- destination:
host: service1.ns.svc.mydomain
port:
number: 4204
number: 4208
weight: 100
- match:
- uri:
prefix: /regex3
regex: /sub/path((\/).*)?
route:
- destination:
host: service1.ns.svc.mydomain
port:
number: 4205
number: 4209
weight: 100
- match:
- uri:
exact: /sub/path
exact: /regex2*
route:
- destination:
host: service1.ns.svc.mydomain
port:
number: 4206
number: 4204
weight: 100
- match:
- uri:
exact: /sub/path/
prefix: /regex1
route:
- destination:
host: service1.ns.svc.mydomain
port:
number: 4207
number: 4203
weight: 100
- match:
- uri:
regex: /sub/path((\/).*)?
prefix: /regex3
route:
- destination:
host: service1.ns.svc.mydomain
port:
number: 4208
number: 4205
weight: 100
- match:
- uri:
regex: /sub/path((\/).*)?
exact: /path
route:
- destination:
host: service1.ns.svc.mydomain
port:
number: 4209
number: 4200
weight: 100
---
58 changes: 45 additions & 13 deletions pilot/pkg/config/kube/ingressv1/conversion.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (

meshconfig "istio.io/api/mesh/v1alpha1"
networking "istio.io/api/networking/v1alpha3"
"istio.io/istio/pilot/pkg/features"
"istio.io/istio/pilot/pkg/serviceregistry/kube"
"istio.io/istio/pkg/config"
"istio.io/istio/pkg/config/constants"
Expand Down Expand Up @@ -150,13 +151,11 @@ func ConvertIngressVirtualService(ingress knetworking.Ingress, domainSuffix stri
host = "*"
}
virtualService := &networking.VirtualService{
Hosts: []string{},
Hosts: []string{host},
Gateways: []string{fmt.Sprintf("%s/%s-%s-%s", ingressNamespace, ingress.Name, constants.IstioIngressGatewayName, ingress.Namespace)},
}

virtualService.Hosts = []string{host}

httpRoutes := make([]*networking.HTTPRoute, 0)
httpRoutes := make([]*networking.HTTPRoute, 0, len(rule.HTTP.Paths))
for _, httpPath := range rule.HTTP.Paths {
httpMatch := &networking.HTTPMatchRequest{}
if httpPath.PathType != nil {
Expand Down Expand Up @@ -216,19 +215,36 @@ func ConvertIngressVirtualService(ingress knetworking.Ingress, domainSuffix stri
if f {
vs := old.Spec.(*networking.VirtualService)
vs.Http = append(vs.Http, httpRoutes...)
if features.LegacyIngressBehavior {
sort.SliceStable(vs.Http, func(i, j int) bool {
r1 := vs.Http[i].Match[0].GetUri()
r2 := vs.Http[j].Match[0].GetUri()
_, r1Ex := r1.GetMatchType().(*networking.StringMatch_Exact)
_, r2Ex := r2.GetMatchType().(*networking.StringMatch_Exact)
// TODO: default at the end
if r1Ex && !r2Ex {
return true
}
return false
})
}
} else {
ingressByHost[host] = &virtualServiceConfig
}

if !features.LegacyIngressBehavior {
// sort routes to meet ingress route precedence requirements
// see https://kubernetes.io/docs/concepts/services-networking/ingress/#multiple-matches
vs := ingressByHost[host].Spec.(*networking.VirtualService)
sort.SliceStable(vs.Http, func(i, j int) bool {
r1 := vs.Http[i].Match[0].GetUri()
r2 := vs.Http[j].Match[0].GetUri()
_, r1Ex := r1.GetMatchType().(*networking.StringMatch_Exact)
_, r2Ex := r2.GetMatchType().(*networking.StringMatch_Exact)
r1Len, r1Ex := getMatchURILength(vs.Http[i].Match[0])
r2Len, r2Ex := getMatchURILength(vs.Http[j].Match[0])
// TODO: default at the end
if r1Ex && !r2Ex {
return true
if r1Len == r2Len {
return r1Ex && !r2Ex
}
return false
return r1Len > r2Len
})
} else {
ingressByHost[host] = &virtualServiceConfig
}
}

Expand All @@ -240,6 +256,22 @@ func ConvertIngressVirtualService(ingress knetworking.Ingress, domainSuffix stri
}
}

// getMatchURILength returns the length of matching path, and whether the match type is EXACT
func getMatchURILength(match *networking.HTTPMatchRequest) (length int, exact bool) {
uri := match.GetUri()
switch uri.GetMatchType().(type) {
case *networking.StringMatch_Exact:
return len(uri.GetExact()), true
case *networking.StringMatch_Prefix:
return len(uri.GetPrefix()), false
case *networking.StringMatch_Regex:
// trim the regex suffix
return len(uri.GetRegex()) - len(prefixMatchRegex), false
}
// should not happen
return -1, false
}

func ingressBackendToHTTPRoute(backend *knetworking.IngressBackend, namespace string, domainSuffix string,
serviceLister listerv1.ServiceLister) *networking.HTTPRoute {
if backend == nil {
Expand Down
Loading

0 comments on commit 42a66dc

Please sign in to comment.