Optional path consuming for the ingress identifier

[esbie]

Similar to the path identifier, I suspect most folks will want the l5d ingress controller consume the beginning of the path used for routing.

One way to do this would be with ingress resource annotations, something like

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: my-ingress
  annotations:
    kubernetes.io/ingress.class: "linkerd"
    linkerd/consume.segments: 2

Another would be in the linkerd config:

routers:
- protocol: http
  identifier:
    kind: io.l5d.http.ingress
    segments: 2

Or we could support both? I'm leaning towards ingress resource annotations since it would be the most flexible for users.

[ppwfx]

Isn't the path identifier intended to be used for routing? At least that's how the mainstream implementations are behaving.

E.g. given the configuration below, /foo/yo would be routed to the service listed in -path: /foo.

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: test
spec:
  rules:
  - host: foo.bar.com
    http:
      paths:
      - path: /foo
        backend:
          serviceName: s1
          servicePort: 80

I actually don't see any need for providing additional vendor specific configuration. Please correct me if I'm wrong.. :)

[esbie]

Hi @21stio, in your example yes /foo/yo is routed to service s1 on port 80. The issue that's being referenced here is this: forwarded requests will still have a path of /foo/yo. This is surprising to a number of clients who expect it to be /yo.

[ppwfx]

Alright, got it. Makes total sense :)

[FuzzOli87]

I completely agree with this being an annotation similar to what the "rewrite" annotation does on NGINX controller configurations.

It could allow us to namespace apps by path which could help in directing to different versions etc.

[pawelprazak]

We are currently using Traefik and we need this feature, so we'd like to implement it.

Any pointers regarding API and implementation are very much welcome.

I propose the simplest implementation, with a similar behavior to Traefik's pathprefixstrip:

Match request prefix path and strip off the path prefix prior to forwarding the request to the backend

In our case, adding linkerd.io/pathprefixstrip: true annotation on Ingress would match any path that starts with the defined prefix and strip this prefix.

Example, given ingress definition:

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: my-ingress
  annotations:
    kubernetes.io/ingress.class: "linkerd"
    linkerd.io/pathprefixstrip: true
spec:
  rules:
  - http:
      paths:
      - path: /hello
        backend:
          serviceName: hello
          servicePort: 7777

and a request for path /hello/world, service hello will only get /world.

As an additional or alternative configuration method we could use:

...
  identifier:
    kind: io.l5d.ingress
    pathprefixstrip: true
...

[esbie]

Great @pawelprazak ! I'm happy to help. Currently the ingress annotations are read here.

The linkerd ingress identifier matches paths based on regex, not path prefix (we took this idea from k8s type definitions, even though most other ingress controllers don't seem to behave this way). So without the concept of a prefix, we don't know how much of the path to strip and how much of the path to keep.

One solution is to keep the regex and specify number of segments to consume, like the path identifier does. Another is to change the path matching behavior to be prefix-based and then use the definition you've described.

I'm not thrilled with either one of these options honestly 😅 , but either one should do the job. Folks should chime in if they have strong opinions about it.

[pawelprazak]

Thank you for your thoughts, esp. for pointing out the k8s API, I was not aware it explicitly allows regexps.

Our current quick and dirty implementation just strips the request path using the ingress path as the prefix. As far as I understand what you wrote, it would brake the existing behavior.

IMHO the segments consumption, like in path identifier, is not very useful, it fails for a simple use case we have, e.g.:

/v1/hello
/v2/world
/heathcheck

You can either match the first 2 path prefixes or the last one, not all 3.

A third option would be to use capturing groups, so with a flag use-capturing-groups e.g.:

/test/path/                    and  /test/path/qwer/asdf/1234  ->  '' (empty - edge case)
/test/path/(.+)                and  /test/path/qwer/asdf/1234  ->  /qwer/asdf/1234
/test/path/(.+)/asdf/([0-9]+)  and  /test/path/qwer/asdf/1234  ->  /qwer/1234
/test/path/(.+)/(asdf)/(.+)    and  /test/path/qwer/asdf/1234  ->  /qwer/asdf/1234
/test/.+/(.+)/(asdf)/(.+)      and  /test/path/qwer/asdf/1234  ->  /qwer/asdf/1234

We could also skip the flag and just leave the path as is if there is no capture group hits, but that feel error prone.

[esbie]

Good feedback!
I like the use-capturing-groups option. Since it's a backwards-compatible change, we could release it as early as v1.0.3 instead of waiting until v1.1.0. And it's not as limiting as consume segments.

[pawelprazak]

I've created a PR with an initial version to get some feedback early.

TODO:

• real tests on Kubernetes
• does h2.IngressIdentifier also needs to be changed? If so, how to deals with the lack of uri field
• update docs

[esbie]

Cool! Convened w/ @adleong and it seems like the h2 requests's :path header should be changed since there's no uri