Add transport socket config#218
Conversation
Signed-off-by: Lizan Zhou <zlizan@google.com>
api/base.proto
Outdated
| } | ||
|
|
||
| // Configuration for low level socket in listeners and clusters. If the configuration is empty, | ||
| // the default low level socket configuration will be choosed based on the platform and |
There was a problem hiding this comment.
s/choosed/chosen/
s/the default/a default/
s/low level/low-level/
api/base.proto
Outdated
| // the default low level socket configuration will be choosed based on the platform and | ||
| // existence of tls_context. | ||
| message LowLevelSocket { | ||
| // The name of low level socket to instantiate. The name must match a supported low level socket |
api/base.proto
Outdated
|
|
||
| // Implementation specific configuration which depends on the implementation being instantiated. | ||
| // See the supported low level socket implementations for further documentation. | ||
| google.protobuf.Any config = 2; |
There was a problem hiding this comment.
This is not very useful from a consumer perspective, especially since I have no idea what goes into this field. Sorry to be a PITA, but could you add some example (like the way we we indent yaml snippet in istio) or something? How do we populate this, if we were to handle this from LDS management plane?
There was a problem hiding this comment.
This is also a place where we are going to introduce inconsistency in the API. I do agree with @lizan that Any is the cleanest way to embed config, since it can support efficient binary representation and also a JSON-like Struct embedding. That is, providing you support at a code level embedded Struct decoding to the expected proto (please do this!).
OTOH, every other API for plugins and opaque config in Envoy today is Struct based. So, the question to be addressed here is do we care about (1) consistency across the Envoy xDS APIs or (2) the cleanest way to represent opaque configs for (i) efficient binary encoding and (ii) nice text proto representation. @lizan is this a fair summary of the question being asked?
I would personally vote for consistency, but I will defer to what the rest of the community thinks is best.
There was a problem hiding this comment.
@rshriram the configuration is to allow plug-in additional low-level socket implementations, with additional config, a default ones will be chosen automatically so you don't to handle this from LDS/CDS, unless you have another low-level socket impl, and want to use that.
Just think this as a filter, if you didn't link any, you don't need to configure one.
There was a problem hiding this comment.
@htuch yes that is a fair summary.
cc/@mattklein123 what is the conclusion for envoyproxy/envoy#1680? If we are moving to Any, (regardless with interim oneof or not, or the priority of it), then I'd prefer Any here. If everything is going to stick with Struct then I'm fine with oneof or Struct. I'd defer to you guys to make the call.
There was a problem hiding this comment.
All things being equal I would prefer consistency. In this case, perf does not matter, so I think struct is fine. So I would vote struct.
| message LowLevelSocket { | ||
| // The name of low level socket to instantiate. The name must match a supported low level socket | ||
| // implementation. | ||
| string name = 1; |
There was a problem hiding this comment.
what are the valid names? Are you thinking that most of the time, people will have to specify the platform name or something and we can infer based on this? Like linux/darwin/windows/etc. ?
There was a problem hiding this comment.
No, it is same concept as filters / tracers, based on what you linked into the binary. You don't need to specify ones by default.
d35842d to
d9c1702
Compare
Signed-off-by: Lizan Zhou <zlizan@google.com>
d9c1702 to
8592717
Compare
api/base.proto
Outdated
| // Configuration for low-level socket in listeners and clusters. If the configuration is empty, | ||
| // a default low-level socket implementation and configuration will be chosen based on the | ||
| // platform and existence of tls_context. | ||
| message LowLevelSocket { |
There was a problem hiding this comment.
I made this name up while brainstorming the overall problem - it wasn't necessarily meant to be the final name. This is probably our last chance to change it here and in Envoy. Do we like this name?
There was a problem hiding this comment.
No strong feelings, but some names to riff on (some are problematic as is) SocketFamily, TransportSocket, SocketConfig, SocketWrapper, ...
There was a problem hiding this comment.
I think I like TransportSocket personally over LowLevelSocket, but I don't really care one way or the other. I mostly just wanted to say that we don't need to do this name because of what I said in the related PR.
There was a problem hiding this comment.
TransportSocket or LowLevelSocket are both fine, IMHO.
There was a problem hiding this comment.
I prefer TransportSocket, but either is fine
There was a problem hiding this comment.
OK @lizan you pick between LowLevelSocket or TransportSocket!
There was a problem hiding this comment.
I'm fine either way, so I'll update this to TransportSocket soon.
Signed-off-by: Lizan Zhou <zlizan@google.com>
Signed-off-by: Lizan Zhou <zlizan@google.com>
Per conversation in envoyproxy/envoy#1924 and #189
Signed-off-by: Lizan Zhou zlizan@google.com