From 7fc7947087ef54530386816a6ce96a22bd887829 Mon Sep 17 00:00:00 2001 From: Denis Date: Thu, 7 Sep 2023 20:06:54 +0100 Subject: [PATCH 1/6] Add an option to override content type for a client --- client/options.go | 13 +++++++++++++ client/proxy.go | 14 ++++++++++---- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/client/options.go b/client/options.go index 449fbd2..50f3da1 100644 --- a/client/options.go +++ b/client/options.go @@ -22,6 +22,7 @@ type connectOptions struct { forceHTTP2 bool forceDowngrade bool useWebSocket bool + contentType string } // ConnectOption is an option that can be passed to the `ConnectViaProxy` method. @@ -66,6 +67,12 @@ func ForceDowngrade(force bool) ConnectOption { return forceDowngradeOption(force) } +// OverriddenContentType returns a connection option that instructs the +// client to use a custom content type for sending requests to the server. +func OverriddenContentType(contentType string) ConnectOption { + return overriddenContentType(contentType) +} + type dialOptsOption []grpc.DialOption func (o dialOptsOption) apply(opts *connectOptions) { @@ -95,3 +102,9 @@ type forceDowngradeOption bool func (o forceDowngradeOption) apply(opts *connectOptions) { opts.forceDowngrade = bool(o) } + +type overriddenContentType string + +func (o overriddenContentType) apply(opts *connectOptions) { + opts.contentType = string(o) +} diff --git a/client/proxy.go b/client/proxy.go index 1ba7489..6d6a4f6 100644 --- a/client/proxy.go +++ b/client/proxy.go @@ -79,7 +79,7 @@ func writeError(w http.ResponseWriter, err error) { w.Header().Set("Grpc-Message", grpcproto.EncodeGrpcMessage(errMsg)) } -func createReverseProxy(endpoint string, transport http.RoundTripper, insecure, forceDowngrade bool) *httputil.ReverseProxy { +func createReverseProxy(endpoint string, transport http.RoundTripper, insecure, forceDowngrade bool, contentType string) *httputil.ReverseProxy { scheme := "https" if insecure { scheme = "http" @@ -91,6 +91,12 @@ func createReverseProxy(endpoint string, transport http.RoundTripper, insecure, req.Header.Del("TE") req.Header.Del("Accept") req.Header.Add(grpcweb.GRPCWebOnlyHeader, "true") + + if len(contentType) > 0 { + // remove old content type (e.g., application/grpc), and set overridden content type. + req.Header.Del("Content-Type") + req.Header.Add("Content-Type", contentType) + } } else { req.Header.Add("Accept", "application/grpc") } @@ -142,12 +148,12 @@ func createTransport(tlsClientConf *tls.Config, forceHTTP2 bool, extraH2ALPNs [] return transport, nil } -func createClientProxy(endpoint string, tlsClientConf *tls.Config, forceHTTP2, forceDowngrade bool, extraH2ALPNs []string) (*http.Server, pipeconn.DialContextFunc, error) { +func createClientProxy(endpoint string, tlsClientConf *tls.Config, forceHTTP2, forceDowngrade bool, extraH2ALPNs []string, contentType string) (*http.Server, pipeconn.DialContextFunc, error) { transport, err := createTransport(tlsClientConf, forceHTTP2, extraH2ALPNs) if err != nil { return nil, nil, errors.Wrap(err, "creating transport") } - proxy := createReverseProxy(endpoint, transport, tlsClientConf == nil, forceDowngrade) + proxy := createReverseProxy(endpoint, transport, tlsClientConf == nil, forceDowngrade, contentType) return makeProxyServer(proxy) } @@ -171,7 +177,7 @@ func ConnectViaProxy(ctx context.Context, endpoint string, tlsClientConf *tls.Co if connectOpts.useWebSocket { proxy, dialCtx, err = createClientWSProxy(endpoint, tlsClientConf) } else { - proxy, dialCtx, err = createClientProxy(endpoint, tlsClientConf, connectOpts.forceHTTP2, connectOpts.forceDowngrade, connectOpts.extraH2ALPNs) + proxy, dialCtx, err = createClientProxy(endpoint, tlsClientConf, connectOpts.forceHTTP2, connectOpts.forceDowngrade, connectOpts.extraH2ALPNs, connectOpts.contentType) } if err != nil { From 8f2535f9b5abfcebb86dfc1876ded27606e8e6e1 Mon Sep 17 00:00:00 2001 From: Denis Date: Wed, 13 Sep 2023 00:49:36 +0100 Subject: [PATCH 2/6] Rename OverriddenContentType to WithContentType --- client/options.go | 10 +++++----- client/proxy.go | 14 ++++++++------ 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/client/options.go b/client/options.go index 50f3da1..fdfe6c8 100644 --- a/client/options.go +++ b/client/options.go @@ -67,10 +67,10 @@ func ForceDowngrade(force bool) ConnectOption { return forceDowngradeOption(force) } -// OverriddenContentType returns a connection option that instructs the +// WithContentType returns a connection option that instructs the // client to use a custom content type for sending requests to the server. -func OverriddenContentType(contentType string) ConnectOption { - return overriddenContentType(contentType) +func WithContentType(contentType string) ConnectOption { + return contentTypeOption(contentType) } type dialOptsOption []grpc.DialOption @@ -103,8 +103,8 @@ func (o forceDowngradeOption) apply(opts *connectOptions) { opts.forceDowngrade = bool(o) } -type overriddenContentType string +type contentTypeOption string -func (o overriddenContentType) apply(opts *connectOptions) { +func (o contentTypeOption) apply(opts *connectOptions) { opts.contentType = string(o) } diff --git a/client/proxy.go b/client/proxy.go index 6d6a4f6..1b9270e 100644 --- a/client/proxy.go +++ b/client/proxy.go @@ -91,16 +91,18 @@ func createReverseProxy(endpoint string, transport http.RoundTripper, insecure, req.Header.Del("TE") req.Header.Del("Accept") req.Header.Add(grpcweb.GRPCWebOnlyHeader, "true") - - if len(contentType) > 0 { - // remove old content type (e.g., application/grpc), and set overridden content type. - req.Header.Del("Content-Type") - req.Header.Add("Content-Type", contentType) - } } else { req.Header.Add("Accept", "application/grpc") } req.Header.Add("Accept", "application/grpc-web") + + if len(contentType) > 0 { + // Replacing old content type (e.g., application/grpc), to an overridden content type. + // Without removing old header, some gRPC-Web servers will not work, + // because an HTTP client will send both old and new header values. + req.Header.Set("Content-Type", contentType) + } + req.URL.Scheme = scheme req.URL.Host = endpoint }, From 767dd35f0bd8e893a1ed491fb289745a35ead00e Mon Sep 17 00:00:00 2001 From: Denis Date: Thu, 14 Sep 2023 22:58:24 +0100 Subject: [PATCH 3/6] Add int tests --- _integration-tests/echo_service_test.go | 42 +++++++++++++++++++++++++ server/server.go | 16 +++++++++- 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/_integration-tests/echo_service_test.go b/_integration-tests/echo_service_test.go index f7ab463..e1ceab4 100644 --- a/_integration-tests/echo_service_test.go +++ b/_integration-tests/echo_service_test.go @@ -192,6 +192,39 @@ func testWithEchoService(t *testing.T, serverPreferGRPCWeb bool) { expectClientStreamOK: false, expectBidiStreamOK: false, }, + { + targetID: "downgrading-grpc", + behindHTTP1ReverseProxy: false, + useProxy: true, + forceDowngrade: true, + customContentType: "application/grpc-web", + expectUnaryOK: true, + expectServerStreamOK: true, + expectClientStreamOK: false, + expectBidiStreamOK: false, + }, + { + targetID: "downgrading-grpc", + behindHTTP1ReverseProxy: true, + useProxy: true, + forceDowngrade: true, + customContentType: "application/grpc-web", + expectUnaryOK: true, + expectServerStreamOK: true, + expectClientStreamOK: false, + expectBidiStreamOK: false, + }, + { + targetID: "downgrading-grpc", + behindHTTP1ReverseProxy: true, + useProxy: true, + forceDowngrade: false, + customContentType: "application/grpc-web", + expectUnaryOK: true, + expectServerStreamOK: true, + expectClientStreamOK: false, + expectBidiStreamOK: false, + }, } for _, c := range cases { @@ -317,6 +350,7 @@ type testCase struct { useProxy bool useWebSocket bool forceDowngrade bool + customContentType string expectUnaryOK bool expectClientStreamOK bool @@ -343,6 +377,10 @@ func (c *testCase) Name() string { } else { sb.WriteString("-direct") } + + if len(c.customContentType) > 0 { + sb.WriteString("-custom-content-type") + } return sb.String() } @@ -381,6 +419,10 @@ func (c *testCase) Run(t *testing.T, cfg *testConfig) { } opts = append(opts, client.UseWebSocket(c.useWebSocket), client.ForceDowngrade(c.forceDowngrade)) + if len(c.customContentType) > 0 { + opts = append(opts, client.WithContentType(c.customContentType)) + } + cc, err = client.ConnectViaProxy(ctx, targetAddr, nil, opts...) } else { cc, err = grpc.DialContext(ctx, targetAddr, grpc.WithTransportCredentials(insecure.NewCredentials())) diff --git a/server/server.go b/server/server.go index 140aadb..284306e 100644 --- a/server/server.go +++ b/server/server.go @@ -148,6 +148,10 @@ func handleGRPCWeb(w http.ResponseWriter, req *http.Request, validPaths map[stri if err := finalize(); err != nil { glog.Errorf("Error sending trailers in downgraded gRPC web response: %v", err) } + var a = 5 + if a == 5 { + println("Q") + } } // CreateDowngradingHandler takes a gRPC server and a plain HTTP handler, and returns an HTTP handler that has the @@ -182,16 +186,26 @@ func CreateDowngradingHandler(grpcSrv *grpc.Server, httpHandler http.Handler, op return } - if contentType, _ := stringutils.Split2(req.Header.Get("Content-Type"), "+"); contentType != "application/grpc" { + if !isContentTypeValid(req.Header.Get("Content-Type")) { // Non-gRPC request to the same port. httpHandler.ServeHTTP(w, req) return } + // explicitly set application/grpc content type, + // because underlying grpc library supports only it. + // https://github.com/grpc/grpc-go/blob/9deee9ba5f5b654d38c737c701181dceebb57e44/internal/grpcutil/method.go#L61 + req.Header.Set("Content-Type", "application/grpc") + handleGRPCWeb(w, req, validGRPCWebPaths, grpcSrv, &serverOpts) }) } +func isContentTypeValid(contentType string) bool { + ct, _ := stringutils.Split2(contentType, "+") + return ct == "application/grpc" || ct == "application/grpc-web" +} + func isWebSocketUpgrade(header http.Header) (bool, error) { if header.Get("Sec-Websocket-Protocol") != grpcwebsocket.SubprotocolName { return false, nil From 6834f285f424ec8c3d0c81718d832246d0dc8e78 Mon Sep 17 00:00:00 2001 From: Denis Date: Tue, 19 Sep 2023 09:23:22 +0100 Subject: [PATCH 4/6] Cleanup --- server/server.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/server/server.go b/server/server.go index 284306e..f776a92 100644 --- a/server/server.go +++ b/server/server.go @@ -148,10 +148,6 @@ func handleGRPCWeb(w http.ResponseWriter, req *http.Request, validPaths map[stri if err := finalize(); err != nil { glog.Errorf("Error sending trailers in downgraded gRPC web response: %v", err) } - var a = 5 - if a == 5 { - println("Q") - } } // CreateDowngradingHandler takes a gRPC server and a plain HTTP handler, and returns an HTTP handler that has the From 117a319b827286ccd773e385e39bbe892ba7bb18 Mon Sep 17 00:00:00 2001 From: Piotr Rygielski <114479+vikin91@users.noreply.github.com> Date: Wed, 11 Oct 2023 12:46:33 +0200 Subject: [PATCH 5/6] Add negative testcase --- _integration-tests/echo_service_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/_integration-tests/echo_service_test.go b/_integration-tests/echo_service_test.go index e1ceab4..72cb914 100644 --- a/_integration-tests/echo_service_test.go +++ b/_integration-tests/echo_service_test.go @@ -225,6 +225,17 @@ func testWithEchoService(t *testing.T, serverPreferGRPCWeb bool) { expectClientStreamOK: false, expectBidiStreamOK: false, }, + { + targetID: "downgrading-grpc", + behindHTTP1ReverseProxy: true, + useProxy: true, + forceDowngrade: true, + customContentType: "dummy", + expectUnaryOK: false, + expectServerStreamOK: false, + expectClientStreamOK: false, + expectBidiStreamOK: false, + }, } for _, c := range cases { From edfdea990848a8bbcfd1269890aa4eba3c924bfd Mon Sep 17 00:00:00 2001 From: Piotr Rygielski <114479+vikin91@users.noreply.github.com> Date: Wed, 11 Oct 2023 12:50:00 +0200 Subject: [PATCH 6/6] Minor: Rephrase a comment --- server/server.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/server/server.go b/server/server.go index f776a92..b9dd356 100644 --- a/server/server.go +++ b/server/server.go @@ -188,9 +188,8 @@ func CreateDowngradingHandler(grpcSrv *grpc.Server, httpHandler http.Handler, op return } - // explicitly set application/grpc content type, - // because underlying grpc library supports only it. - // https://github.com/grpc/grpc-go/blob/9deee9ba5f5b654d38c737c701181dceebb57e44/internal/grpcutil/method.go#L61 + // Internally content type must be application/grpc, + // See: https://github.com/grpc/grpc-go/blob/9deee9b/internal/grpcutil/method.go#L61 req.Header.Set("Content-Type", "application/grpc") handleGRPCWeb(w, req, validGRPCWebPaths, grpcSrv, &serverOpts)