Skip to content
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.

Sign up for GitHub

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

Jump to bottom

lds: Add HTTP API listener. #8170

Merged
merged 12 commits into from
Sep 23, 2019
Merged

lds: Add HTTP API listener. #8170

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
677d65b
Add HTTP API listener.
markdroth Sep 6, 2019
3799372
Add missing BUILD dependency.
markdroth Sep 6, 2019
c860458
Add very basic client capability propoposal to Node message.
markdroth Sep 6, 2019
33c5759
Add next-major-version comment.
markdroth Sep 9, 2019
0203af1
Remove port mapping and capability mechanism.
markdroth Sep 16, 2019
7df2ae1
Move API listener config to its own message using a oneof.
markdroth Sep 16, 2019
137a963
Move ApiListener proto to config tree.
markdroth Sep 18, 2019
6039c6d
Remove gogoproto.
markdroth Sep 18, 2019
20fb3f5
Switch from oneof to Any to avoid circular dependency for go protos.
markdroth Sep 18, 2019
1be606a
Change package name.
markdroth Sep 19, 2019
7442342
Fix package name used in lds.proto.
markdroth Sep 20, 2019
265c5ae
Rename directory.
markdroth Sep 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions api/envoy/api/v2/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ api_proto_library_internal(
"//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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

],
)

Expand Down
28 changes: 27 additions & 1 deletion api/envoy/api/v2/lds.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import "envoy/api/v2/core/base.proto";
import "envoy/api/v2/discovery.proto";
import "envoy/api/v2/listener/listener.proto";
import "envoy/api/v2/listener/udp_listener_config.proto";
import "envoy/api/config/filter/network/http_connection_manager/v2/http_connection_manager.proto";

import "google/api/annotations.proto";
import "google/protobuf/duration.proto";
Expand Down Expand Up @@ -45,13 +46,31 @@ service ListenerDiscoveryService {
}
}

// [#comment:next free field: 19]
// [#comment:next free field: 21]
message Listener {
// The unique name by which this listener is known. If no name is provided,
// Envoy will allocate an internal UUID for the listener. If the listener is to be dynamically
// updated or removed via :ref:`LDS <config_listeners_lds>` a unique name must be provided.
string name = 1;

// [#not-implemented-hide:]
// [#next-major-version: In the v3 API, instead of this messy approach where the enum
// value dictates which fields should be set, we should refactor this message such that
// the fields for each type of listener are each in their own sub-message, and we put the
// sub-messages for each type into a oneof. That way, a given Listener message can
// structurally only contain the fields of the relevant type.]
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Added a comment about this.

// filter chains; it simply exposes an HTTP API to the xDS client application.
// The only fields that should be set for this listener type are
// :ref:`name<envoy_api_field_Listener.name>` and
// :ref:`api_http_connection_manager<envoy_api_field_Listener.api_http_connection_manager>`,
API_HTTP = 1;
}
Type type = 19;

// The address that the listener should listen on. In general, the address must be unique, though
// that is governed by the bind rules of the OS. E.g., multiple listeners can listen on port 0 on
// Linux as the actual port will be allocated by the OS.
Expand Down Expand Up @@ -203,4 +222,11 @@ message Listener {
// <envoy_api_field_listener.UdpListenerConfig.udp_listener_name>` = "raw_udp_listener" for
// creating a packet-oriented UDP listener. If not present, treat it as "raw_udp_listener".
listener.UdpListenerConfig udp_listener_config = 18;

// [#not-implemented-hide:]
// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

api_http_connection_manager = 20;
}