⚠️ Bastion is enabled by default if specified

api/v1alpha6/openstackcluster_conversion.go

@@ -416,6 +416,7 @@ func restorev1beta1Bastion(previous **infrav1.Bastion, dst **infrav1.Bastion) {
 
 	optional.RestoreString(&(*previous).FloatingIP, &(*dst).FloatingIP)
 	optional.RestoreString(&(*previous).AvailabilityZone, &(*dst).AvailabilityZone)
+	optional.RestoreBool(&(*previous).Enabled, &(*dst).Enabled)
 }
 
 func Convert_v1alpha6_Bastion_To_v1beta1_Bastion(in *Bastion, out *infrav1.Bastion, s apiconversion.Scope) error {

api/v1alpha7/openstackcluster_conversion.go

@@ -384,6 +384,7 @@ func restorev1beta1Bastion(previous **infrav1.Bastion, dst **infrav1.Bastion) {
 
 		optional.RestoreString(&(*previous).FloatingIP, &(*dst).FloatingIP)
 		optional.RestoreString(&(*previous).AvailabilityZone, &(*dst).AvailabilityZone)
+		optional.RestoreBool(&(*previous).Enabled, &(*dst).Enabled)
 	}
 }
 

api/v1beta1/types.go

@@ -719,12 +719,18 @@ var (
 )
 
 // Bastion represents basic information about the bastion node. If you enable bastion, the spec has to be specified.
-// +kubebuilder:validation:XValidation:rule="!self.enabled || has(self.spec)",message="you need to specify the spec if bastion is enabled"
+// +kubebuilder:validation:XValidation:rule="!self.enabled || has(self.spec)",message="spec is required if bastion is enabled"
 type Bastion struct {
-	// Enabled means that bastion is enabled. Defaults to false.
-	// +kubebuilder:validation:Required
-	// +kubebuilder:default:=false
-	Enabled bool `json:"enabled"`
+	// Enabled means that bastion is enabled. The bastion is enabled by
+	// default if this field is not specified. Set this field to false to disable the
+	// bastion.
+	//
+	// It is not currently possible to remove the bastion from the cluster
+	// spec without first disabling it by setting this field to false and
+	// waiting until the bastion has been deleted.
+	// +kubebuilder:default:=true
+	// +optional
+	Enabled optional.Bool `json:"enabled,omitempty"`
 
 	// Spec for the bastion itself
 	Spec *OpenStackMachineSpec `json:"spec,omitempty"`
@@ -741,6 +747,13 @@ type Bastion struct {
 	FloatingIP optional.String `json:"floatingIP,omitempty"`
 }
 
+func (b *Bastion) IsEnabled() bool {
+	if b == nil {
+		return false
+	}
+	return b.Enabled == nil || *b.Enabled
+}
+
 type APIServerLoadBalancer struct {
 	// Enabled defines whether a load balancer should be created. This value
 	// defaults to true if an APIServerLoadBalancer is given.

controllers/openstackcluster_controller.go

@@ -219,7 +219,7 @@ func contains(arr []string, target string) bool {
 
 func resolveBastionResources(scope *scope.WithLogger, clusterResourceName string, openStackCluster *infrav1.OpenStackCluster) (bool, error) {
 	// Resolve and store resources for the bastion
-	if openStackCluster.Spec.Bastion != nil && openStackCluster.Spec.Bastion.Enabled {
+	if openStackCluster.Spec.Bastion.IsEnabled() {
 		if openStackCluster.Status.Bastion == nil {
 			openStackCluster.Status.Bastion = &infrav1.BastionStatus{}
 		}
@@ -406,7 +406,7 @@ func reconcileBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openS
 	}
 
 	// No Bastion defined
-	if openStackCluster.Spec.Bastion == nil || !openStackCluster.Spec.Bastion.Enabled {
+	if !openStackCluster.Spec.Bastion.IsEnabled() {
 		// Delete any existing bastion
 		if openStackCluster.Status.Bastion != nil {
 			if err := deleteBastion(scope, cluster, openStackCluster); err != nil {
@@ -440,6 +440,8 @@ func reconcileBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openS
 		return nil, fmt.Errorf("failed computing bastion hash from instance spec: %w", err)
 	}
 	if bastionHashHasChanged(bastionHash, openStackCluster.ObjectMeta.Annotations) {
+		scope.Logger().Info("Bastion instance spec has changed, deleting existing bastion")
+
 		if err := deleteBastion(scope, cluster, openStackCluster); err != nil {
 			return nil, err
 		}
@@ -544,7 +546,7 @@ func bastionAddFloatingIP(openStackCluster *infrav1.OpenStackCluster, clusterRes
 func bastionToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, cluster *clusterv1.Cluster) (*compute.InstanceSpec, error) {
 	bastion := openStackCluster.Spec.Bastion
 	if bastion == nil {
-		return nil, fmt.Errorf("bastion spec is nil")
+		bastion = &infrav1.Bastion{}
 	}
 	if bastion.Spec == nil {
 		// For the case when Bastion is deleted but we don't have spec, let's use an empty one.

controllers/openstackcluster_controller_test.go

@@ -196,10 +196,10 @@ var _ = Describe("OpenStackCluster controller", func() {
 		Expect(err).To(MatchError(clientCreateErr))
 		Expect(result).To(Equal(reconcile.Result{}))
 	})
-	It("should be able to reconcile when bastion is disabled and does not exist", func() {
-		testCluster.SetName("no-bastion")
+	It("should be able to reconcile when bastion is explicitly disabled and does not exist", func() {
+		testCluster.SetName("no-bastion-explicit")
 		testCluster.Spec = infrav1.OpenStackClusterSpec{
-			Bastion: &infrav1.Bastion{},
+			Bastion: &infrav1.Bastion{Enabled: pointer.Bool(false)},
 		}
 		err := k8sClient.Create(ctx, testCluster)
 		Expect(err).To(BeNil())
@@ -228,7 +228,7 @@ var _ = Describe("OpenStackCluster controller", func() {
 		testCluster.SetName("adopt-existing-bastion")
 		testCluster.Spec = infrav1.OpenStackClusterSpec{
 			Bastion: &infrav1.Bastion{
-				Enabled: true,
+				Enabled: pointer.Bool(true),
 				Spec:    &bastionSpec,
 			},
 		}
@@ -311,7 +311,7 @@ var _ = Describe("OpenStackCluster controller", func() {
 		testCluster.SetName("requeue-bastion")
 		testCluster.Spec = infrav1.OpenStackClusterSpec{
 			Bastion: &infrav1.Bastion{
-				Enabled: true,
+				Enabled: pointer.Bool(true),
 				Spec:    &bastionSpec,
 			},
 		}
@@ -393,7 +393,7 @@ var _ = Describe("OpenStackCluster controller", func() {
 		testCluster.SetName("requeue-bastion")
 		testCluster.Spec = infrav1.OpenStackClusterSpec{
 			Bastion: &infrav1.Bastion{
-				Enabled: true,
+				Enabled: pointer.Bool(true),
 				Spec:    &bastionSpec,
 			},
 		}
@@ -467,9 +467,7 @@ var _ = Describe("OpenStackCluster controller", func() {
 	})
 	It("should delete an existing bastion even if its uuid is not stored in status", func() {
 		testCluster.SetName("delete-existing-bastion")
-		testCluster.Spec = infrav1.OpenStackClusterSpec{
-			Bastion: &infrav1.Bastion{},
-		}
+		testCluster.Spec = infrav1.OpenStackClusterSpec{}
 		err := k8sClient.Create(ctx, testCluster)
 		Expect(err).To(BeNil())
 		err = k8sClient.Create(ctx, capiCluster)
@@ -516,7 +514,7 @@ var _ = Describe("OpenStackCluster controller", func() {
 		testCluster.SetName("subnet-filtering")
 		testCluster.Spec = infrav1.OpenStackClusterSpec{
 			Bastion: &infrav1.Bastion{
-				Enabled: true,
+				Enabled: pointer.Bool(true),
 				Spec:    &bastionSpec,
 			},
 			DisableAPIServerFloatingIP: pointer.Bool(true),
@@ -586,7 +584,7 @@ var _ = Describe("OpenStackCluster controller", func() {
 		testCluster.SetName("subnet-filtering")
 		testCluster.Spec = infrav1.OpenStackClusterSpec{
 			Bastion: &infrav1.Bastion{
-				Enabled: true,
+				Enabled: pointer.Bool(true),
 				Spec:    &bastionSpec,
 			},
 			DisableAPIServerFloatingIP: pointer.Bool(true),

docs/book/src/topics/crd-changes/v1alpha7-to-v1beta1.md

@@ -14,7 +14,7 @@
       - [Change to image](#change-to-image)
       - [Removal of imageUUID](#removal-of-imageuuid)
       - [Changes to ports](#changes-to-ports)
-      - [Additon of floatingIPPoolRef](#additon-of-floatingippoolref)
+      - [Addition of floatingIPPoolRef](#addition-of-floatingippoolref)
     - [`OpenStackCluster`](#openstackcluster)
       - [Removal of cloudName](#removal-of-cloudname-1)
       - [identityRef is now required](#identityref-is-now-required)
@@ -367,6 +367,8 @@ spec:
 
 #### Changes to bastion
 
+In v1beta1 the Bastion is enabled by default if a bastion is specified in `OpenStackCluster.spec.bastion` but `enabled` is not included. However, it is still required to set `enabled` explicitly to `false` before removing the bastion. We intend to remove this requirement in the future at which point the `enabled` field will be redundant.
+
 In v1beta1, `OpenStackCluster.spec.bastion.instance` becomes `OpenStackCluster.spec.bastion.spec`.
 
 ```yaml

pkg/cloud/services/networking/securitygroups.go

@@ -47,7 +47,7 @@ func (s *Service) ReconcileSecurityGroups(openStackCluster *infrav1.OpenStackClu
 		return nil
 	}
 
-	bastionEnabled := openStackCluster.Spec.Bastion != nil && openStackCluster.Spec.Bastion.Enabled
+	bastionEnabled := openStackCluster.Spec.Bastion.IsEnabled()
 
 	secControlPlaneGroupName := getSecControlPlaneGroupName(clusterResourceName)
 	secWorkerGroupName := getSecWorkerGroupName(clusterResourceName)
@@ -212,7 +212,7 @@ func (s *Service) generateDesiredSecGroups(openStackCluster *infrav1.OpenStackCl
 
 	desiredSecGroupsBySuffix := make(map[string]securityGroupSpec)
 
-	if openStackCluster.Spec.Bastion != nil && openStackCluster.Spec.Bastion.Enabled {
+	if openStackCluster.Spec.Bastion.IsEnabled() {
 		controlPlaneRules = append(controlPlaneRules, getSGControlPlaneSSH(secBastionGroupID)...)
 		workerRules = append(workerRules, getSGWorkerSSH(secBastionGroupID)...)
 
@@ -357,7 +357,7 @@ func (s *Service) DeleteSecurityGroups(openStackCluster *infrav1.OpenStackCluste
 		getSecWorkerGroupName(clusterResourceName),
 	}
 
-	if openStackCluster.Spec.Bastion != nil && openStackCluster.Spec.Bastion.Enabled {
+	if openStackCluster.Spec.Bastion.IsEnabled() {
 		secGroupNames = append(secGroupNames, getSecBastionGroupName(clusterResourceName))
 	}
 

pkg/cloud/services/networking/securitygroups_test.go

@@ -533,7 +533,7 @@ func TestService_ReconcileSecurityGroups(t *testing.T) {
 			name: "Default control plane, worker, and bastion security groups",
 			openStackClusterSpec: infrav1.OpenStackClusterSpec{
 				Bastion: &infrav1.Bastion{
-					Enabled: true,
+					Enabled: pointer.Bool(true),
 				},
 				ManagedSecurityGroups: &infrav1.ManagedSecurityGroups{},
 			},

pkg/webhooks/openstackcluster_webhook.go

@@ -110,7 +110,7 @@ func (*openStackClusterWebhook) ValidateUpdate(_ context.Context, oldObjRaw, new
 
 	// Allow to remove the bastion spec only if it was disabled before.
 	if newObj.Spec.Bastion == nil {
-		if oldObj.Spec.Bastion != nil && oldObj.Spec.Bastion.Enabled {
+		if oldObj.Spec.Bastion.IsEnabled() {
 			allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "bastion"), "cannot be removed before disabling it"))
 		}
 	}