Combine query-params into admin API's path, with API access from MainCommon sinking to main thread#4059
Conversation
…Common sinking to main thread. Signed-off-by: Joshua Marantz <jmarantz@google.com>
alyssawilk
left a comment
There was a problem hiding this comment.
Very nice - the path-and-params argument seems a lot easier to use.
| NOT_REACHED_GCOVR_EXCL_LINE; | ||
| } | ||
|
|
||
| void MainCommonBase::adminRequest(absl::string_view path_and_query, absl::string_view method, |
There was a problem hiding this comment.
This looks unused. Was this intentional? If we're adding a helper for downstream use we should comment, and please test either way.
There was a problem hiding this comment.
Added comments and a test.
source/server/http/admin.cc
Outdated
| for (const UrlHandler* handler : sortedHandlers()) { | ||
| const std::string& url = handler->prefix_; | ||
| absl::string_view path = handler->prefix_; | ||
| // Removing the leading slash from the link, so that the admin page can be |
There was a problem hiding this comment.
Removing -> remove. Also as someone not terribly familiar with this code this comment doesn't quite explain what's going on - could we spell it out a bit more clearly?
| // Removing the leading slash from the link, so that the admin page can be | ||
| // rendered as part of another console, on a sub-path. | ||
| ASSERT(!path.empty()); | ||
| ASSERT(path[0] == '/'); |
There was a problem hiding this comment.
Do we verify that where we register handlers that an empty handler is disallowed? Basically if we attempt to register "" will Envoy fail at start-up or crash when the URL is hit? A regression test would be great if we don't have one.
There was a problem hiding this comment.
Added comment in .h and ASSERTs.
| } | ||
|
|
||
| void MainCommonBase::adminRequest(absl::string_view path_and_query, absl::string_view method, | ||
| const AdminRequestFn& handler) { |
There was a problem hiding this comment.
Rather than just exposing the admin interface for querying, is there any objection to exposing the entire server interface through MainCommonBase? For instance, this would allow containing code to directly query stats, add new admin handlers at runtime, and make timers/posts to Envoy's main thread among other things. The server interface seems like a natural place to build an API boundary into Envoy.
There was a problem hiding this comment.
There has to be some place where we queue up requests to run on the main thread, and this seems like a good place to do that.
If we expose the Server& then it seems like it would be too easy to make a direct call to it from a different thread -- say the thread for handling an http request from a downstream console. But I'm open to suggestions.
There was a problem hiding this comment.
Totally agree. That's a good point about protecting the user from the threading issues. I'm trying to think of a way we can generalize this sort of threading protection for other objects exposed by the server interface because they would be useful to expose, and they would probably have the same sorts of problems, but I don't see a clear way to do it.
As a partially related note, I think this method (and maybe the more general solution for other parts of the server interface) is a good candidate for the promise-future idiom. Using those components, the user doesn't need to worry about synchronizing the data they're getting back from handler. You could just create a promise templated by the return value, create a future from that promise, pass the promise to the post, and immediately return the future to the user at the end of the synchronous call. The user will then be able to tell when their data is available and access it safely through the future API.
There was a problem hiding this comment.
std::future looks interesting, but as there is no current usage in Envoy I think I'll add a TODO. I need to learn more about that, but maybe not block this path on it.
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
alyssawilk
left a comment
There was a problem hiding this comment.
Code looks mostly good. I'll take a final look once you have the tests sorted out. Meanwhile, Dhi, would you be willing to take a (non-Googler) pass?
…initialized. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz jmarantz@google.com
Description: It both simplifies the internal implementation and makes external integration easier to leave the query-params in path_and_query rather than having them be separate args. This also provides an API from MainCommon to synchronize an admin request onto the main thread.
Additionally: in the admin home page, removes the leading "/" from generated a-tag and html forms, so that this page can be hosted in a subdirectory.
Risk Level: low
Testing: //test/... and a working external integration not visible in this PR
Docs Changes: N/A
Release Notes: N/A