build: making admin optional by alyssawilk · Pull Request #23903 · envoyproxy/envoy

alyssawilk · 2022-11-08T21:15:14Z

Adding a build option to compile out Envoy's admin code.
The main API change is making the admin APIs OptRef instead of explicit references, which requires update to any code which adds config trackers for the admin console.

Risk Level: medium
Testing: new CI build with new flags
Docs Changes: n/a
Release Notes: not currently included
Platform Specific Features:
Fixes #23823

repokitteh-read-only · 2022-11-08T21:15:18Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #23903 was opened by alyssawilk.

see: more, trace.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

RyanTheOptimist

88 files? My goodness :)

Looks great other than a few nits.

RyanTheOptimist · 2022-11-09T22:49:45Z

bazel/README.md

 * http3/quic with --//bazel:http3=False
 * autolinking libraries with --define=library_autolink=disabled
 * admin HTML home page with `--define=admin_html=disabled`
+* admin functionlity with `--define=admin_functionaity=disabled`


nit: "admin_functionaity" => "admin_functionality", I think?

yep, thanks

RyanTheOptimist · 2022-11-09T22:53:59Z

ci/do_ci.sh


-  echo "Building and testing with wasm=wasmtime: ${TEST_TARGETS[*]}"
-  bazel_with_collection test "${BAZEL_BUILD_OPTIONS[@]}" --define wasm=wasmtime "${COMPILE_TIME_OPTIONS[@]}" -c dbg "${TEST_TARGETS[@]}" --test_tag_filters=-nofips --build_tests_only
+  echo "Building and testing with wasm=wasmtime: and admin_functionality disabled ${TEST_TARGETS[*]}"


nit: Maybe mention admin_html too, for completeness?

RyanTheOptimist · 2022-11-09T22:54:37Z

configs/BUILD

            "using_deprecated_config.yaml",
            "**/*.template.yaml",
            "freebind/freebind.yaml",
+            "envoy-tap-config.yaml",


Out of curiosity, why does this need to be added?

the tap filter has admin functionality. I didn't go through the exercise of carving that out, the entire tap filter simply won't compile.
Because of that we exempt tap here and add it back below as long as admin isn't disabled

RyanTheOptimist · 2022-11-09T22:55:59Z

source/common/config/config_provider_impl.cc

-  // ConfigTracker keys must be unique. We are asserting that no one has stolen the key
-  // from us, since the returned entry will be nullptr if the key already exists.
-  RELEASE_ASSERT(config_tracker_entry_, "");
+  if (admin.has_value()) {


nit: early return

if (!admin.has_value()) { return; } ...

Why not just use the ifdef?

Or do you also support (or plan to support) disabling admin at runtime via a runtime or command-line flag while still compiling it in?

I prefer most of the code compiling in both branches and minimal use of the ifdef. it means we can get coverage for more code for example

RyanTheOptimist · 2022-11-09T22:56:39Z

source/common/rds/route_config_provider_manager.cc

-  // from us, since the returned entry will be nullptr if the key already exists.
-  RELEASE_ASSERT(config_tracker_entry_, "");
+    : proto_traits_(proto_traits) {
+  if (admin.has_value()) {


nit: early return

RyanTheOptimist · 2022-11-09T22:59:43Z

source/exe/main_common.cc

 void MainCommonBase::adminRequest(absl::string_view path_and_query, absl::string_view method,
                                  const AdminRequestFn& handler) {
+  RELEASE_ASSERT(server_->admin().has_value(),
+                 "adminRequest called with admin support compiled out");


You'd know better than I would, but I wonder if instead of RELEASE_ASSERT if we should just ASSERT() in debug, and return in release? But I suspect you already thought about this.

Why would we want to assert at all? Can't we just call the handler with an http error code? Maybe a 501?

it's a bug to call admin requests with them compiled out. I think we should definitely crash in debug mode at least.

RyanTheOptimist · 2022-11-09T23:01:54Z

source/extensions/stat_sinks/hystrix/hystrix.cc

-            "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);
+  if (server.admin().has_value()) {


nit: early return

RyanTheOptimist · 2022-11-09T23:06:22Z

envoy/server/admin.h

+  /**
+   * Closes the listening socket for the admin.
+   */
+  virtual void closeSocket() PURE;


I can't seem to figure out why this new pure virtual method is needed. Oh is it because we're using std::unique_ptr<Admin> admin_ instead of std::unique_ptr<AdminImpl> admin_ in server.h? Do we need to make that change?

we do, or the server (which now has an API pointer because it may not know about the impl class) can't close the socket.

I believe you, but I don't yet understand how to connect all the dots :) Why does the server need an API pointer now when before it had an impl pointer?

RyanTheOptimist · 2022-11-09T23:08:14Z

source/server/config_validation/server.h

  // Server::Instance
-  Admin& admin() override { return *admin_; }
+  OptRef<Admin> admin() override {
+    return makeOptRefFromPtr(reinterpret_cast<Envoy::Server::Admin*>(admin_.get()));


I'm surprised to see reinterpret_cast here instead of static_cast. Does static_cast work here?

apparently - thought I tried that but must have been something else.

RyanTheOptimist · 2022-11-09T23:12:34Z

.../extensions/http/header_formatters/preserve_case/preserve_case_formatter_integration_test.cc

@@ -1,5 +1,7 @@
 #include "envoy/extensions/http/header_formatters/preserve_case/v3/preserve_case.pb.h"

+#include "source/extensions/filters/http/common/pass_through_filter.h"


Out of curiosity, how did you find this to be missing?

build failed with admin disabled flags. because Envoy doesn't IWYU sometimes when you remove one include or library you have to add others in unexpected places.

jmarantz · 2022-11-09T23:36:38Z

source/exe/main_common.cc

 void MainCommonBase::adminRequest(absl::string_view path_and_query, absl::string_view method,
                                  const AdminRequestFn& handler) {
+  RELEASE_ASSERT(server_->admin().has_value(),
+                 "adminRequest called with admin support compiled out");


Why would we want to assert at all? Can't we just call the handler with an http error code? Maybe a 501?

jmarantz · 2022-11-09T23:40:02Z

test/integration/base_integration_test.h

 #endif

+#ifndef ENVOY_ADMIN_FUNCTIONALITY
+#define DISABLE_IF_ADMIN_DISABLED return


nit: RETURN_IF_ADMIN_DISABLED may be more obvious at call-sites. If I read this in the middle of a function I might not know what DISABLE meant.

Also -- why not use this pattern in production code also? It's somewhat nice in a way :)

don't use in production code because we don't always want returns in the production code
for tests I agree I prefer RETURN but I'm following the pattern. I'm inclined to do a follow-up and replace them all together, WDYT?

sure makes sense.

jmarantz · 2022-11-09T23:42:05Z

source/common/config/config_provider_impl.cc

-  // ConfigTracker keys must be unique. We are asserting that no one has stolen the key
-  // from us, since the returned entry will be nullptr if the key already exists.
-  RELEASE_ASSERT(config_tracker_entry_, "");
+  if (admin.has_value()) {


Why not just use the ifdef?

Or do you also support (or plan to support) disabling admin at runtime via a runtime or command-line flag while still compiling it in?

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

jmarantz · 2022-11-10T18:04:00Z

source/exe/main_common.cc


 void MainCommonBase::adminRequest(absl::string_view path_and_query, absl::string_view method,
                                  const AdminRequestFn& handler) {
+  ASSERT(server_->admin().has_value(), "adminRequest called with admin support compiled out");


ok having this assert in in debug mode seems OK. But if we are not in debug mode should we return a 501 or something?

Seems like the fall-through case is going to jus not populate response_headers at all which might be confusing.

Alternatively -- I know you are not going to like this, but this is really a C++ API for folks that instantiate main_common themselves and can call methods on it. You could ifdef out the existence of this method entirely so calling it would be a build failure rather than a crash. But it's up to you.

I don't love it, but I'll take it over a custom error path no one actually wants but I'd otherwise feel obliged to write tests for =P

Great point!

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

RyanTheOptimist

LGTM, though I'm still curious about the AdminImpl => Admin change which necessitated the closeSocket method being added to the interface.

RyanTheOptimist · 2022-11-10T22:57:46Z

envoy/server/admin.h

+  /**
+   * Closes the listening socket for the admin.
+   */
+  virtual void closeSocket() PURE;


I believe you, but I don't yet understand how to connect all the dots :) Why does the server need an API pointer now when before it had an impl pointer?

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2022-11-15T13:45:15Z

Before the server had an admin pointer because it knew about the admin class. With this change we stick the impl behind the "is admin compiled in" flags, so the class tracks the interface (since the impl may be compiled out) and the interface needs close socket. alternately we could stick close socket behind an ifdef and down cast from the interface to the impl, but this seemed much cleaner to me.

alyssawilk · 2022-11-15T14:29:17Z

/retest

repokitteh-read-only · 2022-11-15T14:29:22Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #23903 (comment) was created by @alyssawilk.

see: more, trace.

RyanTheOptimist · 2022-11-15T15:15:48Z

Before the server had an admin pointer because it knew about the admin class. With this change we stick the impl behind the "is admin compiled in" flags, so the class tracks the interface (since the impl may be compiled out) and the interface needs close socket. alternately we could stick close socket behind an ifdef and down cast from the interface to the impl, but this seemed much cleaner to me.

AH! Yeah, ok that makes perfect sense (of course :>). Conditionally compiling would also seem fine but so does this. Thanks for the explanation. Still LGTM!

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the admin branch from 471e043 to 1bc0013 Compare November 9, 2022 14:19

build: making admin optional

cf61641

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the admin branch from 1bc0013 to cf61641 Compare November 9, 2022 20:29

alyssawilk marked this pull request as ready for review November 9, 2022 20:45

alyssawilk requested review from KBaichoo, adisuissa, ggreenway, jmarantz, lizan and mattklein123 as code owners November 9, 2022 20:45

alyssawilk assigned snowp, keith and RyanTheOptimist and unassigned snowp Nov 9, 2022

RyanTheOptimist reviewed Nov 9, 2022

View reviewed changes

jmarantz reviewed Nov 9, 2022

View reviewed changes

alyssawilk added 2 commits November 10, 2022 09:23

comments

e3f01ee

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Merge branch 'main' into admin

44549bc

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

jmarantz reviewed Nov 10, 2022

View reviewed changes

alyssawilk added 3 commits November 10, 2022 13:16

Merge branch 'main' into admin

eaec79b

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

attempt better bazel

a1d5c1a

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

ifdef

dba9757

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

jmarantz previously approved these changes Nov 10, 2022

View reviewed changes

RyanTheOptimist previously approved these changes Nov 10, 2022

View reviewed changes

format

4a3f628

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk dismissed stale reviews from RyanTheOptimist and jmarantz via 4a3f628 November 15, 2022 13:44

RyanTheOptimist previously approved these changes Nov 15, 2022

View reviewed changes

alyssawilk enabled auto-merge (squash) November 15, 2022 15:27

alyssawilk added 2 commits November 15, 2022 10:35

Merge branch 'main' into admin

a5b0c5c

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

merge/compiletime fix

50c5aba

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk dismissed RyanTheOptimist’s stale review via 50c5aba November 15, 2022 20:33

format :-(

f15e6e7

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

RyanTheOptimist approved these changes Nov 15, 2022

View reviewed changes

alyssawilk merged commit 21d0f00 into envoyproxy:main Nov 16, 2022

yanjunxiang-google mentioned this pull request Nov 16, 2022

Adding ECDS config dump support. #23902

Merged

qqustc mentioned this pull request Nov 22, 2022

update Envoy to 8a26244 (2022-11-21) envoyproxy/nighthawk#946

Merged

wbpcode mentioned this pull request Mar 31, 2023

some thoughts on the simplification of factory context #26476

Open

		@@ -1,5 +1,7 @@
		#include "envoy/extensions/http/header_formatters/preserve_case/v3/preserve_case.pb.h"

		#include "source/extensions/filters/http/common/pass_through_filter.h"

Conversation

alyssawilk commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Nov 8, 2022

Uh oh!

RyanTheOptimist 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

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!

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!

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!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Nov 15, 2022

Uh oh!

alyssawilk commented Nov 15, 2022

Uh oh!

alyssawilk commented Nov 8, 2022 •

edited

Loading