Skip to content

admin: fix config dump order #4197

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

Merged
htuch merged 3 commits into from
Aug 20, 2018
Merged

admin: fix config dump order #4197

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
804437c
fix config dump order
ramaraochavali Aug 18, 2018
799582f
fix admin integration test
ramaraochavali Aug 18, 2018
3c4a7a6
address review comments
ramaraochavali Aug 20, 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
10 changes: 5 additions & 5 deletions api/envoy/admin/v2alpha/config_dump.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,17 @@ import "gogoproto/gogo.proto";
// The :ref:`/config_dump <operations_admin_interface_config_dump>` admin endpoint uses this wrapper
// message to maintain and serve arbitrary configuration information from any component in Envoy.
message ConfigDump {
// This map is serialized and dumped in its entirety at the
// This list is serialized and dumped in its entirety at the
// :ref:`/config_dump <operations_admin_interface_config_dump>` endpoint.
//
// Keys are a short descriptor of the config object they map to. The following keys (and the
// messages they map to) are currently supported:
// The following configurations are currently supported and will be dumped in the order given
// below:
//
// * *bootstrap*: :ref:`BootstrapConfigDump <envoy_api_msg_admin.v2alpha.BootstrapConfigDump>`
// * *listeners*: :ref:`ListenersConfigDump <envoy_api_msg_admin.v2alpha.ListenersConfigDump>`
// * *clusters*: :ref:`ClustersConfigDump <envoy_api_msg_admin.v2alpha.ClustersConfigDump>`
// * *listeners*: :ref:`ListenersConfigDump <envoy_api_msg_admin.v2alpha.ListenersConfigDump>`
// * *routes*: :ref:`RoutesConfigDump <envoy_api_msg_admin.v2alpha.RoutesConfigDump>`
map<string, google.protobuf.Any> configs = 1 [(gogoproto.nullable) = false];
repeated google.protobuf.Any configs = 1 [(gogoproto.nullable) = false];
}

// This message describes the bootstrap configuration that Envoy was started with. This includes
Expand Down
4 changes: 1 addition & 3 deletions source/server/http/admin.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,13 +389,11 @@ Http::Code AdminImpl::handlerClusters(absl::string_view url, Http::HeaderMap& re
Http::Code AdminImpl::handlerConfigDump(absl::string_view, Http::HeaderMap& response_headers,
Buffer::Instance& response, AdminStream&) const {
envoy::admin::v2alpha::ConfigDump dump;
auto& config_dump_map = *(dump.mutable_configs());
for (const auto& key_callback_pair : config_tracker_.getCallbacksMap()) {
ProtobufTypes::MessagePtr message = key_callback_pair.second();
RELEASE_ASSERT(message, "");
ProtobufWkt::Any any_message;
auto& any_message = *(dump.add_configs());
any_message.PackFrom(*message);
config_dump_map[key_callback_pair.first] = any_message;
}

response_headers.insertContentType().value().setReference(
Expand Down
23 changes: 14 additions & 9 deletions test/integration/integration_admin_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,20 +283,25 @@ TEST_P(IntegrationAdminTest, Admin) {
EXPECT_STREQ("200", response->headers().Status()->value().c_str());
EXPECT_STREQ("application/json", ContentType(response));
json = Json::Factory::loadFromString(response->body());
EXPECT_TRUE(json->getObject("configs")->hasObject("bootstrap"));
EXPECT_TRUE(json->getObject("configs")->hasObject("clusters"));
EXPECT_TRUE(json->getObject("configs")->hasObject("listeners"));
EXPECT_TRUE(json->getObject("configs")->hasObject("routes"));
size_t index = 0;
const std::string expected_types[] = {
"type.googleapis.com/envoy.admin.v2alpha.BootstrapConfigDump",
"type.googleapis.com/envoy.admin.v2alpha.ClustersConfigDump",
"type.googleapis.com/envoy.admin.v2alpha.ListenersConfigDump",
"type.googleapis.com/envoy.admin.v2alpha.RoutesConfigDump"};
for (Json::ObjectSharedPtr obj_ptr : json->getObjectArray("configs")) {
EXPECT_TRUE(expected_types[index].compare(obj_ptr->getString("@type")) == 0);
index++;
}

// Validate we can parse as proto.
envoy::admin::v2alpha::ConfigDump config_dump;
MessageUtil::loadFromJson(response->body(), config_dump);
EXPECT_EQ(1, config_dump.configs().count("bootstrap"));
EXPECT_EQ(1, config_dump.configs().count("clusters"));
EXPECT_EQ(1, config_dump.configs().count("listeners"));
EXPECT_EQ(1, config_dump.configs().count("routes"));
EXPECT_EQ(4, config_dump.configs_size());

// .. and that we can unpack one of the entries.
envoy::admin::v2alpha::RoutesConfigDump route_config_dump;
config_dump.configs().at("routes").UnpackTo(&route_config_dump);
config_dump.configs(3).UnpackTo(&route_config_dump);
EXPECT_EQ("route_config_0", route_config_dump.static_route_configs(0).route_config().name());
}

Expand Down
59 changes: 56 additions & 3 deletions test/server/http/admin_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,19 +532,72 @@ TEST_P(AdminInstanceTest, ConfigDump) {
return msg;
});
const std::string expected_json = R"EOF({
"configs": {
"foo": {
"configs": [
{
"@type": "type.googleapis.com/google.protobuf.StringValue",
"value": "bar"
}
}
]
}
)EOF";
EXPECT_EQ(Http::Code::OK, getCallback("/config_dump", header_map, response));
std::string output = response.toString();
EXPECT_EQ(expected_json, output);
}

TEST_P(AdminInstanceTest, ConfigDumpMaintainsOrder) {
// Add configs in random order and validate config_dump dumps in the order.
auto bootstrap_entry = admin_.getConfigTracker().add("bootstrap", [] {
auto msg = std::make_unique<ProtobufWkt::StringValue>();
msg->set_value("bootstrap_config");
return msg;
});
auto route_entry = admin_.getConfigTracker().add("routes", [] {

@htuch htuch Aug 20, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: you don't need to assign to a variable/retain results here, they aren't used below.

@ramaraochavali ramaraochavali Aug 20, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

With out that config_dump is not build as expected. Not sure why though. The test above this ConfigDump test also has the same issue. Could n't quite figure out why.

@htuch htuch Aug 20, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, maybe it's because these are smart pointers, RAII, got it.

auto msg = std::make_unique<ProtobufWkt::StringValue>();
msg->set_value("routes_config");
return msg;
});
auto listener_entry = admin_.getConfigTracker().add("listeners", [] {
auto msg = std::make_unique<ProtobufWkt::StringValue>();
msg->set_value("listeners_config");
return msg;
});
auto cluster_entry = admin_.getConfigTracker().add("clusters", [] {
auto msg = std::make_unique<ProtobufWkt::StringValue>();
msg->set_value("clusters_config");
return msg;
});
const std::string expected_json = R"EOF({
"configs": [
{
"@type": "type.googleapis.com/google.protobuf.StringValue",
"value": "bootstrap_config"
},
{
"@type": "type.googleapis.com/google.protobuf.StringValue",
"value": "clusters_config"
},
{
"@type": "type.googleapis.com/google.protobuf.StringValue",
"value": "listeners_config"
},
{
"@type": "type.googleapis.com/google.protobuf.StringValue",
"value": "routes_config"
}
]
}
)EOF";
// Run it multiple times and validate that order is preserved.
for (size_t i = 0; i < 5; i++) {
Buffer::OwnedImpl response;
Http::HeaderMapImpl header_map;
EXPECT_EQ(Http::Code::OK, getCallback("/config_dump", header_map, response));
std::string output = response.toString();

@htuch htuch Aug 20, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: const

EXPECT_EQ(expected_json, output);
}
}

TEST_P(AdminInstanceTest, Runtime) {
Http::HeaderMapImpl header_map;
Buffer::OwnedImpl response;
Expand Down