smi-spec icon indicating copy to clipboard operation
smi-spec copied to clipboard

Define TCPRoute spec

Open stefanprodan opened this issue 5 years ago • 12 comments

Describe the proposal

The current TCPRoute is more of a placeholder than an actual specification as it doesn't have a top level spec or any fields. We should define a spec for it and afterwords revisit the UDPRoute PR.

A TCPRoute can match a list of ports:

apiVersion: specs.smi-spec.io/v1alpha3
kind: TCPRoute
metadata:
  name: mysql-routes
  namespace: default
spec:
  matches:
    ports:
    - 3306
    - 6446

A TCPRoute can match all ports with:

apiVersion: specs.smi-spec.io/v1alpha3
kind: TCPRoute
metadata:
  name: default
  namespace: default
matches:
  ports: []

Scope

  • [ ] New specification
  • [ ] Traffic Access Control
  • [x] Traffic Specs
  • [ ] Traffic Metrics
  • [ ] Traffic Split

Possible use cases A service mesh would use the TCPRoute to avoid protocol detection for the specified ports and just use L4.

stefanprodan avatar Apr 29 '20 18:04 stefanprodan

@stefanprodan how would this interact with the ports already on TrafficTarget? It feels like we can just move the ports into this entirely? Alternatively, it is a little weird to have the ports on every protocol (tcp, udp, http). Is there a protocol that wouldn't include ports?

grampelberg avatar Apr 29 '20 22:04 grampelberg

I actually gave thoughts about that yesterday evening as well, and I think Port could be a good add (also for the UDP Spec).

I think it's fine to have that on the Route, in order to be able to specify subsets of L4 Level Traffic.

SantoDE avatar Apr 30 '20 07:04 SantoDE

Another thought, just something that came to my mind. I know, initially has been said that it could be fine to avoid protocol detection. If we do want to have protocol detection tough, how about some sort of SNI matching? Could / would it make sense?

---
apiVersion: specs.smi-spec.io/v1alpha3
kind: TCPRoute
metadata:
  name: mongo-routes
  namespace: default
spec:
  matches:
    ports:
    - 27017
    - 27018
    - 27019
    sni:
      host: my-mongo.internal.svc

For the port question, I think every protocol will have Ports for sure, but I feel like the TrafficTarget should infer that from the route as the route is in my mind bundled with the Port (and you can have multiple). Currently, port is an optional element on the TrafficTarget, which if not set, will allow all traffic anyway, right? So inferring it from the Route kinda makes sense, I guess.

SantoDE avatar May 01 '20 13:05 SantoDE

I don't think the SNI should be specified on a TCPRoute, you should be able to use the mongo route definition for all your mongo instances no matter the SNI. The TrafficTarget should be the place where you associate a traffic spec to a specific service.

stefanprodan avatar May 01 '20 13:05 stefanprodan

@stefanprodan I think the SNI host would be an optional match, so it would not be required. SNI being a TLS extension would make sense to be tied to the TCPRoute IMO, and it would allow protocol-specific routes to have protocol-specific matches. Just as path makes sense to be on an HTTPRouteGroup, sni makes sense on TCPRoute.

dtomcej avatar May 01 '20 20:05 dtomcej

SNI's interesting, what problem would be solving with it?

grampelberg avatar May 01 '20 20:05 grampelberg

It would allow TCPRoute to operate without statically defined ports, by matching on an SNI header instead.

As long as requests were made from a valid source, with the correct SNI header, it would be allowed.

TCP just has so few properties with which to match on (especially compared to HTTP), that it seems that there should be a method to match on the few that do exist.

dtomcej avatar May 01 '20 20:05 dtomcej

Oooh, that's an interesting idea. How would this work from the implementation perspective? As that's all handled automatically for you in Linkerd (via identity), it might be a little weird.

grampelberg avatar May 01 '20 21:05 grampelberg

---
apiVersion: specs.smi-spec.io/v1alpha3
kind: TCPRoute
metadata:
  name: mongo-routes
  namespace: default
spec:
  matches:
    ports:
    - 27017
    - 27018
    - 27019
    sni:
      host: my-mongo.cluster.svc.local
---
kind: TrafficTarget
metadata:
  name: tcp-demo
  namespace: default
spec:
  destination:
    kind: ServiceAccount
    name: mongo-db
    namespace: default
  sources:
  - kind: ServiceAccount
    name: my-app
    namespace: default
  rules:
  - kind: TCPRoute
    name: mongo-routes
---
apiVersion: v1
kind: Service
metadata:
  name: my-mongo
spec:
  selector:
    app: MyApp
  ports:
    - protocol: TCP
      port: 27017

Alright, from an implementation point of view, we could check the given SNI in the TCP Route and match it against the internal dns address of a service, which is holding a pod which has the destination service account.

To be honest, I'm not quite sure if its greatly "usable" but at least, it's an idea.

However, apart from that, if we decide to keep out SNI for now and "only" use ports, what does stop us from moving on further here?

SantoDE avatar Aug 12 '20 14:08 SantoDE

@SantoDE I think we could open a new issue for the SNI proposal after #185 is merged.

stefanprodan avatar Aug 13 '20 08:08 stefanprodan

SGTM @stefanprodan. Will take a look at #185 too

SantoDE avatar Aug 13 '20 08:08 SantoDE

@stefanprodan How does this proposal handle the use case where we want a combination of TCP and HTTP routes for an endpoint while ensuring no ambiguity?

For ex. HTTP GET on port 9000 should be allowed, but a GET on other ports shouldn't be allowed. This scenario could arise for services that expose multiple ports.

shashankram avatar Aug 20 '20 23:08 shashankram