config: added gRPC-based filter to access_log by crockeo · Pull Request #5682 · envoyproxy/envoy

crockeo · 2019-01-22T17:27:16Z

Description: Creates an access log filter named GrpcStatusFilter that allows a set of configured gRPC statuses to be passed though from logging.

Risk Level: Medium, adds a new filter.

Testing: Added unit tests to //test/common/access_log:access_log_impl_test.

Docs Changes: Autogenerated docs changes from comments in //api/envoy/config/filter/accesslog/v2/accesslog.proto

Release Notes:
Fixes #5567

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

… to work on response trailers Signed-off-by: Cerek Hillen <cerekh@gmail.com>

venilnoronha

Thanks for contributing!

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

source/common/access_log/access_log_impl.h

source/common/grpc/status.cc

source/common/grpc/status.h

test/common/access_log/access_log_impl_test.cc

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

… the HTTP status Signed-off-by: Cerek Hillen <cerekh@gmail.com>

crockeo · 2019-01-23T15:22:19Z

@venilnoronha I added a test to check on valid HTTP to gRPC status code mappings for those specified in the gRPC specification. There is also an entry to check that HTTP status code 200 is not errantly mapped to gRPC status code ok. Should I also add tests for all of the other HTTP status codes?

junr03

Thanks for working on this! A few changes, a test addition, and an idea.

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

include/envoy/grpc/status.h

source/common/access_log/access_log_impl.h

source/common/grpc/status.cc

test/common/access_log/access_log_impl_test.cc

crockeo · 2019-01-23T20:13:46Z

I mentioned this in a comment on the original issue, but is there a canonical way to target an individual build (e.g. //test/common/access_log/access_log_impl_test.h) through the CI tools? A full build takes prohibitively long, but I keep running into issues where clang compiles, and g++ doesn't.

venilnoronha · 2019-01-23T23:28:20Z

@crockeo see https://github.com/envoyproxy/envoy/blob/master/bazel/README.md#testing-envoy-with-bazel.

… to pair, both to prevent g++ compiler errors. Signed-off-by: Cerek Hillen <cerekh@gmail.com>

crockeo · 2019-01-24T00:44:16Z

Thanks @venilnoronha! That's how I've been running my tests, but I don't see anything on specifically instructing Bazel to use a certain compiler.

I checked in //bazel/cc_configure.bzl and it looks like it should use the CXX compiler variable, but I installed g++, ran CXX=<path to G++> bazel test ... and still wasn't able to get it working. I'll give it another shot at some point.

For now I'm just checking specific features (e.g. constructing std::tuple through uniform initialization) across different compilers before I add them to the codebase.

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

…the filter will infer UNKNOWN Signed-off-by: Cerek Hillen <cerekh@gmail.com>

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

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

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

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

source/common/access_log/access_log_impl.cc

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

venilnoronha

Looks much better. I was wondering if you could add a test for exclude: false?

source/common/access_log/access_log_impl.cc

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

crockeo · 2019-02-05T12:51:06Z

Forgot that the updated CI scripts needed to be merged 😄

lizan

LGTM modulo one nit

test/common/access_log/access_log_impl_test.cc

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

crockeo · 2019-02-07T18:32:15Z

@junr03 I think this PR is ready for a final check.

mattklein123

Very nice. 2 small requests than LGTM. Thank you!

/wait

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

source/common/access_log/access_log_impl.cc

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

clicked button by accident

junr03 · 2019-02-12T18:09:59Z

@crockeo looks like the mac failure is valid, and there is now a conflict with the release doc.

crockeo · 2019-02-12T18:13:47Z

I'll fix the conflict and also look into the mac failure.

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

mattklein123

LGTM pending fixes.

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

crockeo · 2019-02-12T19:05:28Z

source/common/access_log/access_log_impl.cc

-                                Grpc::Utility::httpToGrpcStatus(info.responseCode().value()))
-                          : absl::nullopt,
-  };
+  const std::array<absl::optional<Grpc::Status::GrpcStatus>, 3> optional_statuses = {{


I couldn't repro compile errors locally, but it looked like it was expecting braces around each individual object in the array. To do so I needed to add the double brace around the entire initializer list.

Let me know if there's a better way to do it.

mattklein123

Nice!

Signed-off-by: Cerek Hillen <cerekh@gmail.com> Signed-off-by: Fred Douglas <fredlas@google.com>

crockeo added 8 commits January 22, 2019 12:23

Defined GrpcStatusFilter proto

f7ed72d

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

Added a function to convert from grpc name to Status::GrpcStatus

0923e1a

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

Implemented GrpcStatusFilter

4cec844

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

Added validation to GrpcStatusFilter

c9b7c1c

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

Implemented validation-related tests

0f73b01

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

Added tests to check that the filter blocks the correct traffic

6486b4d

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

Added documentation to GrpcStatusFilter

d8fc6d4

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

Finished testing on request headers, did heavy lifting to modify code…

0de9b81

… to work on response trailers Signed-off-by: Cerek Hillen <cerekh@gmail.com>

junr03 self-assigned this Jan 22, 2019

venilnoronha reviewed Jan 22, 2019

View reviewed changes

crockeo added 4 commits January 22, 2019 18:26

Added explicit hash to statuses_ to compile correctly on GCC 5.4.0

188b155

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

Fixed issue with missing hole in HttpGrpcAccessLogTest frame

2ebb7d2

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

Addressed PR comments, aside from testing request.

c06354d

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

Added a test to ensure the filter infers the correct gRPC status from…

93f4a2e

… the HTTP status Signed-off-by: Cerek Hillen <cerekh@gmail.com>

junr03 suggested changes Jan 23, 2019

View reviewed changes

Changed initializer list to uniform initialization, and changed tuple…

8e5a3d1

… to pair, both to prevent g++ compiler errors. Signed-off-by: Cerek Hillen <cerekh@gmail.com>

crockeo added 3 commits January 23, 2019 19:48

Addressed comment updates

ce9a103

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

Added test to check that, when lacking a gRPC status or HTTP status, …

e94e234

…the filter will infer UNKNOWN Signed-off-by: Cerek Hillen <cerekh@gmail.com>

Fixed s/respone/response/

58bac8c

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

venilnoronha reviewed Jan 24, 2019

View reviewed changes

api/envoy/config/filter/accesslog/v2/accesslog.proto Outdated Show resolved Hide resolved

Added test to check status Grpc::Utility::nameToGrpcStatus validity

cab43ed

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

lizan suggested changes Jan 24, 2019

View reviewed changes

api/envoy/config/filter/accesslog/v2/accesslog.proto Outdated Show resolved Hide resolved

source/common/access_log/access_log_impl.cc Outdated Show resolved Hide resolved

crockeo added 2 commits January 24, 2019 09:43

Added option to exclude based on gRPC status code

93eb881

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

Included check for response_headers

a562461

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

venilnoronha reviewed Jan 24, 2019

View reviewed changes

source/common/access_log/access_log_impl.cc Show resolved Hide resolved

Break for optimization!

631b44c

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

mattklein123 self-assigned this Feb 5, 2019

lizan previously approved these changes Feb 6, 2019

View reviewed changes

test/common/access_log/access_log_impl_test.cc Outdated Show resolved Hide resolved

lizan added the waiting label Feb 6, 2019

Removed unused sstream include

968b011

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

crockeo dismissed lizan’s stale review via 968b011 February 7, 2019 13:18

repokitteh-read-only bot removed the waiting label Feb 7, 2019

lizan previously approved these changes Feb 12, 2019

View reviewed changes

mattklein123 requested changes Feb 12, 2019

View reviewed changes

api/envoy/config/filter/accesslog/v2/accesslog.proto Show resolved Hide resolved

source/common/access_log/access_log_impl.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Feb 12, 2019

crockeo added 2 commits February 12, 2019 11:52

Changed vector to array

0915542

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

Added release note to version history

7625146

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

crockeo dismissed lizan’s stale review via 7625146 February 12, 2019 17:27

repokitteh-read-only bot removed the waiting label Feb 12, 2019

junr03 previously approved these changes Feb 12, 2019

View reviewed changes

Merge branch 'master' of github.com:envoyproxy/envoy into dev

99db72b

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

mattklein123 reviewed Feb 12, 2019

View reviewed changes

Fixed mac-specific error with -Wmissing-braces

ad58a61

Signed-off-by: Cerek Hillen <cerekh@gmail.com>

crockeo commented Feb 12, 2019

View reviewed changes

mattklein123 approved these changes Feb 12, 2019

View reviewed changes

mattklein123 merged commit a5e6ed7 into envoyproxy:master Feb 12, 2019

crockeo deleted the dev branch February 12, 2019 23:21

fredlas pushed a commit to fredlas/envoy that referenced this pull request Mar 5, 2019

config: added gRPC-based filter to access_log (envoyproxy#5682)

c81c6fd

Signed-off-by: Cerek Hillen <cerekh@gmail.com> Signed-off-by: Fred Douglas <fredlas@google.com>

TanOfAllCodes mentioned this pull request Jul 8, 2019

How to Include GRPC Status Codes in Envoy Access Logs? istio/istio#15358

Closed

repokitteh-read-only bot added the v2-freeze label Jul 6, 2020

antoniovicente mentioned this pull request Jul 6, 2020

Replace std::unordered_map/set with absl containers #11879

Merged

Conversation

crockeo commented Jan 22, 2019

Uh oh!

venilnoronha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

crockeo commented Jan 23, 2019

Uh oh!

junr03 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

crockeo commented Jan 23, 2019

Uh oh!

venilnoronha commented Jan 23, 2019

Uh oh!

crockeo commented Jan 24, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

venilnoronha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

crockeo commented Feb 5, 2019

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

crockeo commented Feb 7, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

junr03 commented Feb 12, 2019

Uh oh!

crockeo commented Feb 12, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

crockeo Feb 12, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants