WatchDog Extension hook by KBaichoo · Pull Request #12416 · envoyproxy/envoy

KBaichoo · 2020-07-31T19:30:06Z

Commit Message: Establish an extension point for actions to run based on Watch Dog Events.
Additional Description:
Risk Level: Medium
Testing: unit tests
Docs Changes: N/A
Release Notes: Included.
Fixes #11388

Signed-off-by: Kevin Baichoo <kbaichoo@google.com> WIP, added calls to callbacks and structures to store registered callbacks. Signed-off-by: Kevin Baichoo <kbaichoo@google.com> Exposed watchdog actions from configuration, hooked up guarddog to be able to check the registry for callbacks. Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

…tion object (instead of a plain cb) for the registered actions in case we need internal state, set up scaffolding for test. Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Signed-off-by: Kevin Baichoo <kbaichoo@google.com> Slight cleanup. Signed-off-by: Kevin Baichoo <kbaichoo@google.com> Added an additional GuardDogActionFactory to clarify different action behaviors, and use RELEASE_ASSERT with messages instead of different signals. Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

repokitteh-read-only · 2020-07-31T19:30:12Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #12416 was opened by KBaichoo.

see: more, trace.

antoniovicente · 2020-07-31T19:36:26Z

api/envoy/config/bootstrap/v3/bootstrap.proto

  // If not specified the default is 0.
  type.v3.Percent multikill_threshold = 5;
+
+  // Register actions that will fire on given WatchDog events.


you may want to say something about any guarantees about ordering that you intend to provide. They are mostly relevant for the case of KILL/MULTIKILL where it may be useful to specify several debug related actions followed by an alternate FATAL action used to replace the default guarddog initiated fatal action.

antoniovicente

Looking pretty good. Some minor comments below.

include/envoy/server/guarddog_config.h

antoniovicente · 2020-07-31T19:44:20Z

source/server/guarddog_impl.h

  const std::chrono::milliseconds loop_interval_;
  Stats::Counter& watchdog_miss_counter_;
  Stats::Counter& watchdog_megamiss_counter_;
+  using EventToActionsMap = std::unordered_map<WatchDogAction::WatchdogEvent,


Use absl::flat_hash_map instead of std::unordered_map as requested by clang-tidy checks.

antoniovicente · 2020-07-31T19:47:09Z

source/server/guarddog_impl.cc

+        const auto& actions = config.wdActions();
+        for (const auto& action : actions) {
+          if (action.event() == WatchDogAction::UNKNOWN) {
+            ENVOY_LOG_MISC(error, "WatchDogAction specified with UNKNOWN event");


Should we throw an exception on unknown/unspecified event?

Will throw a ProtoValidationException() as that seemed to be the EnvoyException class that suits this scenario.

antoniovicente · 2020-07-31T19:53:37Z

test/server/guarddog_impl_test.cc

  NiceMock<Stats::MockStore> stats;
-  NiceMock<Configuration::MockMain> config(0, 0, 0, 0, 0);
+  std::vector<std::string> actions;
+  NiceMock<Configuration::MockMain> config(0, 0, 0, 0, 0, actions);


You could also write:
NiceMockConfiguration::MockMain config(0, 0, 0, 0, 0, {});

Ended up going with

NiceMockConfiguration::MockMain config(0, 0, 0, 0, 0, std::vector<std::string>{});

as with plain {} the compiler was struggling with the type deduction.

antoniovicente · 2020-07-31T19:55:09Z

test/server/guarddog_impl_test.cc

+// the events vector passed to it.
+// Instances of this class will be registered for GuardDogEvent through
+// TestGuardDogActionFactory.
+class LogGuardDogAction : public Configuration::GuardDogAction {


nit: RecordGuardDogAction may be a more accurate name.

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

api/envoy/config/bootstrap/v3/bootstrap.proto

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo · 2020-08-04T15:27:54Z

/ retest

repokitteh-read-only · 2020-08-04T15:27:57Z

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #12416 (comment) was created by @KBaichoo.

see: more, trace.

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

antoniovicente · 2020-08-04T22:12:50Z

include/envoy/server/guarddog.h

+#include "envoy/common/time.h"
+#include "envoy/config/bootstrap/v3/bootstrap.pb.h"
 #include "envoy/server/watchdog.h"
+#include "envoy/thread/thread.h"


nit: Are envoy/config/bootstrap/v3/bootstrap.pb.h and/or envoy/common/time.h used by this header?

Good catch, must have happened at some point in development (but it's unnecessary)

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo · 2020-08-05T17:08:56Z

/assign @dio

I think you're the on-call maintainer, so I'm supposed to assign it to you. PTAL.

Thanks!

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo · 2020-08-07T15:43:42Z

IIUC now, the on-call maintainer just triages and figures out who to assign the bug.

I think I'm suppose to ask: @envoyproxy/senior-maintainers to take a look. PTAL.

mattklein123

LGTM with small API/doc comments. Thank you!

/wait

mattklein123 · 2020-08-10T20:20:48Z

api/envoy/config/bootstrap/v3/bootstrap.proto

  option (udpa.annotations.versioning).previous_message_type = "envoy.config.bootstrap.v2.Watchdog";

+  message WatchdogAction {
+    // In priority events are fired from KILL, MULTIKILL, MEGAMISS and MISS.


What is an "in priority event" ? Typo? Clarify?

The events are fired in this order: KILL, MULTIKILL, MEGAMISS, MISS.

Changed it to the above to clarify.

api/envoy/config/bootstrap/v3/bootstrap.proto

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

mattklein123

LGTM modulo the doc nits. Don't feel you have to spend a lot of time on that. We can come back and fix them later so feel free to revert part of what you changed if it's not obvious. Thank you!

/wait

mattklein123 · 2020-08-12T00:16:27Z

docs/root/extending/extending.rst

-* Transport sockets
-* BoringSSL private key methods
+* :ref:`Transport sockets <envoy_v3_api_field_extensions.transport_sockets.tls.v3.CommonTlsContext.CertificateProvider.typed_config>`
+* :ref:`BoringSSL FIPS <arch_overview_ssl_fips>`


Is this an extension point? Did you mean to ref link to the private key methods or have a different title?

will revert the BoringSSL FIPs since I'm not entirely sure. I was basing this on pr #6326 but might be missing some context.

mattklein123 · 2020-08-12T00:16:37Z

docs/root/extending/extending.rst

+* :ref:`Transport sockets <envoy_v3_api_field_extensions.transport_sockets.tls.v3.CommonTlsContext.CertificateProvider.typed_config>`
+* :ref:`BoringSSL FIPS <arch_overview_ssl_fips>`
+* :ref:`Watchdog action <envoy_v3_api_msg_config.bootstrap.v3.Watchdog.WatchdogAction>`
+* :ref:`HTTP internal redirects <arch_overview_internal_redirects>`


Is this an extension point?

This was based off of pr #10908.

In https://github.com/envoyproxy/envoy/blob/master/api/envoy/config/route/v3/route_components.proto#L1722 there's a core.v3.TypedExtensionConfig. I linked arch_overview_internal_redirects as in the PR where the change was made http_connection_management.rst was modified primarily in that section it seems to accommodate.

I will revert here, but if you think this makes sense I'm happy to add this in another PR.

I see, OK. I guess I would mildly prefer that we ref link directly to the TypedExtensionConfig field to make it more obvious along with any arch docs. In any case, I don't want to hold up your PR, but any follow ups to improve the docs would be appreciated!

Added a separate PR for this: #12620

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

mattklein123

Awesome work! Just a few small comments and we can ship.

/wait

mattklein123 · 2020-08-12T18:06:49Z

api/envoy/config/bootstrap/v3/bootstrap.proto

+    // For KILL/MULTIKILL there is a default PANIC that will kill the process,
+    // but it might be useful to specify several debug actions, and possibly


Sorry can you clarify this a bit? Does specifying any config for KILL/MULTIKILL override the default PANIC action? It's not clear from the doc.

Changed it to specify the PANIC() call runs after the registered actions for that case if the process wasn't yet killed.

mattklein123 · 2020-08-12T18:09:05Z

source/server/guarddog_impl.cc

+          if (action.event() == WatchDogAction::UNKNOWN) {
+            throw ProtoValidationException("WatchDogAction specified with UNKNOWN event", action);


Please a PGV annotation on the proto for enum defined only. That will make this impossible and checked by the schema.

Great idea, thanks for pointing it out. Done.

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Signed-off-by: Kevin Baichoo <kbaichoo@google.com> Commit Message: Updated the extending envoy documentation to include a missing extension point. Additional Description: See PR #12416 for additional context of this. Risk Level: low Testing: built the documents to ensure they generate correctly. Docs Changes: Release Notes:

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

mattklein123

Thanks!

* master: (67 commits) logger: support log control in admin interface and command line option for Fancy Logger (envoyproxy#12369) test: fix http_timeout_integration_test flake (envoyproxy#12654) [fuzz]added an input check in writefilter fuzzer and added test cases (envoyproxy#12628) add 'explicit' restriction. (envoyproxy#12643) scoped_rds_integration_test migrate from api v2 to api v3. (envoyproxy#12633) fuzz: added fuzz test for listener filter tls_inspector (envoyproxy#12617) testing: fix multiple race conditions in simulated time tests (envoyproxy#12527) [tls] Move handshaking behavior into SslSocketInfo. (envoyproxy#12571) header: getting rid of exception-throwing behaviors in header files [the rest] (envoyproxy#12611) router: add new ratelimited retry backoff strategy (envoyproxy#12202) [redis_proxy] added a constraint for route.prefix().size() (envoyproxy#12637) network: add tcp listener backlog config (envoyproxy#12625) runtime: debug log that condition is always true when fractionalPercent numerator > denominator (envoyproxy#12068) WatchDog Extension hook (envoyproxy#12416) router: add dynamic metadata header formatter (envoyproxy#11858) statsd: revert visibility to public (envoyproxy#12621) Fix regression of /build_* in gitignore (envoyproxy#12630) Added a missing extension point to documentation. (envoyproxy#12620) Reverts proxy protocol test on windows (envoyproxy#12619) caching: Improved the tests and coverage of the CacheFilter tree (envoyproxy#12544) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

KBaichoo added 3 commits July 31, 2020 17:44

WIP: Switched to TypedConfig for WatchDogActions, create a GuardDogAc…

87b3504

…tion object (instead of a plain cb) for the registered actions in case we need internal state, set up scaffolding for test. Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

repokitteh-read-only bot added the api label Jul 31, 2020

KBaichoo mentioned this pull request Jul 31, 2020

Watchdog extension #12211

Closed

antoniovicente reviewed Jul 31, 2020

View reviewed changes

mattklein123 assigned antoniovicente Jul 31, 2020

Minor changes (naming, comments, etc.)

ace255c

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

antoniovicente reviewed Aug 4, 2020

View reviewed changes

api/envoy/config/bootstrap/v3/bootstrap.proto Outdated Show resolved Hide resolved

KBaichoo added 2 commits August 4, 2020 14:01

Clang tidy fix; added GuardDog's dispatcher to context.

ce6b715

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Spelling fix.

693be5c

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Kick CI

204b09f

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

antoniovicente previously approved these changes Aug 4, 2020

View reviewed changes

Removed unnecessary includes in guarddog.h

60014d8

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo dismissed antoniovicente’s stale review via 60014d8 August 4, 2020 22:55

KBaichoo added 2 commits August 5, 2020 03:24

formatting.

e9d0238

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Cross-platform test fixes.

c5574a9

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

repokitteh-read-only bot assigned dio Aug 5, 2020

Merge remote-tracking branch 'upstream/master' into wd-extension

7d8ae02

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

antoniovicente previously approved these changes Aug 7, 2020

View reviewed changes

mattklein123 self-assigned this Aug 7, 2020

mattklein123 requested changes Aug 10, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 10, 2020

Comment clarifications / update docs.

e6eea37

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo dismissed antoniovicente’s stale review via e6eea37 August 11, 2020 23:00

repokitteh-read-only bot removed the waiting label Aug 11, 2020

mattklein123 requested changes Aug 12, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 12, 2020

reverted doc nits.

9a0079d

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

repokitteh-read-only bot removed the waiting label Aug 12, 2020

mattklein123 requested changes Aug 12, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 12, 2020

Added PGV and clarified comments.

54fb656

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

repokitteh-read-only bot removed the waiting label Aug 12, 2020

KBaichoo mentioned this pull request Aug 12, 2020

Updated the extending envoy documentation to include a missing extension point. #12620

Merged

KBaichoo added 2 commits August 13, 2020 13:31

clangtidy fix.

9de27b0

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

merged in master.

7f4866e

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

mattklein123 approved these changes Aug 13, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Aug 13, 2020

mattklein123 merged commit 8c312f2 into envoyproxy:master Aug 13, 2020

		// For KILL/MULTIKILL there is a default PANIC that will kill the process,
		// but it might be useful to specify several debug actions, and possibly

		if (action.event() == WatchDogAction::UNKNOWN) {
		throw ProtoValidationException("WatchDogAction specified with UNKNOWN event", action);

Conversation

KBaichoo commented Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Jul 31, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

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!

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!

Uh oh!

KBaichoo commented Aug 4, 2020

Uh oh!

repokitteh-read-only bot commented Aug 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KBaichoo commented Aug 5, 2020

Uh oh!

KBaichoo commented Aug 7, 2020

Uh oh!

mattklein123 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!

Uh oh!

mattklein123 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!

mattklein123 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!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

KBaichoo commented Jul 31, 2020 •

edited

Loading