-
Notifications
You must be signed in to change notification settings - Fork 386
Support LoadBalancerAddress for mesh gateways #388
Conversation
Reviewers:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Luke, I haven't looked at the code yet, but decided to try it out first.
So far, it didn't work because the mesh gateway clusterrole needs to be updated to allow it to read services. The error I get is:
2020-03-19T22:14:46.604Z [ERROR] getting service iryna-consul-mesh-gateway: services "iryna-consul-mesh-gateway" is forbidden: User "system:serviceaccount:default:iryna-consul-mesh-gateway" cannot get resource "services" in API group "" in the namespace "default"
I also had a thought on the UX. I initially looked at the config and tried to figure out which values I need to set to enable the Load Balancer. I set meshGateway.service.enabled
to true
and meshGateway.service.type
to LoadBalancer
. But that didn't trigger the mesh gateway deployment to fetch the load balancer address. I then realized that I need to the meshGateway.wanAddress.source
to LoadBalancerAddress
. I'm wondering if my stupidity is an indicator that the UX could be a bit more obvious. It would be super nice if we could infer from the service settings what you most likely would want.
One idea on how to do that could be enabling service by default and adding an option to wanAddress.source
called something like service
. This option would imply that we'll use the service for wan address, and perhaps, even the port. It might also make sense to change the default service type from ClusterIP to something else. IIRC routable cluster IPs are pretty niche, and so I'm not sure if this is a sane default for the mesh gateway. Just an idea, but I'm curious what you think (and apologies for the long story).
Ack sorry I missed that in my commit, I had it locally. You need
I'll update the PR. |
Update: my tests were successful after adding the clusterrole rules. Barring what I said about UX improvements, I think this is good to go, but would like to hear what you think first. |
f46a444
to
0474f5d
Compare
4582459
to
8a3e4ef
Compare
Updated to have |
189e10b
to
1790a25
Compare
@lkysow I found a problem with my own suggestion to add
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the changes to support the different kinds of services! I've left a couple of thoughts so far.
Removed all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good (except for some failing unit tests)!
Add support for using the external address of Kubernetes load balancer for the mesh gateway wan address. This change uses the new consul-k8s service-address command to get the address of the load balancer. It also removes meshGateway.wanAddress.{useNodeIP, useNodeName, host} config values in favour of meshGateway.wanAddress.{source, static}. The new source value allows selecting NodeIP, NodeName, Service or Static. This is more extensible than the previous boolean values. We also change the default containerPort to 8443 from 443 because that port can't be bound to on GKE. These are backwards incompatible changes.
76e78be
to
a69bee4
Compare
Add support for using the external address of Kubernetes load balancer
for the mesh gateway wan address.
This change uses the new consul-k8s load-balancer-address command to get
the address of the load balancer.
It also removes meshGateway.wanAddress.{useNodeIP, useNodeName, host}
config values in favour of meshGateway.wanAddress.{source, static}. The
new source value allows selecting NodeIP, NodeName, LoadBalancerAddress
or Static. This is more extensible than the previous boolean values.