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

router: correct cluster name for ads in routes #2449

Merged
merged 11 commits into from
Feb 1, 2018
6 changes: 3 additions & 3 deletions include/envoy/router/rds.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,10 @@ class RdsRouteConfigProvider : public RouteConfigProvider {
virtual const std::string& routeConfigName() const PURE;

/**
* @return const std::string& the name of the cluster the RdsRouteConfigProvider is issuing RDS
* requests to.
* @return const std::string& the configuration of the service the RdsRouteConfigProvider is
* issuing RDS requests to.
*/
virtual const std::string& clusterName() const PURE;
virtual const std::string& configSource() const PURE;
};

typedef std::shared_ptr<RouteConfigProvider> RouteConfigProviderSharedPtr;
Expand Down
6 changes: 5 additions & 1 deletion source/common/protobuf/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,16 @@ void MessageUtil::loadFromFile(const std::string& path, Protobuf::Message& messa
}
}

std::string MessageUtil::getJsonStringFromMessage(const Protobuf::Message& message) {
std::string MessageUtil::getJsonStringFromMessage(const Protobuf::Message& message,
const bool pretty_print) {
Protobuf::util::JsonPrintOptions json_options;
// By default, proto field names are converted to camelCase when the message is converted to JSON.
// Setting this option makes debugging easier because it keeps field names consistent in JSON
// printouts.
json_options.preserve_proto_field_names = true;
if (pretty_print) {
json_options.add_whitespace = true;
}
ProtobufTypes::String json;
const auto status = Protobuf::util::MessageToJsonString(message, &json, json_options);
// This should always succeed unless something crash-worthy such as out-of-memory.
Expand Down
8 changes: 5 additions & 3 deletions source/common/protobuf/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -178,9 +178,11 @@ class MessageUtil {
/**
* Extract JSON as string from a google.protobuf.Message.
* @param message message of type type.googleapis.com/google.protobuf.Message.
* @return std::string of JSON object.
* @param pretty_print whether the returned JSON should be formatted.
* @return std::string of formatted JSON object.
*/
static std::string getJsonStringFromMessage(const Protobuf::Message& message);
static std::string getJsonStringFromMessage(const Protobuf::Message& message,
bool pretty_print = false);

/**
* Extract JSON object from a google.protobuf.Message.
Expand Down Expand Up @@ -236,4 +238,4 @@ template <> struct hash<Envoy::HashedValue> {
std::size_t operator()(Envoy::HashedValue const& v) const { return v.hash(); }
};

} // namespace std
} // namespace std
12 changes: 2 additions & 10 deletions source/common/router/rds_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -69,15 +69,7 @@ RdsRouteConfigProviderImpl::RdsRouteConfigProviderImpl(
},
"envoy.api.v2.RouteDiscoveryService.FetchRoutes",
"envoy.api.v2.RouteDiscoveryService.StreamRoutes");

// In V2 we use a Subscription model where the fetch can happen via gRPC, REST, or
// local filesystem. If the subscription happens via local filesystem (e.g xds_integration_test),
// then there is no actual RDS server, and hence no RDS cluster name.
if (rds.has_config_source() && rds.config_source().has_api_config_source()) {
cluster_name_ = rds.config_source().api_config_source().cluster_names()[0];
} else {
cluster_name_ = "NOT_USING_CLUSTER";
}
config_source_ = MessageUtil::getJsonStringFromMessage(rds.config_source(), true);
}

RdsRouteConfigProviderImpl::~RdsRouteConfigProviderImpl() {
Expand Down Expand Up @@ -247,7 +239,7 @@ Http::Code RouteConfigProviderManagerImpl::handlerRoutesLoop(
response.add("{\n");
response.add(fmt::format("\"version_info\": \"{}\",\n", provider->versionInfo()));
response.add(fmt::format("\"route_config_name\": \"{}\",\n", provider->routeConfigName()));
response.add(fmt::format("\"cluster_name\": \"{}\",\n", provider->clusterName()));
response.add(fmt::format("\"config_source\": {},\n", provider->configSource()));
response.add("\"route_table_dump\": ");
response.add(fmt::format("{}\n", provider->configAsJson()));
response.add("}\n");
Expand Down
6 changes: 3 additions & 3 deletions source/common/router/rds_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -98,10 +98,10 @@ class RdsRouteConfigProviderImpl

// Router::RdsRouteConfigProvider
std::string configAsJson() const override {
return MessageUtil::getJsonStringFromMessage(route_config_proto_);
return MessageUtil::getJsonStringFromMessage(route_config_proto_, true);
}
const std::string& routeConfigName() const override { return route_config_name_; }
const std::string& clusterName() const override { return cluster_name_; }
const std::string& configSource() const override { return config_source_; }
const std::string versionInfo() const override { return subscription_->versionInfo(); }

// Config::SubscriptionCallbacks
Expand Down Expand Up @@ -130,7 +130,7 @@ class RdsRouteConfigProviderImpl
Upstream::ClusterManager& cm_;
std::unique_ptr<Envoy::Config::Subscription<envoy::api::v2::RouteConfiguration>> subscription_;
ThreadLocal::SlotPtr tls_;
std::string cluster_name_;
std::string config_source_;
const std::string route_config_name_;
bool initialized_{};
uint64_t last_config_hash_{};
Expand Down
89 changes: 82 additions & 7 deletions test/common/router/rds_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -215,8 +215,17 @@ TEST_F(RdsImplTest, Basic) {
{
"version_info": "",
"route_config_name": "foo_route_config",
"cluster_name": "foo_cluster",
"config_source": {
"api_config_source": {
"cluster_names": [
"foo_cluster"
],
"refresh_delay": "1s"
}
}
,
"route_table_dump": {}

}
]
)EOF";
Expand Down Expand Up @@ -248,8 +257,19 @@ TEST_F(RdsImplTest, Basic) {
{
"version_info": "hash_15ed54077da94d8b",
"route_config_name": "foo_route_config",
"cluster_name": "foo_cluster",
"route_table_dump": {"name":"foo_route_config"}
"config_source": {
"api_config_source": {
"cluster_names": [
"foo_cluster"
],
"refresh_delay": "1s"
}
}
,
"route_table_dump": {
"name": "foo_route_config"
}

}
]
)EOF";
Expand Down Expand Up @@ -327,8 +347,45 @@ TEST_F(RdsImplTest, Basic) {
{
"version_info": "hash_7a3f97b327d08382",
"route_config_name": "foo_route_config",
"cluster_name": "foo_cluster",
"route_table_dump": {"name":"foo_route_config","virtual_hosts":[{"name":"local_service","domains":["*"],"routes":[{"match":{"prefix":"/foo"},"route":{"cluster_header":":authority"}},{"match":{"prefix":"/bar"},"route":{"cluster":"bar"}}]}]}
"config_source": {
"api_config_source": {
"cluster_names": [
"foo_cluster"
],
"refresh_delay": "1s"
}
}
,
"route_table_dump": {
"name": "foo_route_config",
"virtual_hosts": [
{
"name": "local_service",
"domains": [
"*"
],
"routes": [
{
"match": {
"prefix": "/foo"
},
"route": {
"cluster_header": ":authority"
}
},
{
"match": {
"prefix": "/bar"
},
"route": {
"cluster": "bar"
}
}
]
}
]
}

}
]
)EOF";
Expand Down Expand Up @@ -524,14 +581,32 @@ TEST_F(RouteConfigProviderManagerImplTest, Basic) {
{
"version_info": "",
"route_config_name": "foo_route_config",
"cluster_name": "bar_cluster",
"config_source": {
"api_config_source": {
"cluster_names": [
"bar_cluster"
],
"refresh_delay": "1s"
}
}
,
"route_table_dump": {}

}
,{
"version_info": "",
"route_config_name": "foo_route_config",
"cluster_name": "foo_cluster",
"config_source": {
"api_config_source": {
"cluster_names": [
"foo_cluster"
],
"refresh_delay": "1s"
}
}
,
"route_table_dump": {}

}
]
)EOF";
Expand Down