From c4101e19d740335c6811efc54adc3ec150cef843 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 13 Jan 2022 22:50:20 -0500 Subject: [PATCH 01/61] initial patch Signed-off-by: Joshua Marantz --- envoy/server/admin.h | 26 +- source/extensions/common/tap/admin.cc | 2 +- .../extensions/stat_sinks/hystrix/hystrix.cc | 2 +- source/server/admin/BUILD | 33 +- source/server/admin/admin.cc | 343 +++++----- source/server/admin/admin.css | 71 ++ source/server/admin/admin.h | 14 +- source/server/admin/admin_head_start.html | 2 + source/server/admin/admin_html_generator.cc | 179 +++++ source/server/admin/admin_html_generator.h | 36 + source/server/admin/generate_admin_html.sh | 15 + source/server/admin/stats_handler.cc | 635 +++++++++++++++--- source/server/admin/stats_handler.h | 68 +- source/server/config_validation/admin.cc | 3 +- source/server/config_validation/admin.h | 3 +- test/extensions/common/tap/admin_test.cc | 2 +- .../http/common/fuzz/uber_per_filter.cc | 3 +- test/integration/integration_admin_test.cc | 4 +- test/integration/server.h | 11 +- test/mocks/server/admin.cc | 2 +- test/mocks/server/admin.h | 2 +- test/server/admin/admin_test.cc | 23 +- test/server/admin/stats_handler_test.cc | 269 +++++--- test/server/config_validation/server_test.cc | 2 +- tools/spelling/spelling_dictionary.txt | 2 + 25 files changed, 1369 insertions(+), 383 deletions(-) create mode 100644 source/server/admin/admin.css create mode 100644 source/server/admin/admin_head_start.html create mode 100644 source/server/admin/admin_html_generator.cc create mode 100644 source/server/admin/admin_html_generator.h create mode 100755 source/server/admin/generate_admin_html.sh diff --git a/envoy/server/admin.h b/envoy/server/admin.h index 477bde02f175f..e901249d2e01b 100644 --- a/envoy/server/admin.h +++ b/envoy/server/admin.h @@ -74,7 +74,14 @@ class AdminStream { */ class Admin { public: - virtual ~Admin() = default; + struct ParamDescriptor { + enum class Type { Boolean, String, Enum, Mask, Hidden }; + 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. + std::vector choices_{}; // Valid values for enums or masks + }; + using ParamDescriptorVec = std::vector; /** * Callback for admin URL handlers. @@ -90,6 +97,20 @@ class Admin { absl::string_view path_and_query, Http::ResponseHeaderMap& response_headers, Buffer::Instance& response, AdminStream& admin_stream)>; + /** + * Individual admin handler including prefix, help text, and callback. + */ + struct UrlHandler { + const std::string prefix_; + const std::string help_text_; + const HandlerCb handler_; + const bool removable_; + const bool mutates_server_state_; + const ParamDescriptorVec params_; + }; + + virtual ~Admin() = default; + /** * Add an admin handler. * @param prefix supplies the URL prefix to handle. @@ -100,7 +121,8 @@ class Admin { * @return bool true if the handler was added, false if it was not added. */ virtual bool addHandler(const std::string& prefix, const std::string& help_text, - HandlerCb callback, bool removable, bool mutates_server_state) PURE; + HandlerCb callback, bool removable, bool mutates_server_state, + const ParamDescriptorVec& params) PURE; /** * Remove an admin handler if it is removable. diff --git a/source/extensions/common/tap/admin.cc b/source/extensions/common/tap/admin.cc index 8c674cb141f6f..c82eb54e127bb 100644 --- a/source/extensions/common/tap/admin.cc +++ b/source/extensions/common/tap/admin.cc @@ -29,7 +29,7 @@ AdminHandlerSharedPtr AdminHandler::getSingleton(Server::Admin& admin, AdminHandler::AdminHandler(Server::Admin& admin, Event::Dispatcher& main_thread_dispatcher) : admin_(admin), main_thread_dispatcher_(main_thread_dispatcher) { const bool rc = - admin_.addHandler("/tap", "tap filter control", MAKE_ADMIN_HANDLER(handler), true, true); + admin_.addHandler("/tap", "tap filter control", MAKE_ADMIN_HANDLER(handler), true, true, {}); RELEASE_ASSERT(rc, "/tap admin endpoint is taken"); if (admin_.socket().addressType() == Network::Address::Type::Pipe) { ENVOY_LOG(warn, "Admin tapping (via /tap) is unreliable when the admin endpoint is a pipe and " diff --git a/source/extensions/stat_sinks/hystrix/hystrix.cc b/source/extensions/stat_sinks/hystrix/hystrix.cc index 1df26f9e9448f..3a29849c9a9f2 100644 --- a/source/extensions/stat_sinks/hystrix/hystrix.cc +++ b/source/extensions/stat_sinks/hystrix/hystrix.cc @@ -287,7 +287,7 @@ HystrixSink::HystrixSink(Server::Configuration::ServerFactoryContext& server, ENVOY_LOG(debug, "adding hystrix_event_stream endpoint to enable connection to hystrix dashboard"); admin.addHandler("/hystrix_event_stream", "send hystrix event stream", - MAKE_ADMIN_HANDLER(handlerHystrixEventStream), false, false); + MAKE_ADMIN_HANDLER(handlerHystrixEventStream), false, false, {}); } Http::Code HystrixSink::handlerHystrixEventStream(absl::string_view, diff --git a/source/server/admin/BUILD b/source/server/admin/BUILD index 365a5b8109a9a..3e18e25c1d5f3 100644 --- a/source/server/admin/BUILD +++ b/source/server/admin/BUILD @@ -14,6 +14,7 @@ envoy_cc_library( hdrs = ["admin.h"], deps = [ ":admin_filter_lib", + ":admin_html_generator_lib", ":clusters_handler_lib", ":config_dump_handler_lib", ":config_tracker_lib", @@ -45,7 +46,6 @@ envoy_cc_library( "//source/common/common:mutex_tracer_lib", "//source/common/common:utility_lib", "//source/common/formatter:substitution_formatter_lib", - "//source/common/html:utility_lib", "//source/common/http:codes_lib", "//source/common/http:conn_manager_lib", "//source/common/http:date_provider_lib", @@ -83,6 +83,36 @@ envoy_cc_library( ], ) +genrule( + name = "generate_admin_html", + srcs = [ + "admin_head_start.html", + "admin.css", + "admin.js", + ], + outs = ["admin_html_gen.h"], + cmd = "./$(location :generate_admin_html.sh) \ + $(location admin_head_start.html) $(location admin.css) \ + $(location admin.js)> $@", + tools = [":generate_admin_html.sh"], + visibility = ["//visibility:private"], +) + +envoy_cc_library( + name = "admin_html_generator_lib", + srcs = [ + "admin_html_generator.cc", + ":generate_admin_html", + ], + hdrs = ["admin_html_generator.h"], + deps = [ + "//envoy/server:admin_interface", + "//source/common/buffer:buffer_lib", + "//source/common/common:assert_lib", + "//source/common/html:utility_lib", + ], +) + envoy_cc_library( name = "handler_ctx_lib", hdrs = ["handler_ctx.h"], @@ -96,6 +126,7 @@ envoy_cc_library( srcs = ["stats_handler.cc"], hdrs = ["stats_handler.h"], deps = [ + ":admin_html_generator_lib", ":handler_ctx_lib", ":prometheus_stats_lib", ":utils_lib", diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc index 0039d7cf4567b..35f99c583713e 100644 --- a/source/server/admin/admin.cc +++ b/source/server/admin/admin.cc @@ -23,7 +23,6 @@ #include "source/common/common/mutex_tracer_impl.h" #include "source/common/common/utility.h" #include "source/common/formatter/substitution_formatter.h" -#include "source/common/html/utility.h" #include "source/common/http/codes.h" #include "source/common/http/conn_manager_utility.h" #include "source/common/http/header_map_impl.h" @@ -34,6 +33,7 @@ #include "source/common/protobuf/utility.h" #include "source/common/router/config_impl.h" #include "source/extensions/request_id/uuid/config.h" +#include "source/server/admin/admin_html_generator.h" #include "source/server/admin/utils.h" #include "source/server/listener_impl.h" @@ -45,72 +45,6 @@ namespace Envoy { namespace Server { -namespace { - -/** - * Favicon base64 image was harvested by screen-capturing the favicon from a Chrome tab - * while visiting https://www.envoyproxy.io/. The resulting PNG was translated to base64 - * by dropping it into https://www.base64-image.de/ and then pasting the resulting string - * below. - * - * The actual favicon source for that, https://www.envoyproxy.io/img/favicon.ico is nicer - * because it's transparent, but is also 67646 bytes, which is annoying to inline. We could - * just reference that rather than inlining it, but then the favicon won't work when visiting - * the admin page from a network that can't see the internet. - */ -const char EnvoyFavicon[] = - "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABgAAAAYCAYAAADgdz34AAAAAXNSR0IArs4c6QAAAARnQU1" - "BAACxjwv8YQUAAAAJcEhZcwAAEnQAABJ0Ad5mH3gAAAH9SURBVEhL7ZRdTttAFIUrUFaAX5w9gIhgUfzshFRK+gIbaVbA" - "zwaqCly1dSpKk5A485/YCdXpHTB4BsdgVe0bD0cZ3Xsm38yZ8byTUuJ/6g3wqqoBrBhPTzmmLfptMbAzttJTpTKAF2MWC" - "7ADCdNIwXZpvMMwayiIwwS874CcOc9VuQPR1dBBChPMITpFXXU45hukIIH6kHhzVqkEYB8F5HYGvZ5B7EvwmHt9K/59Cr" - "U3QbY2RNYaQPYmJc+jPIBICNCcg20ZsAsCPfbcrFlRF+cJZpvXSJt9yMTxO/IAzJrCOfhJXiOgFEX/SbZmezTWxyNk4Q9" - "anHMmjnzAhEyhAW8LCE6wl26J7ZFHH1FMYQxh567weQBOO1AW8D7P/UXAQySq/QvL8Fu9HfCEw4SKALm5BkC3bwjwhSKr" - "A5hYAMXTJnPNiMyRBVzVjcgCyHiSm+8P+WGlnmwtP2RzbCMiQJ0d2KtmmmPorRHEhfMROVfTG5/fYrF5iWXzE80tfy9WP" - "sCqx5Buj7FYH0LvDyHiqd+3otpsr4/fa5+xbEVQPfrYnntylQG5VGeMLBhgEfyE7o6e6qYzwHIjwl0QwXSvvTmrVAY4D5" - "ddvT64wV0jRrr7FekO/XEjwuwwhuw7Ef7NY+dlfXpLb06EtHUJdVbsxvNUqBrwj/QGeEUSfwBAkmWHn5Bb/gAAAABJRU5"; - -const char AdminHtmlStart[] = R"( - - Envoy Admin - - - - - - - - - - -)"; - -const char AdminHtmlEnd[] = R"( - -
CommandDescription
- -)"; -} // namespace - ConfigTracker& AdminImpl::getConfigTracker() { return config_tracker_; } AdminImpl::NullRouteConfigProvider::NullRouteConfigProvider(TimeSource& time_source) @@ -161,63 +95,175 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server, server_info_handler_(server), // TODO(jsedgwick) add /runtime_reset endpoint that removes all admin-set values handlers_{ - {"/", "Admin home page", MAKE_ADMIN_HANDLER(handlerAdminHome), false, false}, - {"/certs", "print certs on machine", - MAKE_ADMIN_HANDLER(server_info_handler_.handlerCerts), false, false}, - {"/clusters", "upstream cluster status", - MAKE_ADMIN_HANDLER(clusters_handler_.handlerClusters), false, false}, - {"/config_dump", "dump current Envoy configs (experimental)", - MAKE_ADMIN_HANDLER(config_dump_handler_.handlerConfigDump), false, false}, - {"/init_dump", "dump current Envoy init manager information (experimental)", - MAKE_ADMIN_HANDLER(init_dump_handler_.handlerInitDump), false, false}, - {"/contention", "dump current Envoy mutex contention stats (if enabled)", - MAKE_ADMIN_HANDLER(stats_handler_.handlerContention), false, false}, - {"/cpuprofiler", "enable/disable the CPU profiler", - MAKE_ADMIN_HANDLER(profiling_handler_.handlerCpuProfiler), false, true}, - {"/heapprofiler", "enable/disable the heap profiler", - MAKE_ADMIN_HANDLER(profiling_handler_.handlerHeapProfiler), false, true}, - {"/healthcheck/fail", "cause the server to fail health checks", - MAKE_ADMIN_HANDLER(server_cmd_handler_.handlerHealthcheckFail), false, true}, - {"/healthcheck/ok", "cause the server to pass health checks", - MAKE_ADMIN_HANDLER(server_cmd_handler_.handlerHealthcheckOk), false, true}, - {"/help", "print out list of admin commands", MAKE_ADMIN_HANDLER(handlerHelp), false, - false}, - {"/hot_restart_version", "print the hot restart compatibility version", - MAKE_ADMIN_HANDLER(server_info_handler_.handlerHotRestartVersion), false, false}, - {"/logging", "query/change logging levels", - MAKE_ADMIN_HANDLER(logs_handler_.handlerLogging), false, true}, - {"/memory", "print current allocation/heap usage", - MAKE_ADMIN_HANDLER(server_info_handler_.handlerMemory), false, false}, - {"/quitquitquit", "exit the server", - MAKE_ADMIN_HANDLER(server_cmd_handler_.handlerQuitQuitQuit), false, true}, - {"/reset_counters", "reset all counters to zero", - MAKE_ADMIN_HANDLER(stats_handler_.handlerResetCounters), false, true}, - {"/drain_listeners", "drain listeners", - MAKE_ADMIN_HANDLER(listeners_handler_.handlerDrainListeners), false, true}, - {"/server_info", "print server version/status information", - MAKE_ADMIN_HANDLER(server_info_handler_.handlerServerInfo), false, false}, - {"/ready", "print server state, return 200 if LIVE, otherwise return 503", - MAKE_ADMIN_HANDLER(server_info_handler_.handlerReady), false, false}, - {"/stats", "print server stats", MAKE_ADMIN_HANDLER(stats_handler_.handlerStats), false, - false}, - {"/stats/prometheus", "print server stats in prometheus format", - MAKE_ADMIN_HANDLER(stats_handler_.handlerPrometheusStats), false, false}, - {"/stats/recentlookups", "Show recent stat-name lookups", - MAKE_ADMIN_HANDLER(stats_handler_.handlerStatsRecentLookups), false, false}, - {"/stats/recentlookups/clear", "clear list of stat-name lookups and counter", - MAKE_ADMIN_HANDLER(stats_handler_.handlerStatsRecentLookupsClear), false, true}, - {"/stats/recentlookups/disable", "disable recording of reset stat-name lookup names", - MAKE_ADMIN_HANDLER(stats_handler_.handlerStatsRecentLookupsDisable), false, true}, - {"/stats/recentlookups/enable", "enable recording of reset stat-name lookup names", - MAKE_ADMIN_HANDLER(stats_handler_.handlerStatsRecentLookupsEnable), false, true}, - {"/listeners", "print listener info", - MAKE_ADMIN_HANDLER(listeners_handler_.handlerListenerInfo), false, false}, - {"/runtime", "print runtime values", MAKE_ADMIN_HANDLER(runtime_handler_.handlerRuntime), - false, false}, - {"/runtime_modify", "modify runtime values", - MAKE_ADMIN_HANDLER(runtime_handler_.handlerRuntimeModify), false, true}, - {"/reopen_logs", "reopen access logs", - MAKE_ADMIN_HANDLER(logs_handler_.handlerReopenLogs), false, true}, + {"/", "Admin home page", MAKE_ADMIN_HANDLER(handlerAdminHome), false, false, {}}, + {"/certs", + "print certs on machine", + MAKE_ADMIN_HANDLER(server_info_handler_.handlerCerts), + false, + false, + {}}, + {"/clusters", + "upstream cluster status", + MAKE_ADMIN_HANDLER(clusters_handler_.handlerClusters), + false, + false, + {}}, + {"/config_dump", + "dump current Envoy configs (experimental)", + MAKE_ADMIN_HANDLER(config_dump_handler_.handlerConfigDump), + false, + false, + {}}, + {"/init_dump", + "dump current Envoy init manager information (experimental)", + MAKE_ADMIN_HANDLER(init_dump_handler_.handlerInitDump), + false, + false, + {}}, + {"/contention", + "dump current Envoy mutex contention stats (if enabled)", + MAKE_ADMIN_HANDLER(stats_handler_.handlerContention), + false, + false, + {}}, + {"/cpuprofiler", + "enable/disable the CPU profiler", + MAKE_ADMIN_HANDLER(profiling_handler_.handlerCpuProfiler), + false, + true, + {}}, + {"/heapprofiler", + "enable/disable the heap profiler", + MAKE_ADMIN_HANDLER(profiling_handler_.handlerHeapProfiler), + false, + true, + {}}, + {"/healthcheck/fail", + "cause the server to fail health checks", + MAKE_ADMIN_HANDLER(server_cmd_handler_.handlerHealthcheckFail), + false, + true, + {}}, + {"/healthcheck/ok", + "cause the server to pass health checks", + MAKE_ADMIN_HANDLER(server_cmd_handler_.handlerHealthcheckOk), + false, + true, + {}}, + {"/help", + "print out list of admin commands", + MAKE_ADMIN_HANDLER(handlerHelp), + false, + false, + {}}, + {"/hot_restart_version", + "print the hot restart compatibility version", + MAKE_ADMIN_HANDLER(server_info_handler_.handlerHotRestartVersion), + false, + false, + {}}, + {"/logging", + "query/change logging levels", + MAKE_ADMIN_HANDLER(logs_handler_.handlerLogging), + false, + true, + {}}, + {"/memory", + "print current allocation/heap usage", + MAKE_ADMIN_HANDLER(server_info_handler_.handlerMemory), + false, + false, + {}}, + {"/quitquitquit", + "exit the server", + MAKE_ADMIN_HANDLER(server_cmd_handler_.handlerQuitQuitQuit), + false, + true, + {}}, + {"/reset_counters", + "reset all counters to zero", + MAKE_ADMIN_HANDLER(stats_handler_.handlerResetCounters), + false, + true, + {}}, + {"/drain_listeners", + "drain listeners", + MAKE_ADMIN_HANDLER(listeners_handler_.handlerDrainListeners), + false, + true, + {}}, + {"/server_info", + "print server version/status information", + MAKE_ADMIN_HANDLER(server_info_handler_.handlerServerInfo), + false, + false, + {}}, + {"/ready", + "print server state, return 200 if LIVE, otherwise return 503", + MAKE_ADMIN_HANDLER(server_info_handler_.handlerReady), + false, + false, + {}}, + stats_handler_.statsHandler(), + {"/stats/prometheus", + "print server stats in prometheus format", + MAKE_ADMIN_HANDLER(stats_handler_.handlerStatsPrometheus), + false, + false, + {{ParamDescriptor::Type::Boolean, "usedonly", + "Only include stats that have been written by system since restart"}, + {ParamDescriptor::Type::Boolean, "text_readouts", + "Render text_readouts as new gaugues with value 0 (increases Prometheus data size)"}, + {ParamDescriptor::Type::String, "filter", + "Regular expression (ecmascript) for filtering stats"}}}, + {"/stats/recentlookups", + "Show recent stat-name lookups", + MAKE_ADMIN_HANDLER(stats_handler_.handlerStatsRecentLookups), + false, + false, + {}}, + {"/stats/recentlookups/clear", + "clear list of stat-name lookups and counter", + MAKE_ADMIN_HANDLER(stats_handler_.handlerStatsRecentLookupsClear), + false, + true, + {}}, + {"/stats/recentlookups/disable", + "disable recording of reset stat-name lookup names", + MAKE_ADMIN_HANDLER(stats_handler_.handlerStatsRecentLookupsDisable), + false, + true, + {}}, + {"/stats/recentlookups/enable", + "enable recording of reset stat-name lookup names", + MAKE_ADMIN_HANDLER(stats_handler_.handlerStatsRecentLookupsEnable), + false, + true, + {}}, + {"/listeners", + "print listener info", + MAKE_ADMIN_HANDLER(listeners_handler_.handlerListenerInfo), + false, + false, + {}}, + {"/runtime", + "print runtime values", + MAKE_ADMIN_HANDLER(runtime_handler_.handlerRuntime), + false, + false, + {}}, + {"/runtime_modify", + "modify runtime values", + MAKE_ADMIN_HANDLER(runtime_handler_.handlerRuntimeModify), + false, + true, + {}}, + {"/reopen_logs", + "reopen access logs", + MAKE_ADMIN_HANDLER(logs_handler_.handlerReopenLogs), + false, + true, + {}}, }, date_provider_(server.dispatcher().timeSource()), admin_filter_chain_(std::make_shared()), @@ -317,47 +363,19 @@ Http::Code AdminImpl::handlerHelp(absl::string_view, Http::ResponseHeaderMap&, Http::Code AdminImpl::handlerAdminHome(absl::string_view, Http::ResponseHeaderMap& response_headers, Buffer::Instance& response, AdminStream&) { response_headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Html); - - response.add(absl::StrReplaceAll(AdminHtmlStart, {{"@FAVICON@", EnvoyFavicon}})); + AdminHtmlGenerator html(response); + html.renderHead(); + html.renderTableBegin(); // Prefix order is used during searching, but for printing do them in alpha order. + OptRef no_query_params; for (const UrlHandler* handler : sortedHandlers()) { - absl::string_view path = handler->prefix_; + html.renderUrlHandler(*handler, no_query_params); + } - if (path == "/") { - continue; // No need to print self-link to index page. - } + html.renderTableEnd(); - // Remove the leading slash from the link, so that the admin page can be - // rendered as part of another console, on a sub-path. - // - // E.g. consider a downstream dashboard that embeds the Envoy admin console. - // In that case, the "/stats" endpoint would be at - // https://DASHBOARD/envoy_admin/stats. If the links we present on the home - // page are absolute (e.g. "/stats") they won't work in the context of the - // dashboard. Removing the leading slash, they will work properly in both - // the raw admin console and when embedded in another page and URL - // hierarchy. - ASSERT(!path.empty()); - ASSERT(path[0] == '/'); - path = path.substr(1); - - // For handlers that mutate state, render the link as a button in a POST form, - // rather than an anchor tag. This should discourage crawlers that find the / - // page from accidentally mutating all the server state by GETting all the hrefs. - const char* link_format = - handler->mutates_server_state_ - ? "
" - : "{}"; - const std::string link = fmt::format(link_format, path, path); - - // Handlers are all specified by statically above, and are thus trusted and do - // not require escaping. - response.add(fmt::format("{}" - "{}\n", - link, Html::Utility::sanitize(handler->help_text_))); - } - response.add(AdminHtmlEnd); + // gen.renderTail(); return Http::Code::OK; } @@ -366,7 +384,8 @@ const Network::Address::Instance& AdminImpl::localAddress() { } bool AdminImpl::addHandler(const std::string& prefix, const std::string& help_text, - HandlerCb callback, bool removable, bool mutates_state) { + HandlerCb callback, bool removable, bool mutates_state, + const ParamDescriptorVec& params) { ASSERT(prefix.size() > 1); ASSERT(prefix[0] == '/'); @@ -383,7 +402,7 @@ bool AdminImpl::addHandler(const std::string& prefix, const std::string& help_te auto it = std::find_if(handlers_.cbegin(), handlers_.cend(), [&prefix](const UrlHandler& entry) { return prefix == entry.prefix_; }); if (it == handlers_.end()) { - handlers_.push_back({prefix, help_text, callback, removable, mutates_state}); + handlers_.push_back({prefix, help_text, callback, removable, mutates_state, params}); return true; } return false; diff --git a/source/server/admin/admin.css b/source/server/admin/admin.css new file mode 100644 index 0000000000000..6f48892c0e508 --- /dev/null +++ b/source/server/admin/admin.css @@ -0,0 +1,71 @@ +.home-table { + font-family: sans-serif; + font-size: medium; + border-collapse: collapse; + border-spacing: 0; +} + +.home-data { + text-align: left; + padding: 4px; +} + +.home-form { + margin-bottom: 0; +} + +.button-as-link { + background: none!important; + border: none; + padding: 0!important; + font-family: sans-serif; + font-size: medium; + color: #069; + text-decoration: underline; + cursor: pointer; +} + +.gray { + background-color: #dddddd; +} + +.vert-space { + height: 4px; +} + +.option { + padding-bottom: 4px; + padding-top: 4px; + padding-right: 4px; + padding-left: 20px; + text-align: right; +} + +#scopes-outline { + margin: 0; + padding: 0; + font-family: sans-serif +} + +.scope-children { + list-style-type: none; +} + +/* Style the caret/arrow */ +.caret { + cursor: pointer; + user-select: none; /* Prevent text selection */ +} + +/* Create the caret/arrow with a unicode, and style it */ +.caret::before { + content: "\25B6"; + color: black; + display: inline-block; + margin-right: 6px; +} + +/* Rotate the caret/arrow icon when clicked on (using JavaScript) */ +.caret-down::before { + transform: rotate(90deg); +}*/ diff --git a/source/server/admin/admin.h b/source/server/admin/admin.h index b2f84e42f8a72..b4f931b540d5c 100644 --- a/source/server/admin/admin.h +++ b/source/server/admin/admin.h @@ -83,7 +83,8 @@ class AdminImpl : public Admin, // // The prefix must start with "/" and contain at least one additional character. bool addHandler(const std::string& prefix, const std::string& help_text, HandlerCb callback, - bool removable, bool mutates_server_state) override; + bool removable, bool mutates_server_state, + const ParamDescriptorVec& params) override; bool removeHandler(const std::string& prefix) override; ConfigTracker& getConfigTracker() override; @@ -209,17 +210,6 @@ class AdminImpl : public Admin, uint64_t maxRequestsPerConnection() const override { return 0; } private: - /** - * Individual admin handler including prefix, help text, and callback. - */ - struct UrlHandler { - const std::string prefix_; - const std::string help_text_; - const HandlerCb handler_; - const bool removable_; - const bool mutates_server_state_; - }; - /** * Implementation of RouteConfigProvider that returns a static null route config. */ diff --git a/source/server/admin/admin_head_start.html b/source/server/admin/admin_head_start.html new file mode 100644 index 0000000000000..8096871b20c73 --- /dev/null +++ b/source/server/admin/admin_head_start.html @@ -0,0 +1,2 @@ + Envoy Admin + diff --git a/source/server/admin/admin_html_generator.cc b/source/server/admin/admin_html_generator.cc new file mode 100644 index 0000000000000..8351ef820c53f --- /dev/null +++ b/source/server/admin/admin_html_generator.cc @@ -0,0 +1,179 @@ +#include "source/server/admin/admin_html_generator.h" + +#include + +#include "source/common/buffer/buffer_impl.h" +#include "source/common/common/assert.h" +#include "source/common/html/utility.h" +#include "source/server/admin/admin_html_gen.h" + +#include "absl/strings/str_replace.h" + +namespace { + +/** + * Favicon base64 image was harvested by screen-capturing the favicon from a Chrome tab + * while visiting https://www.envoyproxy.io/. The resulting PNG was translated to base64 + * by dropping it into https://www.base64-image.de/ and then pasting the resulting string + * below. + * + * The actual favicon source for that, https://www.envoyproxy.io/img/favicon.ico is nicer + * because it's transparent, but is also 67646 bytes, which is annoying to inline. We could + * just reference that rather than inlining it, but then the favicon won't work when visiting + * the admin page from a network that can't see the internet. + */ +const char EnvoyFavicon[] = + "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABgAAAAYCAYAAADgdz34AAAAAXNSR0IArs4c6QAAAARnQU1" + "BAACxjwv8YQUAAAAJcEhZcwAAEnQAABJ0Ad5mH3gAAAH9SURBVEhL7ZRdTttAFIUrUFaAX5w9gIhgUfzshFRK+gIbaVbA" + "zwaqCly1dSpKk5A485/YCdXpHTB4BsdgVe0bD0cZ3Xsm38yZ8byTUuJ/6g3wqqoBrBhPTzmmLfptMbAzttJTpTKAF2MWC" + "7ADCdNIwXZpvMMwayiIwwS874CcOc9VuQPR1dBBChPMITpFXXU45hukIIH6kHhzVqkEYB8F5HYGvZ5B7EvwmHt9K/59Cr" + "U3QbY2RNYaQPYmJc+jPIBICNCcg20ZsAsCPfbcrFlRF+cJZpvXSJt9yMTxO/IAzJrCOfhJXiOgFEX/SbZmezTWxyNk4Q9" + "anHMmjnzAhEyhAW8LCE6wl26J7ZFHH1FMYQxh567weQBOO1AW8D7P/UXAQySq/QvL8Fu9HfCEw4SKALm5BkC3bwjwhSKr" + "A5hYAMXTJnPNiMyRBVzVjcgCyHiSm+8P+WGlnmwtP2RzbCMiQJ0d2KtmmmPorRHEhfMROVfTG5/fYrF5iWXzE80tfy9WP" + "sCqx5Buj7FYH0LvDyHiqd+3otpsr4/fa5+xbEVQPfrYnntylQG5VGeMLBhgEfyE7o6e6qYzwHIjwl0QwXSvvTmrVAY4D5" + "ddvT64wV0jRrr7FekO/XEjwuwwhuw7Ef7NY+dlfXpLb06EtHUJdVbsxvNUqBrwj/QGeEUSfwBAkmWHn5Bb/gAAAABJRU5"; + +const char AdminHtmlTableBegin[] = R"( + + + + + + + +)"; + +const char AdminHtmlTableEnd[] = R"( + +
CommandDescription
+)"; + +} // namespace + +namespace Envoy { +namespace Server { + +void AdminHtmlGenerator::renderHead() { + response_.add(absl::StrReplaceAll(AdminHtmlStart, {{"@FAVICON@", EnvoyFavicon}})); +} + +void AdminHtmlGenerator::renderTableBegin() { response_.add(AdminHtmlTableBegin); } + +void AdminHtmlGenerator::renderTableEnd() { response_.add(AdminHtmlTableEnd); } + +void AdminHtmlGenerator::renderUrlHandler(const Admin::UrlHandler& handler, + OptRef query) { + absl::string_view path = handler.prefix_; + + if (path == "/") { + return; // No need to print self-link to index page. + } + + // Remove the leading slash from the link, so that the admin page can be + // rendered as part of another console, on a sub-path. + // + // E.g. consider a downstream dashboard that embeds the Envoy admin console. + // In that case, the "/stats" endpoint would be at + // https://DASHBOARD/envoy_admin/stats. If the links we present on the home + // page are absolute (e.g. "/stats") they won't work in the context of the + // dashboard. Removing the leading slash, they will work properly in both + // the raw admin console and when embedded in another page and URL + // hierarchy. + ASSERT(!path.empty()); + ASSERT(path[0] == '/'); + path = path.substr(1); + + // Alternate gray and white param-blocks. The pure CSS way of doing based on + // row index doesn't work correctly for us we are using a row for each + // parameter, and we want each endpoint/option-block to be colored the same. + const char* row_class = (++index_ & 1) ? " class='gray'" : ""; + + // For handlers that mutate state, render the link as a button in a POST form, + // rather than an anchor tag. This should discourage crawlers that find the / + // page from accidentally mutating all the server state by GETting all the hrefs. + const char* button_style = handler.mutates_server_state_ ? "" : " class='button-as-link'"; + const char* method = handler.mutates_server_state_ ? "post" : "get"; + if (visible_submit_) { + response_.add(absl::StrCat("\n\n", "\n", + "
\n", " ", path, "\n", + " \n" + " ", + Html::Utility::sanitize(handler.help_text_), "\n", "\n")); + } else { + response_.add(absl::StrCat("\n
\n")); + } + + std::vector params; + for (const Admin::ParamDescriptor& param : handler.params_) { + response_.add(absl::StrCat("\n", " ")); + renderInput(param.id_, path, param.type_, query, param.choices_); + response_.add(absl::StrCat("\n", " ", + Html::Utility::sanitize(param.help_), "\n", "\n")); + } +} + +void AdminHtmlGenerator::renderInput(absl::string_view id, absl::string_view path, + Admin::ParamDescriptor::Type type, + OptRef query, + const std::vector& choices) { + std::string value; + if (query.has_value()) { + auto iter = query->find(std::string(id)); + if (iter != query->end()) { + value = iter->second; + } + } + + std::string on_change; + if (submit_on_change_) { + on_change = absl::StrCat(" onchange='", path, ".submit()'"); + } + + switch (type) { + case Admin::ParamDescriptor::Type::Boolean: + response_.add(absl::StrCat("")); + break; + case Admin::ParamDescriptor::Type::String: + response_.add(absl::StrCat("")); + break; + case Admin::ParamDescriptor::Type::Hidden: + response_.add(absl::StrCat("")); + break; + case Admin::ParamDescriptor::Type::Mask: { + response_.add("\n"); + for (size_t row = 0; row < (choices.size() + 1) / 2; ++row) { + response_.add(" \n "); + uint32_t last_index = std::min(2 * (row + 1), choices.size()); + for (size_t i = 2 * row; i < last_index; ++i) { + response_.add(absl::StrCat(" \n")); + } + response_.add(" \n"); + } + response_.add("
", choices[i], "
\n "); + break; + } + case Admin::ParamDescriptor::Type::Enum: + response_.add(absl::StrCat("\n \n "); + break; + } +} + +} // namespace Server +} // namespace Envoy diff --git a/source/server/admin/admin_html_generator.h b/source/server/admin/admin_html_generator.h new file mode 100644 index 0000000000000..9d93d4b6874dc --- /dev/null +++ b/source/server/admin/admin_html_generator.h @@ -0,0 +1,36 @@ +#pragma once + +#include "envoy/common/optref.h" +#include "envoy/server/admin.h" + +#include "source/common/buffer/buffer_impl.h" + +#include "absl/strings/str_replace.h" + +namespace Envoy { +namespace Server { + +class AdminHtmlGenerator { +public: + AdminHtmlGenerator(Buffer::Instance& response) : response_(response) {} + + void renderHead(); + void renderTableBegin(); + void renderTableEnd(); + void renderUrlHandler(const Admin::UrlHandler& handler, + OptRef query); + void renderInput(absl::string_view id, absl::string_view path, Admin::ParamDescriptor::Type type, + OptRef query, + const std::vector& enum_choices); + void setSubmitOnChange(bool submit_on_change) { submit_on_change_ = submit_on_change; } + void setVisibleSubmit(bool visible_submit) { visible_submit_ = visible_submit; } + +private: + Buffer::Instance& response_; + int index_{0}; + bool submit_on_change_{false}; + bool visible_submit_{true}; +}; + +} // namespace Server +} // namespace Envoy diff --git a/source/server/admin/generate_admin_html.sh b/source/server/admin/generate_admin_html.sh new file mode 100755 index 0000000000000..e139dda1061ac --- /dev/null +++ b/source/server/admin/generate_admin_html.sh @@ -0,0 +1,15 @@ +#!/bin/sh + +echo '#include "absl/strings/str_replace.h"' +echo '' +echo 'constexpr absl::string_view AdminHtmlStart = R"EOF(' +echo '' +cat "$1" +echo '' +echo '' +echo '' +echo ')EOF";' diff --git a/source/server/admin/stats_handler.cc b/source/server/admin/stats_handler.cc index b3c1a66cad1c5..c6c424388cef1 100644 --- a/source/server/admin/stats_handler.cc +++ b/source/server/admin/stats_handler.cc @@ -9,12 +9,25 @@ #include "source/common/html/utility.h" #include "source/common/http/headers.h" #include "source/common/http/utility.h" +#include "source/server/admin/admin_html_generator.h" #include "source/server/admin/prometheus_stats.h" #include "source/server/admin/utils.h" +#include "absl/strings/numbers.h" + namespace Envoy { namespace Server { +namespace { + +constexpr absl::string_view AllLabel = "All"; +constexpr absl::string_view CountersLabel = "Counters"; +constexpr absl::string_view GaugesLabel = "Gauges"; +constexpr absl::string_view HistogramsLabel = "Histograms"; +constexpr absl::string_view TextReadoutsLabel = "TextReadouts"; + +} // namespace + const uint64_t RecentLookupsCapacity = 100; StatsHandler::StatsHandler(Server::Instance& server) : HandlerContextBase(server) {} @@ -70,84 +83,513 @@ Http::Code StatsHandler::handlerStatsRecentLookupsEnable(absl::string_view, return Http::Code::OK; } +bool StatsHandler::Params::shouldShowMetric(const Stats::Metric& metric) const { + if (used_only_ && !metric.used()) { + return false; + } + if (!scope_.empty() || filter_.has_value()) { + std::string name = metric.name(); + if (filter_.has_value() && !std::regex_search(name, filter_.value())) { + return false; + } + if (!scope_.empty()) { + if (!absl::StartsWith(name, scope_ + ".")) { + return false; + } + } + } + return true; +} + +Http::Code StatsHandler::Params::parse(absl::string_view url, Buffer::Instance& response) { + query_ = Http::Utility::parseAndDecodeQueryString(url); + used_only_ = query_.find("usedonly") != query_.end(); + pretty_ = query_.find("pretty") != query_.end(); + prometheus_text_readouts_ = query_.find("text_readouts") != query_.end(); + if (!Utility::filterParam(query_, response, filter_)) { + return Http::Code::BadRequest; + } + if (filter_.has_value()) { + filter_string_ = query_.find("filter")->second; + } + + auto parse_type = [](absl::string_view str, Type& type) { + if (str == GaugesLabel) { + type = Type::Gauges; + } else if (str == CountersLabel) { + type = Type::Counters; + } else if (str == HistogramsLabel) { + type = Type::Histograms; + } else if (str == TextReadoutsLabel) { + type = Type::TextReadouts; + } else if (str == AllLabel) { + type = Type::All; + } else { + return false; + } + return true; + }; + + auto type_iter = query_.find("type"); + if (type_iter != query_.end() && !parse_type(type_iter->second, type_)) { + response.add("invalid &type= param"); + return Http::Code::BadRequest; + } + + const absl::optional format_value = Utility::formatParam(query_); + if (format_value.has_value()) { + if (format_value.value() == "prometheus") { + format_ = Format::Prometheus; + } else if (format_value.value() == "json") { + format_ = Format::Json; + } else if (format_value.value() == "text") { + format_ = Format::Text; + } else if (format_value.value() == "html") { + format_ = Format::Html; + } else { + response.add("usage: /stats?format=json or /stats?format=prometheus \n\n"); + return Http::Code::BadRequest; + } + } + + auto scope_iter = query_.find("scope"); + if (scope_iter != query_.end()) { + scope_ = scope_iter->second; + } + return Http::Code::OK; +} + Http::Code StatsHandler::handlerStats(absl::string_view url, Http::ResponseHeaderMap& response_headers, - Buffer::Instance& response, AdminStream& admin_stream) { + Buffer::Instance& response, AdminStream&) { + Params params; + Http::Code code = params.parse(url, response); + if (code != Http::Code::OK) { + return code; + } if (server_.statsConfig().flushOnAdmin()) { server_.flushStats(); } + Stats::Store& store = server_.stats(); + if (params.format_ == Format::Prometheus) { + const std::vector& text_readouts_vec = + params.prometheus_text_readouts_ ? store.textReadouts() + : std::vector(); + PrometheusStatsFormatter::statsAsPrometheus( + store.counters(), store.gauges(), store.histograms(), text_readouts_vec, response, + params.used_only_, params.filter_, server_.api().customStatNamespaces()); + return Http::Code::OK; + } - const Http::Utility::QueryParams params = Http::Utility::parseAndDecodeQueryString(url); + return stats(params, server_.stats(), response_headers, response); +} - const bool used_only = params.find("usedonly") != params.end(); - absl::optional regex; - if (!Utility::filterParam(params, response, regex)) { - return Http::Code::BadRequest; +class StatsHandler::Render { +public: + virtual ~Render() = default; + virtual void generate(Stats::Counter&) PURE; + virtual void generate(Stats::Gauge&) PURE; + virtual void generate(Stats::TextReadout&) PURE; + virtual void generate(Stats::Histogram&) PURE; + virtual void addScope(const std::string&) {} + virtual void noStats(Type type) { UNREFERENCED_PARAMETER(type); } + virtual void render(Buffer::Instance& response) PURE; +}; + +class StatsHandler::TextRender : public StatsHandler::Render { +protected: + using Group = std::vector; + +public: + void generate(Stats::TextReadout& text_readout) override { + groups_[Type::TextReadouts].emplace_back(absl::StrCat( + text_readout.name(), ": \"", Html::Utility::sanitize(text_readout.value()), "\"\n")); + } + void generate(Stats::Counter& counter) override { + groups_[Type::Counters].emplace_back(absl::StrCat(counter.name(), ": ", counter.value(), "\n")); + } + void generate(Stats::Gauge& gauge) override { + groups_[Type::Gauges].emplace_back(absl::StrCat(gauge.name(), ": ", gauge.value(), "\n")); + } + void generate(Stats::Histogram& histogram) override { + Stats::ParentHistogram* phist = dynamic_cast(&histogram); + if (phist != nullptr) { + groups_[Type::Histograms].emplace_back( + absl::StrCat(phist->name(), ": ", phist->quantileSummary(), "\n")); + } } - const absl::optional format_value = Utility::formatParam(params); - if (format_value.has_value() && format_value.value() == "prometheus") { - return handlerPrometheusStats(url, response_headers, response, admin_stream); + void render(Buffer::Instance& response) override { + for (const auto& iter : groups_) { + const Group& group = iter.second; + for (const std::string& str : group) { + response.add(str); + } + } } - std::map all_stats; - for (const Stats::CounterSharedPtr& counter : server_.stats().counters()) { - if (shouldShowMetric(*counter, used_only, regex)) { - all_stats.emplace(counter->name(), counter->value()); +protected: + std::map groups_; // Ordered by Type's numeric value. +}; + +class StatsHandler::HtmlRender : public StatsHandler::TextRender { +public: + HtmlRender(Http::ResponseHeaderMap& response_headers, Buffer::Instance& response, + StatsHandler& stats_handler, const Params& params) + : response_(response), html_(response) { + response_headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Html); + html_.setVisibleSubmit(false); + html_.setSubmitOnChange(true); + html_.renderHead(); + html_.renderTableBegin(); + html_.renderUrlHandler(stats_handler.statsHandler(), params.query_); + html_.renderInput("scope", "stats", Admin::ParamDescriptor::Type::Hidden, params.query_, {}); + html_.renderTableEnd(); + } + + ~HtmlRender() override { response_.add("\n"); } + + void noStats(Type type) override { + groups_[type]; // Make an empty group for this type. + } + + void render(Buffer::Instance&) override { + for (const auto& iter : groups_) { + absl::string_view label = StatsHandler::typeToString(iter.first); + const Group& group = iter.second; + if (group.empty()) { + response_.add(absl::StrCat("
No ", label, " found
\n")); + } else { + response_.add(absl::StrCat("

", label, "

\n
\n"));
+        for (const std::string& str : group) {
+          response_.add(str);
+        }
+        response_.add("
\n"); + } } } - for (const Stats::GaugeSharedPtr& gauge : server_.stats().gauges()) { - if (shouldShowMetric(*gauge, used_only, regex)) { - ASSERT(gauge->importMode() != Stats::Gauge::ImportMode::Uninitialized); - all_stats.emplace(gauge->name(), gauge->value()); + static void renderAjaxRequestForScopes(Buffer::Instance& response, const Params& params) { + // Delegate to JavaScript to fetch all top-level scopes and render as an outline. + std::string url = + absl::StrCat("stats?format=json&show_json_scopes&type=", typeToString(params.type_)); + if (params.used_only_) { + url += "&usedonly"; + } + if (!params.filter_string_.empty()) { + absl::StrAppend(&url, "&filter=", params.filter_string_); } + response.add(absl::StrCat("
    \n" + "\n")); + } + +private: + Buffer::Instance& response_; + AdminHtmlGenerator html_; +}; + +class StatsHandler::JsonRender : public StatsHandler::Render { +public: + explicit JsonRender(const Params& params) : params_(params) {} + + void generate(Stats::Counter& counter) override { + add(counter, ValueUtil::numberValue(counter.value())); + } + void generate(Stats::Gauge& gauge) override { add(gauge, ValueUtil::numberValue(gauge.value())); } + void generate(Stats::TextReadout& text_readout) override { + add(text_readout, ValueUtil::stringValue(text_readout.value())); } + void generate(Stats::Histogram& histogram) override { + Stats::ParentHistogram* phist = dynamic_cast(&histogram); + if (phist != nullptr) { + if (!found_used_histogram_) { + auto* histograms_obj_fields = histograms_obj_.mutable_fields(); - std::map text_readouts; - for (const auto& text_readout : server_.stats().textReadouts()) { - if (shouldShowMetric(*text_readout, used_only, regex)) { - text_readouts.emplace(text_readout->name(), text_readout->value()); + // It is not possible for the supported quantiles to differ across histograms, so it is ok + // to send them once. + Stats::HistogramStatisticsImpl empty_statistics; + std::vector supported_quantile_array; + for (double quantile : empty_statistics.supportedQuantiles()) { + supported_quantile_array.push_back(ValueUtil::numberValue(quantile * 100)); + } + (*histograms_obj_fields)["supported_quantiles"] = + ValueUtil::listValue(supported_quantile_array); + found_used_histogram_ = true; + } + + ProtobufWkt::Struct computed_quantile; + auto* computed_quantile_fields = computed_quantile.mutable_fields(); + (*computed_quantile_fields)["name"] = ValueUtil::stringValue(histogram.name()); + + std::vector computed_quantile_value_array; + for (size_t i = 0; i < phist->intervalStatistics().supportedQuantiles().size(); ++i) { + ProtobufWkt::Struct computed_quantile_value; + auto* computed_quantile_value_fields = computed_quantile_value.mutable_fields(); + const auto& interval = phist->intervalStatistics().computedQuantiles()[i]; + const auto& cumulative = phist->cumulativeStatistics().computedQuantiles()[i]; + (*computed_quantile_value_fields)["interval"] = + std::isnan(interval) ? ValueUtil::nullValue() : ValueUtil::numberValue(interval); + (*computed_quantile_value_fields)["cumulative"] = + std::isnan(cumulative) ? ValueUtil::nullValue() : ValueUtil::numberValue(cumulative); + + computed_quantile_value_array.push_back(ValueUtil::structValue(computed_quantile_value)); + } + (*computed_quantile_fields)["values"] = ValueUtil::listValue(computed_quantile_value_array); + computed_quantile_array_.push_back(ValueUtil::structValue(computed_quantile)); } } - if (!format_value.has_value()) { - // Display plain stats if format query param is not there. - statsAsText(all_stats, text_readouts, server_.stats().histograms(), used_only, regex, response); - return Http::Code::OK; + void addScope(const std::string& prefix) override { + scope_array_.push_back(ValueUtil::stringValue(prefix)); } - if (format_value.value() == "json") { + void render(Buffer::Instance& response) override { + if (found_used_histogram_) { + auto* histograms_obj_fields = histograms_obj_.mutable_fields(); + (*histograms_obj_fields)["computed_quantiles"] = + ValueUtil::listValue(computed_quantile_array_); + auto* histograms_obj_container_fields = histograms_obj_container_.mutable_fields(); + (*histograms_obj_container_fields)["histograms"] = ValueUtil::structValue(histograms_obj_); + stats_array_.push_back(ValueUtil::structValue(histograms_obj_container_)); + } + + auto* document_fields = document_.mutable_fields(); + (*document_fields)["stats"] = ValueUtil::listValue(stats_array_); + if (params_.query_.find("show_json_scopes") != params_.query_.end()) { + (*document_fields)["scopes"] = ValueUtil::listValue(scope_array_); + } + response.add(MessageUtil::getJsonStringFromMessageOrDie(document_, params_.pretty_, true)); + } + +private: + template void add(StatType& stat, const Value& value) { + ProtobufWkt::Struct stat_obj; + auto* stat_obj_fields = stat_obj.mutable_fields(); + (*stat_obj_fields)["name"] = ValueUtil::stringValue(stat.name()); + (*stat_obj_fields)["value"] = value; + stats_array_.push_back(ValueUtil::structValue(stat_obj)); + } + + const StatsHandler::Params& params_; + ProtobufWkt::Struct document_; + std::vector stats_array_; + std::vector scope_array_; + ProtobufWkt::Struct histograms_obj_; + ProtobufWkt::Struct histograms_obj_container_; + std::vector computed_quantile_array_; + bool found_used_histogram_{false}; +}; + +class StatsHandler::Context { +public: + // We need to hold ref-counts to each stat in our intermediate sets to avoid + // having the stats be deleted while we are computing results. + template struct Hash { + size_t operator()(const Stats::RefcountPtr& a) const { return a->statName().hash(); } + }; + + template struct Compare { + bool operator()(const Stats::RefcountPtr& a, + const Stats::RefcountPtr& b) const { + return a->statName() == b->statName(); + } + }; + + template + using SharedStatSet = + absl::flat_hash_set, Hash, Compare>; + + Context(const Params& params, Render& render, Buffer::Instance& response) + : params_(params), render_(render), response_(response) {} + + void collectScope(const Stats::Scope& scope) { + collect(Type::TextReadouts, scope, text_readouts_); + collect(Type::Counters, scope, counters_); + collect(Type::Gauges, scope, gauges_); + collect(Type::Histograms, scope, histograms_); + } + + void emit() { + emit(Type::TextReadouts, text_readouts_); + emit(Type::Counters, counters_); + emit(Type::Gauges, gauges_); + emit(Type::Histograms, histograms_); + emitScopes(); + } + + template + void collect(Type type, const Stats::Scope& scope, SharedStatSet& set) { + // Bail early if the requested type does not match the current type. + if (params_.type_ != Type::All && params_.type_ != type) { + return; + } + + Stats::IterateFn fn = [this, &set](const Stats::RefcountPtr& stat) -> bool { + if (params_.shouldShowMetric(*stat)) { + set.insert(stat); + } + return true; + }; + scope.iterate(fn); + } + + template void emit(Type type, SharedStatSet& set) { + // Bail early if the requested type does not match the current type. + if (params_.type_ != Type::All && params_.type_ != type) { + return; + } + + if (set.empty()) { + render_.noStats(type); + } + + std::vector> sorted; + sorted.reserve(set.size()); + for (const Stats::RefcountPtr& stat : set) { + sorted.emplace_back(stat); + } + + struct Cmp { + bool operator()(const Stats::RefcountPtr& a, + const Stats::RefcountPtr& b) const { + return a->constSymbolTable().lessThan(a->statName(), b->statName()); + } + }; + std::sort(sorted.begin(), sorted.end(), Cmp()); + + for (const Stats::RefcountPtr& stat : sorted) { + render_.generate(*stat); + } + } + + void emitScopes() { + // Prune the scopes-list so that if both "a.b.c" and "a.b" are present, we drop "a.b.c". + for (const std::string& scope : scopes_) { + size_t last_dot = scope.rfind('.'); + if (last_dot != 0 && last_dot != std::string::npos && + scopes_.find(scope.substr(0, last_dot)) != scopes_.end()) { + continue; + } + render_.addScope(scope); + } + } + + void addScope(const std::string& scope) { + if (scope != params_.scope_) { + scopes_.insert(scope); + } + } + + Render& render() { return render_; } + + const Params& params_; + Render& render_; + Buffer::Instance& response_; + + SharedStatSet counters_; + SharedStatSet gauges_; + SharedStatSet text_readouts_; + SharedStatSet histograms_; + std::set scopes_; +}; + +Http::Code StatsHandler::stats(const Params& params, Stats::Store& stats, + Http::ResponseHeaderMap& response_headers, + Buffer::Instance& response) { + std::unique_ptr render; + + switch (params.format_) { + case Format::Html: + render = std::make_unique(response_headers, response, *this, params); + break; + case Format::Json: + render = std::make_unique(params); response_headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Json); - response.add( - statsAsJson(all_stats, text_readouts, server_.stats().histograms(), used_only, regex)); - return Http::Code::OK; + break; + case Format::Prometheus: + ASSERT(false); + ABSL_FALLTHROUGH_INTENDED; + case Format::Text: + render = std::make_unique(); + break; + } + + // The default HTML view, with no scope query-param specified (or an empty string), we will + // just generate some HTML to initiate a JSON request to populate the scopes. This way the + // rendering for scopes and sub-scopes can be handled consistently in JavaScript. + if (params.scope_.empty() && params.format_ == Format::Html) { + StatsHandler::HtmlRender::renderAjaxRequestForScopes(response, params); + } else { + Context context(params, *render, response); + + // Note that multiple scopes can exist with the same name, and also that + // scopes may be created/destroyed in other threads, whenever we are not + // holding the store's lock. So when we traverse the scopes, we'll both + // look for Scope objects matching our expected name, and we'll create + // a sorted list of sort names for use in populating next/previous buttons. + auto scope_fn = [this, ¶ms, &context](const Stats::Scope& scope) { + std::string prefix_str = server_.stats().symbolTable().toString(scope.prefix()); + + if (params.query_.find("show_json_scopes") != params.query_.end()) { + if (params.scope_ == prefix_str) { + context.collectScope(scope); + } else if (params.scope_.empty() || + absl::StartsWith(prefix_str + ".", params.scope_ + ".")) { + // Truncate any hierarchy after the prefix. + size_t dot_search = params.scope_.empty() ? 0 : params.scope_.size() + 1; + size_t dot = prefix_str.find('.', dot_search); + if (dot != std::string::npos) { + prefix_str.resize(dot); + } + context.addScope(prefix_str); + } + } else if (prefix_str == params.scope_ || prefix_str.empty() || params.scope_.empty()) { + // If the scope matches the prefix of what the user wants, append in the + // stats from it. Note that scopes with a prefix of "" will match anything + // the user types, in which case we'll still be filtering based on stat + // name prefix. + context.collectScope(scope); + } else if (absl::StartsWith(prefix_str, params.scope_) && + prefix_str[params.scope_.size()] == '.') { + context.addScope(prefix_str); + } + }; + stats.forEachScope([](size_t) {}, scope_fn); + context.emit(); + render->render(response); } - response.add("usage: /stats?format=json or /stats?format=prometheus \n"); - response.add("\n"); - return Http::Code::NotFound; + if (params.format_ == Format::Html) { + response.add("\n"); + } + + // Display plain stats if format query param is not there. + // statsAsText(counters_and_gauges, text_readouts, histograms, response); + return Http::Code::OK; } -Http::Code StatsHandler::handlerPrometheusStats(absl::string_view path_and_query, +Http::Code StatsHandler::handlerStatsPrometheus(absl::string_view path_and_query, Http::ResponseHeaderMap&, Buffer::Instance& response, AdminStream&) { - const Http::Utility::QueryParams params = - Http::Utility::parseAndDecodeQueryString(path_and_query); - const bool used_only = params.find("usedonly") != params.end(); - const bool text_readouts = params.find("text_readouts") != params.end(); - - const std::vector& text_readouts_vec = - text_readouts ? server_.stats().textReadouts() : std::vector(); - - absl::optional regex; - if (!Utility::filterParam(params, response, regex)) { - return Http::Code::BadRequest; + Params params; + Http::Code code = params.parse(path_and_query, response); + if (code != Http::Code::OK) { + return code; } - - PrometheusStatsFormatter::statsAsPrometheus( - server_.stats().counters(), server_.stats().gauges(), server_.stats().histograms(), - text_readouts_vec, response, used_only, regex, server_.api().customStatNamespaces()); + if (server_.statsConfig().flushOnAdmin()) { + server_.flushStats(); + } + Stats::Store& stats = server_.stats(); + const std::vector& text_readouts_vec = + params.prometheus_text_readouts_ ? stats.textReadouts() + : std::vector(); + PrometheusStatsFormatter::statsAsPrometheus(stats.counters(), stats.gauges(), stats.histograms(), + text_readouts_vec, response, params.used_only_, + params.filter_, server_.api().customStatNamespaces()); return Http::Code::OK; } @@ -171,37 +613,10 @@ Http::Code StatsHandler::handlerContention(absl::string_view, return Http::Code::OK; } -void StatsHandler::statsAsText(const std::map& all_stats, - const std::map& text_readouts, - const std::vector& histograms, - bool used_only, const absl::optional& regex, - Buffer::Instance& response) { - // Display plain stats if format query param is not there. - for (const auto& text_readout : text_readouts) { - response.add(fmt::format("{}: \"{}\"\n", text_readout.first, - Html::Utility::sanitize(text_readout.second))); - } - for (const auto& stat : all_stats) { - response.add(fmt::format("{}: {}\n", stat.first, stat.second)); - } - std::map all_histograms; - for (const Stats::ParentHistogramSharedPtr& histogram : histograms) { - if (shouldShowMetric(*histogram, used_only, regex)) { - auto insert = all_histograms.emplace(histogram->name(), histogram->quantileSummary()); - ASSERT(insert.second); // No duplicates expected. - } - } - for (const auto& histogram : all_histograms) { - response.add(fmt::format("{}: {}\n", histogram.first, histogram.second)); - } -} - -std::string -StatsHandler::statsAsJson(const std::map& all_stats, - const std::map& text_readouts, - const std::vector& all_histograms, - const bool used_only, const absl::optional& regex, - const bool pretty_print) { +std::string StatsHandler::statsAsJson(const std::map& counters_and_gauges, + const std::map& text_readouts, + const std::vector& all_histograms, + const bool pretty_print) { ProtobufWkt::Struct document; std::vector stats_array; @@ -212,7 +627,7 @@ StatsHandler::statsAsJson(const std::map& all_stats, (*stat_obj_fields)["value"] = ValueUtil::stringValue(text_readout.second); stats_array.push_back(ValueUtil::structValue(stat_obj)); } - for (const auto& stat : all_stats) { + for (const auto& stat : counters_and_gauges) { ProtobufWkt::Struct stat_obj; auto* stat_obj_fields = stat_obj.mutable_fields(); (*stat_obj_fields)["name"] = ValueUtil::stringValue(stat.first); @@ -228,8 +643,9 @@ StatsHandler::statsAsJson(const std::map& all_stats, std::vector computed_quantile_array; bool found_used_histogram = false; - for (const Stats::ParentHistogramSharedPtr& histogram : all_histograms) { - if (shouldShowMetric(*histogram, used_only, regex)) { + for (const Stats::HistogramSharedPtr& histogram : all_histograms) { + Stats::ParentHistogram* phist = dynamic_cast(histogram.get()); + if (phist != nullptr) { if (!found_used_histogram) { // It is not possible for the supported quantiles to differ across histograms, so it is ok // to send them once. @@ -248,11 +664,11 @@ StatsHandler::statsAsJson(const std::map& all_stats, (*computed_quantile_fields)["name"] = ValueUtil::stringValue(histogram->name()); std::vector computed_quantile_value_array; - for (size_t i = 0; i < histogram->intervalStatistics().supportedQuantiles().size(); ++i) { + for (size_t i = 0; i < phist->intervalStatistics().supportedQuantiles().size(); ++i) { ProtobufWkt::Struct computed_quantile_value; auto* computed_quantile_value_fields = computed_quantile_value.mutable_fields(); - const auto& interval = histogram->intervalStatistics().computedQuantiles()[i]; - const auto& cumulative = histogram->cumulativeStatistics().computedQuantiles()[i]; + const auto& interval = phist->intervalStatistics().computedQuantiles()[i]; + const auto& cumulative = phist->cumulativeStatistics().computedQuantiles()[i]; (*computed_quantile_value_fields)["interval"] = std::isnan(interval) ? ValueUtil::nullValue() : ValueUtil::numberValue(interval); (*computed_quantile_value_fields)["cumulative"] = @@ -273,9 +689,56 @@ StatsHandler::statsAsJson(const std::map& all_stats, auto* document_fields = document.mutable_fields(); (*document_fields)["stats"] = ValueUtil::listValue(stats_array); - return MessageUtil::getJsonStringFromMessageOrDie(document, pretty_print, true); } +Admin::UrlHandler StatsHandler::statsHandler() { + return {"/stats", + "Print server stats.", + MAKE_ADMIN_HANDLER(handlerStats), + false, + false, + {{Admin::ParamDescriptor::Type::Boolean, "usedonly", + "Only include stats that have been written by system since restart"}, + {Admin::ParamDescriptor::Type::String, "filter", + "Regular expression (ecmascript) for filtering stats"}, + {Admin::ParamDescriptor::Type::Enum, + "format", + "File format to use.", + {"html", "text", "json", "prometheus"}}, + {Admin::ParamDescriptor::Type::Enum, + "type", + "Stat types to include.", + {AllLabel, CountersLabel, HistogramsLabel, GaugesLabel, TextReadoutsLabel}}}}; +} + +absl::string_view StatsHandler::typeToString(StatsHandler::Type type) { + static constexpr absl::string_view TextReadouts = "TextReadouts"; + static constexpr absl::string_view Counters = "Counters"; + static constexpr absl::string_view Gauges = "Gauges"; + static constexpr absl::string_view Histograms = "Histograms"; + static constexpr absl::string_view All = "All"; + + absl::string_view ret; + switch (type) { + case Type::TextReadouts: + ret = TextReadouts; + break; + case Type::Counters: + ret = Counters; + break; + case Type::Gauges: + ret = Gauges; + break; + case Type::Histograms: + ret = Histograms; + break; + case Type::All: + ret = All; + break; + } + return ret; +} + } // namespace Server } // namespace Envoy diff --git a/source/server/admin/stats_handler.h b/source/server/admin/stats_handler.h index 796bd35bc8ff7..c17a6ed727260 100644 --- a/source/server/admin/stats_handler.h +++ b/source/server/admin/stats_handler.h @@ -40,34 +40,70 @@ class StatsHandler : public HandlerContextBase { Http::Code handlerStats(absl::string_view path_and_query, Http::ResponseHeaderMap& response_headers, Buffer::Instance& response, AdminStream&); - Http::Code handlerPrometheusStats(absl::string_view path_and_query, + Http::Code handlerStatsPrometheus(absl::string_view path_and_query, Http::ResponseHeaderMap& response_headers, Buffer::Instance& response, AdminStream&); Http::Code handlerContention(absl::string_view path_and_query, Http::ResponseHeaderMap& response_headers, Buffer::Instance& response, AdminStream&); + Admin::UrlHandler statsHandler(); + private: - template - static bool shouldShowMetric(const StatType& metric, const bool used_only, - const absl::optional& regex) { - return ((!used_only || metric.used()) && - (!regex.has_value() || std::regex_search(metric.name(), regex.value()))); - } + class Context; + class HtmlRender; + class JsonRender; + class Render; + class TextRender; + + enum class Format { + Html, + Json, + Prometheus, + Text, + }; + + // The order is used to linearize the ordering of stats of all types. + enum class Type { + TextReadouts, + Counters, + Gauges, + Histograms, + All, + }; + + struct Params { + Http::Code parse(absl::string_view url, Buffer::Instance& response); + bool shouldShowMetric(const Stats::Metric& metric) const; + + bool used_only_{false}; + bool prometheus_text_readouts_{false}; + bool pretty_{false}; + Format format_{ + Format::Text}; // If no `format=` param we use Text, but the `UI` defaults to HTML. + Type type_{Type::All}; + Type start_type_{Type::TextReadouts}; + std::string filter_string_; + absl::optional filter_; + std::string scope_; + Http::Utility::QueryParams query_; + }; friend class StatsHandlerTest; + Http::Code stats(const Params& parmams, Stats::Store& store, + Http::ResponseHeaderMap& response_headers, Buffer::Instance& response); + + static Http::Code prometheusStats(absl::string_view path_and_query, Buffer::Instance& response, + Stats::Store& stats, + Stats::CustomStatNamespaces& custom_namespaces); + static std::string statsAsJson(const std::map& all_stats, const std::map& text_readouts, - const std::vector& all_histograms, - bool used_only, const absl::optional& regex, - bool pretty_print = false); - - void statsAsText(const std::map& all_stats, - const std::map& text_readouts, - const std::vector& all_histograms, - bool used_only, const absl::optional& regex, - Buffer::Instance& response); + const std::vector& all_histograms, + bool pretty_print); + + static absl::string_view typeToString(Type type); }; } // namespace Server diff --git a/source/server/config_validation/admin.cc b/source/server/config_validation/admin.cc index 565ceb1b11879..19233dd7fec80 100644 --- a/source/server/config_validation/admin.cc +++ b/source/server/config_validation/admin.cc @@ -4,7 +4,8 @@ namespace Envoy { namespace Server { // Pretend that handler was added successfully. -bool ValidationAdmin::addHandler(const std::string&, const std::string&, HandlerCb, bool, bool) { +bool ValidationAdmin::addHandler(const std::string&, const std::string&, HandlerCb, bool, bool, + const ParamDescriptorVec&) { return true; } diff --git a/source/server/config_validation/admin.h b/source/server/config_validation/admin.h index 45fc2a224f796..e992414dfc428 100644 --- a/source/server/config_validation/admin.h +++ b/source/server/config_validation/admin.h @@ -23,7 +23,8 @@ class ValidationAdmin : public Admin { : socket_(address ? std::make_shared(nullptr, std::move(address), nullptr) : nullptr) {} - bool addHandler(const std::string&, const std::string&, HandlerCb, bool, bool) override; + bool addHandler(const std::string&, const std::string&, HandlerCb, bool, bool, + const ParamDescriptorVec&) override; bool removeHandler(const std::string&) override; const Network::Socket& socket() override; ConfigTracker& getConfigTracker() override; diff --git a/test/extensions/common/tap/admin_test.cc b/test/extensions/common/tap/admin_test.cc index 58cfe3da4d941..77066947add97 100644 --- a/test/extensions/common/tap/admin_test.cc +++ b/test/extensions/common/tap/admin_test.cc @@ -31,7 +31,7 @@ class AdminHandlerTest : public testing::Test { public: void setup(Network::Address::Type socket_type = Network::Address::Type::Ip) { ON_CALL(admin_.socket_, addressType()).WillByDefault(Return(socket_type)); - EXPECT_CALL(admin_, addHandler("/tap", "tap filter control", _, true, true)) + EXPECT_CALL(admin_, addHandler("/tap", "tap filter control", _, true, true, _)) .WillOnce(DoAll(SaveArg<2>(&cb_), Return(true))); EXPECT_CALL(admin_, socket()); handler_ = std::make_unique(admin_, main_thread_dispatcher_); diff --git a/test/extensions/filters/http/common/fuzz/uber_per_filter.cc b/test/extensions/filters/http/common/fuzz/uber_per_filter.cc index 4e3527b8cb465..b75a3b1c92719 100644 --- a/test/extensions/filters/http/common/fuzz/uber_per_filter.cc +++ b/test/extensions/filters/http/common/fuzz/uber_per_filter.cc @@ -130,7 +130,8 @@ void UberFilterFuzzer::perFilterSetup() { // Prepare expectations for TAP config. ON_CALL(factory_context_, admin()).WillByDefault(testing::ReturnRef(factory_context_.admin_)); - ON_CALL(factory_context_.admin_, addHandler(_, _, _, _, _)).WillByDefault(testing::Return(true)); + ON_CALL(factory_context_.admin_, addHandler(_, _, _, _, _, _)) + .WillByDefault(testing::Return(true)); ON_CALL(factory_context_.admin_, removeHandler(_)).WillByDefault(testing::Return(true)); // Prepare expectations for WASM filter. diff --git a/test/integration/integration_admin_test.cc b/test/integration/integration_admin_test.cc index b9ec98612876f..8c69f2e8e0579 100644 --- a/test/integration/integration_admin_test.cc +++ b/test/integration/integration_admin_test.cc @@ -139,7 +139,7 @@ TEST_P(IntegrationAdminTest, Admin) { EXPECT_EQ("application/json", ContentType(response)); validateStatsJson(response->body(), 0); - EXPECT_EQ("404", request("admin", "GET", "/stats?format=blah", response)); + EXPECT_EQ("400", request("admin", "GET", "/stats?format=blah", response)); EXPECT_EQ("text/plain; charset=UTF-8", ContentType(response)); EXPECT_EQ("200", request("admin", "GET", "/stats?format=json", response)); @@ -427,7 +427,7 @@ TEST_P(IntegrationAdminTest, AdminOnDestroyCallbacks) { }; EXPECT_TRUE( - test_server_->server().admin().addHandler("/foo/bar", "hello", callback, true, false)); + test_server_->server().admin().addHandler("/foo/bar", "hello", callback, true, false, {})); // As part of the request, on destroy() should be called and the on_destroy_callback invoked. BufferingStreamDecoderPtr response; diff --git a/test/integration/server.h b/test/integration/server.h index 48e4c5eb08ae4..d076e183bf2b1 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -164,6 +164,8 @@ class TestScopeWrapper : public Scope { return wrapped_scope_->iterate(fn); } + StatName prefix() const override { return wrapped_scope_->prefix(); } + private: Thread::MutexBasicLockable& lock_; ScopePtr wrapped_scope_; @@ -282,6 +284,7 @@ class TestIsolatedStoreImpl : public StoreRoot { Thread::LockGuard lock(lock_); return store_.counterFromStatNameWithTags(name, tags); } + void forEachCounter(Stats::SizeFn f_size, StatFn f_stat) const override { Thread::LockGuard lock(lock_); store_.forEachCounter(f_size, f_stat); @@ -294,6 +297,11 @@ class TestIsolatedStoreImpl : public StoreRoot { Thread::LockGuard lock(lock_); store_.forEachTextReadout(f_size, f_stat); } + void forEachScope(std::function f_size, + StatFn f_scope) const override { + Thread::LockGuard lock(lock_); + store_.forEachScope(f_size, f_scope); + } void forEachSinkedCounter(Stats::SizeFn f_size, StatFn f_stat) const override { Thread::LockGuard lock(lock_); store_.forEachSinkedCounter(f_size, f_stat); @@ -309,7 +317,6 @@ class TestIsolatedStoreImpl : public StoreRoot { void setSinkPredicates(std::unique_ptr&& sink_predicates) override { UNREFERENCED_PARAMETER(sink_predicates); } - Counter& counterFromString(const std::string& name) override { Thread::LockGuard lock(lock_); return store_.counterFromString(name); @@ -393,6 +400,8 @@ class TestIsolatedStoreImpl : public StoreRoot { bool iterate(const IterateFn& fn) const override { return store_.iterate(fn); } bool iterate(const IterateFn& fn) const override { return store_.iterate(fn); } + StatName prefix() const override { return store_.prefix(); } + // Stats::StoreRoot void addSink(Sink&) override {} void setTagProducer(TagProducerPtr&&) override {} diff --git a/test/mocks/server/admin.cc b/test/mocks/server/admin.cc index db35d25034e4f..4a41b1887f7d8 100644 --- a/test/mocks/server/admin.cc +++ b/test/mocks/server/admin.cc @@ -14,7 +14,7 @@ MockAdmin::MockAdmin() { ON_CALL(*this, getConfigTracker()).WillByDefault(ReturnRef(config_tracker_)); ON_CALL(*this, concurrency()).WillByDefault(Return(1)); ON_CALL(*this, socket()).WillByDefault(ReturnRef(socket_)); - ON_CALL(*this, addHandler(_, _, _, _, _)).WillByDefault(Return(true)); + ON_CALL(*this, addHandler(_, _, _, _, _, _)).WillByDefault(Return(true)); ON_CALL(*this, removeHandler(_)).WillByDefault(Return(true)); } diff --git a/test/mocks/server/admin.h b/test/mocks/server/admin.h index 6d9b150edc4d1..e8c7384f02019 100644 --- a/test/mocks/server/admin.h +++ b/test/mocks/server/admin.h @@ -23,7 +23,7 @@ class MockAdmin : public Admin { // Server::Admin MOCK_METHOD(bool, addHandler, (const std::string& prefix, const std::string& help_text, HandlerCb callback, - bool removable, bool mutates_server_state)); + bool removable, bool mutates_server_state, const ParamDescriptorVec& params)); MOCK_METHOD(bool, removeHandler, (const std::string& prefix)); MOCK_METHOD(Network::Socket&, socket, ()); MOCK_METHOD(ConfigTracker&, getConfigTracker, ()); diff --git a/test/server/admin/admin_test.cc b/test/server/admin/admin_test.cc index 99b93afda1d4e..740653474e390 100644 --- a/test/server/admin/admin_test.cc +++ b/test/server/admin/admin_test.cc @@ -101,7 +101,7 @@ TEST_P(AdminInstanceTest, CustomHandler) { AdminStream&) -> Http::Code { return Http::Code::Accepted; }; // Test removable handler. - EXPECT_NO_LOGS(EXPECT_TRUE(admin_.addHandler("/foo/bar", "hello", callback, true, false))); + EXPECT_NO_LOGS(EXPECT_TRUE(admin_.addHandler("/foo/bar", "hello", callback, true, false, {}))); Http::TestResponseHeaderMapImpl header_map; Buffer::OwnedImpl response; EXPECT_EQ(Http::Code::Accepted, getCallback("/foo/bar", header_map, response)); @@ -112,11 +112,11 @@ TEST_P(AdminInstanceTest, CustomHandler) { EXPECT_FALSE(admin_.removeHandler("/foo/bar")); // Add non removable handler. - EXPECT_TRUE(admin_.addHandler("/foo/bar", "hello", callback, false, false)); + EXPECT_TRUE(admin_.addHandler("/foo/bar", "hello", callback, false, false, {})); EXPECT_EQ(Http::Code::Accepted, getCallback("/foo/bar", header_map, response)); // Add again and make sure it is not there twice. - EXPECT_FALSE(admin_.addHandler("/foo/bar", "hello", callback, false, false)); + EXPECT_FALSE(admin_.addHandler("/foo/bar", "hello", callback, false, false, {})); // Try to remove non removable handler, and make sure it is not removed. EXPECT_FALSE(admin_.removeHandler("/foo/bar")); @@ -129,7 +129,7 @@ TEST_P(AdminInstanceTest, RejectHandlerWithXss) { EXPECT_LOG_CONTAINS("error", "filter \"/foo\" contains invalid character '<'", EXPECT_FALSE(admin_.addHandler("/foo", "hello", - callback, true, false))); + callback, true, false, {}))); } TEST_P(AdminInstanceTest, RejectHandlerWithEmbeddedQuery) { @@ -138,7 +138,7 @@ TEST_P(AdminInstanceTest, RejectHandlerWithEmbeddedQuery) { EXPECT_LOG_CONTAINS("error", "filter \"/bar?queryShouldNotBeInPrefix\" contains invalid character '?'", EXPECT_FALSE(admin_.addHandler("/bar?queryShouldNotBeInPrefix", "hello", - callback, true, false))); + callback, true, false, {}))); } TEST_P(AdminInstanceTest, EscapeHelpTextWithPunctuation) { @@ -148,7 +148,7 @@ TEST_P(AdminInstanceTest, EscapeHelpTextWithPunctuation) { // It's OK to have help text with HTML characters in it, but when we render the home // page they need to be escaped. const std::string planets = "jupiter>saturn>mars"; - EXPECT_TRUE(admin_.addHandler("/planets", planets, callback, true, false)); + EXPECT_TRUE(admin_.addHandler("/planets", planets, callback, true, false, {})); Http::TestResponseHeaderMapImpl header_map; Buffer::OwnedImpl response; @@ -160,14 +160,15 @@ TEST_P(AdminInstanceTest, EscapeHelpTextWithPunctuation) { EXPECT_NE(-1, response.search(escaped_planets.data(), escaped_planets.size(), 0, 0)); } -TEST_P(AdminInstanceTest, HelpUsesFormForMutations) { +TEST_P(AdminInstanceTest, HelpUsesPostForMutations) { Http::TestResponseHeaderMapImpl header_map; Buffer::OwnedImpl response; EXPECT_EQ(Http::Code::OK, getCallback("/", header_map, response)); - const std::string logging_action = "
    addSink(sink_); } - static std::string - statsAsJsonHandler(std::map& all_stats, - std::map& all_text_readouts, - const std::vector& all_histograms, - const bool used_only, const absl::optional regex = absl::nullopt) { - return StatsHandler::statsAsJson(all_stats, all_text_readouts, all_histograms, used_only, regex, - true /*pretty_print*/); + std::shared_ptr setupMockedInstance() { + auto instance = std::make_shared(); + EXPECT_CALL(stats_config_, flushOnAdmin()).WillRepeatedly(testing::Return(false)); + store_->initializeThreading(main_thread_dispatcher_, tls_); + EXPECT_CALL(*instance, stats()).WillRepeatedly(testing::ReturnRef(*store_)); + EXPECT_CALL(*instance, statsConfig()).WillRepeatedly(testing::ReturnRef(stats_config_)); + EXPECT_CALL(api_, customStatNamespaces()) + .WillRepeatedly(testing::ReturnRef(customNamespaces())); + EXPECT_CALL(*instance, api()).WillRepeatedly(testing::ReturnRef(api_)); + return instance; + } + + Http::Code handlerStats(absl::string_view url, Buffer::Instance& response) { + Http::TestResponseHeaderMapImpl response_headers; + StatsHandler::Params params; + Http::Code code = params.parse(url, response); + if (code != Http::Code::OK) { + return code; + } + std::shared_ptr instance = setupMockedInstance(); + StatsHandler handler(*instance); + return handler.stats(params, *store_, response_headers, response); + } + + Http::Code statsAsJsonHandler(std::string& data, absl::string_view params = "") { + std::string url = absl::StrCat("/stats?format=json&pretty", params); + Buffer::OwnedImpl response; + Http::Code code = handlerStats(url, response); + data = response.toString(); + return code; } Stats::StatName makeStat(absl::string_view name) { return pool_.add(name); } @@ -45,10 +68,11 @@ class StatsHandlerTest { tls_.shutdownThread(); } + Api::MockApi api_; + Configuration::MockStatsConfig stats_config_; Stats::SymbolTableImpl symbol_table_; NiceMock main_thread_dispatcher_; NiceMock tls_; - NiceMock api_; Stats::AllocatorImpl alloc_; Stats::MockSink sink_; Stats::ThreadLocalStoreImplPtr store_; @@ -64,33 +88,17 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, AdminStatsTest, TestUtility::ipTestParamsToString); TEST_P(AdminStatsTest, HandlerStatsInvalidFormat) { - const std::string url = "/stats?format=blergh"; - Http::TestResponseHeaderMapImpl response_headers; Buffer::OwnedImpl data; - MockAdminStream admin_stream; - Configuration::MockStatsConfig stats_config; - EXPECT_CALL(stats_config, flushOnAdmin()).WillRepeatedly(testing::Return(false)); - MockInstance instance; - EXPECT_CALL(instance, stats()).WillRepeatedly(testing::ReturnRef(*store_)); - EXPECT_CALL(instance, statsConfig()).WillRepeatedly(testing::ReturnRef(stats_config)); - StatsHandler handler(instance); - Http::Code code = handler.handlerStats(url, response_headers, data, admin_stream); - EXPECT_EQ(Http::Code::NotFound, code); + Http::Code code = handlerStats("/stats?format=blergh", data); + EXPECT_EQ(Http::Code::BadRequest, code); EXPECT_EQ("usage: /stats?format=json or /stats?format=prometheus \n\n", data.toString()); } TEST_P(AdminStatsTest, HandlerStatsPlainText) { + store_->initializeThreading(main_thread_dispatcher_, tls_); + const std::string url = "/stats"; - Http::TestResponseHeaderMapImpl response_headers, used_response_headers; Buffer::OwnedImpl data, used_data; - MockAdminStream admin_stream; - Configuration::MockStatsConfig stats_config; - EXPECT_CALL(stats_config, flushOnAdmin()).WillRepeatedly(testing::Return(false)); - MockInstance instance; - store_->initializeThreading(main_thread_dispatcher_, tls_); - EXPECT_CALL(instance, stats()).WillRepeatedly(testing::ReturnRef(*store_)); - EXPECT_CALL(instance, statsConfig()).WillRepeatedly(testing::ReturnRef(stats_config)); - StatsHandler handler(instance); Stats::Counter& c1 = store_->counterFromString("c1"); Stats::Counter& c2 = store_->counterFromString("c2"); @@ -112,7 +120,7 @@ TEST_P(AdminStatsTest, HandlerStatsPlainText) { store_->mergeHistograms([]() -> void {}); - Http::Code code = handler.handlerStats(url, response_headers, data, admin_stream); + Http::Code code = handlerStats(url, data); EXPECT_EQ(Http::Code::OK, code); constexpr char expected[] = "t: \"hello world\"\n" @@ -126,25 +134,154 @@ TEST_P(AdminStatsTest, HandlerStatsPlainText) { "P99.9(109.99,109.99) P100(110.0,110.0)\n"; EXPECT_EQ(expected, data.toString()); - code = handler.handlerStats(url + "?usedonly", used_response_headers, used_data, admin_stream); + code = handlerStats(url + "?usedonly", used_data); EXPECT_EQ(Http::Code::OK, code); EXPECT_EQ(expected, used_data.toString()); shutdownThreading(); } +#if 0 +TEST_P(AdminStatsTest, HandlerStatsPage) { + InSequence s; + store_->initializeThreading(main_thread_dispatcher_, tls_); + + // no text readouts for now. + for (uint32_t i = 0; i < 10; ++i) { + store_->counterFromString(absl::StrCat("c", i)).add(10 * i); + } + store_->gaugeFromString("g1", Stats::Gauge::ImportMode::Accumulate).set(100); + store_->gaugeFromString("g2", Stats::Gauge::ImportMode::Accumulate).set(200); + store_->histogramFromString("h1", Stats::Histogram::Unit::Unspecified); + + auto test_page = [this](absl::string_view direction, absl::string_view start, + absl::string_view expected_items, absl::string_view prev, + absl::string_view next) { + Buffer::OwnedImpl data; + std::string url = "/stats?format=html&pagesize=4&type=All&"; + if (direction == "prev") { + absl::StrAppend(&url, "before=", start); + } else { + absl::StrAppend(&url, "after=", start); + } + + Http::Code code = handlerStats(url, data); + EXPECT_EQ(Http::Code::OK, code); + + // Find all the line between "
    " and "
    ", and collect + // the first token of each line, up to the ":". + bool in_pre = false; + std::string out = data.toString(); + std::vector item_vector; + for (absl::string_view line : absl::StrSplit(out, '\n')) { + if (line == "
    ") {
+        in_pre = true;
+      } else if (line == "
    ") { + in_pre = false; + } else if (in_pre) { + absl::string_view::size_type colon = line.find(':'); + if (colon != absl::string_view::npos) { + item_vector.push_back(line.substr(0, colon)); + } + } + } + std::string actual_items = absl::StrJoin(item_vector, ","); + EXPECT_EQ(expected_items, actual_items) << "url=" << url; + + auto nav = [](absl::string_view direction, absl::string_view start = "") -> std::string { + std::string out = absl::StrCat("javascript:", direction); + if (!start.empty()) { + absl::StrAppend(&out, "(\"", start, "\")"); + } + return out; + }; + + if (prev.empty()) { + EXPECT_THAT(out, Not(HasSubstr(nav("prev")))) << "url=" << url; + } else { + EXPECT_THAT(out, HasSubstr(nav("prev", prev))) << "url=" << url; + } + if (next.empty()) { + EXPECT_THAT(out, Not(HasSubstr(nav("next")))) << "url=" << url; + } else { + EXPECT_THAT(out, HasSubstr(nav("next", next))) << "url=" << url; + } + }; + + // Forward walk to end. + test_page("next", "", "c0,c1,c2,c3", "", "Counters:c3"); + test_page("next", "Counters:c3", "c4,c5,c6,c7", "Counters:c4", "Counters:c7"); + test_page("next", "Counters:c7", "c8,c9,g1,g2", "Counters:c8", "Histograms:"); + test_page("next", "Gauges:g2", "h1", "Histograms:h1", ""); + + // Reverse walk to beginning. + test_page("prev", "Histograms:h1", "c9,g1,g2,h1", "Counters:c9", "Histograms:h1"); + test_page("prev", "Counters:c9", "c5,c6,c7,c8", "Counters:c5", "Counters:c8"); + test_page("prev", "Counters:c5", "c1,c2,c3,c4", "Counters:c1", "Counters:c4"); + test_page("prev", "Counters:c1", "c0", "TextReadouts:", "Counters:c0"); + + shutdownThreading(); +} +#endif + +TEST_P(AdminStatsTest, HandlerStatsScoped) { + InSequence s; + store_->initializeThreading(main_thread_dispatcher_, tls_); + + store_->counterFromStatName(makeStat("foo.c0")).add(0); + Stats::ScopePtr scope0 = store_->createScope(""); + store_->counterFromStatName(makeStat("foo.c1")).add(1); + Stats::ScopePtr scope = store_->createScope("scope"); + scope->counterFromStatName(makeStat("c2")).add(2); + Stats::ScopePtr scope2 = store_->createScope("scope1.scope2"); + scope2->counterFromStatName(makeStat("c3")).add(3); + Stats::ScopePtr scope3 = store_->createScope("scope1.scope2.scope3"); + scope3->counterFromStatName(makeStat("c4")).add(4); + Stats::ScopePtr scope4 = store_->createScope("scope4"); + scope4->counterFromStatName(makeStat("c5")).add(500); + + // Duplicate created with the same name, overriding the stat value. This + // will all be de-duped by the handlers, and the values combined. + Stats::ScopePtr scope4a = store_->createScope("scope4"); + scope4->counterFromStatName(makeStat("c5")).add(55); + + auto test = [this](absl::string_view params, const std::string& expected) { + Buffer::OwnedImpl data; + std::string url = absl::StrCat("/stats?format=json&pretty", params); + EXPECT_EQ(Http::Code::OK, handlerStats(url, data)); + EXPECT_THAT(data.toString(), JsonStringEq(expected)) << "params=" << params; + }; + test("", R"({ + "stats": [ + {"name": "foo.c0", "value": 0}, + {"name": "foo.c1", "value": 1}, + {"name": "scope.c2", "value": 2}, + {"name": "scope1.scope2.c3", "value": 3}, + {"name": "scope1.scope2.scope3.c4", "value": 4}, + {"name": "scope4.c5", "value": 555}]})"); + test("&show_json_scopes", R"({ + "stats": [ + {"name":"foo.c0", "value": 0}, + {"name":"foo.c1", "value": 1}], + "scopes": ["scope", "scope1", "scope4"]})"); + test("&show_json_scopes&scope=scope", R"({ + "stats":[ + {"name":"scope.c2", "value": 2}], + "scopes": []})"); + test("&show_json_scopes&scope=scope1", R"({ + "stats": [], + "scopes": ["scope1.scope2"]})"); + test("&show_json_scopes&scope=scope4", R"({ + "stats": [ + {"name":"scope4.c5", "value": 555}], + "scopes": []})"); + shutdownThreading(); +} + TEST_P(AdminStatsTest, HandlerStatsJson) { - const std::string url = "/stats?format=json"; - Http::TestResponseHeaderMapImpl response_headers; - Buffer::OwnedImpl data; - MockAdminStream admin_stream; - Configuration::MockStatsConfig stats_config; - EXPECT_CALL(stats_config, flushOnAdmin()).WillRepeatedly(testing::Return(false)); - MockInstance instance; + InSequence s; store_->initializeThreading(main_thread_dispatcher_, tls_); - EXPECT_CALL(instance, stats()).WillRepeatedly(testing::ReturnRef(*store_)); - EXPECT_CALL(instance, statsConfig()).WillRepeatedly(testing::ReturnRef(stats_config)); - StatsHandler handler(instance); + const std::string url = "/stats?format=json"; Stats::Counter& c1 = store_->counterFromString("c1"); Stats::Counter& c2 = store_->counterFromString("c2"); @@ -162,8 +299,8 @@ TEST_P(AdminStatsTest, HandlerStatsJson) { store_->mergeHistograms([]() -> void {}); - Http::Code code = handler.handlerStats(url, response_headers, data, admin_stream); - EXPECT_EQ(Http::Code::OK, code); + std::string json_out; + EXPECT_EQ(Http::Code::OK, statsAsJsonHandler(json_out)); const std::string expected_json_old = R"EOF({ "stats": [ @@ -245,7 +382,7 @@ TEST_P(AdminStatsTest, HandlerStatsJson) { ] })EOF"; - EXPECT_THAT(expected_json_old, JsonStringEq(data.toString())); + EXPECT_THAT(expected_json_old, JsonStringEq(json_out)); shutdownThreading(); } @@ -271,14 +408,8 @@ TEST_P(AdminStatsTest, StatsAsJson) { h1.recordValue(100); store_->mergeHistograms([]() -> void {}); - - std::vector histograms = store_->histograms(); - std::sort(histograms.begin(), histograms.end(), - [](const Stats::ParentHistogramSharedPtr& a, - const Stats::ParentHistogramSharedPtr& b) -> bool { return a->name() < b->name(); }); - std::map all_stats; - std::map all_text_readouts; - std::string actual_json = statsAsJsonHandler(all_stats, all_text_readouts, histograms, false); + std::string actual_json; + ASSERT_EQ(Http::Code::OK, statsAsJsonHandler(actual_json)); const std::string expected_json = R"EOF({ "stats": [ @@ -418,11 +549,8 @@ TEST_P(AdminStatsTest, UsedOnlyStatsAsJson) { h1.recordValue(100); store_->mergeHistograms([]() -> void {}); - - std::map all_stats; - std::map all_text_readouts; - std::string actual_json = - statsAsJsonHandler(all_stats, all_text_readouts, store_->histograms(), true); + std::string actual_json; + ASSERT_EQ(Http::Code::OK, statsAsJsonHandler(actual_json, "&usedonly")); // Expected JSON should not have h2 values as it is not used. const std::string expected_json = R"EOF({ @@ -518,12 +646,8 @@ TEST_P(AdminStatsTest, StatsAsJsonFilterString) { h1.recordValue(100); store_->mergeHistograms([]() -> void {}); - - std::map all_stats; - std::map all_text_readouts; - std::string actual_json = - statsAsJsonHandler(all_stats, all_text_readouts, store_->histograms(), false, - absl::optional{std::regex("[a-z]1")}); + std::string actual_json; + ASSERT_EQ(Http::Code::OK, statsAsJsonHandler(actual_json, "&filter=[a-z]1")); // Because this is a filter case, we don't expect to see any stats except for those containing // "h1" in their name. @@ -629,12 +753,8 @@ TEST_P(AdminStatsTest, UsedOnlyStatsAsJsonFilterString) { h3.recordValue(100); store_->mergeHistograms([]() -> void {}); - - std::map all_stats; - std::map all_text_readouts; - std::string actual_json = - statsAsJsonHandler(all_stats, all_text_readouts, store_->histograms(), true, - absl::optional{std::regex("h[12]")}); + std::string actual_json; + ASSERT_EQ(Http::Code::OK, statsAsJsonHandler(actual_json, "&usedonly&filter=h[12]")); // Expected JSON should not have h2 values as it is not used, and should not have h3 values as // they are used but do not match. @@ -778,19 +898,6 @@ class StatsHandlerPrometheusTest : public StatsHandlerTest { Http::TestResponseHeaderMapImpl response_headers; Buffer::OwnedImpl data; MockAdminStream admin_stream; - Configuration::MockStatsConfig stats_config; - Api::MockApi api; - - std::shared_ptr setupMockedInstance() { - auto instance = std::make_shared(); - EXPECT_CALL(stats_config, flushOnAdmin()).WillRepeatedly(testing::Return(false)); - store_->initializeThreading(main_thread_dispatcher_, tls_); - EXPECT_CALL(*instance, stats()).WillRepeatedly(testing::ReturnRef(*store_)); - EXPECT_CALL(*instance, statsConfig()).WillRepeatedly(testing::ReturnRef(stats_config)); - EXPECT_CALL(api, customStatNamespaces()).WillRepeatedly(testing::ReturnRef(customNamespaces())); - EXPECT_CALL(*instance, api()).WillRepeatedly(testing::ReturnRef(api)); - return instance; - } void createTestStats() { Stats::StatNameTagVector c1Tags{{makeStat("cluster"), makeStat("c1")}}; diff --git a/test/server/config_validation/server_test.cc b/test/server/config_validation/server_test.cc index d65e7354fa651..96ccea822ee92 100644 --- a/test/server/config_validation/server_test.cc +++ b/test/server/config_validation/server_test.cc @@ -181,7 +181,7 @@ TEST(ValidationTest, Admin) { ValidationAdmin admin(local_address); std::string empty = ""; Server::Admin::HandlerCb cb; - EXPECT_TRUE(admin.addHandler(empty, empty, cb, false, false)); + EXPECT_TRUE(admin.addHandler(empty, empty, cb, false, false, {})); EXPECT_TRUE(admin.removeHandler(empty)); EXPECT_EQ(1, admin.concurrency()); admin.socket(); diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index d32fe15384b33..c3646e356adec 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -25,6 +25,8 @@ BBR BSON BPF Repick +UI +ajax btree CAS CB From 29cf7765f27d48d99bca15e2b37b46a48fb3d4cb Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 14 Jan 2022 09:28:24 -0500 Subject: [PATCH 02/61] get tests working Signed-off-by: Joshua Marantz --- envoy/server/admin.h | 2 +- source/extensions/common/tap/admin.cc | 2 +- .../extensions/stat_sinks/hystrix/hystrix.cc | 2 +- source/server/admin/BUILD | 4 +- source/server/admin/admin.h | 2 +- source/server/admin/generate_admin_html.sh | 3 - source/server/admin/stats_handler.cc | 94 ++++++++----------- source/server/config_validation/admin.h | 2 +- test/integration/integration_admin_test.cc | 2 +- test/integration/server.h | 9 -- test/server/admin/admin_test.cc | 12 +-- test/server/admin/stats_handler_test.cc | 54 ----------- test/server/config_validation/server_test.cc | 2 +- 13 files changed, 52 insertions(+), 138 deletions(-) diff --git a/envoy/server/admin.h b/envoy/server/admin.h index e901249d2e01b..f7add989fe67b 100644 --- a/envoy/server/admin.h +++ b/envoy/server/admin.h @@ -122,7 +122,7 @@ class Admin { */ virtual bool addHandler(const std::string& prefix, const std::string& help_text, HandlerCb callback, bool removable, bool mutates_server_state, - const ParamDescriptorVec& params) PURE; + const ParamDescriptorVec& params = {}) PURE; /** * Remove an admin handler if it is removable. diff --git a/source/extensions/common/tap/admin.cc b/source/extensions/common/tap/admin.cc index c82eb54e127bb..8c674cb141f6f 100644 --- a/source/extensions/common/tap/admin.cc +++ b/source/extensions/common/tap/admin.cc @@ -29,7 +29,7 @@ AdminHandlerSharedPtr AdminHandler::getSingleton(Server::Admin& admin, AdminHandler::AdminHandler(Server::Admin& admin, Event::Dispatcher& main_thread_dispatcher) : admin_(admin), main_thread_dispatcher_(main_thread_dispatcher) { const bool rc = - admin_.addHandler("/tap", "tap filter control", MAKE_ADMIN_HANDLER(handler), true, true, {}); + admin_.addHandler("/tap", "tap filter control", MAKE_ADMIN_HANDLER(handler), true, true); RELEASE_ASSERT(rc, "/tap admin endpoint is taken"); if (admin_.socket().addressType() == Network::Address::Type::Pipe) { ENVOY_LOG(warn, "Admin tapping (via /tap) is unreliable when the admin endpoint is a pipe and " diff --git a/source/extensions/stat_sinks/hystrix/hystrix.cc b/source/extensions/stat_sinks/hystrix/hystrix.cc index 3a29849c9a9f2..1df26f9e9448f 100644 --- a/source/extensions/stat_sinks/hystrix/hystrix.cc +++ b/source/extensions/stat_sinks/hystrix/hystrix.cc @@ -287,7 +287,7 @@ HystrixSink::HystrixSink(Server::Configuration::ServerFactoryContext& server, ENVOY_LOG(debug, "adding hystrix_event_stream endpoint to enable connection to hystrix dashboard"); admin.addHandler("/hystrix_event_stream", "send hystrix event stream", - MAKE_ADMIN_HANDLER(handlerHystrixEventStream), false, false, {}); + MAKE_ADMIN_HANDLER(handlerHystrixEventStream), false, false); } Http::Code HystrixSink::handlerHystrixEventStream(absl::string_view, diff --git a/source/server/admin/BUILD b/source/server/admin/BUILD index 3e18e25c1d5f3..e39c3de79e093 100644 --- a/source/server/admin/BUILD +++ b/source/server/admin/BUILD @@ -88,12 +88,10 @@ genrule( srcs = [ "admin_head_start.html", "admin.css", - "admin.js", ], outs = ["admin_html_gen.h"], cmd = "./$(location :generate_admin_html.sh) \ - $(location admin_head_start.html) $(location admin.css) \ - $(location admin.js)> $@", + $(location admin_head_start.html) $(location admin.css) > $@", tools = [":generate_admin_html.sh"], visibility = ["//visibility:private"], ) diff --git a/source/server/admin/admin.h b/source/server/admin/admin.h index b4f931b540d5c..a1e6a0a133c3a 100644 --- a/source/server/admin/admin.h +++ b/source/server/admin/admin.h @@ -84,7 +84,7 @@ class AdminImpl : public Admin, // The prefix must start with "/" and contain at least one additional character. bool addHandler(const std::string& prefix, const std::string& help_text, HandlerCb callback, bool removable, bool mutates_server_state, - const ParamDescriptorVec& params) override; + const ParamDescriptorVec& params = {}) override; bool removeHandler(const std::string& prefix) override; ConfigTracker& getConfigTracker() override; diff --git a/source/server/admin/generate_admin_html.sh b/source/server/admin/generate_admin_html.sh index e139dda1061ac..c67783831e4ea 100755 --- a/source/server/admin/generate_admin_html.sh +++ b/source/server/admin/generate_admin_html.sh @@ -8,8 +8,5 @@ cat "$1" echo '' -echo '' echo '' echo ')EOF";' diff --git a/source/server/admin/stats_handler.cc b/source/server/admin/stats_handler.cc index c6c424388cef1..1f12a983b9191 100644 --- a/source/server/admin/stats_handler.cc +++ b/source/server/admin/stats_handler.cc @@ -406,11 +406,11 @@ class StatsHandler::Context { Context(const Params& params, Render& render, Buffer::Instance& response) : params_(params), render_(render), response_(response) {} - void collectScope(const Stats::Scope& scope) { - collect(Type::TextReadouts, scope, text_readouts_); - collect(Type::Counters, scope, counters_); - collect(Type::Gauges, scope, gauges_); - collect(Type::Histograms, scope, histograms_); + void collectStats(const Stats::Store& stats) { + collect(Type::TextReadouts, stats, text_readouts_); + collect(Type::Counters, stats, counters_); + collect(Type::Gauges, stats, gauges_); + collect(Type::Histograms, stats, histograms_); } void emit() { @@ -418,23 +418,45 @@ class StatsHandler::Context { emit(Type::Counters, counters_); emit(Type::Gauges, gauges_); emit(Type::Histograms, histograms_); - emitScopes(); } template - void collect(Type type, const Stats::Scope& scope, SharedStatSet& set) { + void collect(Type type, const Stats::Store& stats, SharedStatSet& set) { // Bail early if the requested type does not match the current type. if (params_.type_ != Type::All && params_.type_ != type) { return; } - Stats::IterateFn fn = [this, &set](const Stats::RefcountPtr& stat) -> bool { - if (params_.shouldShowMetric(*stat)) { - set.insert(stat); + collectHelper(stats, [this, &set](StatType& stat) { + if (params_.shouldShowMetric(stat)) { + set.insert(&stat); } return true; - }; - scope.iterate(fn); + }); + } + + void collectHelper(const Stats::Store& stats, Stats::StatFn fn) { + for (const Stats::TextReadoutSharedPtr& text_readout : stats.textReadouts()) { + fn(*text_readout); + } + } + + void collectHelper(const Stats::Store& stats, Stats::StatFn fn) { + for (const Stats::CounterSharedPtr& counter : stats.counters()) { + fn(*counter); + } + } + + void collectHelper(const Stats::Store& stats, Stats::StatFn fn) { + for (const Stats::GaugeSharedPtr& gauge : stats.gauges()) { + fn(*gauge); + } + } + + void collectHelper(const Stats::Store& stats, Stats::StatFn fn) { + for (const Stats::ParentHistogramSharedPtr& histogram : stats.histograms()) { + fn(*histogram); + } } template void emit(Type type, SharedStatSet& set) { @@ -518,50 +540,10 @@ Http::Code StatsHandler::stats(const Params& params, Stats::Store& stats, break; } - // The default HTML view, with no scope query-param specified (or an empty string), we will - // just generate some HTML to initiate a JSON request to populate the scopes. This way the - // rendering for scopes and sub-scopes can be handled consistently in JavaScript. - if (params.scope_.empty() && params.format_ == Format::Html) { - StatsHandler::HtmlRender::renderAjaxRequestForScopes(response, params); - } else { - Context context(params, *render, response); - - // Note that multiple scopes can exist with the same name, and also that - // scopes may be created/destroyed in other threads, whenever we are not - // holding the store's lock. So when we traverse the scopes, we'll both - // look for Scope objects matching our expected name, and we'll create - // a sorted list of sort names for use in populating next/previous buttons. - auto scope_fn = [this, ¶ms, &context](const Stats::Scope& scope) { - std::string prefix_str = server_.stats().symbolTable().toString(scope.prefix()); - - if (params.query_.find("show_json_scopes") != params.query_.end()) { - if (params.scope_ == prefix_str) { - context.collectScope(scope); - } else if (params.scope_.empty() || - absl::StartsWith(prefix_str + ".", params.scope_ + ".")) { - // Truncate any hierarchy after the prefix. - size_t dot_search = params.scope_.empty() ? 0 : params.scope_.size() + 1; - size_t dot = prefix_str.find('.', dot_search); - if (dot != std::string::npos) { - prefix_str.resize(dot); - } - context.addScope(prefix_str); - } - } else if (prefix_str == params.scope_ || prefix_str.empty() || params.scope_.empty()) { - // If the scope matches the prefix of what the user wants, append in the - // stats from it. Note that scopes with a prefix of "" will match anything - // the user types, in which case we'll still be filtering based on stat - // name prefix. - context.collectScope(scope); - } else if (absl::StartsWith(prefix_str, params.scope_) && - prefix_str[params.scope_.size()] == '.') { - context.addScope(prefix_str); - } - }; - stats.forEachScope([](size_t) {}, scope_fn); - context.emit(); - render->render(response); - } + Context context(params, *render, response); + context.collectStats(stats); + context.emit(); + render->render(response); if (params.format_ == Format::Html) { response.add("\n"); diff --git a/source/server/config_validation/admin.h b/source/server/config_validation/admin.h index e992414dfc428..77d37f8c8e8e8 100644 --- a/source/server/config_validation/admin.h +++ b/source/server/config_validation/admin.h @@ -24,7 +24,7 @@ class ValidationAdmin : public Admin { nullptr) : nullptr) {} bool addHandler(const std::string&, const std::string&, HandlerCb, bool, bool, - const ParamDescriptorVec&) override; + const ParamDescriptorVec& = {}) override; bool removeHandler(const std::string&) override; const Network::Socket& socket() override; ConfigTracker& getConfigTracker() override; diff --git a/test/integration/integration_admin_test.cc b/test/integration/integration_admin_test.cc index 8c69f2e8e0579..4fd1be8016aef 100644 --- a/test/integration/integration_admin_test.cc +++ b/test/integration/integration_admin_test.cc @@ -427,7 +427,7 @@ TEST_P(IntegrationAdminTest, AdminOnDestroyCallbacks) { }; EXPECT_TRUE( - test_server_->server().admin().addHandler("/foo/bar", "hello", callback, true, false, {})); + test_server_->server().admin().addHandler("/foo/bar", "hello", callback, true, false)); // As part of the request, on destroy() should be called and the on_destroy_callback invoked. BufferingStreamDecoderPtr response; diff --git a/test/integration/server.h b/test/integration/server.h index d076e183bf2b1..ac8208ff4f2e9 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -164,8 +164,6 @@ class TestScopeWrapper : public Scope { return wrapped_scope_->iterate(fn); } - StatName prefix() const override { return wrapped_scope_->prefix(); } - private: Thread::MutexBasicLockable& lock_; ScopePtr wrapped_scope_; @@ -297,11 +295,6 @@ class TestIsolatedStoreImpl : public StoreRoot { Thread::LockGuard lock(lock_); store_.forEachTextReadout(f_size, f_stat); } - void forEachScope(std::function f_size, - StatFn f_scope) const override { - Thread::LockGuard lock(lock_); - store_.forEachScope(f_size, f_scope); - } void forEachSinkedCounter(Stats::SizeFn f_size, StatFn f_stat) const override { Thread::LockGuard lock(lock_); store_.forEachSinkedCounter(f_size, f_stat); @@ -400,8 +393,6 @@ class TestIsolatedStoreImpl : public StoreRoot { bool iterate(const IterateFn& fn) const override { return store_.iterate(fn); } bool iterate(const IterateFn& fn) const override { return store_.iterate(fn); } - StatName prefix() const override { return store_.prefix(); } - // Stats::StoreRoot void addSink(Sink&) override {} void setTagProducer(TagProducerPtr&&) override {} diff --git a/test/server/admin/admin_test.cc b/test/server/admin/admin_test.cc index 740653474e390..a6ed33a268866 100644 --- a/test/server/admin/admin_test.cc +++ b/test/server/admin/admin_test.cc @@ -101,7 +101,7 @@ TEST_P(AdminInstanceTest, CustomHandler) { AdminStream&) -> Http::Code { return Http::Code::Accepted; }; // Test removable handler. - EXPECT_NO_LOGS(EXPECT_TRUE(admin_.addHandler("/foo/bar", "hello", callback, true, false, {}))); + EXPECT_NO_LOGS(EXPECT_TRUE(admin_.addHandler("/foo/bar", "hello", callback, true, false))); Http::TestResponseHeaderMapImpl header_map; Buffer::OwnedImpl response; EXPECT_EQ(Http::Code::Accepted, getCallback("/foo/bar", header_map, response)); @@ -112,11 +112,11 @@ TEST_P(AdminInstanceTest, CustomHandler) { EXPECT_FALSE(admin_.removeHandler("/foo/bar")); // Add non removable handler. - EXPECT_TRUE(admin_.addHandler("/foo/bar", "hello", callback, false, false, {})); + EXPECT_TRUE(admin_.addHandler("/foo/bar", "hello", callback, false, false)); EXPECT_EQ(Http::Code::Accepted, getCallback("/foo/bar", header_map, response)); // Add again and make sure it is not there twice. - EXPECT_FALSE(admin_.addHandler("/foo/bar", "hello", callback, false, false, {})); + EXPECT_FALSE(admin_.addHandler("/foo/bar", "hello", callback, false, false)); // Try to remove non removable handler, and make sure it is not removed. EXPECT_FALSE(admin_.removeHandler("/foo/bar")); @@ -129,7 +129,7 @@ TEST_P(AdminInstanceTest, RejectHandlerWithXss) { EXPECT_LOG_CONTAINS("error", "filter \"/foo\" contains invalid character '<'", EXPECT_FALSE(admin_.addHandler("/foo", "hello", - callback, true, false, {}))); + callback, true, false))); } TEST_P(AdminInstanceTest, RejectHandlerWithEmbeddedQuery) { @@ -138,7 +138,7 @@ TEST_P(AdminInstanceTest, RejectHandlerWithEmbeddedQuery) { EXPECT_LOG_CONTAINS("error", "filter \"/bar?queryShouldNotBeInPrefix\" contains invalid character '?'", EXPECT_FALSE(admin_.addHandler("/bar?queryShouldNotBeInPrefix", "hello", - callback, true, false, {}))); + callback, true, false))); } TEST_P(AdminInstanceTest, EscapeHelpTextWithPunctuation) { @@ -148,7 +148,7 @@ TEST_P(AdminInstanceTest, EscapeHelpTextWithPunctuation) { // It's OK to have help text with HTML characters in it, but when we render the home // page they need to be escaped. const std::string planets = "jupiter>saturn>mars"; - EXPECT_TRUE(admin_.addHandler("/planets", planets, callback, true, false, {})); + EXPECT_TRUE(admin_.addHandler("/planets", planets, callback, true, false)); Http::TestResponseHeaderMapImpl header_map; Buffer::OwnedImpl response; diff --git a/test/server/admin/stats_handler_test.cc b/test/server/admin/stats_handler_test.cc index b9986cda945e1..e35d0abef5b02 100644 --- a/test/server/admin/stats_handler_test.cc +++ b/test/server/admin/stats_handler_test.cc @@ -224,60 +224,6 @@ TEST_P(AdminStatsTest, HandlerStatsPage) { } #endif -TEST_P(AdminStatsTest, HandlerStatsScoped) { - InSequence s; - store_->initializeThreading(main_thread_dispatcher_, tls_); - - store_->counterFromStatName(makeStat("foo.c0")).add(0); - Stats::ScopePtr scope0 = store_->createScope(""); - store_->counterFromStatName(makeStat("foo.c1")).add(1); - Stats::ScopePtr scope = store_->createScope("scope"); - scope->counterFromStatName(makeStat("c2")).add(2); - Stats::ScopePtr scope2 = store_->createScope("scope1.scope2"); - scope2->counterFromStatName(makeStat("c3")).add(3); - Stats::ScopePtr scope3 = store_->createScope("scope1.scope2.scope3"); - scope3->counterFromStatName(makeStat("c4")).add(4); - Stats::ScopePtr scope4 = store_->createScope("scope4"); - scope4->counterFromStatName(makeStat("c5")).add(500); - - // Duplicate created with the same name, overriding the stat value. This - // will all be de-duped by the handlers, and the values combined. - Stats::ScopePtr scope4a = store_->createScope("scope4"); - scope4->counterFromStatName(makeStat("c5")).add(55); - - auto test = [this](absl::string_view params, const std::string& expected) { - Buffer::OwnedImpl data; - std::string url = absl::StrCat("/stats?format=json&pretty", params); - EXPECT_EQ(Http::Code::OK, handlerStats(url, data)); - EXPECT_THAT(data.toString(), JsonStringEq(expected)) << "params=" << params; - }; - test("", R"({ - "stats": [ - {"name": "foo.c0", "value": 0}, - {"name": "foo.c1", "value": 1}, - {"name": "scope.c2", "value": 2}, - {"name": "scope1.scope2.c3", "value": 3}, - {"name": "scope1.scope2.scope3.c4", "value": 4}, - {"name": "scope4.c5", "value": 555}]})"); - test("&show_json_scopes", R"({ - "stats": [ - {"name":"foo.c0", "value": 0}, - {"name":"foo.c1", "value": 1}], - "scopes": ["scope", "scope1", "scope4"]})"); - test("&show_json_scopes&scope=scope", R"({ - "stats":[ - {"name":"scope.c2", "value": 2}], - "scopes": []})"); - test("&show_json_scopes&scope=scope1", R"({ - "stats": [], - "scopes": ["scope1.scope2"]})"); - test("&show_json_scopes&scope=scope4", R"({ - "stats": [ - {"name":"scope4.c5", "value": 555}], - "scopes": []})"); - shutdownThreading(); -} - TEST_P(AdminStatsTest, HandlerStatsJson) { InSequence s; store_->initializeThreading(main_thread_dispatcher_, tls_); diff --git a/test/server/config_validation/server_test.cc b/test/server/config_validation/server_test.cc index 96ccea822ee92..d65e7354fa651 100644 --- a/test/server/config_validation/server_test.cc +++ b/test/server/config_validation/server_test.cc @@ -181,7 +181,7 @@ TEST(ValidationTest, Admin) { ValidationAdmin admin(local_address); std::string empty = ""; Server::Admin::HandlerCb cb; - EXPECT_TRUE(admin.addHandler(empty, empty, cb, false, false, {})); + EXPECT_TRUE(admin.addHandler(empty, empty, cb, false, false)); EXPECT_TRUE(admin.removeHandler(empty)); EXPECT_EQ(1, admin.concurrency()); admin.socket(); From 94c97c05f46ce2c6b0491b643e5515e131e0c1a0 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 14 Jan 2022 13:13:27 -0500 Subject: [PATCH 03/61] Add parameters for other admin endpoints. Still need runtime_modify. Signed-off-by: Joshua Marantz --- envoy/server/admin.h | 2 +- source/server/admin/admin.cc | 100 ++++++++++++++--------------------- 2 files changed, 42 insertions(+), 60 deletions(-) diff --git a/envoy/server/admin.h b/envoy/server/admin.h index f7add989fe67b..671291750da67 100644 --- a/envoy/server/admin.h +++ b/envoy/server/admin.h @@ -106,7 +106,7 @@ class Admin { const HandlerCb handler_; const bool removable_; const bool mutates_server_state_; - const ParamDescriptorVec params_; + const ParamDescriptorVec params_{}; }; virtual ~Admin() = default; diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc index 35f99c583713e..7ab3455012438 100644 --- a/source/server/admin/admin.cc +++ b/source/server/admin/admin.cc @@ -95,13 +95,9 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server, server_info_handler_(server), // TODO(jsedgwick) add /runtime_reset endpoint that removes all admin-set values handlers_{ - {"/", "Admin home page", MAKE_ADMIN_HANDLER(handlerAdminHome), false, false, {}}, - {"/certs", - "print certs on machine", - MAKE_ADMIN_HANDLER(server_info_handler_.handlerCerts), - false, - false, - {}}, + {"/", "Admin home page", MAKE_ADMIN_HANDLER(handlerAdminHome), false, false}, + {"/certs", "print certs on machine", + MAKE_ADMIN_HANDLER(server_info_handler_.handlerCerts), false, false}, {"/clusters", "upstream cluster status", MAKE_ADMIN_HANDLER(clusters_handler_.handlerClusters), @@ -113,13 +109,21 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server, MAKE_ADMIN_HANDLER(config_dump_handler_.handlerConfigDump), false, false, - {}}, + {{Admin::ParamDescriptor::Type::String, "resource", + "See documentation for 'mask' below"}, + {Admin::ParamDescriptor::Type::String, "mask", + "When both resource and mask query parameters are specified, the mask is " + "applied to every element in the desired repeated field so that only a " + "subset of fields are returned. The mask is parsed as a ProtobufWkt::FieldMask"}}}, {"/init_dump", "dump current Envoy init manager information (experimental)", MAKE_ADMIN_HANDLER(init_dump_handler_.handlerInitDump), false, false, - {}}, + {{Admin::ParamDescriptor::Type::String, "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`"}}}, {"/contention", "dump current Envoy mutex contention stats (if enabled)", MAKE_ADMIN_HANDLER(stats_handler_.handlerContention), @@ -156,24 +160,16 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server, false, false, {}}, - {"/hot_restart_version", - "print the hot restart compatibility version", - MAKE_ADMIN_HANDLER(server_info_handler_.handlerHotRestartVersion), - false, - false, - {}}, + {"/hot_restart_version", "print the hot restart compatibility version", + MAKE_ADMIN_HANDLER(server_info_handler_.handlerHotRestartVersion), false, false}, {"/logging", "query/change logging levels", MAKE_ADMIN_HANDLER(logs_handler_.handlerLogging), false, true, {}}, - {"/memory", - "print current allocation/heap usage", - MAKE_ADMIN_HANDLER(server_info_handler_.handlerMemory), - false, - false, - {}}, + {"/memory", "print current allocation/heap usage", + MAKE_ADMIN_HANDLER(server_info_handler_.handlerMemory), false, false}, {"/quitquitquit", "exit the server", MAKE_ADMIN_HANDLER(server_cmd_handler_.handlerQuitQuitQuit), @@ -191,19 +187,18 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server, MAKE_ADMIN_HANDLER(listeners_handler_.handlerDrainListeners), false, true, - {}}, - {"/server_info", - "print server version/status information", - MAKE_ADMIN_HANDLER(server_info_handler_.handlerServerInfo), - false, - false, - {}}, - {"/ready", - "print server state, return 200 if LIVE, otherwise return 503", - MAKE_ADMIN_HANDLER(server_info_handler_.handlerReady), - false, - false, - {}}, + {{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."}}}, + {"/server_info", "print server version/status information", + MAKE_ADMIN_HANDLER(server_info_handler_.handlerServerInfo), false, false}, + {"/ready", "print server state, return 200 if LIVE, otherwise return 503", + MAKE_ADMIN_HANDLER(server_info_handler_.handlerReady), false, false}, stats_handler_.statsHandler(), {"/stats/prometheus", "print server stats in prometheus format", @@ -216,36 +211,23 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server, "Render text_readouts as new gaugues with value 0 (increases Prometheus data size)"}, {ParamDescriptor::Type::String, "filter", "Regular expression (ecmascript) for filtering stats"}}}, - {"/stats/recentlookups", - "Show recent stat-name lookups", - MAKE_ADMIN_HANDLER(stats_handler_.handlerStatsRecentLookups), - false, - false, - {}}, - {"/stats/recentlookups/clear", - "clear list of stat-name lookups and counter", - MAKE_ADMIN_HANDLER(stats_handler_.handlerStatsRecentLookupsClear), - false, - true, - {}}, - {"/stats/recentlookups/disable", - "disable recording of reset stat-name lookup names", - MAKE_ADMIN_HANDLER(stats_handler_.handlerStatsRecentLookupsDisable), - false, - true, - {}}, - {"/stats/recentlookups/enable", - "enable recording of reset stat-name lookup names", - MAKE_ADMIN_HANDLER(stats_handler_.handlerStatsRecentLookupsEnable), - false, - true, - {}}, + {"/stats/recentlookups", "Show recent stat-name lookups", + MAKE_ADMIN_HANDLER(stats_handler_.handlerStatsRecentLookups), false, false}, + {"/stats/recentlookups/clear", "clear list of stat-name lookups and counter", + MAKE_ADMIN_HANDLER(stats_handler_.handlerStatsRecentLookupsClear), false, true}, + {"/stats/recentlookups/disable", "disable recording of reset stat-name lookup names", + MAKE_ADMIN_HANDLER(stats_handler_.handlerStatsRecentLookupsDisable), false, true}, + {"/stats/recentlookups/enable", "enable recording of reset stat-name lookup names", + MAKE_ADMIN_HANDLER(stats_handler_.handlerStatsRecentLookupsEnable), false, true}, {"/listeners", "print listener info", MAKE_ADMIN_HANDLER(listeners_handler_.handlerListenerInfo), false, false, - {}}, + {{Admin::ParamDescriptor::Type::Enum, + "format", + "File format to use", + {"text", "json"}}}}, {"/runtime", "print runtime values", MAKE_ADMIN_HANDLER(runtime_handler_.handlerRuntime), From aa00670b92bffd89ec215ea181f28599f9225542 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sat, 15 Jan 2022 09:33:02 -0500 Subject: [PATCH 04/61] add coverage for the HTML generator Signed-off-by: Joshua Marantz --- source/server/admin/admin.cc | 97 ++++++------------- source/server/admin/admin_html_generator.cc | 11 ++- source/server/admin/admin_html_generator.h | 11 ++- source/server/admin/stats_handler.cc | 1 - test/server/admin/BUILD | 8 ++ .../server/admin/admin_html_generator_test.cc | 81 ++++++++++++++++ 6 files changed, 132 insertions(+), 77 deletions(-) create mode 100644 test/server/admin/admin_html_generator_test.cc diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc index 7ab3455012438..eb3403bda12d4 100644 --- a/source/server/admin/admin.cc +++ b/source/server/admin/admin.cc @@ -98,22 +98,17 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server, {"/", "Admin home page", MAKE_ADMIN_HANDLER(handlerAdminHome), false, false}, {"/certs", "print certs on machine", MAKE_ADMIN_HANDLER(server_info_handler_.handlerCerts), false, false}, - {"/clusters", - "upstream cluster status", - MAKE_ADMIN_HANDLER(clusters_handler_.handlerClusters), - false, - false, - {}}, + {"/clusters", "upstream cluster status", + MAKE_ADMIN_HANDLER(clusters_handler_.handlerClusters), false, false}, {"/config_dump", "dump current Envoy configs (experimental)", MAKE_ADMIN_HANDLER(config_dump_handler_.handlerConfigDump), false, false, - {{Admin::ParamDescriptor::Type::String, "resource", - "See documentation for 'mask' below"}, + {{Admin::ParamDescriptor::Type::String, "resource", "The resource to dump"}, {Admin::ParamDescriptor::Type::String, "mask", - "When both resource and mask query parameters are specified, the mask is " - "applied to every element in the desired repeated field so that only a " + "The mask to apply. When both resource and mask are specified, " + "the mask is applied to every element in the desired repeated field so that only a " "subset of fields are returned. The mask is parsed as a ProtobufWkt::FieldMask"}}}, {"/init_dump", "dump current Envoy init manager information (experimental)", @@ -130,52 +125,24 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server, false, false, {}}, - {"/cpuprofiler", - "enable/disable the CPU profiler", - MAKE_ADMIN_HANDLER(profiling_handler_.handlerCpuProfiler), - false, - true, - {}}, - {"/heapprofiler", - "enable/disable the heap profiler", - MAKE_ADMIN_HANDLER(profiling_handler_.handlerHeapProfiler), - false, - true, - {}}, - {"/healthcheck/fail", - "cause the server to fail health checks", - MAKE_ADMIN_HANDLER(server_cmd_handler_.handlerHealthcheckFail), - false, - true, - {}}, - {"/healthcheck/ok", - "cause the server to pass health checks", - MAKE_ADMIN_HANDLER(server_cmd_handler_.handlerHealthcheckOk), - false, - true, - {}}, - {"/help", - "print out list of admin commands", - MAKE_ADMIN_HANDLER(handlerHelp), - false, - false, - {}}, + {"/cpuprofiler", "enable/disable the CPU profiler", + MAKE_ADMIN_HANDLER(profiling_handler_.handlerCpuProfiler), false, true}, + {"/heapprofiler", "enable/disable the heap profiler", + MAKE_ADMIN_HANDLER(profiling_handler_.handlerHeapProfiler), false, true}, + {"/healthcheck/fail", "cause the server to fail health checks", + MAKE_ADMIN_HANDLER(server_cmd_handler_.handlerHealthcheckFail), false, true}, + {"/healthcheck/ok", "cause the server to pass health checks", + MAKE_ADMIN_HANDLER(server_cmd_handler_.handlerHealthcheckOk), false, true}, + {"/help", "print out list of admin commands", MAKE_ADMIN_HANDLER(handlerHelp), false, + false}, {"/hot_restart_version", "print the hot restart compatibility version", MAKE_ADMIN_HANDLER(server_info_handler_.handlerHotRestartVersion), false, false}, - {"/logging", - "query/change logging levels", - MAKE_ADMIN_HANDLER(logs_handler_.handlerLogging), - false, - true, - {}}, + {"/logging", "query/change logging levels", + MAKE_ADMIN_HANDLER(logs_handler_.handlerLogging), false, true}, {"/memory", "print current allocation/heap usage", MAKE_ADMIN_HANDLER(server_info_handler_.handlerMemory), false, false}, - {"/quitquitquit", - "exit the server", - MAKE_ADMIN_HANDLER(server_cmd_handler_.handlerQuitQuitQuit), - false, - true, - {}}, + {"/quitquitquit", "exit the server", + MAKE_ADMIN_HANDLER(server_cmd_handler_.handlerQuitQuitQuit), false, true}, {"/reset_counters", "reset all counters to zero", MAKE_ADMIN_HANDLER(stats_handler_.handlerResetCounters), @@ -228,24 +195,16 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server, "format", "File format to use", {"text", "json"}}}}, - {"/runtime", - "print runtime values", - MAKE_ADMIN_HANDLER(runtime_handler_.handlerRuntime), - false, - false, - {}}, + {"/runtime", "print runtime values", MAKE_ADMIN_HANDLER(runtime_handler_.handlerRuntime), + false, false}, {"/runtime_modify", - "modify runtime values", - MAKE_ADMIN_HANDLER(runtime_handler_.handlerRuntimeModify), - false, - true, - {}}, - {"/reopen_logs", - "reopen access logs", - MAKE_ADMIN_HANDLER(logs_handler_.handlerReopenLogs), - false, - true, - {}}, + "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...", + MAKE_ADMIN_HANDLER(runtime_handler_.handlerRuntimeModify), false, true}, + {"/reopen_logs", "reopen access logs", + MAKE_ADMIN_HANDLER(logs_handler_.handlerReopenLogs), false, true}, }, date_provider_(server.dispatcher().timeSource()), admin_filter_chain_(std::make_shared()), diff --git a/source/server/admin/admin_html_generator.cc b/source/server/admin/admin_html_generator.cc index 8351ef820c53f..fed6778ec784a 100644 --- a/source/server/admin/admin_html_generator.cc +++ b/source/server/admin/admin_html_generator.cc @@ -91,9 +91,13 @@ void AdminHtmlGenerator::renderUrlHandler(const Admin::UrlHandler& handler, // For handlers that mutate state, render the link as a button in a POST form, // rather than an anchor tag. This should discourage crawlers that find the / // page from accidentally mutating all the server state by GETting all the hrefs. - const char* button_style = handler.mutates_server_state_ ? "" : " class='button-as-link'"; const char* method = handler.mutates_server_state_ ? "post" : "get"; - if (visible_submit_) { + if (submit_on_change_) { + response_.add(absl::StrCat("\n\n")); + } else { + // Render an explicit visible submit as a link (for GET) or button (for POST). + const char* button_style = handler.mutates_server_state_ ? "" : " class='button-as-link'"; response_.add(absl::StrCat("\n\n", "\n", "
    \n", " \n" " ", Html::Utility::sanitize(handler.help_text_), "\n", "\n")); - } else { - response_.add(absl::StrCat("\n\n")); } std::vector params; diff --git a/source/server/admin/admin_html_generator.h b/source/server/admin/admin_html_generator.h index 9d93d4b6874dc..4c4fbbb2d5ade 100644 --- a/source/server/admin/admin_html_generator.h +++ b/source/server/admin/admin_html_generator.h @@ -22,14 +22,21 @@ class AdminHtmlGenerator { void renderInput(absl::string_view id, absl::string_view path, Admin::ParamDescriptor::Type type, OptRef query, const std::vector& enum_choices); + + // By default, editing parameters does not cause a form-submit -- you have + // to click on the link or button first. This is useful for the admin home + // page which lays out all the parameters so users can tweak them before submitting. + // + // Calling setSubmitOnChange(true) makes the form auto-submits when any + // parameters change, and does not have its own explicit submit button. This + // is used to enable the user to adjust query-parameters while visiting an + // html-rendered endpoint. void setSubmitOnChange(bool submit_on_change) { submit_on_change_ = submit_on_change; } - void setVisibleSubmit(bool visible_submit) { visible_submit_ = visible_submit; } private: Buffer::Instance& response_; int index_{0}; bool submit_on_change_{false}; - bool visible_submit_{true}; }; } // namespace Server diff --git a/source/server/admin/stats_handler.cc b/source/server/admin/stats_handler.cc index 1f12a983b9191..465d51f836c9d 100644 --- a/source/server/admin/stats_handler.cc +++ b/source/server/admin/stats_handler.cc @@ -238,7 +238,6 @@ class StatsHandler::HtmlRender : public StatsHandler::TextRender { StatsHandler& stats_handler, const Params& params) : response_(response), html_(response) { response_headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Html); - html_.setVisibleSubmit(false); html_.setSubmitOnChange(true); html_.renderHead(); html_.renderTableBegin(); diff --git a/test/server/admin/BUILD b/test/server/admin/BUILD index d5c121c5c653c..e44b44ad66dfa 100644 --- a/test/server/admin/BUILD +++ b/test/server/admin/BUILD @@ -44,6 +44,14 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "admin_html_generator_test", + srcs = ["admin_html_generator_test.cc"], + deps = [ + "//source/server/admin:admin_html_generator_lib", + ], +) + envoy_cc_test( name = "admin_filter_test", srcs = ["admin_filter_test.cc"], diff --git a/test/server/admin/admin_html_generator_test.cc b/test/server/admin/admin_html_generator_test.cc new file mode 100644 index 0000000000000..480d23307fab7 --- /dev/null +++ b/test/server/admin/admin_html_generator_test.cc @@ -0,0 +1,81 @@ +#include "source/server/admin/admin_html_generator.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +using testing::HasSubstr; +using testing::Not; + +namespace Envoy { +namespace Server { + +class AdminHtmlGeneratorTest : public testing::Test { +protected: + AdminHtmlGeneratorTest() + : generator_(data_), handler_{ + "/prefix", + "help text", + handlerCallback, + false, + false, + {{Admin::ParamDescriptor::Type::Boolean, "param", "param help"}}} {} + + static Http::Code handlerCallback(absl::string_view, Http::ResponseHeaderMap&, Buffer::Instance&, + AdminStream&) { + return Http::Code::OK; + } + + Buffer::OwnedImpl data_; + AdminHtmlGenerator generator_; + Admin::UrlHandler handler_; + Http::Utility::QueryParams query_params_; +}; + +TEST_F(AdminHtmlGeneratorTest, RenderHead) { + generator_.renderHead(); + EXPECT_THAT(data_.toString(), HasSubstr("")); +} + +TEST_F(AdminHtmlGeneratorTest, RenderTableBegin) { + generator_.renderTableBegin(); + EXPECT_THAT(data_.toString(), HasSubstr("")); +} + +TEST_F(AdminHtmlGeneratorTest, RenderTableEnd) { + generator_.renderTableEnd(); + EXPECT_THAT(data_.toString(), HasSubstr("
    ")); +} + +TEST_F(AdminHtmlGeneratorTest, RenderUrlHandlerNoQuery) { + generator_.renderUrlHandler(handler_, query_params_); + std::string out = data_.toString(); + EXPECT_THAT(out, HasSubstr(""))); + EXPECT_THAT(out, HasSubstr("help text")); + EXPECT_THAT(out, HasSubstr("param help")); + EXPECT_THAT(out, HasSubstr("")); + EXPECT_THAT(out, Not(HasSubstr(" onchange='prefix.submit()"))); +} + +TEST_F(AdminHtmlGeneratorTest, RenderUrlHandlerWithQuery) { + query_params_["param"] = "on"; + generator_.renderUrlHandler(handler_, query_params_); + std::string out = data_.toString(); + EXPECT_THAT(out, HasSubstr("")); + EXPECT_THAT(out, HasSubstr("help text")); + EXPECT_THAT(out, HasSubstr("param help")); + EXPECT_THAT(out, HasSubstr("")); + EXPECT_THAT(out, Not(HasSubstr(" onchange='prefix.submit()"))); +} + +TEST_F(AdminHtmlGeneratorTest, RenderUrlHandlerSubmitOnChange) { + generator_.setSubmitOnChange(true); + generator_.renderUrlHandler(handler_, query_params_); + std::string out = data_.toString(); + EXPECT_THAT(out, HasSubstr(" onchange='prefix.submit()")); + EXPECT_THAT(out, Not(HasSubstr(""))); +} + +} // namespace Server +} // namespace Envoy From 34c760716944127a2109fba7ecf8e55e7177d5c9 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sat, 15 Jan 2022 16:46:56 -0500 Subject: [PATCH 05/61] coverage Signed-off-by: Joshua Marantz --- envoy/server/admin.h | 4 +- source/server/admin/admin_html_generator.cc | 20 +--- source/server/admin/stats_handler.cc | 104 +----------------- source/server/admin/stats_handler.h | 71 ++++++------ .../server/admin/admin_html_generator_test.cc | 9 ++ test/server/admin/admin_instance.h | 2 + test/server/admin/stats_handler_test.cc | 51 ++++++++- 7 files changed, 99 insertions(+), 162 deletions(-) diff --git a/envoy/server/admin.h b/envoy/server/admin.h index 671291750da67..e2d4a3ac0ea13 100644 --- a/envoy/server/admin.h +++ b/envoy/server/admin.h @@ -75,11 +75,11 @@ class AdminStream { class Admin { public: struct ParamDescriptor { - enum class Type { Boolean, String, Enum, Mask, Hidden }; + enum class Type { Boolean, String, Enum, Hidden }; 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. - std::vector choices_{}; // Valid values for enums or masks + std::vector enum_choices_{}; }; using ParamDescriptorVec = std::vector; diff --git a/source/server/admin/admin_html_generator.cc b/source/server/admin/admin_html_generator.cc index fed6778ec784a..47565fe87c425 100644 --- a/source/server/admin/admin_html_generator.cc +++ b/source/server/admin/admin_html_generator.cc @@ -110,7 +110,7 @@ void AdminHtmlGenerator::renderUrlHandler(const Admin::UrlHandler& handler, std::vector params; for (const Admin::ParamDescriptor& param : handler.params_) { response_.add(absl::StrCat("\n", " ")); - renderInput(param.id_, path, param.type_, query, param.choices_); + renderInput(param.id_, path, param.type_, query, param.enum_choices_); response_.add(absl::StrCat("\n", " ", Html::Utility::sanitize(param.help_), "\n", "\n")); } @@ -119,7 +119,7 @@ void AdminHtmlGenerator::renderUrlHandler(const Admin::UrlHandler& handler, void AdminHtmlGenerator::renderInput(absl::string_view id, absl::string_view path, Admin::ParamDescriptor::Type type, OptRef query, - const std::vector& choices) { + const std::vector& enum_choices) { std::string value; if (query.has_value()) { auto iter = query->find(std::string(id)); @@ -148,24 +148,10 @@ void AdminHtmlGenerator::renderInput(absl::string_view id, absl::string_view pat "'", on_change, value.empty() ? "" : absl::StrCat(" value='", value, "'"), "/>")); break; - case Admin::ParamDescriptor::Type::Mask: { - response_.add("\n"); - for (size_t row = 0; row < (choices.size() + 1) / 2; ++row) { - response_.add(" \n "); - uint32_t last_index = std::min(2 * (row + 1), choices.size()); - for (size_t i = 2 * row; i < last_index; ++i) { - response_.add(absl::StrCat(" \n")); - } - response_.add(" \n"); - } - response_.add("
    ", choices[i], "
    \n "); - break; - } case Admin::ParamDescriptor::Type::Enum: response_.add(absl::StrCat("\n ")); + response_.addFragments({""}); break; case Admin::ParamDescriptor::Type::String: - response_.add(absl::StrCat("")); + response_.addFragments({""}); break; case Admin::ParamDescriptor::Type::Hidden: - response_.add(absl::StrCat("")); + response_.addFragments({""}); break; case Admin::ParamDescriptor::Type::Enum: - response_.add(absl::StrCat("\n \n"}); for (absl::string_view choice : enum_choices) { std::string sanitized = Html::Utility::sanitize(choice); - response_.add(absl::StrCat(" \n")); + absl::string_view selected = (value == sanitized) ? " selected" : ""; + response_.addFragments({" \n"}); } response_.add(" \n "); break; diff --git a/source/server/admin/stats_handler.cc b/source/server/admin/stats_handler.cc index d58519b197f57..71bfdfc5c2f5b 100644 --- a/source/server/admin/stats_handler.cc +++ b/source/server/admin/stats_handler.cc @@ -10,7 +10,6 @@ #include "source/common/common/empty_string.h" #include "source/common/http/headers.h" #include "source/common/http/utility.h" -#include "source/common/memory/stats.h" #include "source/server/admin/prometheus_stats.h" #include "source/server/admin/stats_request.h" From 5f5ae3c0e5bd652853602b4ae60b9a7324a5e8ff Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sun, 10 Apr 2022 12:02:23 -0400 Subject: [PATCH 25/61] use the correct UrlHandler block for /stats. Signed-off-by: Joshua Marantz --- source/server/admin/admin.cc | 2 +- source/server/admin/admin_html_generator.cc | 35 ++++++++++----------- 2 files changed, 18 insertions(+), 19 deletions(-) diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc index 4b6379b0c357d..ff5b416895973 100644 --- a/source/server/admin/admin.cc +++ b/source/server/admin/admin.cc @@ -152,7 +152,7 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server, MAKE_ADMIN_HANDLER(server_info_handler_.handlerServerInfo), false, false), makeHandler("/ready", "print server state, return 200 if LIVE, otherwise return 503", MAKE_ADMIN_HANDLER(server_info_handler_.handlerReady), false, false), - makeStreamingHandler("/stats", "print server stats", stats_handler_, false, false), + stats_handler_.statsHandler(), makeHandler("/stats/prometheus", "print server stats in prometheus format", MAKE_ADMIN_HANDLER(stats_handler_.handlerPrometheusStats), false, false, {{ParamDescriptor::Type::Boolean, "usedonly", diff --git a/source/server/admin/admin_html_generator.cc b/source/server/admin/admin_html_generator.cc index bcdb14f2c54f2..80a2bf6ae9776 100644 --- a/source/server/admin/admin_html_generator.cc +++ b/source/server/admin/admin_html_generator.cc @@ -93,24 +93,23 @@ void AdminHtmlGenerator::renderUrlHandler(const Admin::UrlHandler& handler, // page from accidentally mutating all the server state by GETting all the hrefs. const char* method = handler.mutates_server_state_ ? "post" : "get"; if (submit_on_change_) { - response_.addFragments({"\n
    \n"}); + response_.addFragments({"\n
    \n"}); } else { // Render an explicit visible submit as a link (for GET) or button (for POST). const char* button_style = handler.mutates_server_state_ ? "" : " class='button-as-link'"; - response_.addFragments({ - "\n\n\n
    \n ", path, - "\n \n ", - Html::Utility::sanitize(handler.help_text_), "\n\n"}); + response_.addFragments({"\n\n\n
    \n ", + path, "\n \n ", + Html::Utility::sanitize(handler.help_text_), "\n\n"}); } for (const Admin::ParamDescriptor& param : handler.params_) { response_.addFragments({"\n "}); renderInput(param.id_, path, param.type_, query, param.enum_choices_); - response_.addFragments({"\n ", - Html::Utility::sanitize(param.help_), "\n\n"}); + response_.addFragments({"\n ", Html::Utility::sanitize(param.help_), + "\n\n"}); } } @@ -138,24 +137,24 @@ void AdminHtmlGenerator::renderInput(absl::string_view id, absl::string_view pat switch (type) { case Admin::ParamDescriptor::Type::Boolean: response_.addFragments({""}); + "'", on_change, value.empty() ? "" : " checked/>"}); break; case Admin::ParamDescriptor::Type::String: response_.addFragments({""}); + on_change, value_tag(value), "/>"}); break; case Admin::ParamDescriptor::Type::Hidden: - response_.addFragments({""}); + response_.addFragments({""}); break; case Admin::ParamDescriptor::Type::Enum: - response_.addFragments({"\n \n"}); for (absl::string_view choice : enum_choices) { std::string sanitized = Html::Utility::sanitize(choice); absl::string_view selected = (value == sanitized) ? " selected" : ""; - response_.addFragments({" \n"}); + response_.addFragments( + {" \n"}); } response_.add(" \n "); break; From c9041e69fcf190b091afb668e063f46a0fba854f Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sun, 10 Apr 2022 22:19:11 -0400 Subject: [PATCH 26/61] add histogram buckets into UI params. Signed-off-by: Joshua Marantz --- envoy/server/admin.h | 6 +++--- source/server/admin/stats_handler.cc | 6 +++++- source/server/admin/utils.cc | 3 ++- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/envoy/server/admin.h b/envoy/server/admin.h index 668869dd6d54d..d438b5eb1304e 100644 --- a/envoy/server/admin.h +++ b/envoy/server/admin.h @@ -93,9 +93,9 @@ class Admin { public: virtual ~Admin() = default; - // Describes a parameter for a stats endpoint. This structure is used to - // populate an HTML form to enable a visitor to the admin console to - // intuitively specify parameters for each command that takes them. + // Describes a parameter for an endpoint. This structure is used to populate + // an HTML form to enable a visitor to the admin console to intuitively + // specify query-parameters for each endpoint. struct ParamDescriptor { enum class Type { Boolean, String, Enum, Hidden }; const Type type_; diff --git a/source/server/admin/stats_handler.cc b/source/server/admin/stats_handler.cc index 71bfdfc5c2f5b..bcf141f932e21 100644 --- a/source/server/admin/stats_handler.cc +++ b/source/server/admin/stats_handler.cc @@ -182,7 +182,11 @@ Admin::UrlHandler StatsHandler::statsHandler() { "type", "Stat types to include.", {Labels::All, Labels::Counters, Labels::Histograms, Labels::Gauges, - Labels::TextReadouts}}}}; + Labels::TextReadouts}}, + {Admin::ParamDescriptor::Type::Enum, + "histogram_buckets", + "Histogram bucket display mode", + {"cumulative", "disjoint", "none"}}}}; } } // namespace Server diff --git a/source/server/admin/utils.cc b/source/server/admin/utils.cc index 7ce492142bec4..38f5daad65c39 100644 --- a/source/server/admin/utils.cc +++ b/source/server/admin/utils.cc @@ -68,8 +68,9 @@ absl::Status histogramBucketsParam(const Http::Utility::QueryParams& params, histogram_buckets_mode = HistogramBucketsMode::Cumulative; } else if (histogram_buckets_query_param.value() == "disjoint") { histogram_buckets_mode = HistogramBucketsMode::Disjoint; - } else { + } else if (histogram_buckets_query_param.value() == "none") { histogram_buckets_mode = HistogramBucketsMode::NoBuckets; + } else { return absl::InvalidArgumentError( "usage: /stats?histogram_buckets=cumulative or /stats?histogram_buckets=disjoint \n"); } From afbcf1a5c3847d3f926371f841b68617c33ffb28 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sun, 10 Apr 2022 23:19:46 -0400 Subject: [PATCH 27/61] finalize before returning when we are not covering all types. Signed-off-by: Joshua Marantz --- source/server/admin/stats_request.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/source/server/admin/stats_request.cc b/source/server/admin/stats_request.cc index 29393e3e2a17e..098fa3cade34e 100644 --- a/source/server/admin/stats_request.cc +++ b/source/server/admin/stats_request.cc @@ -65,6 +65,7 @@ bool StatsRequest::nextChunk(Buffer::Instance& response) { phase_stat_count_ = 0; } if (params_.type_ != StatsType::All) { + render_->finalize(response); return false; } switch (phase_) { From 3fc02f0f4725d86726c6099716ad6973b5cc5d7e Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Mon, 11 Apr 2022 08:34:33 -0400 Subject: [PATCH 28/61] format Signed-off-by: Joshua Marantz --- source/server/admin/stats_handler.cc | 48 ++++++++++++++-------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/source/server/admin/stats_handler.cc b/source/server/admin/stats_handler.cc index bcf141f932e21..092ef5abc3852 100644 --- a/source/server/admin/stats_handler.cc +++ b/source/server/admin/stats_handler.cc @@ -163,30 +163,30 @@ Http::Code StatsHandler::handlerContention(absl::string_view, } Admin::UrlHandler StatsHandler::statsHandler() { - return {"/stats", - "print server stats", - [this](absl::string_view path, AdminStream& admin_stream) -> Admin::RequestPtr { - return makeRequest(path, admin_stream); - }, - false, - false, - {{Admin::ParamDescriptor::Type::Boolean, "usedonly", - "Only include stats that have been written by system since restart"}, - {Admin::ParamDescriptor::Type::String, "filter", - "Regular expression (ecmascript) for filtering stats"}, - {Admin::ParamDescriptor::Type::Enum, - "format", - "File format to use.", - {"html", "text", "json"}}, - {Admin::ParamDescriptor::Type::Enum, - "type", - "Stat types to include.", - {Labels::All, Labels::Counters, Labels::Histograms, Labels::Gauges, - Labels::TextReadouts}}, - {Admin::ParamDescriptor::Type::Enum, - "histogram_buckets", - "Histogram bucket display mode", - {"cumulative", "disjoint", "none"}}}}; + return { + "/stats", + "print server stats", + [this](absl::string_view path, AdminStream& admin_stream) -> Admin::RequestPtr { + return makeRequest(path, admin_stream); + }, + false, + false, + {{Admin::ParamDescriptor::Type::Boolean, "usedonly", + "Only include stats that have been written by system since restart"}, + {Admin::ParamDescriptor::Type::String, "filter", + "Regular expression (ecmascript) for filtering stats"}, + {Admin::ParamDescriptor::Type::Enum, + "format", + "File format to use.", + {"html", "text", "json"}}, + {Admin::ParamDescriptor::Type::Enum, + "type", + "Stat types to include.", + {Labels::All, Labels::Counters, Labels::Histograms, Labels::Gauges, Labels::TextReadouts}}, + {Admin::ParamDescriptor::Type::Enum, + "histogram_buckets", + "Histogram bucket display mode", + {"cumulative", "disjoint", "none"}}}}; } } // namespace Server From 6dcac05eb945e0b5471ca2abc24773bc02726bf8 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 20 Apr 2022 17:38:41 -0400 Subject: [PATCH 29/61] format Signed-off-by: Joshua Marantz --- test/server/admin/stats_handler_test.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/server/admin/stats_handler_test.cc b/test/server/admin/stats_handler_test.cc index 659cd16b85460..9a6f72514c0b0 100644 --- a/test/server/admin/stats_handler_test.cc +++ b/test/server/admin/stats_handler_test.cc @@ -188,11 +188,11 @@ TEST_P(AdminStatsTest, HandlerStatsHtml) { store_->initializeThreading(main_thread_dispatcher_, tls_); store_->counterFromStatName(makeStat("foo.c0")).add(0); - Stats::ScopePtr scope0 = store_->createScope(""); + Stats::ScopeSharedPtr scope0 = store_->createScope(""); store_->counterFromStatName(makeStat("foo.c1")).add(1); - Stats::ScopePtr scope = store_->createScope("scope"); + Stats::ScopeSharedPtr scope = store_->createScope("scope"); scope->gaugeFromStatName(makeStat("g2"), Stats::Gauge::ImportMode::Accumulate).set(2); - Stats::ScopePtr scope2 = store_->createScope("scope1.scope2"); + Stats::ScopeSharedPtr scope2 = store_->createScope("scope1.scope2"); scope2->textReadoutFromStatName(makeStat("t3")).set("text readout value"); scope2->counterFromStatName(makeStat("unset")); From 81f2763884df295d6a25bc8e8667aad2f9eab451 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 20 Apr 2022 19:07:07 -0400 Subject: [PATCH 30/61] API change for StatsParams Signed-off-by: Joshua Marantz --- test/server/admin/stats_handler_test.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/server/admin/stats_handler_test.cc b/test/server/admin/stats_handler_test.cc index 9a6f72514c0b0..bd0072b1087df 100644 --- a/test/server/admin/stats_handler_test.cc +++ b/test/server/admin/stats_handler_test.cc @@ -1171,8 +1171,7 @@ class ThreadedTest : public testing::Test { } void statsEndpoint() { - StatsRequest request(*store_, false, false, Utility::HistogramBucketsMode::NoBuckets, - absl::nullopt); + StatsRequest request(*store_, StatsParams()); Http::TestResponseHeaderMapImpl response_headers; request.start(response_headers); Buffer::OwnedImpl data; From 25c871e9f31859dd5e54a14c9360bafa477a9e46 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 11 May 2022 20:06:42 -0400 Subject: [PATCH 31/61] try to get it to compile Signed-off-by: Joshua Marantz --- source/server/admin/BUILD | 15 ++------------ source/server/admin/stats_handler.cc | 26 ------------------------- source/server/admin/stats_handler.h | 7 ++++--- source/server/admin/stats_params.cc | 4 +--- source/server/admin/stats_params.h | 9 ++------- source/server/admin/stats_request.cc | 2 -- test/server/admin/stats_handler_test.cc | 1 - 7 files changed, 9 insertions(+), 55 deletions(-) diff --git a/source/server/admin/BUILD b/source/server/admin/BUILD index 9dc6ad3f4b054..7783bc1995f77 100644 --- a/source/server/admin/BUILD +++ b/source/server/admin/BUILD @@ -125,6 +125,8 @@ envoy_cc_library( deps = [ ":utils_lib", "//envoy/buffer:buffer_interface", + "//envoy/http:codes_interface", + "//envoy/server:admin_interface", "//source/common/stats:histogram_lib", ], ) @@ -147,19 +149,6 @@ envoy_cc_library( ], ) -envoy_cc_library( - name = "stats_params_lib", - srcs = ["stats_params.cc"], - hdrs = ["stats_params.h"], - deps = [ - ":utils_lib", - "//envoy/buffer:buffer_interface", - "//envoy/http:codes_interface", - "//envoy/server:admin_interface", - "//source/common/stats:histogram_lib", - ], -) - envoy_cc_library( name = "stats_render_lib", srcs = ["stats_render.cc"], diff --git a/source/server/admin/stats_handler.cc b/source/server/admin/stats_handler.cc index ff12ace1e33b0..a97d8c7b66961 100644 --- a/source/server/admin/stats_handler.cc +++ b/source/server/admin/stats_handler.cc @@ -98,36 +98,10 @@ Admin::RequestPtr StatsHandler::makeRequest(absl::string_view path, AdminStream& server_.flushStats(); } - if (params.format_ == StatsFormat::Prometheus) { - // TODO(#16139): modify streaming algorithm to cover Prometheus. - // - // This may be easiest to accomplish by populating the set - // with tagExtractedName(), and allowing for vectors of - // stats as multiples will have the same tag-extracted names. - // Ideally we'd find a way to do this without slowing down - // the non-Prometheus implementations. - Stats::Store& store = server_.stats(); - std::vector histograms = store.histograms(); - store.symbolTable().sortByStatNames( - histograms.begin(), histograms.end(), - [](const Stats::ParentHistogramSharedPtr& a) -> Stats::StatName { return a->statName(); }); - const std::vector& text_readouts_vec = - params.prometheus_text_readouts_ ? store.textReadouts() - : std::vector(); - PrometheusStatsFormatter::statsAsPrometheus( - store.counters(), store.gauges(), histograms, text_readouts_vec, response, - params.used_only_, params.filter_, server_.api().customStatNamespaces()); - return Admin::makeStaticTextRequest(response, code); - } return makeRequest(server_.stats(), params, [this]() -> Admin::UrlHandler { return statsHandler(); }); } -Admin::RequestPtr StatsHandler::makeRequest(Stats::Store& stats, const StatsParams& params, - StatsRequest::UrlHandlerFn url_handler_fn) { - return makeRequest(server_.stats(), params); -} - Admin::RequestPtr StatsHandler::makeRequest(Stats::Store& stats, const StatsParams& params) { return std::make_unique(stats, params); } diff --git a/source/server/admin/stats_handler.h b/source/server/admin/stats_handler.h index ffdbaca48a932..18999940ac41d 100644 --- a/source/server/admin/stats_handler.h +++ b/source/server/admin/stats_handler.h @@ -90,9 +90,10 @@ class StatsHandler : public HandlerContextBase { */ Admin::UrlHandler statsHandler(); - Admin::RequestPtr makeRequest(absl::string_view path, AdminStream& admin_stream); - static Admin::RequestPtr makeRequest(Stats::Store& stats, const StatsParams& params, - StatsRequest::UrlHandlerFn url_handler_fn); + static Admin::RequestPtr makeRequest(Stats::Store& stats, const StatsParams& params); + Admin::RequestPtr makeRequest(absl::string_view path, AdminStream&); + //static Admin::RequestPtr makeRequest(Stats::Store& stats, const StatsParams& params, + // StatsRequest::UrlHandlerFn url_handler_fn); private: class Context; diff --git a/source/server/admin/stats_params.cc b/source/server/admin/stats_params.cc index 5a4b129ea0242..5c54d9a0da410 100644 --- a/source/server/admin/stats_params.cc +++ b/source/server/admin/stats_params.cc @@ -70,9 +70,7 @@ Http::Code StatsParams::parse(absl::string_view url, Buffer::Instance& response) } else if (format_value.value() == "html") { format_ = StatsFormat::Html; } else { - response.add("usage: /stats?format=json or /stats?format=prometheus \n\n"); - } else { - response.add("usage: /stats?format=(json|prometheus|text)\n\n"); + response.add("usage: /stats?format=(html|json|prometheus|text)\n\n"); return Http::Code::BadRequest; } } diff --git a/source/server/admin/stats_params.h b/source/server/admin/stats_params.h index 8cf0338798a60..c1be98e060edf 100644 --- a/source/server/admin/stats_params.h +++ b/source/server/admin/stats_params.h @@ -9,6 +9,8 @@ #include "source/server/admin/utils.h" +#include "re2/re2.h" + namespace Envoy { namespace Server { @@ -20,13 +22,6 @@ constexpr absl::string_view Histograms = "Histograms"; constexpr absl::string_view TextReadouts = "TextReadouts"; } // namespace Labels -enum class StatsFormat { -======= -#include "re2/re2.h" - -namespace Envoy { -namespace Server { - enum class StatsFormat { Html, Json, diff --git a/source/server/admin/stats_request.cc b/source/server/admin/stats_request.cc index 740476ac004ef..602392a0894cc 100644 --- a/source/server/admin/stats_request.cc +++ b/source/server/admin/stats_request.cc @@ -33,8 +33,6 @@ Http::Code StatsRequest::start(Http::ResponseHeaderMap& response_headers) { render_ = std::make_unique(response_headers, response_, url_handler_fn_(), params_); break; - case StatsFormat::Prometheus: - ASSERT(false); case StatsFormat::Prometheus: // TODO(#16139): once Prometheus shares this algorithm here, this becomes a legitimate choice. IS_ENVOY_BUG("reached Prometheus case in switch unexpectedly"); diff --git a/test/server/admin/stats_handler_test.cc b/test/server/admin/stats_handler_test.cc index 650635977bcbf..00f6adb06cfc7 100644 --- a/test/server/admin/stats_handler_test.cc +++ b/test/server/admin/stats_handler_test.cc @@ -181,7 +181,6 @@ TEST_F(AdminStatsTest, HandlerStatsPlainText) { "P90(109,109) P95(109.5,109.5) P99(109.9,109.9) P99.5(109.95,109.95) " "P99.9(109.99,109.99) P100(110,110)\n"; EXPECT_EQ(expected, code_response.second); -<<<<<<< HEAD } TEST_P(AdminStatsTest, HandlerStatsHtml) { From 685e307fadb2257353cbd54958ff84ffcc7c894c Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 11 May 2022 20:45:13 -0400 Subject: [PATCH 32/61] get tests working Signed-off-by: Joshua Marantz --- source/server/admin/stats_handler.cc | 5 +++-- source/server/admin/stats_handler.h | 5 +++-- test/server/admin/BUILD | 10 +--------- test/server/admin/stats_handler_test.cc | 6 ++---- 4 files changed, 9 insertions(+), 17 deletions(-) diff --git a/source/server/admin/stats_handler.cc b/source/server/admin/stats_handler.cc index a97d8c7b66961..5cbdff2809963 100644 --- a/source/server/admin/stats_handler.cc +++ b/source/server/admin/stats_handler.cc @@ -102,8 +102,9 @@ Admin::RequestPtr StatsHandler::makeRequest(absl::string_view path, AdminStream& [this]() -> Admin::UrlHandler { return statsHandler(); }); } -Admin::RequestPtr StatsHandler::makeRequest(Stats::Store& stats, const StatsParams& params) { - return std::make_unique(stats, params); +Admin::RequestPtr StatsHandler::makeRequest(Stats::Store& stats, const StatsParams& params, + StatsRequest::UrlHandlerFn url_handler_fn) { + return std::make_unique(stats, params, url_handler_fn); } Http::Code StatsHandler::handlerPrometheusStats(absl::string_view path_and_query, diff --git a/source/server/admin/stats_handler.h b/source/server/admin/stats_handler.h index 18999940ac41d..34cd0635a05ad 100644 --- a/source/server/admin/stats_handler.h +++ b/source/server/admin/stats_handler.h @@ -90,9 +90,10 @@ class StatsHandler : public HandlerContextBase { */ Admin::UrlHandler statsHandler(); - static Admin::RequestPtr makeRequest(Stats::Store& stats, const StatsParams& params); + static Admin::RequestPtr makeRequest(Stats::Store& stats, const StatsParams& params, + StatsRequest::UrlHandlerFn url_handler_fn = nullptr); Admin::RequestPtr makeRequest(absl::string_view path, AdminStream&); - //static Admin::RequestPtr makeRequest(Stats::Store& stats, const StatsParams& params, + // static Admin::RequestPtr makeRequest(Stats::Store& stats, const StatsParams& params, // StatsRequest::UrlHandlerFn url_handler_fn); private: diff --git a/test/server/admin/BUILD b/test/server/admin/BUILD index c87f29c78c16a..8b397faa5feb8 100644 --- a/test/server/admin/BUILD +++ b/test/server/admin/BUILD @@ -88,6 +88,7 @@ envoy_cc_test( name = "stats_params_test", srcs = ["stats_params_test.cc"], deps = [ + "//source/common/buffer:buffer_lib", "//source/server/admin:stats_params_lib", "//test/test_common:test_runtime_lib", ], @@ -119,15 +120,6 @@ envoy_cc_test( ], ) -envoy_cc_test( - name = "stats_params_test", - srcs = ["stats_params_test.cc"], - deps = [ - "//source/common/buffer:buffer_lib", - "//source/server/admin:stats_params_lib", - ], -) - envoy_cc_benchmark_binary( name = "stats_handler_speed_test", srcs = ["stats_handler_speed_test.cc"], diff --git a/test/server/admin/stats_handler_test.cc b/test/server/admin/stats_handler_test.cc index 00f6adb06cfc7..84376ec0892c3 100644 --- a/test/server/admin/stats_handler_test.cc +++ b/test/server/admin/stats_handler_test.cc @@ -142,7 +142,7 @@ TEST_F(AdminStatsTest, HandlerStatsInvalidFormat) { const std::string url = "/stats?format=blergh"; const CodeResponse code_response(handlerStats(url)); EXPECT_EQ(Http::Code::BadRequest, code_response.first); - EXPECT_EQ("usage: /stats?format=(json|prometheus|text)\n\n", code_response.second); + EXPECT_EQ("usage: /stats?format=(html|json|prometheus|text)\n\n", code_response.second); } TEST_F(AdminStatsTest, HandlerStatsPlainText) { @@ -183,7 +183,7 @@ TEST_F(AdminStatsTest, HandlerStatsPlainText) { EXPECT_EQ(expected, code_response.second); } -TEST_P(AdminStatsTest, HandlerStatsHtml) { +TEST_F(AdminStatsTest, HandlerStatsHtml) { InSequence s; store_->initializeThreading(main_thread_dispatcher_, tls_); @@ -216,8 +216,6 @@ TEST_P(AdminStatsTest, HandlerStatsHtml) { {"No Histograms found", "scope.g2: 2"}); // not expected test("&usedonly", {"foo.c0: 0", "foo.c1: 1"}, // expected {"scope1.scope2.unset"}); // not expected -======= ->>>>>>> main } TEST_F(AdminStatsTest, HandlerStatsPlainTextHistogramBucketsCumulative) { From fecece3189c4926ab23c1784924aaeadcef8a350 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 24 Jun 2022 08:42:35 -0400 Subject: [PATCH 33/61] fix tests Signed-off-by: Joshua Marantz --- source/server/admin/BUILD | 5 +- source/server/admin/admin.cc | 19 ------- source/server/admin/admin_html.cc | 81 ++++++---------------------- test/server/admin/admin_html_test.cc | 7 +-- 4 files changed, 21 insertions(+), 91 deletions(-) diff --git a/source/server/admin/BUILD b/source/server/admin/BUILD index 1407300672099..24e9dc606311c 100644 --- a/source/server/admin/BUILD +++ b/source/server/admin/BUILD @@ -160,7 +160,6 @@ envoy_cc_library( srcs = ["stats_render.cc"], hdrs = ["stats_render.h"], deps = [ - ":admin_html_generator_lib", ":stats_params_lib", ":utils_lib", "//envoy/server:admin_interface", @@ -168,7 +167,9 @@ envoy_cc_library( "//source/common/html:utility_lib", "//source/common/json:json_sanitizer_lib", "//source/common/stats:histogram_lib", - ], + ] + envoy_select_admin_html([ + ":admin_html_generator_lib", + ]), ) envoy_cc_library( diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc index d2cfdcc441122..cbe1bc1ce13bc 100644 --- a/source/server/admin/admin.cc +++ b/source/server/admin/admin.cc @@ -33,7 +33,6 @@ #include "source/common/protobuf/utility.h" #include "source/common/router/config_impl.h" #include "source/extensions/request_id/uuid/config.h" -#include "source/server/admin/admin_html_generator.h" #include "source/server/admin/utils.h" #include "source/server/listener_impl.h" @@ -361,24 +360,6 @@ void AdminImpl::getHelp(Buffer::Instance& response) { } } -Http::Code AdminImpl::handlerAdminHome(absl::string_view, Http::ResponseHeaderMap& response_headers, - Buffer::Instance& response, AdminStream&) { - response_headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Html); - AdminHtmlGenerator html(response); - html.renderHead(); - html.renderTableBegin(); - - // Prefix order is used during searching, but for printing do them in alpha order. - OptRef no_query_params; - for (const UrlHandler* handler : sortedHandlers()) { - html.renderUrlHandler(*handler, no_query_params); - } - - html.renderTableEnd(); - - return Http::Code::OK; -} - const Network::Address::Instance& AdminImpl::localAddress() { return *server_.localInfo().address(); } diff --git a/source/server/admin/admin_html.cc b/source/server/admin/admin_html.cc index 3745bf85538d4..c8f365099a453 100644 --- a/source/server/admin/admin_html.cc +++ b/source/server/admin/admin_html.cc @@ -1,78 +1,28 @@ #include "source/common/html/utility.h" #include "source/server/admin/admin.h" +#include "source/server/admin/admin_html_generator.h" namespace Envoy { namespace Server { -namespace { - -/** - * Favicon base64 image was harvested by screen-capturing the favicon from a Chrome tab - * while visiting https://www.envoyproxy.io/. The resulting PNG was translated to base64 - * by dropping it into https://www.base64-image.de/ and then pasting the resulting string - * below. - * - * The actual favicon source for that, https://www.envoyproxy.io/img/favicon.ico is nicer - * because it's transparent, but is also 67646 bytes, which is annoying to inline. We could - * just reference that rather than inlining it, but then the favicon won't work when visiting - * the admin page from a network that can't see the internet. - */ -const char EnvoyFavicon[] = - "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABgAAAAYCAYAAADgdz34AAAAAXNSR0IArs4c6QAAAARnQU1" - "BAACxjwv8YQUAAAAJcEhZcwAAEnQAABJ0Ad5mH3gAAAH9SURBVEhL7ZRdTttAFIUrUFaAX5w9gIhgUfzshFRK+gIbaVbA" - "zwaqCly1dSpKk5A485/YCdXpHTB4BsdgVe0bD0cZ3Xsm38yZ8byTUuJ/6g3wqqoBrBhPTzmmLfptMbAzttJTpTKAF2MWC" - "7ADCdNIwXZpvMMwayiIwwS874CcOc9VuQPR1dBBChPMITpFXXU45hukIIH6kHhzVqkEYB8F5HYGvZ5B7EvwmHt9K/59Cr" - "U3QbY2RNYaQPYmJc+jPIBICNCcg20ZsAsCPfbcrFlRF+cJZpvXSJt9yMTxO/IAzJrCOfhJXiOgFEX/SbZmezTWxyNk4Q9" - "anHMmjnzAhEyhAW8LCE6wl26J7ZFHH1FMYQxh567weQBOO1AW8D7P/UXAQySq/QvL8Fu9HfCEw4SKALm5BkC3bwjwhSKr" - "A5hYAMXTJnPNiMyRBVzVjcgCyHiSm+8P+WGlnmwtP2RzbCMiQJ0d2KtmmmPorRHEhfMROVfTG5/fYrF5iWXzE80tfy9WP" - "sCqx5Buj7FYH0LvDyHiqd+3otpsr4/fa5+xbEVQPfrYnntylQG5VGeMLBhgEfyE7o6e6qYzwHIjwl0QwXSvvTmrVAY4D5" - "ddvT64wV0jRrr7FekO/XEjwuwwhuw7Ef7NY+dlfXpLb06EtHUJdVbsxvNUqBrwj/QGeEUSfwBAkmWHn5Bb/gAAAABJRU5"; - -const char AdminHtmlStart[] = R"( - - Envoy Admin - - - - - - - - - - -)"; + // Prefix order is used during searching, but for printing do them in alpha order. + OptRef no_query_params; + for (const UrlHandler* handler : sortedHandlers()) { + html.renderUrlHandler(*handler, no_query_params); + } -const char AdminHtmlEnd[] = R"( - -
    CommandDescription
    - -)"; + html.renderTableEnd(); -} // namespace + return Http::Code::OK; -Http::Code AdminImpl::handlerAdminHome(absl::string_view, Http::ResponseHeaderMap& response_headers, - Buffer::Instance& response, AdminStream&) { +#if 0 response_headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Html); response.add(absl::StrReplaceAll(AdminHtmlStart, {{"@FAVICON@", EnvoyFavicon}})); @@ -116,6 +66,7 @@ Http::Code AdminImpl::handlerAdminHome(absl::string_view, Http::ResponseHeaderMa } response.add(AdminHtmlEnd); return Http::Code::OK; +#endif } } // namespace Server diff --git a/test/server/admin/admin_html_test.cc b/test/server/admin/admin_html_test.cc index 9b9faa406b526..feae8252e6afd 100644 --- a/test/server/admin/admin_html_test.cc +++ b/test/server/admin/admin_html_test.cc @@ -35,11 +35,8 @@ TEST_P(AdminInstanceTest, HelpUsesPostForMutations) { Http::TestResponseHeaderMapImpl header_map; Buffer::OwnedImpl response; EXPECT_EQ(Http::Code::OK, getCallback("/", header_map, response)); - const std::string logging_post = "
    Date: Fri, 24 Jun 2022 09:11:40 -0400 Subject: [PATCH 34/61] get tests working in html-disabled mode. Signed-off-by: Joshua Marantz --- source/server/admin/stats_params.cc | 5 +++++ source/server/admin/stats_params.h | 2 ++ source/server/admin/stats_render.cc | 2 ++ source/server/admin/stats_render.h | 5 +++++ source/server/admin/stats_request.cc | 2 ++ test/server/admin/stats_handler_test.cc | 2 ++ test/server/admin/stats_params_test.cc | 4 ++++ 7 files changed, 22 insertions(+) diff --git a/source/server/admin/stats_params.cc b/source/server/admin/stats_params.cc index 5c54d9a0da410..b117c7498746e 100644 --- a/source/server/admin/stats_params.cc +++ b/source/server/admin/stats_params.cc @@ -68,7 +68,12 @@ Http::Code StatsParams::parse(absl::string_view url, Buffer::Instance& response) } else if (format_value.value() == "text") { format_ = StatsFormat::Text; } else if (format_value.value() == "html") { +#ifdef ENVOY_ADMIN_HTML format_ = StatsFormat::Html; +#else + response.add("HTML output was disabled by building with --define=admin_html=disabled"); + return Http::Code::BadRequest; +#endif } else { response.add("usage: /stats?format=(html|json|prometheus|text)\n\n"); return Http::Code::BadRequest; diff --git a/source/server/admin/stats_params.h b/source/server/admin/stats_params.h index c1be98e060edf..d50886972e0d0 100644 --- a/source/server/admin/stats_params.h +++ b/source/server/admin/stats_params.h @@ -23,7 +23,9 @@ constexpr absl::string_view TextReadouts = "TextReadouts"; } // namespace Labels enum class StatsFormat { +#ifdef ENVOY_ADMIN_HTML Html, +#endif Json, Prometheus, Text, diff --git a/source/server/admin/stats_render.cc b/source/server/admin/stats_render.cc index 7ba541e1379e2..54b35f2774a84 100644 --- a/source/server/admin/stats_render.cc +++ b/source/server/admin/stats_render.cc @@ -263,6 +263,7 @@ void StatsJsonRender::collectBuckets(const std::string& name, *histogram_array_->add_values() = ValueUtil::structValue(histogram_obj); } +#ifdef ENVOY_ADMIN_HTML StatsHtmlRender::StatsHtmlRender(Http::ResponseHeaderMap& response_headers, Buffer::Instance& response, const Admin::UrlHandler& url_handler, const StatsParams& params) @@ -283,6 +284,7 @@ void StatsHtmlRender::finalize(Buffer::Instance& response) { response.add("\n
    No ", types, " found
    \n
    \n"});
 }
+#endif
 
 } // namespace Server
 } // namespace Envoy
diff --git a/source/server/admin/stats_render.h b/source/server/admin/stats_render.h
index a2e56988e9da9..815a085767d5c 100644
--- a/source/server/admin/stats_render.h
+++ b/source/server/admin/stats_render.h
@@ -4,7 +4,10 @@
 #include "envoy/stats/stats.h"
 
 #include "source/common/buffer/buffer_impl.h"
+
+#ifdef ENVOY_ADMIN_HTML
 #include "source/server/admin/admin_html_generator.h"
+#endif
 #include "source/server/admin/stats_params.h"
 #include "source/server/admin/utils.h"
 
@@ -96,6 +99,7 @@ class StatsJsonRender : public StatsRender {
   std::string value_buffer_; // Used for Json::sanitize for text-readout values.
 };
 
+#ifdef ENVOY_ADMIN_HTML
 class StatsHtmlRender : public StatsTextRender {
 public:
   StatsHtmlRender(Http::ResponseHeaderMap& response_headers, Buffer::Instance& response,
@@ -107,6 +111,7 @@ class StatsHtmlRender : public StatsTextRender {
 private:
   AdminHtmlGenerator html_;
 };
+#endif
 
 } // namespace Server
 } // namespace Envoy
diff --git a/source/server/admin/stats_request.cc b/source/server/admin/stats_request.cc
index 602392a0894cc..2797a1aaa1b24 100644
--- a/source/server/admin/stats_request.cc
+++ b/source/server/admin/stats_request.cc
@@ -29,10 +29,12 @@ Http::Code StatsRequest::start(Http::ResponseHeaderMap& response_headers) {
   case StatsFormat::Text:
     render_ = std::make_unique(params_);
     break;
+#ifdef ENVOY_ADMIN_HTML
   case StatsFormat::Html:
     render_ =
         std::make_unique(response_headers, response_, url_handler_fn_(), params_);
     break;
+#endif
   case StatsFormat::Prometheus:
     // TODO(#16139): once Prometheus shares this algorithm here, this becomes a legitimate choice.
     IS_ENVOY_BUG("reached Prometheus case in switch unexpectedly");
diff --git a/test/server/admin/stats_handler_test.cc b/test/server/admin/stats_handler_test.cc
index 84376ec0892c3..652b788948d74 100644
--- a/test/server/admin/stats_handler_test.cc
+++ b/test/server/admin/stats_handler_test.cc
@@ -183,6 +183,7 @@ TEST_F(AdminStatsTest, HandlerStatsPlainText) {
   EXPECT_EQ(expected, code_response.second);
 }
 
+#ifdef ENVOY_ADMIN_HTML
 TEST_F(AdminStatsTest, HandlerStatsHtml) {
   InSequence s;
   store_->initializeThreading(main_thread_dispatcher_, tls_);
@@ -217,6 +218,7 @@ TEST_F(AdminStatsTest, HandlerStatsHtml) {
   test("&usedonly", {"foo.c0: 0", "foo.c1: 1"},      // expected
        {"scope1.scope2.unset"});                     // not expected
 }
+#endif
 
 TEST_F(AdminStatsTest, HandlerStatsPlainTextHistogramBucketsCumulative) {
   const std::string url = "/stats?histogram_buckets=cumulative";
diff --git a/test/server/admin/stats_params_test.cc b/test/server/admin/stats_params_test.cc
index fecdb06cbf806..6dbf9b68d4ac4 100644
--- a/test/server/admin/stats_params_test.cc
+++ b/test/server/admin/stats_params_test.cc
@@ -39,8 +39,12 @@ TEST(StatsParamsTest, ParseParamsFormat) {
 
   ASSERT_EQ(Http::Code::OK, params.parse("?format=text", response));
   EXPECT_EQ(StatsFormat::Text, params.format_);
+#ifdef ENVOY_ADMIN_HTML
   ASSERT_EQ(Http::Code::OK, params.parse("?format=html", response));
   EXPECT_EQ(StatsFormat::Html, params.format_);
+#else
+  EXPECT_EQ(Http::Code::BadRequest, params.parse("?format=html", response));
+#endif
   ASSERT_EQ(Http::Code::OK, params.parse("?format=json", response));
   EXPECT_EQ(StatsFormat::Json, params.format_);
   ASSERT_EQ(Http::Code::OK, params.parse("?format=prometheus", response));

From d16f773f70667d1c333062db814de8f1e831c5dd Mon Sep 17 00:00:00 2001
From: Joshua Marantz 
Date: Fri, 24 Jun 2022 21:22:33 -0400
Subject: [PATCH 35/61] cleanup

Signed-off-by: Joshua Marantz 
---
 source/server/admin/BUILD           |  2 +-
 source/server/admin/admin_html.cc   | 46 -----------------------------
 source/server/admin/stats_handler.h | 11 -------
 3 files changed, 1 insertion(+), 58 deletions(-)

diff --git a/source/server/admin/BUILD b/source/server/admin/BUILD
index 24e9dc606311c..2a44c5e912d74 100644
--- a/source/server/admin/BUILD
+++ b/source/server/admin/BUILD
@@ -164,11 +164,11 @@ envoy_cc_library(
         ":utils_lib",
         "//envoy/server:admin_interface",
         "//source/common/buffer:buffer_lib",
-        "//source/common/html:utility_lib",
         "//source/common/json:json_sanitizer_lib",
         "//source/common/stats:histogram_lib",
     ] + envoy_select_admin_html([
         ":admin_html_generator_lib",
+        "//source/common/html:utility_lib",
     ]),
 )
 
diff --git a/source/server/admin/admin_html.cc b/source/server/admin/admin_html.cc
index c8f365099a453..c91aa864ae8e4 100644
--- a/source/server/admin/admin_html.cc
+++ b/source/server/admin/admin_html.cc
@@ -21,52 +21,6 @@ Http::Code AdminImpl::handlerAdminHome(absl::string_view, Http::ResponseHeaderMa
   html.renderTableEnd();
 
   return Http::Code::OK;
-
-#if 0
-  response_headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Html);
-
-  response.add(absl::StrReplaceAll(AdminHtmlStart, {{"@FAVICON@", EnvoyFavicon}}));
-
-  // Prefix order is used during searching, but for printing do them in alpha order.
-  for (const UrlHandler* handler : sortedHandlers()) {
-    absl::string_view path = handler->prefix_;
-
-    if (path == "/") {
-      continue; // No need to print self-link to index page.
-    }
-
-    // Remove the leading slash from the link, so that the admin page can be
-    // rendered as part of another console, on a sub-path.
-    //
-    // E.g. consider a downstream dashboard that embeds the Envoy admin console.
-    // In that case, the "/stats" endpoint would be at
-    // https://DASHBOARD/envoy_admin/stats. If the links we present on the home
-    // page are absolute (e.g. "/stats") they won't work in the context of the
-    // dashboard. Removing the leading slash, they will work properly in both
-    // the raw admin console and when embedded in another page and URL
-    // hierarchy.
-    ASSERT(!path.empty());
-    ASSERT(path[0] == '/');
-    path = path.substr(1);
-
-    // For handlers that mutate state, render the link as a button in a POST form,
-    // rather than an anchor tag. This should discourage crawlers that find the /
-    // page from accidentally mutating all the server state by GETting all the hrefs.
-    const char* link_format =
-        handler->mutates_server_state_
-            ? ""
-            : "{}";
-    const std::string link = fmt::format(link_format, path, path);
-
-    // Handlers are all specified by statically above, and are thus trusted and do
-    // not require escaping.
-    response.add(fmt::format("{}"
-                             "{}\n",
-                             link, Html::Utility::sanitize(handler->help_text_)));
-  }
-  response.add(AdminHtmlEnd);
-  return Http::Code::OK;
-#endif
 }
 
 } // namespace Server
diff --git a/source/server/admin/stats_handler.h b/source/server/admin/stats_handler.h
index 34cd0635a05ad..29b7a08fd7a02 100644
--- a/source/server/admin/stats_handler.h
+++ b/source/server/admin/stats_handler.h
@@ -97,20 +97,9 @@ class StatsHandler : public HandlerContextBase {
   //                                     StatsRequest::UrlHandlerFn url_handler_fn);
 
 private:
-  class Context;
-  class HtmlRender;
-  class JsonRender;
-  class Render;
-  class TextRender;
-
-  friend class StatsHandlerTest;
-
   static Http::Code prometheusStats(absl::string_view path_and_query, Buffer::Instance& response,
                                     Stats::Store& stats,
                                     Stats::CustomStatNamespaces& custom_namespaces);
-
-private:
-  friend class StatsHandlerTest;
 };
 
 } // namespace Server

From 2ec28dea9ee47d4ccfc55621b95c1f65fdb16ba4 Mon Sep 17 00:00:00 2001
From: Joshua Marantz 
Date: Fri, 1 Jul 2022 18:32:36 -0400
Subject: [PATCH 36/61] emit the parameter info in the /help response.

Signed-off-by: Joshua Marantz 
---
 envoy/server/admin.h                          |  2 +-
 source/server/admin/admin.cc                  | 10 ++++
 source/server/admin/admin_html_generator.cc   |  4 --
 source/server/admin/stats_handler.cc          |  2 +-
 source/server/admin/stats_render.cc           |  1 -
 .../server/admin/admin_html_generator_test.cc |  6 ---
 test/server/admin/admin_test.cc               | 53 +++++++++++++++++++
 7 files changed, 65 insertions(+), 13 deletions(-)

diff --git a/envoy/server/admin.h b/envoy/server/admin.h
index c98975b2aa79e..00fdd864ae095 100644
--- a/envoy/server/admin.h
+++ b/envoy/server/admin.h
@@ -97,7 +97,7 @@ class Admin {
   // an HTML form to enable a visitor to the admin console to intuitively
   // specify query-parameters for each endpoint.
   struct ParamDescriptor {
-    enum class Type { Boolean, String, Enum, Hidden };
+    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.
diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc
index cbe1bc1ce13bc..7751b18153afc 100644
--- a/source/server/admin/admin.cc
+++ b/source/server/admin/admin.cc
@@ -357,6 +357,16 @@ void AdminImpl::getHelp(Buffer::Instance& response) {
   // 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_));
+    for (const ParamDescriptor& param : handler->params_) {
+      response.add(fmt::format("      {}: {}", param.id_, param.help_));
+      if (param.type_ == ParamDescriptor::Type::Enum) {
+        response.add("; One of");
+        for (absl::string_view choice : param.enum_choices_) {
+          response.addFragments({" ", choice});
+        }
+      }
+      response.add("\n");
+    }
   }
 }
 
diff --git a/source/server/admin/admin_html_generator.cc b/source/server/admin/admin_html_generator.cc
index 80a2bf6ae9776..bbd18199e0b31 100644
--- a/source/server/admin/admin_html_generator.cc
+++ b/source/server/admin/admin_html_generator.cc
@@ -143,10 +143,6 @@ void AdminHtmlGenerator::renderInput(absl::string_view id, absl::string_view pat
     response_.addFragments({""});
     break;
-  case Admin::ParamDescriptor::Type::Hidden:
-    response_.addFragments({""});
-    break;
   case Admin::ParamDescriptor::Type::Enum:
     response_.addFragments(
         {"\n    "});
+                            on_change, value_tag(value), " />"});
     break;
   case Admin::ParamDescriptor::Type::Enum:
     response_.addFragments(

From 048d239fcd15ef187a4d56e5f25aab06bcc5afcc Mon Sep 17 00:00:00 2001
From: Joshua Marantz 
Date: Mon, 11 Jul 2022 21:56:23 -0400
Subject: [PATCH 48/61] split out the html rendered into its own files.

Signed-off-by: Joshua Marantz 
---
 source/server/admin/BUILD                |  8 ++++--
 source/server/admin/stats_html_render.cc | 34 ++++++++++++++++++++++++
 source/server/admin/stats_html_render.h  | 32 ++++++++++++++++++++++
 source/server/admin/stats_render.cc      | 30 ---------------------
 source/server/admin/stats_render.h       | 27 -------------------
 source/server/admin/stats_request.cc     |  4 +++
 test/server/admin/stats_render_test.cc   |  4 +++
 7 files changed, 80 insertions(+), 59 deletions(-)
 create mode 100644 source/server/admin/stats_html_render.cc
 create mode 100644 source/server/admin/stats_html_render.h

diff --git a/source/server/admin/BUILD b/source/server/admin/BUILD
index 2a44c5e912d74..28718837e1c06 100644
--- a/source/server/admin/BUILD
+++ b/source/server/admin/BUILD
@@ -157,8 +157,12 @@ envoy_cc_library(
 
 envoy_cc_library(
     name = "stats_render_lib",
-    srcs = ["stats_render.cc"],
-    hdrs = ["stats_render.h"],
+    srcs = ["stats_render.cc"] + envoy_select_admin_html([
+        "stats_html_render.cc",
+    ]),
+    hdrs = ["stats_render.h"] + envoy_select_admin_html([
+        "stats_html_render.h",
+    ]),
     deps = [
         ":stats_params_lib",
         ":utils_lib",
diff --git a/source/server/admin/stats_html_render.cc b/source/server/admin/stats_html_render.cc
new file mode 100644
index 0000000000000..86aa3e5c4317e
--- /dev/null
+++ b/source/server/admin/stats_html_render.cc
@@ -0,0 +1,34 @@
+#include "source/server/admin/stats_html_render.h"
+
+#include "source/common/html/utility.h"
+
+namespace Envoy {
+namespace Server {
+
+StatsHtmlRender::StatsHtmlRender(Http::ResponseHeaderMap& response_headers,
+                                 Buffer::Instance& response, const Admin::UrlHandler& url_handler,
+                                 const StatsParams& params)
+    : StatsTextRender(params), html_(response) {
+  response_headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Html);
+  html_.setSubmitOnChange(true);
+  html_.renderHead();
+  response.add("\n");
+  html_.renderTableBegin();
+  html_.renderUrlHandler(url_handler, params.query_);
+  html_.renderTableEnd();
+  response.add("
    \n");
+}
+
+void StatsHtmlRender::generate(Buffer::Instance& response, const std::string& name,
+                               const std::string& value) {
+  response.addFragments({name, ": \"", Html::Utility::sanitize(value), "\"\n"});
+}
+
+void StatsHtmlRender::finalize(Buffer::Instance& response) { response.add("
    \n"); }
+
+void StatsHtmlRender::noStats(Buffer::Instance& response, absl::string_view types) {
+  response.addFragments({"
    \n
    No ", types, " found
    \n
    \n"});
+}
+
+} // namespace Server
+} // namespace Envoy
diff --git a/source/server/admin/stats_html_render.h b/source/server/admin/stats_html_render.h
new file mode 100644
index 0000000000000..87daf50f7ed72
--- /dev/null
+++ b/source/server/admin/stats_html_render.h
@@ -0,0 +1,32 @@
+#pragma once
+
+#include "source/server/admin/stats_render.h"
+
+#include "source/server/admin/admin_html_generator.h"
+
+namespace Envoy {
+namespace Server {
+
+class StatsHtmlRender : public StatsTextRender {
+public:
+  StatsHtmlRender(Http::ResponseHeaderMap& response_headers, Buffer::Instance& response,
+                  const Admin::UrlHandler& url_handler, const StatsParams& params);
+
+  void noStats(Buffer::Instance&, absl::string_view types) override;
+  void finalize(Buffer::Instance&) override;
+  void generate(Buffer::Instance& response, const std::string& name,
+                const std::string& value) override;
+  void generate(Buffer::Instance& response, const std::string& name, uint64_t value) override {
+    StatsTextRender::generate(response, name, value);
+  }
+  void generate(Buffer::Instance& response, const std::string& name,
+                const Stats::ParentHistogram& histogram) override {
+    StatsTextRender::generate(response, name, histogram);
+  }
+
+private:
+  AdminHtmlGenerator html_;
+};
+
+} // namespace Server
+} // namespace Envoy
diff --git a/source/server/admin/stats_render.cc b/source/server/admin/stats_render.cc
index 3c77ce6446810..f802d49de5e58 100644
--- a/source/server/admin/stats_render.cc
+++ b/source/server/admin/stats_render.cc
@@ -1,8 +1,5 @@
 #include "source/server/admin/stats_render.h"
 
-#ifdef ENVOY_ADMIN_HTML
-#include "source/common/html/utility.h"
-#endif
 #include "source/common/json/json_sanitizer.h"
 #include "source/common/stats/histogram_impl.h"
 
@@ -265,32 +262,5 @@ void StatsJsonRender::collectBuckets(const std::string& name,
   *histogram_array_->add_values() = ValueUtil::structValue(histogram_obj);
 }
 
-#ifdef ENVOY_ADMIN_HTML
-StatsHtmlRender::StatsHtmlRender(Http::ResponseHeaderMap& response_headers,
-                                 Buffer::Instance& response, const Admin::UrlHandler& url_handler,
-                                 const StatsParams& params)
-    : StatsTextRender(params), html_(response) {
-  response_headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Html);
-  html_.setSubmitOnChange(true);
-  html_.renderHead();
-  response.add("\n");
-  html_.renderTableBegin();
-  html_.renderUrlHandler(url_handler, params.query_);
-  html_.renderTableEnd();
-  response.add("
    \n");
-}
-
-void StatsHtmlRender::generate(Buffer::Instance& response, const std::string& name,
-                               const std::string& value) {
-  response.addFragments({name, ": \"", Html::Utility::sanitize(value), "\"\n"});
-}
-
-void StatsHtmlRender::finalize(Buffer::Instance& response) { response.add("
    \n"); }
-
-void StatsHtmlRender::noStats(Buffer::Instance& response, absl::string_view types) {
-  response.addFragments({"
    \n
    No ", types, " found
    \n
    \n"});
-}
-#endif
-
 } // namespace Server
 } // namespace Envoy
diff --git a/source/server/admin/stats_render.h b/source/server/admin/stats_render.h
index 7fc0b010ad98e..477c3eb6695e4 100644
--- a/source/server/admin/stats_render.h
+++ b/source/server/admin/stats_render.h
@@ -4,10 +4,6 @@
 #include "envoy/stats/stats.h"
 
 #include "source/common/buffer/buffer_impl.h"
-
-#ifdef ENVOY_ADMIN_HTML
-#include "source/server/admin/admin_html_generator.h"
-#endif
 #include "source/server/admin/stats_params.h"
 #include "source/server/admin/utils.h"
 
@@ -99,28 +95,5 @@ class StatsJsonRender : public StatsRender {
   std::string value_buffer_; // Used for Json::sanitize for text-readout values.
 };
 
-#ifdef ENVOY_ADMIN_HTML
-class StatsHtmlRender : public StatsTextRender {
-public:
-  StatsHtmlRender(Http::ResponseHeaderMap& response_headers, Buffer::Instance& response,
-                  const Admin::UrlHandler& url_handler, const StatsParams& params);
-
-  void noStats(Buffer::Instance&, absl::string_view types) override;
-  void finalize(Buffer::Instance&) override;
-  void generate(Buffer::Instance& response, const std::string& name,
-                const std::string& value) override;
-  void generate(Buffer::Instance& response, const std::string& name, uint64_t value) override {
-    StatsTextRender::generate(response, name, value);
-  }
-  void generate(Buffer::Instance& response, const std::string& name,
-                const Stats::ParentHistogram& histogram) override {
-    StatsTextRender::generate(response, name, histogram);
-  }
-
-private:
-  AdminHtmlGenerator html_;
-};
-#endif
-
 } // namespace Server
 } // namespace Envoy
diff --git a/source/server/admin/stats_request.cc b/source/server/admin/stats_request.cc
index 2797a1aaa1b24..89265cd014902 100644
--- a/source/server/admin/stats_request.cc
+++ b/source/server/admin/stats_request.cc
@@ -1,5 +1,9 @@
 #include "source/server/admin/stats_request.h"
 
+#ifdef ENVOY_ADMIN_HTML
+#include "source/server/admin/stats_html_render.h"
+#endif
+
 namespace Envoy {
 namespace Server {
 
diff --git a/test/server/admin/stats_render_test.cc b/test/server/admin/stats_render_test.cc
index ad4b58d2b8499..fa196afc066c7 100644
--- a/test/server/admin/stats_render_test.cc
+++ b/test/server/admin/stats_render_test.cc
@@ -9,6 +9,10 @@
 #include "test/mocks/thread_local/mocks.h"
 #include "test/test_common/utility.h"
 
+#ifdef ENVOY_ADMIN_HTML
+#include "source/server/admin/stats_html_render.h"
+#endif
+
 #include "gtest/gtest.h"
 
 using testing::HasSubstr;

From 93fe3f34dd305ae8ca00865874285a0f65260f70 Mon Sep 17 00:00:00 2001
From: Joshua Marantz 
Date: Mon, 11 Jul 2022 23:36:25 -0400
Subject: [PATCH 49/61] format

Signed-off-by: Joshua Marantz 
---
 source/server/admin/stats_html_render.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/source/server/admin/stats_html_render.h b/source/server/admin/stats_html_render.h
index 87daf50f7ed72..978f8f2e825b6 100644
--- a/source/server/admin/stats_html_render.h
+++ b/source/server/admin/stats_html_render.h
@@ -1,8 +1,7 @@
 #pragma once
 
-#include "source/server/admin/stats_render.h"
-
 #include "source/server/admin/admin_html_generator.h"
+#include "source/server/admin/stats_render.h"
 
 namespace Envoy {
 namespace Server {

From 85cde04280a4391f51c39416c83055ec025ee02e Mon Sep 17 00:00:00 2001
From: Joshua Marantz 
Date: Tue, 12 Jul 2022 13:02:32 -0400
Subject: [PATCH 50/61] refactor to remove extra html implementation files

Signed-off-by: Joshua Marantz 
---
 source/server/admin/BUILD                     |  17 +-
 source/server/admin/admin_html.cc             |   6 +-
 source/server/admin/admin_html_generator.cc   | 161 -----------------
 source/server/admin/admin_html_generator.h    |  67 -------
 source/server/admin/stats_html_render.cc      | 165 +++++++++++++++++-
 source/server/admin/stats_html_render.h       |  50 +++++-
 source/server/admin/stats_request.cc          |  12 +-
 test/server/admin/BUILD                       |  25 +--
 .../server/admin/admin_html_generator_test.cc |  84 ---------
 test/server/admin/stats_html_render_test.cc   |  92 ++++++++++
 test/server/admin/stats_render_test.cc        |  81 +--------
 test/server/admin/stats_render_test_base.cc   |  28 +++
 test/server/admin/stats_render_test_base.h    |  44 +++++
 13 files changed, 395 insertions(+), 437 deletions(-)
 delete mode 100644 source/server/admin/admin_html_generator.cc
 delete mode 100644 source/server/admin/admin_html_generator.h
 delete mode 100644 test/server/admin/admin_html_generator_test.cc
 create mode 100644 test/server/admin/stats_html_render_test.cc
 create mode 100644 test/server/admin/stats_render_test_base.cc
 create mode 100644 test/server/admin/stats_render_test_base.h

diff --git a/source/server/admin/BUILD b/source/server/admin/BUILD
index 28718837e1c06..c9e988f6b8a0b 100644
--- a/source/server/admin/BUILD
+++ b/source/server/admin/BUILD
@@ -101,21 +101,6 @@ genrule(
     visibility = ["//visibility:private"],
 )
 
-envoy_cc_library(
-    name = "admin_html_generator_lib",
-    srcs = [
-        "admin_html_generator.cc",
-        ":generate_admin_html",
-    ],
-    hdrs = ["admin_html_generator.h"],
-    deps = [
-        "//envoy/server:admin_interface",
-        "//source/common/buffer:buffer_lib",
-        "//source/common/common:assert_lib",
-        "//source/common/html:utility_lib",
-    ],
-)
-
 envoy_cc_library(
     name = "handler_ctx_lib",
     hdrs = ["handler_ctx.h"],
@@ -158,6 +143,7 @@ envoy_cc_library(
 envoy_cc_library(
     name = "stats_render_lib",
     srcs = ["stats_render.cc"] + envoy_select_admin_html([
+        ":generate_admin_html",
         "stats_html_render.cc",
     ]),
     hdrs = ["stats_render.h"] + envoy_select_admin_html([
@@ -171,7 +157,6 @@ envoy_cc_library(
         "//source/common/json:json_sanitizer_lib",
         "//source/common/stats:histogram_lib",
     ] + envoy_select_admin_html([
-        ":admin_html_generator_lib",
         "//source/common/html:utility_lib",
     ]),
 )
diff --git a/source/server/admin/admin_html.cc b/source/server/admin/admin_html.cc
index c91aa864ae8e4..4f59922909645 100644
--- a/source/server/admin/admin_html.cc
+++ b/source/server/admin/admin_html.cc
@@ -1,15 +1,13 @@
 #include "source/common/html/utility.h"
 #include "source/server/admin/admin.h"
-#include "source/server/admin/admin_html_generator.h"
+#include "source/server/admin/stats_html_render.h"
 
 namespace Envoy {
 namespace Server {
 
 Http::Code AdminImpl::handlerAdminHome(absl::string_view, Http::ResponseHeaderMap& response_headers,
                                        Buffer::Instance& response, AdminStream&) {
-  response_headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Html);
-  AdminHtmlGenerator html(response);
-  html.renderHead();
+  StatsHtmlRender html(response_headers, response, StatsParams());
   html.renderTableBegin();
 
   // Prefix order is used during searching, but for printing do them in alpha order.
diff --git a/source/server/admin/admin_html_generator.cc b/source/server/admin/admin_html_generator.cc
deleted file mode 100644
index 335e684000fb7..0000000000000
--- a/source/server/admin/admin_html_generator.cc
+++ /dev/null
@@ -1,161 +0,0 @@
-#include "source/server/admin/admin_html_generator.h"
-
-#include 
-
-#include "source/common/buffer/buffer_impl.h"
-#include "source/common/common/assert.h"
-#include "source/common/html/utility.h"
-#include "source/server/admin/admin_html_gen.h"
-
-#include "absl/strings/str_replace.h"
-
-namespace {
-
-/**
- * Favicon base64 image was harvested by screen-capturing the favicon from a Chrome tab
- * while visiting https://www.envoyproxy.io/. The resulting PNG was translated to base64
- * by dropping it into https://www.base64-image.de/ and then pasting the resulting string
- * below.
- *
- * The actual favicon source for that, https://www.envoyproxy.io/img/favicon.ico is nicer
- * because it's transparent, but is also 67646 bytes, which is annoying to inline. We could
- * just reference that rather than inlining it, but then the favicon won't work when visiting
- * the admin page from a network that can't see the internet.
- */
-const char EnvoyFavicon[] =
-    "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABgAAAAYCAYAAADgdz34AAAAAXNSR0IArs4c6QAAAARnQU1"
-    "BAACxjwv8YQUAAAAJcEhZcwAAEnQAABJ0Ad5mH3gAAAH9SURBVEhL7ZRdTttAFIUrUFaAX5w9gIhgUfzshFRK+gIbaVbA"
-    "zwaqCly1dSpKk5A485/YCdXpHTB4BsdgVe0bD0cZ3Xsm38yZ8byTUuJ/6g3wqqoBrBhPTzmmLfptMbAzttJTpTKAF2MWC"
-    "7ADCdNIwXZpvMMwayiIwwS874CcOc9VuQPR1dBBChPMITpFXXU45hukIIH6kHhzVqkEYB8F5HYGvZ5B7EvwmHt9K/59Cr"
-    "U3QbY2RNYaQPYmJc+jPIBICNCcg20ZsAsCPfbcrFlRF+cJZpvXSJt9yMTxO/IAzJrCOfhJXiOgFEX/SbZmezTWxyNk4Q9"
-    "anHMmjnzAhEyhAW8LCE6wl26J7ZFHH1FMYQxh567weQBOO1AW8D7P/UXAQySq/QvL8Fu9HfCEw4SKALm5BkC3bwjwhSKr"
-    "A5hYAMXTJnPNiMyRBVzVjcgCyHiSm+8P+WGlnmwtP2RzbCMiQJ0d2KtmmmPorRHEhfMROVfTG5/fYrF5iWXzE80tfy9WP"
-    "sCqx5Buj7FYH0LvDyHiqd+3otpsr4/fa5+xbEVQPfrYnntylQG5VGeMLBhgEfyE7o6e6qYzwHIjwl0QwXSvvTmrVAY4D5"
-    "ddvT64wV0jRrr7FekO/XEjwuwwhuw7Ef7NY+dlfXpLb06EtHUJdVbsxvNUqBrwj/QGeEUSfwBAkmWHn5Bb/gAAAABJRU5";
-
-const char AdminHtmlTableBegin[] = R"(
-  
-    
-      
-      
-    
-    
-)";
-
-const char AdminHtmlTableEnd[] = R"(
-    
-  
    CommandDescription
    
-)";
-
-} // namespace
-
-namespace Envoy {
-namespace Server {
-
-void AdminHtmlGenerator::renderHead() {
-  response_.add(absl::StrReplaceAll(AdminHtmlStart, {{"@FAVICON@", EnvoyFavicon}}));
-}
-
-void AdminHtmlGenerator::renderTableBegin() { response_.add(AdminHtmlTableBegin); }
-
-void AdminHtmlGenerator::renderTableEnd() { response_.add(AdminHtmlTableEnd); }
-
-void AdminHtmlGenerator::renderUrlHandler(const Admin::UrlHandler& handler,
-                                          OptRef query) {
-  absl::string_view path = handler.prefix_;
-
-  if (path == "/") {
-    return; // No need to print self-link to index page.
-  }
-
-  // Remove the leading slash from the link, so that the admin page can be
-  // rendered as part of another console, on a sub-path.
-  //
-  // E.g. consider a downstream dashboard that embeds the Envoy admin console.
-  // In that case, the "/stats" endpoint would be at
-  // https://DASHBOARD/envoy_admin/stats. If the links we present on the home
-  // page are absolute (e.g. "/stats") they won't work in the context of the
-  // dashboard. Removing the leading slash, they will work properly in both
-  // the raw admin console and when embedded in another page and URL
-  // hierarchy.
-  ASSERT(!path.empty());
-  ASSERT(path[0] == '/');
-  std::string sanitized_path = Html::Utility::sanitize(path.substr(1));
-  path = sanitized_path;
-
-  // Alternate gray and white param-blocks. The pure CSS way of coloring based
-  // on row index doesn't work correctly for us we are using a row for each
-  // parameter, and we want each endpoint/option-block to be colored the same.
-  const char* row_class = (++index_ & 1) ? " class='gray'" : "";
-
-  // For handlers that mutate state, render the link as a button in a POST form,
-  // rather than an anchor tag. This should discourage crawlers that find the /
-  // page from accidentally mutating all the server state by GETting all the hrefs.
-  const char* method = handler.mutates_server_state_ ? "post" : "get";
-  if (submit_on_change_) {
-    response_.addFragments({"\n
    \n"});
-  } else {
-    // Render an explicit visible submit as a link (for GET) or button (for POST).
-    const char* button_style = handler.mutates_server_state_ ? "" : " class='button-as-link'";
-    response_.addFragments({"\n\n\n  
    \n    ",
-                            path, "\n  \n  ",
-                            Html::Utility::sanitize(handler.help_text_), "\n\n"});
-  }
-
-  for (const Admin::ParamDescriptor& param : handler.params_) {
-    response_.addFragments({"\n  "});
-    renderInput(param.id_, path, param.type_, query, param.enum_choices_);
-    response_.addFragments({"\n  ", Html::Utility::sanitize(param.help_),
-                            "\n\n"});
-  }
-}
-
-void AdminHtmlGenerator::renderInput(absl::string_view id, absl::string_view path,
-                                     Admin::ParamDescriptor::Type type,
-                                     OptRef query,
-                                     const std::vector& enum_choices) {
-  std::string value;
-  if (query.has_value()) {
-    auto iter = query->find(std::string(id));
-    if (iter != query->end()) {
-      value = iter->second;
-    }
-  }
-
-  std::string on_change;
-  if (submit_on_change_) {
-    on_change = absl::StrCat(" onchange='", path, ".submit()'");
-  }
-
-  auto value_tag = [](const std::string& value) -> std::string {
-    return value.empty() ? "" : absl::StrCat(" value=", Html::Utility::sanitize(value));
-  };
-
-  switch (type) {
-  case Admin::ParamDescriptor::Type::Boolean:
-    response_.addFragments({""});
-    break;
-  case Admin::ParamDescriptor::Type::String:
-    response_.addFragments({""});
-    break;
-  case Admin::ParamDescriptor::Type::Enum:
-    response_.addFragments(
-        {"\n    \n  ");
-    break;
-  }
-}
-
-} // namespace Server
-} // namespace Envoy
diff --git a/source/server/admin/admin_html_generator.h b/source/server/admin/admin_html_generator.h
deleted file mode 100644
index c4d4199615691..0000000000000
--- a/source/server/admin/admin_html_generator.h
+++ /dev/null
@@ -1,67 +0,0 @@
-#pragma once
-
-#include "envoy/common/optref.h"
-#include "envoy/server/admin.h"
-
-#include "source/common/buffer/buffer_impl.h"
-
-#include "absl/strings/str_replace.h"
-
-namespace Envoy {
-namespace Server {
-
-class AdminHtmlGenerator {
-public:
-  AdminHtmlGenerator(Buffer::Instance& response) : response_(response) {}
-
-  /**
-   * Renders the HTML head into the response buffer provided in the constructor.
-   */
-  void renderHead();
-
-  /**
-   * Renders the beginning of the help-table into the response buffer provided
-   * in the constructor.
-   */
-  void renderTableBegin();
-
-  /**
-   * Renders the end of the help-table into the response buffer provided in the
-   * constructor.
-   */
-  void renderTableEnd();
-
-  /**
-   * Renders a table row for a URL endpoint, including the name of the endpoint,
-   * entries for each parameter, and help text.
-   *
-   * This must be called after renderTableBegin and before renderTableEnd. Any
-   * number of URL Handlers can be rendered.
-   *
-   * @param handler the URL handler.
-   */
-  void renderUrlHandler(const Admin::UrlHandler& handler,
-                        OptRef query);
-
-  void renderInput(absl::string_view id, absl::string_view path, Admin::ParamDescriptor::Type type,
-                   OptRef query,
-                   const std::vector& enum_choices);
-
-  // By default, editing parameters does not cause a form-submit -- you have
-  // to click on the link or button first. This is useful for the admin home
-  // page which lays out all the parameters so users can tweak them before submitting.
-  //
-  // Calling setSubmitOnChange(true) makes the form auto-submits when any
-  // parameters change, and does not have its own explicit submit button. This
-  // is used to enable the user to adjust query-parameters while visiting an
-  // html-rendered endpoint.
-  void setSubmitOnChange(bool submit_on_change) { submit_on_change_ = submit_on_change; }
-
-private:
-  Buffer::Instance& response_;
-  int index_{0};
-  bool submit_on_change_{false};
-};
-
-} // namespace Server
-} // namespace Envoy
diff --git a/source/server/admin/stats_html_render.cc b/source/server/admin/stats_html_render.cc
index 86aa3e5c4317e..4f5e74c0caa19 100644
--- a/source/server/admin/stats_html_render.cc
+++ b/source/server/admin/stats_html_render.cc
@@ -1,22 +1,64 @@
 #include "source/server/admin/stats_html_render.h"
 
+#include 
+
+#include "source/common/buffer/buffer_impl.h"
+#include "source/common/common/assert.h"
 #include "source/common/html/utility.h"
+#include "source/server/admin/admin_html_gen.h"
+
+#include "absl/strings/str_replace.h"
+
+namespace {
+
+/**
+ * Favicon base64 image was harvested by screen-capturing the favicon from a Chrome tab
+ * while visiting https://www.envoyproxy.io/. The resulting PNG was translated to base64
+ * by dropping it into https://www.base64-image.de/ and then pasting the resulting string
+ * below.
+ *
+ * The actual favicon source for that, https://www.envoyproxy.io/img/favicon.ico is nicer
+ * because it's transparent, but is also 67646 bytes, which is annoying to inline. We could
+ * just reference that rather than inlining it, but then the favicon won't work when visiting
+ * the admin page from a network that can't see the internet.
+ */
+const char EnvoyFavicon[] =
+    "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABgAAAAYCAYAAADgdz34AAAAAXNSR0IArs4c6QAAAARnQU1"
+    "BAACxjwv8YQUAAAAJcEhZcwAAEnQAABJ0Ad5mH3gAAAH9SURBVEhL7ZRdTttAFIUrUFaAX5w9gIhgUfzshFRK+gIbaVbA"
+    "zwaqCly1dSpKk5A485/YCdXpHTB4BsdgVe0bD0cZ3Xsm38yZ8byTUuJ/6g3wqqoBrBhPTzmmLfptMbAzttJTpTKAF2MWC"
+    "7ADCdNIwXZpvMMwayiIwwS874CcOc9VuQPR1dBBChPMITpFXXU45hukIIH6kHhzVqkEYB8F5HYGvZ5B7EvwmHt9K/59Cr"
+    "U3QbY2RNYaQPYmJc+jPIBICNCcg20ZsAsCPfbcrFlRF+cJZpvXSJt9yMTxO/IAzJrCOfhJXiOgFEX/SbZmezTWxyNk4Q9"
+    "anHMmjnzAhEyhAW8LCE6wl26J7ZFHH1FMYQxh567weQBOO1AW8D7P/UXAQySq/QvL8Fu9HfCEw4SKALm5BkC3bwjwhSKr"
+    "A5hYAMXTJnPNiMyRBVzVjcgCyHiSm+8P+WGlnmwtP2RzbCMiQJ0d2KtmmmPorRHEhfMROVfTG5/fYrF5iWXzE80tfy9WP"
+    "sCqx5Buj7FYH0LvDyHiqd+3otpsr4/fa5+xbEVQPfrYnntylQG5VGeMLBhgEfyE7o6e6qYzwHIjwl0QwXSvvTmrVAY4D5"
+    "ddvT64wV0jRrr7FekO/XEjwuwwhuw7Ef7NY+dlfXpLb06EtHUJdVbsxvNUqBrwj/QGeEUSfwBAkmWHn5Bb/gAAAABJRU5";
+
+const char AdminHtmlTableBegin[] = R"(
+  
+    
+      
+      
+    
+    
+)";
+
+const char AdminHtmlTableEnd[] = R"(
+    
+  
    CommandDescription
    
+)";
+
+} // namespace
 
 namespace Envoy {
 namespace Server {
 
 StatsHtmlRender::StatsHtmlRender(Http::ResponseHeaderMap& response_headers,
-                                 Buffer::Instance& response, const Admin::UrlHandler& url_handler,
-                                 const StatsParams& params)
-    : StatsTextRender(params), html_(response) {
+                                 Buffer::Instance& response, const StatsParams& params)
+    : StatsTextRender(params), response_(response) {
   response_headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Html);
-  html_.setSubmitOnChange(true);
-  html_.renderHead();
+  renderHead();
   response.add("\n");
-  html_.renderTableBegin();
-  html_.renderUrlHandler(url_handler, params.query_);
-  html_.renderTableEnd();
-  response.add("
    \n");
+  renderTableBegin();
 }
 
 void StatsHtmlRender::generate(Buffer::Instance& response, const std::string& name,
@@ -30,5 +72,110 @@ void StatsHtmlRender::noStats(Buffer::Instance& response, absl::string_view type
   response.addFragments({"
    \n
    No ", types, " found
    \n
    \n"});
 }
 
+void StatsHtmlRender::renderHead() {
+  response_.add(absl::StrReplaceAll(AdminHtmlStart, {{"@FAVICON@", EnvoyFavicon}}));
+}
+
+void StatsHtmlRender::renderTableBegin() { response_.add(AdminHtmlTableBegin); }
+
+void StatsHtmlRender::renderTableEnd() { response_.add(AdminHtmlTableEnd); }
+
+void StatsHtmlRender::renderUrlHandler(const Admin::UrlHandler& handler,
+                                       OptRef query) {
+  absl::string_view path = handler.prefix_;
+
+  if (path == "/") {
+    return; // No need to print self-link to index page.
+  }
+
+  // Remove the leading slash from the link, so that the admin page can be
+  // rendered as part of another console, on a sub-path.
+  //
+  // E.g. consider a downstream dashboard that embeds the Envoy admin console.
+  // In that case, the "/stats" endpoint would be at
+  // https://DASHBOARD/envoy_admin/stats. If the links we present on the home
+  // page are absolute (e.g. "/stats") they won't work in the context of the
+  // dashboard. Removing the leading slash, they will work properly in both
+  // the raw admin console and when embedded in another page and URL
+  // hierarchy.
+  ASSERT(!path.empty());
+  ASSERT(path[0] == '/');
+  std::string sanitized_path = Html::Utility::sanitize(path.substr(1));
+  path = sanitized_path;
+
+  // Alternate gray and white param-blocks. The pure CSS way of coloring based
+  // on row index doesn't work correctly for us we are using a row for each
+  // parameter, and we want each endpoint/option-block to be colored the same.
+  const char* row_class = (++index_ & 1) ? " class='gray'" : "";
+
+  // For handlers that mutate state, render the link as a button in a POST form,
+  // rather than an anchor tag. This should discourage crawlers that find the /
+  // page from accidentally mutating all the server state by GETting all the hrefs.
+  const char* method = handler.mutates_server_state_ ? "post" : "get";
+  if (submit_on_change_) {
+    response_.addFragments({"\n
    \n"});
+  } else {
+    // Render an explicit visible submit as a link (for GET) or button (for POST).
+    const char* button_style = handler.mutates_server_state_ ? "" : " class='button-as-link'";
+    response_.addFragments({"\n\n\n  
    \n    ",
+                            path, "\n  \n  ",
+                            Html::Utility::sanitize(handler.help_text_), "\n\n"});
+  }
+
+  for (const Admin::ParamDescriptor& param : handler.params_) {
+    response_.addFragments({"\n  "});
+    renderInput(param.id_, path, param.type_, query, param.enum_choices_);
+    response_.addFragments({"\n  ", Html::Utility::sanitize(param.help_),
+                            "\n\n"});
+  }
+}
+
+void StatsHtmlRender::renderInput(absl::string_view id, absl::string_view path,
+                                  Admin::ParamDescriptor::Type type,
+                                  OptRef query,
+                                  const std::vector& enum_choices) {
+  std::string value;
+  if (query.has_value()) {
+    auto iter = query->find(std::string(id));
+    if (iter != query->end()) {
+      value = iter->second;
+    }
+  }
+
+  std::string on_change;
+  if (submit_on_change_) {
+    on_change = absl::StrCat(" onchange='", path, ".submit()'");
+  }
+
+  auto value_tag = [](const std::string& value) -> std::string {
+    return value.empty() ? "" : absl::StrCat(" value=", Html::Utility::sanitize(value));
+  };
+
+  switch (type) {
+  case Admin::ParamDescriptor::Type::Boolean:
+    response_.addFragments({""});
+    break;
+  case Admin::ParamDescriptor::Type::String:
+    response_.addFragments({""});
+    break;
+  case Admin::ParamDescriptor::Type::Enum:
+    response_.addFragments(
+        {"\n    \n  ");
+    break;
+  }
+}
+
 } // namespace Server
 } // namespace Envoy
diff --git a/source/server/admin/stats_html_render.h b/source/server/admin/stats_html_render.h
index 978f8f2e825b6..f6150957aabb0 100644
--- a/source/server/admin/stats_html_render.h
+++ b/source/server/admin/stats_html_render.h
@@ -1,6 +1,5 @@
 #pragma once
 
-#include "source/server/admin/admin_html_generator.h"
 #include "source/server/admin/stats_render.h"
 
 namespace Envoy {
@@ -9,7 +8,7 @@ namespace Server {
 class StatsHtmlRender : public StatsTextRender {
 public:
   StatsHtmlRender(Http::ResponseHeaderMap& response_headers, Buffer::Instance& response,
-                  const Admin::UrlHandler& url_handler, const StatsParams& params);
+                  const StatsParams& params);
 
   void noStats(Buffer::Instance&, absl::string_view types) override;
   void finalize(Buffer::Instance&) override;
@@ -23,8 +22,53 @@ class StatsHtmlRender : public StatsTextRender {
     StatsTextRender::generate(response, name, histogram);
   }
 
+  /**
+   * Renders the HTML head into the response buffer provided in the constructor.
+   */
+  void renderHead();
+
+  /**
+   * Renders the beginning of the help-table into the response buffer provided
+   * in the constructor.
+   */
+  void renderTableBegin();
+
+  /**
+   * Renders the end of the help-table into the response buffer provided in the
+   * constructor.
+   */
+  void renderTableEnd();
+
+  /**
+   * Renders a table row for a URL endpoint, including the name of the endpoint,
+   * entries for each parameter, and help text.
+   *
+   * This must be called after renderTableBegin and before renderTableEnd. Any
+   * number of URL Handlers can be rendered.
+   *
+   * @param handler the URL handler.
+   */
+  void renderUrlHandler(const Admin::UrlHandler& handler,
+                        OptRef query);
+
+  void renderInput(absl::string_view id, absl::string_view path, Admin::ParamDescriptor::Type type,
+                   OptRef query,
+                   const std::vector& enum_choices);
+
+  // By default, editing parameters does not cause a form-submit -- you have
+  // to click on the link or button first. This is useful for the admin home
+  // page which lays out all the parameters so users can tweak them before submitting.
+  //
+  // Calling setSubmitOnChange(true) makes the form auto-submits when any
+  // parameters change, and does not have its own explicit submit button. This
+  // is used to enable the user to adjust query-parameters while visiting an
+  // html-rendered endpoint.
+  void setSubmitOnChange(bool submit_on_change) { submit_on_change_ = submit_on_change; }
+
 private:
-  AdminHtmlGenerator html_;
+  Buffer::Instance& response_;
+  int index_{0}; // Used to alternate row-group background color
+  bool submit_on_change_{false};
 };
 
 } // namespace Server
diff --git a/source/server/admin/stats_request.cc b/source/server/admin/stats_request.cc
index 89265cd014902..057c435dd0e12 100644
--- a/source/server/admin/stats_request.cc
+++ b/source/server/admin/stats_request.cc
@@ -34,10 +34,16 @@ Http::Code StatsRequest::start(Http::ResponseHeaderMap& response_headers) {
     render_ = std::make_unique(params_);
     break;
 #ifdef ENVOY_ADMIN_HTML
-  case StatsFormat::Html:
-    render_ =
-        std::make_unique(response_headers, response_, url_handler_fn_(), params_);
+  case StatsFormat::Html: {
+    auto html_render = std::make_unique(response_headers, response_, params_);
+    html_render->setSubmitOnChange(true);
+    html_render->renderTableBegin();
+    html_render->renderUrlHandler(url_handler_fn_(), params_.query_);
+    html_render->renderTableEnd();
+    response_.add("
    \n");
+    render_.reset(html_render.release());
     break;
+  }
 #endif
   case StatsFormat::Prometheus:
     // TODO(#16139): once Prometheus shares this algorithm here, this becomes a legitimate choice.
diff --git a/test/server/admin/BUILD b/test/server/admin/BUILD
index 2b632d05e4e6f..35684d1b00382 100644
--- a/test/server/admin/BUILD
+++ b/test/server/admin/BUILD
@@ -47,15 +47,6 @@ envoy_cc_test(
     ],
 )
 
-envoy_cc_test(
-    name = "admin_html_generator_test",
-    srcs = ["admin_html_generator_test.cc"],
-    deps = [
-        "//source/server/admin:admin_html_generator_lib",
-        "//source/server/admin:admin_lib",
-    ],
-)
-
 envoy_cc_test(
     name = "admin_filter_test",
     srcs = ["admin_filter_test.cc"],
@@ -106,10 +97,22 @@ envoy_cc_test(
 
 envoy_cc_test(
     name = "stats_render_test",
-    srcs = ["stats_render_test.cc"],
+    srcs = ["stats_render_test.cc"] + envoy_select_admin_html([
+        "stats_html_render_test.cc",
+    ]),
+    deps = [
+        ":stats_render_test_base",
+        "//source/server/admin:admin_lib",
+    ],
+)
+
+envoy_cc_test_library(
+    name = "stats_render_test_base",
+    srcs = ["stats_render_test_base.cc"],
+    hdrs = ["stats_render_test_base.h"],
     deps = [
         "//source/common/stats:thread_local_store_lib",
-        "//source/server/admin:stats_render_lib",
+        "//source/server/admin:stats_request_lib",
         "//test/mocks/event:event_mocks",
         "//test/mocks/stats:stats_mocks",
         "//test/mocks/thread_local:thread_local_mocks",
diff --git a/test/server/admin/admin_html_generator_test.cc b/test/server/admin/admin_html_generator_test.cc
deleted file mode 100644
index 6e6914c11cdf8..0000000000000
--- a/test/server/admin/admin_html_generator_test.cc
+++ /dev/null
@@ -1,84 +0,0 @@
-#include "source/server/admin/admin.h"
-#include "source/server/admin/admin_html_generator.h"
-
-#include "gmock/gmock.h"
-#include "gtest/gtest.h"
-
-using testing::HasSubstr;
-using testing::Not;
-
-namespace Envoy {
-namespace Server {
-
-class AdminHtmlGeneratorTest : public testing::Test {
-protected:
-  AdminHtmlGeneratorTest()
-      : generator_(data_), handler_{
-                               "/prefix",
-                               "help text",
-                               handlerCallback,
-                               false,
-                               false,
-                               {{Admin::ParamDescriptor::Type::Boolean, "param", "param help"}}} {}
-
-  static Admin::RequestPtr handlerCallback(absl::string_view, AdminStream&) {
-    return Admin::makeStaticTextRequest("", Http::Code::OK);
-  }
-
-  Buffer::OwnedImpl data_;
-  AdminHtmlGenerator generator_;
-  Admin::UrlHandler handler_;
-  Http::Utility::QueryParams query_params_;
-};
-
-TEST_F(AdminHtmlGeneratorTest, RenderHead) {
-  generator_.renderHead();
-  EXPECT_THAT(data_.toString(), HasSubstr(""));
-}
-
-TEST_F(AdminHtmlGeneratorTest, RenderTableBegin) {
-  generator_.renderTableBegin();
-  EXPECT_THAT(data_.toString(), HasSubstr(""));
-}
-
-TEST_F(AdminHtmlGeneratorTest, RenderTableEnd) {
-  generator_.renderTableEnd();
-  EXPECT_THAT(data_.toString(), HasSubstr("
    "));
-}
-
-TEST_F(AdminHtmlGeneratorTest, RenderUrlHandlerNoQuery) {
-  generator_.renderUrlHandler(handler_, query_params_);
-  std::string out = data_.toString();
-  EXPECT_THAT(out, HasSubstr("")));
-  EXPECT_THAT(out, HasSubstr("help text"));
-  EXPECT_THAT(out, HasSubstr("param help"));
-  EXPECT_THAT(out, HasSubstr(""));
-  EXPECT_THAT(out, Not(HasSubstr(" onchange='prefix.submit()")));
-  EXPECT_THAT(out, Not(HasSubstr(" type='hidden' ")));
-}
-
-TEST_F(AdminHtmlGeneratorTest, RenderUrlHandlerWithQuery) {
-  query_params_["param"] = "on";
-  generator_.renderUrlHandler(handler_, query_params_);
-  std::string out = data_.toString();
-  EXPECT_THAT(out, HasSubstr(""));
-  EXPECT_THAT(out, HasSubstr("help text"));
-  EXPECT_THAT(out, HasSubstr("param help"));
-  EXPECT_THAT(out, HasSubstr(""));
-  EXPECT_THAT(out, Not(HasSubstr(" onchange='prefix.submit()")));
-  EXPECT_THAT(out, Not(HasSubstr(" type='hidden' ")));
-}
-
-TEST_F(AdminHtmlGeneratorTest, RenderUrlHandlerSubmitOnChange) {
-  generator_.setSubmitOnChange(true);
-  generator_.renderUrlHandler(handler_, query_params_);
-  std::string out = data_.toString();
-  EXPECT_THAT(out, HasSubstr(" onchange='prefix.submit()"));
-  EXPECT_THAT(out, Not(HasSubstr("")));
-  EXPECT_THAT(out, Not(HasSubstr(" type='hidden' ")));
-}
-
-} // namespace Server
-} // namespace Envoy
diff --git a/test/server/admin/stats_html_render_test.cc b/test/server/admin/stats_html_render_test.cc
new file mode 100644
index 0000000000000..8e7d924ffe8a9
--- /dev/null
+++ b/test/server/admin/stats_html_render_test.cc
@@ -0,0 +1,92 @@
+#include "source/server/admin/admin.h"
+#include "source/server/admin/stats_html_render.h"
+
+#include "test/server/admin/stats_render_test_base.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using testing::HasSubstr;
+using testing::Not;
+
+namespace Envoy {
+namespace Server {
+
+class StatsHtmlRenderTest : public StatsRenderTestBase {
+protected:
+  static Admin::RequestPtr handlerCallback(absl::string_view, AdminStream&) {
+    return Admin::makeStaticTextRequest("", Http::Code::OK);
+  }
+
+  const Admin::UrlHandler handler_{
+      "/prefix", "help text", handlerCallback,
+      false,     false,       {{Admin::ParamDescriptor::Type::Boolean, "param", "param help"}}};
+  StatsHtmlRender renderer_{response_headers_, response_, params_};
+  Http::Utility::QueryParams query_params_;
+};
+
+TEST_F(StatsHtmlRenderTest, String) {
+  EXPECT_THAT(render(renderer_, "name", "abc 123 ~!@#$%^&*()-_=+;:'\",<.>/?"),
+              HasSubstr("name: \"abc 123 ~!@#$%^&*()-_=+;:'",<.>/?\"\n"));
+}
+
+TEST_F(StatsHtmlRenderTest, HistogramNoBuckets) {
+  constexpr absl::string_view expected =
+      "h1: P0(200,200) P25(207.5,207.5) P50(302.5,302.5) P75(306.25,306.25) "
+      "P90(308.5,308.5) P95(309.25,309.25) P99(309.85,309.85) P99.5(309.925,309.925) "
+      "P99.9(309.985,309.985) P100(310,310)\n";
+  EXPECT_THAT(render<>(renderer_, "h1", populateHistogram("h1", {200, 300, 300})),
+              HasSubstr(expected));
+}
+
+TEST_F(StatsHtmlRenderTest, RenderHead) {
+  renderer_.renderHead();
+  EXPECT_THAT(response_.toString(), HasSubstr(""));
+}
+
+TEST_F(StatsHtmlRenderTest, RenderTableBegin) {
+  renderer_.renderTableBegin();
+  EXPECT_THAT(response_.toString(), HasSubstr(""));
+}
+
+TEST_F(StatsHtmlRenderTest, RenderTableEnd) {
+  renderer_.renderTableEnd();
+  EXPECT_THAT(response_.toString(), HasSubstr("
    "));
+}
+
+TEST_F(StatsHtmlRenderTest, RenderUrlHandlerNoQuery) {
+  renderer_.renderUrlHandler(handler_, query_params_);
+  std::string out = response_.toString();
+  EXPECT_THAT(out, HasSubstr("")));
+  EXPECT_THAT(out, HasSubstr("help text"));
+  EXPECT_THAT(out, HasSubstr("param help"));
+  EXPECT_THAT(out, HasSubstr(""));
+  EXPECT_THAT(out, Not(HasSubstr(" onchange='prefix.submit()")));
+  EXPECT_THAT(out, Not(HasSubstr(" type='hidden' ")));
+}
+
+TEST_F(StatsHtmlRenderTest, RenderUrlHandlerWithQuery) {
+  query_params_["param"] = "on";
+  renderer_.renderUrlHandler(handler_, query_params_);
+  std::string out = response_.toString();
+  EXPECT_THAT(out, HasSubstr(""));
+  EXPECT_THAT(out, HasSubstr("help text"));
+  EXPECT_THAT(out, HasSubstr("param help"));
+  EXPECT_THAT(out, HasSubstr(""));
+  EXPECT_THAT(out, Not(HasSubstr(" onchange='prefix.submit()")));
+  EXPECT_THAT(out, Not(HasSubstr(" type='hidden' ")));
+}
+
+TEST_F(StatsHtmlRenderTest, RenderUrlHandlerSubmitOnChange) {
+  renderer_.setSubmitOnChange(true);
+  renderer_.renderUrlHandler(handler_, query_params_);
+  std::string out = response_.toString();
+  EXPECT_THAT(out, HasSubstr(" onchange='prefix.submit()"));
+  EXPECT_THAT(out, Not(HasSubstr("")));
+  EXPECT_THAT(out, Not(HasSubstr(" type='hidden' ")));
+}
+
+} // namespace Server
+} // namespace Envoy
diff --git a/test/server/admin/stats_render_test.cc b/test/server/admin/stats_render_test.cc
index fa196afc066c7..da8ee238d7382 100644
--- a/test/server/admin/stats_render_test.cc
+++ b/test/server/admin/stats_render_test.cc
@@ -1,66 +1,13 @@
 #include 
 
-#include "source/common/buffer/buffer_impl.h"
-#include "source/common/stats/thread_local_store.h"
-#include "source/server/admin/stats_render.h"
-
-#include "test/mocks/event/mocks.h"
-#include "test/mocks/stats/mocks.h"
-#include "test/mocks/thread_local/mocks.h"
-#include "test/test_common/utility.h"
-
-#ifdef ENVOY_ADMIN_HTML
-#include "source/server/admin/stats_html_render.h"
-#endif
-
-#include "gtest/gtest.h"
+#include "test/server/admin/stats_render_test_base.h"
 
 using testing::HasSubstr;
-using testing::NiceMock;
 
 namespace Envoy {
 namespace Server {
 
-class StatsRenderTest : public testing::Test {
-protected:
-  StatsRenderTest() : alloc_(symbol_table_), store_(alloc_) {
-    store_.addSink(sink_);
-    store_.initializeThreading(main_thread_dispatcher_, tls_);
-  }
-
-  ~StatsRenderTest() override {
-    tls_.shutdownGlobalThreading();
-    store_.shutdownThreading();
-    tls_.shutdownThread();
-  }
-
-  template 
-  std::string render(StatsRender& render, absl::string_view name, const T& value) {
-    render.generate(response_, std::string(name), value);
-    render.finalize(response_);
-    return response_.toString();
-  }
-
-  Stats::ParentHistogram& populateHistogram(const std::string& name,
-                                            const std::vector& vals) {
-    Stats::Histogram& h = store_.histogramFromString(name, Stats::Histogram::Unit::Unspecified);
-    for (uint64_t val : vals) {
-      h.recordValue(val);
-    }
-    store_.mergeHistograms([]() -> void {});
-    return dynamic_cast(h);
-  }
-
-  Stats::SymbolTableImpl symbol_table_;
-  Stats::AllocatorImpl alloc_;
-  NiceMock sink_;
-  NiceMock main_thread_dispatcher_;
-  NiceMock tls_;
-  Stats::ThreadLocalStoreImpl store_;
-  Http::TestResponseHeaderMapImpl response_headers_;
-  Buffer::OwnedImpl response_;
-  StatsParams params_;
-};
+using StatsRenderTest = StatsRenderTestBase;
 
 TEST_F(StatsRenderTest, TextInt) {
   StatsTextRender renderer(params_);
@@ -352,29 +299,5 @@ TEST_F(StatsRenderTest, JsonHistogramDisjoint) {
               JsonStringEq(expected));
 }
 
-#ifdef ENVOY_ADMIN_HTML
-class StatsHtmlRenderTest : public StatsRenderTest {
-protected:
-  const Admin::UrlHandler url_handler_{
-      "/foo", "help", [](absl::string_view, AdminStream&) -> Admin::RequestPtr { return nullptr; },
-      false, false};
-  StatsHtmlRender renderer_{response_headers_, response_, url_handler_, params_};
-};
-
-TEST_F(StatsHtmlRenderTest, String) {
-  EXPECT_THAT(render(renderer_, "name", "abc 123 ~!@#$%^&*()-_=+;:'\",<.>/?"),
-              HasSubstr("name: \"abc 123 ~!@#$%^&*()-_=+;:'",<.>/?\"\n"));
-}
-
-TEST_F(StatsHtmlRenderTest, HistogramNoBuckets) {
-  constexpr absl::string_view expected =
-      "h1: P0(200,200) P25(207.5,207.5) P50(302.5,302.5) P75(306.25,306.25) "
-      "P90(308.5,308.5) P95(309.25,309.25) P99(309.85,309.85) P99.5(309.925,309.925) "
-      "P99.9(309.985,309.985) P100(310,310)\n";
-  EXPECT_THAT(render<>(renderer_, "h1", populateHistogram("h1", {200, 300, 300})),
-              HasSubstr(expected));
-}
-#endif
-
 } // namespace Server
 } // namespace Envoy
diff --git a/test/server/admin/stats_render_test_base.cc b/test/server/admin/stats_render_test_base.cc
new file mode 100644
index 0000000000000..23923da1e00c6
--- /dev/null
+++ b/test/server/admin/stats_render_test_base.cc
@@ -0,0 +1,28 @@
+#include "test/server/admin/stats_render_test_base.h"
+
+namespace Envoy {
+namespace Server {
+
+StatsRenderTestBase::StatsRenderTestBase() : alloc_(symbol_table_), store_(alloc_) {
+  store_.addSink(sink_);
+  store_.initializeThreading(main_thread_dispatcher_, tls_);
+}
+
+StatsRenderTestBase::~StatsRenderTestBase() {
+  tls_.shutdownGlobalThreading();
+  store_.shutdownThreading();
+  tls_.shutdownThread();
+}
+
+Stats::ParentHistogram& StatsRenderTestBase::populateHistogram(const std::string& name,
+                                                               const std::vector& vals) {
+  Stats::Histogram& h = store_.histogramFromString(name, Stats::Histogram::Unit::Unspecified);
+  for (uint64_t val : vals) {
+    h.recordValue(val);
+  }
+  store_.mergeHistograms([]() -> void {});
+  return dynamic_cast(h);
+}
+
+} // namespace Server
+} // namespace Envoy
diff --git a/test/server/admin/stats_render_test_base.h b/test/server/admin/stats_render_test_base.h
new file mode 100644
index 0000000000000..0c566f677be00
--- /dev/null
+++ b/test/server/admin/stats_render_test_base.h
@@ -0,0 +1,44 @@
+#pragma once
+
+#include "source/common/buffer/buffer_impl.h"
+#include "source/common/stats/thread_local_store.h"
+#include "source/server/admin/stats_render.h"
+
+#include "test/mocks/event/mocks.h"
+#include "test/mocks/stats/mocks.h"
+#include "test/mocks/thread_local/mocks.h"
+#include "test/test_common/utility.h"
+
+#include "gtest/gtest.h"
+
+namespace Envoy {
+namespace Server {
+
+class StatsRenderTestBase : public testing::Test {
+protected:
+  StatsRenderTestBase();
+  ~StatsRenderTestBase() override;
+
+  template 
+  std::string render(StatsRender& render, absl::string_view name, const T& value) {
+    render.generate(response_, std::string(name), value);
+    render.finalize(response_);
+    return response_.toString();
+  }
+
+  Stats::ParentHistogram& populateHistogram(const std::string& name,
+                                            const std::vector& vals);
+
+  Stats::SymbolTableImpl symbol_table_;
+  Stats::AllocatorImpl alloc_;
+  testing::NiceMock sink_;
+  testing::NiceMock main_thread_dispatcher_;
+  testing::NiceMock tls_;
+  Stats::ThreadLocalStoreImpl store_;
+  Http::TestResponseHeaderMapImpl response_headers_;
+  Buffer::OwnedImpl response_;
+  StatsParams params_;
+};
+
+} // namespace Server
+} // namespace Envoy

From b6269173e11d4fbfbb6d82d04bbf0e82cffda37d Mon Sep 17 00:00:00 2001
From: Joshua Marantz 
Date: Tue, 12 Jul 2022 20:11:15 -0400
Subject: [PATCH 51/61] tighten up the logic.

Signed-off-by: Joshua Marantz 
---
 source/server/admin/admin_html.cc           |  7 +-
 source/server/admin/stats_html_render.cc    | 75 +++++++++++----------
 source/server/admin/stats_html_render.h     | 29 ++++----
 source/server/admin/stats_request.cc        |  8 +--
 test/server/admin/stats_html_render_test.cc | 13 ++--
 test/server/admin/stats_render_test.cc      |  2 -
 6 files changed, 72 insertions(+), 62 deletions(-)

diff --git a/source/server/admin/admin_html.cc b/source/server/admin/admin_html.cc
index 4f59922909645..05eb41e6cb52d 100644
--- a/source/server/admin/admin_html.cc
+++ b/source/server/admin/admin_html.cc
@@ -8,15 +8,16 @@ namespace Server {
 Http::Code AdminImpl::handlerAdminHome(absl::string_view, Http::ResponseHeaderMap& response_headers,
                                        Buffer::Instance& response, AdminStream&) {
   StatsHtmlRender html(response_headers, response, StatsParams());
-  html.renderTableBegin();
+  html.tableBegin(response);
 
   // Prefix order is used during searching, but for printing do them in alpha order.
   OptRef no_query_params;
   for (const UrlHandler* handler : sortedHandlers()) {
-    html.renderUrlHandler(*handler, no_query_params);
+    html.urlHandler(response, *handler, no_query_params);
   }
 
-  html.renderTableEnd();
+  html.tableEnd(response);
+  html.finalize(response);
 
   return Http::Code::OK;
 }
diff --git a/source/server/admin/stats_html_render.cc b/source/server/admin/stats_html_render.cc
index 4f5e74c0caa19..189176ba52a0b 100644
--- a/source/server/admin/stats_html_render.cc
+++ b/source/server/admin/stats_html_render.cc
@@ -56,9 +56,22 @@ StatsHtmlRender::StatsHtmlRender(Http::ResponseHeaderMap& response_headers,
                                  Buffer::Instance& response, const StatsParams& params)
     : StatsTextRender(params), response_(response) {
   response_headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Html);
-  renderHead();
+  response_.add(absl::StrReplaceAll(AdminHtmlStart, {{"@FAVICON@", EnvoyFavicon}}));
   response.add("\n");
-  renderTableBegin();
+}
+
+void StatsHtmlRender::finalize(Buffer::Instance& response) {
+  ASSERT(!finalized_);
+  finalized_ = true;
+  if (has_pre_) {
+    response.add("
    \n");
+  }
+  response.add("\n");
+}
+
+void StatsHtmlRender::startPre(Buffer::Instance& response) {
+  has_pre_ = true;
+  response.add("
    \n");
 }
 
 void StatsHtmlRender::generate(Buffer::Instance& response, const std::string& name,
@@ -66,22 +79,16 @@ void StatsHtmlRender::generate(Buffer::Instance& response, const std::string& na
   response.addFragments({name, ": \"", Html::Utility::sanitize(value), "\"\n"});
 }
 
-void StatsHtmlRender::finalize(Buffer::Instance& response) { response.add("
    \n"); }
-
 void StatsHtmlRender::noStats(Buffer::Instance& response, absl::string_view types) {
   response.addFragments({"
    \n
    No ", types, " found
    \n
    \n"});
 }
 
-void StatsHtmlRender::renderHead() {
-  response_.add(absl::StrReplaceAll(AdminHtmlStart, {{"@FAVICON@", EnvoyFavicon}}));
-}
-
-void StatsHtmlRender::renderTableBegin() { response_.add(AdminHtmlTableBegin); }
+void StatsHtmlRender::tableBegin(Buffer::Instance& response) { response.add(AdminHtmlTableBegin); }
 
-void StatsHtmlRender::renderTableEnd() { response_.add(AdminHtmlTableEnd); }
+void StatsHtmlRender::tableEnd(Buffer::Instance& response) { response.add(AdminHtmlTableEnd); }
 
-void StatsHtmlRender::renderUrlHandler(const Admin::UrlHandler& handler,
-                                       OptRef query) {
+void StatsHtmlRender::urlHandler(Buffer::Instance& response, const Admin::UrlHandler& handler,
+                                 OptRef query) {
   absl::string_view path = handler.prefix_;
 
   if (path == "/") {
@@ -113,30 +120,30 @@ void StatsHtmlRender::renderUrlHandler(const Admin::UrlHandler& handler,
   // page from accidentally mutating all the server state by GETting all the hrefs.
   const char* method = handler.mutates_server_state_ ? "post" : "get";
   if (submit_on_change_) {
-    response_.addFragments({"\n
    \n"});
+    response.addFragments({"\n
    \n"});
   } else {
     // Render an explicit visible submit as a link (for GET) or button (for POST).
     const char* button_style = handler.mutates_server_state_ ? "" : " class='button-as-link'";
-    response_.addFragments({"\n\n\n  
    \n    ",
-                            path, "\n  \n  ",
-                            Html::Utility::sanitize(handler.help_text_), "\n\n"});
+    response.addFragments({"\n\n\n  
    \n    ",
+                           path, "\n  \n  ",
+                           Html::Utility::sanitize(handler.help_text_), "\n\n"});
   }
 
   for (const Admin::ParamDescriptor& param : handler.params_) {
-    response_.addFragments({"\n  "});
-    renderInput(param.id_, path, param.type_, query, param.enum_choices_);
-    response_.addFragments({"\n  ", Html::Utility::sanitize(param.help_),
-                            "\n\n"});
+    response.addFragments({"\n  "});
+    input(response, param.id_, path, param.type_, query, param.enum_choices_);
+    response.addFragments({"\n  ", Html::Utility::sanitize(param.help_),
+                           "\n\n"});
   }
 }
 
-void StatsHtmlRender::renderInput(absl::string_view id, absl::string_view path,
-                                  Admin::ParamDescriptor::Type type,
-                                  OptRef query,
-                                  const std::vector& enum_choices) {
+void StatsHtmlRender::input(Buffer::Instance& response, absl::string_view id,
+                            absl::string_view path, Admin::ParamDescriptor::Type type,
+                            OptRef query,
+                            const std::vector& enum_choices) {
   std::string value;
   if (query.has_value()) {
     auto iter = query->find(std::string(id));
@@ -156,23 +163,23 @@ void StatsHtmlRender::renderInput(absl::string_view id, absl::string_view path,
 
   switch (type) {
   case Admin::ParamDescriptor::Type::Boolean:
-    response_.addFragments({""});
+    response.addFragments({""});
     break;
   case Admin::ParamDescriptor::Type::String:
-    response_.addFragments({""});
+    response.addFragments({""});
     break;
   case Admin::ParamDescriptor::Type::Enum:
-    response_.addFragments(
+    response.addFragments(
         {"\n    \n  ");
+    response.add("    \n  ");
     break;
   }
 }
diff --git a/source/server/admin/stats_html_render.h b/source/server/admin/stats_html_render.h
index f6150957aabb0..d929f85f69d74 100644
--- a/source/server/admin/stats_html_render.h
+++ b/source/server/admin/stats_html_render.h
@@ -11,7 +11,6 @@ class StatsHtmlRender : public StatsTextRender {
                   const StatsParams& params);
 
   void noStats(Buffer::Instance&, absl::string_view types) override;
-  void finalize(Buffer::Instance&) override;
   void generate(Buffer::Instance& response, const std::string& name,
                 const std::string& value) override;
   void generate(Buffer::Instance& response, const std::string& name, uint64_t value) override {
@@ -21,23 +20,25 @@ class StatsHtmlRender : public StatsTextRender {
                 const Stats::ParentHistogram& histogram) override {
     StatsTextRender::generate(response, name, histogram);
   }
-
-  /**
-   * Renders the HTML head into the response buffer provided in the constructor.
-   */
-  void renderHead();
+  void finalize(Buffer::Instance&) override;
 
   /**
    * Renders the beginning of the help-table into the response buffer provided
    * in the constructor.
    */
-  void renderTableBegin();
+  void tableBegin(Buffer::Instance&);
 
   /**
    * Renders the end of the help-table into the response buffer provided in the
    * constructor.
    */
-  void renderTableEnd();
+  void tableEnd(Buffer::Instance&);
+
+  /**
+   * Initiates an HTML PRE section. The PRE will be auto-closed when the render
+   * object is finalized.
+   */
+  void startPre(Buffer::Instance&);
 
   /**
    * Renders a table row for a URL endpoint, including the name of the endpoint,
@@ -48,12 +49,12 @@ class StatsHtmlRender : public StatsTextRender {
    *
    * @param handler the URL handler.
    */
-  void renderUrlHandler(const Admin::UrlHandler& handler,
-                        OptRef query);
+  void urlHandler(Buffer::Instance&, const Admin::UrlHandler& handler,
+                  OptRef query);
 
-  void renderInput(absl::string_view id, absl::string_view path, Admin::ParamDescriptor::Type type,
-                   OptRef query,
-                   const std::vector& enum_choices);
+  void input(Buffer::Instance&, absl::string_view id, absl::string_view path,
+             Admin::ParamDescriptor::Type type, OptRef query,
+             const std::vector& enum_choices);
 
   // By default, editing parameters does not cause a form-submit -- you have
   // to click on the link or button first. This is useful for the admin home
@@ -69,6 +70,8 @@ class StatsHtmlRender : public StatsTextRender {
   Buffer::Instance& response_;
   int index_{0}; // Used to alternate row-group background color
   bool submit_on_change_{false};
+  bool has_pre_{false};
+  bool finalized_{false};
 };
 
 } // namespace Server
diff --git a/source/server/admin/stats_request.cc b/source/server/admin/stats_request.cc
index 057c435dd0e12..c2197261e2835 100644
--- a/source/server/admin/stats_request.cc
+++ b/source/server/admin/stats_request.cc
@@ -37,10 +37,10 @@ Http::Code StatsRequest::start(Http::ResponseHeaderMap& response_headers) {
   case StatsFormat::Html: {
     auto html_render = std::make_unique(response_headers, response_, params_);
     html_render->setSubmitOnChange(true);
-    html_render->renderTableBegin();
-    html_render->renderUrlHandler(url_handler_fn_(), params_.query_);
-    html_render->renderTableEnd();
-    response_.add("
    \n");
+    html_render->tableBegin(response_);
+    html_render->urlHandler(response_, url_handler_fn_(), params_.query_);
+    html_render->tableEnd(response_);
+    html_render->startPre(response_);
     render_.reset(html_render.release());
     break;
   }
diff --git a/test/server/admin/stats_html_render_test.cc b/test/server/admin/stats_html_render_test.cc
index 8e7d924ffe8a9..5ca1898651d4c 100644
--- a/test/server/admin/stats_html_render_test.cc
+++ b/test/server/admin/stats_html_render_test.cc
@@ -40,22 +40,23 @@ TEST_F(StatsHtmlRenderTest, HistogramNoBuckets) {
 }
 
 TEST_F(StatsHtmlRenderTest, RenderHead) {
-  renderer_.renderHead();
+  // The "..." is rendered by the constructor.
   EXPECT_THAT(response_.toString(), HasSubstr(""));
+  EXPECT_THAT(response_.toString(), HasSubstr(""));
 }
 
 TEST_F(StatsHtmlRenderTest, RenderTableBegin) {
-  renderer_.renderTableBegin();
+  renderer_.tableBegin(response_);
   EXPECT_THAT(response_.toString(), HasSubstr(""));
 }
 
 TEST_F(StatsHtmlRenderTest, RenderTableEnd) {
-  renderer_.renderTableEnd();
+  renderer_.tableEnd(response_);
   EXPECT_THAT(response_.toString(), HasSubstr("
    "));
 }
 
 TEST_F(StatsHtmlRenderTest, RenderUrlHandlerNoQuery) {
-  renderer_.renderUrlHandler(handler_, query_params_);
+  renderer_.urlHandler(response_, handler_, query_params_);
   std::string out = response_.toString();
   EXPECT_THAT(out, HasSubstr("")));
@@ -68,7 +69,7 @@ TEST_F(StatsHtmlRenderTest, RenderUrlHandlerNoQuery) {
 
 TEST_F(StatsHtmlRenderTest, RenderUrlHandlerWithQuery) {
   query_params_["param"] = "on";
-  renderer_.renderUrlHandler(handler_, query_params_);
+  renderer_.urlHandler(response_, handler_, query_params_);
   std::string out = response_.toString();
   EXPECT_THAT(out, HasSubstr(""));
@@ -81,7 +82,7 @@ TEST_F(StatsHtmlRenderTest, RenderUrlHandlerWithQuery) {
 
 TEST_F(StatsHtmlRenderTest, RenderUrlHandlerSubmitOnChange) {
   renderer_.setSubmitOnChange(true);
-  renderer_.renderUrlHandler(handler_, query_params_);
+  renderer_.urlHandler(response_, handler_, query_params_);
   std::string out = response_.toString();
   EXPECT_THAT(out, HasSubstr(" onchange='prefix.submit()"));
   EXPECT_THAT(out, Not(HasSubstr("")));
diff --git a/test/server/admin/stats_render_test.cc b/test/server/admin/stats_render_test.cc
index da8ee238d7382..b2f9ce2231531 100644
--- a/test/server/admin/stats_render_test.cc
+++ b/test/server/admin/stats_render_test.cc
@@ -2,8 +2,6 @@
 
 #include "test/server/admin/stats_render_test_base.h"
 
-using testing::HasSubstr;
-
 namespace Envoy {
 namespace Server {
 

From 03be3320e1debf3bdb1bd2131753c06ad107da53 Mon Sep 17 00:00:00 2001
From: Joshua Marantz 
Date: Sun, 17 Jul 2022 12:08:41 -0400
Subject: [PATCH 52/61] add accessibility annotations.

Signed-off-by: Joshua Marantz 
---
 source/server/admin/stats_html_render.cc | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/source/server/admin/stats_html_render.cc b/source/server/admin/stats_html_render.cc
index 189176ba52a0b..50c00938ee093 100644
--- a/source/server/admin/stats_html_render.cc
+++ b/source/server/admin/stats_html_render.cc
@@ -133,10 +133,12 @@ void StatsHtmlRender::urlHandler(Buffer::Instance& response, const Admin::UrlHan
   }
 
   for (const Admin::ParamDescriptor& param : handler.params_) {
-    response.addFragments({"\n  "});
+    std::string id = absl::StrCat("param-", index_, "-", absl::StrReplaceAll(path, {{"/", "-"}}),
+                                  "-", param.id_);
+    response.addFragments({"\n  "});
     input(response, param.id_, path, param.type_, query, param.enum_choices_);
-    response.addFragments({"\n  ", Html::Utility::sanitize(param.help_),
-                           "\n\n"});
+    response.addFragments({"\n  ", "\n\n"});
   }
 }
 

From ce6e90423751e560540d7aed5dacb41759383084 Mon Sep 17 00:00:00 2001
From: Joshua Marantz 
Date: Sun, 17 Jul 2022 14:52:41 -0400
Subject: [PATCH 53/61] fix html validation errors.

Signed-off-by: Joshua Marantz 
---
 source/server/admin/stats_html_render.cc | 21 +++++++++++++--------
 source/server/admin/stats_html_render.h  |  1 -
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/source/server/admin/stats_html_render.cc b/source/server/admin/stats_html_render.cc
index 50c00938ee093..ee7be536970ed 100644
--- a/source/server/admin/stats_html_render.cc
+++ b/source/server/admin/stats_html_render.cc
@@ -36,8 +36,10 @@ const char EnvoyFavicon[] =
 const char AdminHtmlTableBegin[] = R"(
   
-      
-      
+      
+        
+        
+      
 )";
@@ -54,9 +56,11 @@ namespace Server {
 
 StatsHtmlRender::StatsHtmlRender(Http::ResponseHeaderMap& response_headers,
                                  Buffer::Instance& response, const StatsParams& params)
-    : StatsTextRender(params), response_(response) {
+    : StatsTextRender(params) {
   response_headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Html);
-  response_.add(absl::StrReplaceAll(AdminHtmlStart, {{"@FAVICON@", EnvoyFavicon}}));
+  response.add("\n");
+  response.add("\n");
+  response.add(absl::StrReplaceAll(AdminHtmlStart, {{"@FAVICON@", EnvoyFavicon}}));
   response.add("\n");
 }
 
@@ -67,6 +71,7 @@ void StatsHtmlRender::finalize(Buffer::Instance& response) {
     response.add("\n");
   }
   response.add("\n");
+  response.add("");
 }
 
 void StatsHtmlRender::startPre(Buffer::Instance& response) {
@@ -125,7 +130,7 @@ void StatsHtmlRender::urlHandler(Buffer::Instance& response, const Admin::UrlHan
   } else {
     // Render an explicit visible submit as a link (for GET) or button (for POST).
     const char* button_style = handler.mutates_server_state_ ? "" : " class='button-as-link'";
-    response.addFragments({"\n\n\n\n  \n  \n  \n\n"});
   }
@@ -166,7 +171,7 @@ void StatsHtmlRender::input(Buffer::Instance& response, absl::string_view id,
   switch (type) {
   case Admin::ParamDescriptor::Type::Boolean:
     response.addFragments({""});
+                           on_change, value.empty() ? ">" : " checked/>"});
     break;
   case Admin::ParamDescriptor::Type::String:
     response.addFragments({"
Date: Sun, 17 Jul 2022 15:41:14 -0400
Subject: [PATCH 54/61] fix unit tests and prepopulated values.

Signed-off-by: Joshua Marantz 
---
 source/server/admin/stats_html_render.cc    | 19 ++++++++++---------
 source/server/admin/stats_html_render.h     |  5 +++--
 test/server/admin/stats_html_render_test.cc |  8 ++++++--
 3 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/source/server/admin/stats_html_render.cc b/source/server/admin/stats_html_render.cc
index ee7be536970ed..e9848bdca254f 100644
--- a/source/server/admin/stats_html_render.cc
+++ b/source/server/admin/stats_html_render.cc
@@ -125,8 +125,8 @@ void StatsHtmlRender::urlHandler(Buffer::Instance& response, const Admin::UrlHan
   // page from accidentally mutating all the server state by GETting all the hrefs.
   const char* method = handler.mutates_server_state_ ? "post" : "get";
   if (submit_on_change_) {
-    response.addFragments({"\n
    \n"});
+    response.addFragments({"\n    \n"});
   } else {
     // Render an explicit visible submit as a link (for GET) or button (for POST).
     const char* button_style = handler.mutates_server_state_ ? "" : " class='button-as-link'";
@@ -141,19 +141,20 @@ void StatsHtmlRender::urlHandler(Buffer::Instance& response, const Admin::UrlHan
     std::string id = absl::StrCat("param-", index_, "-", absl::StrReplaceAll(path, {{"/", "-"}}),
                                   "-", param.id_);
     response.addFragments({"\n  \n  \n\n"});
   }
 }
 
 void StatsHtmlRender::input(Buffer::Instance& response, absl::string_view id,
-                            absl::string_view path, Admin::ParamDescriptor::Type type,
+                            absl::string_view name, absl::string_view path,
+                            Admin::ParamDescriptor::Type type,
                             OptRef query,
                             const std::vector& enum_choices) {
   std::string value;
   if (query.has_value()) {
-    auto iter = query->find(std::string(id));
+    auto iter = query->find(std::string(name));
     if (iter != query->end()) {
       value = iter->second;
     }
@@ -170,16 +171,16 @@ void StatsHtmlRender::input(Buffer::Instance& response, absl::string_view id,
 
   switch (type) {
   case Admin::ParamDescriptor::Type::Boolean:
-    response.addFragments({"" : " checked/>"});
+    response.addFragments({"" : " checked/>"});
     break;
   case Admin::ParamDescriptor::Type::String:
-    response.addFragments({""});
     break;
   case Admin::ParamDescriptor::Type::Enum:
     response.addFragments(
-        {"\n    \n"});
     for (absl::string_view choice : enum_choices) {
       std::string sanitized = Html::Utility::sanitize(choice);
       absl::string_view selected = (value == sanitized) ? " selected" : "";
diff --git a/source/server/admin/stats_html_render.h b/source/server/admin/stats_html_render.h
index eafc60fd3c6d8..95e4504d8e382 100644
--- a/source/server/admin/stats_html_render.h
+++ b/source/server/admin/stats_html_render.h
@@ -52,8 +52,9 @@ class StatsHtmlRender : public StatsTextRender {
   void urlHandler(Buffer::Instance&, const Admin::UrlHandler& handler,
                   OptRef query);
 
-  void input(Buffer::Instance&, absl::string_view id, absl::string_view path,
-             Admin::ParamDescriptor::Type type, OptRef query,
+  void input(Buffer::Instance&, absl::string_view id, absl::string_view name,
+             absl::string_view path, Admin::ParamDescriptor::Type type,
+             OptRef query,
              const std::vector& enum_choices);
 
   // By default, editing parameters does not cause a form-submit -- you have
diff --git a/test/server/admin/stats_html_render_test.cc b/test/server/admin/stats_html_render_test.cc
index 5ca1898651d4c..50e0b85c5f563 100644
--- a/test/server/admin/stats_html_render_test.cc
+++ b/test/server/admin/stats_html_render_test.cc
@@ -58,7 +58,9 @@ TEST_F(StatsHtmlRenderTest, RenderTableEnd) {
 TEST_F(StatsHtmlRenderTest, RenderUrlHandlerNoQuery) {
   renderer_.urlHandler(response_, handler_, query_params_);
   std::string out = response_.toString();
-  EXPECT_THAT(out, HasSubstr("")));
   EXPECT_THAT(out, HasSubstr("help text"));
   EXPECT_THAT(out, HasSubstr("param help"));
@@ -71,7 +73,9 @@ TEST_F(StatsHtmlRenderTest, RenderUrlHandlerWithQuery) {
   query_params_["param"] = "on";
   renderer_.urlHandler(response_, handler_, query_params_);
   std::string out = response_.toString();
-  EXPECT_THAT(out, HasSubstr(""));
   EXPECT_THAT(out, HasSubstr("help text"));
   EXPECT_THAT(out, HasSubstr("param help"));

From 50dbe5adbfeb4fdab1de470b452321370333a71e Mon Sep 17 00:00:00 2001
From: Joshua Marantz 
Date: Mon, 18 Jul 2022 13:27:29 -0400
Subject: [PATCH 55/61] Render endpoints using GET that don't have params as
 simple a tags.

Signed-off-by: Joshua Marantz 
---
 source/server/admin/stats_html_render.cc | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/source/server/admin/stats_html_render.cc b/source/server/admin/stats_html_render.cc
index e9848bdca254f..e26ffb4c8862b 100644
--- a/source/server/admin/stats_html_render.cc
+++ b/source/server/admin/stats_html_render.cc
@@ -128,12 +128,21 @@ void StatsHtmlRender::urlHandler(Buffer::Instance& response, const Admin::UrlHan
     response.addFragments({"\n\n"});
   } else {
-    // Render an explicit visible submit as a link (for GET) or button (for POST).
-    const char* button_style = handler.mutates_server_state_ ? "" : " class='button-as-link'";
     response.addFragments({"\n\n\n  \n  \n  \n\n"});
   }
 

From 0859243678b58099f11976b0a5b29368299ac2de Mon Sep 17 00:00:00 2001
From: Joshua Marantz 
Date: Thu, 21 Jul 2022 21:25:55 -0400
Subject: [PATCH 56/61] review comments

Signed-off-by: Joshua Marantz 
---
 source/server/admin/stats_handler.cc     |  3 ++-
 source/server/admin/stats_html_render.cc | 24 +++++++++++++-----------
 source/server/admin/stats_params.cc      | 20 ++++++++++----------
 source/server/admin/stats_params.h       |  5 ++---
 4 files changed, 27 insertions(+), 25 deletions(-)

diff --git a/source/server/admin/stats_handler.cc b/source/server/admin/stats_handler.cc
index 5ed4780cc93f5..f8ad9e810166b 100644
--- a/source/server/admin/stats_handler.cc
+++ b/source/server/admin/stats_handler.cc
@@ -183,7 +183,8 @@ Admin::UrlHandler StatsHandler::statsHandler() {
        {Admin::ParamDescriptor::Type::Enum,
         "type",
         "Stat types to include.",
-        {Labels::All, Labels::Counters, Labels::Histograms, Labels::Gauges, Labels::TextReadouts}},
+        {StatLabels::All, StatLabels::Counters, StatLabels::Histograms, StatLabels::Gauges,
+         StatLabels::TextReadouts}},
        {Admin::ParamDescriptor::Type::Enum,
         "histogram_buckets",
         "Histogram bucket display mode",
diff --git a/source/server/admin/stats_html_render.cc b/source/server/admin/stats_html_render.cc
index e26ffb4c8862b..9615442cfbeb7 100644
--- a/source/server/admin/stats_html_render.cc
+++ b/source/server/admin/stats_html_render.cc
@@ -116,7 +116,7 @@ void StatsHtmlRender::urlHandler(Buffer::Instance& response, const Admin::UrlHan
   path = sanitized_path;
 
   // Alternate gray and white param-blocks. The pure CSS way of coloring based
-  // on row index doesn't work correctly for us we are using a row for each
+  // on row index doesn't work correctly for us as we are using a row for each
   // parameter, and we want each endpoint/option-block to be colored the same.
   const char* row_class = (++index_ & 1) ? " class='gray'" : "";
 
@@ -174,27 +174,29 @@ void StatsHtmlRender::input(Buffer::Instance& response, absl::string_view id,
     on_change = absl::StrCat(" onchange='", path, ".submit()'");
   }
 
-  auto value_tag = [](const std::string& value) -> std::string {
-    return value.empty() ? "" : absl::StrCat(" value=", Html::Utility::sanitize(value));
-  };
-
   switch (type) {
   case Admin::ParamDescriptor::Type::Boolean:
     response.addFragments({"" : " checked/>"});
     break;
-  case Admin::ParamDescriptor::Type::String:
+  case Admin::ParamDescriptor::Type::String: {
+    std::string sanitized;
+    if (!value.empty()) {
+      sanitized = absl::StrCat(" value=", Html::Utility::sanitize(value));
+    }
     response.addFragments({""});
+                           on_change, sanitized, " />"});
     break;
+  }
   case Admin::ParamDescriptor::Type::Enum:
     response.addFragments(
         {"\n    \n  ");
     break;
diff --git a/source/server/admin/stats_params.cc b/source/server/admin/stats_params.cc
index b117c7498746e..5fc32eaca399f 100644
--- a/source/server/admin/stats_params.cc
+++ b/source/server/admin/stats_params.cc
@@ -37,15 +37,15 @@ Http::Code StatsParams::parse(absl::string_view url, Buffer::Instance& response)
   }
 
   auto parse_type = [](absl::string_view str, StatsType& type) {
-    if (str == Labels::Gauges) {
+    if (str == StatLabels::Gauges) {
       type = StatsType::Gauges;
-    } else if (str == Labels::Counters) {
+    } else if (str == StatLabels::Counters) {
       type = StatsType::Counters;
-    } else if (str == Labels::Histograms) {
+    } else if (str == StatLabels::Histograms) {
       type = StatsType::Histograms;
-    } else if (str == Labels::TextReadouts) {
+    } else if (str == StatLabels::TextReadouts) {
       type = StatsType::TextReadouts;
-    } else if (str == Labels::All) {
+    } else if (str == StatLabels::All) {
       type = StatsType::All;
     } else {
       return false;
@@ -87,19 +87,19 @@ absl::string_view StatsParams::typeToString(StatsType type) {
   absl::string_view ret;
   switch (type) {
   case StatsType::TextReadouts:
-    ret = Labels::TextReadouts;
+    ret = StatLabels::TextReadouts;
     break;
   case StatsType::Counters:
-    ret = Labels::Counters;
+    ret = StatLabels::Counters;
     break;
   case StatsType::Gauges:
-    ret = Labels::Gauges;
+    ret = StatLabels::Gauges;
     break;
   case StatsType::Histograms:
-    ret = Labels::Histograms;
+    ret = StatLabels::Histograms;
     break;
   case StatsType::All:
-    ret = Labels::All;
+    ret = StatLabels::All;
     break;
   }
   return ret;
diff --git a/source/server/admin/stats_params.h b/source/server/admin/stats_params.h
index d50886972e0d0..f4f0973129cc2 100644
--- a/source/server/admin/stats_params.h
+++ b/source/server/admin/stats_params.h
@@ -14,13 +14,13 @@
 namespace Envoy {
 namespace Server {
 
-namespace Labels {
+namespace StatLabels {
 constexpr absl::string_view All = "All";
 constexpr absl::string_view Counters = "Counters";
 constexpr absl::string_view Gauges = "Gauges";
 constexpr absl::string_view Histograms = "Histograms";
 constexpr absl::string_view TextReadouts = "TextReadouts";
-} // namespace Labels
+} // namespace StatLabels
 
 enum class StatsFormat {
 #ifdef ENVOY_ADMIN_HTML
@@ -55,7 +55,6 @@ struct StatsParams {
   static absl::string_view typeToString(StatsType type);
 
   StatsType type_{StatsType::All};
-  StatsType start_type_{StatsType::TextReadouts};
   bool used_only_{false};
   bool prometheus_text_readouts_{false};
   bool pretty_{false};

From ee21f747abe88c44270ea51281a28796a13a262e Mon Sep 17 00:00:00 2001
From: Joshua Marantz 
Date: Mon, 25 Jul 2022 18:15:04 -0400
Subject: [PATCH 57/61] add missing params (round 1)

Signed-off-by: Joshua Marantz 
---
 docs/root/operations/admin.rst  |  4 +++-
 source/server/admin/admin.cc    | 23 +++++++++++++++++------
 test/server/admin/admin_test.cc | 10 ++++++----
 3 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/docs/root/operations/admin.rst b/docs/root/operations/admin.rst
index bab1a50409586..cc74f9d8e0c3f 100644
--- a/docs/root/operations/admin.rst
+++ b/docs/root/operations/admin.rst
@@ -63,7 +63,9 @@ modify different aspects of the server:
 
 .. http:get:: /
 
-  Render an HTML home page with a table of links to all available options.
+  Render an HTML home page with a table of links to all available options. This can be
+  disabled by compiling Envoy with `--define=admin_html=disabled` in which case an error
+  message is printed. Disabling the HTML mode reduces the Envoy binary size.
 
 .. http:get:: /help
 
diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc
index 7751b18153afc..bd476275f1f81 100644
--- a/source/server/admin/admin.cc
+++ b/source/server/admin/admin.cc
@@ -106,7 +106,13 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server,
                {Admin::ParamDescriptor::Type::String, "mask",
                 "The mask to apply. When both resource and mask are specified, "
                 "the mask is applied to every element in the desired repeated field so that only a "
-                "subset of fields are returned. The mask is parsed as a ProtobufWkt::FieldMask"}}),
+                "subset of fields are returned. The mask is parsed as a ProtobufWkt::FieldMask"},
+               {Admin::ParamDescriptor::Type::String, "name_regex",
+                "Dump only the currently loaded configurations whose names match the specified "
+                "regex. Can be used with both resource and mask query parameters."},
+               {Admin::ParamDescriptor::Type::Boolean, "include_eds",
+                "Dump currently loaded configuration including EDS. See the response definition "
+                "for more information"}}),
           makeHandler("/init_dump", "dump current Envoy init manager information (experimental)",
                       MAKE_ADMIN_HANDLER(init_dump_handler_.handlerInitDump), false, false,
                       {{Admin::ParamDescriptor::Type::String, "mask",
@@ -193,7 +199,15 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server,
       date_provider_(server.dispatcher().timeSource()),
       admin_filter_chain_(std::make_shared()),
       local_reply_(LocalReply::Factory::createDefault()),
-      ignore_global_conn_limit_(ignore_global_conn_limit) {}
+      ignore_global_conn_limit_(ignore_global_conn_limit) {
+#ifndef NDEBUG
+  // Verify that no duplicate handlers exist.
+  absl::flat_hash_set handlers;
+  for (const UrlHandler& handler : handlers_) {
+    ASSERT(handlers.insert(handler.prefix_).second);
+  }
+#endif
+}
 
 Http::ServerConnectionPtr AdminImpl::createCodec(Network::Connection& connection,
                                                  const Buffer::Instance& data,
@@ -360,10 +374,7 @@ void AdminImpl::getHelp(Buffer::Instance& response) {
     for (const ParamDescriptor& param : handler->params_) {
       response.add(fmt::format("      {}: {}", param.id_, param.help_));
       if (param.type_ == ParamDescriptor::Type::Enum) {
-        response.add("; One of");
-        for (absl::string_view choice : param.enum_choices_) {
-          response.addFragments({" ", choice});
-        }
+        response.addFragments({"; One of (", absl::StrJoin(param.enum_choices_, ", "), ")"});
       }
       response.add("\n");
     }
diff --git a/test/server/admin/admin_test.cc b/test/server/admin/admin_test.cc
index cc48ed7e11414..b32219ab0eea6 100644
--- a/test/server/admin/admin_test.cc
+++ b/test/server/admin/admin_test.cc
@@ -137,6 +137,8 @@ TEST_P(AdminInstanceTest, Help) {
   /config_dump: dump current Envoy configs (experimental)
       resource: The resource to dump
       mask: The mask to apply. When both resource and mask are specified, the mask is applied to every element in the desired repeated field so that only a subset of fields are returned. The mask is parsed as a ProtobufWkt::FieldMask
+      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
@@ -150,7 +152,7 @@ TEST_P(AdminInstanceTest, Help) {
   /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
+      format: File format to use; One of (text, json)
   /logging: query/change logging levels
   /memory: print current allocation/heap usage
   /quitquitquit: exit the server
@@ -163,9 +165,9 @@ TEST_P(AdminInstanceTest, Help) {
   /stats: print server stats
       usedonly: Only include stats that have been written by system since restart
       filter: Regular expression (ecmascript) for filtering stats
-      format: Format to use; One of html text json
-      type: Stat types to include.; One of All Counters Histograms Gauges TextReadouts
-      histogram_buckets: Histogram bucket display mode; One of cumulative disjoint none
+      format: Format to use; One of (html, text, json)
+      type: Stat types to include.; One of (All, Counters, Histograms, Gauges, TextReadouts)
+      histogram_buckets: Histogram bucket display mode; One of (cumulative, disjoint, none)
   /stats/prometheus: print server stats in prometheus format
       usedonly: Only include stats that have been written by system since restart
       text_readouts: Render text_readouts as new gaugues with value 0 (increases Prometheus data size)

From 379b31611947b6708acca83e5a5c366443626caf Mon Sep 17 00:00:00 2001
From: Joshua Marantz 
Date: Mon, 25 Jul 2022 20:03:22 -0400
Subject: [PATCH 58/61] add remaining missing admin query params.

Signed-off-by: Joshua Marantz 
---
 source/server/admin/admin.cc        | 17 ++++++++++++++++-
 source/server/admin/logs_handler.cc | 17 +++++++++++++----
 source/server/admin/logs_handler.h  |  5 +++++
 test/server/admin/admin_test.cc     |  2 ++
 4 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc
index bd476275f1f81..ae5ff62fec702 100644
--- a/source/server/admin/admin.cc
+++ b/source/server/admin/admin.cc
@@ -76,6 +76,16 @@ void AdminImpl::startHttpListener(const std::list&
   }
 }
 
+namespace {
+std::vector prependBlank(const std::vector& strings) {
+  std::vector v;
+  v.reserve(strings.size() + 1);
+  v.push_back("");
+  v.insert(v.end(), strings.begin(), strings.end());
+  return v;
+}
+} // namespace
+
 AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server,
                      bool ignore_global_conn_limit)
     : server_(server),
@@ -135,7 +145,12 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server,
                       MAKE_ADMIN_HANDLER(server_info_handler_.handlerHotRestartVersion), false,
                       false),
           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",
+                        "To change multiple logging levels at once, set to "
+                        "=,=."},
+                       {Admin::ParamDescriptor::Type::Enum, "level", "desired logging level",
+                        prependBlank(LogsHandler::levelStrings())}}),
           makeHandler("/memory", "print current allocation/heap usage",
                       MAKE_ADMIN_HANDLER(server_info_handler_.handlerMemory), false, false),
           makeHandler("/quitquitquit", "exit the server",
diff --git a/source/server/admin/logs_handler.cc b/source/server/admin/logs_handler.cc
index 8a50f9f78c0f9..4bc64ae5460dd 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,6 +28,16 @@ absl::flat_hash_map buildLevelMap(
 LogsHandler::LogsHandler(Server::Instance& server)
     : HandlerContextBase(server), log_levels_(buildLevelMap()) {}
 
+std::vector LogsHandler::levelStrings() {
+  std::vector strings;
+  strings.reserve(ARRAY_SIZE(spdlog::level::level_string_views));
+  for (size_t i = 0; i < ARRAY_SIZE(spdlog::level::level_string_views); i++) {
+    spdlog::string_view_t level{spdlog::level::level_string_views[i]};
+    strings.push_back(absl::string_view{level.data(), level.size()});
+  }
+  return strings;
+}
+
 Http::Code LogsHandler::handlerLogging(absl::string_view url, Http::ResponseHeaderMap&,
                                        Buffer::Instance& response, AdminStream&) {
   Http::Utility::QueryParams query_params = Http::Utility::parseQueryString(url);
diff --git a/source/server/admin/logs_handler.h b/source/server/admin/logs_handler.h
index 357f65cc32c94..be050638f5393 100644
--- a/source/server/admin/logs_handler.h
+++ b/source/server/admin/logs_handler.h
@@ -30,6 +30,11 @@ class LogsHandler : public HandlerContextBase, Logger::Loggable levelStrings();
+
 private:
   /**
    * Attempt to change the log level of a logger or all loggers.
diff --git a/test/server/admin/admin_test.cc b/test/server/admin/admin_test.cc
index b32219ab0eea6..6684db9ea8e6b 100644
--- a/test/server/admin/admin_test.cc
+++ b/test/server/admin/admin_test.cc
@@ -154,6 +154,8 @@ TEST_P(AdminInstanceTest, Help) {
   /listeners: print listener info
       format: File format to use; One of (text, json)
   /logging: query/change logging levels
+      paths: To change multiple logging levels at once, set to =,=.
+      level: desired logging level; One of (, trace, debug, info, warning, error, critical, off)
   /memory: print current allocation/heap usage
   /quitquitquit: exit the server
   /ready: print server state, return 200 if LIVE, otherwise return 503

From a7b35656483825f0570e58799b8a328eeb29269c Mon Sep 17 00:00:00 2001
From: Joshua Marantz 
Date: Tue, 26 Jul 2022 08:53:56 -0400
Subject: [PATCH 59/61] back out post support for now

Signed-off-by: Joshua Marantz 
---
 source/server/admin/admin.cc        | 22 ++++++----------------
 source/server/admin/logs_handler.cc | 17 ++++-------------
 source/server/admin/logs_handler.h  |  5 -----
 3 files changed, 10 insertions(+), 34 deletions(-)

diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc
index ae5ff62fec702..536ec1e971346 100644
--- a/source/server/admin/admin.cc
+++ b/source/server/admin/admin.cc
@@ -76,16 +76,6 @@ void AdminImpl::startHttpListener(const std::list&
   }
 }
 
-namespace {
-std::vector prependBlank(const std::vector& strings) {
-  std::vector v;
-  v.reserve(strings.size() + 1);
-  v.push_back("");
-  v.insert(v.end(), strings.begin(), strings.end());
-  return v;
-}
-} // namespace
-
 AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server,
                      bool ignore_global_conn_limit)
     : server_(server),
@@ -144,13 +134,13 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server,
           makeHandler("/hot_restart_version", "print the hot restart compatibility version",
                       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.
           makeHandler("/logging", "query/change logging levels",
-                      MAKE_ADMIN_HANDLER(logs_handler_.handlerLogging), false, true,
-                      {{Admin::ParamDescriptor::Type::String, "paths",
-                        "To change multiple logging levels at once, set to "
-                        "=,=."},
-                       {Admin::ParamDescriptor::Type::Enum, "level", "desired logging level",
-                        prependBlank(LogsHandler::levelStrings())}}),
+                      MAKE_ADMIN_HANDLER(logs_handler_.handlerLogging), false, true),
+
           makeHandler("/memory", "print current allocation/heap usage",
                       MAKE_ADMIN_HANDLER(server_info_handler_.handlerMemory), false, false),
           makeHandler("/quitquitquit", "exit the server",
diff --git a/source/server/admin/logs_handler.cc b/source/server/admin/logs_handler.cc
index 4bc64ae5460dd..8a50f9f78c0f9 100644
--- a/source/server/admin/logs_handler.cc
+++ b/source/server/admin/logs_handler.cc
@@ -16,9 +16,10 @@ namespace {
 absl::flat_hash_map buildLevelMap() {
   absl::flat_hash_map levels;
 
-  uint32_t i = 0;
-  for (absl::string_view level_string : LogsHandler::levelStrings()) {
-    levels[level_string] = static_cast(i++);
+  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);
   }
 
   return levels;
@@ -28,16 +29,6 @@ absl::flat_hash_map buildLevelMap(
 LogsHandler::LogsHandler(Server::Instance& server)
     : HandlerContextBase(server), log_levels_(buildLevelMap()) {}
 
-std::vector LogsHandler::levelStrings() {
-  std::vector strings;
-  strings.reserve(ARRAY_SIZE(spdlog::level::level_string_views));
-  for (size_t i = 0; i < ARRAY_SIZE(spdlog::level::level_string_views); i++) {
-    spdlog::string_view_t level{spdlog::level::level_string_views[i]};
-    strings.push_back(absl::string_view{level.data(), level.size()});
-  }
-  return strings;
-}
-
 Http::Code LogsHandler::handlerLogging(absl::string_view url, Http::ResponseHeaderMap&,
                                        Buffer::Instance& response, AdminStream&) {
   Http::Utility::QueryParams query_params = Http::Utility::parseQueryString(url);
diff --git a/source/server/admin/logs_handler.h b/source/server/admin/logs_handler.h
index be050638f5393..357f65cc32c94 100644
--- a/source/server/admin/logs_handler.h
+++ b/source/server/admin/logs_handler.h
@@ -30,11 +30,6 @@ class LogsHandler : public HandlerContextBase, Logger::Loggable levelStrings();
-
 private:
   /**
    * Attempt to change the log level of a logger or all loggers.

From c4569548726714c23b32cb201332c9fb355d3e14 Mon Sep 17 00:00:00 2001
From: Joshua Marantz 
Date: Tue, 26 Jul 2022 08:59:16 -0400
Subject: [PATCH 60/61] back out more post params

Signed-off-by: Joshua Marantz 
---
 source/server/admin/admin.cc    | 12 ++----------
 test/server/admin/admin_test.cc |  4 ----
 2 files changed, 2 insertions(+), 14 deletions(-)

diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc
index 536ec1e971346..c47937c07a193 100644
--- a/source/server/admin/admin.cc
+++ b/source/server/admin/admin.cc
@@ -137,7 +137,7 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server,
 
           // 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.
+          // re-plumbing through the admin callback API. See also drain_listeners.
           makeHandler("/logging", "query/change logging levels",
                       MAKE_ADMIN_HANDLER(logs_handler_.handlerLogging), false, true),
 
@@ -149,15 +149,7 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server,
                       MAKE_ADMIN_HANDLER(stats_handler_.handlerResetCounters), 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."}}),
+              MAKE_ADMIN_HANDLER(listeners_handler_.handlerDrainListeners), false, true),
           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",
diff --git a/test/server/admin/admin_test.cc b/test/server/admin/admin_test.cc
index 6684db9ea8e6b..cddeea3bbb06e 100644
--- a/test/server/admin/admin_test.cc
+++ b/test/server/admin/admin_test.cc
@@ -142,8 +142,6 @@ TEST_P(AdminInstanceTest, Help) {
   /contention: dump current Envoy mutex contention stats (if enabled)
   /cpuprofiler: enable/disable the CPU profiler
   /drain_listeners: 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: cause the server to fail health checks
   /healthcheck/ok: cause the server to pass health checks
   /heapprofiler: enable/disable the heap profiler
@@ -154,8 +152,6 @@ TEST_P(AdminInstanceTest, Help) {
   /listeners: print listener info
       format: File format to use; One of (text, json)
   /logging: query/change logging levels
-      paths: To change multiple logging levels at once, set to =,=.
-      level: desired logging level; One of (, trace, debug, info, warning, error, critical, off)
   /memory: print current allocation/heap usage
   /quitquitquit: exit the server
   /ready: print server state, return 200 if LIVE, otherwise return 503

From 8c3af8bdc15b7cf7a85bd8f588aab237b570c149 Mon Sep 17 00:00:00 2001
From: Joshua Marantz 
Date: Tue, 26 Jul 2022 09:12:00 -0400
Subject: [PATCH 61/61] format

Signed-off-by: Joshua Marantz 
---
 source/server/admin/admin.cc | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc
index c47937c07a193..95c254b49b6b8 100644
--- a/source/server/admin/admin.cc
+++ b/source/server/admin/admin.cc
@@ -147,9 +147,8 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& 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),
           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",

    
     
    CommandDescription
    CommandDescription
    
         
     
    \n    ",
                            path, "\n  
    		",
@@ -135,8 +140,8 @@ void StatsHtmlRender::urlHandler(Buffer::Instance& response, const Admin::UrlHan
   for (const Admin::ParamDescriptor& param : handler.params_) {
     std::string id = absl::StrCat("param-", index_, "-", absl::StrReplaceAll(path, {{"/", "-"}}),
                                   "-", param.id_);
-    response.addFragments({"\n  "});
-    input(response, param.id_, path, param.type_, query, param.enum_choices_);
+    response.addFragments({"\n  "});
+    input(response, id, path, param.type_, query, param.enum_choices_);
     response.addFragments({"", "
    "});
-    input(response, id, path, param.type_, query, param.enum_choices_);
+    input(response, id, param.id_, path, param.type_, query, param.enum_choices_);
     response.addFragments({"", "
    \n    ",
-                           path, "\n  
    		",
+                           ">\n  "});
+    if (!handler.mutates_server_state_ && handler.params_.empty()) {
+      // GET requests without parameters can be simple links rather than forms with
+      // buttons that are rendered as links. This simplification improves the
+      // usability of the page with screen-readers.
+      response.addFragments({"", path, ""});
+    } else {
+      // Render an explicit visible submit as a link (for GET) or button (for POST).
+      const char* button_style = handler.mutates_server_state_ ? "" : " class='button-as-link'";
+      response.addFragments({"
    \n    ", path,
+                             "\n  "});
+    }
+    response.addFragments({"
    		",
                            Html::Utility::sanitize(handler.help_text_), "