upstream: add EDS indirection to allow repeated hosts #5432
Conversation
Adds the API for an additional EDS indirection that alllows endpoints to specified outside the LB structure. This opens up for being able to reference the same endpoint multiple times in a single CLA. Signed-off-by: Snow Pettersen <[email protected]>
|
Wanted to make sure we agree on the API before committing to code changes, cc @htuch @bplotnick |
Signed-off-by: Snow Pettersen <[email protected]>
Signed-off-by: Snow Pettersen <[email protected]>
| @@ -50,10 +50,22 @@ message Endpoint { | |||
| HealthCheckConfig health_check_config = 2; | |||
| } | |||
|
|
|||
| // Named reference to an Endpoint. | |||
| message NamedEndpoint { | |||
| // Identifier for this endpoint. Should be unique within a given ClusterLoadAssignment. | |||
| // Upstream host identifier or a named reference. | ||
| oneof host_identifier { | ||
| Endpoint endpoint = 1; | ||
| NamedEndpoint named_endpoint = 5; |
There was a problem hiding this comment.
i think what you have makes the most sense, but what's the tradeoff here over adding the optional identifier to the Endpoint message itself?
There was a problem hiding this comment.
Yeah, I reckon conceptually it might be simpler to just add an optional named field to the endpoint directly; the anonymous endpoints that we have today then fit neatly into this with proto defaults.
| // Upstream host identifier or a named reference. | ||
| oneof host_identifier { | ||
| Endpoint endpoint = 1; | ||
| NamedEndpoint named_endpoint = 5; |
There was a problem hiding this comment.
Yeah, I reckon conceptually it might be simpler to just add an optional named field to the endpoint directly; the anonymous endpoints that we have today then fit neatly into this with proto defaults.
Signed-off-by: Snow Pettersen <[email protected]>
api/envoy/api/v2/eds.proto
Outdated
| @@ -51,6 +51,9 @@ message ClusterLoadAssignment { | |||
| // List of endpoints to load balance to. | |||
| repeated endpoint.LocalityLbEndpoints endpoints = 2 [(gogoproto.nullable) = false]; | |||
|
|
|||
| // List of named endpoints that can be referenced multiple times in LocalityLbEndpoints. | |||
| repeated endpoint.Endpoint named_endpoints = 5; | |||
There was a problem hiding this comment.
Cool, this looks simpler. Could even minimize the PR further by making this a map with key being host name string and value being the endpoint. WDYT?
There was a problem hiding this comment.
Works for me. Like how it enforces the key uniqueness
Signed-off-by: Snow Pettersen <[email protected]>
|
@snowp looks like everyone is holding their peace; could you do a master merge to reduce the chance of breaking CI when I merge this? Thanks |
Signed-off-by: Snow Pettersen <[email protected]>
|
This escaped my view and now I'm paying the price...what's the Envoy policy for breaking protocol buffer changes? |
|
@jmarantz what's the issue? |
|
The golang API into a protobuf 'oneof' is different, even with the wire format the same. I've already worked through it for the case I needed to fix, but others may bump into this. |
|
@jmarantz we don't provide backward compatibility to language bindings, only at the wire level. See https://github.com/envoyproxy/envoy/blob/master/api/STYLE.md for the policy. |
Adds the API for an additional EDS indirection that allows endpoints to specified outside the LB structure. This opens up for being able to reference the same endpoint multiple times in a single CLA. Risk Level: Low, only API changes for now Testing: n/a Docs Changes: n/a Release Notes: n/a envoyproxy#4280 Signed-off-by: Snow Pettersen <[email protected]> Signed-off-by: Fred Douglas <[email protected]>
Adds the API for an additional EDS indirection that allows endpoints to
specified outside the LB structure. This opens up for being able to
reference the same endpoint multiple times in a single CLA.
Signed-off-by: Snow Pettersen [email protected]
Risk Level: Low, only API changes for now
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
#4280