From 5429b4b8b80a59795846e969cef4da4937ce5530 Mon Sep 17 00:00:00 2001 From: Emilien Macchi Date: Tue, 19 Nov 2024 10:11:40 -0500 Subject: [PATCH] Better conditions for creating Floating IPs When a floating is created, we need to make sure that `OpenStackCluster.Spec.DisableExternalNetwork` is not set to `True`. Otherwise, we'll have a nil pointer error. * Add a check in `reconcileBastion` to check that external network is not disabled before creating the floating IP for the bastion. * Add a check in `reconcileControlPlaneEndpoint` and `reconcileAPIServerLoadBalancer` to check that external network is not disabled (alongside the DisableAPIServerFloatingIP check) before creating the floating IP for the API server endpoint. * Add a safeguard in `GetOrCreateFloatingIP` to return a proper error (instead of a nil pointer error) when `openStackCluster.Status.ExternalNetwork` is nil. * Add API CEL to check that when DisableExternalNetwork is set and true, the bastion (if defined) doesn't have a floating IP defined and also that disableAPIServerFloatingIP (when set) is not False. --- api/v1beta1/openstackcluster_types.go | 2 ++ ...re.cluster.x-k8s.io_openstackclusters.yaml | 10 ++++++++ ...er.x-k8s.io_openstackclustertemplates.yaml | 10 ++++++++ controllers/openstackcluster_controller.go | 10 +++++--- controllers/openstackmachine_controller.go | 2 +- .../src/clusteropenstack/configuration.md | 4 ++++ pkg/cloud/services/networking/floatingip.go | 5 ++++ .../apivalidations/openstackcluster_test.go | 23 +++++++++++++++++++ 8 files changed, 62 insertions(+), 4 deletions(-) diff --git a/api/v1beta1/openstackcluster_types.go b/api/v1beta1/openstackcluster_types.go index 95f8ec8da8..5fe609e544 100644 --- a/api/v1beta1/openstackcluster_types.go +++ b/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 diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml index 6dad1ba26e..38b70fd2eb 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml @@ -4912,6 +4912,16 @@ spec: required: - identityRef type: object + x-kubernetes-validations: + - message: bastion floating IP cannot be set when disableExternalNetwork + is true + rule: 'has(self.disableExternalNetwork) && self.disableExternalNetwork + ? !has(self.bastion) || !has(self.bastion.floatingIP) : true' + - message: disableAPIServerFloatingIP cannot be false when disableExternalNetwork + is true + rule: 'has(self.disableExternalNetwork) && self.disableExternalNetwork + ? has(self.disableAPIServerFloatingIP) && self.disableAPIServerFloatingIP + : true' status: description: OpenStackClusterStatus defines the observed state of OpenStackCluster. properties: diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml index 928eafdaf6..8d885be29f 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml @@ -3429,6 +3429,16 @@ spec: required: - identityRef type: object + x-kubernetes-validations: + - message: bastion floating IP cannot be set when disableExternalNetwork + is true + rule: 'has(self.disableExternalNetwork) && self.disableExternalNetwork + ? !has(self.bastion) || !has(self.bastion.floatingIP) : true' + - message: disableAPIServerFloatingIP cannot be false when disableExternalNetwork + is true + rule: 'has(self.disableExternalNetwork) && self.disableExternalNetwork + ? has(self.disableAPIServerFloatingIP) && self.disableAPIServerFloatingIP + : true' required: - spec type: object diff --git a/controllers/openstackcluster_controller.go b/controllers/openstackcluster_controller.go index c396b4b4c8..4b7d270c0d 100644 --- a/controllers/openstackcluster_controller.go +++ b/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) diff --git a/controllers/openstackmachine_controller.go b/controllers/openstackmachine_controller.go index 3de18d0dc5..99aa2e808b 100644 --- a/controllers/openstackmachine_controller.go +++ b/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(): diff --git a/docs/book/src/clusteropenstack/configuration.md b/docs/book/src/clusteropenstack/configuration.md index 475e414b6d..74dac7d8a8 100644 --- a/docs/book/src/clusteropenstack/configuration.md +++ b/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: ``` +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 diff --git a/pkg/cloud/services/networking/floatingip.go b/pkg/cloud/services/networking/floatingip.go index 4d33a736fc..139674a2e1 100644 --- a/pkg/cloud/services/networking/floatingip.go +++ b/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 { diff --git a/test/e2e/suites/apivalidations/openstackcluster_test.go b/test/e2e/suites/apivalidations/openstackcluster_test.go index 926c923f65..8d0d4285bd 100644 --- a/test/e2e/suites/apivalidations/openstackcluster_test.go +++ b/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)