Bastion is enabled by default if specified

Bastion.Enabled is a wart. Ideally it would not be required, but because
of limitations in how we delete the bastion we require an intermediate
'disabled' step before removal. Ultimately we intend to remove this
limitation.

Eventually we would like to be able to ignore enabled entirely. e.g. To
create a Bastion just specify it:

```yaml
spec:
  bastion:
    spec:
      ...
    floatingIP: x.x.x.x
```

and to delete it just remove the bastion field.

Right now with enabled defaulting to `false`, doing the above will not
result in the creation of a bastion, because enabled must be explicitly
set to true.

Paving the way for the eventual deprecation of Bastion.Enabled, we
change the default value of enabled to be true so the above does today
what we eventually want it to do. This is also generally more intuitive:
why would you include a bastion and a spec if you didn't want to create
a bastion? Having to also set enabled to true is currently a trip
hazard.

Until we resolve the limitations of bastion deletion, though, we still
need to be able to disable the bastion. For this case enabled can be
explicitly set to false.

In the future when we remove the requirement to disable the bastion
before deletion the user can simply ignore Bastion.Enabled, which will
continue to work but without the limitations.

Author: mdbooth

api/v1alpha5/zz_generated.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/v1alpha6/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/v1alpha6/zz_generated.conversion.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.

api/v1alpha7/openstackcluster_conversion.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.

api/v1alpha7/zz_generated.conversion.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),

api/v1beta1/types.go

@@ -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

api/v1beta1/zz_generated.deepcopy.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))
 	}
 

config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml

@@ -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{},
 			},

config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml

@@ -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"))
 		}
 	}