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

INCOMPLETE: upstream: add filter chains to upstream connections (#173) #3571

Closed
wants to merge 1 commit into from
Closed

INCOMPLETE: upstream: add filter chains to upstream connections (#173) #3571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 @@ -72,6 +72,7 @@ api_proto_library(
"//envoy/api/v2/core:health_check",
"//envoy/api/v2/core:protocol",
"//envoy/api/v2/endpoint",
"//envoy/api/v2/listener", # TODO(alanconway): for Filter definition
"//envoy/type:percent",
],
)
Expand Down
6 changes: 6 additions & 0 deletions api/envoy/api/v2/cds.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import "envoy/api/v2/core/config_source.proto";
import "envoy/api/v2/discovery.proto";
import "envoy/api/v2/core/health_check.proto";
import "envoy/api/v2/core/protocol.proto";
import "envoy/api/v2/listener/listener.proto"; // TODO(alanconway): for Filter definition
import "envoy/api/v2/cluster/circuit_breaker.proto";
import "envoy/api/v2/cluster/outlier_detection.proto";
import "envoy/api/v2/eds.proto";
Expand Down Expand Up @@ -468,6 +469,11 @@ message Cluster {
// If this flag is not set to true, Envoy will wait until the hosts fail active health
// checking before removing it from the cluster.
bool drain_connections_on_host_removal = 32;

// An optional list of network filters that make up the filter chain for
// outgoing connections made by the cluster. Order matters as the filters are
// processed sequentially as connection events happen.
repeated listener.Filter filters = 34 [(gogoproto.nullable) = false];
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that that type should probably be moved to core. I don't know if such a change breaks protobuf compatibility or not; we'll need to sort that out before making a change.

Copy link
Author

Choose a reason for hiding this comment

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

I started to do it and then backed off till I got feedback. I'll resurrect it for review. On the -dev list @mattklein123 seemed to feel it wouldn't be a breaking change but best put it in code first.

Copy link
Member

Choose a reason for hiding this comment

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

This type of change will cause a compile time breakage, but should not cause a wire breakage. I think it's worth it in this case. @alanconway can you make this change and then I can take a look?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please also test that the canonical-json-protobuf is compatible after the change.

Copy link
Author

Choose a reason for hiding this comment

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

@ggreenway can you point to any existing tests for that kind of compatibility? I have yet to dive into the envoy test suite, I've been testing it from the outside so far.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know of any, but all I was thinking was to write a config that current-envoy will load that includes these bits, and add a test this config loads (and that the relevant bits seem to take effect).

}

// An extensible structure containing the address Envoy should bind to when
Expand Down
5 changes: 5 additions & 0 deletions include/envoy/upstream/upstream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,11 @@ class ClusterInfo {
* after a host is removed from service discovery.
*/
virtual bool drainConnectionsOnHostRemoval() const PURE;

/**
* Create network filters on a new upstream connection.
*/
virtual void createNetworkFilters(Network::Connection& connection) const PURE;
};

typedef std::shared_ptr<const ClusterInfo> ClusterInfoConstSharedPtr;
Expand Down
2 changes: 2 additions & 0 deletions source/common/upstream/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,7 @@ envoy_cc_library(
"//include/envoy/network:dns_interface",
"//include/envoy/runtime:runtime_interface",
"//include/envoy/server:transport_socket_config_interface",
"//include/envoy/server:filter_config_interface",
"//include/envoy/ssl:context_manager_interface",
"//include/envoy/thread_local:thread_local_interface",
"//include/envoy/upstream:cluster_manager_interface",
Expand All @@ -373,6 +374,7 @@ envoy_cc_library(
"//source/common/config:metadata_lib",
"//source/common/stats:stats_lib",
"//source/common/upstream:locality_lib",
"//source/server:configuration_lib", # TODO(alanconway): bad dependency
"@envoy_api//envoy/api/v2/core:base_cc",
"@envoy_api//envoy/api/v2/endpoint:endpoint_cc",
],
Expand Down
51 changes: 51 additions & 0 deletions source/common/upstream/upstream_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "envoy/event/timer.h"
#include "envoy/network/dns.h"
#include "envoy/server/transport_socket_config.h"
#include "envoy/server/filter_config.h"
#include "envoy/ssl/context_manager.h"
#include "envoy/upstream/health_checker.h"

Expand All @@ -33,6 +34,7 @@
#include "common/upstream/original_dst_cluster.h"

#include "extensions/transport_sockets/well_known_names.h"
#include "server/configuration_impl.h" // TODO(alanconway): bad dependency

namespace Envoy {
namespace Upstream {
Expand Down Expand Up @@ -135,6 +137,7 @@ HostImpl::createConnection(Event::Dispatcher& dispatcher, const ClusterInfo& clu
address, cluster.sourceAddress(), cluster.transportSocketFactory().createTransportSocket(),
connection_options);
connection->setBufferLimits(cluster.perConnectionBufferLimitBytes());
cluster.createNetworkFilters(*connection);
return connection;
}

Expand Down Expand Up @@ -253,6 +256,30 @@ ClusterLoadReportStats ClusterInfoImpl::generateLoadReportStats(Stats::Scope& sc
return {ALL_CLUSTER_LOAD_REPORT_STATS(POOL_COUNTER(scope))};
}

// TODO(alanconway): dummy factory context, how do we implement this? Some of
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 the only fields that are listener-specific are listenerScope(), listenerMetadata(), and possibly drainDecision().

listenerScope() is only used in a few places in the code-base. We may be able to leave it NOT_IMPLEMENTED here, or maybe rename it to ownerScope() or something more generic, and pass it the cluster's scope.

listenerMetadata() isn't called anywhere except one test. Can it be deleted? Or am I missing something?

// the methods are listener specific, should they throw, return null values, return
// values derived from the Cluster or be moved/removed?
class ClusterInfoImpl::FactoryContextImpl : public Server::Configuration::FactoryContext {
public:
AccessLog::AccessLogManager& accessLogManager() override { NOT_IMPLEMENTED; }
Upstream::ClusterManager& clusterManager() override { NOT_IMPLEMENTED; }
Event::Dispatcher& dispatcher() override { NOT_IMPLEMENTED; }
bool healthCheckFailed() override { NOT_IMPLEMENTED; }
Tracing::HttpTracer& httpTracer() override { NOT_IMPLEMENTED; }
const LocalInfo::LocalInfo& localInfo() const override { NOT_IMPLEMENTED; }
Envoy::Runtime::RandomGenerator& random() override { NOT_IMPLEMENTED; }
RateLimit::ClientPtr rateLimitClient(const absl::optional<std::chrono::milliseconds>&) override { NOT_IMPLEMENTED; }
Envoy::Runtime::Loader& runtime() override { NOT_IMPLEMENTED; }
Singleton::Manager& singletonManager() override { NOT_IMPLEMENTED; }
ThreadLocal::Instance& threadLocal() override { NOT_IMPLEMENTED; }
Server::Admin& admin() override { NOT_IMPLEMENTED; }
Init::Manager& initManager() override { NOT_IMPLEMENTED; }
Stats::Scope& listenerScope() override { NOT_IMPLEMENTED; }
Stats::Scope& scope() override { NOT_IMPLEMENTED; }
const envoy::api::v2::core::Metadata& listenerMetadata() const override { NOT_IMPLEMENTED; }
Network::DrainDecision& drainDecision() override { NOT_IMPLEMENTED; }
};

ClusterInfoImpl::ClusterInfoImpl(const envoy::api::v2::Cluster& config,
const envoy::api::v2::core::BindConfig& bind_config,
Runtime::Loader& runtime, Stats::Store& stats,
Expand Down Expand Up @@ -340,6 +367,30 @@ ClusterInfoImpl::ClusterInfoImpl(const envoy::api::v2::Cluster& config,
idle_timeout_ = std::chrono::milliseconds(
DurationUtil::durationToMilliseconds(config.common_http_protocol_options().idle_timeout()));
}

auto filters = config.filters();
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks very similar to the code in listemer_manager_impl. Dedupe?

Copy link
Author

Choose a reason for hiding this comment

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

Yep, cut-and-paste. It should be pulled up to a common place. Thought I had a TODO on that one, well spotted.

for (ssize_t i = 0; i < filters.size(); i++) {
const auto& proto_config = filters[i];
const ProtobufTypes::String name = proto_config.name();
const Json::ObjectSharedPtr filter_config =
MessageUtil::getJsonObjectFromMessage(proto_config.config());
ENVOY_LOG(debug, "filter #{} name: {} config: {}", i, name, filter_config->asJsonString());
// Now see if there is a factory that will accept the config.
auto& factory =
Config::Utility::getAndCheckFactory<Server::Configuration::NamedNetworkFilterConfigFactory>(name);
Network::FilterFactoryCb callback;
if (filter_config->getBoolean("deprecated_v1", false)) {
callback = factory.createFilterFactory(*filter_config->getObject("value", true), *factory_context_);
} else {
auto message = Config::Utility::translateToFactoryConfig(proto_config, factory);
callback = factory.createFilterFactoryFromProto(*message, *factory_context_);
}
filter_factories_.push_back(callback);
}
}

void ClusterInfoImpl::createNetworkFilters(Network::Connection& connection) const {
Server::Configuration::FilterChainUtility::buildFilterChain(connection, filter_factories_);
}

ClusterSharedPtr ClusterImplBase::create(const envoy::api::v2::Cluster& cluster, ClusterManager& cm,
Expand Down
11 changes: 10 additions & 1 deletion source/common/upstream/upstream_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
#include "envoy/event/timer.h"
#include "envoy/local_info/local_info.h"
#include "envoy/network/dns.h"

#include "envoy/network/filter.h"
#include "envoy/runtime/runtime.h"
#include "envoy/server/transport_socket_config.h"
#include "envoy/ssl/context_manager.h"
Expand Down Expand Up @@ -307,7 +309,8 @@ class PrioritySetImpl : public PrioritySet {
* Implementation of ClusterInfo that reads from JSON.
*/
class ClusterInfoImpl : public ClusterInfo,
public Server::Configuration::TransportSocketFactoryContext {
public Server::Configuration::TransportSocketFactoryContext,
protected Logger::Loggable<Logger::Id::upstream> {
public:
ClusterInfoImpl(const envoy::api::v2::Cluster& config,
const envoy::api::v2::core::BindConfig& bind_config, Runtime::Loader& runtime,
Expand Down Expand Up @@ -362,6 +365,8 @@ class ClusterInfoImpl : public ClusterInfo,

bool drainConnectionsOnHostRemoval() const override { return drain_connections_on_host_removal_; }

void createNetworkFilters(Network::Connection&) const;

private:
struct ResourceManagers {
ResourceManagers(const envoy::api::v2::Cluster& config, Runtime::Loader& runtime,
Expand Down Expand Up @@ -401,6 +406,10 @@ class ClusterInfoImpl : public ClusterInfo,
const envoy::api::v2::Cluster::CommonLbConfig common_lb_config_;
const Network::ConnectionSocket::OptionsSharedPtr cluster_socket_options_;
const bool drain_connections_on_host_removal_;

class FactoryContextImpl;
std::unique_ptr<FactoryContextImpl> factory_context_;
std::vector<Network::FilterFactoryCb> filter_factories_;
};

/**
Expand Down