Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
20 changes: 20 additions & 0 deletions api/envoy/api/v2/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,24 @@ api_go_grpc_library(
],
)

api_proto_library_internal(
name = "fcds",
srcs = ["fcds.proto"],
has_services = 1,
visibility = [":friends"],
deps = [
":discovery",
],
)

api_go_grpc_library(
name = "fcds",
proto = ":fcds",
deps = [
":discovery_go_proto",
],
)

api_proto_library_internal(
name = "lds",
srcs = ["lds.proto"],
Expand All @@ -106,6 +124,7 @@ api_proto_library_internal(
":discovery",
"//envoy/api/v2/core:address",
"//envoy/api/v2/core:base",
"//envoy/api/v2/core:config_source",
"//envoy/api/v2/listener",
],
)
Expand All @@ -117,6 +136,7 @@ api_go_grpc_library(
":discovery_go_proto",
"//envoy/api/v2/core:address_go_proto",
"//envoy/api/v2/core:base_go_proto",
"//envoy/api/v2/core:config_source_go_proto",
"//envoy/api/v2/listener:listener_go_proto",
],
)
Expand Down
36 changes: 36 additions & 0 deletions api/envoy/api/v2/fcds.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
syntax = "proto3";

package envoy.api.v2;

option java_generic_services = true;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: Should this be down next to gogoproto decl? Seems like it makes sense to group options.


import "envoy/api/v2/discovery.proto";

import "google/api/annotations.proto";
import "google/protobuf/wrappers.proto";

import "validate/validate.proto";
import "gogoproto/gogo.proto";

option (gogoproto.equal_all) = true;

// [#protodoc-title: Filter Chain Discovery Service]

// The resource_names field in DiscoveryRequest specifies a listener.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How come a listener and not a filter chain?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

My thought was that envoy would be telling the mgmt server which listener it needs the filter chains for. I believe this is similar to EDS, where the resource_names specify the clusters to get endpoints for.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The only downside to this would be if a user wants multiple listeners to reference the same filter chain, and then specify it within a listener by name. I'm not sure it matters that much in practice so seems fine either way.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

May be I am missing something. In a situation where you have a single listener on original dst port having 100s of filter chains (one for each of the virtual listeners that we would otherwise be constructing with the ye ol bind-to-port), how would specifying the listener name be sufficient?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@rshriram Envoy is asking the mgmt server "Please give me all the filterchains/matches for listener Foo".

// The resources field in DiscoveryResponse is values of type listener.FilterChain.
service FilterChainDiscoveryService {
rpc StreamRoutes(stream DiscoveryRequest) returns (stream DiscoveryResponse) {
}

rpc IncrementalFilterChains(stream IncrementalDiscoveryRequest)
returns (stream IncrementalDiscoveryResponse) {
}

rpc FetchFilterChains(DiscoveryRequest) returns (DiscoveryResponse) {
option (google.api.http) = {
post: "/v2/discovery:filterchains"
body: "*"
};
}
}

18 changes: 15 additions & 3 deletions api/envoy/api/v2/lds.proto
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ option java_generic_services = true;

import "envoy/api/v2/core/address.proto";
import "envoy/api/v2/core/base.proto";
import "envoy/api/v2/core/config_source.proto";
import "envoy/api/v2/discovery.proto";
import "envoy/api/v2/listener/listener.proto";

Expand Down Expand Up @@ -37,7 +38,7 @@ service ListenerDiscoveryService {
}
}

// [#comment:next free field: 16]
// [#comment:next free field: 17]
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
Expand All @@ -57,10 +58,21 @@ message Listener {
// :ref:`FilterChainMatch <envoy_api_msg_listener.FilterChainMatch>` criteria is used on a
// connection.
//
// Precisely one of filter_chains and fdcs_config must be set.
//
// Example using SNI for filter chain selection can be found in the
// :ref:`FAQ entry <faq_how_to_setup_sni>`.
repeated listener.FilterChain filter_chains = 3
[(validate.rules).repeated .min_items = 1, (gogoproto.nullable) = false];
repeated listener.FilterChain filter_chains = 3 [(gogoproto.nullable) = false];

message FcdsConfig {
core.ConfigSource fds_config = 1;

// Optional alternative to the listener name to present to FCDS.
string filter_chain_name = 2;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm thinking we want to make filter chains first class named resource objects and name them directly, instead of making listeners pseudo-filterchains. This seems the cleanest thing to do, curious what the motivation was for the current approach you have in the PR?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I was trying to copy the semantics of EDS for consistency.

Are you suggesting that the filter chains should be entirely independent of the listeners? That would allow multiple listeners to share a set of filter-chains, but we'd have to think carefully about the lifetime issues.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess this is related to my comment above. I don't think @htuch is proposing actually sharing the chains (I think), but is suggesting that they have a first class name and we don't default to the listener name. As I said above I'm fine either way.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree with @htuch . FilterChains should be the resource that is being requested (mandatory and never optional). We can combine it with the listener name for precision. I am not sure sharing filter chains makes sense in the common case

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

As this is currently written, the set of filter chains can be given a first-class name; it just defaults to using the listener name if no name is specified. Is the suggestion here that this name be required instead of optional? Am I misunderstanding or missing something regarding this?

Also, note that the requested resource is always the set of filterchains/matches to use for a listener, not a single filter chain.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it makes more sense to name individual filter chains as first class for the purpose of incremental xDS. This will future proof FDS to a larger extent. Essentially, the Listener has a list of filter chain names that describe the resources that come with it. When we lazy load, we will request an additional named filter chain resource.

I don't think we need to share filter chains across listeners; as an implementation detail you might if you really wanted to save overhead and expect many different listeners to have the same filter chains, but that probably isn't the starting point.

@ggreenway ggreenway Jan 7, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If we did that, it would no longer solve the original problem: we want to be able to add/remove filter chains without modifying the listener (which would result in listener draining).

The answer may be to support both modes: either request "all filter chains for this listener", or request "filter chain X for this listener".

Won't most/all of the xDS APIs need to be augmented in a similar way to support lazy-loading?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As discussed offline, it seems reasonable to make listener changes non-draining for the filter chain list.

}

// Precisely one of filter_chains and fdcs_config must be set.
Comment thread
ggreenway marked this conversation as resolved.
FcdsConfig fcds_config = 17;

// If a connection is redirected using *iptables*, the port on which the proxy
// receives it might be different from the original destination address. When this flag is set to
Expand Down