Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Traffic Route L7 proposal #2013
Traffic Route L7 proposal #2013
Changes from all commits
6ecb880
2374202
ec6c802
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest a match-type field:
This is consistent with Kubernetes API conventions and allows valid match types to be automatically validated by API annotations. It also makes validation check unambigious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, there is a feature of protobuf called "oneof" and providing a couple of options and picking one of them is a convention of Envoy. We use this already in many places. Even
loadBalancer
in TrafficRouteThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were they also migrating from "oneof" convention?
Validation on API annotations? I don't think I'm familiar with this, can you tell me more? We have our own unified validators for both Kubernetes and Universal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The kubernetes CRD generator support validation that will propagate checks into the openapi spec (see kubebuilder docs).
I'm confide about the protobuf reference. Is this a protobuf API? If so, shouldn't it be in proto3 rather than in yaml?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kuma API is described in protobuf and then translated between JSON / YAML (but also sent between control planes in multizone deployment as serialized protobuf). In proposals, we tend to use YAML examples so it's easier to read, but you might be right that maybe we should also include proto schemas.
Protobuf also has a builtin validation functionality but we found this to not be flexible enough (descriptive messages) for our needs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. I'd probably design a YAML-oriented API differently than a protobuf-oriented API :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, Kubernetes CRD validation doesn't support unions, or one-of. There are generator annotations for it, but they don't do anything 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gateway API just moved away from this pattern, to a style that is more consistent with Kubernetes API guidelines:
Similar reasons as for path matches. This structure is also consistent with the structure you have in the modify section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the kubernetes rationale for avoiding maps, https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#lists-of-named-subobjects-preferred-over-maps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this
match
object?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assume that "HTTP" in this context means, http, https, http/2 and so on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https
yes, but only when mTLS is provided by Kuma. Otherwise, we cannot decrypt the trafficThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok you confused me with mtls :) why is mtls required here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imagine there is a client and server with sidecars
If A itself does not encrypt the traffic then even with mTLS A's sidecar can redirect the traffic because it can read the request metadata (it happens before encrypting the traffic)
However, if A encrypts the traffic with TLS itself (for example old service when migrating to mesh), then A's sidecar cannot decrypt the traffic and we cannot route it by matching path, headers, etc.