istio / old_issues_repo

Deprecated issue-tracking repo, please post new issues or feature requests to istio/istio instead.
37 stars 9 forks source link

gRPC ingress routing #87

Open rfevang opened 7 years ago

rfevang commented 7 years ago

Is this a BUG or FEATURE REQUEST?:

Bug

Did you review existing epics or issues to identify if this already being worked on? (please try to add the correct labels and epics):

Tried finding matching issues, not sure where to look for epics.

Bug: Y

What Version of Istio and Kubernetes are you using, where did you get Istio from, Installation details

istioctl version 0.2.6
Installed using curl -L https://git.io/getLatestIstio | ISTIO_VERSION="0.2.6" sh -

kubectl version 1.7.5
Google Container Engine

Is Istio Auth enabled or not ? Yes (used istio-auth.yaml)

What happened:

404 when trying to call gRPC functions using ingress with hostname or path rules (google groups post)

What you expected to happen:

Ingress rules should work.

How to reproduce it:

Set up gRPC server, then make an ingress for contacting it:

Attempt with hostname:

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: my-ingress
  annotations:
    kubernetes.io/ingress.class: istio
spec:
  rules:
  - host: first.api.mycompany.com
    http:
      paths:
      - backend:
          serviceName: my-first-service
          servicePort: grpc

or path:

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: my-ingress
  annotations:
    kubernetes.io/ingress.class: istio
spec:
  rules:
  - http:
      paths:
      - path: /grpc.reflection.v1alpha.ServerReflection/*
        backend:
          serviceName: my-first-service
          servicePort: grpc

Using an ingress with no rules works, however that doesn't allow for contacting more than one service from outside the cluster:

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: my-ingress
  annotations:
    kubernetes.io/ingress.class: istio
spec:
  rules:
  - http:
      paths:
      - backend:
          serviceName: my-first-service
          servicePort: grpc

Feature Request: N

Describe the feature:

rshriram commented 7 years ago

FYI ( @louiscryan @smawson ).. the ingress woes continue.

markeijsermans commented 7 years ago

Can confirm this bug with a recently compiled grpc-go 1.7.0-dev. Tried all variations of hostname and path routing.

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  annotations:
    kubernetes.io/ingress.class: istio
  name: grpc-test-server
  namespace: default
spec:
  rules:
  - host: grpc-test-server.***.com  # tried with & without hostname + path combinations
    http:
      paths:
      - backend:
          serviceName: grpc-test-server
          servicePort: grpc
        path: "/*"                            # produces 404
        #path: "/.*"                          # produces 404
        #path: "/grpctest.*"                  # produces no logs
        #path: "/grpctest.Pinger/SinglePing"  # produces no logs

The endpoint /grpctest.Pinger/SinglePing is a unary rpc request. Istio ingress 404's with logs looking something like:

[2017-10-04T21:42:42.830Z] "POST /grpctest.Pinger/SinglePing HTTP/2" 404 NR 0 0 0 - "10.240.13.0" "grpc-go/1.7.0-dev" "31b3137b-31af-94bd-85cc-0d1b8e2db9b4" "grpc-test-server.***.com:80" "-"

What I have noticed is that the port is suffixed on dest for grpc requests in the log. While a regular curl to another http service does not. I'm assuming this is simply an artifact of http/2 vs http/1.1:

curl debug.***.com

[2017-10-04T22:04:41.387Z] "GET / HTTP/1.1" 200 - 0 2 5 1 "10.240.10.0" "curl/7.54.0" "42004ffa-db97-9c22-b0f4-a208184b1d97" "debug.***.com" "10.240.16.8:8080"
rshriram commented 7 years ago

Btw, can you try with /grpc.reflection.v1alpha.ServerReflection/. (notice the .) and can you add a :80 to the hostname?

markeijsermans commented 7 years ago
rshriram commented 7 years ago

Ooohh.. more ingress pains with ingress spec.. @louiscryan / @smawson / @linsun

linsun commented 7 years ago

Could you try without dot in the path? e.g. /ServerReflection/.* see https://github.com/istio/pilot/issues/1389 with some explanation of why the ask.

Also, can you confirm you hit this issue not only with grpc but also with http? cc @kyessenov

rfevang commented 7 years ago

istio/pilot#1401 fixed the host routing issue. I still can't get path based routing to work though, is that expected to work?

markeijsermans commented 7 years ago

I get same results for istio/pilot#1401 fix. Host routes works. There's no way to escape a dot in the path

---
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: grpc-test-server
  namespace: default
  annotations:
    kubernetes.io/ingress.class: istio
spec:
  rules:
  - host: "grpc-test-server.***.com"
    http:
      paths:
      - backend:
          serviceName: grpc-test-server
          servicePort: grpc
        ##--no path--                                         # ok
        #path: "/.*"                                          # ok
        #path: "/grpctest.*"                                  # ok
        #path: "/*"                                           # 404
        #path: "/grpctest.Pinger/SinglePing"                  # 404
        #path: "/grpc.reflection.v1alpha.ServerReflection/.*" # 404
        #path: "/grpc.reflection.v1alpha.ServerReflection/*"  # 404
        #path: "/ServerReflection/*"                          # 404
        #path: "/grpctest\\.Pinger/SinglePing"                # 404
        #path: "/grpctest[.]Pinger/SinglePing"                # 404
        #path: "/grpctest.*Pinger.*"                          # 404
rshriram commented 7 years ago

do the new steps defined here help: https://istio.io/docs/tasks/traffic-management/ingress.html ? @markeijsermans @rfevang @maruina specifically for the dots in path

maruina commented 7 years ago

I'll have a look as soon as possible

rfevang commented 7 years ago

I don't understand how to use those steps to do path differentiation. It looks like they say to allow all paths, and then add route rules to further differentiate? But as far as I can tell you need to specify a destination service in the ingress, and name that destination in the route rules. The problem I'm trying to solve is that different paths need to go to different services.

I can get prefix differentiation up to the first dot to work, but I'm still failing to get anything after that to match. Up to the first dot is not useful, as several services have the same namespace/namespace prefix.

rshriram commented 7 years ago

Create a dummy ingress spec with multiple rules like

-path: /. backend: Service: foo Port: 123 -path: /. backend: Service: bar Port: 123

Define similar “deny all” route rules for each of these destinations as shown in the docs. Then start exposing individual paths using exact match format (see rules references) where you can have dots.

I am sorry this is convoluted because the ingress spec is ambiguous in specification of paths (path can be exact, prefix, regex). Envoy uses ecmascript. And go uses a different regex engine. Detecting whether a given string is ecmaregex or not involves using a different regex parser in our code, and that complexity seems unjustified given the limited use we get. Compounding this, there are no well tested cpp compliant ecmascript regex parsers in Go.

On Mon, Oct 9, 2017 at 5:44 AM Rune Fevang notifications@github.com wrote:

I don't understand how to use those steps to do path differentiation. It looks like they say to allow all paths, and then add route rules to further differentiate? But as far as I can tell you need to specify a destination service in the ingress, and name that destination in the route rules. The problem I'm trying to solve is that different paths need to go to different services.

I can get prefix differentiation up to the first dot to work, but I'm still failing to get anything after that to match. Up to the first dot is not useful, as several services have the same namespace/namespace prefix.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/istio/issues/issues/87#issuecomment-335110179, or mute the thread https://github.com/notifications/unsubscribe-auth/AH0qd9vZDD5ikuhvC5i29uDWjgc3fYd4ks5sqer0gaJpZM4PuIKw .

rfevang commented 7 years ago

Is there no way to match a period in go regex? The issue isn't so much that the syntax is different than in other ingresses (though that's a problem as well), but more that I can't figure out a single way of matching a period.

I'll try with the dummy ingress spec, though it looks a bit weird to me to have two rules that map the same thing, but to different services. Won't just one of them apply (latest wins or similar)?

rshriram commented 7 years ago

The thing is, code can't distinguish if /fo.b.r represents a valid exact path /fo.b.r or a regular expression that could match /foobar .

rfevang commented 7 years ago

Right, but right now it matches neither (or at least it doesn't match /fo.b.r). If it was a normal regular expression it should still match as . is a character. Regex interpretation would also be sufficient if you could specify that it should match dots, say by doing /fo[.]b[.]r or /fo\.b\.r, but nothing works.

rshriram commented 7 years ago

:( .. Can you try the route rule format and let me know if that works? In the mean time, we will see what we can do about the go regexp vs ecmascript thing..

rfevang commented 7 years ago

Trying the route rule I get name or service is mandatory for a service reference when trying to set up the deny-rule. If I remove the source part of the rule it doesn't error any more, but neither does it result in any 403's (everything just goes through still).

I'm still not clear on why a regex interpretation is a problem though, as /a.b/c* matches /a.b/c in both go and ecmascript syntax (as well as other things like /azb/c, but that's OK).

rshriram commented 7 years ago

Is this with the istio release (0.2.7) or from the master branch (istio/istio) ? We are using this in our e2e tests (https://github.com/istio/istio/blob/master/samples/bookinfo/kube/route-rule-all-v1.yaml#L14) ..

rfevang commented 7 years ago

It's with istio 0.2.6. I've been playing around with it for a few hours now, and I finally got something to visibly use the route rule by changing the destination:

route:
- weight: 100
  destination:
    name: my-other-service

Looks like it was the httpFault part that didn't work, requests always go through normally.

rfevang commented 7 years ago

OK, got things to work with the following workaround:

  1. Create a dummy service not actually pointing to anything:
    apiVersion: v1
    kind: Service
    metadata:
    name: ingress-dummy-service
    spec:
    ports:
    - name: grpc
    port: 1337
  2. Create catch-all ingress pointing to the dummy service:
    apiVersion: extensions/v1beta1
    kind: Ingress
    metadata:
    name: all-istio-ingress
    annotations:
    kubernetes.io/ingress.class: istio
    spec:
    rules:
    - http:
      paths:
      - backend:
          serviceName: ingress-dummy-service
          servicePort: grpc
  3. Create a RouteRule for each path, redirecting to the actual service to be used:
    apiVersion: config.istio.io/v1alpha2
    kind: RouteRule
    metadata:
    name: first-service-route
    spec:
    destination:
    name: ingress-dummy-service
    match:
    request:
      headers:
        uri:
          prefix: "/first.service"
    precedence: 1
    route:
    - weight: 100
    destination:
      name: first-service
    ---
    apiVersion: config.istio.io/v1alpha2
    kind: RouteRule
    metadata:
    name: second-service-route
    spec:
    destination:
    name: ingress-dummy-service
    match:
    request:
      headers:
        uri:
          prefix: "/second.service"
    precedence: 1
    route:
    - weight: 100
    destination:
      name: second-service   

This is a little ugly, so hopefully the prefix/regex issue can be fixed in future version, but at least this seems like it should be enough for me to get something working.

Thanks a lot for your help!

rshriram commented 7 years ago

Glad that this worked. And yes, we are trying to fix the prefix/regex issue. and in case you are interested, would you mind contributing this to the ingress document? (https://github.com/istio/istio.github.io/blob/master/_docs/tasks/traffic-management/ingress.md) .. The dummy service seems like a more intuitive way of achieving the same behavior.

linsun commented 7 years ago

@rfevang thanks, glad to hear it worked for you, would definition welcome your PR on the docs.

Are you using 0.2.7 or master when you get it working?

rfevang commented 7 years ago

I was using 0.2.6, with the pilot image from istio/pilot#1401.

Submitted https://github.com/istio/istio.github.io/pull/656

remster commented 7 years ago

Hitting the same issue, but i can't work out how to use paths (prefixes) with grpc client (i.e.: how do i communicate: prefix: "/second.service" from the client side?). @rfevang can u share your client code (or relevant bits of it at least)

rfevang commented 7 years ago

Nothing special required client side, the client uses the service namespace, name and method name as the path. So if you have something like the following rpc definition:

package second.service;
service MyService {
  rpc MyMethod (MyRequestObj) returns (MyResponseObj);
}

Then the grpc client will use second.service.MyService/MyMethod as the path when calling MyMethod.

remster commented 7 years ago

Thanks!. I've got it working as per the workaround, but indeed, it's a workaround as i happen to have two services exposing the same interface and need the ability to mux between them based on hostname. @rshriram @linsun is there an ETA for the regex/hostname parsing?

rlguarino commented 6 years ago

The documentation added in https://github.com/istio/istio.github.io/pull/656 seems to have disappeared in a recent update to the site. Was that intentional?

rshriram commented 6 years ago

We don;t need the workarounds anymore. We fixed the bugs that prevented gRPC requests with dots in the path from being proxied.

rlguarino commented 6 years ago

Really?! That's awesome news. But I'm not having luck specifying a path. I'm getting 404 from Ingress when I use a path but it works if I remove the path entirely. Do I need to escape the . somehow?

I'm using Istio-0.4

rlguarino commented 6 years ago

Nevermind, that was my fault. It's working as intended!

Thanks for the great work.

--RG

On Thu, Jan 4, 2018 at 2:37 PM Shriram Rajagopalan notifications@github.com wrote:

We don;t need the workarounds anymore. We fixed the bugs that prevented gRPC requests with dots in the path from being proxied.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/istio/issues/issues/87#issuecomment-355419168, or mute the thread https://github.com/notifications/unsubscribe-auth/ABQralrob437dQrmQohT-UFyFvMco0A-ks5tHVKYgaJpZM4PuIKw .

karlmutch commented 6 years ago

FAQ Still indicates using dots is not supported, https://istio.io/help/faq/traffic-management.html. I also have not yet managed to make it work as well with 0.6.0

soleil0-0 commented 6 years ago

I test in istio 0.7.1. host based routing for grpc doesn't work right now. But routing by path is ok.

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: build-dependency-x
  annotations:
    kubernetes.io/ingress.class: istio
spec:
  rules:
  - http:
      paths:
      - path: /first.service/.*
        backend:
          serviceName: firstservice
          servicePort: grpc
      - path: /second.service/.*
        backend:
          serviceName: secondservice
          servicePort: grpc
louiscryan commented 6 years ago

Can you retest with 0.8 ?

xiajidexue commented 6 years ago

@qband, I am a new comer to Istio, I have the same need for Istio supporting gRPC, but failed, can you paste a full example for the configurations? or just email me, my Mail is 1214682008@qq.com, thanks a lot for your quick response.

xiajidexue commented 6 years ago

the bug I filed listed below: https://github.com/istio/istio/issues/6769

soleil0-0 commented 6 years ago

@xiajidexue I have posted the example configuration about path based routing for grpc. Grpc is based on http/2,so it can be routed by path or host.

By the way, I haven't test whether istio(version 0.8) ingress support host based routing for grpc or not .