access_log: add extension filter to allow runtime filter registration. by efimki · Pull Request #7435 · envoyproxy/envoy

efimki · 2019-07-01T17:46:24Z

Signed-off-by: Misha Efimov mef@google.com

Description: Add AccessLog ExtensionFilter to allow runtime filter registration.
Risk Level: Medium
Testing: unit test
Docs Changes: Add AccessLog ExtensionFilter documentation.
Release Notes: updated.
Fixes #7326

Signed-off-by: Misha Efimov <mef@chromium.org>

efimki · 2019-07-01T17:47:25Z

This is a WIP of my first PR, and it is NOT ready for review yet.

junr03 · 2019-07-01T17:51:48Z

Thanks @efimki. I will take a look when you are ready!

/wait

Signed-off-by: Misha Efimov <mef@google.com>

…xtension Signed-off-by: Misha Efimov <mef@google.com>

Signed-off-by: Misha Efimov <mef@google.com>

efimki · 2019-07-09T19:55:41Z

I think it is now ready for review. This is my first Envoy PR, so I apologize if I've missed something.

Signed-off-by: Misha Efimov <mef@google.com>

source/common/access_log/access_log_impl.h

test/common/access_log/access_log_impl_test.cc

source/common/access_log/access_log_impl.h

source/common/access_log/access_log_impl.cc

test/common/access_log/access_log_impl_test.cc

Signed-off-by: Misha Efimov <mef@google.com>

efimki

Hi Auni, thanks for your comments!
I've addressed them, PTAL.

efimki · 2019-07-11T19:05:20Z

@junr03 Could you take a look when you have a chance? I'll appreciate your input. Thanks in advance!

Signed-off-by: Misha Efimov <mef@google.com>

…xtension Signed-off-by: Misha Efimov <mef@google.com>

Signed-off-by: Misha Efimov <mef@google.com>

junr03 · 2019-07-19T16:24:54Z

sorry for the delay. Will get to this today

junr03

Very nice, good extension point. Just a few comments. @htuch can you take a look given you have been chatting with @efimki offline?

junr03 · 2019-07-19T16:35:55Z

api/envoy/config/filter/accesslog/v2/accesslog.proto

+  // Custom configuration that depends on the filter being instantiated.
+  oneof config_type {
+    google.protobuf.Struct config = 2;
+    google.protobuf.Any typed_config = 3;


I've found @htuch article [1] that suggests that Struct and Any have different benefits in terms of type checking and flexibility. It also matches pattern used in message AccessLog.

https://blog.envoyproxy.io/dynamic-extensibility-and-protocol-buffers-dcd0bf0b8801

I thought the plan here is that we aren't going to allow Struct for any new things, as a Struct can be embedded in any Any? @htuch?

I kind of feel we should just be consistent in v2 and in UDPA we can make a clean break.

source/common/access_log/access_log_impl.h

test/common/access_log/access_log_impl_test.cc

Signed-off-by: Misha Efimov <mef@google.com>

efimki

@junr03 thanks a lot for review!

source/common/access_log/access_log_impl.h

test/common/access_log/access_log_impl_test.cc

efimki · 2019-07-19T19:36:47Z

api/envoy/config/filter/accesslog/v2/accesslog.proto

+  // Custom configuration that depends on the filter being instantiated.
+  oneof config_type {
+    google.protobuf.Struct config = 2;
+    google.protobuf.Any typed_config = 3;


I've found @htuch article [1] that suggests that Struct and Any have different benefits in terms of type checking and flexibility. It also matches pattern used in message AccessLog.

https://blog.envoyproxy.io/dynamic-extensibility-and-protocol-buffers-dcd0bf0b8801

htuch

LGTM modulo nits.
/wait

source/common/access_log/access_log_impl.cc

htuch · 2019-07-25T12:16:03Z

test/common/access_log/access_log_impl_test.cc

+  }
+
+private:
+  size_t current_ = 0;


Nit: prefer using unsigned explicit sized integers in Envoy, rather than size_t (which you can use for buffer sizes etc.)

Done, thanks for suggestion!

htuch · 2019-07-25T12:16:26Z

test/common/access_log/access_log_impl_test.cc

+        config, Envoy::ProtobufMessage::getNullValidationVisitor(), *this);
+    const Json::ObjectSharedPtr filter_config =
+        MessageUtil::getJsonObjectFromMessage(*factory_config);
+    return FilterPtr{new SampleExtensionFilter(filter_config->getInteger("rate"))};


Nit: you can make all these Ptr{ new .. just std::make_unique

Thanks for suggestion! Done.

htuch · 2019-07-25T12:17:49Z

test/common/access_log/access_log_impl_test.cc

+  // For rate=5 expect 1st request to be recorded, 2nd-5th skipped, and 6th recorded.
+  EXPECT_CALL(*file_, write(_));
+  logger->log(&request_headers_, &response_headers_, &response_trailers_, stream_info_);
+  for (int i = 2; i <= 5; ++i) {


Nit: just count from 0 to 3?

Done. I was trying to express that requests 2-5 are skipped.

Signed-off-by: Misha Efimov <mef@google.com>

repokitteh-read-only · 2019-07-29T17:54:56Z

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

🐱

Caused by: #7435 was synchronize by efimki.

see: more, trace.

Signed-off-by: Misha Efimov <mef@google.com>

repokitteh-read-only · 2019-07-29T19:34:40Z

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

🐱

Caused by: #7435 was synchronize by efimki.

see: more, trace.

Signed-off-by: Misha Efimov <mef@google.com>

repokitteh-read-only · 2019-07-29T19:45:42Z

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

🐱

Caused by: #7435 was synchronize by efimki.

see: more, trace.

Signed-off-by: Misha Efimov <mef@google.com>

repokitteh-read-only · 2019-07-29T20:22:38Z

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

🐱

Caused by: #7435 was synchronize by efimki.

see: more, trace.

mattklein123

LGTM with one small doc request. Thank you!

/wait

mattklein123 · 2019-07-29T23:54:29Z

docs/root/intro/arch_overview/observability/access_logging.rst

+Access log filters
+==================
+
+Envoy supports several built-in access log filters. Currently supported filters are:


Can you ref link into the actual filter config docs? It's up to you if you want to replicate all of the filters here as they may get out of date vs. just link to the filter structure. I mostly just wanted to call out that extension filters are available. https://www.envoyproxy.io/docs/envoy/latest/api-v2/config/filter/accesslog/v2/accesslog.proto#envoy-api-msg-config-filter-accesslog-v2-accesslog

I'll be happy to ref link into the actual filter config docs, but I can't figure out how.
Is there an example that I could follow?

@efimki see how this is done here: https://github.com/envoyproxy/envoy/blame/master/docs/root/configuration/runtime.rst#L22

Thanks! I think I've done the right thing now, but not 100% sure.

I'm totally confused about msg vs field in references like <envoy_api_msg_config.filter.accesslog.v2.AccessLogFilter> <envoy_api_field_config.filter.accesslog.v2.AccessLogFilter.extension_filter>.

Signed-off-by: Misha Efimov <mef@google.com>

repokitteh-read-only · 2019-07-31T15:31:27Z

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

🐱

Caused by: #7435 was synchronize by efimki.

see: more, trace.

mattklein123

Thanks looks great. Sorry one more thing: please add a release note otherwise LGTM!

/wait

Signed-off-by: Misha Efimov <mef@google.com>

repokitteh-read-only · 2019-07-31T20:36:17Z

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

🐱

Caused by: #7435 was synchronize by efimki.

see: more, trace.

htuch · 2019-07-31T21:49:55Z

docs/root/intro/version_history.rst

 * tls: added verification of IP address SAN fields in certificates against configured SANs in the
  certificate validation context.
 * upstream: added network filter chains to upstream connections, see :ref:`filters<envoy_api_field_Cluster.filters>`.
+* config: added access log :ref:`extension filter<envoy_api_field_config.filter.accesslog.v2.AccessLogFilter.extension_filter>`.


Nit: needs to be alpha sorted :)

Of course. :)
Done.

Signed-off-by: Misha Efimov <mef@google.com>

repokitteh-read-only · 2019-07-31T22:24:08Z

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

🐱

Caused by: #7435 was synchronize by efimki.

see: more, trace.

htuch

Thanks!

mattklein123

Thank you!

Add AccessLog ExtensionFilter to allow runtime filter registration.

38a62a0

Signed-off-by: Misha Efimov <mef@chromium.org>

repokitteh-read-only bot added the waiting label Jul 1, 2019

junr03 self-assigned this Jul 1, 2019

efimki added 2 commits July 2, 2019 12:04

Kick CI

4e73a57

Signed-off-by: Misha Efimov <mef@google.com>

Merge remote-tracking branch 'upstream/master' into envoy_accesslog_e…

254c48e

…xtension Signed-off-by: Misha Efimov <mef@google.com>

repokitteh-read-only bot removed the waiting label Jul 8, 2019

Extract ExtensionFilterFactory and add more tests.

b33823e

Signed-off-by: Misha Efimov <mef@google.com>

efimki marked this pull request as ready for review July 9, 2019 19:54

Fix clang-tidy errors.

3a6c8f0

Signed-off-by: Misha Efimov <mef@google.com>

efimki changed the title ~~Add AccessLog ExtensionFilter to allow runtime filter registration.~~ access_log: add extension filter to allow runtime filter registration. Jul 9, 2019

aunu53 reviewed Jul 10, 2019

View reviewed changes

Address Auni's comments.

38e1b96

Signed-off-by: Misha Efimov <mef@google.com>

efimki commented Jul 10, 2019

View reviewed changes

efimki added 7 commits July 17, 2019 11:05

Kick CI

7c12e52

Signed-off-by: Misha Efimov <mef@google.com>

Merge remote-tracking branch 'upstream/master' into envoy_accesslog_e…

a8c9e91

…xtension Signed-off-by: Misha Efimov <mef@google.com>

Kick CI

bfdd6db

Signed-off-by: Misha Efimov <mef@google.com>

Kick CI

02790b4

Signed-off-by: Misha Efimov <mef@google.com>

Kick CI

3bbbffd

Signed-off-by: Misha Efimov <mef@google.com>

Kick CI

4692fe5

Signed-off-by: Misha Efimov <mef@google.com>

Kick CI

f2757de

Signed-off-by: Misha Efimov <mef@google.com>

junr03 reviewed Jul 19, 2019

View reviewed changes

Add a @return comment.

bf48937

Signed-off-by: Misha Efimov <mef@google.com>

efimki commented Jul 19, 2019

View reviewed changes

htuch suggested changes Jul 25, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 25, 2019

Added access log filters documentation.

b68f415

Signed-off-by: Misha Efimov <mef@google.com>

efimki dismissed stale reviews from junr03 and htuch via b68f415 July 29, 2019 17:54

repokitteh-read-only bot removed the waiting label Jul 29, 2019

Fix API reference.

13d7d42

Signed-off-by: Misha Efimov <mef@google.com>

Remove API reference.

ba2675d

Signed-off-by: Misha Efimov <mef@google.com>

mattklein123 added the waiting label Jul 29, 2019

Fix docs error.

76283ef

Signed-off-by: Misha Efimov <mef@google.com>

repokitteh-read-only bot removed the waiting label Jul 29, 2019

mattklein123 requested changes Jul 29, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 29, 2019

Add links to proto.

c39b78c

Signed-off-by: Misha Efimov <mef@google.com>

repokitteh-read-only bot removed the waiting label Jul 31, 2019

mattklein123 requested changes Jul 31, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 31, 2019

Updated relnotes.

3fc3396

Signed-off-by: Misha Efimov <mef@google.com>

repokitteh-read-only bot removed the waiting label Jul 31, 2019

htuch reviewed Jul 31, 2019

View reviewed changes

Alpha sort.

d9eb881

Signed-off-by: Misha Efimov <mef@google.com>

htuch approved these changes Jul 31, 2019

View reviewed changes

mattklein123 approved these changes Jul 31, 2019

View reviewed changes

mattklein123 merged commit a949328 into envoyproxy:master Jul 31, 2019

efimki deleted the envoy_accesslog_extension branch August 1, 2019 15:50

Conversation

efimki commented Jul 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efimki commented Jul 1, 2019

Uh oh!

junr03 commented Jul 1, 2019

Uh oh!

efimki commented Jul 9, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

efimki left a comment

Choose a reason for hiding this comment

Uh oh!

efimki commented Jul 11, 2019

Uh oh!

junr03 commented Jul 19, 2019

Uh oh!

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

Uh oh!

Uh oh!

efimki left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

repokitteh-read-only bot commented Jul 29, 2019

Uh oh!

repokitteh-read-only bot commented Jul 29, 2019

Uh oh!

repokitteh-read-only bot commented Jul 29, 2019

Uh oh!

repokitteh-read-only bot commented Jul 29, 2019

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

efimki commented Jul 1, 2019 •

edited

Loading