Deregister control plane VM from Kubernetes_API_Server LB rule on CloudStackMachine delete

Author: metbog

controllers/cloudstackmachine_controller.go

@@ -311,6 +311,45 @@ func (r *CloudStackMachineReconciliationRunner) AddToLBIfNeeded() (retRes ctrl.R
 	return ctrl.Result{}, nil
 }
 
+// RemoveFromLBIfNeeded removes the instance from the API server load balancer rule if it is a
+// control plane machine on a non-routed isolated network. Called from ReconcileDelete so the VM
+// is detached before being destroyed.
+//
+// The control-plane check uses the MachineControlPlaneLabel on the CloudStackMachine because the
+// CAPI Machine is not loaded on the delete path (see common stages in Reconcile).
+func (r *CloudStackMachineReconciliationRunner) RemoveFromLBIfNeeded() (retRes ctrl.Result, reterr error) {
+	if _, ok := r.ReconciliationSubject.Labels[clusterv1.MachineControlPlaneLabel]; !ok {
+		return ctrl.Result{}, nil
+	}
+	if r.FailureDomain.Spec.Zone.Network.Type != cloud.NetworkTypeIsolated {
+		return ctrl.Result{}, nil
+	}
+	if r.ReconciliationSubject.Spec.InstanceID == nil {
+		return ctrl.Result{}, nil
+	}
+
+	// IsoNet is not pre-loaded on the delete path; fetch it by the same meta name AddToLBIfNeeded relies on.
+	if res, err := r.GetObjectByName(
+		r.IsoNetMetaName(r.FailureDomain.Spec.Zone.Network.Name), r.IsoNet,
+	)(); r.ShouldReturn(res, err) {
+		return res, err
+	}
+	if r.IsoNet.Spec.Name == "" {
+		// Isolated network object already gone -- nothing to detach from.
+		return ctrl.Result{}, nil
+	}
+	if r.IsoNet.Status.RoutingMode != "" {
+		// Routed isolated networks do not use a load balancer.
+		return ctrl.Result{}, nil
+	}
+
+	r.Log.Info("Removing VM from load balancer rule.", "instance-id", *r.ReconciliationSubject.Spec.InstanceID)
+	if err := r.CSUser.RemoveVMFromLoadBalancerRule(r.IsoNet, *r.ReconciliationSubject.Spec.InstanceID); err != nil {
+		return ctrl.Result{}, err
+	}
+	return ctrl.Result{}, nil
+}
+
 // GetOrCreateMachineStateChecker creates or gets CloudStackMachineStateChecker object.
 func (r *CloudStackMachineReconciliationRunner) GetOrCreateMachineStateChecker() (retRes ctrl.Result, reterr error) {
 	checkerName := r.ReconciliationSubject.Spec.InstanceID
@@ -344,6 +383,14 @@ func (r *CloudStackMachineReconciliationRunner) ReconcileDelete() (retRes ctrl.R
 			return ctrl.Result{}, err
 		}
 	}
+
+	// For control plane machines on a non-routed isolated network, remove the VM from the
+	// API server load balancer rule before destroying the VM, so CloudStack does not keep
+	// routing API traffic to a VM that's about to be expunged.
+	if res, err := r.RemoveFromLBIfNeeded(); r.ShouldReturn(res, err) {
+		return res, err
+	}
+
 	r.Recorder.Eventf(r.ReconciliationSubject, "Normal", "Deleting", CSMachineDeletionMessage, r.ReconciliationSubject.Name)
 	r.Log.Info("Deleting instance", "instance-id", r.ReconciliationSubject.Spec.InstanceID)
 	// Use CSClient instead of CSUser here to expunge as admin.

docs/book/src/topics/cloudstack-permissions.md

@@ -34,6 +34,7 @@ The account that CAPC runs under must minimally be a User type account with a ro
 * listVolumes
 * listZones
 * queryAsyncJobResult
+* removeFromLoadBalancerRule
 * startVirtualMachine
 * stopVirtualMachine
 * updateVMAffinityGroup

pkg/cloud/isolated_network.go

@@ -35,6 +35,7 @@ type IsoNetworkIface interface {
 	ResolveLoadBalancerRuleDetails(*infrav1.CloudStackIsolatedNetwork) error
 
 	AssignVMToLoadBalancerRule(isoNet *infrav1.CloudStackIsolatedNetwork, instanceID string) error
+	RemoveVMFromLoadBalancerRule(isoNet *infrav1.CloudStackIsolatedNetwork, instanceID string) error
 	DeleteNetwork(infrav1.Network) error
 	DisposeIsoNetResources(*infrav1.CloudStackIsolatedNetwork, *infrav1.CloudStackCluster) error
 }
@@ -465,6 +466,39 @@ func (c *client) AssignVMToLoadBalancerRule(isoNet *infrav1.CloudStackIsolatedNe
 	return retErr
 }
 
+// RemoveVMFromLoadBalancerRule removes a VM instance from the load balancer rule referenced by
+// isoNet.Status.LBRuleID. The call is idempotent: if the rule is unknown or the VM is not currently
+// a member, it returns nil.
+func (c *client) RemoveVMFromLoadBalancerRule(isoNet *infrav1.CloudStackIsolatedNetwork, instanceID string) error {
+	if isoNet.Status.LBRuleID == "" {
+		return nil
+	}
+
+	lbRuleInstances, err := c.cs.LoadBalancer.ListLoadBalancerRuleInstances(
+		c.cs.LoadBalancer.NewListLoadBalancerRuleInstancesParams(isoNet.Status.LBRuleID))
+	if err != nil {
+		c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err)
+		return err
+	}
+
+	found := false
+	for _, instance := range lbRuleInstances.LoadBalancerRuleInstances {
+		if instance.Id == instanceID {
+			found = true
+			break
+		}
+	}
+	if !found {
+		return nil
+	}
+
+	p := c.cs.LoadBalancer.NewRemoveFromLoadBalancerRuleParams(isoNet.Status.LBRuleID)
+	p.SetVirtualmachineids([]string{instanceID})
+	_, err = c.cs.LoadBalancer.RemoveFromLoadBalancerRule(p)
+	c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err)
+	return err
+}
+
 // DeleteNetwork deletes an isolated network.
 func (c *client) DeleteNetwork(net infrav1.Network) error {
 	_, err := c.cs.Network.DeleteNetwork(c.cs.Network.NewDeleteNetworkParams(net.ID))

pkg/cloud/isolated_network_test.go

@@ -423,6 +423,66 @@ var _ = ginkgo.Describe("Network", func() {
 		})
 	})
 
+	ginkgo.Context("Remove VM from Load Balancer rule", func() {
+		ginkgo.It("no-op when LBRuleID is empty", func() {
+			dummies.CSISONet1.Status.LBRuleID = ""
+
+			gomega.Ω(client.RemoveVMFromLoadBalancerRule(dummies.CSISONet1, *dummies.CSMachine1.Spec.InstanceID)).Should(gomega.Succeed())
+		})
+
+		ginkgo.It("no-op when VM is not a member of the rule", func() {
+			dummies.CSISONet1.Status.LBRuleID = "lbruleid"
+			lbip := &csapi.ListLoadBalancerRuleInstancesParams{}
+			lbs.EXPECT().NewListLoadBalancerRuleInstancesParams(dummies.CSISONet1.Status.LBRuleID).Return(lbip)
+			lbs.EXPECT().ListLoadBalancerRuleInstances(lbip).Return(&csapi.ListLoadBalancerRuleInstancesResponse{}, nil)
+
+			gomega.Ω(client.RemoveVMFromLoadBalancerRule(dummies.CSISONet1, *dummies.CSMachine1.Spec.InstanceID)).Should(gomega.Succeed())
+		})
+
+		ginkgo.It("removes the VM when it is a member of the rule", func() {
+			dummies.CSISONet1.Status.LBRuleID = "lbruleid"
+			lbip := &csapi.ListLoadBalancerRuleInstancesParams{}
+			rfp := &csapi.RemoveFromLoadBalancerRuleParams{}
+			lbs.EXPECT().NewListLoadBalancerRuleInstancesParams(dummies.CSISONet1.Status.LBRuleID).Return(lbip)
+			lbs.EXPECT().ListLoadBalancerRuleInstances(lbip).Return(&csapi.ListLoadBalancerRuleInstancesResponse{
+				Count: 1,
+				LoadBalancerRuleInstances: []*csapi.VirtualMachine{{
+					Id: *dummies.CSMachine1.Spec.InstanceID,
+				}},
+			}, nil)
+			lbs.EXPECT().NewRemoveFromLoadBalancerRuleParams(dummies.CSISONet1.Status.LBRuleID).Return(rfp)
+			lbs.EXPECT().RemoveFromLoadBalancerRule(rfp).Return(&csapi.RemoveFromLoadBalancerRuleResponse{}, nil)
+
+			gomega.Ω(client.RemoveVMFromLoadBalancerRule(dummies.CSISONet1, *dummies.CSMachine1.Spec.InstanceID)).Should(gomega.Succeed())
+		})
+
+		ginkgo.It("returns an error when listing rule instances fails", func() {
+			dummies.CSISONet1.Status.LBRuleID = "lbruleid"
+			lbip := &csapi.ListLoadBalancerRuleInstancesParams{}
+			lbs.EXPECT().NewListLoadBalancerRuleInstancesParams(dummies.CSISONet1.Status.LBRuleID).Return(lbip)
+			lbs.EXPECT().ListLoadBalancerRuleInstances(lbip).Return(nil, fakeError)
+
+			gomega.Ω(client.RemoveVMFromLoadBalancerRule(dummies.CSISONet1, *dummies.CSMachine1.Spec.InstanceID)).ShouldNot(gomega.Succeed())
+		})
+
+		ginkgo.It("returns an error when CloudStack rejects the removal", func() {
+			dummies.CSISONet1.Status.LBRuleID = "lbruleid"
+			lbip := &csapi.ListLoadBalancerRuleInstancesParams{}
+			rfp := &csapi.RemoveFromLoadBalancerRuleParams{}
+			lbs.EXPECT().NewListLoadBalancerRuleInstancesParams(dummies.CSISONet1.Status.LBRuleID).Return(lbip)
+			lbs.EXPECT().ListLoadBalancerRuleInstances(lbip).Return(&csapi.ListLoadBalancerRuleInstancesResponse{
+				Count: 1,
+				LoadBalancerRuleInstances: []*csapi.VirtualMachine{{
+					Id: *dummies.CSMachine1.Spec.InstanceID,
+				}},
+			}, nil)
+			lbs.EXPECT().NewRemoveFromLoadBalancerRuleParams(dummies.CSISONet1.Status.LBRuleID).Return(rfp)
+			lbs.EXPECT().RemoveFromLoadBalancerRule(rfp).Return(nil, fakeError)
+
+			gomega.Ω(client.RemoveVMFromLoadBalancerRule(dummies.CSISONet1, *dummies.CSMachine1.Spec.InstanceID)).ShouldNot(gomega.Succeed())
+		})
+	})
+
 	ginkgo.Context("load balancer rule does not exist", func() {
 		ginkgo.It("calls cloudstack to create a new load balancer rule.", func() {
 			lbs.EXPECT().NewListLoadBalancerRulesParams().Return(&csapi.ListLoadBalancerRulesParams{})