Fix handling of gRPC trailers-only responses #101

pkwarren · 2023-09-15T20:40:56Z

connect-kotlin wasn't properly handling gRPC trailers-only responses, leading to the inability to read grpc-status headers properly. Update the GRPCCompletionParser to first look for grpc-status in headers then trailers to handle these cases.

Update the connect-kotlin conformance tests to run combinations of both Connect/gRPC/gRPC-Web protocols and Connect/gRPC servers instead of just Connect/gRPC against a Connect server (which always sends trailers). By enabling this earlier, we would've detected the trailers-only issue earlier with the conformance test.

Fix gRPC protocol handlers to not filter out headers with trailer- prefix - this should only happen for the Connect protocol. Stop sending TE: trailers on gRPC-web requests (they don't use trailers). Preserve header value case in gRPC-web (leading to errors parsing base64-encoded error details).

connect-kotlin wasn't properly handling gRPC trailers-only responses (which are actually responses with *no* trailers, just headers), leading to the inability to read grpc-status headers properly. Update the connect-kotlin conformance tests to run combinations of both Connect/gRPC client and server instead of just Connect servers. By enabling this earlier, we would've detected the trailers-only issue earlier with the conformance test.

conformance/google-java/src/test/kotlin/com/connectrpc/conformance/Conformance.kt

library/src/main/kotlin/com/connectrpc/protocols/ConnectInterceptor.kt

library/src/main/kotlin/com/connectrpc/protocols/GRPCInterceptor.kt

conformance/google-java/src/test/kotlin/com/connectrpc/conformance/Conformance.kt

library/src/main/kotlin/com/connectrpc/protocols/GRPCInterceptor.kt

pkwarren · 2023-09-19T02:01:32Z

conformance/google-java/src/test/kotlin/com/connectrpc/conformance/Conformance.kt

-                arrayOf(NetworkProtocol.GRPC, NetworkProtocol.GRPC)
+                arrayOf(NetworkProtocol.CONNECT, ServerType.CONNECT_GO),
+                arrayOf(NetworkProtocol.GRPC, ServerType.CONNECT_GO),
+                arrayOf(NetworkProtocol.GRPC_WEB, ServerType.CONNECT_GO),


Added gRPC web protocol tests as well (and found some issues as a result).

pkwarren · 2023-09-19T02:02:36Z

conformance/google-java/src/test/kotlin/com/connectrpc/conformance/Conformance.kt

            assertThat(response.code).isEqualTo(Code.UNIMPLEMENTED)
            countDownLatch.countDown()
        }
        countDownLatch.await(500, TimeUnit.MILLISECONDS)
        assertThat(countDownLatch.count).isZero()
    }

+    @Test
+    fun unimplementedServerStreamingService(): Unit = runBlocking {


Added this test which caught the streaming behavior issue: #101 (comment)

pkwarren · 2023-09-19T02:04:11Z

library/src/main/kotlin/com/connectrpc/protocols/GRPCWebInterceptor.kt

@@ -276,9 +246,9 @@ internal class GRPCWebInterceptor(
            }
            val i = line.indexOf(":")
            if (i > 0) {
-                val name = line.substring(0, i).trim().lowercase()
-                val value = line.substring(i + 1).trim().lowercase()


This caused issues parsing error details in gRPC-Web (failing conformance tests). We were setting a base64 encoded protobuf to lowercase, which ended up reading as an invalid proto message on the other end.

pkwarren · 2023-09-19T02:04:44Z

library/src/main/kotlin/com/connectrpc/protocols/GRPCWebInterceptor.kt

@@ -259,7 +230,6 @@ internal class GRPCWebInterceptor(
        if (headers.keys.none { it.equals(GRPC_WEB_USER_AGENT, ignoreCase = true) }) {
            headers[GRPC_WEB_USER_AGENT] = listOf("grpc-kotlin-connect/${ConnectConstants.VERSION}")
        }
-        headers[GRPC_TE_HEADER] = listOf("trailers")


gRPC-web specifically doesn't support trailers - we shouldn't send this header.

connectrpc/connect-swift#179

pkwarren · 2023-09-19T02:05:20Z

library/src/main/kotlin/com/connectrpc/protocols/GRPCWebInterceptor.kt

@@ -188,26 +185,14 @@ internal class GRPCWebInterceptor(
            streamResultFunction = { res ->
                val streamResult = res.fold(
                    onHeaders = { result ->
-                        val responseHeaders = result.headers.filter { entry -> !entry.key.startsWith("trailer") }


Only Connect, not the gRPC* protocols have this convention with trailers and the trailer- prefix.

rebello95 · 2023-09-19T13:15:43Z

conformance/google-java/src/test/kotlin/com/connectrpc/conformance/Conformance.kt

    }

    @Before
    fun before() {
-        val host = "https://localhost:${CONFORMANCE_CONTAINER.getMappedPort(8081)}"
+        val serverPort = if (serverType == ServerType.CONNECT_GO) CONFORMANCE_CONTAINER_CONNECT.getMappedPort(8081) else CONFORMANCE_CONTAINER_GRPC.getMappedPort(8081)


Not a big deal, but calling out that Swift uses 8083 for grpc-go because that's where it's hosted in the container: https://github.com/connectrpc/connect-swift/blob/f7aab0e53c38f15d5ab62e8e8afa2f984c7e34f3/Tests/ConnectLibraryTests/ConnectConformance/ConformanceConfiguration.swift#L58

We could update to match, however that is coming from https://github.com/connectrpc/connect-swift/blob/f7aab0e53c38f15d5ab62e8e8afa2f984c7e34f3/Makefile#L49-L54. We're no longer needing to spawn containers from the Makefile in this project - we're using testcontainers to do that, so port 8081 is actually ephemeral on the host.

rebello95 · 2023-09-19T13:16:21Z

conformance/google-java/src/test/kotlin/com/connectrpc/conformance/Conformance.kt

-                                expectedErrorDetail
-                            )
-                            countDownLatch.countDown()
+                            try {


Wouldn't we want this to explode if any of the asserts fail rather than allowing tests to continue?

It will still fail the test, but this will prevent it waiting 5 seconds on the countDownLatch for the failure to register. Verified by adding a failing assert.

rebello95 · 2023-09-19T13:16:32Z

conformance/google-java/src/test/kotlin/com/connectrpc/conformance/Conformance.kt

-                            assertThat(result.connectError()!!.code).isEqualTo(Code.DEADLINE_EXCEEDED)
-                            assertThat(result.code).isEqualTo(Code.DEADLINE_EXCEEDED)
-                            countDownLatch.countDown()
+                            try {


rebello95 · 2023-09-19T13:17:20Z

conformance/google-java/src/test/kotlin/com/connectrpc/conformance/Conformance.kt

+                for (res in stream.resultChannel()) {
+                    res.maybeFold(
+                        onCompletion = { result ->
+                            try {


Same question about trying here

rebello95 · 2023-09-19T13:19:43Z

library/src/main/kotlin/com/connectrpc/protocols/GRPCCompletion.kt

 )
+
+internal fun grpcCompletionToConnectError(completion: GRPCCompletion?, serializationStrategy: SerializationStrategy, error: Throwable?): ConnectError? {
+    val code = completion?.code ?: Code.UNKNOWN


Could move this down to after the error check since it's not used immediately

rebello95 · 2023-09-19T13:20:47Z

library/src/main/kotlin/com/connectrpc/protocols/GRPCCompletionParser.kt

@@ -27,24 +28,34 @@ class GRPCCompletionParser(
    private val errorDetailParser: ErrorDetailParser
 ) {
    /**
-     * Parses the completion of a GRPC response from the Trailers.
+     * Parses the completion of a GRPC response from the Headers (for trailers-only responses) or Trailers.
     *
     * For GRPCWeb, the caller will have to transform the final message into trailers to parse a completion.
     *
     * For GRPC H2, the caller can just take the trailers from the completed stream to parse a completion.


This should probably be updated

rebello95 · 2023-09-19T13:22:28Z

library/src/main/kotlin/com/connectrpc/protocols/GRPCInterceptor.kt

+                        if (completion != null) {
+                            val connectError = grpcCompletionToConnectError(completion, serializationStrategy, result.error)
+                            return@fold StreamResult.Complete(
+                                code = connectError?.code ?: Code.OK,


Should this be completion!!.code?

No - in some cases this is a ConnectError (error is ConnectError) which has its own code which takes precedence. This is important in some of the tests that exercise deadlines.

rebello95 · 2023-09-19T13:31:22Z

library/src/main/kotlin/com/connectrpc/protocols/GRPCWebInterceptor.kt

@@ -259,7 +230,6 @@ internal class GRPCWebInterceptor(
        if (headers.keys.none { it.equals(GRPC_WEB_USER_AGENT, ignoreCase = true) }) {
            headers[GRPC_WEB_USER_AGENT] = listOf("grpc-kotlin-connect/${ConnectConstants.VERSION}")
        }
-        headers[GRPC_TE_HEADER] = listOf("trailers")


connectrpc/connect-swift#179

rebello95 · 2023-09-19T13:34:30Z

Also, can you please update the PR title/description?

pkwarren requested review from smaye81, akshayjshah and smallsamantha September 15, 2023 20:40

pkwarren commented Sep 15, 2023

View reviewed changes

pkwarren requested review from rebello95 and removed request for akshayjshah September 15, 2023 21:04

rebello95 reviewed Sep 15, 2023

View reviewed changes

pkwarren and others added 6 commits September 18, 2023 12:46

Merge branch 'main' into pkw/grpc-trailers-only

63fd9a7

updates to latest main

fb5311e

introduce new enum for conformance server type

c12438f

passing

95c9a70

updates

cdb1246

additional cleanup

3ceadd9

pkwarren requested a review from rebello95 September 19, 2023 02:01

pkwarren commented Sep 19, 2023

View reviewed changes

rebello95 approved these changes Sep 19, 2023

View reviewed changes

address review comments

9e6724a

pkwarren requested a review from rebello95 September 19, 2023 15:45

rebello95 approved these changes Sep 19, 2023

View reviewed changes

pkwarren merged commit 2fd1600 into main Sep 19, 2023
9 checks passed

pkwarren deleted the pkw/grpc-trailers-only branch September 19, 2023 16:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix handling of gRPC trailers-only responses #101

Fix handling of gRPC trailers-only responses #101

pkwarren commented Sep 15, 2023 •

edited

Loading

pkwarren Sep 19, 2023

pkwarren Sep 19, 2023

pkwarren Sep 19, 2023

pkwarren Sep 19, 2023 •

edited

Loading

rebello95 Sep 19, 2023

pkwarren Sep 19, 2023

rebello95 Sep 19, 2023

pkwarren Sep 19, 2023

rebello95 Sep 19, 2023

pkwarren Sep 19, 2023

rebello95 Sep 19, 2023

rebello95 Sep 19, 2023

rebello95 Sep 19, 2023

rebello95 Sep 19, 2023

rebello95 Sep 19, 2023

pkwarren Sep 19, 2023

rebello95 Sep 19, 2023

rebello95 commented Sep 19, 2023

Fix handling of gRPC trailers-only responses #101

Fix handling of gRPC trailers-only responses #101

Conversation

pkwarren commented Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkwarren Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rebello95 commented Sep 19, 2023

pkwarren commented Sep 15, 2023 •

edited

Loading

pkwarren Sep 19, 2023 •

edited

Loading