🐛 loadbalancer: resolve ControlPlaneEndpoint.Host when needed

[EmilienM]

What this PR does / why we need it:

ControlPlaneEndpoint.Host is not guaranteed to be an IP address, it
can also be an hostname.

Now we'll try to lookup the hostname if it's not an IP and set that for
the LB VipAddress.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1714

Special notes for your reviewer:

Unit tests are tricky. We stub the call to net.LookupHost to avoid actual DNS lookups.

• squashed commits
• if necessary:
  • includes documentation
  • adds unit tests

[netlify]

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name	Link
🔨 Latest commit	b319893
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/65ca4cf3308d84000868e84f
😎 Deploy Preview	https://deploy-preview-1738--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[EmilienM]

/assign mdbooth

[jichenjc]

/lgtm

[EmilienM]

Thanks for the review @jichenjc - I'll also tag @mdbooth since he reported the issue, I want to make sure he's fine too with the proposal.

[mdbooth]

This code block feels slightly too long to be duplicated below. Could we DRY this somehow?

[EmilienM]

I've put it into a constant. Let me know if that works.

[mdbooth]

Eww, but I don't personally know of a cleaner way to do this. I'll let it pass, but if anybody else knows better... @pierreprinetti maybe?

[pierreprinetti]

The alternative that pops to mind is dependency injection: If ReconcileLoadBalancer can accept a resolver as an additional argument, I'd inject the resolver from the controller. This lookupHost woud then be defined inside the controller, rather than being a global var (which can be dangerous). However, my solution doesn't work if you want to unit-test the controller this way.

If that's what you're looking for:

type Resolver func(string) ([]string, error)

func (s *Service) ReconcileLoadBalancer(openStackCluster *infrav1.OpenStackCluster, clusterName string, apiServerPort int, lookupHost Resolver) (bool, error) {

and in openstackcluster_controller.go:

		resolver := func(host string) ([]string, error) {
			ctx, cancel := context.WithTimeout(context.TODO(), 30*time.Second)
			defer cancel()
			return net.DefaultResolver.LookupHost(ctx, host)
		}
		terminalFailure, err := loadBalancerService.ReconcileLoadBalancer(openStackCluster, clusterName, apiServerPort, resolver)

Not really a game changer.

[EmilienM]

Thanks Pierre for your analysis. I think the PR is then ready.

[EmilienM]

@mdbooth are we good now or do you expect me to change something else?
Thanks

[mdbooth]

A few things here:

This should probably use Resolver.LookupHost() as documented in net.LookupHost(), but we can't because we don't currently pass the context into this ReconcileLoadBalancer. We should most likely also do that.

I seem to recall we have a forcing function coming soon which will require us to use Context everywhere. So we don't miss this one, which isn't obvious without reading the godocs, could we update it to use Resolver.LookupHost anyway and create a context explicitly to pass in?

Secondly, does this have the correct behaviour when given an IP address? Either way, I wonder if we should check if it's an IP address before calling the resolver and just return it.

[EmilienM]

1. Using net.DefaultResolver.LookupHost: I agree, consider it done in the next patchset.

2. Does this have the correct behaviour when given an IP address? yes:

// LookupHost looks up the given host using the local resolver.
// It returns a slice of that host's addresses.
func (r *Resolver) LookupHost(ctx context.Context, host string) (addrs []string, err error) {
	// Make sure that no matter what we do later, host=="" is rejected.
	if host == "" {
		return nil, &DNSError{Err: errNoSuchHost.Error(), Name: host, IsNotFound: true}
	}
	if _, err := netip.ParseAddr(host); err == nil {             <----------- HERE
		return []string{host}, nil
	}
	return r.lookupHost(ctx, host)
}

[EmilienM]

can someone review it again please?

[jichenjc]

/lgtm

[mdbooth]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: EmilienM, mdbooth

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [mdbooth]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mdbooth]

/lgtm