Skip to content

grpc-json: preserve request method in x-envoy-original-method header#11126

Merged
lizan merged 9 commits into
envoyproxy:masterfrom
JavaScriptBach:envoy-original-method
Jun 2, 2020
Merged

grpc-json: preserve request method in x-envoy-original-method header#11126
lizan merged 9 commits into
envoyproxy:masterfrom
JavaScriptBach:envoy-original-method

Conversation

@JavaScriptBach
Copy link
Copy Markdown
Contributor

@JavaScriptBach JavaScriptBach commented May 10, 2020

Commit Message: grpc-json: preserve http request method in x-envoy-original-method header so that applications have access to it.
Additional Description: The grpc-json transcoder currently forwards HTTP path to applications via "x-envoy-original-path" header. We would find it useful if it also forwarded the HTTP method.
Risk Level: Low
Testing: Updated grpc-json-transcoder unit tests
Docs Changes: Added docs
Release Notes: Added release notes

@JavaScriptBach JavaScriptBach requested a review from lizan as a code owner May 10, 2020 04:20
…nscoder

Signed-off-by: Phillip Huang <phillip@dropbox.com>
Comment thread include/envoy/http/header_map.h Outdated
HEADER_FUNC(EnvoyRetryGrpcOn) \
HEADER_FUNC(EnvoyRetriableStatusCodes) \
HEADER_FUNC(EnvoyRetriableHeaderNames) \
HEADER_FUNC(EnvoyOriginalMethod) \
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Drive by: please don't add this to the O(1) header map, as it only needs to be set.

Also, this change will need documentation and release notes. Thank you!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. Could you point out where you would like to see the documentation? I could not find any prior mention of headers in https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/grpc_json_transcoder_filter. Should I create a section there anyway?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah I think documenting this within the filter config docs would be fine for now. Thank you!

Phillip Huang added 3 commits May 11, 2020 14:24
…ase notes

Signed-off-by: Phillip Huang <phillip@dropbox.com>
Signed-off-by: Phillip Huang <phillip@dropbox.com>
Signed-off-by: Phillip Huang <phillip@dropbox.com>
@JavaScriptBach JavaScriptBach changed the title router: add x-envoy-original-method header and use in grpc-json transcoder grpc-json: preserve request method in x-envoy-original-method header May 11, 2020
Signed-off-by: Phillip Huang <phillip@dropbox.com>
In this case, HTTP response header `Content-Type` will use the `content-type` from the first
`google.api.HttpBody <https://github.com/googleapis/googleapis/blob/master/google/api/httpbody.proto>`.

Metadata
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

let's call this header. Metadata is confusing with dynamic metadata

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Can you review again?

Signed-off-by: Phillip Huang <phillip@dropbox.com>
@stale
Copy link
Copy Markdown

stale Bot commented May 30, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale Bot added the stale stalebot believes this issue/PR has not been touched recently label May 30, 2020
@lizan
Copy link
Copy Markdown
Member

lizan commented Jun 1, 2020

Can you merge master and resolve conflicts? Then we can merge this. Thank you!

@stale stale Bot removed the stale stalebot believes this issue/PR has not been touched recently label Jun 1, 2020
Phillip Huang added 3 commits June 1, 2020 11:36
Signed-off-by: Phillip Huang <phillip@dropbox.com>
Signed-off-by: Phillip Huang <phillip@dropbox.com>
Signed-off-by: Phillip Huang <phillip@dropbox.com>
@JavaScriptBach
Copy link
Copy Markdown
Contributor Author

Can you merge master and resolve conflicts? Then we can merge this. Thank you!

Done. Thank you!

@lizan lizan merged commit 6a69cc5 into envoyproxy:master Jun 2, 2020
@JavaScriptBach JavaScriptBach deleted the envoy-original-method branch June 3, 2020 06:25
aunu53 pushed a commit to aunu53/envoy that referenced this pull request Jun 4, 2020
…nvoyproxy#11126)

Commit Message: grpc-json: preserve http request method in `x-envoy-original-method` header so that applications have access to it.
Additional Description: The grpc-json transcoder currently forwards HTTP path to applications via "x-envoy-original-path" header. We would find it useful if it also forwarded the HTTP method.
Risk Level: Low
Testing: Updated grpc-json-transcoder unit tests
Docs Changes: Added docs
Release Notes: Added release notes

Signed-off-by: Phillip Huang <phillip@dropbox.com>

Signed-off-by: Auni Ahsan <auni@google.com>
yashwant121 pushed a commit to yashwant121/envoy that referenced this pull request Jul 24, 2020
…nvoyproxy#11126)

Commit Message: grpc-json: preserve http request method in `x-envoy-original-method` header so that applications have access to it.
Additional Description: The grpc-json transcoder currently forwards HTTP path to applications via "x-envoy-original-path" header. We would find it useful if it also forwarded the HTTP method.
Risk Level: Low
Testing: Updated grpc-json-transcoder unit tests
Docs Changes: Added docs
Release Notes: Added release notes

Signed-off-by: Phillip Huang <phillip@dropbox.com>
Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>
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