test: use TestRequestHeaderMapImpl for building custom headers by caschoener · Pull Request #2455 · envoyproxy/envoy-mobile

caschoener · 2022-08-03T20:37:20Z

In the commit #2362 we added a custom_headers_ field on the BaseClientIntegrationTest class to use for custom headers.

This commit replaces that class field by instead instantiating a custom_headers object inside tests when necessary. Then adds a helper function to convert it from the TestRequestHeaderMapImpl (an easy to edit type) to RequestHeaders (envoy header type).

Signed-off-by: caschoener schoener@google.com

Signed-off-by: caschoener <schoener@google.com> dos Signed-off-by: caschoener <schoener@google.com>

caschoener · 2022-08-03T20:40:44Z

library/common/http/header_utility.cc

 namespace Http {
 namespace Utility {

-void toEnvoyHeaders(HeaderMap& transformed_headers, envoy_headers headers) {


The edits to this file aren't strictly part of this diff. I just noticed that the toEnvoyHeaders function has better parameter names in its header_utility.h file, figured I'd fix the mismatch.

Signed-off-by: caschoener <schoener@google.com>

alyssawilk

Sweet! Very excited to see that hacky custom header map go away :-)

alyssawilk · 2022-08-04T13:05:50Z

library/cc/bridge_utility.cc

  for (auto i = 0; i < raw_headers.length; i++) {
    auto key = Data::Utility::copyToString(raw_headers.entries[i].key);
    auto value = Data::Utility::copyToString(raw_headers.entries[i].value);
-


looks like changes to this file can be reverted?

alyssawilk · 2022-08-04T13:07:32Z

test/common/integration/base_client_integration_test.cc

+  envoy_headers envoyHeaders = Http::Utility::toBridgeHeaders(request_headers);
+  Platform::RawHeaderMap rawHeaderMap = Platform::envoyHeadersAsRawHeaderMap(envoyHeaders);
+
+  Platform::RequestHeadersBuilder builder(Platform::RequestMethod::GET, scheme_, host, "/");


I think we actually want to get the scheme and host and path from the request headers, which may mean setting the host and scheme in default_request_headers in the test set up (like they were before)
it's rare but a useful corner case to be able to test http schemed requests over a TLS conection

alyssawilk · 2022-08-04T13:10:49Z

test/common/integration/base_client_integration_test.cc

+  Platform::RawHeaderMap rawHeaderMap = Platform::envoyHeadersAsRawHeaderMap(envoyHeaders);
+
+  Platform::RequestHeadersBuilder builder(Platform::RequestMethod::GET, scheme_, host, "/");
+  for (const auto& pair : rawHeaderMap) {


can we do something like
header_map.iterate(
[&request_headers, builder](const HeaderEntry& header) -> HeaderMap::Iterate {
}
to get the key/value pairs from request_headers and stick them in builder without doing the above conversions?

alyssawilk · 2022-08-04T13:11:10Z

test/common/integration/base_client_integration_test.h

  void threadRoutine(absl::Notification& engine_running);
  // Must be called manually by subclasses in their TearDown();
  void TearDown();
+  std::shared_ptr<Platform::RequestHeaders>


alyssawilk · 2022-08-04T13:12:18Z

test/common/integration/client_integration_test.cc


+  Buffer::OwnedImpl request_data = Buffer::OwnedImpl("request body");
+  Http::TestRequestHeaderMapImpl custom_headers;
+  HttpTestUtility::addDefaultHeaders(custom_headers);


I think we can use default_request_headers_ and make sure we addDefaultHeaders in initialize() to avoid having to create headers in each test case

alyssawilk · 2022-08-04T13:12:47Z

test/common/integration/client_integration_test.cc

  std::string upstream_request;
  EXPECT_TRUE(upstream_connection->waitForData(FakeRawConnection::waitForInexactMatch("GET /"),
                                               &upstream_request));
+


nit: always good to revert non-necessary changes just to reduce churn :-)

alyssawilk · 2022-08-04T13:14:50Z

test/common/integration/base_client_integration_test.cc

  if (upstreamProtocol() == Http::CodecType::HTTP2) {
    builder.addUpstreamHttpProtocol(Platform::UpstreamHttpProtocol::HTTP2);
  }
  default_request_headers_ = std::make_shared<Platform::RequestHeaders>(builder.build());


I think we want to move default_request_headers_ to be a TestRequestHeaderMap impl, and shift all the tests over to using the new utility.

Signed-off-by: caschoener <schoener@google.com>

alyssawilk

wohoo! Almost there

alyssawilk · 2022-08-04T19:13:12Z

test/common/integration/base_client_integration_test.cc

+  Platform::RequestHeadersBuilder builder(
+      Platform::RequestMethod::GET,
+      std::string(default_request_headers_.Scheme()->value().getStringView()),
+      std::string(default_request_headers_.Host()->value().getStringView()), "/");


let's get path from the headers as well?

alyssawilk · 2022-08-04T19:14:10Z

test/common/integration/base_client_integration_test.cc

+              request_headers.formatter().value();
+          key_val = formatter.format(key_val);
+        }
+        auto key = std::string(key_val);


isn't key_val already a string? Why do we need the extra one?

alyssawilk · 2022-08-04T19:14:56Z

test/common/integration/base_client_integration_test.cc

+    builder.set(pair.first, pair.second);
+  }
+  auto mobile_headers = std::make_shared<Platform::RequestHeaders>(builder.build());
+  return mobile_headers;


nit: consider skipping the temporary variable and just return the make_shared

alyssawilk · 2022-08-04T19:15:59Z

test/common/integration/client_integration_test.cc

+        "envoy.extensions.http.header_formatters.preserve_case.v3.PreserveCaseFormatterConfig");
+    ConfigHelper::setProtocolOptions(*bootstrap.mutable_static_resources()->mutable_clusters(0),
+                                     protocol_options);
+  });


why do we need this? Envoy defaults should include stateful formatter

Signed-off-by: caschoener <schoener@google.com>

abeyad

Looks good Chris, thanks for working on this! Left a couple comments.

abeyad · 2022-08-04T20:41:20Z

test/common/integration/base_client_integration_test.cc

-    builder.set(entry.first, values);
-  }
+  HttpTestUtility::addDefaultHeaders(default_request_headers_);
+  default_request_headers_.setHost(fake_upstreams_[0]->localAddress()->asStringView());


why is setHost on default_request_headers_ called after adding the default request headers?

addDefaultHeaders is really just an initialization function, which sets host to "/" I believe. And then we overwrite the values afterwards.

Got it, thanks for clarifying. The addDefaultHeaders name was a bit confusing, wonder if something like setDefaultHeaderValues would make more sense, but in any case, that's not something for this PR.

abeyad · 2022-08-04T22:46:46Z

test/common/integration/base_client_integration_test.cc

+
+  for (const auto& pair : rawHeaderMap) {
+    builder.set(pair.first, pair.second);
+  }


I'm not sure I follow L117-133. In L117, we iterate over request_headers, and add them to the RequestHeadersBuilder. In L131, we iterate over the rawHeaderMap and add them to the RequestHeadersBuilder. But the rawHeaderMap is built from envoy_headers which is built from request_headers, so aren't we adding the same headers to the RequestHeadersBuilder twice?

Great catch yeah looks like I forgot to delete some code :O

test/common/integration/base_client_integration_test.cc

Signed-off-by: caschoener <schoener@google.com>

abeyad

LGTM

uno

0f08b73

Signed-off-by: caschoener <schoener@google.com> dos Signed-off-by: caschoener <schoener@google.com>

caschoener commented Aug 3, 2022

View reviewed changes

remove debug

bbb24fd

Signed-off-by: caschoener <schoener@google.com>

alyssawilk self-assigned this Aug 3, 2022

caschoener marked this pull request as draft August 3, 2022 21:19

caschoener added 3 commits August 3, 2022 22:34

add indirection to make tests pass

d7f31d3

Signed-off-by: caschoener <schoener@google.com>

format again

3ac0389

Signed-off-by: caschoener <schoener@google.com>

pass by reference

fe86f65

Signed-off-by: caschoener <schoener@google.com>

caschoener marked this pull request as ready for review August 3, 2022 22:52

alyssawilk reviewed Aug 4, 2022

View reviewed changes

caschoener added 4 commits August 4, 2022 16:49

Change default_headers to impl

defc626

Signed-off-by: caschoener <schoener@google.com>

conversion function from scratch

a398b4e

Signed-off-by: caschoener <schoener@google.com>

cleanup

fb2276d

Signed-off-by: caschoener <schoener@google.com>

cleanup 2

9e72e3c

Signed-off-by: caschoener <schoener@google.com>

alyssawilk reviewed Aug 4, 2022

View reviewed changes

Address comments

464f0e6

Signed-off-by: caschoener <schoener@google.com>

abeyad reviewed Aug 4, 2022

View reviewed changes

ali comments

2703f3a

Signed-off-by: caschoener <schoener@google.com>

alyssawilk approved these changes Aug 8, 2022

View reviewed changes

abeyad approved these changes Aug 8, 2022

View reviewed changes

alyssawilk merged commit 7c44507 into envoyproxy:main Aug 8, 2022

Conversation

caschoener commented Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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

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

Choose a reason for hiding this comment

Uh oh!

Uh oh!

abeyad left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

caschoener commented Aug 3, 2022 •

edited

Loading