proto: Add PATCH method to RequestMethod enum

[glebmish]

Description: Add PATCH method to protobuf so that ALS can receive and log it properly
Risk Level: Low
Testing: None
Docs Changes: -
Release Notes: -
Fixes #6694

[venilnoronha]

The change looks good. I'm wondering if we can solidify this by adding a test. @junr03 thoughts?

[glebmish]

The change looks good. I'm wondering if we can solidify this by adding a test. @junr03 thoughts?

Yes, it would be good. Couldn't think of appropriate place to test it though, will be glad to hear suggestions.

[venilnoronha]

@glebmish somewhere under https://github.com/envoyproxy/envoy/blob/master/test/common/access_log/?

[junr03]

I agree with @venilnoronha, it would be nice to add some testing here so that we verify completeness. There is some prior art around response flags that you can take inspiration from. Let us know if you need some pointers.

[glebmish]

@glebmish somewhere under https://github.com/envoyproxy/envoy/blob/master/test/common/access_log/?

will it be tested there? @junr03 said that text logs work without proto so it seems that tests for access_log will work anyway. It probably makes sense to test the part that works with proto to make sure it really works.

Or do this tests actually work with proto and I'm missing something here?

[junr03]

I think @venilnoronha linked for inspiration, not necessarily suggesting that the test would be written there. Yes, those tests are not for the ALS, they are for text logs. I haven't looked closely at the test suite for the ALS, let me know if you'd like me to take a look for guidance.

[glebmish]

Added test similar to the part below that checks METHOD_UNSPECIFIED

However, I see that in the part below "request_headers" is initialized but not passed to access_log_->log (nullptr is passed instead). And when I modified it to pass headers, new field "request_headers_bytes" appeared in the result and test failed. I guess it just doesn't work correctly right now, what do you think?

[dio]

@glebmish what if:

Create a void expectLogRequestMethod(const std::string& request_method) as a member method of HttpGrpcAccessLogTest class and have a TEST_F e.g.

TEST_F(HttpGrpcAccessLogTest, LogWithRequestMethod) {
  InSequence s;
  expectLogRequestMethod("GET");
  expectLogRequestMethod("HEAD");
  expectLogRequestMethod("POST");
  expectLogRequestMethod("PUT");
  expectLogRequestMethod("DELETE");
  expectLogRequestMethod("CONNECT");
  expectLogRequestMethod("OPTIONS");
  expectLogRequestMethod("TRACE");
  expectLogRequestMethod("PATCH");
}

The body of expectLogRequestMethod skeleton surely can be as simple as:

    NiceMock<StreamInfo::MockStreamInfo> stream_info;
    stream_info.host_ = nullptr;

    Http::TestHeaderMapImpl request_headers{
        {":scheme", "scheme_value"},
        {":authority", "authority_value"},
        {":path", "path_value"},
        {":method", request_method},
    };

    expectLog(fmt::format(R"EOF(
// !!!TODO: replace with the expected structured log here (yes in yaml format). The `fmt::format` helps you to build the string. hint: {} as placeholder, while {{}} to escape.
)EOF",
                          request_method, /* more parameters here, hint: related to `request_headers_bytes` */ ));
    access_log_->log(&request_headers, nullptr, nullptr, stream_info);

WDYT?

[glebmish]

@dio it looks good, I can do that.

Do you think it also make sense to move METHOD_UNSPECIFIED there? Something like expectLogRequestUnspecifiedMethod("WHACKADOO"); which is basically the same method but checks that result is METHOD_UNSPECIFIED.
If you think it make sense, can you please explain the difference between two tests that check METHOD_UNSPECIFIED? One of them seems more complicated but it's not clear for me why two separate tests are needed there

[dio]

You can be as creative as you want 😄.

can you please explain the difference between two tests that check METHOD_UNSPECIFIED?

I think those two tests check different things i.e. the second one checks on marshaling tls_properties.

[glebmish]

@dio added changes you proposed, thanks! Decided to leave METHOD_UNSPECIFIED tests as it is, just fixed them there.

[glebmish]

I see failed test suites, but I can't understand the reason. Can somebody help me with this?

[dio]

@glebmish I think you need to run the formatter. In this case against this file: test/extensions/access_loggers/http_grpc/grpc_access_log_impl_test.cc.

$ ./tools/check_format.py fix test/extensions/access_loggers/http_grpc/grpc_access_log_impl_test.cc

Then add and commit that.

The following is the diff after the format is fixed:

diff --git a/test/extensions/access_loggers/http_grpc/grpc_access_log_impl_test.cc b/test/extensions/access_loggers/http_grpc/grpc_access_log_impl_test.cc
index 89423934e..5aa545b12 100644
--- a/test/extensions/access_loggers/http_grpc/grpc_access_log_impl_test.cc
+++ b/test/extensions/access_loggers/http_grpc/grpc_access_log_impl_test.cc
@@ -143,7 +143,7 @@ public:
     stream_info.host_ = nullptr;

     Http::TestHeaderMapImpl request_headers{
-      {":method", request_method},
+        {":method", request_method},
     };

     expectLog(fmt::format(R"EOF(
@@ -163,7 +163,8 @@ public:
           request_method: {}
           request_headers_bytes: {}
         response: {{}}
-    )EOF", request_method, request_method.length() + 7));
+    )EOF",
+                          request_method, request_method.length() + 7));
     access_log_->log(&request_headers, nullptr, nullptr, stream_info);
   }

[glebmish]

@dio thank you! fixed

[glebmish]

I see that coverage suite is failing with exit code 36. I took a look on bazel exit codes and it seems like it some environment issue, I guess some process is not stopping correctly. What can I do about it?

https://docs.bazel.build/versions/master/guide.html

36 - Local Environmental Issue, suspected permanent.

[dio]

/retest

[repokitteh-read-only]

🔨 rebuilding ci/circleci: coverage (failed build)
🔨 rebuilding ci/circleci: mac (failed build)

🐱

Caused by: a #6737 (comment) was created by @dio.

see: more, trace.

[glebmish]

Well, it doesn't seem to be healed by restart :(
And the only error I see is

FATAL: Attempted to kill stale server process (pid=83) using SIGKILL, but it did not die in a timely fashion.

Did you see this in other PRs? If it's something with environment it should probably fail everywhere

[dio]

@glebmish hum, while investigating the build errors, would you mind to merge master?

[glebmish]

@dio it definitely helped, thanks! Now I understand what's the problem there, coverage is less than threshold. This is funny, I added 1 line of code and several tests and somehow decreased coverage :)

[mattklein123]

Thanks, the coverage issues are broken in master and we are working on them. I will just force merge this for you. Thank you!

[glebmish]

thanks @mattklein123 !