Simplify bastion resource initialisation

Firstly, we remove the resource reconcile calls from the cluster flow
before calling reconcileNormal/reconcileDelete because the guards around
them and various other guards throughout the code are heavily
inter-depdendent and hard to reason about. Instead, we push them to the
the places we:
* know they are required
* know we are sufficiently initialised that they can work

Firstly we resolve references at the top of reconcileBastion. We know
the cluster has been initialised at this point, so we don't need to
guard against it. This also means that it is always called when entering
that function, so we don't need to guard against it not having been
called during first cluster initialisation.

We also force that function to re-reconcile if it calls deleteBastion(),
because deleteBastion() removes the bastion status. We reconcile again,
so we always know that it is set.

We also add an explicit call to resource reconcile in the
reconcileDelete flow. This is the only place we now need a 'weird' guard
against the cluster network not having been set. We add a comment about
that appropriate to its weirdness.

Author: mdbooth

controllers/openstackcluster_controller.go

@@ -24,6 +24,7 @@ import (
 	"time"
 
 	"github.com/gophercloud/gophercloud/openstack/networking/v2/networks"
+	"github.com/gophercloud/gophercloud/openstack/networking/v2/ports"
 	"github.com/gophercloud/gophercloud/openstack/networking/v2/subnets"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -123,30 +124,6 @@ func (r *OpenStackClusterReconciler) Reconcile(ctx context.Context, req ctrl.Req
 	}
 	scope := scope.NewWithLogger(clientScope, log)
 
-	// Resolve and store referenced & dependent resources for the bastion
-	if openStackCluster.Spec.Bastion != nil && openStackCluster.Spec.Bastion.Enabled {
-		if openStackCluster.Status.Bastion == nil {
-			openStackCluster.Status.Bastion = &infrav1.BastionStatus{}
-		}
-		changed, err := compute.ResolveReferencedMachineResources(scope, openStackCluster, &openStackCluster.Spec.Bastion.Instance, &openStackCluster.Status.Bastion.ReferencedResources)
-		if err != nil {
-			return reconcile.Result{}, err
-		}
-		if changed {
-			// If the referenced resources have changed, we need to update the OpenStackCluster status now.
-			return reconcile.Result{}, nil
-		}
-
-		changed, err = compute.ResolveDependentBastionResources(scope, openStackCluster, bastionName(cluster.Name))
-		if err != nil {
-			return reconcile.Result{}, err
-		}
-		if changed {
-			// If the dependent resources have changed, we need to update the OpenStackCluster status now.
-			return reconcile.Result{}, nil
-		}
-	}
-
 	// Handle deleted clusters
 	if !openStackCluster.DeletionTimestamp.IsZero() {
 		return r.reconcileDelete(ctx, scope, cluster, openStackCluster)
@@ -173,8 +150,17 @@ func (r *OpenStackClusterReconciler) reconcileDelete(ctx context.Context, scope
 		return ctrl.Result{RequeueAfter: 5 * time.Second}, nil
 	}
 
-	if err := deleteBastion(scope, cluster, openStackCluster); err != nil {
-		return reconcile.Result{}, err
+	// A bastion may have been created if cluster initialisation previously reached populating the network status
+	// We attempt to delete it even if no status was written, just in case
+	if openStackCluster.Status.Network != nil {
+		// Attempt to resolve bastion resources before delete. We don't need to worry about starting if the resources have changed on update.
+		if _, err := resolveBastionResources(scope, openStackCluster); err != nil {
+			return reconcile.Result{}, err
+		}
+
+		if err := deleteBastion(scope, cluster, openStackCluster); err != nil {
+			return reconcile.Result{}, err
+		}
 	}
 
 	networkingService, err := networking.NewService(scope)
@@ -234,6 +220,32 @@ func contains(arr []string, target string) bool {
 	return false
 }
 
+func resolveBastionResources(scope *scope.WithLogger, openStackCluster *infrav1.OpenStackCluster) (bool, error) {
+	if openStackCluster.Spec.Bastion != nil && openStackCluster.Spec.Bastion.Enabled {
+		if openStackCluster.Status.Bastion == nil {
+			openStackCluster.Status.Bastion = &infrav1.BastionStatus{}
+		}
+		changed, err := compute.ResolveReferencedMachineResources(scope, openStackCluster, &openStackCluster.Spec.Bastion.Instance, &openStackCluster.Status.Bastion.ReferencedResources)
+		if err != nil {
+			return false, err
+		}
+		if changed {
+			// If the referenced resources have changed, we need to update the OpenStackCluster status now.
+			return true, nil
+		}
+
+		changed, err = compute.ResolveDependentBastionResources(scope, openStackCluster, bastionName(openStackCluster.Name))
+		if err != nil {
+			return false, err
+		}
+		if changed {
+			// If the dependent resources have changed, we need to update the OpenStackCluster status now.
+			return true, nil
+		}
+	}
+	return false, nil
+}
+
 func deleteBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openStackCluster *infrav1.OpenStackCluster) error {
 	scope.Logger().Info("Deleting Bastion")
 
@@ -307,7 +319,7 @@ func deleteBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openStac
 		openStackCluster.Status.Bastion.DependentResources.Ports = nil
 	}
 
-	scope.Logger().Info("Deleted Bastion for cluster %s", cluster.Name)
+	scope.Logger().Info("Deleted Bastion")
 
 	openStackCluster.Status.Bastion = nil
 	delete(openStackCluster.ObjectMeta.Annotations, BastionInstanceHashAnnotation)
@@ -335,8 +347,11 @@ func reconcileNormal(scope *scope.WithLogger, cluster *clusterv1.Cluster, openSt
 	}
 
 	result, err := reconcileBastion(scope, cluster, openStackCluster)
-	if err != nil || !reflect.DeepEqual(result, reconcile.Result{}) {
-		return result, err
+	if err != nil {
+		return reconcile.Result{}, err
+	}
+	if result != nil {
+		return *result, nil
 	}
 
 	availabilityZones, err := computeService.GetAvailabilityZones()
@@ -366,102 +381,126 @@ func reconcileNormal(scope *scope.WithLogger, cluster *clusterv1.Cluster, openSt
 	return reconcile.Result{}, nil
 }
 
-func reconcileBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openStackCluster *infrav1.OpenStackCluster) (ctrl.Result, error) {
-	scope.Logger().Info("Reconciling Bastion")
+func reconcileBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openStackCluster *infrav1.OpenStackCluster) (*ctrl.Result, error) {
+	scope.Logger().V(4).Info("Reconciling Bastion")
 
-	if openStackCluster.Spec.Bastion == nil || !openStackCluster.Spec.Bastion.Enabled {
-		return reconcile.Result{}, deleteBastion(scope, cluster, openStackCluster)
+	changed, err := resolveBastionResources(scope, openStackCluster)
+	if err != nil {
+		return nil, err
+	}
+	if changed {
+		return &reconcile.Result{}, nil
 	}
 
-	// If ports options aren't in the status, we'll re-trigger the reconcile to get them
-	// via adopting the referenced resources.
-	if len(openStackCluster.Status.Bastion.ReferencedResources.Ports) == 0 {
-		return reconcile.Result{}, nil
+	// No Bastion defined
+	if openStackCluster.Spec.Bastion == nil || !openStackCluster.Spec.Bastion.Enabled {
+		// Delete any existing bastion
+		if openStackCluster.Status.Bastion != nil {
+			if err := deleteBastion(scope, cluster, openStackCluster); err != nil {
+				return nil, err
+			}
+			// Reconcile again before continuing
+			return &reconcile.Result{}, nil
+		}
+
+		// Otherwise nothing to do
+		return nil, nil
 	}
 
 	computeService, err := compute.NewService(scope)
 	if err != nil {
-		return reconcile.Result{}, err
+		return nil, err
 	}
 
 	networkingService, err := networking.NewService(scope)
 	if err != nil {
-		return reconcile.Result{}, err
+		return nil, err
 	}
 
 	instanceSpec, err := bastionToInstanceSpec(openStackCluster, cluster)
 	if err != nil {
-		return reconcile.Result{}, err
+		return nil, err
 	}
 	clusterName := fmt.Sprintf("%s-%s", cluster.Namespace, cluster.Name)
+
 	bastionHash, err := compute.HashInstanceSpec(instanceSpec)
 	if err != nil {
-		return reconcile.Result{}, fmt.Errorf("failed computing bastion hash from instance spec: %w", err)
+		return nil, fmt.Errorf("failed computing bastion hash from instance spec: %w", err)
 	}
 	if bastionHashHasChanged(bastionHash, openStackCluster.ObjectMeta.Annotations) {
 		if err := deleteBastion(scope, cluster, openStackCluster); err != nil {
-			return ctrl.Result{}, err
+			return nil, err
 		}
+
+		// Add the new annotation and reconcile again before continuing
+		annotations.AddAnnotations(openStackCluster, map[string]string{BastionInstanceHashAnnotation: bastionHash})
+		return &reconcile.Result{}, nil
 	}
 
 	err = getOrCreateBastionPorts(openStackCluster, networkingService, cluster.Name)
 	if err != nil {
 		handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to get or create ports for bastion: %w", err))
-		return ctrl.Result{}, fmt.Errorf("failed to get or create ports for bastion: %w", err)
+		return nil, fmt.Errorf("failed to get or create ports for bastion: %w", err)
 	}
 	bastionPortIDs := GetPortIDs(openStackCluster.Status.Bastion.DependentResources.Ports)
 
 	var instanceStatus *compute.InstanceStatus
 	if openStackCluster.Status.Bastion != nil && openStackCluster.Status.Bastion.ID != "" {
 		if instanceStatus, err = computeService.GetInstanceStatus(openStackCluster.Status.Bastion.ID); err != nil {
-			return reconcile.Result{}, err
+			return nil, err
 		}
 	}
 	if instanceStatus == nil {
 		// Check if there is an existing instance with bastion name, in case where bastion ID would not have been properly stored in cluster status
 		if instanceStatus, err = computeService.GetInstanceStatusByName(openStackCluster, instanceSpec.Name); err != nil {
-			return reconcile.Result{}, err
+			return nil, err
 		}
 	}
 	if instanceStatus == nil {
 		instanceStatus, err = computeService.CreateInstance(openStackCluster, instanceSpec, bastionPortIDs)
 		if err != nil {
-			return reconcile.Result{}, fmt.Errorf("failed to create bastion: %w", err)
+			return nil, fmt.Errorf("failed to create bastion: %w", err)
 		}
 	}
 
 	// Save hash & status as soon as we know we have an instance
 	instanceStatus.UpdateBastionStatus(openStackCluster)
-	annotations.AddAnnotations(openStackCluster, map[string]string{BastionInstanceHashAnnotation: bastionHash})
 
 	// Make sure that bastion instance has a valid state
 	switch instanceStatus.State() {
 	case infrav1.InstanceStateError:
-		return ctrl.Result{}, fmt.Errorf("failed to reconcile bastion, instance state is ERROR")
+		return nil, fmt.Errorf("failed to reconcile bastion, instance state is ERROR")
 	case infrav1.InstanceStateBuild, infrav1.InstanceStateUndefined:
 		scope.Logger().Info("Waiting for bastion instance to become ACTIVE", "id", instanceStatus.ID(), "status", instanceStatus.State())
-		return ctrl.Result{RequeueAfter: waitForBuildingInstanceToReconcile}, nil
+		return &reconcile.Result{RequeueAfter: waitForBuildingInstanceToReconcile}, nil
 	case infrav1.InstanceStateDeleted:
-		// This should normally be handled by deleteBastion
-		openStackCluster.Status.Bastion = nil
-		return ctrl.Result{}, nil
+		// Not clear why this would happen, so try to clean everything up before reconciling again
+		if err := deleteBastion(scope, cluster, openStackCluster); err != nil {
+			return nil, err
+		}
+		return &reconcile.Result{}, nil
 	}
 
 	port, err := computeService.GetManagementPort(openStackCluster, instanceStatus)
 	if err != nil {
 		err = fmt.Errorf("getting management port for bastion: %w", err)
 		handleUpdateOSCError(openStackCluster, err)
-		return ctrl.Result{}, err
+		return nil, err
 	}
+
+	return bastionAddFloatingIP(openStackCluster, clusterName, port, networkingService)
+}
+
+func bastionAddFloatingIP(openStackCluster *infrav1.OpenStackCluster, clusterName string, port *ports.Port, networkingService *networking.Service) (*reconcile.Result, error) {
 	fp, err := networkingService.GetFloatingIPByPortID(port.ID)
 	if err != nil {
 		handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to get or create floating IP for bastion: %w", err))
-		return ctrl.Result{}, fmt.Errorf("failed to get floating IP for bastion port: %w", err)
+		return nil, fmt.Errorf("failed to get floating IP for bastion port: %w", err)
 	}
 	if fp != nil {
 		// Floating IP is already attached to bastion, no need to proceed
 		openStackCluster.Status.Bastion.FloatingIP = fp.FloatingIP
-		return ctrl.Result{}, nil
+		return nil, nil
 	}
 
 	var floatingIP *string
@@ -477,17 +516,17 @@ func reconcileBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openS
 	fp, err = networkingService.GetOrCreateFloatingIP(openStackCluster, openStackCluster, clusterName, floatingIP)
 	if err != nil {
 		handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to get or create floating IP for bastion: %w", err))
-		return ctrl.Result{}, fmt.Errorf("failed to get or create floating IP for bastion: %w", err)
+		return nil, fmt.Errorf("failed to get or create floating IP for bastion: %w", err)
 	}
 	openStackCluster.Status.Bastion.FloatingIP = fp.FloatingIP
 
 	err = networkingService.AssociateFloatingIP(openStackCluster, fp, port.ID)
 	if err != nil {
 		handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to associate floating IP with bastion: %w", err))
-		return ctrl.Result{}, fmt.Errorf("failed to associate floating IP with bastion: %w", err)
+		return nil, fmt.Errorf("failed to associate floating IP with bastion: %w", err)
 	}
 
-	return ctrl.Result{}, nil
+	return nil, nil
 }
 
 func bastionToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, cluster *clusterv1.Cluster) (*compute.InstanceSpec, error) {

controllers/openstackcluster_controller_test.go

@@ -302,7 +302,7 @@ var _ = Describe("OpenStackCluster controller", func() {
 			},
 		}))
 		Expect(err).To(BeNil())
-		Expect(res).To(Equal(reconcile.Result{}))
+		Expect(res).To(BeNil())
 	})
 	It("should adopt an existing bastion Floating IP if even if its uuid is not stored in status", func() {
 		testCluster.SetName("requeue-bastion")
@@ -387,7 +387,7 @@ var _ = Describe("OpenStackCluster controller", func() {
 			},
 		}))
 		Expect(err).To(BeNil())
-		Expect(res).To(Equal(reconcile.Result{}))
+		Expect(res).To(BeNil())
 	})
 	It("should requeue until bastion becomes active", func() {
 		testCluster.SetName("requeue-bastion")
@@ -466,7 +466,7 @@ var _ = Describe("OpenStackCluster controller", func() {
 			},
 		}))
 		Expect(err).To(BeNil())
-		Expect(res).To(Equal(reconcile.Result{RequeueAfter: waitForBuildingInstanceToReconcile}))
+		Expect(res).To(Equal(&reconcile.Result{RequeueAfter: waitForBuildingInstanceToReconcile}))
 	})
 	It("should delete an existing bastion even if its uuid is not stored in status", func() {
 		testCluster.SetName("delete-existing-bastion")