Merge pull request #2261 from kubernetes-sigs/issue_2260

🐛 Better checks before creating Floating IPs

api/v1beta1/openstackcluster_types.go

@@ -31,6 +31,8 @@ const (
 )
 
 // OpenStackClusterSpec defines the desired state of OpenStackCluster.
+// +kubebuilder:validation:XValidation:rule="has(self.disableExternalNetwork) && self.disableExternalNetwork ? !has(self.bastion) || !has(self.bastion.floatingIP) : true",message="bastion floating IP cannot be set when disableExternalNetwork is true"
+// +kubebuilder:validation:XValidation:rule="has(self.disableExternalNetwork) && self.disableExternalNetwork ? has(self.disableAPIServerFloatingIP) && self.disableAPIServerFloatingIP : true",message="disableAPIServerFloatingIP cannot be false when disableExternalNetwork is true"
 type OpenStackClusterSpec struct {
 	// ManagedSubnets describe OpenStack Subnets to be created. Cluster actuator will create a network,
 	// subnets with the defined CIDR, and a router connected to these subnets. Currently only one IPv4

controllers/openstackcluster_controller.go

@@ -395,7 +395,11 @@ func (r *OpenStackClusterReconciler) reconcileBastion(ctx context.Context, scope
 		return nil, err
 	}
 
-	return bastionAddFloatingIP(openStackCluster, clusterResourceName, port, networkingService)
+	if !ptr.Deref(openStackCluster.Spec.DisableExternalNetwork, false) {
+		return bastionAddFloatingIP(openStackCluster, clusterResourceName, port, networkingService)
+	}
+
+	return nil, nil
 }
 
 func bastionAddFloatingIP(openStackCluster *infrav1.OpenStackCluster, clusterResourceName string, port *ports.Port, networkingService *networking.Service) (*reconcile.Result, error) {
@@ -798,9 +802,9 @@ func reconcileControlPlaneEndpoint(scope *scope.WithLogger, networkingService *n
 	case openStackCluster.Spec.ControlPlaneEndpoint != nil && openStackCluster.Spec.ControlPlaneEndpoint.IsValid():
 		host = openStackCluster.Spec.ControlPlaneEndpoint.Host
 
-	// API server load balancer is disabled, but floating IP is not. Create
+	// API server load balancer is disabled, but external netowork and floating IP are not. Create
 	// a floating IP to be attached directly to a control plane host.
-	case !ptr.Deref(openStackCluster.Spec.DisableAPIServerFloatingIP, false):
+	case !ptr.Deref(openStackCluster.Spec.DisableAPIServerFloatingIP, false) && !ptr.Deref(openStackCluster.Spec.DisableExternalNetwork, false):
 		fp, err := networkingService.GetOrCreateFloatingIP(openStackCluster, openStackCluster, clusterResourceName, openStackCluster.Spec.APIServerFloatingIP)
 		if err != nil {
 			handleUpdateOSCError(openStackCluster, fmt.Errorf("floating IP cannot be got or created: %w", err), false)

controllers/openstackmachine_controller.go

@@ -628,7 +628,7 @@ func (r *OpenStackMachineReconciler) reconcileAPIServerLoadBalancer(scope *scope
 			conditions.MarkFalse(openStackMachine, infrav1.APIServerIngressReadyCondition, infrav1.LoadBalancerMemberErrorReason, clusterv1.ConditionSeverityError, "Reconciling load balancer member failed: %v", err)
 			return fmt.Errorf("reconcile load balancer member: %w", err)
 		}
-	} else if !ptr.Deref(openStackCluster.Spec.DisableAPIServerFloatingIP, false) {
+	} else if !ptr.Deref(openStackCluster.Spec.DisableAPIServerFloatingIP, false) && !ptr.Deref(openStackCluster.Spec.DisableExternalNetwork, false) {
 		var floatingIPAddress *string
 		switch {
 		case openStackCluster.Spec.ControlPlaneEndpoint != nil && openStackCluster.Spec.ControlPlaneEndpoint.IsValid():

docs/book/src/clusteropenstack/configuration.md

@@ -270,6 +270,8 @@ associated to the first controller node and the other controller nodes have no f
 to any other controller node. So we recommend to only set one controller node when floating IP is needed,
 or please consider using load balancer instead, see [issue #1265](https://github.com/kubernetes-sigs/cluster-api-provider-openstack/issues/1265) for further information.
 
+Note: `spec.disableExternalNetwork` must be unset or set to `false` to allow the API server to have a floating IP.
+
 ### Disabling the API server floating IP
 
 It is possible to provision a cluster without a floating IP for the API server by setting
@@ -717,6 +719,8 @@ spec:
     floatingIP: <Floating IP address>
 ```
 
+Note: A floating IP can only be added if `OpenStackCluster.Spec.DisableExternalNetwork` is not set or set to `false`.
+
 If `managedSecurityGroups` is set to a non-nil value (e.g. `{}`), security group rule opening 22/tcp is added to security groups for bastion, controller, and worker nodes respectively. Otherwise, you have to add `securityGroups` to the `bastion` in `OpenStackCluster` spec and `OpenStackMachineTemplate` spec template respectively.
 
 ### Making changes to the bastion host

pkg/cloud/services/networking/floatingip.go

@@ -38,6 +38,11 @@ func (s *Service) GetOrCreateFloatingIP(eventObject runtime.Object, openStackClu
 	var err error
 	var fpCreateOpts floatingips.CreateOpts
 
+	// This is a safeguard, we shouldn't reach it and if we do, it's something to fix in the caller of the function.
+	if openStackCluster.Status.ExternalNetwork == nil {
+		return nil, fmt.Errorf("external network not found")
+	}
+
 	if ptr.Deref(ip, "") != "" {
 		fp, err = s.GetFloatingIP(*ip)
 		if err != nil {

test/e2e/suites/apivalidations/openstackcluster_test.go

@@ -178,6 +178,29 @@ var _ = Describe("OpenStackCluster API validations", func() {
 			Expect(createObj(cluster)).NotTo(Succeed(), "OpenStackCluster creation should not succeed")
 		})
 
+		It("should not allow a bastion floating IP with DisableExternalNetwork set to true", func() {
+			cluster.Spec.Bastion = &infrav1.Bastion{
+				Enabled: ptr.To(true),
+				Spec: &infrav1.OpenStackMachineSpec{
+					Flavor: ptr.To("flavor-name"),
+					Image: infrav1.ImageParam{
+						Filter: &infrav1.ImageFilter{
+							Name: ptr.To("fake-image"),
+						},
+					},
+				},
+				FloatingIP: ptr.To("10.0.0.0"),
+			}
+			cluster.Spec.DisableExternalNetwork = ptr.To(true)
+			Expect(createObj(cluster)).ToNot(Succeed(), "OpenStackCluster creation should not succeed")
+		})
+
+		It("should not allow DisableAPIServerFloatingIP to be false with DisableExternalNetwork set to true", func() {
+			cluster.Spec.DisableAPIServerFloatingIP = ptr.To(false)
+			cluster.Spec.DisableExternalNetwork = ptr.To(true)
+			Expect(createObj(cluster)).ToNot(Succeed(), "OpenStackCluster creation should not succeed")
+		})
+
 		// We must use unstructured to set values which can't be marshalled by the Go type
 		unstructuredClusterWithAPIPort := func(apiServerPort any) *unstructured.Unstructured {
 			obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(cluster)