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.

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

thrift_proxy: move protobuf to api/config hierarchy #3963

Merged
merged 1 commit into from
Jul 27, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
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
3 changes: 3 additions & 0 deletions api/STYLE.md
Original file line number Diff line number Diff line change
Expand Up @@ -131,3 +131,6 @@ the build system to prevent circular dependency formation. Package group
`//envoy/api/v2:friends` selects consumers of the core API package (services and configs)
and is the default visibility for the core API packages. The default visibility
for services and configs should be `//docs` (proto documentation tool).

Extensions should use the regular hierarchy. For example, configuration for network filters belongs
in a package under `envoy.config.filter.network`.
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
syntax = "proto3";

package envoy.extensions.filters.network.thrift_proxy.v2alpha1;
package envoy.config.filter.network.thrift_proxy.v2alpha1;
option go_package = "v2";

import "validate/validate.proto";
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
syntax = "proto3";

package envoy.extensions.filters.network.thrift_proxy.v2alpha1.router;
package envoy.config.filter.network.thrift_proxy.v2alpha1.router;
option go_package = "router";

// [#protodoc-title: Thrift Router]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
syntax = "proto3";

package envoy.extensions.filters.network.thrift_proxy.v2alpha1;
package envoy.config.filter.network.thrift_proxy.v2alpha1;
option go_package = "v2";

import "envoy/extensions/filters/network/thrift_proxy/v2alpha1/route.proto";
import "envoy/config/filter/network/thrift_proxy/v2alpha1/route.proto";

import "validate/validate.proto";
import "gogoproto/gogo.proto";
Expand Down
1 change: 1 addition & 0 deletions docs/root/intro/version_history.rst
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ Version history
* router: added ability to set request/response headers at the :ref:`envoy_api_msg_route.Route` level.
* tracing: added support for configuration of :ref:`tracing sampling
<envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.tracing>`.
* thrift_proxy: introduced thrift routing, moved configuration to correct location
* upstream: added configuration option to the subset load balancer to take locality weights into account when
selecting a host from a subset.
* access log: added RESPONSE_DURATION and RESPONSE_TX_DURATION.
Expand Down
2 changes: 1 addition & 1 deletion source/extensions/filters/network/thrift_proxy/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ envoy_cc_library(
"//source/extensions/filters/network/thrift_proxy/filters:filter_config_interface",
"//source/extensions/filters/network/thrift_proxy/filters:well_known_names",
"//source/extensions/filters/network/thrift_proxy/router:router_lib",
"@envoy_api//envoy/extensions/filters/network/thrift_proxy/v2alpha1:thrift_proxy_cc",
"@envoy_api//envoy/config/filter/network/thrift_proxy/v2alpha1:thrift_proxy_cc",
],
)

Expand Down
66 changes: 32 additions & 34 deletions source/extensions/filters/network/thrift_proxy/config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,52 +25,50 @@ namespace NetworkFilters {
namespace ThriftProxy {
namespace {

typedef std::map<
envoy::extensions::filters::network::thrift_proxy::v2alpha1::ThriftProxy_TransportType,
TransportType>
typedef std::map<envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy_TransportType,
TransportType>
TransportTypeMap;

static const TransportTypeMap& transportTypeMap() {
CONSTRUCT_ON_FIRST_USE(TransportTypeMap,
{
{envoy::extensions::filters::network::thrift_proxy::v2alpha1::
ThriftProxy_TransportType_AUTO_TRANSPORT,
TransportType::Auto},
{envoy::extensions::filters::network::thrift_proxy::v2alpha1::
ThriftProxy_TransportType_FRAMED,
TransportType::Framed},
{envoy::extensions::filters::network::thrift_proxy::v2alpha1::
ThriftProxy_TransportType_UNFRAMED,
TransportType::Unframed},
});
CONSTRUCT_ON_FIRST_USE(
TransportTypeMap,
{
{envoy::config::filter::network::thrift_proxy::v2alpha1::
ThriftProxy_TransportType_AUTO_TRANSPORT,
TransportType::Auto},
{envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy_TransportType_FRAMED,
TransportType::Framed},
{envoy::config::filter::network::thrift_proxy::v2alpha1::
ThriftProxy_TransportType_UNFRAMED,
TransportType::Unframed},
});
}

typedef std::map<
envoy::extensions::filters::network::thrift_proxy::v2alpha1::ThriftProxy_ProtocolType,
ProtocolType>
typedef std::map<envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy_ProtocolType,
ProtocolType>
ProtocolTypeMap;

static const ProtocolTypeMap& protocolTypeMap() {
CONSTRUCT_ON_FIRST_USE(ProtocolTypeMap, {
{envoy::extensions::filters::network::thrift_proxy::
v2alpha1::ThriftProxy_ProtocolType_AUTO_PROTOCOL,
ProtocolType::Auto},
{envoy::extensions::filters::network::thrift_proxy::
v2alpha1::ThriftProxy_ProtocolType_BINARY,
ProtocolType::Binary},
{envoy::extensions::filters::network::thrift_proxy::
v2alpha1::ThriftProxy_ProtocolType_LAX_BINARY,
ProtocolType::LaxBinary},
{envoy::extensions::filters::network::thrift_proxy::
v2alpha1::ThriftProxy_ProtocolType_COMPACT,
ProtocolType::Compact},
});
CONSTRUCT_ON_FIRST_USE(
ProtocolTypeMap,
{
{envoy::config::filter::network::thrift_proxy::v2alpha1::
ThriftProxy_ProtocolType_AUTO_PROTOCOL,
ProtocolType::Auto},
{envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy_ProtocolType_BINARY,
ProtocolType::Binary},
{envoy::config::filter::network::thrift_proxy::v2alpha1::
ThriftProxy_ProtocolType_LAX_BINARY,
ProtocolType::LaxBinary},
{envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy_ProtocolType_COMPACT,
ProtocolType::Compact},
});
}

} // namespace

Network::FilterFactoryCb ThriftProxyFilterConfigFactory::createFilterFactoryFromProtoTyped(
const envoy::extensions::filters::network::thrift_proxy::v2alpha1::ThriftProxy& proto_config,
const envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy& proto_config,
Server::Configuration::FactoryContext& context) {
std::shared_ptr<Config> filter_config(new ConfigImpl(proto_config, context));

Expand All @@ -87,7 +85,7 @@ static Registry::RegisterFactory<ThriftProxyFilterConfigFactory,
registered_;

ConfigImpl::ConfigImpl(
const envoy::extensions::filters::network::thrift_proxy::v2alpha1::ThriftProxy& config,
const envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy& config,
Server::Configuration::FactoryContext& context)
: context_(context), stats_prefix_(fmt::format("thrift.{}.", config.stat_prefix())),
stats_(ThriftFilterStats::generateStats(stats_prefix_, context_.scope())),
Expand Down
14 changes: 7 additions & 7 deletions source/extensions/filters/network/thrift_proxy/config.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
#include <map>
#include <string>

#include "envoy/extensions/filters/network/thrift_proxy/v2alpha1/thrift_proxy.pb.h"
#include "envoy/extensions/filters/network/thrift_proxy/v2alpha1/thrift_proxy.pb.validate.h"
#include "envoy/config/filter/network/thrift_proxy/v2alpha1/thrift_proxy.pb.h"
#include "envoy/config/filter/network/thrift_proxy/v2alpha1/thrift_proxy.pb.validate.h"
#include "envoy/stats/stats.h"

#include "extensions/filters/network/common/factory_base.h"
Expand All @@ -23,13 +23,13 @@ namespace ThriftProxy {
*/
class ThriftProxyFilterConfigFactory
: public Common::FactoryBase<
envoy::extensions::filters::network::thrift_proxy::v2alpha1::ThriftProxy> {
envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy> {
public:
ThriftProxyFilterConfigFactory() : FactoryBase(NetworkFilterNames::get().ThriftProxy) {}

private:
Network::FilterFactoryCb createFilterFactoryFromProtoTyped(
const envoy::extensions::filters::network::thrift_proxy::v2alpha1::ThriftProxy& proto_config,
const envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy& proto_config,
Server::Configuration::FactoryContext& context) override;
};

Expand All @@ -38,7 +38,7 @@ class ConfigImpl : public Config,
public ThriftFilters::FilterChainFactory,
Logger::Loggable<Logger::Id::config> {
public:
ConfigImpl(const envoy::extensions::filters::network::thrift_proxy::v2alpha1::ThriftProxy& config,
ConfigImpl(const envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy& config,
Server::Configuration::FactoryContext& context);

// ThriftFilters::FilterChainFactory
Expand All @@ -62,8 +62,8 @@ class ConfigImpl : public Config,
Server::Configuration::FactoryContext& context_;
const std::string stats_prefix_;
ThriftFilterStats stats_;
envoy::extensions::filters::network::thrift_proxy::v2alpha1::ThriftProxy_TransportType transport_;
envoy::extensions::filters::network::thrift_proxy::v2alpha1::ThriftProxy_ProtocolType proto_;
envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy_TransportType transport_;
envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy_ProtocolType proto_;
std::unique_ptr<Router::RouteMatcher> route_matcher_;

std::list<ThriftFilters::FilterFactoryCb> filter_factories_;
Expand Down
4 changes: 2 additions & 2 deletions source/extensions/filters/network/thrift_proxy/router/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ envoy_cc_library(
"//source/extensions/filters/network/thrift_proxy/filters:factory_base_lib",
"//source/extensions/filters/network/thrift_proxy/filters:filter_config_interface",
"//source/extensions/filters/network/thrift_proxy/filters:well_known_names",
"@envoy_api//envoy/extensions/filters/network/thrift_proxy/v2alpha1/router:router_cc",
"@envoy_api//envoy/config/filter/network/thrift_proxy/v2alpha1/router:router_cc",
],
)

Expand Down Expand Up @@ -46,6 +46,6 @@ envoy_cc_library(
"//source/extensions/filters/network/thrift_proxy:protocol_lib",
"//source/extensions/filters/network/thrift_proxy:transport_lib",
"//source/extensions/filters/network/thrift_proxy/filters:filter_interface",
"@envoy_api//envoy/extensions/filters/network/thrift_proxy/v2alpha1:thrift_proxy_cc",
"@envoy_api//envoy/config/filter/network/thrift_proxy/v2alpha1:thrift_proxy_cc",
],
)
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace ThriftProxy {
namespace Router {

ThriftFilters::FilterFactoryCb RouterFilterConfig::createFilterFactoryFromProtoTyped(
const envoy::extensions::filters::network::thrift_proxy::v2alpha1::router::Router& proto_config,
const envoy::config::filter::network::thrift_proxy::v2alpha1::router::Router& proto_config,
const std::string& stat_prefix, Server::Configuration::FactoryContext& context) {
UNREFERENCED_PARAMETER(proto_config);
UNREFERENCED_PARAMETER(stat_prefix);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#pragma once

#include "envoy/extensions/filters/network/thrift_proxy/v2alpha1/router/router.pb.h"
#include "envoy/extensions/filters/network/thrift_proxy/v2alpha1/router/router.pb.validate.h"
#include "envoy/config/filter/network/thrift_proxy/v2alpha1/router/router.pb.h"
#include "envoy/config/filter/network/thrift_proxy/v2alpha1/router/router.pb.validate.h"

#include "extensions/filters/network/thrift_proxy/filters/factory_base.h"
#include "extensions/filters/network/thrift_proxy/filters/well_known_names.h"
Expand All @@ -14,14 +14,13 @@ namespace Router {

class RouterFilterConfig
: public ThriftFilters::FactoryBase<
envoy::extensions::filters::network::thrift_proxy::v2alpha1::router::Router> {
envoy::config::filter::network::thrift_proxy::v2alpha1::router::Router> {
public:
RouterFilterConfig() : FactoryBase(ThriftFilters::ThriftFilterNames::get().ROUTER) {}

private:
ThriftFilters::FilterFactoryCb createFilterFactoryFromProtoTyped(
const envoy::extensions::filters::network::thrift_proxy::v2alpha1::router::Router&
proto_config,
const envoy::config::filter::network::thrift_proxy::v2alpha1::router::Router& proto_config,
const std::string& stat_prefix, Server::Configuration::FactoryContext& context) override;
};

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#include "extensions/filters/network/thrift_proxy/router/router_impl.h"

#include "envoy/extensions/filters/network/thrift_proxy/v2alpha1/thrift_proxy.pb.h"
#include "envoy/config/filter/network/thrift_proxy/v2alpha1/thrift_proxy.pb.h"
#include "envoy/upstream/cluster_manager.h"
#include "envoy/upstream/thread_local_cluster.h"

Expand All @@ -13,7 +13,7 @@ namespace ThriftProxy {
namespace Router {

RouteEntryImplBase::RouteEntryImplBase(
const envoy::extensions::filters::network::thrift_proxy::v2alpha1::Route& route)
const envoy::config::filter::network::thrift_proxy::v2alpha1::Route& route)
: cluster_name_(route.route().cluster()) {}

const std::string& RouteEntryImplBase::clusterName() const { return cluster_name_; }
Expand All @@ -23,7 +23,7 @@ const RouteEntry* RouteEntryImplBase::routeEntry() const { return this; }
RouteConstSharedPtr RouteEntryImplBase::clusterEntry() const { return shared_from_this(); }

MethodNameRouteEntryImpl::MethodNameRouteEntryImpl(
const envoy::extensions::filters::network::thrift_proxy::v2alpha1::Route& route)
const envoy::config::filter::network::thrift_proxy::v2alpha1::Route& route)
: RouteEntryImplBase(route), method_name_(route.match().method()) {}

RouteConstSharedPtr MethodNameRouteEntryImpl::matches(const std::string& method_name) const {
Expand All @@ -35,7 +35,7 @@ RouteConstSharedPtr MethodNameRouteEntryImpl::matches(const std::string& method_
}

RouteMatcher::RouteMatcher(
const envoy::extensions::filters::network::thrift_proxy::v2alpha1::RouteConfiguration& config) {
const envoy::config::filter::network::thrift_proxy::v2alpha1::RouteConfiguration& config) {
for (const auto& route : config.routes()) {
routes_.emplace_back(new MethodNameRouteEntryImpl(route));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
#include <string>
#include <vector>

#include "envoy/extensions/filters/network/thrift_proxy/v2alpha1/thrift_proxy.pb.h"
#include "envoy/config/filter/network/thrift_proxy/v2alpha1/thrift_proxy.pb.h"
#include "envoy/tcp/conn_pool.h"
#include "envoy/upstream/load_balancer.h"

Expand All @@ -26,8 +26,7 @@ class RouteEntryImplBase : public RouteEntry,
public Route,
public std::enable_shared_from_this<RouteEntryImplBase> {
public:
RouteEntryImplBase(
const envoy::extensions::filters::network::thrift_proxy::v2alpha1::Route& route);
RouteEntryImplBase(const envoy::config::filter::network::thrift_proxy::v2alpha1::Route& route);

// Router::RouteEntry
const std::string& clusterName() const override;
Expand All @@ -49,7 +48,7 @@ typedef std::shared_ptr<const RouteEntryImplBase> RouteEntryImplBaseConstSharedP
class MethodNameRouteEntryImpl : public RouteEntryImplBase {
public:
MethodNameRouteEntryImpl(
const envoy::extensions::filters::network::thrift_proxy::v2alpha1::Route& route);
const envoy::config::filter::network::thrift_proxy::v2alpha1::Route& route);

const std::string& methodName() const { return method_name_; }

Expand All @@ -62,8 +61,7 @@ class MethodNameRouteEntryImpl : public RouteEntryImplBase {

class RouteMatcher {
public:
RouteMatcher(
const envoy::extensions::filters::network::thrift_proxy::v2alpha1::RouteConfiguration&);
RouteMatcher(const envoy::config::filter::network::thrift_proxy::v2alpha1::RouteConfiguration&);

RouteConstSharedPtr route(const std::string& method_name) const;

Expand Down
15 changes: 7 additions & 8 deletions test/extensions/filters/network/thrift_proxy/config_test.cc
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#include "envoy/extensions/filters/network/thrift_proxy/v2alpha1/thrift_proxy.pb.validate.h"
#include "envoy/config/filter/network/thrift_proxy/v2alpha1/thrift_proxy.pb.validate.h"

#include "extensions/filters/network/thrift_proxy/config.h"

Expand All @@ -16,14 +16,13 @@ namespace ThriftProxy {

TEST(ThriftFilterConfigTest, ValidateFail) {
NiceMock<Server::Configuration::MockFactoryContext> context;
EXPECT_THROW(
ThriftProxyFilterConfigFactory().createFilterFactoryFromProto(
envoy::extensions::filters::network::thrift_proxy::v2alpha1::ThriftProxy(), context),
ProtoValidationException);
EXPECT_THROW(ThriftProxyFilterConfigFactory().createFilterFactoryFromProto(
envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy(), context),
ProtoValidationException);
}

TEST(ThriftFilterConfigTest, ValidProtoConfiguration) {
envoy::extensions::filters::network::thrift_proxy::v2alpha1::ThriftProxy config{};
envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy config{};

config.set_stat_prefix("my_stat_prefix");

Expand All @@ -38,8 +37,8 @@ TEST(ThriftFilterConfigTest, ValidProtoConfiguration) {
TEST(ThriftFilterConfigTest, ThriftProxyWithEmptyProto) {
NiceMock<Server::Configuration::MockFactoryContext> context;
ThriftProxyFilterConfigFactory factory;
envoy::extensions::filters::network::thrift_proxy::v2alpha1::ThriftProxy config =
*dynamic_cast<envoy::extensions::filters::network::thrift_proxy::v2alpha1::ThriftProxy*>(
envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy config =
*dynamic_cast<envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy*>(
factory.createEmptyConfigProto().get());
config.set_stat_prefix("my_stat_prefix");

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#include "envoy/extensions/filters/network/thrift_proxy/v2alpha1/thrift_proxy.pb.h"
#include "envoy/config/filter/network/thrift_proxy/v2alpha1/thrift_proxy.pb.h"

#include "common/buffer/buffer_impl.h"
#include "common/stats/stats_impl.h"
Expand Down Expand Up @@ -30,10 +30,9 @@ namespace ThriftProxy {

class TestConfigImpl : public ConfigImpl {
public:
TestConfigImpl(
envoy::extensions::filters::network::thrift_proxy::v2alpha1::ThriftProxy proto_config,
Server::Configuration::MockFactoryContext& context,
ThriftFilters::DecoderFilterSharedPtr decoder_filter, ThriftFilterStats& stats)
TestConfigImpl(envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy proto_config,
Server::Configuration::MockFactoryContext& context,
ThriftFilters::DecoderFilterSharedPtr decoder_filter, ThriftFilterStats& stats)
: ConfigImpl(proto_config, context), decoder_filter_(decoder_filter), stats_(stats) {}

// ConfigImpl
Expand Down Expand Up @@ -236,7 +235,7 @@ class ThriftConnectionManagerTest : public testing::Test {
std::shared_ptr<ThriftFilters::MockDecoderFilter> decoder_filter_;
Stats::IsolatedStoreImpl store_;
ThriftFilterStats stats_;
envoy::extensions::filters::network::thrift_proxy::v2alpha1::ThriftProxy proto_config_;
envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy proto_config_;

std::unique_ptr<TestConfigImpl> config_;

Expand Down
Loading