From ed3442f640152e33cb49d32b36bb982e0205f557 Mon Sep 17 00:00:00 2001 From: Chris Privitere <23177737+cprivitere@users.noreply.github.com> Date: Thu, 30 May 2024 21:25:06 +0000 Subject: [PATCH 01/10] feat: delete load balancers when clusters are deleted Signed-off-by: Chris Privitere <23177737+cprivitere@users.noreply.github.com> --- controllers/packetcluster_controller.go | 18 +++++- internal/emlb/emlb.go | 75 ++++++++++++++++++++++++- 2 files changed, 91 insertions(+), 2 deletions(-) diff --git a/controllers/packetcluster_controller.go b/controllers/packetcluster_controller.go index ce078137..e6e8c8ce 100644 --- a/controllers/packetcluster_controller.go +++ b/controllers/packetcluster_controller.go @@ -20,6 +20,7 @@ package controllers import ( "context" "errors" + "fmt" apierrors "k8s.io/apimachinery/pkg/api/errors" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" @@ -177,11 +178,26 @@ func (r *PacketClusterReconciler) reconcileNormal(ctx context.Context, clusterSc return nil } -func (r *PacketClusterReconciler) reconcileDelete(_ context.Context, _ *scope.ClusterScope) (ctrl.Result, error) { +func (r *PacketClusterReconciler) reconcileDelete(ctx context.Context, clusterScope *scope.ClusterScope) (ctrl.Result, error) { + log := ctrl.LoggerFrom(ctx).WithValues("cluster", clusterScope.Cluster.Name) + log.Info("Reconciling PacketCluster Deletion") + + packetCluster := clusterScope.PacketCluster + + switch { + case packetCluster.Spec.VIPManager == emlb.EMLBVIPID: + // Create new EMLB object + lb := emlb.NewEMLB(r.PacketClient.GetConfig().DefaultHeader["X-Auth-Token"], packetCluster.Spec.ProjectID, packetCluster.Spec.Metro) + + if err := lb.DeleteLoadBalancer(ctx, clusterScope); err != nil { + fmt.Println("It's ok!") + } + } // Initially I created this handler to remove an elastic IP when a cluster // gets delete, but it does not sound like a good idea. It is better to // leave to the users the ability to decide if they want to keep and resign // the IP or if they do not need it anymore + return ctrl.Result{}, nil } diff --git a/internal/emlb/emlb.go b/internal/emlb/emlb.go index 7280ff6e..f122a00a 100644 --- a/internal/emlb/emlb.go +++ b/internal/emlb/emlb.go @@ -99,7 +99,7 @@ func NewEMLB(metalAPIKey, projectID, metro string) *EMLB { return manager } -// ReconcileLoadBalancer creates a new Equinix Metal Load Balancer. +// ReconcileLoadBalancer creates a new Equinix Metal Load Balancer and associates it with the given ClusterScope. func (e *EMLB) ReconcileLoadBalancer(ctx context.Context, clusterScope *scope.ClusterScope) error { log := ctrl.LoggerFrom(ctx) @@ -233,6 +233,63 @@ func (e *EMLB) ReconcileVIPOrigin(ctx context.Context, machineScope *scope.Machi return nil } +// DeleteLoadBalancer deletes the Equinix Metal Load Balancer associated with a given ClusterScope. +func (e *EMLB) DeleteLoadBalancer(ctx context.Context, clusterScope *scope.ClusterScope) error { + log := ctrl.LoggerFrom(ctx) + + packetCluster := clusterScope.PacketCluster + clusterName := packetCluster.Name + + // Make sure the cluster already has an EMLB ID in its packetCluster annotations, otherwise abort. + lbID, exists := packetCluster.Annotations[loadBalancerIDAnnotation] + if !exists || (lbID == "") { + return fmt.Errorf("no Equinix Metal Load Balancer found in cluster's annotations") + } + + // See if the EMLB has a Port ID in its packetCluster annotations. + lbPortNumber, exists := packetCluster.Annotations[loadBalancerPortNumberAnnotation] + if !exists || (lbPortNumber == "") { + return fmt.Errorf("no Equinix Metal Load Balancer Port Number found in cluster's annotations") + } + + log.Info("Deleting EMLB", "Cluster Metro", e.metro, "Cluster Name", clusterName, "Project ID", e.projectID, "Load Balancer ID", lbID) + + // Fetch the Load Balancer object. + lb, err := e.getLoadBalancer(ctx, lbID) + if err != nil { + log.Error(err, "failed to load the loadbalancer object, cannot proceed with deletion") + return err + } + + // Get an int version of the listener port number. + portNumber, err := strconv.ParseInt(lbPortNumber, 10, 32) + if err != nil { + log.Error(err, "failed to convert the loadbalancer port number to an int, cannot proceed with deletion") + return err + } + + // Get the entire listener port object. + lbPort, err := e.getLoadBalancerPort(ctx, lbID, int32(portNumber)) + if err != nil { + log.Error(err, "failed to load the loadbalancer port object, cannot proceed with deletion") + return err + } + + resp, err := e.deleteListenerPort(ctx, lbPort.GetId()) + if err != nil { + log.Error(err, "LB Port Delete Failed", "EMLB ID", lb.GetId(), "Port ID", lbPort.GetId(), "Response Body", resp.Body) + return err + } + + resp, err = e.deleteLoadBalancer(ctx, lb.GetId()) + if err != nil { + log.Error(err, "LB Delete Failed", "EMLB ID", lb.GetId(), "Response Body", resp.Body) + return err + } + + return nil +} + // getLoadBalancer Returns a Load Balancer object given an id. func (e *EMLB) getLoadBalancer(ctx context.Context, id string) (*lbaas.LoadBalancer, error) { ctx = context.WithValue(ctx, lbaas.ContextOAuth2, e.tokenExchanger) @@ -401,6 +458,22 @@ func (e *EMLB) createOrigin(ctx context.Context, poolID, originName string, targ return e.client.PoolsApi.CreateLoadBalancerPoolOrigin(ctx, poolID).LoadBalancerPoolOriginCreate(createOriginRequest).Execute() } +func (e *EMLB) deleteLoadBalancer(ctx context.Context, lbID string) (*http.Response, error) { + return e.client.LoadBalancersApi.DeleteLoadBalancer(ctx, lbID).Execute() +} + +func (e *EMLB) deleteListenerPort(ctx context.Context, portID string) (*http.Response, error) { + return e.client.PortsApi.DeleteLoadBalancerPort(ctx, portID).Execute() +} + +func (e *EMLB) deletePool(ctx context.Context, poolID string) (*http.Response, error) { + return e.client.PoolsApi.DeleteLoadBalancerPool(ctx, poolID).Execute() +} + +func (e *EMLB) deleteOrigin(ctx context.Context, originID string) (*http.Response, error) { + return e.client.OriginsApi.DeleteLoadBalancerOrigin(ctx, originID).Execute() +} + func (e *EMLB) updateListenerPort(ctx context.Context, poolID, lbPortID string) (*lbaas.LoadBalancerPort, error) { ctx = context.WithValue(ctx, lbaas.ContextOAuth2, e.tokenExchanger) From f3ee3101719934912f7830d4ce74d20e2f8cf2a5 Mon Sep 17 00:00:00 2001 From: Chris Privitere <23177737+cprivitere@users.noreply.github.com> Date: Mon, 3 Jun 2024 22:49:23 +0000 Subject: [PATCH 02/10] feat: add finalizer code Signed-off-by: Chris Privitere <23177737+cprivitere@users.noreply.github.com> --- api/v1beta1/packetcluster_types.go | 3 +++ controllers/packetcluster_controller.go | 18 ++++++++++++++---- controllers/packetmachine_controller.go | 13 +++++++------ 3 files changed, 24 insertions(+), 10 deletions(-) diff --git a/api/v1beta1/packetcluster_types.go b/api/v1beta1/packetcluster_types.go index cba2ca62..735d4a88 100644 --- a/api/v1beta1/packetcluster_types.go +++ b/api/v1beta1/packetcluster_types.go @@ -22,6 +22,9 @@ import ( ) const ( + // ClusterFinalizer allows DockerClusterReconciler to clean up resources associated with DockerCluster before + // removing it from the apiserver. + ClusterFinalizer = "packetcluster.infrastructure.cluster.x-k8s.io" // NetworkInfrastructureReadyCondition reports of current status of cluster infrastructure. NetworkInfrastructureReadyCondition clusterv1.ConditionType = "NetworkInfrastructureReady" ) diff --git a/controllers/packetcluster_controller.go b/controllers/packetcluster_controller.go index e6e8c8ce..0545f247 100644 --- a/controllers/packetcluster_controller.go +++ b/controllers/packetcluster_controller.go @@ -32,6 +32,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/handler" infrav1 "sigs.k8s.io/cluster-api-provider-packet/api/v1beta1" @@ -103,7 +104,14 @@ func (r *PacketClusterReconciler) Reconcile(ctx context.Context, req ctrl.Reques // Handle deleted clusters if !cluster.DeletionTimestamp.IsZero() { - return r.reconcileDelete(ctx, clusterScope) + return ctrl.Result{}, r.reconcileDelete(ctx, clusterScope) + } + + // Add finalizer first if not set to avoid the race condition between init and delete. + // Note: Finalizers in general can only be added when the deletionTimestamp is not set. + if !controllerutil.ContainsFinalizer(packetcluster, infrav1.ClusterFinalizer) { + controllerutil.AddFinalizer(packetcluster, infrav1.ClusterFinalizer) + return ctrl.Result{}, nil } err = r.reconcileNormal(ctx, clusterScope) @@ -178,7 +186,7 @@ func (r *PacketClusterReconciler) reconcileNormal(ctx context.Context, clusterSc return nil } -func (r *PacketClusterReconciler) reconcileDelete(ctx context.Context, clusterScope *scope.ClusterScope) (ctrl.Result, error) { +func (r *PacketClusterReconciler) reconcileDelete(ctx context.Context, clusterScope *scope.ClusterScope) error { log := ctrl.LoggerFrom(ctx).WithValues("cluster", clusterScope.Cluster.Name) log.Info("Reconciling PacketCluster Deletion") @@ -190,7 +198,7 @@ func (r *PacketClusterReconciler) reconcileDelete(ctx context.Context, clusterSc lb := emlb.NewEMLB(r.PacketClient.GetConfig().DefaultHeader["X-Auth-Token"], packetCluster.Spec.ProjectID, packetCluster.Spec.Metro) if err := lb.DeleteLoadBalancer(ctx, clusterScope); err != nil { - fmt.Println("It's ok!") + return fmt.Errorf("failed to delete load balancer: %w", err) } } // Initially I created this handler to remove an elastic IP when a cluster @@ -198,7 +206,9 @@ func (r *PacketClusterReconciler) reconcileDelete(ctx context.Context, clusterSc // leave to the users the ability to decide if they want to keep and resign // the IP or if they do not need it anymore - return ctrl.Result{}, nil + // Cluster is deleted so remove the finalizer. + controllerutil.RemoveFinalizer(packetCluster, infrav1.ClusterFinalizer) + return nil } func (r *PacketClusterReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error { diff --git a/controllers/packetmachine_controller.go b/controllers/packetmachine_controller.go index 01eab431..b916e2be 100644 --- a/controllers/packetmachine_controller.go +++ b/controllers/packetmachine_controller.go @@ -164,6 +164,13 @@ func (r *PacketMachineReconciler) Reconcile(ctx context.Context, req ctrl.Reques } }() + // Add finalizer first if not set to avoid the race condition between init and delete. + // Note: Finalizers in general can only be added when the deletionTimestamp is not set. + if packetmachine.ObjectMeta.DeletionTimestamp.IsZero() && !controllerutil.ContainsFinalizer(packetmachine, infrav1.MachineFinalizer) { + controllerutil.AddFinalizer(packetmachine, infrav1.MachineFinalizer) + return ctrl.Result{}, nil + } + // Handle deleted machines if !packetmachine.ObjectMeta.DeletionTimestamp.IsZero() { err = r.reconcileDelete(ctx, machineScope) @@ -262,12 +269,6 @@ func (r *PacketMachineReconciler) reconcile(ctx context.Context, machineScope *s return ctrl.Result{}, nil } - // If the PacketMachine doesn't have our finalizer, add it. - controllerutil.AddFinalizer(packetmachine, infrav1.MachineFinalizer) - if err := machineScope.PatchObject(ctx); err != nil { - log.Error(err, "unable to patch object") - } - if !machineScope.Cluster.Status.InfrastructureReady { log.Info("Cluster infrastructure is not ready yet") conditions.MarkFalse(machineScope.PacketMachine, infrav1.DeviceReadyCondition, infrav1.WaitingForClusterInfrastructureReason, clusterv1.ConditionSeverityInfo, "") From ba20443b71549b5285ac7d208dbf477be941e741 Mon Sep 17 00:00:00 2001 From: Chris Privitere <23177737+cprivitere@users.noreply.github.com> Date: Fri, 7 Jun 2024 11:54:37 -0500 Subject: [PATCH 03/10] fix: move from switch to if Signed-off-by: Chris Privitere <23177737+cprivitere@users.noreply.github.com> --- controllers/packetcluster_controller.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/controllers/packetcluster_controller.go b/controllers/packetcluster_controller.go index 0545f247..1bd0d5bf 100644 --- a/controllers/packetcluster_controller.go +++ b/controllers/packetcluster_controller.go @@ -192,8 +192,7 @@ func (r *PacketClusterReconciler) reconcileDelete(ctx context.Context, clusterSc packetCluster := clusterScope.PacketCluster - switch { - case packetCluster.Spec.VIPManager == emlb.EMLBVIPID: + if packetCluster.Spec.VIPManager == emlb.EMLBVIPID { // Create new EMLB object lb := emlb.NewEMLB(r.PacketClient.GetConfig().DefaultHeader["X-Auth-Token"], packetCluster.Spec.ProjectID, packetCluster.Spec.Metro) From e61b829dcf833ba90278c14ecac37b702d9e6c8b Mon Sep 17 00:00:00 2001 From: Chris Privitere <23177737+cprivitere@users.noreply.github.com> Date: Fri, 7 Jun 2024 11:56:02 -0500 Subject: [PATCH 04/10] feat: add emlb deletion to machien controller Signed-off-by: Chris Privitere <23177737+cprivitere@users.noreply.github.com> --- controllers/packetmachine_controller.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/controllers/packetmachine_controller.go b/controllers/packetmachine_controller.go index b916e2be..209aedac 100644 --- a/controllers/packetmachine_controller.go +++ b/controllers/packetmachine_controller.go @@ -537,12 +537,21 @@ func (r *PacketMachineReconciler) reconcileDelete(ctx context.Context, machineSc device = dev } - // We should never get there but this is a safetly check + // We should never get there but this is a safety check if device == nil { controllerutil.RemoveFinalizer(packetmachine, infrav1.MachineFinalizer) return fmt.Errorf("%w: %s", errMissingDevice, packetmachine.Name) } + if machineScope.PacketCluster.Spec.VIPManager == emlb.EMLBVIPID { + // Create new EMLB object + lb := emlb.NewEMLB(r.PacketClient.GetConfig().DefaultHeader["X-Auth-Token"], machineScope.PacketCluster.Spec.ProjectID, packetmachine.Spec.Metro) + + if err := lb.DeleteLoadBalancerOrigin(ctx, machineScope); err != nil { + return fmt.Errorf("failed to delete load balancer origin: %w", err) + } + } + apiRequest := r.PacketClient.DevicesApi.DeleteDevice(ctx, device.GetId()).ForceDelete(force) if _, err := apiRequest.Execute(); err != nil { //nolint:bodyclose // see https://github.com/timakin/bodyclose/issues/42 return fmt.Errorf("failed to delete the machine: %w", err) From cebd848778bea45b814a0082e67c1a537c333c95 Mon Sep 17 00:00:00 2001 From: Chris Privitere <23177737+cprivitere@users.noreply.github.com> Date: Fri, 7 Jun 2024 11:56:20 -0500 Subject: [PATCH 05/10] fix: move forward even though there are errors Signed-off-by: Chris Privitere <23177737+cprivitere@users.noreply.github.com> --- internal/emlb/emlb.go | 66 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 64 insertions(+), 2 deletions(-) diff --git a/internal/emlb/emlb.go b/internal/emlb/emlb.go index f122a00a..870e9994 100644 --- a/internal/emlb/emlb.go +++ b/internal/emlb/emlb.go @@ -243,18 +243,21 @@ func (e *EMLB) DeleteLoadBalancer(ctx context.Context, clusterScope *scope.Clust // Make sure the cluster already has an EMLB ID in its packetCluster annotations, otherwise abort. lbID, exists := packetCluster.Annotations[loadBalancerIDAnnotation] if !exists || (lbID == "") { - return fmt.Errorf("no Equinix Metal Load Balancer found in cluster's annotations") + log.Info("no Equinix Metal Load Balancer found in cluster's annotations, skipping EMLB delete") + return nil } // See if the EMLB has a Port ID in its packetCluster annotations. lbPortNumber, exists := packetCluster.Annotations[loadBalancerPortNumberAnnotation] if !exists || (lbPortNumber == "") { - return fmt.Errorf("no Equinix Metal Load Balancer Port Number found in cluster's annotations") + log.Info("no Equinix Metal Load Balancer Port Number found in cluster's annotations, skipping EMLB delete") + return nil } log.Info("Deleting EMLB", "Cluster Metro", e.metro, "Cluster Name", clusterName, "Project ID", e.projectID, "Load Balancer ID", lbID) // Fetch the Load Balancer object. + // Skip if 404, otherwise error lb, err := e.getLoadBalancer(ctx, lbID) if err != nil { log.Error(err, "failed to load the loadbalancer object, cannot proceed with deletion") @@ -269,6 +272,7 @@ func (e *EMLB) DeleteLoadBalancer(ctx context.Context, clusterScope *scope.Clust } // Get the entire listener port object. + // Skip if 404, otherwise error lbPort, err := e.getLoadBalancerPort(ctx, lbID, int32(portNumber)) if err != nil { log.Error(err, "failed to load the loadbalancer port object, cannot proceed with deletion") @@ -290,6 +294,52 @@ func (e *EMLB) DeleteLoadBalancer(ctx context.Context, clusterScope *scope.Clust return nil } +// DeleteLoadBalancerOrigin deletes the Equinix Metal Load Balancer associated with a given ClusterScope. +func (e *EMLB) DeleteLoadBalancerOrigin(ctx context.Context, machineScope *scope.MachineScope) error { + log := ctrl.LoggerFrom(ctx) + + clusterName := machineScope.Cluster.Name + + // Make sure the machine has an EMLB Pool ID in its packetMachine annotations, otherwise abort. + lbPoolID, exists := machineScope.PacketMachine.Annotations[loadBalancerPoolIDAnnotation] + if !exists || (lbPoolID == "") { + return fmt.Errorf("no Equinix Metal Load Balancer Pool found in machine's annotations") + } + + // Make sure the machine has an EMLB Origin ID in its packetMachine annotations, otherwise abort. + lbOriginID, exists := machineScope.PacketMachine.Annotations[loadBalancerOriginIDAnnotation] + if !exists || (lbOriginID == "") { + return fmt.Errorf("no Equinix Metal Load Balancer Origin found in machine's annotations") + } + + log.Info("Deleting EMLB Origin from Pool", "Cluster Metro", e.metro, "Cluster Name", clusterName, "Project ID", e.projectID, "Pool ID", lbPoolID, "Origin ID", lbOriginID) + + // Fetch the Load Balancer Pool object. + lbPool, err := e.getLoadBalancerPool(ctx, lbPoolID) + if err != nil { + log.Error(err, "failed to load the loadbalancer pool object, cannot proceed with deletion") + return err + } + + resp, err := e.deleteOrigin(ctx, lbOriginID) + if err != nil { + log.Error(err, "LB Origin Delete Failed", "Pool ID", lbPool.GetId(), "Origin ID", lbOriginID, "Response Body", resp.Body) + return err + } + + // TODO: Validate pool is empty + + log.Info("Deleting EMLB Pool", "Cluster Metro", e.metro, "Cluster Name", clusterName, "Project ID", e.projectID, "Pool ID", lbPoolID) + + resp, err = e.deletePool(ctx, lbPool.GetId()) + if err != nil { + log.Error(err, "LB Pool Delete Failed", "Pool ID", lbPool.GetId(), "Response Body", resp.Body) + return err + } + + return nil +} + // getLoadBalancer Returns a Load Balancer object given an id. func (e *EMLB) getLoadBalancer(ctx context.Context, id string) (*lbaas.LoadBalancer, error) { ctx = context.WithValue(ctx, lbaas.ContextOAuth2, e.tokenExchanger) @@ -306,6 +356,14 @@ func (e *EMLB) getLoadBalancerPort(ctx context.Context, id string, portNumber in return LoadBalancerPort, err } +// getLoadBalancerPool Returns a Load Balancer Pool object given an id. +func (e *EMLB) getLoadBalancerPool(ctx context.Context, id string) (*lbaas.LoadBalancerPool, error) { + ctx = context.WithValue(ctx, lbaas.ContextOAuth2, e.tokenExchanger) + + LoadBalancerPool, _, err := e.client.PoolsApi.GetLoadBalancerPool(ctx, id).Execute() + return LoadBalancerPool, err +} + // EnsureLoadBalancerOrigin takes the devices list of IP addresses in a Load Balancer Origin Pool and ensures an origin // for the first IPv4 address in the list exists. func (e *EMLB) ensureLoadBalancerOrigin(ctx context.Context, originID, poolID, lbName string, deviceAddr []corev1.NodeAddress) (*lbaas.LoadBalancerPoolOrigin, error) { @@ -459,18 +517,22 @@ func (e *EMLB) createOrigin(ctx context.Context, poolID, originName string, targ } func (e *EMLB) deleteLoadBalancer(ctx context.Context, lbID string) (*http.Response, error) { + ctx = context.WithValue(ctx, lbaas.ContextOAuth2, e.tokenExchanger) return e.client.LoadBalancersApi.DeleteLoadBalancer(ctx, lbID).Execute() } func (e *EMLB) deleteListenerPort(ctx context.Context, portID string) (*http.Response, error) { + ctx = context.WithValue(ctx, lbaas.ContextOAuth2, e.tokenExchanger) return e.client.PortsApi.DeleteLoadBalancerPort(ctx, portID).Execute() } func (e *EMLB) deletePool(ctx context.Context, poolID string) (*http.Response, error) { + ctx = context.WithValue(ctx, lbaas.ContextOAuth2, e.tokenExchanger) return e.client.PoolsApi.DeleteLoadBalancerPool(ctx, poolID).Execute() } func (e *EMLB) deleteOrigin(ctx context.Context, originID string) (*http.Response, error) { + ctx = context.WithValue(ctx, lbaas.ContextOAuth2, e.tokenExchanger) return e.client.OriginsApi.DeleteLoadBalancerOrigin(ctx, originID).Execute() } From 9eb281217c3c80c137d43e7c325b2a5e7a77a07d Mon Sep 17 00:00:00 2001 From: Chris Privitere <23177737+cprivitere@users.noreply.github.com> Date: Mon, 10 Jun 2024 20:48:47 +0000 Subject: [PATCH 06/10] chore: remove origin and port deletions as they were unnecessary Signed-off-by: Chris Privitere <23177737+cprivitere@users.noreply.github.com> --- internal/emlb/emlb.go | 59 +++---------------------------------------- 1 file changed, 4 insertions(+), 55 deletions(-) diff --git a/internal/emlb/emlb.go b/internal/emlb/emlb.go index 870e9994..0f4ffd5e 100644 --- a/internal/emlb/emlb.go +++ b/internal/emlb/emlb.go @@ -247,13 +247,6 @@ func (e *EMLB) DeleteLoadBalancer(ctx context.Context, clusterScope *scope.Clust return nil } - // See if the EMLB has a Port ID in its packetCluster annotations. - lbPortNumber, exists := packetCluster.Annotations[loadBalancerPortNumberAnnotation] - if !exists || (lbPortNumber == "") { - log.Info("no Equinix Metal Load Balancer Port Number found in cluster's annotations, skipping EMLB delete") - return nil - } - log.Info("Deleting EMLB", "Cluster Metro", e.metro, "Cluster Name", clusterName, "Project ID", e.projectID, "Load Balancer ID", lbID) // Fetch the Load Balancer object. @@ -264,28 +257,7 @@ func (e *EMLB) DeleteLoadBalancer(ctx context.Context, clusterScope *scope.Clust return err } - // Get an int version of the listener port number. - portNumber, err := strconv.ParseInt(lbPortNumber, 10, 32) - if err != nil { - log.Error(err, "failed to convert the loadbalancer port number to an int, cannot proceed with deletion") - return err - } - - // Get the entire listener port object. - // Skip if 404, otherwise error - lbPort, err := e.getLoadBalancerPort(ctx, lbID, int32(portNumber)) - if err != nil { - log.Error(err, "failed to load the loadbalancer port object, cannot proceed with deletion") - return err - } - - resp, err := e.deleteListenerPort(ctx, lbPort.GetId()) - if err != nil { - log.Error(err, "LB Port Delete Failed", "EMLB ID", lb.GetId(), "Port ID", lbPort.GetId(), "Response Body", resp.Body) - return err - } - - resp, err = e.deleteLoadBalancer(ctx, lb.GetId()) + resp, err := e.deleteLoadBalancer(ctx, lb.GetId()) if err != nil { log.Error(err, "LB Delete Failed", "EMLB ID", lb.GetId(), "Response Body", resp.Body) return err @@ -296,6 +268,7 @@ func (e *EMLB) DeleteLoadBalancer(ctx context.Context, clusterScope *scope.Clust // DeleteLoadBalancerOrigin deletes the Equinix Metal Load Balancer associated with a given ClusterScope. func (e *EMLB) DeleteLoadBalancerOrigin(ctx context.Context, machineScope *scope.MachineScope) error { + // Initially, we're creating a single pool per origin, logic below needs to be updated if we move to a shared load balancer pool model. log := ctrl.LoggerFrom(ctx) clusterName := machineScope.Cluster.Name @@ -306,13 +279,7 @@ func (e *EMLB) DeleteLoadBalancerOrigin(ctx context.Context, machineScope *scope return fmt.Errorf("no Equinix Metal Load Balancer Pool found in machine's annotations") } - // Make sure the machine has an EMLB Origin ID in its packetMachine annotations, otherwise abort. - lbOriginID, exists := machineScope.PacketMachine.Annotations[loadBalancerOriginIDAnnotation] - if !exists || (lbOriginID == "") { - return fmt.Errorf("no Equinix Metal Load Balancer Origin found in machine's annotations") - } - - log.Info("Deleting EMLB Origin from Pool", "Cluster Metro", e.metro, "Cluster Name", clusterName, "Project ID", e.projectID, "Pool ID", lbPoolID, "Origin ID", lbOriginID) + log.Info("Deleting EMLB Origin from Pool", "Cluster Metro", e.metro, "Cluster Name", clusterName, "Project ID", e.projectID, "Pool ID", lbPoolID) // Fetch the Load Balancer Pool object. lbPool, err := e.getLoadBalancerPool(ctx, lbPoolID) @@ -321,17 +288,9 @@ func (e *EMLB) DeleteLoadBalancerOrigin(ctx context.Context, machineScope *scope return err } - resp, err := e.deleteOrigin(ctx, lbOriginID) - if err != nil { - log.Error(err, "LB Origin Delete Failed", "Pool ID", lbPool.GetId(), "Origin ID", lbOriginID, "Response Body", resp.Body) - return err - } - - // TODO: Validate pool is empty - log.Info("Deleting EMLB Pool", "Cluster Metro", e.metro, "Cluster Name", clusterName, "Project ID", e.projectID, "Pool ID", lbPoolID) - resp, err = e.deletePool(ctx, lbPool.GetId()) + resp, err := e.deletePool(ctx, lbPool.GetId()) if err != nil { log.Error(err, "LB Pool Delete Failed", "Pool ID", lbPool.GetId(), "Response Body", resp.Body) return err @@ -521,21 +480,11 @@ func (e *EMLB) deleteLoadBalancer(ctx context.Context, lbID string) (*http.Respo return e.client.LoadBalancersApi.DeleteLoadBalancer(ctx, lbID).Execute() } -func (e *EMLB) deleteListenerPort(ctx context.Context, portID string) (*http.Response, error) { - ctx = context.WithValue(ctx, lbaas.ContextOAuth2, e.tokenExchanger) - return e.client.PortsApi.DeleteLoadBalancerPort(ctx, portID).Execute() -} - func (e *EMLB) deletePool(ctx context.Context, poolID string) (*http.Response, error) { ctx = context.WithValue(ctx, lbaas.ContextOAuth2, e.tokenExchanger) return e.client.PoolsApi.DeleteLoadBalancerPool(ctx, poolID).Execute() } -func (e *EMLB) deleteOrigin(ctx context.Context, originID string) (*http.Response, error) { - ctx = context.WithValue(ctx, lbaas.ContextOAuth2, e.tokenExchanger) - return e.client.OriginsApi.DeleteLoadBalancerOrigin(ctx, originID).Execute() -} - func (e *EMLB) updateListenerPort(ctx context.Context, poolID, lbPortID string) (*lbaas.LoadBalancerPort, error) { ctx = context.WithValue(ctx, lbaas.ContextOAuth2, e.tokenExchanger) From f3215df83ba4a34953c262869ba8e031fa23edfd Mon Sep 17 00:00:00 2001 From: Chris Privitere <23177737+cprivitere@users.noreply.github.com> Date: Wed, 12 Jun 2024 15:22:19 -0500 Subject: [PATCH 07/10] refactor: ignore 404 errors while deleting load balancers Signed-off-by: Chris Privitere <23177737+cprivitere@users.noreply.github.com> --- internal/emlb/emlb.go | 42 ++++++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/internal/emlb/emlb.go b/internal/emlb/emlb.go index 0f4ffd5e..df289856 100644 --- a/internal/emlb/emlb.go +++ b/internal/emlb/emlb.go @@ -156,7 +156,7 @@ func (e *EMLB) ReconcileVIPOrigin(ctx context.Context, machineScope *scope.Machi } // Fetch the Load Balancer object. - lb, err := e.getLoadBalancer(ctx, lbID) + lb, _, err := e.getLoadBalancer(ctx, lbID) if err != nil { return err } @@ -247,18 +247,18 @@ func (e *EMLB) DeleteLoadBalancer(ctx context.Context, clusterScope *scope.Clust return nil } - log.Info("Deleting EMLB", "Cluster Metro", e.metro, "Cluster Name", clusterName, "Project ID", e.projectID, "Load Balancer ID", lbID) - // Fetch the Load Balancer object. // Skip if 404, otherwise error - lb, err := e.getLoadBalancer(ctx, lbID) - if err != nil { - log.Error(err, "failed to load the loadbalancer object, cannot proceed with deletion") + lb, resp, err := e.getLoadBalancer(ctx, lbID) + if err != nil && (resp.StatusCode != http.StatusNotFound) { + log.Error(err, "unexpected error while loading the loadbalancer object, cannot proceed with deletion") return err } - resp, err := e.deleteLoadBalancer(ctx, lb.GetId()) - if err != nil { + log.Info("Deleting EMLB", "Cluster Metro", e.metro, "Cluster Name", clusterName, "Project ID", e.projectID, "Load Balancer ID", lbID) + + resp, err = e.deleteLoadBalancer(ctx, lb.GetId()) + if err != nil && (resp.StatusCode != http.StatusNotFound) { log.Error(err, "LB Delete Failed", "EMLB ID", lb.GetId(), "Response Body", resp.Body) return err } @@ -279,19 +279,17 @@ func (e *EMLB) DeleteLoadBalancerOrigin(ctx context.Context, machineScope *scope return fmt.Errorf("no Equinix Metal Load Balancer Pool found in machine's annotations") } - log.Info("Deleting EMLB Origin from Pool", "Cluster Metro", e.metro, "Cluster Name", clusterName, "Project ID", e.projectID, "Pool ID", lbPoolID) - // Fetch the Load Balancer Pool object. - lbPool, err := e.getLoadBalancerPool(ctx, lbPoolID) - if err != nil { - log.Error(err, "failed to load the loadbalancer pool object, cannot proceed with deletion") + lbPool, resp, err := e.getLoadBalancerPool(ctx, lbPoolID) + if err != nil && (resp.StatusCode != http.StatusNotFound) { + log.Error(err, "unexpected error while loading the loadbalancer pool object, cannot proceed with deletion") return err } log.Info("Deleting EMLB Pool", "Cluster Metro", e.metro, "Cluster Name", clusterName, "Project ID", e.projectID, "Pool ID", lbPoolID) - resp, err := e.deletePool(ctx, lbPool.GetId()) - if err != nil { + resp, err = e.deletePool(ctx, lbPool.GetId()) + if err != nil && (resp.StatusCode != http.StatusNotFound) { log.Error(err, "LB Pool Delete Failed", "Pool ID", lbPool.GetId(), "Response Body", resp.Body) return err } @@ -300,11 +298,11 @@ func (e *EMLB) DeleteLoadBalancerOrigin(ctx context.Context, machineScope *scope } // getLoadBalancer Returns a Load Balancer object given an id. -func (e *EMLB) getLoadBalancer(ctx context.Context, id string) (*lbaas.LoadBalancer, error) { +func (e *EMLB) getLoadBalancer(ctx context.Context, id string) (*lbaas.LoadBalancer, *http.Response, error) { ctx = context.WithValue(ctx, lbaas.ContextOAuth2, e.tokenExchanger) - LoadBalancer, _, err := e.client.LoadBalancersApi.GetLoadBalancer(ctx, id).Execute() - return LoadBalancer, err + LoadBalancer, resp, err := e.client.LoadBalancersApi.GetLoadBalancer(ctx, id).Execute() + return LoadBalancer, resp, err } // getLoadBalancerPort Returns a Load Balancer Port object given an id. @@ -316,11 +314,11 @@ func (e *EMLB) getLoadBalancerPort(ctx context.Context, id string, portNumber in } // getLoadBalancerPool Returns a Load Balancer Pool object given an id. -func (e *EMLB) getLoadBalancerPool(ctx context.Context, id string) (*lbaas.LoadBalancerPool, error) { +func (e *EMLB) getLoadBalancerPool(ctx context.Context, id string) (*lbaas.LoadBalancerPool, *http.Response, error) { ctx = context.WithValue(ctx, lbaas.ContextOAuth2, e.tokenExchanger) - LoadBalancerPool, _, err := e.client.PoolsApi.GetLoadBalancerPool(ctx, id).Execute() - return LoadBalancerPool, err + LoadBalancerPool, resp, err := e.client.PoolsApi.GetLoadBalancerPool(ctx, id).Execute() + return LoadBalancerPool, resp, err } // EnsureLoadBalancerOrigin takes the devices list of IP addresses in a Load Balancer Origin Pool and ensures an origin @@ -424,7 +422,7 @@ func (e *EMLB) ensureLoadBalancer(ctx context.Context, lbID, lbname string, port } // Regardless of whether we just created it, fetch the loadbalancer object. - lb, err := e.getLoadBalancer(ctx, lbID) + lb, _, err := e.getLoadBalancer(ctx, lbID) if err != nil { return nil, nil, err } From 70b4b26fddc0782bb0bf1b0ad1a1f6397a00d5d0 Mon Sep 17 00:00:00 2001 From: Chris Privitere <23177737+cprivitere@users.noreply.github.com> Date: Mon, 17 Jun 2024 12:26:32 -0500 Subject: [PATCH 08/10] refactor: take marques suggestion to remove unnecessary get calls Signed-off-by: Chris Privitere <23177737+cprivitere@users.noreply.github.com> --- internal/emlb/emlb.go | 39 ++++++++++++++++----------------------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/internal/emlb/emlb.go b/internal/emlb/emlb.go index df289856..18ce5f23 100644 --- a/internal/emlb/emlb.go +++ b/internal/emlb/emlb.go @@ -247,20 +247,16 @@ func (e *EMLB) DeleteLoadBalancer(ctx context.Context, clusterScope *scope.Clust return nil } - // Fetch the Load Balancer object. - // Skip if 404, otherwise error - lb, resp, err := e.getLoadBalancer(ctx, lbID) - if err != nil && (resp.StatusCode != http.StatusNotFound) { - log.Error(err, "unexpected error while loading the loadbalancer object, cannot proceed with deletion") - return err - } - log.Info("Deleting EMLB", "Cluster Metro", e.metro, "Cluster Name", clusterName, "Project ID", e.projectID, "Load Balancer ID", lbID) - resp, err = e.deleteLoadBalancer(ctx, lb.GetId()) - if err != nil && (resp.StatusCode != http.StatusNotFound) { - log.Error(err, "LB Delete Failed", "EMLB ID", lb.GetId(), "Response Body", resp.Body) - return err + resp, err := e.deleteLoadBalancer(ctx, lbID) + if err != nil { + if resp.StatusCode == http.StatusNotFound { + return nil + } else { + log.Error(err, "LB Delete Failed", "EMLB ID", lbID, "Response Body", resp.Body) + return err + } } return nil @@ -279,19 +275,16 @@ func (e *EMLB) DeleteLoadBalancerOrigin(ctx context.Context, machineScope *scope return fmt.Errorf("no Equinix Metal Load Balancer Pool found in machine's annotations") } - // Fetch the Load Balancer Pool object. - lbPool, resp, err := e.getLoadBalancerPool(ctx, lbPoolID) - if err != nil && (resp.StatusCode != http.StatusNotFound) { - log.Error(err, "unexpected error while loading the loadbalancer pool object, cannot proceed with deletion") - return err - } - log.Info("Deleting EMLB Pool", "Cluster Metro", e.metro, "Cluster Name", clusterName, "Project ID", e.projectID, "Pool ID", lbPoolID) - resp, err = e.deletePool(ctx, lbPool.GetId()) - if err != nil && (resp.StatusCode != http.StatusNotFound) { - log.Error(err, "LB Pool Delete Failed", "Pool ID", lbPool.GetId(), "Response Body", resp.Body) - return err + resp, err := e.deletePool(ctx, lbPoolID) + if err != nil { + if resp.StatusCode != http.StatusNotFound { + return nil + } else { + log.Error(err, "LB Pool Delete Failed", "Pool ID", lbPoolID, "Response Body", resp.Body) + return err + } } return nil From 0817226f8e92246c37c247ab2756ec345550517b Mon Sep 17 00:00:00 2001 From: Chris Privitere <23177737+cprivitere@users.noreply.github.com> Date: Mon, 17 Jun 2024 12:53:49 -0500 Subject: [PATCH 09/10] chore(lint): fix lint errors Signed-off-by: Chris Privitere <23177737+cprivitere@users.noreply.github.com> --- internal/emlb/emlb.go | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/internal/emlb/emlb.go b/internal/emlb/emlb.go index 18ce5f23..367d8965 100644 --- a/internal/emlb/emlb.go +++ b/internal/emlb/emlb.go @@ -253,10 +253,9 @@ func (e *EMLB) DeleteLoadBalancer(ctx context.Context, clusterScope *scope.Clust if err != nil { if resp.StatusCode == http.StatusNotFound { return nil - } else { - log.Error(err, "LB Delete Failed", "EMLB ID", lbID, "Response Body", resp.Body) - return err } + log.Error(err, "LB Delete Failed", "EMLB ID", lbID, "Response Body", resp.Body) + return err } return nil @@ -281,10 +280,9 @@ func (e *EMLB) DeleteLoadBalancerOrigin(ctx context.Context, machineScope *scope if err != nil { if resp.StatusCode != http.StatusNotFound { return nil - } else { - log.Error(err, "LB Pool Delete Failed", "Pool ID", lbPoolID, "Response Body", resp.Body) - return err } + log.Error(err, "LB Pool Delete Failed", "Pool ID", lbPoolID, "Response Body", resp.Body) + return err } return nil @@ -306,14 +304,6 @@ func (e *EMLB) getLoadBalancerPort(ctx context.Context, id string, portNumber in return LoadBalancerPort, err } -// getLoadBalancerPool Returns a Load Balancer Pool object given an id. -func (e *EMLB) getLoadBalancerPool(ctx context.Context, id string) (*lbaas.LoadBalancerPool, *http.Response, error) { - ctx = context.WithValue(ctx, lbaas.ContextOAuth2, e.tokenExchanger) - - LoadBalancerPool, resp, err := e.client.PoolsApi.GetLoadBalancerPool(ctx, id).Execute() - return LoadBalancerPool, resp, err -} - // EnsureLoadBalancerOrigin takes the devices list of IP addresses in a Load Balancer Origin Pool and ensures an origin // for the first IPv4 address in the list exists. func (e *EMLB) ensureLoadBalancerOrigin(ctx context.Context, originID, poolID, lbName string, deviceAddr []corev1.NodeAddress) (*lbaas.LoadBalancerPoolOrigin, error) { From 53f1647e674c7467e5624625d1df034b87a47a34 Mon Sep 17 00:00:00 2001 From: Chris Privitere <23177737+cprivitere@users.noreply.github.com> Date: Mon, 17 Jun 2024 16:59:22 -0500 Subject: [PATCH 10/10] Apply suggestions from code review Co-authored-by: Marques Johansson --- internal/emlb/emlb.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/internal/emlb/emlb.go b/internal/emlb/emlb.go index 367d8965..3c1eccab 100644 --- a/internal/emlb/emlb.go +++ b/internal/emlb/emlb.go @@ -255,10 +255,9 @@ func (e *EMLB) DeleteLoadBalancer(ctx context.Context, clusterScope *scope.Clust return nil } log.Error(err, "LB Delete Failed", "EMLB ID", lbID, "Response Body", resp.Body) - return err } - return nil + return err } // DeleteLoadBalancerOrigin deletes the Equinix Metal Load Balancer associated with a given ClusterScope. @@ -282,10 +281,9 @@ func (e *EMLB) DeleteLoadBalancerOrigin(ctx context.Context, machineScope *scope return nil } log.Error(err, "LB Pool Delete Failed", "Pool ID", lbPoolID, "Response Body", resp.Body) - return err } - return nil + return err } // getLoadBalancer Returns a Load Balancer object given an id.