Merged
Conversation
olix0r
commented
Mar 6, 2023
|
|
||
| // Indicates the proxy is connecting to a named address (like an HTTP | ||
| // authority). | ||
| string authority = 3; |
Member
Author
There was a problem hiding this comment.
Using the word "authority" specifically indicates that it may have a port! Looking around the API, we don't have anywhere else where we use a string name and numeric port, so let's just go with a string encoded address.
olix0r
commented
Mar 6, 2023
| rpc Watch(TrafficSpec) returns (stream OutboundPolicy) {} | ||
| } | ||
|
|
||
| message TargetSpec { |
Member
Author
There was a problem hiding this comment.
It isn't a target if it includes a source workload.
To understand whether the changes proposed in #165 will be suitable for extension, this change adds a GrpcRoute type. This motivates a number of followup changes, all of which make the API more closely mirror the proxy's representation: * Filter and Distribution moved into HttpRoute. Each protocol/variant may have its own filter types, so we want to preserve namespacing. * Weights moved up into the Distribution * HttpRoute and GrpcRoute each get a RouteBackend that holds protocol-specific filter kinds. * Queue extracted from Backend. We could use it elsewhere... * Extracted an `EndpointDiscovery` strategy from `BalanceP2c` so that we can change that API if desired. * Added a `Metadata` onto the backend -- the controller is going to have populate that.
olix0r
added a commit
that referenced
this pull request
Mar 7, 2023
The new API needs to be able to describe, generally, opaque routes, even though there is no user-facing configuration for these routes at the moment. This change eliminates the `Backend` on the top level response--it was used _only_ to synthesize an opaque route in the proxy. This opaque route should be represented properly in the API and synthesized in the control plane.
adleong
approved these changes
Mar 7, 2023
olix0r
added a commit
that referenced
this pull request
Mar 7, 2023
The new API needs to be able to describe, generally, opaque routes, even though there is no user-facing configuration for these routes at the moment. This change eliminates the `Backend` on the top level response--it was used only to synthesize an opaque route in the proxy. This opaque route should be represented properly in the API and synthesized in the control plane.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
To understand whether the changes proposed in #165 will be suitable for extension, this change adds a GrpcRoute type. This motivates a number of followup changes, all of which make the API more closely mirror the proxy's representation:
EndpointDiscoverystrategy fromBalanceP2cso that we can change that API if desired.Metadataonto the backend -- the controller is going to have populate that.I'd like to include the GrpcRoute type in the API, even if some of the implementations are lacking. There's no reason we can't support this in the proxy in the near-term.