admin: add endpoint to access current runtime values#2353
admin: add endpoint to access current runtime values#2353mattklein123 merged 3 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Chris Roche <croche@lyft.com>
mattklein123
left a comment
There was a problem hiding this comment.
Super solid, thanks. A few comments. Please remove "fixes" from the description as we should keep the issue open until we also have modify support on the endpoint (via POST).
| EXPECT_CALL(loader, snapshot()).WillRepeatedly(testing::ReturnPointee(&snapshot)); | ||
| EXPECT_CALL(server_, runtime()).WillRepeatedly(testing::ReturnPointee(&loader)); | ||
|
|
||
| EXPECT_EQ(Http::Code::BadRequest, |
There was a problem hiding this comment.
optimally, you would expect the help string that you expect here also.
test/server/http/admin_test.cc
Outdated
|
|
||
| std::unordered_map<std::string, const Runtime::Snapshot::Entry> entries{ | ||
| {"string_key", {"foo", {}}}, {"int_key", {"1", {1}}}, {"other_key", {"bar", {}}}}; | ||
| Runtime::MockSnapshot snapshot{}; |
There was a problem hiding this comment.
nit: {} initializers for objects are redundant. I would remove them here and elsewhere.
There was a problem hiding this comment.
Can take the boy out of Go...
test/server/http/admin_test.cc
Outdated
| } | ||
| )"}; | ||
|
|
||
| EXPECT_NE(-1, response.search(json_format.data(), json_format.size(), 0)); |
There was a problem hiding this comment.
nit: It would be a better test to actually parse the response as JSON using JSON loader and then check some values.
|
@htuch I'll take a pass over this later today |
| std::sort(pairs.begin(), pairs.end(), | ||
| [](const std::pair<std::string, const Runtime::Snapshot::Entry>& a, | ||
| const std::pair<std::string, const Runtime::Snapshot::Entry>& b) -> bool { | ||
| return a.first < b.first; |
There was a problem hiding this comment.
std::pair implements operator<, so you could just say: std::sort(pairs.begin(), pairs.end()). The pair comparison function does what you'd expect: comparison by the first, then the second only if they are unequal. The lambda ignores the second if they are equal, of course, but std::sort isn't guaranteed to be a stable sort anyway.
There was a problem hiding this comment.
Without the lambda, the const Entry values result in errors:
error: invalid operands to binary expression ('const Envoy::Runtime::Snapshot::Entry' and 'const Envoy::Runtime::Snapshot::Entry')
return __x.first < __y.first || (!(__y.first < __x.first) && __x.second < __y.second);
~~~~~~~~~~ ^ ~~~~~~~~~~
I could implement operator< for Entry, but I figured that might be overkill given the usecase.
There was a problem hiding this comment.
Ah, OK. I think leave the lambda then. Thanks for looking into it!
Signed-off-by: Chris Roche <croche@lyft.com>
mattklein123
left a comment
There was a problem hiding this comment.
LGTM, small remaining nit
test/server/http/admin_test.cc
Outdated
| EXPECT_EQ(Http::Code::OK, admin_.runCallback("/runtime", header_map, response)); | ||
|
|
||
| const std::string std_format{"int_key: 1\nother_key: bar\nstring_key: foo\n"}; | ||
| EXPECT_NE(-1, response.search(std_format.data(), std_format.size(), 0)); |
There was a problem hiding this comment.
nit: Collapse all of this into a single EXPECT_EQ without any local variable and use BufferToString. Same in bad format test.
Signed-off-by: Chris Roche <croche@lyft.com>
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
This patch adds a read-only admin endpoint to access runtime values. Values pairs are sorted by keys and available in either a human readable format or, with the query
format=json, as JSON.Risk Level: Small-Medium (new read-only endpoint on admin interface)
Testing: Unit + Integration
Docs Changes: envoyproxy/data-plane-api#414
Release Notes: Updated
Related: #514