access log: add the detected protocol set by kyessenov · Pull Request #10524 · envoyproxy/envoy

kyessenov · 2020-03-25T20:47:12Z

Signed-off-by: Kuat Yessenov kuat@google.com

Description: Change StreamInfo to carry a list of the detected protocols instead of just HTTP versions. For example, the common case should have TCP, HTTP, HTTP/2 as the value.
Risk Level: low, I tried to keep the usage in HTTP context identical
Testing: updated all tests
Docs Changes: none
Release Notes: none
Fixes #10487

Signed-off-by: Kuat Yessenov <kuat@google.com>

mattklein123

Thanks for working on this. I flushed out some comments from a first pass. My gut reaction is that I think the strong only approach feels a little fragile. WDYT about retaining the HTTP protocol support as it exists today and adding additional string protocol support alongside? It's not optimal but it feels safer and less fragile to me. cc @zuercher for some additional thoughts on this PR.

/wait

mattklein123 · 2020-03-26T18:09:52Z

include/envoy/stream_info/stream_info.h

   * filters (append only). Both object types can be consumed by multiple filters.
   * @return the filter state associated with this request.
   */
+  virtual FilterStateSharedPtr& mutableFilterState() PURE;


It doesn't thrill me to expose a reference to the shared_ptr on the interface? Can you explain why this is needed?

HTTP request stream info takes TCP stream info filter state by a mutable shared_ptr. I think an additional filter state was created before that was used for HTTP parent filter state, which does not feel right to me. I'm not sure why HTTP stream info is allowed to modify TCP stream info filter state.

mattklein123 · 2020-03-26T18:13:31Z

source/common/access_log/access_log_formatter.cc

-  if (protocol) {
+const std::string AccessLogFormatUtils::protocolToString(absl::Span<const std::string> protocols) {
+  auto protocol = Http::Utility::getProtocol(protocols);
+  if (protocol.has_value()) {


Why do we need to do the string to enum to string dance here? Can't we just return the protocol string directly? Or this is for HTTP back compat? If for back compat can you add comments and also maybe skip the final getProtocolString call by returning the string reference directly from getProtocol?

Mostly for backwards compatibility. I can change the interface to return both the enum and the string.

I think trying to mix strings and the protocol enum is going to be confusing and it already involves a lot of converting back and forth between the string and enum representation.

Perhaps a better approach would be to rename the existing StreamInfo::protocol() and StreamInfo::protocol(Http::Protocol) to http_protocol and provide a new StreamInfo::protocols() and StreamInfo::addProtocol that deal with strings. The implementation of http_protocol(Http::Protocol) would invoke addProtocol with the appropriate protocol name and also store the protocol in an absl::optional (returned by http_protocol()). That will make the changes in HCM simpler to review and allows access to the full stack of protocols in other situations.

Otherwise I think we'd need to work out a proper registry. I think that's feasible as long as we the identifiers are not exposed outside a given Envoy process (e.g. the X extension's ID may change from build-to-build but as long as the id is kept internal it shouldn't matter). We might even be able to arrange registrations of core protocols in a stable order so their ids wouldn't change from build-to-build.

Got it. I'll preserve protocol() for HTTP as-is and add two more stubs to deal with the layered protocols. Thank you! I can also do the rename separately (maybe there's some clang tool to automate that) to make reviews easier.

+1 this sounds like a good plan. Thank you!

mattklein123 · 2020-03-26T18:15:34Z

source/common/http/conn_manager_impl.cc

    // The protocol may have shifted in the HTTP/1.0 case so reset it.
-    stream_info_.protocol(protocol);
+    if (Http::Utility::getProtocol(stream_info_.protocols()) != protocol) {
+      stream_info_.addProtocol(Http::Utility::getProtocolString(protocol));


This is going to change the behavior, right? Meaning we will now have multiple HTTP protocol strings?

It is always appended and getProtocol looks from the back, so there is no change in the behavior. But we will have multiple strings in the protocol list as decisions are made about the protocol.

mattklein123 · 2020-03-26T18:49:39Z

source/common/http/utility.cc

 }

+absl::optional<const Protocol> Utility::getProtocol(absl::Span<const std::string> protocols) {
+  for (auto it = protocols.rbegin(); it != protocols.rend(); it++) {


I assume you are going in reverse for a reason? Comment?

Sure, sorry if it was not clear but it's basically append-only history of discovered protocols, so looking from the back is right.

mattklein123 · 2020-03-26T18:50:15Z

source/common/stream_info/stream_info_impl.h

  absl::optional<MonotonicTime> final_time_;

-  absl::optional<Http::Protocol> protocol_;
+  std::vector<std::string> protocols_;


I would recommend an InlinedVector here with a small number of protocols.

Thanks. I am relying on inlined strings to save some space.

kyessenov · 2020-03-26T19:04:15Z

source/common/http/conn_manager_impl.h

  const Server::OverloadActionState& overload_stop_accepting_requests_ref_;
  const Server::OverloadActionState& overload_disable_keepalive_ref_;
  TimeSource& time_source_;
-  std::shared_ptr<StreamInfo::FilterState> filter_state_;


This is the filter state that was (lazily) created by a network filter. I changed it to use the filter state from the connection stream info. Otherwise, it's invisible to access loggers at the connection level.

kyessenov · 2020-03-26T19:06:51Z

I'm fine preserving the existing protocol() as-is and adding another interface method to carry a list. I believe what we have here achieves it, but we have to be careful not to break HTTP code.

kyessenov · 2020-03-31T20:20:18Z

Gentle ping @zuercher for your comments before I can proceed.

kyessenov · 2020-04-02T21:10:23Z

Closing since I have split out smaller changes.

access log: add detected protocol set

c1ce7b7

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov requested review from PiotrSikora, alyssawilk, dio, jmarantz, lizan, mattklein123, snowp and zuercher as code owners March 25, 2020 20:47

kyessenov added 2 commits March 25, 2020 13:53

merge fix

fecf6de

Signed-off-by: Kuat Yessenov <kuat@google.com>

debugging cruft

fb8cd06

Signed-off-by: Kuat Yessenov <kuat@google.com>

mattklein123 self-assigned this Mar 25, 2020

mattklein123 requested changes Mar 26, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 26, 2020

mattklein123 assigned zuercher Mar 26, 2020

kyessenov commented Mar 26, 2020

View reviewed changes

kyessenov closed this Apr 2, 2020

kyessenov mentioned this pull request Apr 2, 2020

stream info: wire http protocol to the connection info #10627

Merged

Conversation

kyessenov commented Mar 25, 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!

kyessenov Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

zuercher Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyessenov Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

kyessenov Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyessenov commented Mar 26, 2020

Uh oh!

kyessenov commented Mar 31, 2020

Uh oh!

kyessenov commented Apr 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kyessenov Mar 26, 2020 •

edited

Loading

zuercher Apr 2, 2020 •

edited

Loading

kyessenov Apr 2, 2020 •

edited

Loading

kyessenov Mar 26, 2020 •

edited

Loading