headers: Ability to rewrite headers received from upstream cluster #12938

shivanshu21 · 2020-09-02T06:38:19Z

…response

Signed-off-by: Shivanshu Goswami shigoswami@ebay.com

Fixes: #12591
Commit Message:
Additional Description:
Risk Level: low
Testing: Added unit tests, performed manual testing
Docs Changes: Added
Release Notes:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]

repokitteh-read-only · 2020-09-02T06:38:25Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #12938 was opened by shivanshu21.

see: more, trace.

htuch · 2020-09-02T23:25:34Z

api/envoy/config/route/v3/route_components.proto

+  //         prefix: "/itm/"
+  //       route:
+  //         location_header_rewrite: "/n/error"
+  string location_header_rewrite = 36


Do you think there will ever be a case to rewrite via regex, e.g. using RegexMatchAndSubstitute, or does it seem that a literal location always works?

I wonder if we want a more general Rewriter message that can be both string and RegexMatchAndSubstitute @envoyproxy/api-shepherds

I was thinking about it too. Regex based rewrite will be more useful. Let me see how I can do that.

+1 to have a wrapper message that contains an internal oneof with different rewrite types.

location HTTP header rewrites are exactly what I need as well! However, could this be more generalized to support any HTTP header rewrite with regex?

shivanshu21 · 2020-09-03T22:31:23Z

@ccravens
I am okay with a generic response_headers_to_rewrite API which we can use in out Location rewrite usecase.
@alyssawilk @htuch Your thoughts?

We can have an API which is like this:

oneof header_rewrite_specifier {
    typename1 header_rewrite_string = 1;
    typename2 header_rewrite_regex = 2;
}

Typename1 and Typename2 consist of Header name and rewrite pattern which could be a string or regex

shivanshu21 · 2020-09-03T22:46:27Z

Just went through @ccravens open issue.

responseHeaderPolicy:
            set:
              - name: X-Service-Name
                value: s1
            remove:
              - X-Internal-Secret
            replace:
              - header: location
                regex: \http:\
                value: https:

This kind of API looks better. Will wait for suggestions. This takes care of add/remove as well as modify on response headers.
Wondering if we should introduce this at both Route as well as Virtualhost levels.

htuch · 2020-09-08T14:08:42Z

We already have API fields to add/remove response headers, so I would discourage introducing a redundant mechanism unless there is a plan to deprecate the existing mechanisms (and have your new scheme apply hierarchically similar to the existing one).

ccravens · 2020-09-08T14:33:57Z

@htuch thanks for your response. I don't think we're requesting to deprecate functionality. I'm aware and using the add / remove functionality. I think we're requesting a feature that addresses a need/requirement that the add/remove functions don't provide. @shivanshu21 let me know if I'm mistaken in that assumption from your side, thanks

shivanshu21 · 2020-09-08T18:37:06Z

@htuch @ccravens If we try to introduce a comprehensive responseHeaderPolicy like the following:

responseHeaderPolicy:
            set:
              - name: X-Service-Name
                value: s1
            remove:
              - X-Internal-Secret
            replace:
              - header: location
                regex: \http:\
                value: https:

We would be adding new options to add/remove header too as shown above in set/remove sections, and then it would make sense to change existing APIs for add/remove. But looks like that is not what we want to do right now.

Going by what @htuch said, I think it should suffice to have a response_headers_to_rewrite field which can take the name of the header, a value to match and the substitution value. Match can be based on regex as well as a blanket replacement string.

The core idea is that this should be generic for all headers, not just location.

shivanshu21 · 2020-09-09T06:37:09Z

/wait

shivanshu21 · 2020-09-09T08:00:12Z

/wait

Will have to redo the code changes. @htuch @ccravens please take a look at the Proto files for the API changes in the latest commit and let me know if the API is satisfactory.

htuch · 2020-09-09T21:32:58Z

api/envoy/config/route/v3/route_components.proto

+  // In the below example there are two response header rewrites mentioned: one for location header
+  // and one for x-random-token header.
+  //
+  // The location header's value will be matched with the given match pattern as a prefix to the header's


You probably want to use envoy.type.v3.StringMatcher for this, rather than hardcode prefix match semantics.

htuch · 2020-09-09T21:36:16Z

api/envoy/config/route/v3/route_components.proto

+  // If no match string is given, the value is replaced entirely with the given substitution. The
+  // response status header is also matched before making a substitution. If the response status
+  // returned from the upstream cluster matches with one of the response statuses in the list, the
+  // substitution can proceed after matching the match section. If this list is empty, response status


I think the response status is really part of the matcher. Also, this only applies to the literal and not when we are doing regex substitute. I think you might want to move this up a level to a header_rewriter message.

htuch · 2020-09-09T21:39:17Z

api/envoy/config/route/v3/route_components.proto

+  //             header_rewrite_literal:
+  //               match: /itm/not-found/
+  //               substitution: /n/error
+  //               response_status:


Status is just one header you could potentially also match one. One thing you could consider doing here for matching is to look at what ResponseMapper in HCM does, where it uses AccessLogFilter as a general purpose matcher for responses prior to applying an action.

I looked at response mapper in HCM. It has a repeated field headers_to_add. They are trying to add headers, so the default assumption is that every header must be added.

For me the case is different. I am trying to match the headers.

repeated config.core.v3.HeaderValue upstream_headers_matcher = 3 [(validate.rules).repeated = {max_items: 1000}];

Suppose I want to match:

Response status should be either 301 OR 302 OR 307

Response status is 200 AND content-type is text/html

For case 1, I would want that if ANY header KV pair matches, match the condition and rewrite the requested header.
For case 2, I would want that if ALL header KV pairs match, only then match the condition, and rewrite the requested header.

For this reason, I have introduced a list of acceptable response codes ANY of which could match.
I have also added a list of headers ALL of which must match.
Header rewrite will happen when both the subconditions match.

My question was more around whether you could use AccessLogFilter, which is more general purpose than what you currently have.

shivanshu21 · 2020-09-11T08:53:47Z

/wait

htuch · 2020-09-11T19:24:53Z

api/envoy/config/route/v3/route_components.proto

+  // part is substituted. In case of present match the whole value of the header is overwritten.
+  //
+  // The x-random-token header's value will be matched using the regex pattern specified in the pattern
+  // section, and the matched part will be replaced by the value provided in the substitution section.


Can you comment individual fields rather than a large detached block comment? Please take a look at the generated docs to see how they render, e.g. https://380489-65214191-gh.circle-artifacts.com/0/generated/docs/api-v3/config/route/v3/route_components.proto.html#envoy-v3-api-msg-config-route-v3-routeaction-headerrewriter

htuch · 2020-09-11T19:27:29Z

api/envoy/config/route/v3/route_components.proto

+  //             header_rewrite_literal:
+  //               match: /itm/not-found/
+  //               substitution: /n/error
+  //               response_status:


My question was more around whether you could use AccessLogFilter, which is more general purpose than what you currently have.

htuch

Looks good, mostly docs and would like to get agreement on the matcher form.

shivanshu21 · 2020-09-13T07:53:10Z

ERROR: /build/tmp/_bazel_root/b570b5ccd0454dc9af9f65ab1833764d/external/envoy_api_canonical/envoy/config/route/v3/BUILD:7:18: in proto_library rule @envoy_api_canonical//envoy/config/route/v3:pkg: cycle in dependency graph:
    //tools/protoxform:protoprint
    //tools/type_whisperer:api_type_db.pb_text
    //tools/type_whisperer:api_type_db
    //tools/type_whisperer:api_type_db_target
    @envoy_api_canonical//versioning:active_protos
    @envoy_api_canonical//envoy/config/common/matcher/v3:pkg
.-> @envoy_api_canonical//envoy/config/route/v3:pkg.     <<<<<<<<<<<<<
|   @envoy_api_canonical//envoy/config/accesslog/v3:pkg <<<<<<<<<<<<<
`-- @envoy_api_canonical//envoy/config/route/v3:pkg
This cycle occurred because of a configuration option

@htuch
AccessLogFilter proto refers to route components proto. So there is a cyclic dependency if we refer to AccessLogFilter proto in route components.

I like the idea of and_filter and or_filter. I am gravitating towards duplication as AccessLogFilter anyway does not have a reason to be in Route components. Thinking of introducing a message just like access log filter in route components.

shivanshu21 · 2020-09-14T08:36:06Z

/wait

htuch · 2020-09-14T19:28:31Z

api/envoy/config/route/v3/route_components.proto

+    HeaderMatcherFilter request_match_criteria = 3 [(validate.rules).message = {required: true}];
+  }
+
+  message HeaderMatcherFilter {


Thanks. How strongly do @envoyproxy/api-shepherds feel about trying to clean this up further in this PR? I think the ideal would be to migrate both the matching going on in AccessLogfilter and HeaderMatcher to envoy.type.matcher.v3.

Sorry @htuch what are you proposing? cc @snowp @yangminzhu for other efforts in flight to merge matching behavior. I agree at a high level that it pains me to see yet another and/or, etc. in the API here.

mattklein123

Thanks for working on this and taking the time to get this right.

mattklein123 · 2020-09-14T22:33:53Z

api/envoy/config/route/v3/route_components.proto

  reserved 12, 18, 19, 16, 22, 21, 10;

  reserved "request_mirror_policy";

+  map<string, HeaderRewriter> response_headers_to_rewrite = 36;


Please add docs for this field. Also, we haven't typically used a map for things like this. IMO it would be more future proof to have a list of messages, even if internally it is translated to a map? @envoyproxy/api-shepherds?

mattklein123 · 2020-09-14T22:35:46Z

api/envoy/config/route/v3/route_components.proto

+    HeaderMatcherFilter request_match_criteria = 3 [(validate.rules).message = {required: true}];
+  }
+
+  message HeaderMatcherFilter {


Sorry @htuch what are you proposing? cc @snowp @yangminzhu for other efforts in flight to merge matching behavior. I agree at a high level that it pains me to see yet another and/or, etc. in the API here.

* Add UpstreamHeaderRewriter and HeaderRewriteLiteral classes * Call rewriteUpstreamResponseHeaders() from finalizeResponseHeaders() * Only allow exact and prefix match on header value * Fix substitution string logic * Fix format * Adjusting messages and comments Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

…rbage Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

htuch · 2020-10-20T14:03:10Z

@shivanshu21 I'll give this a review now, but please try avoid force pushes, as they make it hard to track deltas during review.

htuch · 2020-10-20T14:05:17Z

api/envoy/config/route/v3/route_components.proto

+    HeaderRewriteLiteral value_rewrite_literal = 1;
+  }
+
+  type.matcher.v3.MatchPredicate response_match_criteria = 2;


Comments for all fields, thanks (there are some others in this PR that are not commented.

htuch · 2020-10-20T14:09:40Z

api/envoy/config/route/v3/route_components.proto

-// [#next-free-field: 37]
+message HeaderRewriteLiteral {
+  // String match can use prefix match and present match. In case of prefix match, the matched prefix part of
+  // the header's value is substituted. In case of present match the whole value of the header is overwritten.


What are the semantics for the other string match types? Does capture group semantics apply?

I wonder if we want a StringMatchAndSubstitute that is analogous to RegexMatchAndSubstitute.

Capture group semantics do not apply but this can be a future enhancement in the message HeaderMatchAndRewrite which has a oneof type with an option for this (HeaderRewriteLiteral) message.

Added explanation for other StringMatcher types.

htuch · 2020-10-20T14:10:13Z

api/envoy/config/route/v3/route_components.proto

+message UpstreamHeaderRewriter {
+  string header_name = 1;
+
+  HeaderMatchAndRewrite match_and_rewrite = 2;


htuch · 2020-10-20T14:10:58Z

api/envoy/config/route/v3/route_components.proto

+  // In the example below, we rewrite the location header received from the upstream cluster
+  // if the location header is present.
+  //
+  // If a header is specified in response_headers_to_rewrite section but is not returned from the


Nit: in the *response_headers_to_write* field.

htuch · 2020-10-20T14:11:36Z

api/envoy/type/matcher/v3/header.proto

+//          "exact_match": "POST"
+//        }
+//     }
+// [#next-major-version: This message deprecates config.route.v3.HeaderMatcher]


Please add an explicit comment on this as well for user-facing docs, since users will want to know which one to use.

htuch · 2020-10-20T14:13:10Z

api/envoy/type/matcher/v3/matcher.proto

+// [#protodoc-title: Logical Matcher API]
+
+// Match configuration. This is a recursive structure which allows complex nested match
+// configurations to be built using various logical operators.


Same comment here on user facing docs.

Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

shivanshu21

config_impl_test passes in local

envoy/config/listener/v3/udp_gso_batch_writer_config.proto:5:1: warning: Import google/protobuf/duration.proto but not used.
envoy/config/listener/v3/udp_gso_batch_writer_config.proto:6:1: warning: Import google/protobuf/wrappers.proto but not used.
INFO: From Generating Descriptor Set proto_library @envoy_api//envoy/admin/v3:pkg:
envoy/admin/v3/init_dump.proto:6:1: warning: Import udpa/annotations/versioning.proto but not used.
Target //test/common/router:config_impl_test up-to-date:
  bazel-bin/test/common/router/config_impl_test
INFO: Elapsed time: 118.371s, Critical Path: 113.56s
INFO: 14 processes: 14 processwrapper-sandbox.
INFO: Build completed successfully, 15 total actions
//test/common/router:config_impl_test                                    PASSED in 11.4s

Executed 1 out of 1 test: 1 test passes.
INFO: Build completed successfully, 15 total actions

shivanshu21 · 2020-10-22T07:24:37Z

api/envoy/config/route/v3/route_components.proto

-// [#next-free-field: 37]
+message HeaderRewriteLiteral {
+  // String match can use prefix match and present match. In case of prefix match, the matched prefix part of
+  // the header's value is substituted. In case of present match the whole value of the header is overwritten.


Capture group semantics do not apply but this can be a future enhancement in the message HeaderMatchAndRewrite which has a oneof type with an option for this (HeaderRewriteLiteral) message.

Added explanation for other StringMatcher types.

Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

lizan

In general this seems on a right direction, though it would be nice to wait a bit to land the new matcher API (at least in proto level), so we would have less tech debt to clean up.

lizan · 2020-10-22T22:37:13Z

api/envoy/config/tap/v3/common.proto

@@ -39,6 +39,7 @@ message TapConfig {
  // Exactly one of :ref:`match <envoy_api_field_config.tap.v3.TapConfig.match>` and
  // :ref:`match_config <envoy_api_field_config.tap.v3.TapConfig.match_config>` must be set. If both
  // are set, the :ref:`match <envoy_api_field_config.tap.v3.TapConfig.match>` will be used.
+  // [#next-major-version: common.matcher.v3.MatchPredicate should be refactored to use type.matcher.v3.MatchPredicate.]


This doc change for tap isn't really related? And we don't have type.matcher.v3.MatchPredicate (same for elsewhere), and that's not part of the new matcher API either.

type.matcher.v3.MatchPredicate This is being introduced in this PR. We spoke to @snowp and the Unified matcher is going to take time.

I will handle the tech debt in the next, separate PR while HeaderMatcher references are moved to StringMatcher based type.Matcher.v3.HeaderMatcher (which is also introduced in this PR).

Sorry, I know that is what we said, but we are actively working on the new matching protos now. I think it's worth intersecting these efforts. Let's circle back next week and see where we are at.

Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

databus23 · 2021-05-20T10:51:30Z

Just curios why was the closed? Is it still planned to support response header rewriting eventually?

repokitteh-read-only bot added the api label Sep 2, 2020

htuch reviewed Sep 2, 2020

View reviewed changes

mattklein123 assigned alyssawilk Sep 3, 2020

alyssawilk added the waiting label Sep 3, 2020

ccravens mentioned this pull request Sep 3, 2020

Add Header "Replace" Function projectcontour/contour#2776

Closed

repokitteh-read-only bot removed the waiting label Sep 9, 2020

repokitteh-read-only bot added waiting and removed waiting labels Sep 9, 2020

repokitteh-read-only bot added the waiting label Sep 9, 2020

htuch reviewed Sep 9, 2020

View reviewed changes

repokitteh-read-only bot removed the waiting label Sep 11, 2020

repokitteh-read-only bot added waiting and removed waiting labels Sep 11, 2020

htuch reviewed Sep 11, 2020

View reviewed changes

htuch suggested changes Sep 11, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 14, 2020

htuch reviewed Sep 14, 2020

View reviewed changes

mattklein123 requested changes Sep 14, 2020

View reviewed changes

mattklein123 self-assigned this Sep 14, 2020

Shivanshu Goswami added 14 commits October 20, 2020 00:47

Implement match predicate based response header matching

5b24385

Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

Adding docs entry

0896583

Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

Fixing match predicate call from finalizeResponseHeaders

e02988a

Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

Fixing crashes

a0bb375

Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

Stop using header entry get()

9ab6018

Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

MatchStatusVector should be initialized so that local vars are non ga…

b2e93c5

…rbage Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

Adding unit tests

432d53c

Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

Deprecate older match predicate

8e10541

Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

Adding deprecation messages to proto files

6b9f803

Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

Adding more tests

ea7343e

Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

fixing format

8d4a72e

Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

Add new types to toc tree

05962a6

Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

Fixing test failure and rebasing with master

352adbb

Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

htuch reviewed Oct 20, 2020

View reviewed changes

Shivanshu Goswami added 2 commits October 22, 2020 00:21

Separate out the test cases

8015b9c

Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

Addressing comments

b095e21

Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

shivanshu21 commented Oct 22, 2020

View reviewed changes

Shivanshu Goswami added 2 commits October 22, 2020 01:36

Addressing more comments

924a402

Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

Spell checker failing the tests

ac5f609

Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

lizan reviewed Oct 22, 2020

View reviewed changes

Shivanshu Goswami added 2 commits October 22, 2020 16:24

TestResponseHeaderMapImpl object created like previous examples

5442751

Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

Remove problematic check and test out the remaining first

0a928b2

Signed-off-by: Shivanshu Goswami <shigoswami@ebay.com>

alyssawilk added the waiting label Dec 14, 2020

shivanshu21 mentioned this pull request Dec 17, 2020

Need 3XX Location URL rewrite feature #12591

Open

mattklein123 unassigned htuch Dec 18, 2020

Base automatically changed from master to main January 15, 2021 23:01

alyssawilk closed this Feb 22, 2021

headers: Ability to rewrite headers received from upstream cluster #12938

headers: Ability to rewrite headers received from upstream cluster #12938

Conversation

shivanshu21 commented Sep 2, 2020 • edited Loading

repokitteh-read-only bot commented Sep 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivanshu21 commented Sep 3, 2020 • edited Loading

shivanshu21 commented Sep 3, 2020

htuch commented Sep 8, 2020

ccravens commented Sep 8, 2020

shivanshu21 commented Sep 8, 2020 • edited Loading

shivanshu21 commented Sep 9, 2020

shivanshu21 commented Sep 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivanshu21 commented Sep 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

shivanshu21 commented Sep 13, 2020

shivanshu21 commented Sep 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch commented Oct 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivanshu21 left a comment

Choose a reason for hiding this comment

config_impl_test passes in local

Choose a reason for hiding this comment

lizan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

databus23 commented May 20, 2021

shivanshu21 commented Sep 2, 2020 •

edited

Loading

shivanshu21 commented Sep 3, 2020 •

edited

Loading

shivanshu21 commented Sep 8, 2020 •

edited

Loading