Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

router: support for request/response header manipulation in weighted clusters #2765

Merged

Conversation

zuercher
Copy link
Member

@zuercher zuercher commented Mar 8, 2018

Implements the request_headers_to_add, response_headers_to_add, and response_headers_to_remove fields added to weighted clusters by envoyproxy/data-plane-api#441.

Risk Level: Low - no change in behavior without configuration changes

Testing: unit and integration tests

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

Release Notes: updated

Fixes: #2455

Signed-off-by: Stephan Zuercher [email protected]

@htuch
Copy link
Member

htuch commented Mar 9, 2018

@junr03 can you take a first pass on this? Thanks.

request_headers_parser_(HeaderParser::configure(cluster.request_headers_to_add())),
response_headers_parser_(HeaderParser::configure(cluster.response_headers_to_add(),
cluster.response_headers_to_remove())) {
if (cluster.has_metadata_match()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the cleanup here!

{":path", "/vhost-route-and-weighted-clusters"},
{":method", "GET"},
},
Http::TestHeaderMapImpl{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing something in how these tests are run, but why not have the x-weighted-cluster-response-remove header here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's in the previous test. You can think of these tests as existing in append/replace pairs, and I decided not to repeat the testing of header removal in the replace tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I was wondering if having the remove behavior tested in both places would ensure that remove behavior works correctly regardless of adding scheme (append/replace). But if you don't think that gives us anything I am good with it.

@junr03
Copy link
Member

junr03 commented Mar 13, 2018

@htuch lgtm, do you mind taking a final pass?

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@zuercher zuercher merged commit 96c645c into envoyproxy:master Mar 13, 2018
zuercher added a commit to envoyproxy/data-plane-api that referenced this pull request Mar 13, 2018
Unhide the request_headers_to_add, response_headers_to_add, and response_headers_to_remove fields in ClusterWeight. Update HTTP conn manager docs related to same.

Doc update for envoyproxy/envoy#2765.

Signed-off-by: Stephan Zuercher [email protected]
@zuercher zuercher deleted the stephan/weighted-cluster-headers branch July 19, 2018 20:54
Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020
…2765)

* fix(stackdriver): use more appropriate buckets for bytes

Signed-off-by: Douglas Reid <[email protected]>

* update buckets

Signed-off-by: Douglas Reid <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants