diff --git a/controllers/openstackcluster_controller.go b/controllers/openstackcluster_controller.go index 6adcc1100d..b34f0f4201 100644 --- a/controllers/openstackcluster_controller.go +++ b/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) { diff --git a/controllers/openstackcluster_controller_test.go b/controllers/openstackcluster_controller_test.go index 5ab69b2d77..3f3ec703d7 100644 --- a/controllers/openstackcluster_controller_test.go +++ b/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")