From 77383e65fb9824e7cab55ce10eda2c11f70d7006 Mon Sep 17 00:00:00 2001 From: Emilien Macchi Date: Tue, 19 Nov 2024 10:11:40 -0500 Subject: [PATCH 1/2] 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. (cherry picked from commit 5429b4b8b80a59795846e969cef4da4937ce5530) --- 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 d6745e5ccd..5da4d77111 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 fd240c51f5..80778a76da 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml @@ -6570,6 +6570,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 96680265b6..3155b8c054 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml @@ -4027,6 +4027,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 7429958805..96846f58ca 100644 --- a/controllers/openstackcluster_controller.go +++ b/controllers/openstackcluster_controller.go @@ -507,7 +507,11 @@ func reconcileBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openS 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) { @@ -841,9 +845,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)) diff --git a/controllers/openstackmachine_controller.go b/controllers/openstackmachine_controller.go index 230ecd5342..8638b72f5a 100644 --- a/controllers/openstackmachine_controller.go +++ b/controllers/openstackmachine_controller.go @@ -708,7 +708,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 193720927e..3a0439c584 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 @@ -715,6 +717,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 4e611f7fe0..dc474b846d 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 76bcbb40bf..943af0605c 100644 --- a/test/e2e/suites/apivalidations/openstackcluster_test.go +++ b/test/e2e/suites/apivalidations/openstackcluster_test.go @@ -169,6 +169,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: "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") + }) }) Context("v1alpha7", func() { From eacd146d16b280709ada22e1f9093f81f833f6e4 Mon Sep 17 00:00:00 2001 From: Emilien Macchi Date: Tue, 26 Nov 2024 08:12:21 -0500 Subject: [PATCH 2/2] CARRY: `make merge-bot` --- ...penstack_04_infrastructure-components.yaml | 20 +++++++++++++++++++ .../api/v1beta1/openstackcluster_types.go | 2 ++ 2 files changed, 22 insertions(+) diff --git a/openshift/manifests/0000_30_cluster-api-provider-openstack_04_infrastructure-components.yaml b/openshift/manifests/0000_30_cluster-api-provider-openstack_04_infrastructure-components.yaml index 8bf587f807..1a7f888d98 100644 --- a/openshift/manifests/0000_30_cluster-api-provider-openstack_04_infrastructure-components.yaml +++ b/openshift/manifests/0000_30_cluster-api-provider-openstack_04_infrastructure-components.yaml @@ -6590,6 +6590,16 @@ data: 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: @@ -11124,6 +11134,16 @@ data: 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/openshift/vendor/sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1/openstackcluster_types.go b/openshift/vendor/sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1/openstackcluster_types.go index d6745e5ccd..5da4d77111 100644 --- a/openshift/vendor/sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1/openstackcluster_types.go +++ b/openshift/vendor/sigs.k8s.io/cluster-api-provider-openstack/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