Skipper: Nondeterministic route matching ?

[davydkov]

I have an ingress. Lets say this simplified:

---
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: my-ingress
  annotations:
spec:
  rules:     
  - host: "myhost.com"
    http:
      paths:
      - path: "/path"
         backend:
          serviceName: upstream1
          servicePort: 80
      - backend:
          serviceName: upstream2
          servicePort: 80

I have three nodes in cluster.
When I run curl -H "host: myhost.com" localhost:9999/path/foo/bar/long/path/1 locally on each node I got different results, for instance two times expected result from upstream1, and on another node I got unexpected result from upstream2
But If I do the same request directly to clusterIP of the service , I got expected result on each node. This narrows down the source of my issue to Skipper.

Version is 0.10.13

[hjacobs]

This looks like a bug, but can you first check whether skipper correctly updated all routes on both nodes? I could suspect that one skipper instance (on of the nodes) has outdated routes.

[szuecs]

Hi!

We had a similar case yesterday. The current workaround is to use 2 ingress and to specify also a Predicate annotation on the path ingress.

---
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: my-ingress-path
  annotations:
    zalando.org/skipper-predicate: PathSubtree("/path")
spec:
  rules:     
  - host: "myhost.com"
    http:
      paths:
      - path: "/path"
         backend:
          serviceName: upstream1
          servicePort: 80

---
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: my-ingress-root
  annotations:
spec:
  rules:     
  - host: "myhost.com"
    http:
      paths:
      - backend:
          serviceName: upstream2
          servicePort: 80

The problem is that both routes have the same precedence and if you use a predicate, then you will have more priority to the other routehaving no predicate.

[szuecs]

@hjacobs this was also found internally, yesterday. We had a discussion and I think we agreed on this is a bug for the use in kubernetes at least. We will defintively fix it.

[davydkov]

skipper correctly updated all routes on both nodes? I could suspect that one skipper instance (on of the nodes) has outdated routes.

Yes, localhost:9911/routes shows all the nodes are aware of these routes.
I think the problem appears, when you have:

path: "/some/path/longer"

and

path: "/some/path"

Then skipper can't pick one deterministically

fyi, traefik and nginx sort by path length in such cases.

[ese]

This is related with #360 and #304

[szuecs]

I talked with @aryszka and we thought about a solution that would add an annotation to choose the matching algorithm.

1. Skipper subtree
2. Skipper (Go) regexp
3. Kubernetes regexp (default)

In my honest opinion I think for users it makes in general more sense to choose 1. as default, but it would mean that we are not compatible to other ingress controllers which would be a risk. We do not want to take the risk so 3. is the default.
We can provide a chosen default via a flag which defaults to 3., such that users could opt in to use 1. or 2.
If you have thoughts or an opinion, please add your comments.

[mikkeloscar]

but it would mean that we are not compatible to other ingress controllers which would be a risk

@szuecs can you clarify what you mean by this? Do other ingress controllers treat the path value as regex? Because from the docs (\o/) I would assume it should just match on the string, which is also what we effectively do currently: https://github.com/zalando/skipper/blob/452f8ae028fc480ad9192688f7ade2b3d5f90c3a/dataclients/kubernetes/kube.go#L549. Therefore option 1 sounds like the right solution to me, unless I'm misunderstanding how the path value should be interpreted.

[ese]

Because from the docs (\o/) I would assume it should just match on the string, which is also what we effectively do currently:

According with the spec of the ingress object: Path is an extended POSIX regex as defined by IEEE Std 1003.1 https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.10/#httpingresspath-v1beta1-extensions

[aryszka]

@ese that's my understanding as well.

@mikkeloscar let me try to answer your question at least in part :)

(careful with kubedoc links, the page may jump after loaded due to web design) here's an alternative link to the same specification: https://kubernetes.io/docs/reference/federation/extensions/v1beta1/definitions/#_v1beta1_httpingresspath

there's one lucky and one challenging thing about this spec. The lucky one is that it is mostly compatible with the Go regexp implementation, the challenging thing though is the one saying "Paths must begin with a /". Due to this, we considered this as the # 3 case listed above in @szuecs proposal.

with the # 2 case, we would also accept a regexp like ^/, basically that's all about # 2

the case # 1 is the right solution, I agree with you on so many levels. This is what most users expect, and also the runtime challenges are much more lax. It's just not compatible with the specification, that's why we are proposing the explicit opt-in.

About the opt-in, notice that with the combination of the per-ingress annotation and the startup option, it becomes easy to control this behaviour for both the cluster operator and the tenant.