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

router: correct cluster name for ads in routes #2449

Merged
merged 11 commits into from
Feb 1, 2018
Merged

router: correct cluster name for ads in routes #2449

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
82527a1
correct cluster name for ads in routes
ramaraochavali Jan 25, 2018
de59b03
dumped the proto
ramaraochavali Jan 26, 2018
faf0d8c
changed variable and method names, fixed tests
ramaraochavali Jan 26, 2018
ccec8ed
changed cluster_config to config_source
ramaraochavali Jan 27, 2018
6af94cb
fixed tests
ramaraochavali Jan 27, 2018
6bc4587
added pretty print support for protobuf json
ramaraochavali Jan 28, 2018
195f256
changed rds_impl_test.cc to adhere to new formatted print
ramaraochavali Jan 29, 2018
a4df1fb
changed the doc
ramaraochavali Jan 30, 2018
b53faa9
added DCO signoff
ramaraochavali Jan 30, 2018
8b8078d
added DCO signoff
ramaraochavali Jan 30, 2018
67dc1cf
removed the unnecessary if and else part
ramaraochavali Feb 1, 2018
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
6 changes: 3 additions & 3 deletions include/envoy/router/rds.h
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
8 changes: 4 additions & 4 deletions source/common/router/rds_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,10 @@ RdsRouteConfigProviderImpl::RdsRouteConfigProviderImpl(
// 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];
if (rds.has_config_source()) {
config_source_ = MessageUtil::getJsonStringFromMessage(rds.config_source(), true);
} else {
cluster_name_ = "NOT_USING_CLUSTER";
config_source_ = "{}";
Copy link
Member

Choose a reason for hiding this comment

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

Sorry quick question: Is there a test that should have caught this (switching to {})? Is it easy to add one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattklein123 there is no test. Will look in to adding it. One more thing, /routes is not working for routes defined statically. It is only iterating through the RdsRouteConfigProviders. https://github.com/envoyproxy/envoy/blob/master/source/common/router/rds_impl.cc#L215. Also it does not store the StaticRouteConfigProviders any where. Is this intentional or a bug? @htuch your thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and the else block changing config_source = {}, I think we can get rid of the else block completely because if rds is configured, with out config_source, EnvoyException will be thrown. And that is validated with this test https://github.com/envoyproxy/envoy/blob/master/test/common/router/rds_impl_test.cc#L126.

Copy link
Member

Choose a reason for hiding this comment

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

Also it does not store the StaticRouteConfigProviders any where. Is this intentional or a bug?

It just wasn't implemented. In the context of #2421 we will want to revisit this.

and the else block changing config_source = {}, I think we can get rid of the else block completely because if rds is configured, with out config_source, EnvoyException will be thrown.

Yup, great catch. If the code can't be hit just delete it and remove the if statement entirely. Thanks!

}
}
Copy link
Member

Choose a reason for hiding this comment

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

This whole if statement should now just be:

if (rds.has_config_source()) {
  cluster_source_ = MessageUtil::getJsonStringFromMessage(rds.config_source());
}


Expand Down Expand Up @@ -247,7 +247,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
View file
Open in desktop
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
View file
Open in desktop
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