Skip to content
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-dump fails to JSON encode google.protobuf.Any fields embedding proto messages #96

Closed
marcellanz opened this issue May 1, 2020 · 0 comments

Comments

@marcellanz
Copy link
Contributor

marcellanz commented May 1, 2020

This might be a duplicate of #88 but I think I have a fix for that and it may fail differently for me.

Given

  1. Communication to be dumped is based on the following protbuf files:
    https://github.com/cloudstateio/go-support/tree/master/protobuf
  2. A or some proto files might/are not be imported by any other protofiles. Seems not to be a problem really.
  3. A sample message sequence dumped with grpc-dump can be found here dump_001_no_proto_roots.json
  4. A message sequence dumped with -proto_roots can be found here dump_001_proto_root_with_errors.out

If grpc-dump is run with -proto_roots, it fails with:

WARN[0020] Failed to search for unknown fields in message  =proto_decoder error="failed to enrich decode descriptor: failed to search nested field init: failed to search nested field snapshot: internal: got nil message"
WARN[0020] Failed to search for unknown fields in message  =proto_decoder error="failed to enrich decode descriptor: failed to search nested field init: internal: got nil message"
WARN[0020] Failed to search for unknown fields in message  =proto_decoder error="failed to enrich decode descriptor: failed to search nested field init: failed to search nested field snapshot: internal: got nil message"
WARN[0020] Failed to search for unknown fields in message  =proto_decoder error="failed to enrich decode descriptor: failed to search nested field reply: failed to search nested field client_action: failed to search nested field forward: internal: got nil message"

If grpc-dump is run without -proto_roots it does not fail but message fields are just named with their corresponding tags:

{
      "message_origin": "server",
      "raw_message": "CpIBCAISLwotCisKKXR5cGUuZ29vZ2xlYXBpcy5jb20vZ29vZ2xlLnByb3RvYnVmLkVtcHR5Il0KQnR5cGUuZ29vZ2xlYXBpcy5jb20vY29tLmV4YW1wbGUuc2hvcHBpbmdjYXJ0LnBlcnNpc3RlbmNlLkl0ZW1BZGRlZBIXChUKCUExNDM2MjM0NxIGRGVsdXhlGAU=",
      "message": {
        "1": {
          "1": "2",
          "2": {
            "1": {
              "1": {
                "1": "type.googleapis.com/google.protobuf.Empty"
              }
            }
          },
          "4": {
            "1": "type.googleapis.com/com.example.shoppingcart.persistence.ItemAdded",
            "2": {
              "1": {
                "1": "A14362347",
                "2": "Deluxe",
                "3": "5"
              }
            }
          }
        }
      },
      "timestamp": "2020-05-01T15:30:57.510682+02:00"
},

This message should look like this if encoded properly:

    {
      "message_origin": "server",
      "raw_message": "CpIBCAISLwotCisKKXR5cGUuZ29vZ2xlYXBpcy5jb20vZ29vZ2xlLnByb3RvYnVmLkVtcHR5Il0KQnR5cGUuZ29vZ2xlYXBpcy5jb20vY29tLmV4YW1wbGUuc2hvcHBpbmdjYXJ0LnBlcnNpc3RlbmNlLkl0ZW1BZGRlZBIXChUKCUExNDM2MjM0NxIGRGVsdXhlGAU=",
      "message": {
        "reply": {
          "commandId": "2",
          "clientAction": {
            "reply": {
              "payload": {
                "@type": "type.googleapis.com/google.protobuf.Empty",
                "value": {}
              }
            }
          },
          "events": [
            {
              "@type": "type.googleapis.com/com.example.shoppingcart.persistence.ItemAdded",
              "item": {
                "productId": "A14362347",
                "name": "Deluxe",
                "quantity": 5
              }
            }
          ]
        }
      },
      "timestamp": "2020-05-01T15:44:10.461715+02:00"
    },

Analysis
a) The error got nil message is coming from proto_decoder.unknownFieldResolver#enrichMessageand seems to be triggered by a recursive call to itself whenever the condition dynamicNestedMessage == nil is met here. I read the comment here about message being nil and a potential race condition, but it never occurred to me during my debugging sessions for this issue.
Returning nil in enrichMessage in this case seems to solve this issue. I'm not sure if this is valid, but I did not miss anything during my sessions doing so.

b) As soon as this is fixed, I got an error like this

FATA[0013] Failed to marshal rpc   error="json: error calling MarshalJSON for type *dynamic.Message: unknown message type "com.example.shoppingcart.GetShoppingCart"

which is coming from dynamic.AnyResolver#Resolve of protoreflect and is triggered by dump#dumpInterceptor which tries to json.Marshal the RPC struct with its collected messages.
json.Marshal in its path down to marshal types, will call ultimatively dynamic.Messages#MarshalJSON and will fail as the jsonpb.Marshaler config lacks any AnyResolver that, when json.Marshal calls it, could resolve a messages type not well known or proto.MessageType would find and then ultimatively fails with

unknown message type

Fix
Getting messages resolved using proto.MessageTypeand get then registered by proto.RegisterType is probably a bad idea and should only used by generated Go types by protoc.
After some debugging and tests I got with a solution where dumpInterceptor wraps a messages event into a type that implements json.Marshal interface and configures an AnyResolver for the jsonpb.Marshaler knowing all FileDescriptors the proto_descriptor.LoadProtoDirectories function ever sees during the load of the protofiles provided by the -proto_roots argument.

This is implemented in this PR #97 and works for the use case mentioned at the beginning of this issue.

marcellanz added a commit to mrcllnz/grpc-tools that referenced this issue May 1, 2020
marcellanz added a commit to mrcllnz/grpc-tools that referenced this issue Jun 22, 2020
bradleyjkemp added a commit that referenced this issue Jun 30, 2020
* [Issue #96] this is a PoC to solve Issue #96.

* update grpc-dump/dump/dump_interceptor.go

Co-authored-by: Bradley Kemp <bradleyjkemp@users.noreply.github.com>

* [Issue #96] PR feedback: removed superfluous mutex.

* Update grpc-dump/dump/dump_interceptor.go

Co-authored-by: Bradley Kemp <bradleyjkemp@users.noreply.github.com>

Co-authored-by: Bradley Kemp <bradleyjkemp@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant