Skip to content

Commit

Permalink
thrift_proxy: move protobuf to api/config hierarchy (#3963)
Browse files Browse the repository at this point in the history
As discussed in Slack, these should have been under api/config/filter/network
to begin with. Added a note to the style doc to make this clear in the
future.

*Risk level*: low (rename only)
*Testing*: existing tests suffice
*Doc Changes*: n/a
*Release Notes*: updated

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
  • Loading branch information
zuercher authored and htuch committed Jul 27, 2018
1 parent 0e2c795 commit 1dfde38
Show file tree
Hide file tree
Showing 19 changed files with 81 additions and 84 deletions.
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 @@ -48,6 +48,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

0 comments on commit 1dfde38

Please sign in to comment.