Problem forwardServerToClient() since last month's modernization

[esnible]

I spent some time today trying to understand why my code fails with the recent modernization.

My change is related to switching out frame for anypb.Any in handler.go at https://github.com/mwitkow/grpc-proxy/pull/52/files#diff-e803501e5fa26d41cd2723669bd3e5c3df2252d9e8004cc8f42dbada07f80802R115

Before this change things behaved as I expected. With the use of Any to hold the data my code fails because my data doesn't want to go into an Any. It was probably happy going into a frame because that is just a byte array. I never make it past forwardServerToClient().

I am not an expert in the details of protobuf serialization. I have data that is already in a different protobuf format, a format that includes bytes, that somehow isn't compatible with anypb.Any. I will describe what I am doing, perhaps it will be clear which one of us is doing something wrong.

The data that I am trying to forward is Open Telemetry Protobufs, created for example using grpcurl -d @ -plaintext -import-path "$OTEL_DIR"/opentelemetry-proto/ -proto "$OTEL_DIR"/opentelemetry-proto/opentelemetry/proto/collector/trace/v1/trace_service.proto localhost:8317 opentelemetry.proto.collector.trace.v1.TraceService/Export where the data that I attempt to proxy that grpcurl encodes is

{"resourceSpans":[{"instrumentationLibrarySpans":[{"spans":[
  {
    "trace_id":"26/mx4ytaQfaV87uqKocZA==",
    "span_id": "wPE19XLMS30=",
    "name": "test"
  }
]}]}]}

(trace_id and span_id are bytes, encoded as base64 the way grpcurl likes it.). Source of the .proto is https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/trace/v1/trace.proto

Your proxy/handler.go forwardClientToServer() calls grpc.RecvMsg(), which eventually tries to unmarshal using codec proxy>proto, bytes are [10 38 18 36 18 34 10 16 219 175 230 199 140 173 105 7 218 87 206 238 168 170 28 100 18 8 192 241 53 245 114 204 75 125 42 4 116 101 115 116]) into a *anypb.Any. This fails when it eventually gets to protobuf impl.consumeStringValidateUTF8() with some bytes that aren't a valid string. (I am not sure why anypb.Any thought there was a UTF-8-valid string in there.)

Any thoughts?

[esnible]

I can verify the bytes that are trying to be decoded are a valid protobuf and match what I sent.

python -c "print(''.join('{:02x}'.format(c) for c in [18, 36, 18, 34, 10, 16, 219, 175, 230, 199, 140, 173, 105, 7, 218, 87, 206, 238, 168, 170, 28, 100, 18, 8, 192, 241, 53, 245, 114, 204, 75, 125, 42, 4, 116, 101, 115, 116]))" | xxd -r -p | protoc --decode_raw
2 {
  2 {
    1: "\333\257\346\307\214\255i\007\332W\316\356\250\252\034d"
    2: "\300\3615\365r\314K}"
    5: "test"
  }
}

echo -n "26/mx4ytaQfaV87uqKocZA==" | base64 -D | od -b  
0000000   333 257 346 307 214 255 151 007 332 127 316 356 250 252 034 144

Thus I believe the data comes in, and is a valid protobuf, but it is not a protobuf wrapped in anypb.Any.

[esnible]

@marcusirgens What was the rationale for replacing frame with anypb.Any in handler.go?

[marcusirgens]

@esnible This is unfortunately one of the edge cases I didn't find, it seems. The hope was to use common, well-supported general types instead of these rather opaque types for forwarding data, while removing the deprecated codecs. I'll try to reproduce once I find the time.

[danw]

It looks like this is because Any isn't actually a generic protobuf, but a typed holder of a serialized protobuf. It's just a proto that contains a string and bytes, which is why the protobuf library is trying to convert things into valid strings.

Using emptypb.Empty instead of anypb.Any seems to work, as it won't try to verify anything and the unknown fields get preserved from proto.Unmarshal to proto.Marshal.

[a4chet]

@esnible This is unfortunately one of the edge cases I didn't find, it seems. The hope was to use common, well-supported general types instead of these rather opaque types for forwarding data, while removing the deprecated codecs. I'll try to reproduce once I find the time.

Just ran into this also. I'd say that TransparentHandler is a major use case of this software and shouldn't incur marshalling overhead to enable reverse proxy.

@danw - Thank you for the workaround 👍

[ntaylor-barnett]

You are a life saver @danw! changing it to emptypb.Empty in the handler worked like a charm. Have raised a PR.

P.S: @mwitkow, this is a very useful repo. Keep up the good work on it!

[HenriBeck]

@ntaylor-barnett @danw, wouldn't we also need to change the type in the forwardServerToClient?

I also ran into this bug when trying to proxy the response from the server to the client.

[taeyeon-Kim]

@danw Thank you for the workaround 👍

@marcusirgens please, Fix it

[andremarianiello]

This can be closed as it was fixed in #62