router: support dynamic host metadata headers by zuercher · Pull Request #2179 · envoyproxy/envoy

zuercher · 2017-12-09T00:34:02Z

Modifies the Router::RequestInfoHeaderFormatter to support extracting string, numeric, and boolean values from the upstream host provided by AccessLog::RequestInfo.

Modifies Router::HeaderParser to omit headers whose formatted value is an empty string.

Risk Level: Low

Testing:
Unit tests to cover parsing of new header variable expression.

Docs Changes:
envoyproxy/data-plane-api#335

Release Notes:
Updated RAW_RELEASE_NOTES.md

Fixes #2148.

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

zuercher · 2017-12-09T00:35:13Z

I wanted to update test/integration/header_integration_test.cc to test these, but it seems pretty tricky to use the existing config modifiers to set up an EDS cluster (which is required to get endpoint metadata). Am I missing something?

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

mattklein123 · 2017-12-11T03:38:29Z

I wanted to update test/integration/header_integration_test.cc to test these, but it seems pretty tricky to use the existing config modifiers to set up an EDS cluster (which is required to get endpoint metadata). Am I missing something?

For an integration test, you would need to also setup a fake EDS server. Harvey did a bunch of work to get an ADS integration test working: https://github.com/envoyproxy/envoy/blob/master/test/integration/ads_integration_test.cc. If you want to do an integration test you could build on that and it would probably be pretty straightforward.

mattklein123 · 2017-12-11T03:45:28Z

source/common/router/header_formatter.cc

+namespace {
+
+// Parse list of namespace and keys into a vector. Supports backslash escaping.
+std::vector<std::string> parseUpstreamMetadataParams(const std::string& field) {


Before doing a full review we should consider @htuch comment in the data-plane-api PR as to whether we potentially want to use embedded JSON here or something so we could use the JSON parser. Seems like a good idea to me to use JSON perhaps, but I could go either way. Thoughts?

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

mattklein123 · 2017-12-12T18:18:45Z

@zuercher can you look at TSAN? I can take a look after that is fixed.

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

zuercher · 2017-12-12T20:25:50Z

For reference, the TSAN error occurs when some field descriptor in the DiscoveryRequest hasn't been fully initialized -- the protobuf lib initializes it using what looks to be an atomicity-based mutex-free algorithm, but apparently TSAN doesn't like it. I believe the offending address is the atomically-accessed guard field.

In any event, my fix is to initialize the field descriptors by parsing the message from YAML, which is probably easier to read than the C++ API version I had before.

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

mattklein123

Great stuff. Few small comments. @alyssawilk do you mind taking a look at the test stuff? Might be best for your other change to merge if we are going to do other base test changes.

mattklein123 · 2017-12-13T01:27:33Z

source/common/router/header_formatter.cc

      return request_info.getDownstreamAddress();
    };
+  } else if (StringUtil::startsWith(field_name.c_str(), "UPSTREAM_METADATA")) {
+    field_extractor_ = parseUpstreamMetadataField(field_name.substr(17));


nit: Might use sizeof() here or some other constant to avoid the magic '17'

mattklein123 · 2017-12-13T01:28:45Z

test/common/router/header_formatter_test.cc

+      "Invalid value.\n");
+
+  // No parameters.
+  EXPECT_THROW(RequestInfoHeaderFormatter requestInfoHeaderFormatter("UPSTREAM_METADATA", false),


Is it possible to use EXPECT_THROW_WITH_MESSAGE for all of these?

mattklein123 · 2017-12-13T01:30:04Z

test/integration/header_integration_test.cc

+              )EOF"));
+
+      // Give the config utility a place to stuff the upstream host's port (must come before
+      // the eds-cluster to keep the upstreams and ports in the same order).)


nit: trailing ')'

mattklein123 · 2017-12-13T01:30:58Z

test/integration/header_integration_test.cc

    EXPECT_EQ(expected_headers, headers);
  }
+
+  bool use_eds_{0};


nit: I would use '{}' or '{false}' here vs. 0

mattklein123 · 2017-12-13T01:32:28Z

test/integration/header_integration_test.cc

+                        api_type: GRPC
+              )EOF"));
+
+      // Give the config utility a place to stuff the upstream host's port (must come before


This is a little confusing. Is this just because of how the default config stuff works? Would it be worth it to build this EDS stuff into the base integration class? It seems generally useful and very cool. cc @alyssawilk

Yeah, I think the same way that we special cased the HttpConnectionManager it'd be useful to have utilities to play with EDS config directly.

I can take a look at making the config smarter about EDS in another review, if you'd like.

If you want to do a follow up / TODO that's fine, but can you add more of a comment here? It's confusing why the placeholder is necessary.

But yes, it's to do with ConfigHelper expecting the number of ports assigned to match the number of upstreams during the call to finalize.

ggreenway · 2017-12-13T17:21:36Z

source/common/router/header_formatter.cc

+
+std::function<std::string(const Envoy::AccessLog::RequestInfo&)>
+parseUpstreamMetadataField(const std::string& params_str) {
+  if (params_str.empty() || params_str[0] != '(' || params_str[params_str.size() - 1] != ')') {


if (params_str.empty() || params_str.front() != '(' || params_str.back() != ')')

ggreenway · 2017-12-13T17:27:00Z

source/common/router/header_formatter.cc

+  return [params](const Envoy::AccessLog::RequestInfo& request_info) -> std::string {
+    Upstream::HostDescriptionConstSharedPtr host = request_info.upstreamHost();
+    if (!host) {
+      return "";


I slightly prefer "return std::string()" (same for other places in this function)

ggreenway · 2017-12-13T17:29:30Z

source/common/router/header_formatter.cc

+}
+
+std::function<std::string(const Envoy::AccessLog::RequestInfo&)>
+parseUpstreamMetadataField(const std::string& params_str) {


Can you add a comment with an example string of the correct format near the top of the function, so I can quickly see visually what it is this function is parsing?

ggreenway · 2017-12-13T17:33:00Z

source/common/router/header_formatter.cc

+      return value->bool_value() ? "true" : "false";
+
+    default:
+      // Unsupported type or null value.


This case will be hard for a user to debug. Since we found something, but it's the wrong type, would it be better to return "unsupported metadata type" or something, or perhaps log something?

I think returning an arbitrary string would be bad. I've added a log message.

IMO debug is a more appropriate log level here, as this could emit at high rate due to external factors.

Dumb story, but yes, that's what I forgot to change it back to before committing.

ggreenway · 2017-12-13T17:33:33Z

source/common/router/header_formatter.cc

      return request_info.getDownstreamAddress();
    };
+  } else if (StringUtil::startsWith(field_name.c_str(), "UPSTREAM_METADATA")) {
+    field_extractor_ = parseUpstreamMetadataField(field_name.substr(17));


Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

* update envoy sha * fix build

router: support dynamic host metadata headers

2c99a27

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

zuercher added 2 commits December 8, 2017 16:38

cleanup and ignore empty headers

939257b

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

more specific release notes

bb8bae1

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

mattklein123 reviewed Dec 11, 2017

View reviewed changes

zuercher mentioned this pull request Dec 11, 2017

Use CDS metadata to populate headers via the UPSTREAM_METADATA variable #2184

Closed

zuercher added 4 commits December 11, 2017 13:05

switch to JSON array params

f0cd0da

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

add dynamic response header to header integration test

5288893

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

Merge branch 'master' into impl-2148-dynamic-host-headers

ba6cd16

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

remove a stray include

1f04d64

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

zuercher added 2 commits December 12, 2017 12:17

fix race

8e7a25c

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

Merge branch 'master' into impl-2148-dynamic-host-headers

3cfa31a

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

zuercher added 3 commits December 12, 2017 12:56

undo accidental change

95c4b62

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

Merge branch 'master' into impl-2148-dynamic-host-headers

370781c

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

fix invalid test, simplify another

13449ed

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

mattklein123 reviewed Dec 13, 2017

View reviewed changes

ggreenway requested changes Dec 13, 2017

View reviewed changes

zuercher added 5 commits December 13, 2017 09:39

review comments and additional tests

82b89ab

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

better comment re: ConfigHelper, add TODO

8fc363f

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

further review comments

e6e8f88

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

fix my derp

3265a9e

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

Merge branch 'master' into impl-2148-dynamic-host-headers

0364242

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

mattklein123 approved these changes Dec 13, 2017

View reviewed changes

ggreenway approved these changes Dec 13, 2017

View reviewed changes

zuercher merged commit a4fa738 into envoyproxy:master Dec 13, 2017

zuercher deleted the impl-2148-dynamic-host-headers branch December 15, 2017 17:36

Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020

Update envoy sha and fix bulid break (envoyproxy#2179)

e613d1c

* update envoy sha * fix build

Conversation

zuercher commented Dec 9, 2017

Uh oh!

zuercher commented Dec 9, 2017

Uh oh!

mattklein123 commented Dec 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Dec 12, 2017

Uh oh!

zuercher commented Dec 12, 2017

Uh oh!

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

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants