Skip to content

Suppress connection and content-length headers #2224

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

Merged
merged 8 commits into from
Oct 8, 2020

Conversation

youngbupark
Copy link
Contributor

@youngbupark youngbupark commented Oct 8, 2020

Description

Suppress connection and content-length headers.

In http->gRPC invocation scenario, translating HTTP 1.1 request headers to gRPC request metadata, both headers are copied to GRPC metadata. However, both headers are applicable only for http 1.1. gRPC over Kestrel server validate these headers for http 2 connection unlike the other gRPC implementation. So we can see the problem only in .net gRPC service application.

This PR will added dapr- prefix to two headers to suppress them.

Reference:

Issue reference

#2221

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

@youngbupark youngbupark requested a review from rynowak October 8, 2020 16:02
@codecov
Copy link

codecov bot commented Oct 8, 2020

Codecov Report

Merging #2224 into master will increase coverage by 0.35%.
The diff coverage is 62.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2224      +/-   ##
==========================================
+ Coverage   46.99%   47.34%   +0.35%     
==========================================
  Files          69       69              
  Lines        6090     6102      +12     
==========================================
+ Hits         2862     2889      +27     
+ Misses       2970     2952      -18     
- Partials      258      261       +3     
Impacted Files Coverage Δ
pkg/messaging/v1/util.go 51.04% <ø> (ø)
pkg/runtime/runtime.go 40.89% <62.50%> (+2.03%) ⬆️
pkg/scopes/scopes.go 87.50% <0.00%> (+4.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3bf0c0d...0ab6061. Read the comment docs.

Copy link
Member

@tcnghia tcnghia left a comment

Choose a reason for hiding this comment

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

I would also extract the switch to a membership check in a set of strings, but we can also leave for later PR.

@rynowak
Copy link
Contributor

rynowak commented Oct 8, 2020

Falls into this category: https://tools.ietf.org/html/rfc7540#section-8.1.2.2

  This means that an intermediary transforming an HTTP/1.x message to
   HTTP/2 will need to remove any header fields nominated by the
   Connection header field, along with the Connection header field
   itself.  Such intermediaries SHOULD also remove other connection-
   specific header fields, such as Keep-Alive, Proxy-Connection,
   Transfer-Encoding, and Upgrade, even if they are not nominated by the
   Connection header field.

We're not exactly translating HTTP 1.1-> 2.0 - but we are tunneling an individual HTTP message.

@youngbupark
Copy link
Contributor Author

Falls into this category: https://tools.ietf.org/html/rfc7540#section-8.1.2.2

  This means that an intermediary transforming an HTTP/1.x message to
   HTTP/2 will need to remove any header fields nominated by the
   Connection header field, along with the Connection header field
   itself.  Such intermediaries SHOULD also remove other connection-
   specific header fields, such as Keep-Alive, Proxy-Connection,
   Transfer-Encoding, and Upgrade, even if they are not nominated by the
   Connection header field.

We're not exactly translating HTTP 1.1-> 2.0 - but we are tunneling an individual HTTP message.

Let me add these headers to the http/1.1 specific header list.
Keep-Alive, Proxy-Connection, Transfer-Encoding, and Upgrade

@youngbupark youngbupark requested a review from tcnghia October 8, 2020 20:13
Copy link
Member

@yaron2 yaron2 left a comment

Choose a reason for hiding this comment

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

lgtm

@youngbupark youngbupark merged commit bdd4654 into master Oct 8, 2020
@youngbupark youngbupark deleted the youngp/fix-net-issue branch October 8, 2020 21:03
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.

.net gRPC appcallback server OnInvoke method doesn't work for http->grpc invocation scenario
4 participants