pieterlouw / caddy-grpc

grpc plugin for Caddy Server
Apache License 2.0
48 stars 15 forks source link

Allow specifying GRPC load balancer #5

Open d4l3k opened 6 years ago

d4l3k commented 6 years ago

This is the easiest way I can see of adding in load balancing. I don't think there should be an unexpected side effects but please let me know if you can think of anything.

This adds in a round robin DNSResolver for load balancing purposes. The GRPC DNSResolver first checks if SRV records are present on the specified host and if so, uses them. If they don't exist it just resolves the DNS hostname as normal. If there's an IP specified it just uses that.

d4l3k commented 6 years ago

@pieterlouw Any thoughts on this?

pieterlouw commented 6 years ago

Ho @d4l3k , Thanks for this, it looks great. I need to find a moment to test this myself before I merge it in.

Few things:

How does this affect the Caddyfile setup? Can you perhaps post an example?

The only thing that bothers me is the gRPC Naming package is still experimental according to the documentation. I'm sure it's stable, it's just I'm reluctant to add an API that might change and break future Caddy builds.

d4l3k commented 6 years ago

Hey,

This shouldn't change the Caddyfile setup at all. It just checks for a GRPC SRV record first. and falls back to just normal DNS resolution.

And yeah, I totally missed the fact that that package was experimental and deprecated. I'll investigate a bit more and update the PR.

d4l3k commented 6 years ago

Okay, I just took a deeper look into this. Looks like load balancing is actually supported out of the box, but the default resolver is "passthrough" and load balancer is "pick_first".

For SRV resolving you can just do dns://service.examples.com.

d4l3k commented 6 years ago

I updated this PR to add a balancer config option which lets you specify which load balancer mode you want to do. Resolvers can just be specified by the scheme :// in the backend_addr field.

As for the balancing stuff being experimental, this code no longer depends on any package other than the main grpc one which should prevent most issues and it seems unlikely that they'd remove the grpc.WithBalancerName method without notice considering the old deprecated load balancing mechanism still works.

This in use would look like

grpc dns://service.foo {
  balancer round_robin
}
pieterlouw commented 6 years ago

Thanks @d4l3k

I added a comment on the changes you made in setup.go (https://github.com/pieterlouw/caddy-grpc/pull/5/files#diff-4d86aab957c347ca87f7021d57b17205)