-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
gRPC-web proxy #8077
gRPC-web proxy #8077
Conversation
…eem/7345-grpc-web-proxy
Codecov Report
@@ Coverage Diff @@
## master #8077 +/- ##
==========================================
- Coverage 61.91% 61.91% -0.01%
==========================================
Files 609 610 +1
Lines 35100 35135 +35
==========================================
+ Hits 21732 21753 +21
- Misses 11085 11096 +11
- Partials 2283 2286 +3
|
…eem/7345-grpc-web-proxy
…eem/7345-grpc-web-proxy
…eem/7345-grpc-web-proxy
…eem/7345-grpc-web-proxy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Some initial questions about grpc-web-proxy.
Also: how hard would it be to add a test?
server/config/toml.go
Outdated
# BindAddress defines address to bind the server to. | ||
bind-address = "{{ .GRPC.GRPCWebProxy.BindAddress }}" | ||
|
||
# HTTPPort defines TCP port to listen on for HTTP1.1 debug calls. | ||
http-port = {{ .GRPC.GRPCWebProxy.HTTPPort }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to bind to the same port as the grpc-gateway rest server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Co-authored-by: Anil Kumar Kammari <anil@vitwit.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, thanks for the test!
my main question is the pros & cons of binding grpc-web on a separate port than the HTTP one.
// Address defines the gRPC-web server to listen on | ||
Address string `mapstructure:"address"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also just keep the enable
config, and if true, bind the server to the HTTP port. I'm not sure it's useful to expose so many ports. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to bind to the HTTP server. Getting this error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created this issue to discuss it: #8254
Co-authored-by: Amaury <amaury.martiny@protonmail.com>
Co-authored-by: Amaury <amaury.martiny@protonmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@alexanderbez Since you requested changes in a previous iteration, could you have another look at this PR? |
* init * WIP config * WIP add proxy server * fmt * WIP * setup proxy server * clean go.mod * lint * lint * lint * custom codec * lint * add comments * change grpc-proxy port * add grpc-web * update server/start.go * add tests * add test with client * Update server/start.go Co-authored-by: Anil Kumar Kammari <anil@vitwit.com> * Update server/start.go Co-authored-by: Amaury <amaury.martiny@protonmail.com> * Update server/start.go Co-authored-by: Amaury <amaury.martiny@protonmail.com> * review changes * review changes * Update server/start.go Co-authored-by: Anil Kumar Kammari <anil@vitwit.com> Co-authored-by: Amaury <amaury.martiny@protonmail.com> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
closes: #7345
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes