Skip to content

router: support random value for cluster specifier plugin #39979

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
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
2 changes: 1 addition & 1 deletion contrib/golang/router/cluster_specifier/source/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ envoy_cc_library(
"//envoy/router:cluster_specifier_plugin_interface",
"//source/common/common:utility_lib",
"//source/common/http:utility_lib",
"//source/common/router:config_lib",
"//source/common/router:delegating_route_lib",
"@envoy_api//contrib/envoy/extensions/router/cluster_specifier/golang/v3alpha:pkg_cc_proto",
],
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

#include <chrono>

#include "source/common/router/config_impl.h"
#include "source/common/router/delegating_route_impl.h"

namespace Envoy {
namespace Router {
Expand Down Expand Up @@ -46,7 +46,8 @@ ClusterConfig::ClusterConfig(const GolangClusterProto& config)

RouteConstSharedPtr GolangClusterSpecifierPlugin::route(RouteEntryAndRouteConstSharedPtr parent,
const Http::RequestHeaderMap& header,
const StreamInfo::StreamInfo&) const {
const StreamInfo::StreamInfo&,
uint64_t) const {
int buffer_len = 256;
std::string buffer;
std::string cluster;
Expand Down Expand Up @@ -77,7 +78,7 @@ RouteConstSharedPtr GolangClusterSpecifierPlugin::route(RouteEntryAndRouteConstS
}
}

return std::make_shared<RouteEntryImplBase::DynamicRouteEntry>(std::move(parent), cluster);
return std::make_shared<Router::DynamicRouteEntry>(std::move(parent), std::move(cluster));
}

void GolangClusterSpecifierPlugin::log(absl::string_view& msg) const {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ class GolangClusterSpecifierPlugin : public ClusterSpecifierPlugin,

RouteConstSharedPtr route(RouteEntryAndRouteConstSharedPtr parent,
const Http::RequestHeaderMap& header,
const StreamInfo::StreamInfo& stream_info) const override;
const StreamInfo::StreamInfo& stream_info,
uint64_t random) const override;

void log(absl::string_view& msg) const;

Expand Down
4 changes: 3 additions & 1 deletion envoy/router/cluster_specifier_plugin.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,13 @@ class ClusterSpecifierPlugin {
* @param parent related route.
* @param headers request headers.
* @param stream_info stream info of the downstream request.
* @param random random value for cluster selection.
* @return RouteConstSharedPtr final route with specific cluster.
*/
virtual RouteConstSharedPtr route(RouteEntryAndRouteConstSharedPtr parent,
const Http::RequestHeaderMap& headers,
const StreamInfo::StreamInfo& stream_info) const PURE;
const StreamInfo::StreamInfo& stream_info,
uint64_t random) const PURE;
};

using ClusterSpecifierPluginSharedPtr = std::shared_ptr<ClusterSpecifierPlugin>;
Expand Down
5 changes: 3 additions & 2 deletions source/common/router/config_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ getHeaderParsers(const HeaderParser* global_route_config_header_parser,
class NullClusterSpecifierPlugin : public ClusterSpecifierPlugin {
public:
RouteConstSharedPtr route(RouteEntryAndRouteConstSharedPtr, const Http::RequestHeaderMap&,
const StreamInfo::StreamInfo&) const override {
const StreamInfo::StreamInfo&, uint64_t) const override {
return nullptr;
}
};
Expand Down Expand Up @@ -1390,7 +1390,8 @@ RouteConstSharedPtr RouteEntryImplBase::clusterEntry(const Http::RequestHeaderMa
// TODO(wbpcode): make the weighted clusters an implementation of the
// cluster specifier plugin.
ASSERT(cluster_specifier_plugin_ != nullptr);
return cluster_specifier_plugin_->route(shared_from_this(), headers, stream_info);
return cluster_specifier_plugin_->route(shared_from_this(), headers, stream_info,
random_value);
}
}
return pickWeightedCluster(headers, random_value);
Expand Down
3 changes: 2 additions & 1 deletion source/common/router/header_cluster_specifier.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ HeaderClusterSpecifierPlugin::HeaderClusterSpecifierPlugin(absl::string_view clu

RouteConstSharedPtr HeaderClusterSpecifierPlugin::route(RouteEntryAndRouteConstSharedPtr parent,
const Http::RequestHeaderMap& headers,
const StreamInfo::StreamInfo&) const {
const StreamInfo::StreamInfo&,
uint64_t) const {

const auto entry = headers.get(cluster_header_);
absl::string_view cluster_name;
Expand Down
3 changes: 2 additions & 1 deletion source/common/router/header_cluster_specifier.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ class HeaderClusterSpecifierPlugin : public ClusterSpecifierPlugin {

RouteConstSharedPtr route(RouteEntryAndRouteConstSharedPtr parent,
const Http::RequestHeaderMap& headers,
const StreamInfo::StreamInfo& stream_info) const override;
const StreamInfo::StreamInfo& stream_info,
uint64_t random) const override;

private:
const Http::LowerCaseString cluster_header_;
Expand Down
1 change: 1 addition & 0 deletions source/extensions/router/cluster_specifiers/lua/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ envoy_cc_library(
"//source/common/common:utility_lib",
"//source/common/http:utility_lib",
"//source/common/router:config_lib",
"//source/common/router:delegating_route_lib",
"//source/common/runtime:runtime_features_lib",
"//source/extensions/filters/common/lua:lua_lib",
"//source/extensions/filters/common/lua:wrappers_lib",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include "source/extensions/router/cluster_specifiers/lua/lua_cluster_specifier.h"

#include "source/common/router/config_impl.h"
#include "source/common/http/header_utility.h"
#include "source/common/router/delegating_route_impl.h"

namespace Envoy {
namespace Extensions {
Expand Down Expand Up @@ -123,10 +124,10 @@ std::string LuaClusterSpecifierPlugin::startLua(const Http::HeaderMap& headers)
Envoy::Router::RouteConstSharedPtr
LuaClusterSpecifierPlugin::route(Envoy::Router::RouteEntryAndRouteConstSharedPtr parent,
const Http::RequestHeaderMap& headers,
const StreamInfo::StreamInfo&) const {
return std::make_shared<Envoy::Router::RouteEntryImplBase::DynamicRouteEntry>(std::move(parent),
startLua(headers));
const StreamInfo::StreamInfo&, uint64_t) const {
return std::make_shared<Envoy::Router::DynamicRouteEntry>(std::move(parent), startLua(headers));
}

} // namespace Lua
} // namespace Router
} // namespace Extensions
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,10 +134,9 @@ class LuaClusterSpecifierPlugin : public Envoy::Router::ClusterSpecifierPlugin,
Logger::Loggable<Logger::Id::lua> {
public:
LuaClusterSpecifierPlugin(LuaClusterSpecifierConfigSharedPtr config);
Envoy::Router::RouteConstSharedPtr
route(Envoy::Router::RouteEntryAndRouteConstSharedPtr parent,
const Http::RequestHeaderMap& header,
const StreamInfo::StreamInfo& stream_info) const override;
Envoy::Router::RouteConstSharedPtr route(Envoy::Router::RouteEntryAndRouteConstSharedPtr parent,
const Http::RequestHeaderMap& header,
const StreamInfo::StreamInfo&, uint64_t) const override;

private:
std::string startLua(const Http::HeaderMap& headers) const;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class MatcherRouteEntry : public Envoy::Router::DelegatingRouteEntry {
Envoy::Router::RouteConstSharedPtr
MatcherClusterSpecifierPlugin::route(Envoy::Router::RouteEntryAndRouteConstSharedPtr parent,
const Http::RequestHeaderMap& headers,
const StreamInfo::StreamInfo& stream_info) const {
const StreamInfo::StreamInfo& stream_info, uint64_t) const {
auto matcher_route = std::make_shared<MatcherRouteEntry>(parent, match_tree_);
matcher_route->refreshRouteCluster(headers, stream_info);
return matcher_route;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,10 @@ class MatcherClusterSpecifierPlugin : public Envoy::Router::ClusterSpecifierPlug
MatcherClusterSpecifierPlugin(
Envoy::Matcher::MatchTreeSharedPtr<Http::HttpMatchingData> match_tree)
: match_tree_(match_tree) {}
Envoy::Router::RouteConstSharedPtr
route(Envoy::Router::RouteEntryAndRouteConstSharedPtr parent,
const Http::RequestHeaderMap& headers,
const StreamInfo::StreamInfo& stream_info) const override;
Envoy::Router::RouteConstSharedPtr route(Envoy::Router::RouteEntryAndRouteConstSharedPtr parent,
const Http::RequestHeaderMap& headers,
const StreamInfo::StreamInfo& stream_info,
uint64_t) const override;

private:
Envoy::Matcher::MatchTreeSharedPtr<Http::HttpMatchingData> match_tree_;
Expand Down
4 changes: 2 additions & 2 deletions test/common/router/config_impl_integration_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ class FakeClusterSpecifierPluginFactoryConfig : public ClusterSpecifierPluginFac
FakeClusterSpecifierPlugin(absl::string_view cluster) : cluster_name_(cluster) {}

RouteConstSharedPtr route(RouteEntryAndRouteConstSharedPtr parent,
const Http::RequestHeaderMap&,
const StreamInfo::StreamInfo&) const override {
const Http::RequestHeaderMap&, const StreamInfo::StreamInfo&,
uint64_t) const override {
ASSERT(dynamic_cast<const RouteEntryImplBase*>(parent.get()) != nullptr);
return std::make_shared<RouteEntryImplBase::DynamicRouteEntry>(
dynamic_cast<const RouteEntryImplBase*>(parent.get()), parent, cluster_name_);
Expand Down
9 changes: 5 additions & 4 deletions test/common/router/config_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3669,7 +3669,7 @@ TEST_F(RouteMatcherTest, InlineClusterSpecifierPlugin) {

auto mock_route = std::make_shared<NiceMock<MockRoute>>();

EXPECT_CALL(*mock_cluster_specifier_plugin, route(_, _, _)).WillOnce(Return(mock_route));
EXPECT_CALL(*mock_cluster_specifier_plugin, route(_, _, _, _)).WillOnce(Return(mock_route));

EXPECT_EQ(mock_route.get(), config.route(genHeaders("some_cluster", "/foo", "GET"), 0).get());
}
Expand Down Expand Up @@ -3799,10 +3799,10 @@ TEST_F(RouteMatcherTest, ClusterSpecifierPlugin) {

auto mock_route = std::make_shared<NiceMock<MockRoute>>();

EXPECT_CALL(*mock_cluster_specifier_plugin_2, route(_, _, _)).WillOnce(Return(mock_route));
EXPECT_CALL(*mock_cluster_specifier_plugin_2, route(_, _, _, _)).WillOnce(Return(mock_route));
EXPECT_EQ(mock_route.get(), config.route(genHeaders("some_cluster", "/foo", "GET"), 0).get());

EXPECT_CALL(*mock_cluster_specifier_plugin_3, route(_, _, _)).WillOnce(Return(mock_route));
EXPECT_CALL(*mock_cluster_specifier_plugin_3, route(_, _, _, _)).WillOnce(Return(mock_route));
EXPECT_EQ(mock_route.get(), config.route(genHeaders("some_cluster", "/bar", "GET"), 0).get());
}

Expand Down Expand Up @@ -5025,7 +5025,8 @@ TEST_F(RouteMatcherTest, InvalidRetryBackOff) {
)EOF";

factory_context_.cluster_manager_.initializeClusters({"backoff"}, {});
TestConfigImpl(parseRouteConfigurationFromYaml(yaml), factory_context_, true, creation_status_);
TestConfigImpl config(parseRouteConfigurationFromYaml(yaml), factory_context_, true,
creation_status_);
EXPECT_EQ(creation_status_.message(),
"retry_policy.max_interval must greater than or equal to the base_interval");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,15 +78,15 @@ TEST_F(LuaClusterSpecifierPluginTest, NormalLuaCode) {
auto mock_route = std::make_shared<NiceMock<Envoy::Router::MockRoute>>();
{
Http::TestRequestHeaderMapImpl headers{{":path", "/"}, {"header_key", "fake"}};
auto route = plugin_->route(mock_route, headers, stream_info_);
auto route = plugin_->route(mock_route, headers, stream_info_, 0);
EXPECT_EQ("fake_service", route->routeEntry()->clusterName());
// Force the runtime to gc and destroy all the userdata.
config_->perLuaCodeSetup()->runtimeGC();
}

{
Http::TestRequestHeaderMapImpl headers{{":path", "/"}, {"header_key", "header_value"}};
auto route = plugin_->route(mock_route, headers, stream_info_);
auto route = plugin_->route(mock_route, headers, stream_info_, 0);
EXPECT_EQ("web_service", route->routeEntry()->clusterName());
// Force the runtime to gc and destroy all the userdata.
config_->perLuaCodeSetup()->runtimeGC();
Expand All @@ -100,15 +100,15 @@ TEST_F(LuaClusterSpecifierPluginTest, ErrorLuaCode) {
auto mock_route = std::make_shared<NiceMock<Envoy::Router::MockRoute>>();
{
Http::TestRequestHeaderMapImpl headers{{":path", "/"}, {"header_key", "fake"}};
auto route = plugin_->route(mock_route, headers, stream_info_);
auto route = plugin_->route(mock_route, headers, stream_info_, 0);
EXPECT_EQ("default_service", route->routeEntry()->clusterName());
// Force the runtime to gc and destroy all the userdata.
config_->perLuaCodeSetup()->runtimeGC();
}

{
Http::TestRequestHeaderMapImpl headers{{":path", "/"}, {"header_key", "header_value"}};
auto route = plugin_->route(mock_route, headers, stream_info_);
auto route = plugin_->route(mock_route, headers, stream_info_, 0);
EXPECT_EQ("default_service", route->routeEntry()->clusterName());
// Force the runtime to gc and destroy all the userdata.
config_->perLuaCodeSetup()->runtimeGC();
Expand All @@ -122,15 +122,15 @@ TEST_F(LuaClusterSpecifierPluginTest, ReturnTypeNotStringLuaCode) {
auto mock_route = std::make_shared<NiceMock<Envoy::Router::MockRoute>>();
{
Http::TestRequestHeaderMapImpl headers{{":path", "/"}, {"header_key", "fake"}};
auto route = plugin_->route(mock_route, headers, stream_info_);
auto route = plugin_->route(mock_route, headers, stream_info_, 0);
EXPECT_EQ("fake_service", route->routeEntry()->clusterName());
// Force the runtime to gc and destroy all the userdata.
config_->perLuaCodeSetup()->runtimeGC();
}

{
Http::TestRequestHeaderMapImpl headers{{":path", "/"}, {"header_key", "header_value"}};
auto route = plugin_->route(mock_route, headers, stream_info_);
auto route = plugin_->route(mock_route, headers, stream_info_, 0);
EXPECT_EQ("default_service", route->routeEntry()->clusterName());
// Force the runtime to gc and destroy all the userdata.
config_->perLuaCodeSetup()->runtimeGC();
Expand Down Expand Up @@ -167,7 +167,7 @@ TEST_F(LuaClusterSpecifierPluginTest, GetClustersBadArg) {

auto mock_route = std::make_shared<NiceMock<Envoy::Router::MockRoute>>();
Http::TestRequestHeaderMapImpl headers{{":path", "/"}};
auto route = plugin_->route(mock_route, headers, stream_info_);
auto route = plugin_->route(mock_route, headers, stream_info_, 0);
EXPECT_EQ("default_service", route->routeEntry()->clusterName());
}

Expand All @@ -192,7 +192,7 @@ TEST_F(LuaClusterSpecifierPluginTest, GetClustersMissing) {

auto mock_route = std::make_shared<NiceMock<Envoy::Router::MockRoute>>();
Http::TestRequestHeaderMapImpl headers{{":path", "/"}};
auto route = plugin_->route(mock_route, headers, stream_info_);
auto route = plugin_->route(mock_route, headers, stream_info_, 0);
EXPECT_EQ("nil", route->routeEntry()->clusterName());
}

Expand Down Expand Up @@ -242,7 +242,7 @@ TEST_F(LuaClusterSpecifierPluginTest, ClusterMethods) {

auto mock_route = std::make_shared<NiceMock<Envoy::Router::MockRoute>>();
Http::TestRequestHeaderMapImpl headers{{":path", "/"}};
auto route = plugin_->route(mock_route, headers, stream_info_);
auto route = plugin_->route(mock_route, headers, stream_info_, 0);
EXPECT_EQ("pass", route->routeEntry()->clusterName());

// Force the runtime to gc and destroy all the userdata.
Expand Down Expand Up @@ -279,7 +279,7 @@ TEST_F(LuaClusterSpecifierPluginTest, ClusterRef) {

auto mock_route = std::make_shared<NiceMock<Envoy::Router::MockRoute>>();
Http::TestRequestHeaderMapImpl headers{{":path", "/"}};
auto route = plugin_->route(mock_route, headers, stream_info_);
auto route = plugin_->route(mock_route, headers, stream_info_, 0);
EXPECT_EQ("pass", route->routeEntry()->clusterName());

// Force the runtime to gc and destroy all the userdata.
Expand Down Expand Up @@ -310,7 +310,7 @@ TEST_F(LuaClusterSpecifierPluginTest, Logging) {
{"warn", "log test"},
{"error", "log test"},
{"critical", "log test"}}),
{ plugin_->route(mock_route, headers, stream_info_); });
{ plugin_->route(mock_route, headers, stream_info_, 0); });
}

} // namespace Lua
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,19 +116,19 @@ TEST_F(MatcherClusterSpecifierPluginTest, NormalConfigTest) {
auto mock_route = std::make_shared<NiceMock<Envoy::Router::MockRoute>>();
{
Http::TestRequestHeaderMapImpl headers{{":path", "/"}, {"env", "staging"}};
auto route = plugin_->route(mock_route, headers, stream_info_);
auto route = plugin_->route(mock_route, headers, stream_info_, 0);
EXPECT_EQ("staging-cluster", route->routeEntry()->clusterName());
}

{
Http::TestRequestHeaderMapImpl headers{{":path", "/"}, {"env", "prod"}};
auto route = plugin_->route(mock_route, headers, stream_info_);
auto route = plugin_->route(mock_route, headers, stream_info_, 0);
EXPECT_EQ("prod-cluster", route->routeEntry()->clusterName());
}

{
Http::TestRequestHeaderMapImpl headers{{":path", "/"}, {"env", "not-exist"}};
auto route = plugin_->route(mock_route, headers, stream_info_);
auto route = plugin_->route(mock_route, headers, stream_info_, 0);
EXPECT_EQ("default-cluster", route->routeEntry()->clusterName());
}
}
Expand All @@ -139,13 +139,13 @@ TEST_F(MatcherClusterSpecifierPluginTest, NormalConfigWithoutDefaultCluster) {
auto mock_route = std::make_shared<NiceMock<Envoy::Router::MockRoute>>();
{
Http::TestRequestHeaderMapImpl headers{{":path", "/"}, {"env", "staging"}};
auto route = plugin_->route(mock_route, headers, stream_info_);
auto route = plugin_->route(mock_route, headers, stream_info_, 0);
EXPECT_EQ("staging-cluster", route->routeEntry()->clusterName());
}

{
Http::TestRequestHeaderMapImpl headers{{":path", "/"}, {"env", "not-exist"}};
auto route = plugin_->route(mock_route, headers, stream_info_);
auto route = plugin_->route(mock_route, headers, stream_info_, 0);
EXPECT_EQ("", route->routeEntry()->clusterName());
}
}
Expand Down
2 changes: 1 addition & 1 deletion test/mocks/router/mocks.cc
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ MockGenericConnectionPoolCallbacks::MockGenericConnectionPoolCallbacks() {
}

MockClusterSpecifierPlugin::MockClusterSpecifierPlugin() {
ON_CALL(*this, route(_, _, _)).WillByDefault(Return(nullptr));
ON_CALL(*this, route(_, _, _, _)).WillByDefault(Return(nullptr));
}

MockClusterSpecifierPluginFactoryConfig::MockClusterSpecifierPluginFactoryConfig() {
Expand Down
2 changes: 1 addition & 1 deletion test/mocks/router/mocks.h
Original file line number Diff line number Diff line change
Expand Up @@ -739,7 +739,7 @@ class MockClusterSpecifierPlugin : public ClusterSpecifierPlugin {

MOCK_METHOD(RouteConstSharedPtr, route,
(RouteEntryAndRouteConstSharedPtr parent, const Http::RequestHeaderMap& headers,
const StreamInfo::StreamInfo& stream_info),
const StreamInfo::StreamInfo& stream_info, uint64_t random),
(const));
};

Expand Down
Loading