Removes TrafficSplit in favor of ForwardToTarget

[danehans]

Currently, TrafficSplit is scaffolding. This PR removes TrafficSplit in favor of implementing traffic splitting within ForwardToTarget.

1. Pluralizes forwardTo to allow forwarding to multiple targets (i.e. destination objects).
2. Introduces the weight target attribute used by forwardTo to distribute traffic among multiple targets.

Partially fixes #58.

[danehans]

/assign @bowei

[danehans]

fixes issue: #58

[hbagdi]

What decision parameters are we using here to limit the scope of what is included in the core API and what is part of extensions? This is one of those areas where implementations will have different implementations and behaviors.

[danehans]

TODO: Should Filter be invalid for passthrough tls connections?

[hbagdi]

I think yes. This should be valid only for HTTP routes. Filter should not be interpreted for TCP routes all together, no matter it is plain-text or TLS pass through.

[danehans]

@hbagdi I believe the decision is "do most implementations support this feature". How it's supported is up to the implementation. IMO most implementations do support percentage-based traffic splitting. Do you disagree? Are there any other properties presented in this PR that you feel most implementations do not support... maybe Selector?

[hbagdi]

I think selector is not a problem. Traffic splitting itself is a common enough construct but not all providers, especially cloud providers and managed offerings don't support injecting headers specifically for different splits. Most have support for injecting headers but not different headers in context of splits.

[danehans]

@hbagdi thanks for the feedback. I created #108 to discuss this in more detail.

[danehans]

Most have support for injecting headers but not different headers in context of splits.

@hbagdi I marked the Filter field as "Extended". This is the same field from HTTPRoute. The primary difference from HTTPRoute is TrafficSplit performs the Filter, i.e. add/remove header, post routing decision. Can you help me better understand why Filter is a non-issue for HTTPRoute but is an issue for TrafficSplit?

[hbagdi]

I marked the Filter field as "Extended". This is the same field from HTTPRoute. The primary difference from HTTPRoute is TrafficSplit performs the Filter, i.e. add/remove header, post routing decision. Can you help me better understand why Filter is a non-issue for HTTPRoute but is an issue for TrafficSplit?

I think it is fair to add support for filters and transformation but it would be better to do that in one place rather several different. How do you plan to connect TrafficSplit with Route?

[danehans]

TODO:

1. Update spec to state where the type should be referenced from.
2. Get input from implementors on the best match>filter>action ordering. For example, Knative requires filtering post routing.

[yiyangy]

It seems to me this is more of Action, i.e. what happens to a request, than Filter.

[danehans]

Manipulating a request is considered a filter in terms of Service API's. PTAL at the route types as a reference implementation.

[yiyangy]

If a header with a given name already exists in the request, will this become a replace? Or it should be configurable, i.e. whether add or replace?

[danehans]

@yiyangy I believe that is implementation-specific. cc: @bowei @youngnick

[youngnick]

Yes, for Contour we chose to make Add include an implicit replace in the case the header already existed, but I don't know if we want to dictate that at this level.

[danehans]

@yiyangy note that api/v1alpha1/trafficsplit_types.go has been removed in the latest iteration of the PR.

[yiyangy]

Shall this Selector be at the upper level i.e. TrafficSplitSpec? In the sense that for all the traffic matches Selector, the traffic is split among multiple rules with each rule carrying a weight. In case I misunderstand, could you please maybe give an example to elaborate on the use of Selector?

[danehans]

@yiyangy config/samples/networking_v1alpha1_trafficsplit.yaml provides an example, PTAL.

[danehans]

I think it is fair to add support for filters and transformation but it would be better to do that in one place rather several different. How do you plan to connect TrafficSplit with Route?

@hbagdi xRoute performs transformations prior to routing traffic. We need the ability to support post-routing transformations for use cases such as Knative. This is why I added TrafficSplitFilter as a field of the TrafficSplitRule type. Do you have suggestions on how to handle pre and post filtering across the different objects? Should filters be a separate object that is referenced by the route/trafficfilter types?

The following is an example usage of TrafficSplit. The HTTPRoute forwards traffic for host "foo.com/bar" to a TrafficSplit named "my-trafficsplit", which splits traffic to service "my-trafficsplit-svc", forwarding 80% of the traffic to endpoints labeled "app=v1" and 20% to endpoints labeled "app=v2". The example removes header "bar" and adds header "my-header: foo" before forwarding traffic to the service:

kind: HTTPRoute
apiVersion: networking.x.k8s.io/v1alpha1
metadata:
  name: http-app-1
  namespace: default
spec:
  hosts:
    - hostnames:
        - "foo.com"
      rules:
        - match:
            path: /bar
          action:
            forwardTo:
              resource: trafficsplit
              group: networking.x.k8s.io/v1alpha1
              name: my-trafficsplit
---
apiVersion: networking.x.k8s.io/v1alpha1
kind: TrafficSplit
metadata:
  name: my-trafficsplit
spec:
  rules:
    - destination:
        group: core
        resource: service
        name: my-trafficsplit-svc
      selector:
        app: v1
      weight: 80
      filter:
        headers:
          add:
            my-header: foo
          remove:
            - bar
    - destination:
        group: core
        resource: service
        name: my-trafficsplit-svc
      selector:
        app: v2
      weight: 20
      filter:
        headers:
          add:
            my-added-header: foo
          remove:
            - bar

[jpeach]

This could easily be confused with the SMI trafficsplit. I'm not sure that we should add more CRD types, it seems likely that if we go down this path, the number of types will grow very large.

I cases like this, I think that a brief implementation survey is useful to give context to the design. Many ingress controllers already have support for this feature; how do they do it and what are the outcomes of theirs approaches?

[bowei]

It's a good question regarding traffic spit, someone on the SMI side even mentioned trying to merge these two or to use the same resource, trying to find the reference.

[yiyangy]

Another option is that instead of weight specifying the fraction, the fraction of traffic to be forwarded to a targetRef is computed as weight / (sum of all weights in targetRefs). Then the constraint of "A valid weight value is 0-100. The sum of weights across targetRefs must equal 100" can be removed.

[danehans]

@yiyangy thanks for the review. I've updated the godocs based on your feedback.

[robscott]

After some discussion about multicluster implications last week, I spent some time writing a doc that shows how this could work with multicluster services. At a high level, the theory is that traffic splitting as defined by Service APIs is focused on splitting traffic between Services. In a multicluster scenario, traffic can further be segmented by configuring how much traffic is distributed to each cluster. That would be accomplished either on the Service or ServiceExport resource. I've run that by @JeremyOT to confirm that this is compatible with his vision of multicluster services, and it sounds like it is.

Would love additional feedback on that doc to ensure the approach makes sense. The doc is entirely based on the approach proposed by this PR, just translating it to multicluster use cases. If there's general consensus on that, I think we're good to proceed here as well.

[JeremyOT]

Consider making weight required and using CRD defaults to set a default value. We can simplify by e.g. using weight: 1 when not specified. This makes behavior intuitive when a weight isn't supplied, and makes weighting functionality more discoverable as users will see a value to play with when viewing their configuration.

[danehans]

@JeremyOT commit 9abadaa updates weight based on your feedback above. Thanks for the review.

[hbagdi]

/lgtm

[hbagdi]

/assign @bowei

[robscott]

Thanks for the work on this! Just had a couple small nits.

/lgtm

[robscott]

Nit: It would be good to clarify that weight is not actually a percentage and that all weights do not need to add up to 100.

[danehans]

@robscott commit d7e4c20 addresses your comment above.

[robscott]

Nit: I don't think we need this clarification. Weight doesn't need to be ignored. If there is only forwardTo it is guaranteed to include 100% of the total weight, even if that is only 1.

[danehans]

@robscott commit d7e4c20 addresses your comment above.

[robscott]

Thanks!

/lgtm

[bowei]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, danehans

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [bowei]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment