istio / old_pilot_repo

Deprecated home of Istio's Pilot, now in istio/istio's pilot dir
Apache License 2.0
137 stars 91 forks source link

destination.labels is ignored in weighted rule #1625

Closed rshriram closed 6 years ago

rshriram commented 6 years ago

Weighted route rules allow using IstioService object as a way of specifying the destination (https://istio.io/docs/reference/config/traffic-rules/routing-rules.html#destinationweight). IstioService object has the notion of labels, name/namespace, etc. However, the weighted route rule (DestinationWeight object) also has the notion of labels as a separate field. Fortunately, no one has discovered the discrepancy yet. We should fix this by either ignoring the labels in the IstioService object (if so, who is using the labels there?), or by removing the top level labels field from the DestinationWeight object, and asking users to do

metadata:
  name: my-rule
  namespace: default
spec:
  destination:
    name: reviews
  route:
  - destination:
        labels:
         version: v2
    weight: 25
  - destination:
       labels:
        version: v1
    weight: 75

While the above format adds another nesting layer, the advantage is that users can now do cross namespace routing easily.

cc @frankbu @ijsnellf @GregHanson @kyessenov

( @ijsnellf / @GregHanson can one of you take up this issue?)

kyessenov commented 6 years ago

Deprecating labels in destination weight would be nice (although a bit verbose). I think the code does check for this case, or least it's documented here: https://github.com/istio/api/blob/master/proxy/v1/config/route_rule.proto#L157

frankbu commented 6 years ago

At one point during the design we had two different proto messages for the source and destination fields, because destinations don't use labels. We simplified to use IstioService for both and added the doc: "When used for a RouteRule destination, labels MUST be empty.". https://istio.io/docs/reference/config/traffic-rules/routing-rules.html#istioservice

kyessenov commented 6 years ago

Issue moved to istio/istio #1361 via ZenHub

kyessenov commented 6 years ago

Issue moved to istio/istio #1362 via ZenHub