fix(vmip): add validation when updating the vmip (#1530)

- Fix VirtualMachineIP controller:
  - Added validation to ensure that an IP address is not already in use when updating the VirtualMachineIP address.

- Enhancements to Complex Test:
  - Enabled the previously skipped section of the test involving the patching of custom IP addresses.
  - Corrected the test case to ensure that all virtual machines are listed for power state checks, migration checks, and other verifications.
  - Added validations to ensure all virtual machines are correctly identified and included in checks.

Signed-off-by: Dmitry Lopatin <[email protected]>

Author: LopatinDmitr

images/virtualization-artifact/pkg/controller/vmip/internal/step/take_lease_step.go

@@ -78,7 +78,7 @@ func (s TakeLeaseStep) Take(ctx context.Context, vmip *v1alpha2.VirtualMachineIP
 		s.cb.
 			Status(metav1.ConditionFalse).
 			Reason(vmipcondition.VirtualMachineIPAddressLeaseNotReady).
-			Message(fmt.Sprintf("The VirtualMachineIPAddressLease %q alrady has a reference to another VirtualMachineIPAddress.", s.lease.Name))
+			Message(fmt.Sprintf("The VirtualMachineIPAddressLease %q already has a reference to another VirtualMachineIPAddress.", s.lease.Name))
 		return &reconcile.Result{}, nil
 	}
 

images/virtualization-artifact/pkg/controller/vmip/vmip_webhook.go

@@ -82,7 +82,7 @@ func (v *Validator) ValidateCreate(ctx context.Context, obj runtime.Object) (adm
 	return warnings, nil
 }
 
-func (v *Validator) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
+func (v *Validator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
 	oldVmip, ok := oldObj.(*v1alpha2.VirtualMachineIPAddress)
 	if !ok {
 		return nil, fmt.Errorf("expected an old VirtualMachineIP but got a %T", oldObj)
@@ -103,6 +103,20 @@ func (v *Validator) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Obj
 		return nil, fmt.Errorf("error validating VirtualMachineIP update: %w", err)
 	}
 
+	var warnings admission.Warnings
+
+	if newVmip.Spec.StaticIP != "" && oldVmip.Spec.StaticIP != newVmip.Spec.StaticIP {
+		err = v.validateAllocatedIPAddresses(ctx, newVmip.Spec.StaticIP)
+		switch {
+		case err == nil:
+			// OK.
+		case errors.Is(err, service.ErrIPAddressOutOfRange):
+			warnings = append(warnings, fmt.Sprintf("The requested address %s is out of the valid range", newVmip.Spec.StaticIP))
+		default:
+			return nil, err
+		}
+	}
+
 	boundCondition, _ := conditions.GetCondition(vmipcondition.BoundType, oldVmip.Status.Conditions)
 	if boundCondition.Status == metav1.ConditionTrue {
 		if oldVmip.Spec.Type == v1alpha2.VirtualMachineIPAddressTypeAuto && newVmip.Spec.Type == v1alpha2.VirtualMachineIPAddressTypeStatic {
@@ -123,7 +137,7 @@ func (v *Validator) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Obj
 		}
 	}
 
-	return nil, nil
+	return warnings, nil
 }
 
 func (v *Validator) ValidateDelete(_ context.Context, _ runtime.Object) (admission.Warnings, error) {

tests/e2e/complex_test.go

@@ -30,6 +30,8 @@ import (
 	kc "github.com/deckhouse/virtualization/tests/e2e/kubectl"
 )
 
+const VirtualMachineCount = 12
+
 var _ = Describe("ComplexTest", Serial, framework.CommonE2ETestDecorators(), func() {
 	var (
 		testCaseLabel            = map[string]string{"testcase": "complex-test"}
@@ -38,6 +40,7 @@ var _ = Describe("ComplexTest", Serial, framework.CommonE2ETestDecorators(), fun
 		notAlwaysOnLabel         = map[string]string{"notAlwaysOn": "complex-test"}
 		ns                       string
 		phaseByVolumeBindingMode = GetPhaseByVolumeBindingModeForTemplateSc()
+		f                        = framework.NewFramework("")
 	)
 
 	AfterEach(func() {
@@ -78,7 +81,10 @@ var _ = Describe("ComplexTest", Serial, framework.CommonE2ETestDecorators(), fun
 		})
 
 		It("should fill empty virtualMachineClassName with the default class name", func() {
-			defaultVMLabels := testCaseLabel
+			defaultVMLabels := make(map[string]string, len(testCaseLabel)+1)
+			for k, v := range testCaseLabel {
+				defaultVMLabels[k] = v
+			}
 			defaultVMLabels["vm"] = "default"
 			res := kubectl.List(kc.ResourceVM, kc.GetOptions{
 				Labels:    testCaseLabel,
@@ -125,13 +131,12 @@ var _ = Describe("ComplexTest", Serial, framework.CommonE2ETestDecorators(), fun
 	})
 
 	Context("When virtual machines IP addresses are applied", func() {
-		// TODO: fix: the custom IP address loses its lease and becomes Lost.
-		// It("patches custom VMIP with unassigned address", func() {
-		// 	vmipName := fmt.Sprintf("%s-%s", namePrefix, "vm-custom-ip")
-		// 	Eventually(func() error {
-		// 		return AssignIPToVMIP(f, ns, vmipName)
-		// 	}).WithTimeout(LongWaitDuration).WithPolling(Interval).Should(Succeed())
-		// })
+		It("patches custom VMIP with unassigned address", func() {
+			vmipName := fmt.Sprintf("%s-%s", namePrefix, "vm-custom-ip")
+			Eventually(func() error {
+				return AssignIPToVMIP(f, ns, vmipName)
+			}).WithTimeout(LongWaitDuration).WithPolling(Interval).Should(Succeed())
+		})
 
 		It("checks VMIPs phases", func() {
 			By(fmt.Sprintf("VMIPs should be in %s phases", PhaseAttached))
@@ -219,6 +224,7 @@ var _ = Describe("ComplexTest", Serial, framework.CommonE2ETestDecorators(), fun
 					Namespace: ns,
 				})
 				Expect(err).ShouldNot(HaveOccurred())
+				Expect(len(vmList.Items)).To(Equal(VirtualMachineCount))
 
 				for _, vmObj := range vmList.Items {
 					if vmObj.Spec.RunPolicy == v1alpha2.AlwaysOnPolicy {
@@ -278,6 +284,7 @@ var _ = Describe("ComplexTest", Serial, framework.CommonE2ETestDecorators(), fun
 					Labels:    testCaseLabel,
 				})
 				Expect(err).NotTo(HaveOccurred())
+				Expect(len(vms.Items)).To(Equal(VirtualMachineCount))
 
 				var notAlwaysOnVMs []string
 				for _, vm := range vms.Items {
@@ -313,6 +320,7 @@ var _ = Describe("ComplexTest", Serial, framework.CommonE2ETestDecorators(), fun
 					Namespace: ns,
 				})
 				Expect(err).ShouldNot(HaveOccurred())
+				Expect(len(vmList.Items)).To(Equal(VirtualMachineCount))
 
 				alwaysOnVMs = []string{}
 				notAlwaysOnVMs = []string{}

tests/e2e/testdata/complex-test/vm/kustomization.yaml

@@ -4,8 +4,7 @@ resources:
   - overlays/default
   - overlays/always-on
   - overlays/embedded-cloudinit
-  # TODO: the custom IP address loses its lease and becomes Lost.
-  # - overlays/custom-ip
+  - overlays/custom-ip
   - overlays/automatic
   - overlays/automatic-with-hotplug
   - overlays/hotplug