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

proposal: move API to gRPC with gRPC-gateway proxy #245

Closed
ericchiang opened this issue Dec 23, 2015 · 8 comments
Closed

proposal: move API to gRPC with gRPC-gateway proxy #245

ericchiang opened this issue Dec 23, 2015 · 8 comments
Assignees

Comments

@ericchiang
Copy link
Contributor

low priority

  • Define admin and worker APIs as gRPC services.
  • Have dex listen for gRPC requests on high port.
  • Register gRPC-gateway proxy on HTTP traffic to maintain compatibility with front end JavaScript.

Proof of concept here.

@bobbyrullo
Copy link
Contributor

Do you really think we'll be able to maintain wire compatibility via the gateway? That would be awesome...but I'm skeptical.

Also, do we have to listen on separate ports?

@ericchiang
Copy link
Contributor Author

Do you really think we'll be able to maintain wire compatibility via the gateway? That would be awesome...but I'm skeptical.

With our limited API, yes.

Also, do we have to listen on separate ports?

I'm sure there are clever ways we could have them listen on the same port.

@bobbyrullo
Copy link
Contributor

Awesome.

@ericchiang
Copy link
Contributor Author

Note that grpc-gateway currently uses encoding/json which is problematic because generated gRPC structs have omitempty on every field. For example here's a struct from rkt's api.

// Network describes the network information of a pod.
type Network struct {
    // Name of the network that a pod belongs to, required.
    Name string `protobuf:"bytes,1,opt,name=name" json:"name,omitempty"`
    // Pod's IPv4 address within the network, optional if IPv6 address is given.
    Ipv4 string `protobuf:"bytes,2,opt,name=ipv4" json:"ipv4,omitempty"`
    // Pod's IPv6 address within the network, optional if IPv4 address is given.
    Ipv6 string `protobuf:"bytes,3,opt,name=ipv6" json:"ipv6,omitempty"`
}

So for instance when the following message has AdminUserCreated as false

message State {
    bool AdminUserCreated = 1;
}

grpc-gateway will respond with an empty object ({}) rather than {"AdminUserCreated":false}. Similar cases might brake JavaScript code that expects keys.

This is fixed if grpc-gateway uses a JSON encoder which complies with the protobuf -> JSON spec. I've opened an issue for this at grpc-ecosystem/grpc-gateway#79.

@bobbyrullo
Copy link
Contributor

another thing to consider is authentication

@sym3tri
Copy link

sym3tri commented Dec 30, 2015

omitempty on everything was a huge annoyance for our team too. If you go this route you may want to consider using https://github.com/gogo/protobuf. It offers much more flexibility WRT defining tags on the generated structs.

@sadlil
Copy link

sadlil commented Jan 8, 2016

@bobbyrullo grpc supports authentication.
Here it shows the support of TLS and JWT authentication. Using grpc.WithPerRPCCredentials() we can also implement the Basic or Bearer auth support with the metadata of context.

@ericchiang
Copy link
Contributor Author

Closing this. It's going to be a lot of work to get this up and running smoothly, and I'm not convinced of the benefit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants