-
Notifications
You must be signed in to change notification settings - Fork 5.5k
build: making admin optional #23903
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
build: making admin optional #23903
Changes from 3 commits
cf61641
e3f01ee
44549bc
eaec79b
a1d5c1a
dba9757
4a3f628
a5b0c5c
50c5aba
f15e6e7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -279,6 +279,11 @@ class Admin { | |
| */ | ||
| static RequestPtr makeStaticTextRequest(absl::string_view response_text, Http::Code code); | ||
| static RequestPtr makeStaticTextRequest(Buffer::Instance& response_text, Http::Code code); | ||
|
|
||
| /** | ||
| * Closes the listening socket for the admin. | ||
| */ | ||
| virtual void closeSocket() PURE; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't seem to figure out why this new pure virtual method is needed. Oh is it because we're using
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we do, or the server (which now has an API pointer because it may not know about the impl class) can't close the socket.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe you, but I don't yet understand how to connect all the dots :) Why does the server need an API pointer now when before it had an impl pointer? |
||
| }; | ||
|
|
||
| } // namespace Server | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -186,12 +186,15 @@ bool MainCommonBase::run() { | |
|
|
||
| void MainCommonBase::adminRequest(absl::string_view path_and_query, absl::string_view method, | ||
| const AdminRequestFn& handler) { | ||
| ASSERT(server_->admin().has_value(), "adminRequest called with admin support compiled out"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok having this assert in in debug mode seems OK. But if we are not in debug mode should we return a 501 or something? Seems like the fall-through case is going to jus not populate response_headers at all which might be confusing. Alternatively -- I know you are not going to like this, but this is really a C++ API for folks that instantiate main_common themselves and can call methods on it. You could ifdef out the existence of this method entirely so calling it would be a build failure rather than a crash. But it's up to you.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't love it, but I'll take it over a custom error path no one actually wants but I'd otherwise feel obliged to write tests for =P
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great point! |
||
| std::string path_and_query_buf = std::string(path_and_query); | ||
| std::string method_buf = std::string(method); | ||
| server_->dispatcher().post([this, path_and_query_buf, method_buf, handler]() { | ||
| auto response_headers = Http::ResponseHeaderMapImpl::create(); | ||
| std::string body; | ||
| server_->admin().request(path_and_query_buf, method_buf, *response_headers, body); | ||
| if (server_->admin()) { | ||
| server_->admin()->request(path_and_query_buf, method_buf, *response_headers, body); | ||
| } | ||
| handler(*response_headers, body); | ||
| }); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, why does this need to be added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the tap filter has admin functionality. I didn't go through the exercise of carving that out, the entire tap filter simply won't compile.
Because of that we exempt tap here and add it back below as long as admin isn't disabled