Add an option to override content type for a client

[Hixon10]

At present, current implementation of a client sends Content-Type: application/grpc to a server. Because of it, some gRPC Web Servers does not work (for example, Official C# web gRPC server). Also, gRPC Web spec says that it is expected to get either application/grpc-web, or application/grpc-web-text content type.

This PR adds a way to override content type, which will be sent to a server. This fix allows me to use your client with my web gRPC server, so I would be happy to merge it to upstream.

[vikin91]

Many thanks for your contribution! I looked at the changes and they look good - I left few detailed comments to address or reply.

However, I am missing tests here. I see that there are no tests for that pkg yet - would you be okay with writing few unit tests for the change that you proposed? We usually write table-driven unit tests.
As a minimum test coverage, I would be curious to see:

• Behavior of the content-type header combined with various values of the forceDowngrade parameter,
• Ensuring that the default value application/grpc is applied when contentType is empty,
• Making sure that the library does not crash when nonsense values of contentType are provided

[Hixon10]

Many thanks for your contribution! I looked at the changes and they look good - I left few detailed comments to address or reply.

However, I am missing tests here. I see that there are no tests for that pkg yet - would you be okay with writing few unit tests for the change that you proposed? We usually write table-driven unit tests. As a minimum test coverage, I would be curious to see:

* Behavior of the content-type header combined with various values of the `forceDowngrade` parameter,

* Ensuring that the default value `application/grpc` is applied when  `contentType` is empty,

* Making sure that the library does not crash when nonsense values of `contentType` are provided

Thanks for the review!

I am thinking, what is the best way to implement requested tests? Initially, I tried to add new tests to

go-grpc-http1/_integration-tests/echo_service_test.go

Lines 185 to 194 in c0b77f0

	{
	targetID: "downgrading-grpc",
	behindHTTP1ReverseProxy: true,
	useProxy: true,
	forceDowngrade: true,
	expectUnaryOK: true,
	expectServerStreamOK: true,
	expectClientStreamOK: false,
	expectBidiStreamOK: false,
	},

, something like:

		{
			targetID:                "downgrading-grpc",
			behindHTTP1ReverseProxy: true,
			useProxy:                true,
			forceDowngrade:          true,
			customContentType:       "application/grpc-web",
			expectUnaryOK:           true,
			expectServerStreamOK:    true,
			expectClientStreamOK:    false,
			expectBidiStreamOK:      false,
		},

the problem is tests are failed, because of https://github.com/dhaus67/go-grpc-http1/blob/78361be32b62187099a40e07fb4a9d6eda16221a/server/server.go#L185-L189

Basically, right now current server/server.go expects to have Content Type Header == application/grpc. If the server receives something else, it returns http.NotFoundHandler(). (in case of c# server implementation, it is vice versa; it expects to get application/grpc-web).

So, I can see the following ways to implement some tests:

1. Modify current httpHandler http.Handler, which we pass to CreateDowngradingHandler(..). Right now, it is http.NotFoundHandler(), but we can collect all request HTTP headers and return 404 after. It allows to check, which HTTP headers do we send from an HTTP client.
2. Test createClientProxy, instead of using current integration tests. For example, I could use something like httptest.Server, and just verify, that we send correct HTTP headers.

But it looks like that both ways don't allow to test gRPC. It still will be just HTTP header assert, without guarantees that gRPC still works.

[vikin91]

Thanks for the attempt at writing the tests! I have several thoughts on that.

The existing integration tests would definitely need to be adapted. We must make sure that by adding and using that feature we are not breaking the old behavior. Maybe instead of checking contentType != "application/grpc", we could check against more than one value (e.g., func validateContentType(ct string) error, so that this check also includes the cases you are naming? That would need to be verified with several different gRPC servers, to make sure that is still works.

Additionally to the integration tests one could add unit tests that would cover the connectOptions and the createReverseProxy function.

I think, that there is more value in the integration tests and the unit tests could be skipped. The problem you stumbled upon while attempting to write the test shows that the change in its current state is untested and cannot be added. The tests must by first updated and add test coverage for that change must be added.

[Hixon10]

Thanks for the attempt at writing the tests! I have several thoughts on that.

The existing integration tests would definitely need to be adapted. We must make sure that by adding and using that feature we are not breaking the old behavior. Maybe instead of checking contentType != "application/grpc", we could check against more than one value (e.g., func validateContentType(ct string) error, so that this check also includes the cases you are naming? That would need to be verified with several different gRPC servers, to make sure that is still works.

Additionally to the integration tests one could add unit tests that would cover the connectOptions and the createReverseProxy function.

I think, that there is more value in the integration tests and the unit tests could be skipped. The problem you stumbled upon while attempting to write the test shows that the change in its current state is untested and cannot be added. The tests must by first updated and add test coverage for that change must be added.

Sorry, I want to ask follow-up question to better understand, what I can do.

I tried func validateContentType(ct string) error locally. When I "allowed" both application/grpc, and application/grpc-web (e.g.:

		if contentType, _ := stringutils.Split2(req.Header.Get("Content-Type"), "+"); contentType != "application/grpc" && contentType != "application/grpc-web" {
			// Non-gRPC request to the same port.
			httpHandler.ServeHTTP(w, req)
			return
		}

)

I got HTTP 415. Apparently, inside gRPC server (part of gRPC impl., not this library) there is another check for contentType - https://github.com/grpc/grpc-go/blob/9deee9ba5f5b654d38c737c701181dceebb57e44/internal/transport/handler_server.go#L66-L71

Basically, we can use only "application/grpc+", or "application/grpc;" - https://github.com/grpc/grpc-go/blob/9deee9ba5f5b654d38c737c701181dceebb57e44/internal/grpcutil/method.go#L61

So, if we want to support application/grpc-web content type in stackrox/go-grpc-http1 (server part), we need to write something like that (pseudocode):

		if contentType, _ := stringutils.Split2(req.Header.Get("Content-Type"), "+"); contentType != "application/grpc" && contentType != "application/grpc-web" {
			// Non-gRPC request to the same port.
			httpHandler.ServeHTTP(w, req)
			return
		}

		req.Header.Set("Content-Type", "application/grpc") // override any content type to the right one

		handleGRPCWeb(w, req, validGRPCWebPaths, grpcSrv, &serverOpts)

With such code, all new tests are green. Should I do this change?

[vikin91]

With such code, all new tests are green. Should I do this change?

It is difficult for me to see the entire thing only based on a comment. It would be better to implement the change that you have in mind, so that we can see it in the code.

If I understand your idea correctly, you propose that this library would accept multiple content types, but it would pass only the application/grpc to the underlying grpc library. Is that correct?

[Hixon10]

Is that correct?

sorry, yes, you are right. I've pushed a code to show an idea.

[vikin91]

Hi @Hixon10 , I am sorry for no progress on reviewing this PR, we are having a hackathon week and some delays in other activities shall be expected.

[Hixon10]

Hi @Hixon10 , I am sorry for no progress on reviewing this PR, we are having a hackathon week and some delays in other activities shall be expected.

Hi! No problem at all! I will be happy to continue work on this PR any time, when you are available.

[vikin91]

Thank you for waiting for the review. I think this is safe to be merged - I run some additional integration tests locally and added a negative test scenario (when the content type is set to something bogus). Let's wait for the CI to evaluate this.

Suggested changes: I rephrased one comment for brevity.

[vikin91]

Test results look good, marking it green

[Hixon10]

@vikin91 thanks a lot for helping with this PR and merging it at the end!

[vikin91]

Thank you for the contribution @Hixon10 . I merged myself, as external contributions must be merged through team members, but if you have some additional things that you want to add or change, then feel free to open another PR.