ManagedAPIServerLoadBalancer flag doesn't work as expected

[malstoun]

/kind bug

What steps did you take and what happened:
We don’t have loadbalancer in our Openstack installation. If I set ManagedAPIServerLoadBalancer: false to disable load balancer usage I see an error failed to create load balancer service client: No suitable endpoint could be found in the service catalog. in logs and reconciliation fails.

What did you expect to happen:
I expected to see successful reconciliation without load balancer errors. I think that if I set ManagedAPIServerLoadBalancer: false, controller shouldn’t try to create loadBalancerService.

Anything else you would like to add:
Discussion in slack: https://kubernetes.slack.com/archives/CFKJB65G9/p1628930844032400

Environment:

• Cluster API Provider OpenStack version: v0.4.0
• Cluster-API version: v0.4.0
• OpenStack version: N/A
• Minikube/KIND version: v0.11.1
• Kubernetes version (use kubectl version): v1.21.1
• OS (e.g. from /etc/os-release): 20.04.2 LTS (Focal Fossa)

[chrischdi]

I think there are three occurencies where the loadbalancer.NewService function is getting used without checking if ManagedAPIServerLoadBalancer was set:

• https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/master/controllers/openstackcluster_controller.go#L139
• https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/master/controllers/openstackcluster_controller.go#L375
• https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/master/controllers/openstackmachine_controller.go#L208

We have to adjust the code to only call loadbalancer.NewService if we really want to use the client (ManagedAPIServerLoadBalancer==true).

[chrischdi]

/help

[k8s-ci-robot]

@chrischdi:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[jichenjc]

in addition, we need disable
https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/master/hack/ci/devstack-default-cloud-init.yaml.tpl#L56

when doing CI without LB case, thus, we should be able to find this issue early

[raildo]

Just adding my 2 cents in this thread, this is a regression comparing with the CAPO v0.3.3, since we were able to have a Cluster API deployed without LB and then creating an external LB object. I believe that we should keep the same behavior as we had before.