Skip to content

Enable envoy grpc-web filter for HTTP2 listener#886

Merged
Xunzhuo merged 1 commit intoenvoyproxy:mainfrom
arkodg:grpc-web
Jan 12, 2023
Merged

Enable envoy grpc-web filter for HTTP2 listener#886
Xunzhuo merged 1 commit intoenvoyproxy:mainfrom
arkodg:grpc-web

Conversation

@arkodg
Copy link
Copy Markdown
Contributor

@arkodg arkodg commented Jan 10, 2023

Fixes: #879

Signed-off-by: Arko Dasgupta arko@tetrate.io

@arkodg arkodg requested a review from a team as a code owner January 10, 2023 23:37
@arkodg arkodg requested a review from sunjayBhatia January 11, 2023 00:05
Xunzhuo
Xunzhuo previously approved these changes Jan 11, 2023
@Xunzhuo Xunzhuo enabled auto-merge (squash) January 11, 2023 02:12
@Xunzhuo
Copy link
Copy Markdown
Member

Xunzhuo commented Jan 11, 2023

Looks like some conflicts need to be resolved.

@arkodg
Copy link
Copy Markdown
Contributor Author

arkodg commented Jan 11, 2023

yeah @Xunzhuo also hoping to discuss this in the next community meeting

@HoangTheQuyen
Copy link
Copy Markdown

Is it possible to add documentation such as a quickstart or tcp-routing item?

@arkodg
Copy link
Copy Markdown
Contributor Author

arkodg commented Jan 11, 2023

@HoangTheQuyen will make sure to add docs as part of #642, since some work items are still WIP

@HoangTheQuyen
Copy link
Copy Markdown

@arkodg Is it this PR: Here?

Comment thread internal/xds/translator/listener.go Outdated
Comment thread internal/xds/translator/listener.go Outdated
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.

from the godoc on IsHTTP2:

// IsHTTP2 is set if the upstream client as well as the downstream server are configured to serve HTTP2 traffic.

this is a little misleading i think? since the codec type is always set to "auto" and will infer the downstream protocol which could be HTTP/1 and upgraded when talking with the backend to gRPC now that this filter is applied

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

How should I configure it so that it can touch it ? @sunjayBhatia

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.

good point @sunjayBhatia, we could limit IsHTTP2 to downstreams speaking HTTP/2, thereby limiting grpc-web for HTTP/2 only clients

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.

updated the PR with the codec type change from auto to http2

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.

seems useful for clients to be able to use gRPC-Web over HTTP/1 -> Envoy -> gRPC backend over HTTP/2 as well! but seems like that could be a separate feature if desired

Fixes: envoyproxy#879

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #886 (8712574) into main (ba7c576) will increase coverage by 0.02%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##             main     #886      +/-   ##
==========================================
+ Coverage   63.49%   63.52%   +0.02%     
==========================================
  Files          53       53              
  Lines        7347     7361      +14     
==========================================
+ Hits         4665     4676      +11     
- Misses       2393     2395       +2     
- Partials      289      290       +1     
Impacted Files Coverage Δ
internal/xds/translator/listener.go 81.57% <83.33%> (-0.14%) ⬇️
internal/provider/kubernetes/controller.go 45.84% <0.00%> (-1.28%) ⬇️
internal/provider/kubernetes/predicates.go 64.20% <0.00%> (+2.27%) ⬆️
internal/provider/kubernetes/helpers.go 81.96% <0.00%> (+4.91%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@arkodg arkodg disabled auto-merge January 11, 2023 23:03
@arkodg arkodg requested a review from Xunzhuo January 11, 2023 23:44
Copy link
Copy Markdown
Member

@zirain zirain left a comment

Choose a reason for hiding this comment

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

lgtm

@Xunzhuo Xunzhuo merged commit 8e4e0ce into envoyproxy:main Jan 12, 2023
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.

How configure to expose grpc to grpc-web to FE call

6 participants