-
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 1 commit
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 |
|---|---|---|
|
|
@@ -396,8 +396,8 @@ elif [[ "$CI_TARGET" == "bazel.compile_time_options" ]]; then | |
| echo "Building and testing with wasm=wamr: ${TEST_TARGETS[*]}" | ||
| bazel_with_collection test "${BAZEL_BUILD_OPTIONS[@]}" --define wasm=wamr "${COMPILE_TIME_OPTIONS[@]}" -c dbg "${TEST_TARGETS[@]}" --test_tag_filters=-nofips --build_tests_only | ||
|
|
||
| echo "Building and testing with wasm=wasmtime: ${TEST_TARGETS[*]}" | ||
| bazel_with_collection test "${BAZEL_BUILD_OPTIONS[@]}" --define wasm=wasmtime "${COMPILE_TIME_OPTIONS[@]}" -c dbg "${TEST_TARGETS[@]}" --test_tag_filters=-nofips --build_tests_only | ||
| echo "Building and testing with wasm=wasmtime: and admin_functionality disabled ${TEST_TARGETS[*]}" | ||
|
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. nit: Maybe mention admin_html too, for completeness?
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. done |
||
| bazel_with_collection test "${BAZEL_BUILD_OPTIONS[@]}" --define wasm=wasmtime --define admin_html=disabled --define admin_functionality=disabled "${COMPILE_TIME_OPTIONS[@]}" -c dbg "${TEST_TARGETS[@]}" --test_tag_filters=-nofips --build_tests_only | ||
|
|
||
| echo "Building and testing with wasm=wavm: ${TEST_TARGETS[*]}" | ||
| bazel_with_collection test "${BAZEL_BUILD_OPTIONS[@]}" --define wasm=wavm "${COMPILE_TIME_OPTIONS[@]}" -c dbg "${TEST_TARGETS[@]}" --test_tag_filters=-nofips --build_tests_only | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,14 +34,17 @@ filegroup( | |
| "using_deprecated_config.yaml", | ||
| "**/*.template.yaml", | ||
| "freebind/freebind.yaml", | ||
| "envoy-tap-config.yaml", | ||
|
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. Out of curiosity, why does this need to be added?
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. 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. |
||
| "envoy-demo.yaml", | ||
| ], | ||
| ) + select({ | ||
| "//bazel:apple": ["envoy-demo.yaml"], | ||
| "//bazel:windows_x86_64": [], | ||
| "//bazel:disable_admin_functionality": [], | ||
| "//conditions:default": [ | ||
| "envoy-demo.yaml", | ||
| "freebind/freebind.yaml", | ||
| "envoy-tap-config.yaml", | ||
| ], | ||
| }), | ||
| ) | ||
|
|
||
| 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 |
|---|---|---|
|
|
@@ -50,14 +50,16 @@ bool ConfigSubscriptionInstance::checkAndApplyConfigUpdate(const Protobuf::Messa | |
| return true; | ||
| } | ||
|
|
||
| ConfigProviderManagerImplBase::ConfigProviderManagerImplBase(Server::Admin& admin, | ||
| ConfigProviderManagerImplBase::ConfigProviderManagerImplBase(OptRef<Server::Admin> admin, | ||
| const std::string& config_name) { | ||
| config_tracker_entry_ = admin.getConfigTracker().add( | ||
| config_name, | ||
| [this](const Matchers::StringMatcher& name_matcher) { return dumpConfigs(name_matcher); }); | ||
| // ConfigTracker keys must be unique. We are asserting that no one has stolen the key | ||
| // from us, since the returned entry will be nullptr if the key already exists. | ||
| RELEASE_ASSERT(config_tracker_entry_, ""); | ||
| if (admin.has_value()) { | ||
|
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. nit: early return
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. Why not just use the ifdef? Or do you also support (or plan to support) disabling admin at runtime via a runtime or command-line flag while still compiling it in?
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 prefer most of the code compiling in both branches and minimal use of the ifdef. it means we can get coverage for more code for example |
||
| config_tracker_entry_ = admin->getConfigTracker().add( | ||
| config_name, | ||
| [this](const Matchers::StringMatcher& name_matcher) { return dumpConfigs(name_matcher); }); | ||
| // ConfigTracker keys must be unique. We are asserting that no one has stolen the key | ||
| // from us, since the returned entry will be nullptr if the key already exists. | ||
| RELEASE_ASSERT(config_tracker_entry_, ""); | ||
| } | ||
| } | ||
|
|
||
| const ConfigProviderManagerImplBase::ConfigProviderSet& | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,16 +5,18 @@ | |
| namespace Envoy { | ||
| namespace Rds { | ||
|
|
||
| RouteConfigProviderManager::RouteConfigProviderManager(Server::Admin& admin, | ||
| RouteConfigProviderManager::RouteConfigProviderManager(OptRef<Server::Admin> admin, | ||
| const std::string& config_tracker_key, | ||
| ProtoTraits& proto_traits) | ||
| : config_tracker_entry_(admin.getConfigTracker().add( | ||
| config_tracker_key, | ||
| [this](const Matchers::StringMatcher& matcher) { return dumpRouteConfigs(matcher); })), | ||
| proto_traits_(proto_traits) { | ||
| // ConfigTracker keys must be unique. We are asserting that no one has stolen the "routes" key | ||
| // from us, since the returned entry will be nullptr if the key already exists. | ||
| RELEASE_ASSERT(config_tracker_entry_, ""); | ||
| : proto_traits_(proto_traits) { | ||
| if (admin.has_value()) { | ||
|
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. nit: early return
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. done |
||
| config_tracker_entry_ = admin->getConfigTracker().add( | ||
| config_tracker_key, | ||
| [this](const Matchers::StringMatcher& matcher) { return dumpRouteConfigs(matcher); }); | ||
| // ConfigTracker keys must be unique. We are asserting that no one has stolen the "routes" key | ||
| // from us, since the returned entry will be nullptr if the key already exists. | ||
| RELEASE_ASSERT(config_tracker_entry_, ""); | ||
| } | ||
| } | ||
|
|
||
| void RouteConfigProviderManager::eraseStaticProvider(RouteConfigProvider* provider) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -186,12 +186,14 @@ bool MainCommonBase::run() { | |
|
|
||
| void MainCommonBase::adminRequest(absl::string_view path_and_query, absl::string_view method, | ||
| const AdminRequestFn& handler) { | ||
| RELEASE_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. You'd know better than I would, but I wonder if instead of RELEASE_ASSERT if we should just ASSERT() in debug, and return in release? But I suspect you already thought about this.
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. Why would we want to assert at all? Can't we just call the handler with an http error code? Maybe a 501?
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. it's a bug to call admin requests with them compiled out. I think we should definitely crash in debug mode at least. |
||
| 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); | ||
| 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.
nit: "admin_functionaity" => "admin_functionality", I think?
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.
yep, thanks