admin: Add an API on Server to initiate a programmatic admin request. by jmarantz · Pull Request #3667 · envoyproxy/envoy

jmarantz · 2018-06-19T14:24:22Z

Description:

Adds a programmatic API so an Envoy integration can provide admin content without exposing it on a network port. This partially addresses #3674 . That will be completely addressed by creating a mechanism to turn off the admin port.

Risk Level: Medium

Testing: //test/...

Docs Changes: will be done with the PR that enables turning off admin.
Release Notes: N/A

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

jmarantz · 2018-06-19T21:01:44Z

include/envoy/buffer/buffer.h

+   * Construct a flattened string from a buffer.
+   * @return the flattened string.
+   */
+  virtual std::string toString() const PURE;


Note: this method was previously implemented as a test utility; I made it slightly faster by right-sizing the string buffer first, and moving it into source/...

jmarantz · 2018-06-19T21:03:04Z

source/common/http/utility.h


 namespace Envoy {
 namespace Http {
+namespace Utility {


Note: I changed this from a class with all static methods into a namespace in order to be able to extract QueryParams into type that was accessible from interfaces in include/...

Textually it looks like this whole file changed but that's just because it lost 2 chars of indent and a lot of 'static' keywords. Really it just lost QueryParams, which is now in include/envoy/http/query_params.h

Thanks for the heads up, this is useful.

htuch · 2018-06-19T21:20:02Z

source/server/server.cc

+    Http::HeaderMapImpl response_headers;
+    std::string body;
+    Http::Code code = admin_->request(path_copy, params, method_copy, response_headers, body);
+    response_handler(code, response_headers, body);


Are you sure this is the pattern we want here? The callback is going to execute on some other thread (i.e. the main thread) to that of the caller. So, there will need to be some cross-thread synchronization happening. This has been a total nightmare in the FakeUpstream integration test code, and might lead to problematic main thread blocking.

An alternative would be to have the calling thread allocate a buffer, pass it to the post, wait using a CondVar for a response, and then take the buffer. That works nicely if the calling thread is not on an event loop. Alternatively, you post the response buffer to the event loop of the calling thread when done if async.

per f2f:

I will rename adminRequest to be adminRequestAsync, and add adminRequestBlocking, which will not use a function-pointer but block on the calling thread until the admin thread responds.

I could also implement this by replacing the network socket with some other sort of local socket, such as socketpair(). I think that might be a similar amount of work or slightly less, but might present an OS porting barrier in the future.

Why isn't this always just happening on the main thread? Do we really need to support post here at all?

The caller will likely come from another thread, at least in our case.

WDYT about handling the threading complexity in our private code then? I think for most people a use of this would be on the main thread...

It sounds reasonable to have this PR just define Admin::request() requiring it be called from the main thread, so there's no complexity in this PR.

And I realize now that sever()->dispatcher()->post() provides all the infrastructure I'd need to call this from a different thread. I could make Admin::request() call DispatcherImpl::isThreadSafe() I suppose, if that were public, but if that's not generally going to happen I suppose it's sufficient to just document it.

I'm a little reluctant to do the post/block in this PR because that would tie up the calling thread, when the system we are merging it into might want to continue to do other work on that thread while the post is being processed.

mattklein123 · 2018-06-19T22:49:32Z

include/envoy/server/instance.h

+   * @param path The admin request URL path as a string.
+   * @param params map of parameter names and values.
+   */
+  virtual void adminRequest(absl::string_view path, const Http::Utility::QueryParams& params,


The server instance already exposes an Admin object. Seems like this isn't needed and should actually get routed through the admin object?

Fair point, that shouldn simplify the PR. Thanks.

It turns out not to dramatically simplify the PR as I just pushed the mocking boilerplate out of server and into admin :)

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

…d expose Admin::request() as an interface. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2018-06-20T14:40:23Z

test/server/server_test.cc

          hooks_, restart_, stats_store_, fakelock_, component_factory_,
          std::make_unique<NiceMock<Runtime::MockRandomGenerator>>(), thread_local_)),
-      EnvoyException, "unable to read file: ")
+      EnvoyException, "unable to read file: ");


I'm not sure how server_test.cc got checked in without the semi-colon, because without it, the auto-formatter scrambles this test-method, or the 'format' test should fail.

mattklein123

This approach LGTM. Will defer to someone else for detailed review.

htuch

Looks good modulo comments.

htuch · 2018-06-21T22:32:43Z

source/common/buffer/buffer_impl.cc


 OwnedImpl::OwnedImpl(const void* data, uint64_t size) : OwnedImpl() { add(data, size); }

+std::string OwnedImpl::toString() const {


How is this different to linearize()? Do we need both variants?

I looked at linearize and though they seem similar, I don't see how either can be efficiently implemented on the other.

well, you could linearize and then create a std;;string with the now linearized buffer. This would involve two copies, but is still O(n) and might be fine for the applications we would use this for, with the assumption being that you don't want to do toString() or linearize for anything performance sensitive anyway. There's also the option of just doing an absl::string_view around the linearized buffer.

linearize mutates the buffer IIUC; I thought it made sense to have a simple observer that can return the fully flattened string.

Moreover it exists in the codebase now, in a test helper, and I'm mostly just moving it. However per your other comment -- I don't really need this in the interface, as everywhere in tests and in admin.cc where it's needed, I have an OwnedImpl. Removed.

I think mutating is a benefit here, it's kind of like defragging. At least naively, it seems you should be able to implement it about as efficiently as the toString() operation.

htuch · 2018-06-21T22:35:20Z

source/common/http/utility.h


 namespace Envoy {
 namespace Http {
+namespace Utility {


Thanks for the heads up, this is useful.

htuch · 2018-06-21T22:38:21Z

source/server/http/admin.h

  const Http::StreamDecoderFilterCallbacks& getDecoderFilterCallbacks() const override;
  const Http::HeaderMap& getRequestHeaders() const override;

+  static void populateFallbackResponseHeaders(Http::Code code, Http::HeaderMap& header_map);


Should this just be in the anonymous namespace in the .cc?

htuch · 2018-06-21T22:40:05Z

source/server/http/admin.cc

+  filter.decodeHeaders(request_headers, false);
+
+  // TODO(jmarantz): make QueryParams a real class and put this serializer there,
+  // along with proper URL escaping of the name and value.


Yeah, ++escaping.

TBH I think what I'd really like to do is change admin filter callbacks to take a const QueryParam& as an arg, rather than having each one parse it. Then serializing/parsing will be skipped and escaping will be unnecessary. Changing TODO. I don't want to tackle that in this PR though; will follow up.

htuch · 2018-06-21T22:42:33Z

test/server/http/admin_test.cc

+  Http::HeaderMapImpl response_headers;
+  std::string body;
+  EXPECT_EQ(Http::Code::OK,
+            admin_.request("/stats", Http::Utility::QueryParams({{"format", "json"}}), "GET",


Probably should have a test for > 1 query param, since there is slightly different logic (switches to & separator).

I couldn't find an admin handler that dealt with >1 query param, but I factored out the serialization to an http utility function and added tests for that to cover all the cases.

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

jmarantz · 2018-06-22T16:50:03Z

@mrice32 @kmyerson @akonradi FYI

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

htuch

LGTM, but I'm still somewhat skeptical we need toString() added to the buffer virtual interface. Needs mater merge.

htuch · 2018-06-25T00:18:29Z

source/common/buffer/buffer_impl.cc


 OwnedImpl::OwnedImpl(const void* data, uint64_t size) : OwnedImpl() { add(data, size); }

+std::string OwnedImpl::toString() const {


well, you could linearize and then create a std;;string with the now linearized buffer. This would involve two copies, but is still O(n) and might be fine for the applications we would use this for, with the assumption being that you don't want to do toString() or linearize for anything performance sensitive anyway. There's also the option of just doing an absl::string_view around the linearized buffer.

htuch · 2018-06-25T00:23:14Z

test/common/buffer/owned_impl_test.cc

  EXPECT_EQ(0, buffer.length());
 }

+TEST_F(OwnedImplTest, toString) {


Nit: s/toString/ToString/

ok, in this particular file most of the test methods start with lower-case letters, which may be more Envoyish? But lots of Envoy tests have test-methods starting with an upper-case letter. I'll just change all of them in this file to upper-case. SG?

htuch · 2018-06-25T00:23:32Z

test/common/buffer/owned_impl_test.cc

+  auto append = [&buffer](absl::string_view str) { buffer.add(str.data(), str.size()); };
+  append("Hello, ");
+  append("world!");
+  EXPECT_EQ("Hello, world!", buffer.toString());


Should we have more corner cases for the conversion?

Good question: what would you suggest? I actually don't know a ton about the underlying structure here.

At least empty, single span, two spans..

… simplfy abstract Buffer interface. Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

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

htuch

LGTM

mattklein123 · 2018-06-26T16:11:31Z

test/test_common/utility.h

   */
-  static std::string bufferToString(const Buffer::Instance& buffer);
+  static std::string bufferToString(const Buffer::OwnedImpl& buffer) {
+    // TODO(jmarantz): remove this indirection and update all ~53 call sites.


@jmarantz +1 can we do this as a cleanup sooner rather than later? Thank you.

jmarantz added 4 commits June 19, 2018 10:21

Add an API on Server to initiate a programmatic admin request.

034b5ff

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

Cleanup and get compiling with g++ for coverage.

1f45160

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

Copy args when posting. Add test for adminRequest with query-params.

d9ac410

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

Restore missing semicolon to get things to format properly.

a22b7de

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

jmarantz changed the title ~~WIP admin: Add an API on Server to initiate a programmatic admin request.~~ admin: Add an API on Server to initiate a programmatic admin request. Jun 19, 2018

jmarantz commented Jun 19, 2018

View reviewed changes

htuch reviewed Jun 19, 2018

View reviewed changes

htuch self-assigned this Jun 19, 2018

mattklein123 reviewed Jun 19, 2018

View reviewed changes

jmarantz added 2 commits June 20, 2018 09:40

Merge branch 'master' into admin-port-as-callback

61d532f

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

Remove the threading/posting layer from the new admin API, and instea…

355e071

…d expose Admin::request() as an interface. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz commented Jun 20, 2018

View reviewed changes

mattklein123 reviewed Jun 20, 2018

View reviewed changes

htuch suggested changes Jun 21, 2018

View reviewed changes

jmarantz added 2 commits June 22, 2018 09:28

Merge branch 'master' into admin-port-as-callback

a7e84e2

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

separately test query-param serializing, and other misc cleanups.

3b2f6b4

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

jmarantz requested a review from zuercher as a code owner June 22, 2018 14:17

Merge branch 'master' into admin-port-as-callback

be5aa96

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

htuch suggested changes Jun 25, 2018

View reviewed changes

jmarantz added 3 commits June 24, 2018 22:23

Merge branch 'master' into admin-port-as-callback, and experimentally…

1228879

… simplfy abstract Buffer interface. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Remove toString from the Buffer interface, but leaving it in Buffer::…

c7d9f6e

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

Add a few more corner-cases to the buffer toString test.

7b0ea3f

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

htuch approved these changes Jun 25, 2018

View reviewed changes

htuch merged commit bfd2615 into envoyproxy:master Jun 25, 2018

jmarantz deleted the admin-port-as-callback branch June 25, 2018 21:49

mattklein123 reviewed Jun 26, 2018

View reviewed changes


		OwnedImpl::OwnedImpl(const void* data, uint64_t size) : OwnedImpl() { add(data, size); }

		std::string OwnedImpl::toString() const {

Conversation

jmarantz commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz Jun 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Jun 22, 2018

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jmarantz commented Jun 19, 2018 •

edited

Loading

jmarantz Jun 19, 2018 •

edited

Loading

jmarantz Jun 25, 2018 •

edited

Loading