Feature : Listener Filter Chain Discovery Service #23096
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ import "udpa/annotations/security.proto"; | |
| import "udpa/annotations/status.proto"; | ||
| import "udpa/annotations/versioning.proto"; | ||
| import "validate/validate.proto"; | ||
| import "envoy/config/core/v3/config_source.proto"; | ||
|
|
||
| option java_package = "io.envoyproxy.envoy.config.listener.v3"; | ||
| option java_outer_classname = "ListenerProto"; | ||
|
|
@@ -44,7 +45,7 @@ message ListenerCollection { | |
| repeated xds.core.v3.CollectionEntry entries = 1; | ||
| } | ||
|
|
||
| // [#next-free-field: 35] | ||
| message Listener { | ||
| option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.Listener"; | ||
|
|
||
|
|
@@ -138,6 +139,9 @@ message Listener { | |
| // :ref:`FAQ entry <faq_how_to_setup_sni>`. | ||
| repeated FilterChain filter_chains = 3; | ||
|
|
||
| // FCDS: Filter Chain Discovery Service config block. filter_chains and fcds are mutually exclusive | ||
| Fcds fcds = 34; | ||
|
|
||
| // :ref:`Matcher API <arch_overview_matching_listener>` resolving the filter chain name from the | ||
| // network properties. This matcher is used as a replacement for the filter chain match condition | ||
| // :ref:`filter_chain_match | ||
|
|
@@ -358,3 +362,8 @@ message Listener { | |
| // :ref:`global_downstream_max_connections <config_overload_manager_limiting_connections>`. | ||
| bool ignore_global_conn_limit = 31; | ||
| } | ||
|
|
||
| message Fcds{ | ||
| string fcds_name = 1 [(validate.rules).string.min_len = 1]; | ||
|
There was a problem hiding this comment. Every listener filter chain has a name so one can think of a listener as a collection of filter chains. Is this name referring to the collection name? There was a problem hiding this comment. That's right @kyessenov. It is referring to the name of the collection of listener filter chains under a listener. It is not used in the code anywhere today though. It's just a name to refer to. Can I have multiple of those such collections of listener filter-chains under the same listener? No. There was a problem hiding this comment. Thanks for explaining. If two collections share a filter chain, then at the transport level, do we still have to get duplicate resources There was a problem hiding this comment.
Somewhat dependent on the use-case, but if you think of ADS, then there should be a single subscription to the management server, and it will notify the multiple "watchers". |
||
| envoy.config.core.v3.ConfigSource config_source = 2 [(validate.rules).message.required = true]; | ||
| } | ||
|
There was a problem hiding this comment. This should probably be called ListenerFCDS, to differentiate between listener and http filter chains. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| # DO NOT EDIT. This file is generated by tools/proto_format/proto_sync.py. | ||
|
|
||
| load("@envoy_api//bazel:api_build_system.bzl", "api_proto_package") | ||
|
|
||
| licenses(["notice"]) # Apache 2 | ||
|
|
||
| api_proto_package( | ||
| has_services = True, | ||
| deps = [ | ||
| "//envoy/annotations:pkg", | ||
| "//envoy/api/v2:pkg", | ||
| "//envoy/service/discovery/v3:pkg", | ||
| "@com_github_cncf_udpa//udpa/annotations:pkg", | ||
| ], | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| syntax = "proto3"; | ||
|
|
||
| package envoy.service.filter_chain.v3; | ||
|
|
||
| import "envoy/service/discovery/v3/discovery.proto"; | ||
|
|
||
| import "google/api/annotations.proto"; | ||
|
|
||
| import "envoy/annotations/resource.proto"; | ||
| import "udpa/annotations/status.proto"; | ||
| import "udpa/annotations/versioning.proto"; | ||
|
|
||
| option java_package = "io.envoyproxy.envoy.service.filter_chain.v3"; | ||
| option java_outer_classname = "FcdsProto"; | ||
| option java_multiple_files = true; | ||
| option java_generic_services = true; | ||
| option (udpa.annotations.file_status).package_version_status = ACTIVE; | ||
|
|
||
| // [#protodoc-title: FCDS] | ||
|
|
||
| // The resource_names field in DiscoveryRequest specifies a route configuration. | ||
| // This allows an Envoy configuration with multiple HTTP listeners (and | ||
| // associated HTTP connection manager filters) to use different route | ||
| // configurations. Each listener will bind its HTTP connection manager filter to | ||
| // a route table via this identifier. | ||
| service FcdsDiscoveryService { | ||
|
There was a problem hiding this comment. Do we actually need this RPC service? I think we've agreed to move the xDS API to a model where new resource types can be added and used via ADS without having to add a new RPC service for each one. There was a problem hiding this comment. Hi, @markdroth Thanks for the comment. This is new to me; I was not aware. As I am not sure about this, tagging @adisuissa to comment. Also, wondering what is the downside of having new RPC APIs for this new XDS service. In case we don't have this, can u help me understand how we hook the FCDS back-end logic to the ADS RPC APIs? There was a problem hiding this comment. Hi @markdroth @adisuissa Plz suggest if it is still acceptable to have these separate RPC endpoint for FCDS. P.S. I resolved couple of bugs and handling another one on the draining (only the filter chains, without recreating a new listener) sequence now. I am testing off the file inotify way now, and should get to the grpc transport next. There was a problem hiding this comment. From the wire protocol perspective, ADS already supports this; all you need to do is use the new resource type in the ADS stream. I don't know how this would work from the perspective of Envoy's existing ADS implementation, although I would expect that this would get much easier once Envoy implements the xDS caching layer described in #13009. @adisuissa can advise further as to how to actually implement this today. |
||
| option (envoy.annotations.resource).type = "envoy.config.litener.v3.FilterChain"; | ||
|
|
||
| rpc StreamFilterChains(stream discovery.v3.DiscoveryRequest) | ||
| returns (stream discovery.v3.DiscoveryResponse) { | ||
| } | ||
|
|
||
| rpc DeltaFilterChains(stream discovery.v3.DeltaDiscoveryRequest) | ||
|
There was a problem hiding this comment. Not yet implemented in this PR. There was a problem hiding this comment. Yes @adisuissa Delta is not implemented in this PR. I am working on it and will submit using a subsequent PR. Hope that works. |
||
| returns (stream discovery.v3.DeltaDiscoveryResponse) { | ||
| } | ||
|
|
||
| rpc FetchFilterChains(discovery.v3.DiscoveryRequest) returns (discovery.v3.DiscoveryResponse) { | ||
| option (google.api.http).post = "/v3/discovery:filterchains"; | ||
| option (google.api.http).body = "*"; | ||
| } | ||
| } | ||
|
|
||
| // [#not-implemented-hide:] Not configuration. Workaround c++ protobuf issue with importing | ||
| // services: https://github.com/google/protobuf/issues/4221 and protoxform to upgrade the file. | ||
| message FcdsDummy { | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,169 @@ | ||
| #include <chrono> | ||
| #include <cstdint> | ||
| #include <memory> | ||
| #include <string> | ||
|
|
||
| #include "envoy/admin/v3/config_dump.pb.h" | ||
| #include "envoy/config/core/v3/config_source.pb.h" | ||
| #include "envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.pb.h" | ||
| #include "envoy/service/discovery/v3/discovery.pb.h" | ||
|
|
||
| #include "source/common/common/assert.h" | ||
| #include "source/common/common/fmt.h" | ||
| #include "source/common/config/api_version.h" | ||
| #include "source/common/config/utility.h" | ||
| #include "source/common/http/header_map_impl.h" | ||
| #include "source/common/protobuf/utility.h" | ||
| #include "source/server/fcds_impl.h" | ||
| #include "source/server/filter_chain_manager_impl.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Server { | ||
|
|
||
| FcdsApi::FcdsApi( | ||
| const envoy::config::listener::v3::Fcds& fcds, | ||
| Configuration::FactoryContext& parent_context, | ||
| Init::Manager& init_manager, | ||
| FilterChainManagerImpl& filter_chain_manager, | ||
| FilterChainFactoryBuilder* fc_builder) | ||
| : Envoy::Config::SubscriptionBase<envoy::config::listener::v3::FilterChain>( | ||
| parent_context.messageValidationContext().dynamicValidationVisitor(), "name"), | ||
| fcds_name_(fcds.fcds_name()), | ||
| parent_init_target_(fmt::format("FcdsConfigSubscription parent_init_target {}", fcds_name_), | ||
| [this]() { local_init_manager_.initialize(local_init_watcher_); }), | ||
| local_init_watcher_(fmt::format("Fcds local_init_watcher {}", fcds.fcds_name()), | ||
| [this]() { parent_init_target_.ready(); }), | ||
| local_init_target_( | ||
| fmt::format("FcdsConfigSubscription local_init_target {}", fcds_name_), | ||
| [this]() { fcds_subscription_->start({fcds_name_}); }), | ||
| local_init_manager_(fmt::format("FCDS local_init_manager {}", fcds_name_)), | ||
| init_manager_(init_manager), | ||
| cluster_manager_(parent_context.clusterManager()), | ||
| scope_(parent_context.listenerScope().createScope("")), | ||
| filter_chain_manager_(filter_chain_manager), | ||
| fc_builder_(fc_builder) | ||
| { | ||
|
|
||
| const auto resource_name = getResourceName(); | ||
|
|
||
| fcds_subscription_ = | ||
| cluster_manager_.subscriptionFactory().subscriptionFromConfigSource( | ||
| fcds.config_source(), Grpc::Common::typeUrl(resource_name), *scope_, *this, | ||
| resource_decoder_, {}); | ||
|
|
||
| } | ||
|
|
||
|
|
||
| FcdsApi::~FcdsApi() | ||
| { | ||
| // If we get destroyed during initialization, make sure we signal that we "initialized". | ||
| local_init_target_.ready(); | ||
| } | ||
|
|
||
| void FcdsApi::initialize() | ||
| { | ||
| local_init_manager_.add(local_init_target_); | ||
| init_manager_.add(parent_init_target_); | ||
| } | ||
|
|
||
| void FcdsApi::onConfigUpdate( | ||
| const std::vector<Envoy::Config::DecodedResourceRef>& resources, | ||
| const std::string& version_info) { | ||
|
|
||
| if (!validateUpdateSize(resources.size())) { | ||
| return; | ||
| } | ||
|
|
||
| // Construct the list of filter chains to be removed | ||
| std::unordered_set<std::string> fc_to_remove; | ||
| auto fc_name_to_resource_map = filter_chain_manager_.getFcdsResources(); | ||
| auto itr = fc_name_to_resource_map.begin(); | ||
| while (itr != fc_name_to_resource_map.end()) { | ||
| fc_to_remove.insert(itr->first); | ||
| ++itr; | ||
| } | ||
|
|
||
| // Remove the list of FCs in the incoming FCDS config from the above list | ||
| for (const auto& filter_chain : resources) | ||
| { | ||
| fc_to_remove.erase(filter_chain.get().name()); | ||
| } | ||
|
|
||
| Protobuf::RepeatedPtrField<std::string> to_be_deleted_fc_list; | ||
| for (const auto& filter_chain : fc_to_remove) { | ||
| *to_be_deleted_fc_list.Add() = filter_chain; | ||
| } | ||
|
|
||
| // Delete the old fcs and add the new fcs | ||
| onConfigUpdate(resources, to_be_deleted_fc_list, version_info); | ||
| } | ||
|
|
||
|
|
||
| void FcdsApi::onConfigUpdate( | ||
| const std::vector<Envoy::Config::DecodedResourceRef>& added_resources, | ||
| const Protobuf::RepeatedPtrField<std::string>& removed_resources, const std::string&) { | ||
|
|
||
| // Get all the existing filter chains | ||
| auto& fc_name_to_resource_map = filter_chain_manager_.getFcdsResources(); | ||
|
|
||
| // Delete the filter chains which are missing in this config update | ||
| std::vector<envoy::config::listener::v3::FilterChain> fc_to_del_vector; | ||
|
|
||
| for (const std::string& fc_name : removed_resources) | ||
| { | ||
| ENVOY_LOG(debug, "fcds: removing filter chain from FCM: fcds name = {} resource={}", fcds_name_, fc_name); | ||
| auto itr = fc_name_to_resource_map.find(fc_name); | ||
| if (itr != fc_name_to_resource_map.end()) { | ||
| const envoy::config::listener::v3::FilterChain& fc = itr->second; | ||
| fc_to_del_vector.push_back(fc); | ||
| } | ||
| } | ||
|
|
||
| if (fc_to_del_vector.size()) { | ||
| filter_chain_manager_.removeFilterChains(fc_to_del_vector); | ||
| } | ||
|
|
||
| // Add/Update the filter chains which are part of the new update | ||
| std::vector<const envoy::config::listener::v3::FilterChain*> fc_to_add_vector; | ||
| for (const auto& fcds_resource : added_resources) | ||
| { | ||
| envoy::config::listener::v3::FilterChain fc_config; | ||
| fc_config = dynamic_cast<const envoy::config::listener::v3::FilterChain&>(fcds_resource.get().resource()); | ||
|
|
||
| if (fc_config.name().size() == 0) { | ||
| ENVOY_LOG(debug, "fcds: skipping this filter chain config update due to missing filter chain name, resource_name={}", fc_config.name()); | ||
| continue; | ||
| } | ||
| fc_to_add_vector.clear(); | ||
| fc_to_add_vector.push_back(&fc_config); | ||
|
|
||
| ENVOY_LOG(debug, "fcds: adding filter chain: fcds name = {} resource_name={}", fcds_name_, fc_config.name()); | ||
| filter_chain_manager_.addFilterChains(fc_to_add_vector, nullptr, *fc_builder_, filter_chain_manager_, true); | ||
| } | ||
|
|
||
| local_init_target_.ready(); | ||
| } | ||
|
|
||
| void FcdsApi::onConfigUpdateFailed( | ||
| Envoy::Config::ConfigUpdateFailureReason reason, const EnvoyException*) { | ||
| ASSERT(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure != reason); | ||
| // We need to allow server startup to continue, even if we have a bad | ||
| // config. | ||
| local_init_target_.ready(); | ||
| } | ||
|
|
||
| bool FcdsApi::validateUpdateSize(int num_resources) { | ||
| if (num_resources == 0) { | ||
| ENVOY_LOG(debug, "Missing Filter Chain Configuration for {} in onConfigUpdate()", fcds_name_); | ||
| local_init_target_.ready(); | ||
| return false; | ||
| } | ||
|
|
||
| ENVOY_LOG(debug, "fcds: recived {} filter chain for fcds {}", num_resources, fcds_name_); | ||
| return true; | ||
| } | ||
|
|
||
| } // namespace Server | ||
| } // 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.
I think this needs to be promoted to oneof with
filter_chains, and clearly comment that they are mutual exclusive.Also, is this going to return a single filter-chain? Maybe it should return a glob collection of filter-chains.
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.
I tried oneof when I started implementing this. Had some issues that I can't recall. Will give it another try and update this PR as needed.
FCDS block would encompass a collection of filter chains., which are under a listener Plz refer to the yaml samples in the PR description.
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.
Changing to oneof breaks backwards compatibility. This should be promoted in future major version, and clearly stated in the comments.
Looking here it implies that the returned type is a single FilterChain object for each request, not multiple ones. I think this should be a Glob collection and return a list of such elements.
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.
Hi @adisuissa
I gave it a thought. I want to represent a single resource as a filter chain, not a list of filter chains. I think the name that I added in the proto, created confusion.
In that case, much like LDS proto, I believe the resource type not being a list is fine. Is my understanding correct?
However, the challenge here is, how could a listener subscribe to a specific list of filter chain updates. Seems like thats a challenge that glob collection will solve? Is that the reason you suggested using glob collection?
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.
@adisuissa - Can you please show an example how to promote a proto change to a "Future major version"?