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

Expose cloudevent proto in non-internal package to support receiving protobuf events over gRPC #761

Closed
costap opened this issue Mar 11, 2022 · 8 comments · Fixed by #793
Closed

Comments

@costap
Copy link

costap commented Mar 11, 2022

The use case is writing a gRPC service that accept cloud events in protobuf format, using the sdk-go to generate and parse events.

E.g.
gRPC proto definition

syntax = "proto3";

// needs to exist in local
import "github.com/cloudevents/sdk-go/binding/format/protobuf/v2/internal/pb/cloudevent.proto";

option go_package = "cloudevents";

message SendResponse {
  enum Status {
    OK = 0;
    ERROR = 1;
  }
  Status status = 1;
  string message = 2;
}

service CloudEventsService {
  rpc Send(CloudEventBatch) returns (SendResponse);
}

message CloudEventBatch {
  repeated pb.CloudEvent events = 1;
}

e.g. proto data

syntax = "proto3";

option go_package = "proto";

message User {
  string id = 1;
  string first_name = 2;
  string last_name = 3;
}

server code:

// Send implements cloudevents.Send
func (s *server) Send(ctx context.Context, in *cloudevents.CloudEventBatch) (*cloudevents.SendResponse, error) {
	for _, ce := range in.GetEvents() {
		e, err := format.FromProto(ce)
		if err != nil {
			log.Println(err)
			continue
		}
		log.Printf("Received: %v from %v", e.ID(), e.Source())
	}
	return &cloudevents.SendResponse{
		Status:  cloudevents.SendResponse_OK,
		Message: "OK",
	}, nil
}

Client code:

	u := proto.User{
                Id: "12345-67890"
		FirstName: "aFirstName",
		LastName:  "aLastName",
	}

	e := event.New()
	e.SetID(uuid.New().String())
	e.SetSource("aSource")
	e.SetSubject(u.Id)
	e.SetType("com.example.ce.user.created")
	e.SetTime(time.Now())
	e.SetData(format.ContentTypeProtobuf, u)

	ce, err := format.ToProto(&e)
	if err != nil {
		log.Fatal("¯\\_(ツ)_/¯", err)
	}
	ceb := cloudevents.CloudEventBatch{Events: []*_.CloudEvent{ce}}

	conn, err := grpc.Dial("localhost:50051", grpc.WithTransportCredentials(insecure.NewCredentials()))
	if err != nil {
		log.Fatalf("did not connect: %v", err)
	}
	defer conn.Close()

	cl := cloudevents.NewCloudEventsServiceClient(conn)
	cl.Send(context.Background(), &ceb)

At the moment this is not possible as cloudevent proto is inside an internal package, would be possible to expose it or some other way to achieve the same?

@embano1
Copy link
Member

embano1 commented Jun 2, 2022

Sorry if I'm misunderstanding, but what goes against using the official CE proto file? https://github.com/cloudevents/spec/blob/main/cloudevents/formats/cloudevents.proto

Btw: related #744

@seitau
Copy link

seitau commented Jul 27, 2022

I am encountering the same issue.

Sorry if I'm misunderstanding, but what goes against using the official CE proto file?

We can use the official CE proto file to generate CloudEvent model in golang, but then we can not use format.FromProto method because it expects github.com/cloudevents/sdk-go/binding/format/protobuf/v2/internal/pb.CloudEvent which is private.

Edit:
I guess we are supposed to use Protobuf variable to interact with pb.CloudEvent.

Protobuf = protobufFmt{}

package main

import (
	format "github.com/cloudevents/sdk-go/binding/format/protobuf/v2"
	"github.com/cloudevents/sdk-go/v2/event"
	"google.golang.org/protobuf/proto"
)

func main() {
        // own CloudEvent struct generated from official CE proto file  
	ce := &own.CloudEvent{
		Id: "test",
	}
	b, _ := proto.Marshal(ce)
	var e event.Event
	format.Protobuf.Unmarshal(b, &e)
	fmt.Println(e.Context.GetID())
}

@n3wscott
Copy link
Member

Those that are using proto, would you expect proto to look like one of the transport choices rather than a bit extra on the http client?

Would anyone be interested in a first class proto transport like the kafka and pubsub transports exist?

@muncus
Copy link
Contributor

muncus commented Aug 15, 2022

For my use case, a first-class proto transport would not help.

What I would like is more control over the way the CloudEvent protos are encoded (e.g. to work around #759 by moving content to text_data), or to embed a CloudEvent object in a different proto message as proto (instead of serialized json).
I view this as trying to use proto CloudEvents at the "Resource Level" as described in Investement Level.

Can you provide more background on why the protos were put in an internal package? With the proto Messages being part of the spec, I would not expect breaking changes. Would you prefer them to be versioned (by the spec version) if we expose them?

@n3wscott
Copy link
Member

n3wscott commented Aug 16, 2022

@muncus As to why internal, I don't see a reason provided. Personally I don't use internal unless there is a very very good reason and I don't see one here. #662

I would be supportive of moving those packages to a non-internal package.

@muncus
Copy link
Contributor

muncus commented Aug 16, 2022

Great, I will work up a PR to expose them.

Would you like to have the protos versioned by spec version? I'm not sure it is necessary, but i will defer to a maintainer's judgement 😄

@n3wscott
Copy link
Member

Would you like to have the protos versioned by spec version? I'm not sure it is necessary, but i will defer to a maintainer's judgement 😄

If you are asking if we should make releases based on the cloudevents spec version, I would say no, it would make the release process very tricky and it is already tricky with so many modules. But I could be misunderstanding the question. Is there a verison that is embedded in the proto file?

Maybe first pass is just pop that internal package into an exported package so you can get at the generated clients and vendor the proto in other projects?

@muncus
Copy link
Contributor

muncus commented Aug 19, 2022

Scott, PR #793 should address the concerns on this issue, please take a look when you have a chance. Thanks!

For clarity, you did understand my question correctly regarding cloudevents spec version, and I agree with you. 👍🏼

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

Successfully merging a pull request may close this issue.

5 participants