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

Pilot is dropping route rules for ingress in 0.7.1 #296

Closed dcberg closed 6 years ago

dcberg commented 6 years ago

Is this a BUG or FEATURE REQUEST?:

Did you review https://istio.io/help/ and existing issues to identify if this is already solved or being worked on?:

Bug: Y

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

istioctl version 0.7.1
kubectl version 1.9.1

Is Istio Auth enabled or not ? Did you install the stable istio.yaml, istio-auth.yaml.... or if using the Helm chart please provide full command line input. Y What happened:

Istio pilot is dropping route rules for ingress in release 0.7.1.

What you expected to happen:

I expect that istio-ingress will preserve all route rules in precedence order as is done for sidecars.

How to reproduce it:

Follow the steps in exercise 10 of the following workshop - https://github.com/retroryan/istio-workshop/tree/master/exercise-10

When you get the step to apply the route-rule-user-mobile.yaml then a curl using -A mobile will work but if you remove -A mobile then the curl command returns a 404 error. It is expected that the weighted 80/20 would be used. Looking at both the pilot and istio-ingress configurations I can see that the lower precedence route rules are not being preserved for ingress but they are for sidecar.

While this may not be fixed in the 0.7.1 release, please ensure that the same problem does not exist with the new v2 routing format for 0.8

linsun commented 6 years ago

@GregHanson or @ijsnellf could you help look into this? cc @esnible

linsun commented 6 years ago

Dan and I went through the pilot discovery API route request (e.g. curl istio-pilot.istio-system:8080/v1/routes/80/istio-proxy/ingress~~istio-ingress-5bb556fcbf-nrdnl.istio-system~istio-system.svc.cluster.local) , and didn't see the base route (80/20 rule) in there.

esnible commented 6 years ago

I was looking at this last night and wondering if it was a precedence or a namespace issue.

linsun commented 6 years ago
hklai commented 6 years ago

Since this is blocking 0.8, I'd to make sure this is currently being worked on. Who is the owner?

rshriram commented 6 years ago

we are not supporting route rules for ingress in 0.8. It will only have thje same functionality as ingress spec in 0.8 (i.e. path based routing, no version routing stuff). This has been the source of all bugs in ingress.

ldemailly commented 6 years ago

there is a regression from 0.6 what are customers supposed to use to get the same behavior that used to work ?

if there is a new way, at minimum it needs to be documented

linsun commented 6 years ago

TODO: need to create this with the gateway and new route rule to determine if this issue occurs in 0.8

dcberg commented 6 years ago

If Ingress route rules are no longer supported in 0.8 then will the istio-Ingress be removed? Thinking being that customers can use whatever Ingress controller they wish but will need to route to the istio gateway. Correct?

linsun commented 6 years ago

@dcberg istio-ingress will be there to allow users to transit to the new gateway if desired. route using the new gateway is recommended as it uses envoy v2 and the new route rule. cc @ZackButcher @frankbu to correct me if I'm wrong here.

rshriram commented 6 years ago

Yes.. with ingress, you get the same behavior as normal ingress - route by path, route to a backend service. You cannot merge ingress spec with istio route rules anymore (that feature was alpha anyway, so no guarantees. Not to mention it was buggy as hell).

The recommended path is you launch a gateway as a LoadBalancer service and route all traffic there. You could use this straight up as your ingress, or you could route from another ingress to this gateway if you wish.

This is not a regression as ingress+routing rules were not working properly to start with (you can look at all the ingress bugs). There were numerous issues. So, if anything, this alleviates that bug in a clear explicit way.

@ldemailly please stop changing labels or adding arbitrary labels to networking issues. It is the responsibility of working group leads to decide the criticality of the issue, and whether it is a release blocker or not.

frankbu commented 6 years ago

I guess the other option for ingress support is to add a sidecar to any nginx ingress controller and use it, now that https://github.com/istio/istio/pull/4569 is done.

ldemailly commented 6 years ago

@rshriram I want @costinm to put "won't fix" and close this if we think indeed it's ok not to fix nor document

Please don't keep changing assignment or labels either

ldemailly commented 6 years ago

@frankbu imo, our solution shouldn't be to use another ingress, it's nice to support nginx for people who do have a large investment in nginx setup but we shouldn't drop functionality that has been working fine since 0.1 using envoy as the ingress

cmluciano commented 6 years ago

/subscribe

rshriram commented 6 years ago

That functionality (route rules with ingress) has not been working fine since 0.1. It would be great if you could help fix the bug since you seem passionate about it.

kdillane commented 6 years ago

Is there documentation about the changes coming to Ingress + route-rules? This sounds like a (possible) regression and it would be helpful to understand the impact to Istio Customers.

kdillane commented 6 years ago

Presumably this is the documentation: https://preliminary.istio.io/docs/reference/config/istio.networking.v1alpha3.html#Gateway

linsun commented 6 years ago

The release call decides it is okay to mark this one as non release block. @ZackButcher - can you update with the rough steps of migrate istio ingress to gateway (co-exist, then transition)?

linsun commented 6 years ago

Played with guestbook with istio-ingress and v1alpha1 rules on 0.8, it appears to be working fine (except when 2 rules over lap e.g. default to v1 and 80% v1 20% v2, which doesn't work as Dan reported.).

vadimeisenbergibm commented 6 years ago

@kdillane Here is an excellent blog post from @frankbu and @rshriram https://preliminary.istio.io/blog/2018/v1alpha3-routing.html on the new routing and gateways in Istio, it describes the syntax and the design rationale behind the changes.

Note that Istio will be backward compatible with regard to the previous routing and ingress, probably until 0.9.

vadimeisenbergibm commented 6 years ago

Adding my two cents:

@dcberg

customers can use whatever Ingress controller they wish but will need to route to the istio gateway

If I were a customer, I would consider deploying a custom istio-ingressgateway - to patch the Istio provided Gateway with whatever functionality required, adding more proxies on the same pod if needed, etc. .istio-ingressgateway can be deployed as a Kubernetes LoadBalancer service with a public IP, and function in the same way as the previous istio-ingress, but with the enhanced functionality of Istio VirtualServices. This way an additional hop on the ingress traffic could be spared.

@linsun

rough steps of migrate istio ingress to gateway

The Ingress definition should be rewritten as a Gateway + VirtualService definition, and the ingress traffic should be directed to istio-ingressgateway (from the outside, Kubernetes-wise - giving a LoadBalancer public IP etc.). Compare the old and the new ways of the Bookinfo Sample Ingress configuration: https://github.com/istio/istio/blob/master/samples/bookinfo/kube/bookinfo-gateway.yaml

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: gateway
  annotations:
    kubernetes.io/ingress.class: "istio"
spec:
  rules:
  - http:
      paths:
      - path: /productpage
        backend:
          serviceName: productpage
          servicePort: 9080
      - path: /login
        backend:
          serviceName: productpage
          servicePort: 9080
      - path: /logout
        backend:
          serviceName: productpage
          servicePort: 9080
      - path: /api/v1/products.*
        backend:
          serviceName: productpage
          servicePort: 9080

https://github.com/istio/istio/blob/master/samples/bookinfo/routing/bookinfo-gateway.yaml

apiVersion: networking.istio.io/v1alpha3
kind: Gateway
metadata:
  name: bookinfo-gateway
spec:
  selector:
    istio: ingressgateway # use istio default controller
  servers:
  - port:
      number: 80
      name: http
      protocol: HTTP
    hosts:
    - "*"
---
apiVersion: networking.istio.io/v1alpha3
kind: VirtualService
metadata:
  name: bookinfo
spec:
  hosts:
  - "*"
  gateways:
  - bookinfo-gateway
  http:
  - match:
    - uri:
        exact: /productpage
    - uri:
        exact: /login
    - uri:
        exact: /logout
    - uri:
        prefix: /api/v1/products
    route:
    - destination:
        host: productpage
        port:
          number: 9080
andraxylia commented 6 years ago

Fixed in 0.8.