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

ref(specs): matches -> routes in HTTPRouteGroup #229

Merged
merged 1 commit into from
Aug 25, 2021

Conversation

michelleN
Copy link
Contributor

@michelleN michelleN commented Aug 11, 2021

This PR changes spec.matches to spec.routes in HTTPRouteGroup.
This change makes the spec a little clearer and easier to
understand. This change will also to more easily talk about
routes in the Traffic Split spec as well.

As part of this refactor, I also removed the matches key from the
TCPRoute spec and the UDPRoute spec as I haven't seen a use for it.
Please let me know if there are any objections.

This PR also updates the directory structure of the draft branch
to make it clearer what file is being used to develop a specific API.

Signed-off-by: Michelle Noorali minooral@microsoft.com

This PR changes spec.matches to spec.routes in HTTPRouteGroup.
This change makes the spec a little clearer and easier to
understand. This change will also to more easily talk about
routes in the Traffic Split spec as well.

This PR also updates the directory structure of the draft branch
to make it clearer what file is being used to develop a specific
API.

Signed-off-by: Michelle Noorali <minooral@microsoft.com>
@trstringer
Copy link

It looks like ports was moved from matches to the root of spec. Can you clarify the different use-cases for the change?

@michelleN
Copy link
Contributor Author

michelleN commented Aug 17, 2021

@trstringer The use case for defining TCPRoute has not changed. If you want to allow a source workload to communicate via tcp to a destination workload on certain ports, you would define a TrafficTarget which would reference a TCPRoute that contains a list of ports you'd want to allow. The matches key in the original TCPRoute spec doesn't seem to serve a specific purpose so I took it out. Please let me know if you think it makes sense to keep it in there given this example below.

Example:

kind: TrafficTarget
metadata:
  name: policies-for-destination-workload
  namespace: default
spec:
  destination:
    kind: ServiceAccount
    name: destination-workload
    namespace: default
  rules:
  - kind: TCPRoute
    name: the-routes
  sources:
  - kind: ServiceAccount
    name: source-workload
    namespace: default
---

kind: TCPRoute
metadata:
  name: the-routes
spec:
  ports:
  - 8080
  - 8081

@trstringer
Copy link

That makes total sense! Thanks for the explanation, and after reading that I also agree.

Copy link
Contributor

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks @michelleN this makes the API more readable 🏅

Copy link
Member

@bridgetkromhout bridgetkromhout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@michelleN michelleN merged commit dcefa32 into servicemeshinterface:draft Aug 25, 2021
@michelleN michelleN deleted the routes branch August 25, 2021 16:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants