[thrift_proxy] Replace local HeaderMap with Http::HeaderMap[Impl] #4169

brirams · 2018-08-15T22:17:53Z

Description:
As part of implementing header matching in the thrift proxy, we'll want to re-use a lot of the existing machinery that exists for http header matching. To make that more straightforward, use the http header implementation in the thrift proxy instead of our own rolled one.

Risk Level:
low

Testing:
tests, new and old, pass.

zuercher

Thank you!

zuercher · 2018-08-16T17:01:55Z

source/extensions/filters/network/thrift_proxy/metadata.h

@@ -123,11 +48,14 @@ class MessageMetadata {
  MessageType messageType() const { return msg_type_.value(); }
  void setMessageType(MessageType msg_type) { msg_type_ = msg_type; }

-  void addHeader(Header&& header) { headers_.add(std::move(header)); }
+  void addHeader(const std::string key, const std::string value) {
+    headers_.addReference(Http::LowerCaseString(key), value);


Note the docs on addReference in include/envoy/http/header_map.h -- it's expecting key and value to live as long as headers_ does and they'll get destroyed when this method exits. I think you need to use addCopy.

That said, I think now that we're using the Http::HeaderMapImpl, we should add a Http::HeaderMap& headers() method (keep the existing one), delete addHeader, and let callers use the full HeaderMap interface. Then the caller, who may know more about the lifetime of the data, can make an informed decision on how to set the header.

zuercher · 2018-08-16T21:17:18Z

test/extensions/filters/network/thrift_proxy/metadata_test.cc

@@ -56,8 +56,7 @@ TEST(MessageMetadataTest, Headers) {
  MessageMetadata metadata;

  EXPECT_EQ(metadata.headers().size(), 0);
-
-  metadata.addHeader("k", "v");
+  metadata.headers().addReference(Http::LowerCaseString("k"), "v");


addCopy -- those strings still don't live beyond this line of code.

bah you're right!

zuercher · 2018-08-16T22:11:03Z

tag @rgs1 and @fishcakez

…apImpl Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

… to mutate Http::HeaderMap Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

brirams · 2018-08-17T17:11:37Z

mistakenly rebased my work vs. merging master. sorry, ya'll.

zuercher

Cool. One little thing, but otherwise 👍

zuercher · 2018-08-17T17:30:49Z

source/extensions/filters/network/thrift_proxy/header_transport_impl.cc

@@ -296,7 +300,7 @@ void HeaderTransportImpl::writeVarString(Buffer::Instance& buffer, const std::st
  if (len == 0) {
    return;
  }
-  buffer.add(str);
+  buffer.add(str.data(), str.length());


Just use len

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

rgs1 · 2018-08-17T21:52:04Z

source/extensions/filters/network/thrift_proxy/header_transport_impl.cc

@@ -146,7 +146,7 @@ bool HeaderTransportImpl::decodeFrameStart(Buffer::Instance& buffer, MessageMeta
    while (num_headers-- > 0) {
      std::string key = drainVarString(buffer, header_size, "header key");


nit: couldn't key be constructed here as a LowerCaseString?

rgs1 · 2018-08-17T21:52:28Z

source/extensions/filters/network/thrift_proxy/header_transport_impl.cc

@@ -146,7 +146,7 @@ bool HeaderTransportImpl::decodeFrameStart(Buffer::Instance& buffer, MessageMeta
    while (num_headers-- > 0) {
      std::string key = drainVarString(buffer, header_size, "header key");
      std::string value = drainVarString(buffer, header_size, "header value");


nit: should key/value be consts?

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

rgs1

lgtm, thanks!

zuercher

👍

* origin/master: (34 commits) fuzz: fixes oss-fuzz: 9895 (envoyproxy#4189) docs: added developer docs for pprof/tcmalloc testing (envoyproxy#4194) fix sometimes returns response body to HEAD requests (envoyproxy#3985) admin: fix config dump order (envoyproxy#4197) thrift: allow translation between downstream and upstream protocols (envoyproxy#4136) Use local_info.node() instead of bootstrap.node() whenever possible (envoyproxy#4120) upstream: allow extension protocol options to be used with http filters (envoyproxy#4165) [thrift_proxy] Replace local HeaderMap with Http::HeaderMap[Impl] (envoyproxy#4169) docs: improve gRPC-JSON filter doc (envoyproxy#4184) stats: refactoring MetricImpl without strings (envoyproxy#4190) fuzz: coverage profile generation instructions. (envoyproxy#4193) upstream: rebuild cluster when health check config is changed (envoyproxy#4075) build: use clang-6.0. (envoyproxy#4168) thrift_proxy: introduce header transport (envoyproxy#4082) tcp: allow connection pool callers to store protocol state (envoyproxy#4131) configs: match latest API changes (envoyproxy#4185) Fix wrong mock function name. (envoyproxy#4187) Bump yaml-cpp so it builds with Visual Studio 15.8 (envoyproxy#4182) Converting envoy configs to V2 (envoyproxy#2957) Add timestamp to HealthCheckEvent definition (envoyproxy#4119) ... Signed-off-by: Snow Pettersen <snowp@squareup.com>

brirams requested a review from zuercher as a code owner August 15, 2018 22:17

mattklein123 assigned zuercher Aug 16, 2018

zuercher reviewed Aug 16, 2018

View reviewed changes

zuercher assigned brian-pane Aug 16, 2018

brirams added 4 commits August 17, 2018 10:09

replace ThriftProxy::HeaderMap with Http::HeaderMap and Http::HeaderM…

ee0c291

…apImpl Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

stop using addReference and let clients of MessageMetadata decide how…

e5cdb3b

… to mutate Http::HeaderMap Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

use the HeaderMap interface correctly in tests

a55e68c

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

update header_transport_impl to use new http headers

24952b1

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

brirams force-pushed the brirams/#5555/thrift_proxy/use_http_header_impl branch from c56928d to 24952b1 Compare August 17, 2018 17:10

zuercher reviewed Aug 17, 2018

View reviewed changes

brirams added 4 commits August 17, 2018 10:33

format header_transport_impl correctly

46eaad6

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

use len

2164b60

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

clean up after ourselves

29af572

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

learn that the formatter should just never be skipped

b01a6f7

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

rgs1 reviewed Aug 17, 2018

View reviewed changes

brirams added 2 commits August 17, 2018 15:20

use a shared_ptr instead

436ce7c

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

construct LowerCaseString straight away, const it up

790a81d

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

rgs1 approved these changes Aug 17, 2018

View reviewed changes

zuercher approved these changes Aug 17, 2018

View reviewed changes

zuercher merged commit fbf6af7 into envoyproxy:master Aug 17, 2018

brirams deleted the brirams/#5555/thrift_proxy/use_http_header_impl branch August 20, 2018 16:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[thrift_proxy] Replace local HeaderMap with Http::HeaderMap[Impl] #4169

[thrift_proxy] Replace local HeaderMap with Http::HeaderMap[Impl] #4169

brirams commented Aug 15, 2018

zuercher left a comment

zuercher Aug 16, 2018

zuercher Aug 16, 2018

brirams Aug 16, 2018

zuercher commented Aug 16, 2018

brirams commented Aug 17, 2018

zuercher left a comment

zuercher Aug 17, 2018

rgs1 Aug 17, 2018

rgs1 Aug 17, 2018

rgs1 left a comment

zuercher left a comment

		@@ -146,7 +146,7 @@ bool HeaderTransportImpl::decodeFrameStart(Buffer::Instance& buffer, MessageMeta
		while (num_headers-- > 0) {
		std::string key = drainVarString(buffer, header_size, "header key");

[thrift_proxy] Replace local HeaderMap with Http::HeaderMap[Impl] #4169

[thrift_proxy] Replace local HeaderMap with Http::HeaderMap[Impl] #4169

Conversation

brirams commented Aug 15, 2018

zuercher left a comment

Choose a reason for hiding this comment

zuercher Aug 16, 2018

Choose a reason for hiding this comment

zuercher Aug 16, 2018

Choose a reason for hiding this comment

brirams Aug 16, 2018

Choose a reason for hiding this comment

zuercher commented Aug 16, 2018

brirams commented Aug 17, 2018

zuercher left a comment

Choose a reason for hiding this comment

zuercher Aug 17, 2018

Choose a reason for hiding this comment

rgs1 Aug 17, 2018

Choose a reason for hiding this comment

rgs1 Aug 17, 2018

Choose a reason for hiding this comment

rgs1 left a comment

Choose a reason for hiding this comment

zuercher left a comment

Choose a reason for hiding this comment