Add support for VirtualService delegate

[hzxuzhonghu]

Based on this proposal, added http route rules chaining.

[frankbu]

It seems strange that the delegate VirtualService has no hosts. I would have expected:

hosts:
- productpage.nsA.svc.cluster.local

Otherwise, where are the rules for routing internal requests to the reviews service? Won't I need to create another VS with exactly the same contents (+ the hosts field) to define the rules for internal calls from inside the mesh to the productpage service?

[hzxuzhonghu]

Yes, why I note this is that:
Here we need to distinguish if this is a delegate or root VS, we don't want the delegate rule function alone. Because in addition to the matching rules, there are some fields like headers, if the delegate function alone, the result may be not intended.

[frankbu]

In general, I would say the same behavior would be expected for a delegate or standalone VS for the same target. Can you give me a specific example of where they need to be different?

I also think that if there really is a difference between a delegate and a standalone VS, you should either use a different structure for delegates (e.g., kind: VirtualServiceDelegate), or at least have some clear way to mark it as a delegate (e.g., maybe a special reserved host value). Using a VS with no hosts to represent a delegate seems a little like a hack.

[hzxuzhonghu]

I agree it is a littler hack, but that's consistent with k8s proposal https://docs.google.com/document/d/1BxYbDovMwnEqe8lj8JwHo8YxHAt3oC7ezhlFsG_tyag/edit#heading=h.czggbhygorv and also coutour IngressRoute https://github.com/projectcontour/contour/blob/master/design/ingressroute-design.md

[hzxuzhonghu]

I also think that if there really is a difference between a delegate and a standalone VS, you should either use a different structure for delegates (e.g., kind: VirtualServiceDelegate), or at least have some clear way to mark it as a delegate (e.g., maybe a special reserved host value).

Indeed it has a difference, say multi delegates should work as referred orders, but if we donot know whether they are delegates, the routing rule will be applied un ordered.

So, in order to remove the hack way, a field indicates it is a delegate sounds good to me.

[howardjohn]

I'll repeat what I'll say over and over again: we should do what HTTPRoute does

[rshriram]

Here is one thought.

Virtual service can have a separate include section that specifies names of other virtual services and conditions under which these virtual services should be included.

Eg

 host: foo.com
 IncludeVirtualServices:
 - resource: ns1/vs1
    condition:
     - httpMatch:
         urimatchHasPrefix: /barbar

This will cause pilot to only include the http rules that have /barbar as a prefix or exact match in the vs1 virtual service in the ns1 namespace.

Now that said, @louiscryan & @frankbu the problem with delegation now is how to deal with the imports at gateway. Gateway hosts can specify something like ns1/*.bar.com - wherein the semantics is that only virtual services matching *.bar.com from ns1 namespace will be included by the gateway. If such a virtual service in ns1 namespace then delegates to ns2 namespace, do we import even though the gateway didn’t say so ? If we do, the semantics will be confusing. If we don’t and force people to put all the delegated vs in the ns1 namespace only, then the effectiveness of delegation is less as the motivation for delegation is that the gateway admin in say istio-system can define the top level virtual service alone while delegating the specific virtual services to the namespaces that own the destination hosts of the virtual services.

[hzxuzhonghu]

kind: HTTPRoute
name: http-app-3
namespace: internal-app
rules:
- match:
    http:
      host: bar.com
      path:
        prefix: /store
  action:
    backend:                            # Delegated to another team that
      name: delegate-1                  # manages "bar.com/store". This is
      namespace: other-team             # a reference to another route 
      kind: HTTPRoute # in a different namespace.
      apiGroup: networking.k8s.io
  ...
---
kind: HTTPRoute
name: delegate-1
namespace: other-team
rules:
- match:
    http:
      host: bar.com
      path: 
        prefix: /store
  action:
    backend:
      name: store-app
  ...

This is what k8s community proposed, cross namespace reference in istio also exists from the beginning, like DestinationRule, gateway and virtualservice, etc.

[hzxuzhonghu]

 host: foo.com
 IncludeVirtualServices:
 - resource: ns1/vs1
    condition:
     - httpMatch:
         urimatchHasPrefix: /barbar

By this we can introduce a new struct with simplified matching conditions(maybe just has uri, headers and query params), which can simplify the merging semantics in the original proposal.

[howardjohn]

I think we should stick to what the have in the k8s API so we can converge. Adding different behavior just makes users confused

[howardjohn]

also we should probably be designing this in the design doc not a pr

[rshriram]

I think we should stick to what the have in the k8s API so we can converge. Adding different behavior just makes users confused

Lets first do what makes sense in our api as users today will be confused if this is incongruent with what we have. K8s api has to evolve. We dont want to restrict delegation to just http. Delegation is possible with sni routing and tcp routing as well. A top level include with match conditions will allow us to delegate for all scenarios in Istio

[howardjohn]

Lets first do what makes sense in our api as users today will be confused if this is incongruent with what we have. K8s api has to evolve. We dont want to restrict delegation to just http. Delegation is possible with sni routing and tcp routing as well. A top level include with match conditions will allow us to delegate for all scenarios in Istio

For what its worth, the k8s apis also support tls/tcp delegation. We don't need to completely restrict ourselves to their API but I do think its valuable to draw inspiration from it, and when we do drift from it make sure we are actually getting value from it

[hzxuzhonghu]

I have already supplemented tcp/tls delegate days ago, please have a look if you have a chance.

[hzxuzhonghu]

FYI, basically it aligns with istio in high level.

K8s gateway
https://github.com/kubernetes-sigs/service-apis/blob/12ca3b8264c51d49ec5b31d351aae19a57c0eccc/api/v1alpha1/gateway_types.go#L53-L62

type GatewaySpec struct {
	// Class used for this Gateway. This is the name of a GatewayClass resource.
	Class string `json:"class"`
	// Listeners associated with this Gateway. Listeners define what addresses,
	// ports, protocols are bound on this Gateway.
	Listeners []Listener `json:"listeners"`
	// Routes associated with this Gateway. Routes define
	// protocol-specific routing to backends (e.g. Services).
	Routes []core.TypedLocalObjectReference `json:"routes"`
}

HTTPRoute

https://github.com/kubernetes-sigs/service-apis/blob/12ca3b8264c51d49ec5b31d351aae19a57c0eccc/api/v1alpha1/httproute_types.go#L24-L53

// HTTPRouteSpec defines the desired state of HTTPRoute
type HTTPRouteSpec struct {
	// Hosts is a list of Host definitions.
	Hosts []HTTPRouteHost `json:"hosts,omitempty"`

	// Default is the default host to use. Default.Hostnames must
	// be an empty list.
	//
	// +optional
	Default *HTTPRouteHost `json:"default"`
}

// HTTPRouteHost is the configuration for a given host.
type HTTPRouteHost struct {
	// Hostnames are a list of hosts names that match this host
	// block.
	//
	// TODO: RFC link
	Hostnames []string `json:"hostnames"`

	// Rules are a list of HTTP matchers, filters and actions.
	Rules []HTTPRouteRule `json:"rules"`

	// Extension is an optional, implementation-specific extension
	// to the "host" block.
	//
	// Support: custom
	//
	// +optional
	Extension *core.TypedLocalObjectReference `json:"extension"`
}

[hzxuzhonghu]

/test gencheck_api

[rshriram]

skip delegate for tcp. We have no way of setting parent match conditions

[hzxuzhonghu]

ok

[ramaraochavali]

Left couple of couple of minor comments, otherwise LGTM

[ramaraochavali]

nit: /s/"Only one tier of delegate is supported"/"Only one level delegation is supported"

[ramaraochavali]

can we make this oneof (HttpRoute, HttpRedirect, Delegate)?

[hzxuzhonghu]

I would like to make this a separate pr.

[hzxuzhonghu]

It seems possible

[ramaraochavali]

Sure. not a problem