Skip to content
This repository has been archived by the owner on Oct 20, 2023. It is now read-only.

Add the possibility to constraint TrafficSplit based on sources, while aligning TrafficSplit with TrafficTarget #190

Closed
4 of 5 tasks
patricekrakow opened this issue Sep 24, 2020 · 4 comments
Labels
proposal Propose changes to the SMI specification

Comments

@patricekrakow
Copy link
Contributor

patricekrakow commented Sep 24, 2020

Describe the proposal
The current TrafficSplit allows to restrict routing based on (1) the HTTP request content via the HTTPRouteGroup and (2) the destination via (Kubernetes) services. But, it does not include (yet?) a possible restriction based on (3) the source, for which we could also use (Kubernetes) services.
I also think that we should align more the TrafficSplit structure with the TrafficTarget structure.

Scope

  • New specification
  • Traffic Access Control
  • Traffic Specs
  • Traffic Metrics
  • Traffic Split

Possible use cases
The new structure of the TrafficSplit would look like the following

kind: TrafficSplit
metadata:
  name: routing
  namespace: demo
spec:
  sources:
  - apiVersion: ... 
    kind: Service
    name: client-x-v1-0-0
  rules:
  - kind: HTTPRouteGroup
    name: alpha-api-routes
    matches:
    - get-path-01
  destination:
    kind: Service
    name: acme-api
    backends:
    - kind: Service
      name: service-a-v1-0-0
      weight: 90
    - kind: Service
      name: service-a-v1-1-0
      weight: 10

If this draft proposal would make sense to some of you, I can support it by a PR. Just let me know...

@patricekrakow patricekrakow added the proposal Propose changes to the SMI specification label Sep 24, 2020
@patricekrakow
Copy link
Contributor Author

patricekrakow commented Sep 29, 2020

The main purpose of this issue was to discuss the possibility to add a reference to a source Service within the TrafficSplit.

However looking at the current version of TrafficTarget, TrafficSplit and TrafficMetrics, it looks that they all use a different way to reference to other resources. So before modifying the TrafficSplit to add a reference to a source Service, I would first align the way to reference to other resources for all SMI resources.

Short Analysis

TrafficTarget

The TrafficTarget resource is using the combination of kind, name and namespace to refer to other resources.

spec:
  destination:
    kind: ServiceAccount
    name: service-a
    namespace: default
    port: 8080
  rules:
  - kind: HTTPRouteGroup
    name: the-routes
    matches:
    - metrics
  sources:
  - kind: ServiceAccount
    name: prometheus
    namespace: default

source: https://github.com/servicemeshinterface/smi-spec/blob/master/apis/traffic-access/v1alpha2/traffic-access.md

It looks like rules[].namespace is missing (?)

TrafficSplit

The TrafficTarget resource is using service to refer to other Services only.

spec:
  service: website
  backends:
  - service: website-v1
    weight: 90
  - service: website-v2
    weight: 10

source: https://github.com/servicemeshinterface/smi-spec/blob/master/apis/traffic-split/v1alpha3/traffic-split.md

It looks like namespace is missing (?)

TrafficMetrics

The TrafficMetrics resource is also using (like TrafficTarget) the combination of kind, name and namespace to refer to other resources, but as children of a generic resource field.

resource:
  name: foo-775b9cbd88-ntxsl
  namespace: foobar
  kind: Pod

source: https://github.com/servicemeshinterface/smi-spec/blob/master/apis/traffic-metrics/v1alpha1/traffic-metrics.md

The TrafficMetrics resource does not have a specfield (?)

Canary from Flagger

In addition to that, it seems that other CRD from other project, such as the Canary from the Flagger are using yet other structure to refer to other resources, such as apiVersion, name and kind:

spec:
  # service mesh provider (optional)
  # can be: kubernetes, istio, linkerd, appmesh, nginx, skipper, contour, gloo, supergloo
  provider: istio
  # deployment reference
  targetRef:
    apiVersion: apps/v1
    kind: Deployment
    name: podinfo

source: https://github.com/weaveworks/flagger/blob/master/artifacts/flagger/crd.yaml

It looks like namespace is missing (?)

Preliminary Result

I found all these different ways to to refer to other resources quite confusing, and I think we should select one way to be applied to all SMI resources.

Personally, I like

<purpose>Ref:
  kind: ...
  name: ...
  namespace: ...
...
<purpose>Ref:
  - kind: ...
    name: ...
    namespace: ...

@patricekrakow
Copy link
Contributor Author

Following our discussion during the SMI Community Meeting (2020-09-30), I will split this issue into two separate issues:

  • One issue to promote a common structure - apiVersion, kind, name and namespace if applicable - to reference other object(s)/resource(s);
  • One issue for adding the possibility to constraint TrafficSplit based on sources (including a precise use case).

@michelleN
Copy link
Contributor

@patricekrakow - have you had any luck working on the issue to promote a common structure? While implementing, I ran into this ambiguity around how to implement Matches in TrafficSplit

@patricekrakow
Copy link
Contributor Author

Hello @michelleN - it has been a while since I looked at SMI. I would be happy to look back at it and (re-)check the consistency between the different CRDs ;-) Just give me a few days...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
proposal Propose changes to the SMI specification
Projects
None yet
Development

No branches or pull requests

3 participants