-
Notifications
You must be signed in to change notification settings - Fork 5.3k
WIP: config: Add FilterChain Discovery Service. #5385
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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; | ||
|
|
||
| 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. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How come a listener and not a filter chain?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 StreamFilterChains(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: "*" | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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"; | ||
|
|
||
|
|
@@ -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 | ||
|
|
@@ -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 config = 1 [(validate.rules).message.required = true]; | ||
|
|
||
| // Optional alternative to the listener name to present to FCDS. | ||
| string filter_chain_name = 2; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
ggreenway marked this conversation as resolved.
Show resolved
Hide 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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| #include "test/config/utility.h" | ||
| #include "test/integration/http_integration.h" | ||
| #include "test/test_common/network_utility.h" | ||
|
|
||
| #include "gtest/gtest.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace { | ||
|
|
||
| // Integration test for EDS features. EDS is consumed via filesystem | ||
| // subscription. | ||
| class FcdsIntegrationTest : public HttpIntegrationTest, | ||
| public testing::TestWithParam<Network::Address::IpVersion> { | ||
| public: | ||
| FcdsIntegrationTest() | ||
| : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, GetParam(), realTime()) {} | ||
|
|
||
| void initialize() override { | ||
| // setUpstreamCount(4); | ||
| config_helper_.addConfigModifier([this](envoy::config::bootstrap::v2::Bootstrap& bootstrap) { | ||
| auto* listener_0 = bootstrap.mutable_static_resources()->mutable_listeners(0); | ||
| listener_0->mutable_filter_chains()->Clear(); | ||
| listener_0->mutable_fcds_config()->mutable_config()->set_path("/"); | ||
| }); | ||
| HttpIntegrationTest::initialize(); | ||
| } | ||
| }; | ||
|
|
||
| INSTANTIATE_TEST_CASE_P(IpVersions, FcdsIntegrationTest, | ||
| testing::ValuesIn(TestEnvironment::getIpVersionsForTest())); | ||
|
|
||
| TEST_P(FcdsIntegrationTest, test) { initialize(); } | ||
|
|
||
| } // namespace | ||
| } // namespace Envoy |
There was a problem hiding this comment.
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
gogoprotodecl? Seems like it makes sense to group options.