diff --git a/envoy/server/admin.h b/envoy/server/admin.h index 496e57544a982..4ca2bbefbe5ee 100644 --- a/envoy/server/admin.h +++ b/envoy/server/admin.h @@ -55,6 +55,14 @@ class AdminStream { * absl::nullopt. */ virtual Http::Http1StreamEncoderOptionsOptRef http1StreamEncoderOptions() PURE; + + /** + * Construct query-param map for the stream, using the URL-specified params, + * or the request data if that is url-form-encoded. + * + * @param The query name/value map. + */ + virtual Http::Utility::QueryParams queryParams() const PURE; }; /** @@ -102,7 +110,7 @@ class Admin { enum class Type { Boolean, String, Enum }; const Type type_; const std::string id_; // HTML form ID and query-param name (JS var name rules). - const std::string help_; // Help text rendered into UI. + const std::string help_; // Rendered into home-page HTML and /help text. std::vector enum_choices_{}; }; using ParamDescriptorVec = std::vector; diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc index 796c7e63a2fef..61aab94fde56d 100644 --- a/source/server/admin/admin.cc +++ b/source/server/admin/admin.cc @@ -76,6 +76,15 @@ void AdminImpl::startHttpListener(const std::list& } } +namespace { +// Prepends an element to an array, modifying it as passed in. +std::vector prepend(const absl::string_view first, + std::vector strings) { + strings.insert(strings.begin(), first); + return strings; +} +} // namespace + AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server, bool ignore_global_conn_limit) : server_(server), @@ -122,9 +131,17 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server, makeHandler("/contention", "dump current Envoy mutex contention stats (if enabled)", MAKE_ADMIN_HANDLER(stats_handler_.handlerContention), false, false), makeHandler("/cpuprofiler", "enable/disable the CPU profiler", - MAKE_ADMIN_HANDLER(profiling_handler_.handlerCpuProfiler), false, true), + MAKE_ADMIN_HANDLER(profiling_handler_.handlerCpuProfiler), false, true, + {{Admin::ParamDescriptor::Type::Enum, + "enable", + "enables the CPU profiler", + {"y", "n"}}}), makeHandler("/heapprofiler", "enable/disable the heap profiler", - MAKE_ADMIN_HANDLER(profiling_handler_.handlerHeapProfiler), false, true), + MAKE_ADMIN_HANDLER(profiling_handler_.handlerHeapProfiler), false, true, + {{Admin::ParamDescriptor::Type::Enum, + "enable", + "enable/disable the heap profiler", + {"y", "n"}}}), makeHandler("/heap_dump", "dump current Envoy heap (if supported)", MAKE_ADMIN_HANDLER(tcmalloc_profiling_handler_.handlerHeapDump), false, false), @@ -138,20 +155,34 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server, MAKE_ADMIN_HANDLER(server_info_handler_.handlerHotRestartVersion), false, false), - // TODO(jmarantz): add support for param-passing through a POST. Browsers send - // those params as the post-body rather than query-params and that requires some - // re-plumbing through the admin callback API. See also drain_listeners. + // The logging "level" parameter, if specified as a non-blank entry, + // changes all the logging-paths to that level. So the enum parameter + // needs to include a an empty string as the default (first) option. + // Thus we prepend an empty string to the logging-levels list. makeHandler("/logging", "query/change logging levels", - MAKE_ADMIN_HANDLER(logs_handler_.handlerLogging), false, true), - + MAKE_ADMIN_HANDLER(logs_handler_.handlerLogging), false, true, + {{Admin::ParamDescriptor::Type::String, "paths", + "Change multiple logging levels by setting to " + ":,:."}, + {Admin::ParamDescriptor::Type::Enum, "level", "desired logging level", + prepend("", LogsHandler::levelStrings())}}), makeHandler("/memory", "print current allocation/heap usage", MAKE_ADMIN_HANDLER(server_info_handler_.handlerMemory), false, false), makeHandler("/quitquitquit", "exit the server", MAKE_ADMIN_HANDLER(server_cmd_handler_.handlerQuitQuitQuit), false, true), makeHandler("/reset_counters", "reset all counters to zero", MAKE_ADMIN_HANDLER(stats_handler_.handlerResetCounters), false, true), - makeHandler("/drain_listeners", "drain listeners", - MAKE_ADMIN_HANDLER(listeners_handler_.handlerDrainListeners), false, true), + makeHandler( + "/drain_listeners", "drain listeners", + MAKE_ADMIN_HANDLER(listeners_handler_.handlerDrainListeners), false, true, + {{ParamDescriptor::Type::Boolean, "graceful", + "When draining listeners, enter a graceful drain period prior to closing " + "listeners. This behaviour and duration is configurable via server options " + "or CLI"}, + {ParamDescriptor::Type::Boolean, "inboundonly", + "Drains all inbound listeners. traffic_direction field in " + "envoy_v3_api_msg_config.listener.v3.Listener is used to determine whether a " + "listener is inbound or outbound."}}), makeHandler("/server_info", "print server version/status information", MAKE_ADMIN_HANDLER(server_info_handler_.handlerServerInfo), false, false), makeHandler("/ready", "print server state, return 200 if LIVE, otherwise return 503", @@ -335,6 +366,7 @@ Admin::RequestPtr AdminImpl::makeRequest(absl::string_view path_and_query, } } + ASSERT(admin_stream.getRequestHeaders().getPathValue() == path_and_query); return handler.handler_(path_and_query, admin_stream); } } @@ -369,7 +401,8 @@ void AdminImpl::getHelp(Buffer::Instance& response) const { // Prefix order is used during searching, but for printing do them in alpha order. for (const UrlHandler* handler : sortedHandlers()) { - response.add(fmt::format(" {}: {}\n", handler->prefix_, handler->help_text_)); + const absl::string_view method = handler->mutates_server_state_ ? " (POST)" : ""; + response.add(fmt::format(" {}{}: {}\n", handler->prefix_, method, handler->help_text_)); for (const ParamDescriptor& param : handler->params_) { response.add(fmt::format(" {}: {}", param.id_, param.help_)); if (param.type_ == ParamDescriptor::Type::Enum) { @@ -440,6 +473,7 @@ Http::Code AdminImpl::request(absl::string_view path_and_query, absl::string_vie auto request_headers = Http::RequestHeaderMapImpl::create(); request_headers->setMethod(method); + request_headers->setPath(path_and_query); filter.decodeHeaders(*request_headers, false); Buffer::OwnedImpl response; diff --git a/source/server/admin/admin_filter.cc b/source/server/admin/admin_filter.cc index faa10b6a34bda..e1341366b0df1 100644 --- a/source/server/admin/admin_filter.cc +++ b/source/server/admin/admin_filter.cc @@ -61,8 +61,27 @@ const Http::RequestHeaderMap& AdminFilter::getRequestHeaders() const { return *request_headers_; } +Http::Utility::QueryParams AdminFilter::queryParams() const { + absl::string_view path = request_headers_->getPathValue(); + Http::Utility::QueryParams query = Http::Utility::parseAndDecodeQueryString(path); + if (!query.empty()) { + return query; + } + + // Check if the params are in the request's body. + if (request_headers_->getContentTypeValue() == + Http::Headers::get().ContentTypeValues.FormUrlEncoded) { + const Buffer::Instance* body = getRequestBody(); + if (body != nullptr) { + query = Http::Utility::parseFromBody(body->toString()); + } + } + + return query; +} + void AdminFilter::onComplete() { - const absl::string_view path = request_headers_->getPathValue(); + absl::string_view path = request_headers_->getPathValue(); ENVOY_STREAM_LOG(debug, "request complete: path: {}", *decoder_callbacks_, path); auto header_map = Http::ResponseHeaderMapImpl::create(); diff --git a/source/server/admin/admin_filter.h b/source/server/admin/admin_filter.h index f93fb40c93995..2bc528125ec49 100644 --- a/source/server/admin/admin_filter.h +++ b/source/server/admin/admin_filter.h @@ -51,6 +51,7 @@ class AdminFilter : public Http::PassThroughFilter, Http::Http1StreamEncoderOptionsOptRef http1StreamEncoderOptions() override { return encoder_callbacks_->http1StreamEncoderOptions(); } + Http::Utility::QueryParams queryParams() const override; private: /** diff --git a/source/server/admin/clusters_handler.cc b/source/server/admin/clusters_handler.cc index a0425e40c6747..12dc2f6209bd2 100644 --- a/source/server/admin/clusters_handler.cc +++ b/source/server/admin/clusters_handler.cc @@ -42,11 +42,10 @@ void addCircuitBreakerSettingsAsJson(const envoy::config::core::v3::RoutingPrior ClustersHandler::ClustersHandler(Server::Instance& server) : HandlerContextBase(server) {} -Http::Code ClustersHandler::handlerClusters(absl::string_view url, +Http::Code ClustersHandler::handlerClusters(absl::string_view, Http::ResponseHeaderMap& response_headers, - Buffer::Instance& response, AdminStream&) { - Http::Utility::QueryParams query_params = Http::Utility::parseAndDecodeQueryString(url); - const auto format_value = Utility::formatParam(query_params); + Buffer::Instance& response, AdminStream& admin_stream) { + const auto format_value = Utility::formatParam(admin_stream.queryParams()); if (format_value.has_value() && format_value.value() == "json") { writeClustersAsJson(response); diff --git a/source/server/admin/config_dump_handler.cc b/source/server/admin/config_dump_handler.cc index ad23b2434fce5..f456057e49b6b 100644 --- a/source/server/admin/config_dump_handler.cc +++ b/source/server/admin/config_dump_handler.cc @@ -131,13 +131,13 @@ absl::optional maskParam(const Http::Utility::QueryParams& params) // Helper method to get the eds parameter. bool shouldIncludeEdsInDump(const Http::Utility::QueryParams& params) { - return Utility::queryParam(params, "include_eds") != absl::nullopt; + return params.find("include_eds") != params.end(); } absl::StatusOr buildNameMatcher(const Http::Utility::QueryParams& params) { const auto name_regex = Utility::queryParam(params, "name_regex"); - if (!name_regex.has_value()) { + if (!name_regex.has_value() || name_regex->empty()) { return std::make_unique(); } envoy::type::matcher::v3::RegexMatcher matcher; @@ -157,10 +157,11 @@ buildNameMatcher(const Http::Utility::QueryParams& params) { ConfigDumpHandler::ConfigDumpHandler(ConfigTracker& config_tracker, Server::Instance& server) : HandlerContextBase(server), config_tracker_(config_tracker) {} -Http::Code ConfigDumpHandler::handlerConfigDump(absl::string_view url, +Http::Code ConfigDumpHandler::handlerConfigDump(absl::string_view, Http::ResponseHeaderMap& response_headers, - Buffer::Instance& response, AdminStream&) const { - Http::Utility::QueryParams query_params = Http::Utility::parseAndDecodeQueryString(url); + Buffer::Instance& response, + AdminStream& admin_stream) const { + Http::Utility::QueryParams query_params = admin_stream.queryParams(); const auto resource = resourceParam(query_params); const auto mask = maskParam(query_params); const bool include_eds = shouldIncludeEdsInDump(query_params); diff --git a/source/server/admin/init_dump_handler.cc b/source/server/admin/init_dump_handler.cc index 837625d161804..55c91115d3dcc 100644 --- a/source/server/admin/init_dump_handler.cc +++ b/source/server/admin/init_dump_handler.cc @@ -18,11 +18,11 @@ absl::optional maskParam(const Http::Utility::QueryParams& params) InitDumpHandler::InitDumpHandler(Server::Instance& server) : HandlerContextBase(server) {} -Http::Code InitDumpHandler::handlerInitDump(absl::string_view url, +Http::Code InitDumpHandler::handlerInitDump(absl::string_view, Http::ResponseHeaderMap& response_headers, - Buffer::Instance& response, AdminStream&) const { - Http::Utility::QueryParams query_params = Http::Utility::parseAndDecodeQueryString(url); - const auto mask = maskParam(query_params); + Buffer::Instance& response, + AdminStream& admin_stream) const { + const auto mask = maskParam(admin_stream.queryParams()); envoy::admin::v3::UnreadyTargetsDumps dump = *dumpUnreadyTargets(mask); MessageUtil::redact(dump); diff --git a/source/server/admin/listeners_handler.cc b/source/server/admin/listeners_handler.cc index 20eb3f531ddac..c3adafcafcab5 100644 --- a/source/server/admin/listeners_handler.cc +++ b/source/server/admin/listeners_handler.cc @@ -12,9 +12,10 @@ namespace Server { ListenersHandler::ListenersHandler(Server::Instance& server) : HandlerContextBase(server) {} -Http::Code ListenersHandler::handlerDrainListeners(absl::string_view url, Http::ResponseHeaderMap&, - Buffer::Instance& response, AdminStream&) { - const Http::Utility::QueryParams params = Http::Utility::parseQueryString(url); +Http::Code ListenersHandler::handlerDrainListeners(absl::string_view, Http::ResponseHeaderMap&, + Buffer::Instance& response, + AdminStream& admin_query) { + const Http::Utility::QueryParams params = admin_query.queryParams(); ListenerManager::StopListenersType stop_listeners_type = params.find("inboundonly") != params.end() ? ListenerManager::StopListenersType::InboundOnly @@ -37,10 +38,11 @@ Http::Code ListenersHandler::handlerDrainListeners(absl::string_view url, Http:: return Http::Code::OK; } -Http::Code ListenersHandler::handlerListenerInfo(absl::string_view url, +Http::Code ListenersHandler::handlerListenerInfo(absl::string_view, Http::ResponseHeaderMap& response_headers, - Buffer::Instance& response, AdminStream&) { - const Http::Utility::QueryParams query_params = Http::Utility::parseQueryString(url); + Buffer::Instance& response, + AdminStream& admin_query) { + const Http::Utility::QueryParams query_params = admin_query.queryParams(); const auto format_value = Utility::formatParam(query_params); if (format_value.has_value() && format_value.value() == "json") { diff --git a/source/server/admin/logs_handler.cc b/source/server/admin/logs_handler.cc index 8a50f9f78c0f9..d8afe2c6acf6f 100644 --- a/source/server/admin/logs_handler.cc +++ b/source/server/admin/logs_handler.cc @@ -16,10 +16,9 @@ namespace { absl::flat_hash_map buildLevelMap() { absl::flat_hash_map levels; - for (size_t i = 0; i < ARRAY_SIZE(spdlog::level::level_string_views); i++) { - spdlog::string_view_t spd_level_string{spdlog::level::level_string_views[i]}; - absl::string_view level_string{spd_level_string.data(), spd_level_string.size()}; - levels[level_string] = static_cast(i); + uint32_t i = 0; + for (absl::string_view level_string : LogsHandler::levelStrings()) { + levels[level_string] = static_cast(i++); } return levels; @@ -29,28 +28,34 @@ absl::flat_hash_map buildLevelMap( LogsHandler::LogsHandler(Server::Instance& server) : HandlerContextBase(server), log_levels_(buildLevelMap()) {} -Http::Code LogsHandler::handlerLogging(absl::string_view url, Http::ResponseHeaderMap&, - Buffer::Instance& response, AdminStream&) { - Http::Utility::QueryParams query_params = Http::Utility::parseQueryString(url); +std::vector LogsHandler::levelStrings() { + std::vector strings; + strings.reserve(ARRAY_SIZE(spdlog::level::level_string_views)); + for (spdlog::string_view_t level : spdlog::level::level_string_views) { + strings.emplace_back(absl::string_view{level.data(), level.size()}); + } + return strings; +} - Http::Code rc = Http::Code::OK; - if (!query_params.empty()) { - auto status = changeLogLevel(query_params); - if (!status.ok()) { - rc = Http::Code::BadRequest; - response.add(fmt::format("error: {}\n\n", status.message())); - - response.add("usage: /logging?= (change single level)\n"); - response.add( - "usage: /logging?paths=name1:level1,name2:level2,... (change multiple levels)\n"); - response.add("usage: /logging?level= (change all levels)\n"); - response.add("levels: "); - for (auto level_string_view : spdlog::level::level_string_views) { - response.add(fmt::format("{} ", level_string_view)); - } +Http::Code LogsHandler::handlerLogging(absl::string_view, Http::ResponseHeaderMap&, + Buffer::Instance& response, AdminStream& admin_stream) { + Http::Utility::QueryParams query_params = admin_stream.queryParams(); - response.add("\n"); + Http::Code rc = Http::Code::OK; + const absl::Status status = changeLogLevel(query_params); + if (!status.ok()) { + rc = Http::Code::BadRequest; + response.add(fmt::format("error: {}\n\n", status.message())); + + response.add("usage: /logging?= (change single level)\n"); + response.add("usage: /logging?paths=name1:level1,name2:level2,... (change multiple levels)\n"); + response.add("usage: /logging?level= (change all levels)\n"); + response.add("levels: "); + for (auto level_string_view : spdlog::level::level_string_views) { + response.add(fmt::format("{} ", level_string_view)); } + + response.add("\n"); } if (!Logger::Context::useFineGrainLogger()) { @@ -77,18 +82,31 @@ Http::Code LogsHandler::handlerReopenLogs(absl::string_view, Http::ResponseHeade return Http::Code::OK; } -absl::Status LogsHandler::changeLogLevel(const Http::Utility::QueryParams& params) { +absl::Status LogsHandler::changeLogLevel(Http::Utility::QueryParams& params) { + // "level" and "paths" will be set to the empty string when this is invoked + // from HTML without setting them, so clean out empty values. + auto level = params.find("level"); + if (level != params.end() && level->second.empty()) { + params.erase(level); + level = params.end(); + } + auto paths = params.find("paths"); + if (paths != params.end() && paths->second.empty()) { + params.erase(paths); + paths = params.end(); + } + + if (params.empty()) { + return absl::OkStatus(); + } + if (params.size() != 1) { return absl::InvalidArgumentError("invalid number of parameters"); } - const auto it = params.begin(); - absl::string_view key(it->first); - absl::string_view value(it->second); - - if (key == "level") { + if (level != params.end()) { // Change all log levels. - auto level_to_use = parseLogLevel(value); + const absl::StatusOr level_to_use = parseLogLevel(level->second); if (!level_to_use.ok()) { return level_to_use.status(); } @@ -101,18 +119,19 @@ absl::Status LogsHandler::changeLogLevel(const Http::Utility::QueryParams& param // not common to call this function at a high rate. absl::flat_hash_map name_levels; - if (key == "paths") { + if (paths != params.end()) { // Bulk change log level by name:level pairs, separated by comma. - std::vector pairs = absl::StrSplit(value, ',', absl::SkipWhitespace()); + std::vector pairs = + absl::StrSplit(paths->second, ',', absl::SkipWhitespace()); for (const auto& name_level : pairs) { - std::pair name_level_pair = + const std::pair name_level_pair = absl::StrSplit(name_level, absl::MaxSplits(':', 1), absl::SkipWhitespace()); auto [name, level] = name_level_pair; if (name.empty() || level.empty()) { return absl::InvalidArgumentError("empty logger name or empty logger level"); } - auto level_to_use = parseLogLevel(level); + const absl::StatusOr level_to_use = parseLogLevel(level); if (!level_to_use.ok()) { return level_to_use.status(); } @@ -120,8 +139,20 @@ absl::Status LogsHandler::changeLogLevel(const Http::Utility::QueryParams& param name_levels[name] = *level_to_use; } } else { + // The HTML admin interface will always populate "level" and "paths" though + // they may be empty. There's a legacy non-HTML-accessible mechanism to + // set a single logger to a level, which we'll handle now. In this scenario, + // "level" and "paths" will not be populated. + if (params.size() != 1) { + return absl::InvalidArgumentError("invalid number of parameters"); + } + // Change particular log level by name. - auto level_to_use = parseLogLevel(value); + const auto it = params.begin(); + const std::string& key = it->first; + const std::string& value = it->second; + + const absl::StatusOr level_to_use = parseLogLevel(value); if (!level_to_use.ok()) { return level_to_use.status(); } diff --git a/source/server/admin/logs_handler.h b/source/server/admin/logs_handler.h index 357f65cc32c94..c25bef7ca058c 100644 --- a/source/server/admin/logs_handler.h +++ b/source/server/admin/logs_handler.h @@ -30,15 +30,21 @@ class LogsHandler : public HandlerContextBase, Logger::Loggable levelStrings(); + private: /** * Attempt to change the log level of a logger or all loggers. * - * Returns StatusCode::kInvalidArgument if validation failed. + * @return OkStatus if there are no non-empty params, StatusCode::kInvalidArgument if + * validation failed. * - * @param params supplies the incoming endpoint query params. + * @param params supplies the incoming endpoint query or post params. */ - absl::Status changeLogLevel(const Http::Utility::QueryParams& params); + absl::Status changeLogLevel(Http::Utility::QueryParams& params); void changeAllLogLevels(spdlog::level::level_enum level); absl::Status changeLogLevels(const absl::flat_hash_map& changes); @@ -52,7 +58,8 @@ class LogsHandler : public HandlerContextBase, Logger::Loggable log_levels_; + using StringViewLevelMap = absl::flat_hash_map; + const StringViewLevelMap log_levels_; }; } // namespace Server diff --git a/source/server/admin/profiling_handler.cc b/source/server/admin/profiling_handler.cc index c84b2adea5b61..fd2e5c0f49867 100644 --- a/source/server/admin/profiling_handler.cc +++ b/source/server/admin/profiling_handler.cc @@ -8,9 +8,10 @@ namespace Server { ProfilingHandler::ProfilingHandler(const std::string& profile_path) : profile_path_(profile_path) {} -Http::Code ProfilingHandler::handlerCpuProfiler(absl::string_view url, Http::ResponseHeaderMap&, - Buffer::Instance& response, AdminStream&) { - Http::Utility::QueryParams query_params = Http::Utility::parseAndDecodeQueryString(url); +Http::Code ProfilingHandler::handlerCpuProfiler(absl::string_view, Http::ResponseHeaderMap&, + Buffer::Instance& response, + AdminStream& admin_stream) { + Http::Utility::QueryParams query_params = admin_stream.queryParams(); if (query_params.size() != 1 || query_params.begin()->first != "enable" || (query_params.begin()->second != "y" && query_params.begin()->second != "n")) { response.add("?enable=\n"); @@ -32,14 +33,15 @@ Http::Code ProfilingHandler::handlerCpuProfiler(absl::string_view url, Http::Res return Http::Code::OK; } -Http::Code ProfilingHandler::handlerHeapProfiler(absl::string_view url, Http::ResponseHeaderMap&, - Buffer::Instance& response, AdminStream&) { +Http::Code ProfilingHandler::handlerHeapProfiler(absl::string_view, Http::ResponseHeaderMap&, + Buffer::Instance& response, + AdminStream& admin_stream) { if (!Profiler::Heap::profilerEnabled()) { response.add("The current build does not support heap profiler"); return Http::Code::NotImplemented; } - Http::Utility::QueryParams query_params = Http::Utility::parseAndDecodeQueryString(url); + Http::Utility::QueryParams query_params = admin_stream.queryParams(); if (query_params.size() != 1 || query_params.begin()->first != "enable" || (query_params.begin()->second != "y" && query_params.begin()->second != "n")) { response.add("?enable=\n"); diff --git a/source/server/admin/runtime_handler.cc b/source/server/admin/runtime_handler.cc index c7fdc07e27483..d9bdc8f557dda 100644 --- a/source/server/admin/runtime_handler.cc +++ b/source/server/admin/runtime_handler.cc @@ -15,10 +15,10 @@ namespace Server { RuntimeHandler::RuntimeHandler(Server::Instance& server) : HandlerContextBase(server) {} -Http::Code RuntimeHandler::handlerRuntime(absl::string_view url, +Http::Code RuntimeHandler::handlerRuntime(absl::string_view, Http::ResponseHeaderMap& response_headers, - Buffer::Instance& response, AdminStream&) { - const Http::Utility::QueryParams params = Http::Utility::parseAndDecodeQueryString(url); + Buffer::Instance& response, AdminStream& admin_stream) { + const Http::Utility::QueryParams params = admin_stream.queryParams(); response_headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Json); // TODO(jsedgwick): Use proto to structure this output instead of arbitrary JSON. @@ -77,24 +77,15 @@ Http::Code RuntimeHandler::handlerRuntime(absl::string_view url, return Http::Code::OK; } -Http::Code RuntimeHandler::handlerRuntimeModify(absl::string_view url, Http::ResponseHeaderMap&, +Http::Code RuntimeHandler::handlerRuntimeModify(absl::string_view, Http::ResponseHeaderMap&, Buffer::Instance& response, AdminStream& admin_stream) { - Http::Utility::QueryParams params = Http::Utility::parseAndDecodeQueryString(url); + Http::Utility::QueryParams params = admin_stream.queryParams(); if (params.empty()) { - // Check if the params are in the request's body. - if (admin_stream.getRequestBody() != nullptr && - admin_stream.getRequestHeaders().getContentTypeValue() == - Http::Headers::get().ContentTypeValues.FormUrlEncoded) { - params = Http::Utility::parseFromBody(admin_stream.getRequestBody()->toString()); - } - - if (params.empty()) { - response.add("usage: /runtime_modify?key1=value1&key2=value2&keyN=valueN\n"); - response.add(" or send the parameters as form values\n"); - response.add("use an empty value to remove a previously added override"); - return Http::Code::BadRequest; - } + response.add("usage: /runtime_modify?key1=value1&key2=value2&keyN=valueN\n"); + response.add(" or send the parameters as form values\n"); + response.add("use an empty value to remove a previously added override"); + return Http::Code::BadRequest; } absl::node_hash_map overrides; overrides.insert(params.begin(), params.end()); diff --git a/source/server/admin/utils.cc b/source/server/admin/utils.cc index 3dd500a410668..20862721fb3d3 100644 --- a/source/server/admin/utils.cc +++ b/source/server/admin/utils.cc @@ -45,13 +45,15 @@ bool filterParam(Http::Utility::QueryParams params, Buffer::Instance& response, auto p = params.find("filter"); if (p != params.end()) { const std::string& pattern = p->second; - TRY_ASSERT_MAIN_THREAD { regex = std::make_shared(pattern); } - END_TRY - catch (std::regex_error& error) { - // Include the offending pattern in the log, but not the error message. - response.add(fmt::format("Invalid regex: \"{}\"\n", error.what())); - ENVOY_LOG_MISC(error, "admin: Invalid regex: \"{}\": {}", error.what(), pattern); - return false; + if (!pattern.empty()) { + TRY_ASSERT_MAIN_THREAD { regex = std::make_shared(pattern); } + END_TRY + catch (std::regex_error& error) { + // Include the offending pattern in the log, but not the error message. + response.add(fmt::format("Invalid regex: \"{}\"\n", error.what())); + ENVOY_LOG_MISC(error, "admin: Invalid regex: \"{}\": {}", error.what(), pattern); + return false; + } } } return true; @@ -85,8 +87,14 @@ absl::optional formatParam(const Http::Utility::QueryParams& params // Helper method to get a query parameter. absl::optional queryParam(const Http::Utility::QueryParams& params, const std::string& key) { - return (params.find(key) != params.end()) ? absl::optional{params.at(key)} - : absl::nullopt; + const auto iter = params.find(key); + if (iter != params.end()) { + const std::string& value = iter->second; + if (!value.empty()) { + return value; + } + } + return absl::nullopt; } } // namespace Utility diff --git a/test/mocks/server/admin_stream.h b/test/mocks/server/admin_stream.h index 81fa85bd4514d..f4d52d277e3be 100644 --- a/test/mocks/server/admin_stream.h +++ b/test/mocks/server/admin_stream.h @@ -26,6 +26,7 @@ class MockAdminStream : public AdminStream { MOCK_METHOD(NiceMock&, getDecoderFilterCallbacks, (), (const)); MOCK_METHOD(Http::Http1StreamEncoderOptionsOptRef, http1StreamEncoderOptions, ()); + MOCK_METHOD(Http::Utility::QueryParams, queryParams, (), (const)); }; /** @@ -45,6 +46,7 @@ class StrictMockAdminStream : public AdminStream { MOCK_METHOD(::testing::StrictMock&, getDecoderFilterCallbacks, (), (const)); MOCK_METHOD(Http::Http1StreamEncoderOptionsOptRef, http1StreamEncoderOptions, ()); + MOCK_METHOD(Http::Utility::QueryParams, queryParams, (), (const)); }; } // namespace Server } // namespace Envoy diff --git a/test/per_file_coverage.sh b/test/per_file_coverage.sh index fce56daf274cc..d0e7f1a48d20d 100755 --- a/test/per_file_coverage.sh +++ b/test/per_file_coverage.sh @@ -93,7 +93,8 @@ declare -a KNOWN_LOW_COVERAGE=( "source/extensions/watchdog:83.3" # Death tests within extensions "source/extensions/watchdog/profile_action:83.3" "source/server:93.3" # flaky: be careful adjusting. See https://github.com/envoyproxy/envoy/issues/15239 -"source/server/admin:97.6" +"source/server/admin:97.5" +"source/server/admin:profiler-lib:83" "source/server/config_validation:74.8" ) diff --git a/test/server/admin/admin_instance.cc b/test/server/admin/admin_instance.cc index 83c40aa841b44..478430758bee5 100644 --- a/test/server/admin/admin_instance.cc +++ b/test/server/admin/admin_instance.cc @@ -35,6 +35,7 @@ Http::Code AdminInstanceTest::runCallback(absl::string_view path_and_query, } request_headers_.setMethod(method); + request_headers_.setPath(path_and_query); admin_filter_.decodeHeaders(request_headers_, false); return admin_.runCallback(path_and_query, response_headers, response, admin_filter_); diff --git a/test/server/admin/admin_test.cc b/test/server/admin/admin_test.cc index 53313407f5f82..19da5f6257e4a 100644 --- a/test/server/admin/admin_test.cc +++ b/test/server/admin/admin_test.cc @@ -140,26 +140,32 @@ TEST_P(AdminInstanceTest, Help) { name_regex: Dump only the currently loaded configurations whose names match the specified regex. Can be used with both resource and mask query parameters. include_eds: Dump currently loaded configuration including EDS. See the response definition for more information /contention: dump current Envoy mutex contention stats (if enabled) - /cpuprofiler: enable/disable the CPU profiler - /drain_listeners: drain listeners - /healthcheck/fail: cause the server to fail health checks - /healthcheck/ok: cause the server to pass health checks + /cpuprofiler (POST): enable/disable the CPU profiler + enable: enables the CPU profiler; One of (y, n) + /drain_listeners (POST): drain listeners + graceful: When draining listeners, enter a graceful drain period prior to closing listeners. This behaviour and duration is configurable via server options or CLI + inboundonly: Drains all inbound listeners. traffic_direction field in envoy_v3_api_msg_config.listener.v3.Listener is used to determine whether a listener is inbound or outbound. + /healthcheck/fail (POST): cause the server to fail health checks + /healthcheck/ok (POST): cause the server to pass health checks /heap_dump: dump current Envoy heap (if supported) - /heapprofiler: enable/disable the heap profiler + /heapprofiler (POST): enable/disable the heap profiler + enable: enable/disable the heap profiler; One of (y, n) /help: print out list of admin commands /hot_restart_version: print the hot restart compatibility version /init_dump: dump current Envoy init manager information (experimental) mask: The desired component to dump unready targets. The mask is parsed as a ProtobufWkt::FieldMask. For example, get the unready targets of all listeners with /init_dump?mask=listener` /listeners: print listener info format: File format to use; One of (text, json) - /logging: query/change logging levels + /logging (POST): query/change logging levels + paths: Change multiple logging levels by setting to :,:. + level: desired logging level; One of (, trace, debug, info, warning, error, critical, off) /memory: print current allocation/heap usage - /quitquitquit: exit the server + /quitquitquit (POST): exit the server /ready: print server state, return 200 if LIVE, otherwise return 503 - /reopen_logs: reopen access logs - /reset_counters: reset all counters to zero + /reopen_logs (POST): reopen access logs + /reset_counters (POST): reset all counters to zero /runtime: print runtime values - /runtime_modify: Adds or modifies runtime values as passed in query parameters. To delete a previously added key, use an empty string as the value. Note that deletion only applies to overrides added via this endpoint; values loaded from disk can be modified via override but not deleted. E.g. ?key1=value1&key2=value2... + /runtime_modify (POST): Adds or modifies runtime values as passed in query parameters. To delete a previously added key, use an empty string as the value. Note that deletion only applies to overrides added via this endpoint; values loaded from disk can be modified via override but not deleted. E.g. ?key1=value1&key2=value2... /server_info: print server version/status information /stats: print server stats usedonly: Only include stats that have been written by system since restart @@ -172,9 +178,9 @@ TEST_P(AdminInstanceTest, Help) { text_readouts: Render text_readouts as new gaugues with value 0 (increases Prometheus data size) filter: Regular expression (ecmascript) for filtering stats /stats/recentlookups: Show recent stat-name lookups - /stats/recentlookups/clear: clear list of stat-name lookups and counter - /stats/recentlookups/disable: disable recording of reset stat-name lookup names - /stats/recentlookups/enable: enable recording of reset stat-name lookup names + /stats/recentlookups/clear (POST): clear list of stat-name lookups and counter + /stats/recentlookups/disable (POST): disable recording of reset stat-name lookup names + /stats/recentlookups/enable (POST): enable recording of reset stat-name lookup names )EOF"; EXPECT_EQ(expected, response.toString()); } diff --git a/test/server/admin/logs_handler_test.cc b/test/server/admin/logs_handler_test.cc index 0cebd1a422c87..1bc8be70925d1 100644 --- a/test/server/admin/logs_handler_test.cc +++ b/test/server/admin/logs_handler_test.cc @@ -60,6 +60,14 @@ TEST_P(AdminInstanceTest, LogLevelSetting) { FINE_GRAIN_LOG(trace, "After post 4: level for this file is trace now!"); EXPECT_EQ(getFineGrainLogContext().getFineGrainLogEntry(__FILE__)->level(), spdlog::level::trace); EXPECT_EQ(getFineGrainLogContext().getFineGrainLogEntry(file)->level(), spdlog::level::trace); + + // You can't set the overall level and multiple levels at once at the same time. + EXPECT_EQ(Http::Code::BadRequest, postCallback(query + "&level=info", header_map, response)); + + // It's OK to set a path with a blank level -- that happens with HTML forms. + EXPECT_EQ(Http::Code::OK, postCallback(query + "&level=", header_map, response)); + // Likewise it's OK to set the level even if there's a blank path. + EXPECT_EQ(Http::Code::OK, postCallback("/logging?level=warning&paths=", header_map, response)); } } // namespace Server diff --git a/test/server/admin/profiling_handler_test.cc b/test/server/admin/profiling_handler_test.cc index cef2c8f3360d0..e4417c54748d6 100644 --- a/test/server/admin/profiling_handler_test.cc +++ b/test/server/admin/profiling_handler_test.cc @@ -69,12 +69,13 @@ TEST_P(AdminInstanceTest, AdminBadProfiler) { AdminImpl admin_bad_profile_path(TestEnvironment::temporaryPath("some/unlikely/bad/path.prof"), server_, false); Http::TestResponseHeaderMapImpl header_map; - const absl::string_view post = Http::Headers::get().MethodValues.Post; - request_headers_.setMethod(post); + constexpr absl::string_view url = "/cpuprofiler?enable=y"; + request_headers_.setMethod(Http::Headers::get().MethodValues.Post); + request_headers_.setPath(url); admin_filter_.decodeHeaders(request_headers_, false); - EXPECT_NO_LOGS(EXPECT_EQ(Http::Code::InternalServerError, - admin_bad_profile_path.runCallback("/cpuprofiler?enable=y", header_map, - data, admin_filter_))); + EXPECT_NO_LOGS( + EXPECT_EQ(Http::Code::InternalServerError, + admin_bad_profile_path.runCallback(url, header_map, data, admin_filter_))); EXPECT_FALSE(Profiler::Cpu::profilerEnabled()); } diff --git a/test/server/admin/utils_test.cc b/test/server/admin/utils_test.cc index 8a97da84a96bf..a0e8364ffdda1 100644 --- a/test/server/admin/utils_test.cc +++ b/test/server/admin/utils_test.cc @@ -46,5 +46,14 @@ TEST_F(UtilsTest, HistogramMode) { HasSubstr("usage: /stats?histogram_buckets=(cumulative|disjoint|none)")); } +TEST_F(UtilsTest, QueryParam) { + EXPECT_FALSE(Utility::queryParam(query_, "key").has_value()); + query_["key"] = ""; + EXPECT_FALSE(Utility::queryParam(query_, "key").has_value()); + query_["key"] = "value"; + EXPECT_TRUE(Utility::queryParam(query_, "key").has_value()); + EXPECT_EQ("value", Utility::queryParam(query_, "key").value()); +} + } // namespace Server } // namespace Envoy