lds: Add HTTP API listener. by markdroth · Pull Request #8170 · envoyproxy/envoy

markdroth · 2019-09-06T18:23:34Z

Signed-off-by: Mark D. Roth roth@google.com

Description: Add HTTP API listener.
Risk Level: Low
Testing: None
Docs Changes: Inline with API protos
Release Notes: N/A

@mattklein123 and @htuch, this is an initial stab at the HTTP API listener structure we talked about. Please let me know what you think. Thanks!

Signed-off-by: Mark D. Roth <roth@google.com>

repokitteh-read-only · 2019-09-06T18:45:37Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #8170 was synchronize by markdroth.

see: more, trace.

markdroth · 2019-09-06T18:48:33Z

Looks like there's a BUILD visibility issue including the HTTP connection manager config from the LDS config. Is it okay to change the HCM config to be visible from there, or is there some better way to solve this layering issue?

Signed-off-by: Mark D. Roth <roth@google.com>

markdroth · 2019-09-06T19:14:57Z

I've also added a very basic client capabilities mechanism to the Node message. The idea is that gRPC would send an LDS request with the envoy.grpc_client capability set but with resource_names unset, and the server would send back exactly one HTTP API listener.

I'm quite certain that there are additional things we'll want in client capabilities, but I don't know what they all are yet. My goal here is just to propose something simple that will meet our immediate needs. We can do something different in v3 if we think of something better in the next month or so.

Please let me know what you think. Thanks!

lizan · 2019-09-06T20:43:43Z

@markdroth The visibility constraints exists to prevent core protos depends on extensions.

Btw what is the whole context for adding this? Is this for v2 and not v3alpha (which already exists in the tree)? My speculation is this is for the gRPC client to receive LDS?

mattklein123 · 2019-09-06T20:45:13Z

Btw what is the whole context for adding this? Is this for v2 and not v3alpha (which already exists in the tree)? My speculation is this is for the gRPC client to receive LDS?

This is how gRPC and Envoy Mobile (and similar things) will model an API listener. cc @goaway @junr03.

(I have not reviewed yet, will do so soon)

kyessenov · 2019-09-06T23:34:54Z

api/envoy/api/v2/BUILD

        "//envoy/api/v2/core:base",
        "//envoy/api/v2/listener",
        "//envoy/api/v2/listener:udp_listener_config",
+        "//envoy/config/filter/network/http_connection_manager/v2:http_connection_manager",


I think this causes a circular build loop for proto packages as extensions depend on v2 transitively.

Understood. But it's not obvious how to resolve this problem. Any suggestions...?

We might need to make a decision whether HCM belongs to the core, or whether HTTP API listener belongs to an extension.

@markdroth let's resolve the discussion below then thinks about this, if we get rid of the port to HCM map then we don't need it.

Or just use Filter message which is the opaque config supports HCM.

I'm fine with waiting until some of the broader issues here are resolved. However, I don't think the discussion below will actually make this issue go away. Even if we remove the port map, we will still want an HttpConnectionManager field directly in the listener for the HTTP API listener, since the whole point of this new listener type is that it is always hard-coded to a single HttpConnectionManager.

Anyway, let's see how things shake out with the broader design discussion below, and then we can come back to this.

htuch

This is heading the right place directionally, thanks for getting this started, but lots of API concerns, we should have full consensus from @envoyproxy/api-shepherds before merging.

api/envoy/api/v2/core/base.proto

api/envoy/api/v2/lds.proto

mattklein123

Thanks for getting this started. I think this is a really critical concept for client libraries consuming the APIs.

api/envoy/api/v2/core/base.proto

mattklein123 · 2019-09-08T20:25:08Z

api/envoy/api/v2/lds.proto

+  enum Type {
+    // Default listener type. Binds to a port.
+    SOCKET = 0;
+    // HTTP API listener. This type of listener does not bind to a socket or support L3/L4


For discussion, see the existing deprecated bind_to_port option in this message. IMO the API listener we are proposing is different, but it would be good to make sure that we are fully committed to removing that other option.

Also, while I think this is fine for v2 in which this message is a bit of a mess, this is one of the examples that I want to clean up in v3 in terms of making the various listener types polymorphic in terms of TCP listener, UDP listener, QUIC listener, API listener, etc. so I think we need to move this entire message into a set of common fields and then a oneof with specific messages for each listener type. I think what you are doing here is OK for v2, but can we add comments to remind us of what we immediately want to clean up for v3?

Agreed. Added a comment about this.

api/envoy/api/v2/lds.proto

Signed-off-by: Mark D. Roth <roth@google.com>

markdroth

Thanks for all the feedback!

@ejona86 may want to weigh in on some of this discussion.

markdroth · 2019-09-09T16:51:43Z

api/envoy/api/v2/lds.proto

+  enum Type {
+    // Default listener type. Binds to a port.
+    SOCKET = 0;
+    // HTTP API listener. This type of listener does not bind to a socket or support L3/L4


Agreed. Added a comment about this.

api/envoy/api/v2/core/base.proto

api/envoy/api/v2/lds.proto

markdroth · 2019-09-09T17:17:22Z

api/envoy/api/v2/BUILD

        "//envoy/api/v2/core:base",
        "//envoy/api/v2/listener",
        "//envoy/api/v2/listener:udp_listener_config",
+        "//envoy/config/filter/network/http_connection_manager/v2:http_connection_manager",


Understood. But it's not obvious how to resolve this problem. Any suggestions...?

mattklein123 · 2019-09-10T18:56:02Z

I'm tagging this with no stale bot. Let's continue this conversation when @htuch is back from OOO.

/wait-any

lambdai · 2019-09-11T06:55:55Z

Now let's say that you have some gRPC clients that are currently going through Envoy but that you want to switch to use xDS directly
Eventually I am catching up: This new API is to support grpc client, which is not envoy. Instead it is an another envoy-ish xds client.
Your proposal will probably never implemented by envoy. There is not clear boundary which part of the LDS should be provided if the xds client is envoy, and which part is used if the xds client is the grpc library.

With this PR any management server has to generate LDS response by understand which xds client is served. IMHO this is a huge regression.

Signed-off-by: Mark D. Roth <roth@google.com>

markdroth · 2019-09-16T15:15:20Z

I think the one significant remaining issue to resolve here is the build dependency problem introduced by including the HCM config in lds.proto. I'd welcome suggestions on how to address that.

mattklein123 · 2019-09-16T18:33:00Z

api/envoy/api/v2/lds.proto

+  // Used only when the :ref:`type<envoy_api_field_Listener.type>` field is set to
+  // :ref:`API_HTTP<envoy_api_field_Listener.Type.API_HTTP>`.
+  // The HttpConnectionManager configuration to be used by the HTTP listener.
+  envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager


WDYT about adding an ApiListener message, and then inside of that having a oneof with various types of API listeners, HCM being the only one today? I think @lizan already made this point, but in the future we might want to support TCP, Redis, etc.

I've attempted to do this. Please let me know if this is not what you had in mind.

Signed-off-by: Mark D. Roth <roth@google.com>

mattklein123

Thanks, yeah this is what I had in mind. @htuch will probably need to advise on the compile issues though.

htuch · 2019-09-18T17:04:02Z

api/envoy/api/v2/lds.proto

+// [#not-implemented-hide:]
+// Describes a type of API listener, which is used in non-proxy clients. The type of API
+// exposed to the non-proxy application depends on the type of API listener.
+message ApiListener {


I think we should make ApiListener a envoy.config scoped type. We're going to retire the legacy envoy.api.vN namespace in v3.

Signed-off-by: Mark D. Roth <roth@google.com>

lizan · 2019-09-18T19:50:48Z

api/envoy/config/api_listener/v2/api_listener.proto

+// Describes a type of API listener, which is used in non-proxy clients. The type of API
+// exposed to the non-proxy application depends on the type of API listener.
+message ApiListener {
+  oneof api_type {


Can this just be a Filter? then you don't have to explicitly use HCM so build will pass, also we don't need to modify this to support other protocols.

The goal here is that this message should explicitly allow only the specific type of configuration appropriate to the type of API listener. If we make it a Filter, we allow invalid configs like specifying a type of filter that is not related to a listener; this way, that kind of invalid config is structurally impossible, which makes validation easier.

Don't we need to validate anyway (with PGV and additional checks to check if the config is valid HCM config), so just one more type check shouldn't be a deal breaker, no?

I'll let @htuch and @mattklein123 weigh in on this.

From my perspective, the fact that the HCM config happens to be a filter config is basically a coincidence; in the future, we could have other types of API listeners whose fields are not existing filter messages. And the vast majority of filter messages are not suitable as configuration for a type of API listener. So it seems better to me to tailor this to what we actually need rather than providing unnecessary flexibility that IMHO makes the configuration harder to understand.

The Filter message is just a name with Any or Struct, what do you mean by "the vast majority of filter messages are not suitable as configuration for a type of API listener"?

What I mean is that a Filter field expects its Any field to be populated with one of the messages in the envoy/config/filter tree (or a message for a third-party filter). The vast majority of those messages will never be suitable for use as a type of API listener.

The concept here is that an API listener exposes a type-specific API to the application. The HTTP listener conceptually provides an API that exposes reading and writing headers, data, and trailers for an HTTP request; this happens to map to the functionality provided by the HCM, and the HCM happens to also be used as a filter in Envoy, but that's just a coincidence. In the future, we could also have (e.g.) an SQL API listener that exposes an API based around sending SQL queries and processing result records, or any other L7 protocol for which there is no existing Envoy filter. And conversely, most of the existing filters do not define L7 protocols and therefore can't act as API listeners; for example, it would be inherently non-sensical to say that we are going to expose an API for applications to access a service via (e.g.) fault injection or access logging.

What I mean is that a Filter field expects its Any field to be populated with one of the messages in the envoy/config/filter tree (or a message for a third-party filter). The vast majority of those messages will never be suitable for use as a type of API listener.

That's why I said, "do a type check and reject the config as DPLB", seems you just don't like the name "Filter"?

In the future, we could also have (e.g.) an SQL API listener that exposes an API based around sending SQL queries and processing result records, or any other L7 protocol for which there is no existing Envoy filter. And conversely, most of the existing filters do not define L7 protocols and therefore can't act as API listeners; for example, it would be inherently non-sensical to say that we are going to expose an API for applications to access a service via (e.g.) fault injection or access logging.

This doesn't seems conflicting with use of Any (or "Filter"), it is just a free form Any, the xDS protocol we're making here should guard that at reporting capabilities of which type of Any is supported, IMHO that should not be limited by the types of proto message level here.

I do think that using Filter is worse than Any, because it implies to the reader that any filter configuration can be used here, which is not the case. API listeners really have nothing to do with filters, and IMHO it does not make sense to conflate two unrelated concepts.

With regard to the difference between a oneof and an Any, I think the trade-off is that an Any makes it easier to add new types, whereas a oneof provides more type safety and structurally prevents invalid configurations. I realize that we can also catch that kind of problem with validation, and I'm certainly not opposed to validation, but I think it makes everyone's lives easier if the underlying structure doesn't allow misconfiguration in the first place. If that weren't the case, every single field in every proto would be an Any. :)

IMHO, Any makes sense only in cases where we expect new types to be added frequently, especially by third parties. But I don't think that's the case here; I don't expect that adding new types of API listener will happen that often, so the additional flexibility of using Any isn't much of an offsetting advantage here.

In any case, as mentioned below, we have no choice but to go with an Any for now, due to the go proto circular dependency issue. But I've added a note that we should convert this to a oneof in the v3 API.

I agree with Mark here that makes sense to be more specific, but given that we can't due to circular dependency issues, it seems fine to use Any for now.

IMHO, Any makes sense only in cases where we expect new types to be added frequently, especially by third parties. But I don't think that's the case here; I don't expect that adding new types of API listener will happen that often, so the additional flexibility of using Any isn't much of an offsetting advantage here.

If we don't expect third parties add new types here then oneof is totally fine. If we don't expect any "I have my own API library want to implement in-house RPC protocol with UDPA" to be come up, this is fine. Historically we did convert oneof to Any or add Any to oneof in several places in Envoy (cluster, retry policies...), so if we're fine on that expectation I'm good with that.

kyessenov · 2019-09-18T20:48:28Z

api/envoy/config/api_listener/v2/BUILD

+
+api_proto_package(
+    deps = [
+        "//envoy/config/filter/network/http_connection_manager/v2:http_connection_manager",


I think this should be v2:pkg.

I tried this, but I ran into a circular dependency problem with the go protos: http_connection_manager.proto depends on rds.proto, which is the same directory as lds.proto, so we cannot make lds.proto transitively depend on http_connection_manager.proto.

After chatting with @kyessenov, @htuch, and @dfawley about this, I've changed the ApiListener proto to contain an Any field instead of a oneof, and I've documented that the type of the Any field dictates the type of API listener. I've also added a large comment saying that in v3, we need to fix the circular dependency and replace the Any with a oneof.

I'm not terribly happy about this, but it seems like it's the best we can do in v2.

kyessenov · 2019-09-18T20:48:55Z

api/envoy/config/api_listener/v2/api_listener.proto

+
+import "gogoproto/gogo.proto";
+
+option (gogoproto.equal_all) = true;


Please remove this. We don't need to import gogoproto anymore.

Signed-off-by: Mark D. Roth <roth@google.com>

mattklein123

Thanks this LGTM.

@htuch @lizan @junr03 @goaway WDYT?

htuch

LGTM, just some package namespace comment.
/wait

htuch · 2019-09-19T21:36:24Z

api/envoy/config/api_listener/v2/api_listener.proto

@@ -0,0 +1,24 @@
+syntax = "proto3";
+
+package envoy.config.api_listener.v2;


Do you want to make this v2alpha until you have verified this works?

I don't think that would actually buy us anything, because we need to use it from the v2 LDS API. But I really don't expect any significant changes here, especially since we're using Any for now; if we do need something else, we can just change what message we're using in the Any.

htuch · 2019-09-19T21:36:59Z

api/envoy/config/api_listener/v2/api_listener.proto

@@ -0,0 +1,24 @@
+syntax = "proto3";
+
+package envoy.config.api_listener.v2;


I would suggest making this envoy.config.listener.v2 as a package name. The proto can still be api_listener.proto, but this can be a package where general listener config goes in the future.

Signed-off-by: Mark D. Roth <roth@google.com>

junr03

lgtm, I don't see any constraint here that would not work with future envoy mobile work given that in brief what is being added here is an Any. Thanks for the thoughtful discussion everyone :)

junr03 · 2019-09-19T22:58:29Z

api/envoy/api/v2/lds.proto

+  // exposed to the non-proxy application depends on the type of API listener.
+  // When this field is set, no other field except for :ref:`name<envoy_api_field_Listener.name>`
+  // should be set.
+  // [#next-major-version: In the v3 API, instead of this messy approach where the socket


Agree with this. It would be nice to subsume socket-based listener fields to their own message.

Signed-off-by: Mark D. Roth <roth@google.com>

htuch · 2019-09-23T15:08:56Z

api/envoy/config/api_listener/v2/api_listener.proto

@@ -0,0 +1,24 @@
+syntax = "proto3";
+
+package envoy.config.listener.v2;


Nit: also change the directory to api/envoy/config/listener/v2. LGTM once that is done.

Signed-off-by: Mark D. Roth <roth@google.com>

htuch

Thanks!

Signed-off-by: Mark D. Roth <roth@google.com>

Description: this PR introduces the initial implementation of an Api Listener based on the proto configuration merged in #8170. Notably, this PR introduces the ability to add only _one_ api listener via _bootstrap config only_. This decision was made in order to iterate into more complex setups (multiple listeners, LDS supplied listeners) in subsequent PRs. Moreover, the API listener is created in the context of Envoy's main thread not worker threads. A first use of this Api Listener can be seen in envoyproxy/envoy-mobile#616. Risk Level: low, only used in Envoy Mobile. The risk here is about building something generally useful and flexible. Note however that a couple of things were rejiggered in the HCM. Testing: unit and integration tests. Additional testing in https://github.com/lyft/envoy-mobile. Docs Changes: Added inline comments and TODOs. Proto documentation is up-to-date. Release Notes: similar to doc changes. Signed-off-by: Jose Nino <jnino@lyft.com>

Description: this PR introduces the initial implementation of an Api Listener based on the proto configuration merged in envoyproxy/envoy#8170. Notably, this PR introduces the ability to add only _one_ api listener via _bootstrap config only_. This decision was made in order to iterate into more complex setups (multiple listeners, LDS supplied listeners) in subsequent PRs. Moreover, the API listener is created in the context of Envoy's main thread not worker threads. A first use of this Api Listener can be seen in envoyproxy/envoy-mobile#616. Risk Level: low, only used in Envoy Mobile. The risk here is about building something generally useful and flexible. Note however that a couple of things were rejiggered in the HCM. Testing: unit and integration tests. Additional testing in https://github.com/lyft/envoy-mobile. Docs Changes: Added inline comments and TODOs. Proto documentation is up-to-date. Release Notes: similar to doc changes. Signed-off-by: Jose Nino <jnino@lyft.com> Mirrored from https://github.com/envoyproxy/envoy @ 9b6260fcf6ee1299744b8e5c76c1e6d9d36f7c89

Add HTTP API listener.

677d65b

Signed-off-by: Mark D. Roth <roth@google.com>

zuercher assigned mattklein123 and htuch Sep 6, 2019

Add missing BUILD dependency.

3799372

Signed-off-by: Mark D. Roth <roth@google.com>

Add very basic client capability propoposal to Node message.

c860458

Signed-off-by: Mark D. Roth <roth@google.com>

kyessenov reviewed Sep 6, 2019

View reviewed changes

htuch suggested changes Sep 8, 2019

View reviewed changes

api/envoy/api/v2/core/base.proto Outdated Show resolved Hide resolved

api/envoy/api/v2/core/base.proto Outdated Show resolved Hide resolved

api/envoy/api/v2/lds.proto Outdated Show resolved Hide resolved

mattklein123 requested changes Sep 8, 2019

View reviewed changes

Add next-major-version comment.

33c5759

Signed-off-by: Mark D. Roth <roth@google.com>

markdroth commented Sep 9, 2019

View reviewed changes

mattklein123 added the no stalebot Disables stalebot from closing an issue label Sep 10, 2019

repokitteh-read-only bot added the waiting:any label Sep 10, 2019

repokitteh-read-only bot removed the waiting:any label Sep 11, 2019

mattklein123 added the waiting label Sep 14, 2019

Remove port mapping and capability mechanism.

0203af1

Signed-off-by: Mark D. Roth <roth@google.com>

repokitteh-read-only bot removed the waiting label Sep 16, 2019

mattklein123 reviewed Sep 16, 2019

View reviewed changes

Move API listener config to its own message using a oneof.

7df2ae1

Signed-off-by: Mark D. Roth <roth@google.com>

mattklein123 reviewed Sep 17, 2019

View reviewed changes

htuch reviewed Sep 18, 2019

View reviewed changes

Move ApiListener proto to config tree.

137a963

Signed-off-by: Mark D. Roth <roth@google.com>

lizan reviewed Sep 18, 2019

View reviewed changes

kyessenov reviewed Sep 18, 2019

View reviewed changes

markdroth added 2 commits September 18, 2019 14:10

Remove gogoproto.

6039c6d

Signed-off-by: Mark D. Roth <roth@google.com>

Switch from oneof to Any to avoid circular dependency for go protos.

20fb3f5

Signed-off-by: Mark D. Roth <roth@google.com>

mattklein123 previously approved these changes Sep 19, 2019

View reviewed changes

lizan previously approved these changes Sep 19, 2019

View reviewed changes

htuch suggested changes Sep 19, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 19, 2019

Change package name.

1be606a

Signed-off-by: Mark D. Roth <roth@google.com>

markdroth dismissed stale reviews from lizan and mattklein123 via 1be606a September 19, 2019 22:03

repokitteh-read-only bot removed the waiting label Sep 19, 2019

junr03 reviewed Sep 19, 2019

View reviewed changes

Fix package name used in lds.proto.

7442342

Signed-off-by: Mark D. Roth <roth@google.com>

htuch reviewed Sep 23, 2019

View reviewed changes

Rename directory.

265c5ae

Signed-off-by: Mark D. Roth <roth@google.com>

htuch approved these changes Sep 23, 2019

View reviewed changes

mattklein123 merged commit 081b0b9 into envoyproxy:master Sep 23, 2019

markdroth deleted the lds_api_listener branch September 23, 2019 16:35

danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Sep 24, 2019

lds: Add HTTP API listener. (envoyproxy#8170)

dcd0666

Signed-off-by: Mark D. Roth <roth@google.com>

danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019

lds: Add HTTP API listener. (envoyproxy#8170)

5e5d7ec

Signed-off-by: Mark D. Roth <roth@google.com>

danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019

lds: Add HTTP API listener. (envoyproxy#8170)

46140ff

Signed-off-by: Mark D. Roth <roth@google.com>

junr03 mentioned this pull request Dec 28, 2019

listener: first implementation of an Api Listener #9516

Merged

menghanl mentioned this pull request Dec 30, 2019

Documentation/examples for xds resolver & balancer grpc/grpc-go#3286

Closed

sanjaypujare mentioned this pull request Feb 21, 2020

Document behavior for non-proxy (such as gRPC) server side xDS clients #10126

Closed


		import "gogoproto/gogo.proto";

		option (gogoproto.equal_all) = true;

		@@ -0,0 +1,24 @@
		syntax = "proto3";

		package envoy.config.api_listener.v2;

		@@ -0,0 +1,24 @@
		syntax = "proto3";

		package envoy.config.listener.v2;

Conversation

markdroth commented Sep 6, 2019

Uh oh!

repokitteh-read-only bot commented Sep 6, 2019

Uh oh!

markdroth commented Sep 6, 2019

Uh oh!

markdroth commented Sep 6, 2019

Uh oh!

lizan commented Sep 6, 2019

Uh oh!

mattklein123 commented Sep 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Sep 10, 2019

Uh oh!

lambdai commented Sep 11, 2019

Uh oh!

markdroth commented Sep 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!