🐛 Do not store "<nil>" as load balancer IP - #2256
Conversation
A load balancer without a public interface has no public IPv4 and no public IPv6. net.IP.String() returns "<nil>" for such an address, and that string ended up in status.controlPlaneLoadBalancer.ipv4 and .ipv6. Store an empty string instead. processControlPlaneEndpoint still treats "<nil>" as "no public IPv4", so clusters whose status was written by an older release keep working until the next reconcile overwrites the field. # Committing as: thomas.guettler@syself.com
github-actions
Bot
added
size/M
processControlPlaneEndpoint set the condition message "load balancer not ready yet" even when the load balancer existed and just had no public IPv4. Waiting does not help in that case, so say what is wrong instead. Route the private IP of the load balancer through ipToStatusString, too. An unset private IP would end up as "<nil>" in status.controlPlaneLoadBalancer.internalIP otherwise. Fold the three test cases for "load balancer has no usable IPv4" into a table, and check the condition message in all of them.
net.IP.String() returns "<nil>" for a nil IP and for a non-nil IP which has no bytes. The check for nil missed the second case. len() covers both. Add a unit test for ipToStatusString.
net.IP("1.2.3.4") is a seven byte slice, not a parsed address. String()
returns "?312e322e332e34" for it, so every test which created a load
balancer via the fake client stored that in the load balancer status.
Use net.ParseIP().
github-actions
Bot
added
size/L
| const ( | ||
| msgLoadBalancerNotReadyYet = "enabled LoadBalancer but load balancer not ready yet" | ||
|
|
||
| msgLoadBalancerWithoutPublicIPv4 = "enabled LoadBalancer but the load balancer has no public IPv4, " + |
There was a problem hiding this comment.
I didn't really understand what we try to do here. Currently, from my understanding, in the rare case that the load balancer has no IPv4, we would print "" in the status. This PR changes this so it would be empty.
However, we still don't have a proper handling of this case, do we? All the other parts of the code would still fail if they rely on this.
Can you explain why this PR makes a lot of sense?
There was a problem hiding this comment.
The "issue" was found while working on #2252.
Here (for LoadBalancers) it is unlikely that there is no IP.
Overall, I am happy with closing and not merging the PR. It is a very unlikely edge-case.
There was a problem hiding this comment.
Okay. So if you think that it is really unlikely (aka it would never occur in reasonable situations and edge cases), then we should better close it to not "distract" people in the code.
2 participants
Summary
net.IP.String()returns"<nil>"for an unset IP, and that string ended up instatus.controlPlaneLoadBalancer.ipv4and.ipv6. Now an empty string gets stored instead.This happens for a load balancer without a public interface (
public_interface: false). caph always creates its load balancers with a public interface, so a cluster can only run into this by adopting an existing load balancer viaspec.controlPlaneLoadBalancer.name.The private IP (
.internalIP) goes through the same helper now. Hetzner always reports a private IP for a load balancer which is attached to a network, so that part is only a safeguard.processControlPlaneEndpoint()compared the IPv4 against the literal"<nil>"to decide whether the load balancer can be used as control plane endpoint. It now checks for an empty string. The comparison with"<nil>"is kept as a defensive check only: the load balancer status is rewritten from the Hetzner API earlier in the same reconcile, so a value written by an older release never reaches this code.Such a load balancer can never provide a control plane endpoint, but the condition
ControlPlaneEndpointSetsaid "load balancer not ready yet", which sounds like waiting would help. The message now says that the load balancer has no public IPv4.Why this is worth changing
This does not repair a broken cluster. caph creates its load balancers with a public interface and never changes that afterwards, so its own load balancers do have a public IPv4. A load balancer adopted by name could have the public interface disabled, but such a cluster can never get a control plane endpoint anyway.
It is still worth cleaning up:
"<nil>"is how Go formats an unset IP, not an address. It leaks into a status field which users and scripts read, and comparing against it makes the control plane endpoint depend on an implementation detail of the standard library. Code like this also gets copied.While being here: the fake hcloud client built its IPs as
net.IP("1.2.3.4"), which is a seven byte slice and not a parsed address.String()returned"?312e322e332e34"for it, so the tests using the fake client stored that in the load balancer status without anybody noticing. It usesnet.ParseIP()now.Same class of bug as #2252, which addresses it for servers.