server: warn on admin mutations, GET requests, add mocking of messages #2912

jmarantz · 2018-03-27T16:51:15Z

Description:
When a mutating admin-console endpoint is reached, verify that it's a POST. For now, the operation succeeds but a warning log is printed.

To test this, log-mocking needed to be added, which was more involved than just checking the POST. The logging sink sent to spdlog::logger now delegates to another sink, which can be changed during tests, and mocked.

This is a step toward fixing #2763

Risk Level: Low

Testing: //test/...

Docs Changes:

Link to Data Plane PR]
[in progress, will update]

Release Notes: [in progress, will update]

If this change is user impacting you must add a release note via a discrete PR to
version_history.rst.

[Optional Fixes #Issue]

[Optional API Changes:]

Link to Data Plane PR]

Deprecated:
Deprecates supporting admin mutations with a GET.

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

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

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

…a test to be more intuitive. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2018-03-27T22:01:24Z

source/common/common/logger.cc

+  //
+  // TODO(jmarantz): filesystem file write should take string_view. Adding that requires
+  // finding a way to make that work for mocked filesystem writes.
+  log_file_->write(std::string(msg));


This is resolved in #2916 -- the second PR to go in should eliminate this conversion.

cleaned up now.

jmarantz

TWIMC: I am looking at the coverage compile failure now. But this should mostly be reviewable.

…he gmock types. Note: clang worked, but g++ is needed for coverage. Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

Demonstrates common code across multiple filters. Signed-off-by: Matt Klein <mklein@lyft.com>

…oxy#2878) This removes the need to package a config file with the Envoy binary. Signed-off-by: Nicolas Thiebaud <nicothieb@google.com>

jmarantz · 2018-03-28T22:09:51Z

per discussion with @htuch I added a snippet to the style guide about testing log warnings.

ggreenway · 2018-03-28T22:14:28Z

@jmarantz Would you please break this into 2 separate PRs, one for the logging changes, and one for the admin change?

jmarantz · 2018-03-28T22:21:26Z

@ggreenway sure -- I was thinking the logging changes would be dead code without the admin change, but I guess it's a pretty significant refactor on its own. Will do.

… post-admin-mutations Signed-off-by: Joshua Marantz <jmarantz@google.com>

ggreenway · 2018-03-28T22:17:44Z

source/common/common/logger.h

-  Filesystem::FileSharedPtr log_file_;
+  friend class SinkDelegate;
+
+  DelegatingLogSink() {}


DelegatingLogSink() = default;

ggreenway · 2018-03-28T22:20:33Z

source/server/server.cc

@@ -80,7 +81,7 @@ InstanceImpl::~InstanceImpl() {

  // Stop logging to file before all the AccessLogManager and its dependencies are
  // destructed to avoid crashing at shutdown.
-  Logger::Registry::getSink()->logToStdErr();
+  file_logger_.reset(nullptr);


nullptr is the default value; envoy convention is to omit it as an arg at the callsite.

ggreenway · 2018-03-28T22:45:01Z

source/common/common/logger.h

   */
  static void initialize(uint64_t log_level, const std::string& log_format,
-                         Thread::BasicLockable& lock);
+                         Thread::BasicLockable& lock) {


I have a minor preference for putting this in the .cc file because nearly every file has to parse this header.

ggreenway · 2018-03-28T22:46:47Z

Side note: Is the DCO bot complaining about a commit that you merged in from master? If so, I have no idea how to fix that...

jmarantz · 2018-03-29T02:46:32Z

egads -- I don't know how to solve that DCO either without erasing your comments by rebasing. Actually i don't really know how to solve this with rebasing either :)

In any event, I have started #2930, with just the log infrastructure changes and new tests. I will close this PR, at least temporarily, until that one is merged.

…e log-sinks (#2930) This is a fork of #2912, including only the logs mocking, and new tests for existing logging behavior in admin. Risk Level: Low Testing: //test/... Docs Changes: N/A Release Notes: N/A Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz added 7 commits March 23, 2018 11:19

Move a few internal APIs from std::string to absl::string_view.

10f225f

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

Make arg-names consistent.

6e19a68

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

Remove superfluous std::string conversion.

f985810

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

Merge branch 'master' into post-admin-mutations

372363c

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

server: warn on admin mutations, GET requests, add mocking of messages

2696a11

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

Formatting fixes, name changes.

0dc74f0

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

Split out lock-establishment from loglevel setting in logger registry…

900fc3d

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

jmarantz mentioned this pull request Mar 27, 2018

xds_rate_limit: implemented xds rate-limit and alerts #2783

Merged

Merge branch 'master' into post-admin-mutations

7e3c3f4

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

alyssawilk requested a review from ggreenway March 27, 2018 21:21

Fix another callback that was added after the initial commit. Rename …

fde6a65

…a test to be more intuitive. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz commented Mar 27, 2018

View reviewed changes

Manually create mock infra for MockLogSink, as g++ had trouble with t…

24975f0

…he gmock types. Note: clang worked, but g++ is needed for coverage. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz mentioned this pull request Mar 28, 2018

admin: add /runtime_modify endpoint and make runtime work without fs backing #2837

Merged

alyssawilk assigned ggreenway Mar 28, 2018

jmarantz and others added 6 commits March 28, 2018 09:50

Merge branch 'master' into post-admin-mutations

2cdc010

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

Merge branch 'master' into post-admin-mutations

14de253

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

Remove now-redundant std::string construction.

d92bf08

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

Add style guide reference to the ability to test log messages.

7ea82f2

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

repo reorg: move ext auth filters (envoyproxy#2923)

218da90

Demonstrates common code across multiple filters. Signed-off-by: Matt Klein <mklein@lyft.com>

Add a --config-yaml flag to pass the full bootstrap to Envoy (envoypr…

5fcc829

…oxy#2878) This removes the need to package a config file with the Envoy binary. Signed-off-by: Nicolas Thiebaud <nicothieb@google.com>

Merge branch 'post-admin-mutations' of github.com:jmarantz/envoy into…

cd177b0

… post-admin-mutations Signed-off-by: Joshua Marantz <jmarantz@google.com>

ggreenway requested changes Mar 28, 2018

View reviewed changes

jmarantz mentioned this pull request Mar 29, 2018

server: Create a mocking infrastructure for logging, by delegating the log-sinks #2930

Merged

jmarantz closed this Mar 29, 2018

jmarantz mentioned this pull request Apr 3, 2018

server: Warn in logs on admin mutations via GET. Mutations should be POSTed. #2971

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: warn on admin mutations, GET requests, add mocking of messages #2912

server: warn on admin mutations, GET requests, add mocking of messages #2912

jmarantz commented Mar 27, 2018

jmarantz Mar 27, 2018

jmarantz Mar 28, 2018

jmarantz left a comment

jmarantz commented Mar 28, 2018

ggreenway commented Mar 28, 2018

jmarantz commented Mar 28, 2018

ggreenway Mar 28, 2018

ggreenway Mar 28, 2018

ggreenway Mar 28, 2018

ggreenway commented Mar 28, 2018

jmarantz commented Mar 29, 2018 •

edited

Loading

server: warn on admin mutations, GET requests, add mocking of messages #2912

server: warn on admin mutations, GET requests, add mocking of messages #2912

Conversation

jmarantz commented Mar 27, 2018

jmarantz Mar 27, 2018

Choose a reason for hiding this comment

jmarantz Mar 28, 2018

Choose a reason for hiding this comment

jmarantz left a comment

Choose a reason for hiding this comment

jmarantz commented Mar 28, 2018

ggreenway commented Mar 28, 2018

jmarantz commented Mar 28, 2018

ggreenway Mar 28, 2018

Choose a reason for hiding this comment

ggreenway Mar 28, 2018

Choose a reason for hiding this comment

ggreenway Mar 28, 2018

Choose a reason for hiding this comment

ggreenway commented Mar 28, 2018

jmarantz commented Mar 29, 2018 • edited Loading

jmarantz commented Mar 29, 2018 •

edited

Loading