admin: add API for creating chunked handlers (#19831)

Commit Message: The admin handler structure uses a single callback that is expected to fill in the entire response body, making it impossible to stream data out as it is generated. This PR refactors the internal admin structures to require a Handler object with a nextChunk method, to enable chunking. It defers to other PRs to
 * actually convert any handlers to use chunking (e.g. WiP stats: Reimplement existing /stats API in a way that can be chunked. #19693)
 * implement proper flow control in the AdminFilter to avoid generating chunks before clients are ready to receive them (WiP admin: add flow control to admin filter #19898)

Additional Description:
Risk Level: medium -- this is a pure refactor but not a trivial one. The same exact admin content should be generated before/after this change
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

Signed-off-by: Joshua Marantz <jmarantz@google.com>

envoy/server/admin.h

@@ -76,6 +76,34 @@ class Admin {
 public:
  virtual ~Admin() = default;
 
+ // Represents a handler for admin endpoints, allowing for chunked responses.
+ class Handler {
+ public:
+ virtual ~Handler() = default;
+
+ /**
+ * Initiates a handler. The URL can be supplied to the constructor if needed.
+ *
+ * @param response_headers successful text responses don't need to modify this,
+ * but if we want to respond with (e.g.) JSON or HTML we can can set
+ * those here.
+ * @return the HTTP status of the response.
+ */
+ virtual Http::Code start(Http::ResponseHeaderMap& response_headers) PURE;
+
+ /**
+ * Adds the next chunk of data to the response. Note that nextChunk can
+ * return 'true' but not add any data to the response, in which case a chunk
+ * is not sent, and a subsequent call to nextChunk can be made later,
+ * possibly after a post() or low-watermark callback on the http filter.
+ *
+ * @param response a buffer in which to write the chunk
+ * @return whether or not any chunks follow this one.
+ */
+ virtual bool nextChunk(Buffer::Instance& response) PURE;
+ };
+ using HandlerPtr = std::unique_ptr<Handler>;
+
  /**
  * Callback for admin URL handlers.
  * @param path_and_query supplies the path and query of the request URL.
@@ -91,7 +119,14 @@ class Admin {
  Buffer::Instance& response, AdminStream& admin_stream)>;
 
  /**
- * Add an admin handler.
+ * Lambda to generate a Handler.
+ */
+ using GenHandlerCb = std::function<HandlerPtr(absl::string_view path, AdminStream&)>;
+
+ /**
+ * Add a legacy admin handler where the entire response is written in
+ * one chunk.
+ *
  * @param prefix supplies the URL prefix to handle.
  * @param help_text supplies the help text for the handler.
  * @param callback supplies the callback to invoke when the prefix matches.
@@ -102,6 +137,20 @@ class Admin {
  virtual bool addHandler(const std::string& prefix, const std::string& help_text,
  HandlerCb callback, bool removable, bool mutates_server_state) PURE;
 
+ /**
+ * Adds a an chunked admin handler.
+ *
+ * @param prefix supplies the URL prefix to handle.
+ * @param help_text supplies the help text for the handler.
+ * @param callback supplies the callback to generate a Handler.
+ * @param removable if true allows the handler to be removed via removeHandler.
+ * @param mutates_server_state indicates whether callback will mutate server state.
+ * @return bool true if the handler was added, false if it was not added.
+ */
+ virtual bool addChunkedHandler(const std::string& prefix, const std::string& help_text,
+ GenHandlerCb callback, bool removable,
+ bool mutates_server_state) PURE;
+
  /**
  * Remove an admin handler if it is removable.
  * @param prefix supplies the URL prefix of the handler to delete.

source/server/admin/admin.h

@@ -84,6 +84,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 addChunkedHandler(const std::string& prefix, const std::string& help_text,
+ GenHandlerCb callback, bool removable, bool mutates_server_state) override;
  bool removeHandler(const std::string& prefix) override;
  ConfigTracker& getConfigTracker() override;
 
@@ -200,6 +202,11 @@ class AdminImpl : public Admin,
  void addListenerToHandler(Network::ConnectionHandler* handler) override;
  Server::Instance& server() { return server_; }
 
+ GenHandlerCb createHandlerFunction() {
+ return [this](absl::string_view path_and_query, AdminStream& admin_stream) -> HandlerPtr {
+ return findHandler(path_and_query, admin_stream);
+ };
+ }
  AdminFilter::AdminServerCallbackFunction createCallbackFunction() {
  return [this](absl::string_view path_and_query, Http::ResponseHeaderMap& response_headers,
  Buffer::OwnedImpl& response, AdminFilter& filter) -> Http::Code {
@@ -211,18 +218,36 @@ class AdminImpl : public Admin,
  return proxy_status_config_.get();
  }
 
+ /**
+ * Makes a chunked handler for static text.
+ * @param resposne_text the text to populate response with
+ * @param code the Http::Code for the response
+ * @return the handler
+ */
+ static HandlerPtr makeStaticTextHandler(absl::string_view response_text, Http::Code code);
+
 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 GenHandlerCb handler_;
  const bool removable_;
  const bool mutates_server_state_;
  };
 
+ /**
+ * Creates a Handler instance given a request.
+ */
+ HandlerPtr findHandler(absl::string_view path_and_query, AdminStream& admin_stream);
+ /**
+ * Creates a UrlHandler structure from a non-chunked callback.
+ */
+ UrlHandler makeHandler(const std::string& prefix, const std::string& help_text,
+ HandlerCb callback, bool removable, bool mutates_state);
+
  /**
  * Implementation of RouteConfigProvider that returns a static null route config.
  */
@@ -315,6 +340,7 @@ class AdminImpl : public Admin,
  Http::Code handlerHelp(absl::string_view path_and_query,
  Http::ResponseHeaderMap& response_headers, Buffer::Instance& response,
  AdminStream&);
+ void getHelp(Buffer::Instance& response);
 
  class AdminListenSocketFactory : public Network::ListenSocketFactory {
  public:

source/server/admin/admin_filter.cc

@@ -5,8 +5,8 @@
 namespace Envoy {
 namespace Server {
 
-AdminFilter::AdminFilter(AdminServerCallbackFunction admin_server_callback_func)
- : admin_server_callback_func_(admin_server_callback_func) {}
+AdminFilter::AdminFilter(Admin::GenHandlerCb admin_handler_fn)
+ : admin_handler_fn_(admin_handler_fn) {}
 
 Http::FilterHeadersStatus AdminFilter::decodeHeaders(Http::RequestHeaderMap& headers,
  bool end_stream) {
@@ -65,18 +65,25 @@ void AdminFilter::onComplete() {
  const absl::string_view path = request_headers_->getPathValue();
  ENVOY_STREAM_LOG(debug, "request complete: path: {}", *decoder_callbacks_, path);
 
- Buffer::OwnedImpl response;
  auto header_map = Http::ResponseHeaderMapImpl::create();
  RELEASE_ASSERT(request_headers_, "");
- Http::Code code = admin_server_callback_func_(path, *header_map, response, *this);
+ Admin::HandlerPtr handler = admin_handler_fn_(path, *this);
+ Http::Code code = handler->start(/*path, */ *header_map);
  Utility::populateFallbackResponseHeaders(code, *header_map);
- decoder_callbacks_->encodeHeaders(std::move(header_map),
- end_stream_on_complete_ && response.length() == 0,
+ decoder_callbacks_->encodeHeaders(std::move(header_map), false,
  StreamInfo::ResponseCodeDetails::get().AdminFilterResponse);
 
- if (response.length() > 0) {
- decoder_callbacks_->encodeData(response, end_stream_on_complete_);
- }
+ bool more_data;
+ do {
+ Buffer::OwnedImpl response;
+ more_data = handler->nextChunk(response);
+ bool end_stream = end_stream_on_complete_ && !more_data;
+ ENVOY_LOG_MISC(error, "nextChunk: response.length={} more_data={} end_stream={}",
+ response.length(), more_data, end_stream);
+ if (response.length() > 0 || end_stream) {
+ decoder_callbacks_->encodeData(response, end_stream);
+ }
+ } while (more_data);
 }
 
 } // namespace Server

source/server/admin/admin_filter.h

@@ -28,7 +28,7 @@ class AdminFilter : public Http::PassThroughFilter,
  absl::string_view path_and_query, Http::ResponseHeaderMap& response_headers,
  Buffer::OwnedImpl& response, AdminFilter& filter)>;
 
- AdminFilter(AdminServerCallbackFunction admin_server_run_callback_func);
+ AdminFilter(Admin::GenHandlerCb admin_handler_func);
 
  // Http::StreamFilterBase
  // Handlers relying on the reference should use addOnDestroyCallback()
@@ -57,7 +57,7 @@ class AdminFilter : public Http::PassThroughFilter,
  * Called when an admin request has been completely received.
  */
  void onComplete();
- AdminServerCallbackFunction admin_server_callback_func_;
+ Admin::GenHandlerCb admin_handler_fn_;
  Http::RequestHeaderMap* request_headers_{};
  std::list<std::function<void()>> on_destroy_callbacks_;
  bool end_stream_on_complete_ = true;

source/server/config_validation/admin.cc

@@ -4,6 +4,10 @@ namespace Envoy {
 namespace Server {
 
 // Pretend that handler was added successfully.
+bool ValidationAdmin::addChunkedHandler(const std::string&, const std::string&, GenHandlerCb, bool,
+ bool) {
+ return true;
+}
 bool ValidationAdmin::addHandler(const std::string&, const std::string&, HandlerCb, bool, bool) {
  return true;
 }

source/server/config_validation/admin.h

@@ -24,6 +24,7 @@ class ValidationAdmin : public Admin {
  nullptr)
  : nullptr) {}
  bool addHandler(const std::string&, const std::string&, HandlerCb, bool, bool) override;
+ bool addChunkedHandler(const std::string&, const std::string&, GenHandlerCb, bool, bool) override;
  bool removeHandler(const std::string&) override;
  const Network::Socket& socket() override;
  ConfigTracker& getConfigTracker() override;

test/integration/integration_admin_test.cc

@@ -81,17 +81,15 @@ TEST_P(IntegrationAdminTest, Admin) {
  BufferingStreamDecoderPtr response;
  EXPECT_EQ("404", request("admin", "GET", "/notfound", response));
  EXPECT_EQ("text/plain; charset=UTF-8", ContentType(response));
- EXPECT_NE(std::string::npos, response->body().find("invalid path. admin commands are:"))
- << response->body();
+ EXPECT_THAT(response->body(), HasSubstr("invalid path. admin commands are:"));
 
  EXPECT_EQ("200", request("admin", "GET", "/help", response));
  EXPECT_EQ("text/plain; charset=UTF-8", ContentType(response));
- EXPECT_NE(std::string::npos, response->body().find("admin commands are:")) << response->body();
+ EXPECT_THAT(response->body(), HasSubstr("admin commands are:"));
 
  EXPECT_EQ("200", request("admin", "GET", "/", response));
  EXPECT_EQ("text/html; charset=UTF-8", ContentType(response));
- EXPECT_NE(std::string::npos, response->body().find("<title>Envoy Admin</title>"))
- << response->body();
+ EXPECT_THAT(response->body(), HasSubstr("<title>Envoy Admin</title>"));
 
  EXPECT_EQ("200", request("admin", "GET", "/server_info", response));
  EXPECT_EQ("application/json", ContentType(response));
@@ -106,7 +104,7 @@ TEST_P(IntegrationAdminTest, Admin) {
  // are off by default.
  EXPECT_EQ("200", request("admin", "GET", "/stats/recentlookups", response));
  EXPECT_EQ("text/plain; charset=UTF-8", ContentType(response));
- EXPECT_THAT(response->body(), testing::HasSubstr("Lookup tracking is not enabled"));
+ EXPECT_THAT(response->body(), HasSubstr("Lookup tracking is not enabled"));
 
  // Now enable recent-lookups tracking and check that we get a count.
  EXPECT_EQ("200", request("admin", "POST", "/stats/recentlookups/enable", response));
@@ -119,7 +117,7 @@ TEST_P(IntegrationAdminTest, Admin) {
  EXPECT_EQ("200", request("admin", "POST", "/stats/recentlookups/disable", response));
  EXPECT_EQ("200", request("admin", "GET", "/stats/recentlookups", response));
  EXPECT_EQ("text/plain; charset=UTF-8", ContentType(response));
- EXPECT_THAT(response->body(), testing::HasSubstr("Lookup tracking is not enabled"));
+ EXPECT_THAT(response->body(), HasSubstr("Lookup tracking is not enabled"));
 
  EXPECT_EQ("200", request("admin", "GET", "/stats?usedonly", response));
  EXPECT_EQ("text/plain; charset=UTF-8", ContentType(response));

test/mocks/server/admin.h

@@ -24,6 +24,9 @@ class MockAdmin : public Admin {
  MOCK_METHOD(bool, addHandler,
  (const std::string& prefix, const std::string& help_text, HandlerCb callback,
  bool removable, bool mutates_server_state));
+ MOCK_METHOD(bool, addChunkedHandler,
+ (const std::string& prefix, const std::string& help_text, GenHandlerCb callback,
+ bool removable, bool mutates_server_state));
  MOCK_METHOD(bool, removeHandler, (const std::string& prefix));
  MOCK_METHOD(Network::Socket&, socket, ());
  MOCK_METHOD(ConfigTracker&, getConfigTracker, ());

test/server/admin/BUILD

@@ -49,6 +49,7 @@ envoy_cc_test(
  srcs = ["admin_filter_test.cc"],
  deps = [
  "//source/server/admin:admin_filter_lib",
+ "//source/server/admin:admin_lib",
  "//test/mocks/server:instance_mocks",
  "//test/test_common:environment_lib",
  ],

test/server/admin/admin_filter_test.cc

@@ -1,3 +1,4 @@
+#include "source/server/admin/admin.h"
 #include "source/server/admin/admin_filter.h"
 
 #include "test/mocks/server/instance.h"
@@ -14,7 +15,7 @@ namespace Server {
 
 class AdminFilterTest : public testing::TestWithParam<Network::Address::IpVersion> {
 public:
- AdminFilterTest() : filter_(adminServerCallback), request_headers_{{":path", "/"}} {
+ AdminFilterTest() : filter_(adminHandlerCallback), request_headers_{{":path", "/"}} {
  filter_.setDecoderFilterCallbacks(callbacks_);
  }
 
@@ -24,16 +25,12 @@ class AdminFilterTest : public testing::TestWithParam<Network::Address::IpVersio
  NiceMock<Http::MockStreamDecoderFilterCallbacks> callbacks_;
  Http::TestRequestHeaderMapImpl request_headers_;
 
- static Http::Code adminServerCallback(absl::string_view path_and_query,
- Http::ResponseHeaderMap& response_headers,
- Buffer::OwnedImpl& response, AdminFilter& filter) {
+ static Admin::HandlerPtr adminHandlerCallback(absl::string_view path_and_query,
+ AdminStream& admin_stream) {
  // silence compiler warnings for unused params
  UNREFERENCED_PARAMETER(path_and_query);
- UNREFERENCED_PARAMETER(response_headers);
- UNREFERENCED_PARAMETER(filter);
-
- response.add("OK\n");
- return Http::Code::OK;
+ UNREFERENCED_PARAMETER(admin_stream);
+ return AdminImpl::makeStaticTextHandler("OK\n", Http::Code::OK);
  }
 };
 

test/server/admin/admin_instance.cc

@@ -10,7 +10,7 @@ AdminInstanceTest::AdminInstanceTest()
  : address_out_path_(TestEnvironment::temporaryPath("admin.address")),
  cpu_profile_path_(TestEnvironment::temporaryPath("envoy.prof")),
  admin_(cpu_profile_path_, server_, false), request_headers_{{":path", "/"}},
- admin_filter_(admin_.createCallbackFunction()) {
+ admin_filter_(admin_.createHandlerFunction()) {
  std::list<AccessLog::InstanceSharedPtr> access_logs;
  Filesystem::FilePathAndType file_info{Filesystem::DestinationType::File, "/dev/null"};
  access_logs.emplace_back(new Extensions::AccessLoggers::File::FileAccessLog(

test/server/admin/admin_test.cc

@@ -123,6 +123,56 @@ TEST_P(AdminInstanceTest, CustomHandler) {
  EXPECT_EQ(Http::Code::Accepted, getCallback("/foo/bar", header_map, response));
 }
 
+class ChunkedHandler : public Admin::Handler {
+public:
+ Http::Code start(Http::ResponseHeaderMap&) override { return Http::Code::OK; }
+
+ bool nextChunk(Buffer::Instance& response) override {
+ response.add("Text ");
+ return ++count_ < 3;
+ }
+
+private:
+ uint32_t count_{0};
+};
+
+TEST_P(AdminInstanceTest, CustomChunkedHandler) {
+ auto callback = [](absl::string_view, AdminStream&) -> Admin::HandlerPtr {
+ Admin::HandlerPtr handler = Admin::HandlerPtr(new ChunkedHandler);
+ return handler;
+ };
+
+ // Test removable handler.
+ EXPECT_NO_LOGS(EXPECT_TRUE(admin_.addChunkedHandler("/foo/bar", "hello", callback, true, false)));
+ Http::TestResponseHeaderMapImpl header_map;
+ {
+ Buffer::OwnedImpl response;
+ EXPECT_EQ(Http::Code::OK, getCallback("/foo/bar", header_map, response));
+ EXPECT_EQ("Text Text Text ", response.toString());
+ }
+
+ // Test that removable handler gets removed.
+ EXPECT_TRUE(admin_.removeHandler("/foo/bar"));
+ Buffer::OwnedImpl response;
+ EXPECT_EQ(Http::Code::NotFound, getCallback("/foo/bar", header_map, response));
+ EXPECT_FALSE(admin_.removeHandler("/foo/bar"));
+
+ // Add non removable handler.
+ EXPECT_TRUE(admin_.addChunkedHandler("/foo/bar", "hello", callback, false, false));
+ EXPECT_EQ(Http::Code::OK, getCallback("/foo/bar", header_map, response));
+
+ // Add again and make sure it is not there twice.
+ EXPECT_FALSE(admin_.addChunkedHandler("/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"));
+ {
+ Buffer::OwnedImpl response;
+ EXPECT_EQ(Http::Code::OK, getCallback("/foo/bar", header_map, response));
+ EXPECT_EQ("Text Text Text ", response.toString());
+ }
+}
+
 TEST_P(AdminInstanceTest, RejectHandlerWithXss) {
  auto callback = [](absl::string_view, Http::HeaderMap&, Buffer::Instance&,
  AdminStream&) -> Http::Code { return Http::Code::Accepted; };