Change the cloud provider TCPLoadBalancerExists function to GetTCPLoadBalancer... #7669
Conversation
…dBalancer, which returns the IP/DNS address of the load balancer, enabling more intelligent reconciliation in the service controller.
| // determine if it matches the needs of a service rather than if it exists. | ||
| TCPLoadBalancerExists(name, region string) (bool, error) | ||
| // GetTCPLoadBalancer returns whether the specified load balancer exists, and | ||
| // if so, what its IP address or hostname is. |
There was a problem hiding this comment.
I don't like the "or" part of this. Can we make this a struct that has both hostname and ip and fill in one (or both) as appropriate? Also make the IP address a net.IPAddress.
We've already had pain from hostname vs. ip confusion in other part so of the stack that we had to iron out.
There was a problem hiding this comment.
We could make the change locally to this interface, but that leads to a cascade of other things that'll also have to be changed - CreateTCPLoadBalancer, the service controller, the API itself (since publicIPs has been hacked to include the DNS address for AWS ELBs), and kube-proxy, at the very least.
If you think it's worth our time at this point, go ahead and file an issue or reopen #5224 - although you were the one that merged the PR introducing the overlap (#5228).
|
I share @brandandburns distaste for jamming a DNS name or IP address into a string. I'm sure @thockin would too. But I also agree with @a-robinson that this needs to be fixed holistically across the code base, as a separate PR (or PR's). The rest of this PR LGTM. Merging. |
Change the cloud provider TCPLoadBalancerExists function to GetTCPLoadBalancer...
ghost
merged commit 
...which returns the IP/DNS address of the load balancer, enabling more intelligent reconciliation in the service controller.
@justinsb since this slightly modifies the interface that #5225 is implementing