fix webhook validation

Author: qJkee

api/v1alpha1/lvmcluster_test.go

@@ -496,38 +496,44 @@ var _ = Describe("webhook acceptance tests", func() {
 		Expect(k8sClient.Delete(ctx, resource)).To(Succeed())
 	})
 
-	It("device paths cannot be removed from device class in update", func(ctx SpecContext) {
+	It("device paths can be removed but at least one device must remain", func(ctx SpecContext) {
 		resource := defaultLVMClusterInUniqueNamespace(ctx)
-		resource.Spec.Storage.DeviceClasses[0].DeviceSelector = &DeviceSelector{Paths: []DevicePath{"/dev/newpath"}}
+		resource.Spec.Storage.DeviceClasses[0].DeviceSelector = &DeviceSelector{
+			Paths:         []DevicePath{"/dev/path1", "/dev/path2"},
+			OptionalPaths: []DevicePath{"/dev/optional1"},
+		}
 		Expect(k8sClient.Create(ctx, resource)).To(Succeed())
 
+		// Should succeed - removing some devices but keeping at least one
 		updated := resource.DeepCopy()
-		updated.Spec.Storage.DeviceClasses[0].DeviceSelector.Paths = []DevicePath{"/dev/otherpath"}
+		updated.Spec.Storage.DeviceClasses[0].DeviceSelector.Paths = []DevicePath{"/dev/path1"}
+		updated.Spec.Storage.DeviceClasses[0].DeviceSelector.OptionalPaths = []DevicePath{}
+		Expect(k8sClient.Update(ctx, updated)).To(Succeed())
 
-		err := k8sClient.Update(ctx, updated)
+		// Should fail - removing all devices
+		updated2 := resource.DeepCopy()
+		updated2.Spec.Storage.DeviceClasses[0].DeviceSelector.Paths = []DevicePath{}
+		updated2.Spec.Storage.DeviceClasses[0].DeviceSelector.OptionalPaths = []DevicePath{}
+		err := k8sClient.Update(ctx, updated2)
 		Expect(err).To(HaveOccurred())
 		Expect(err).To(Satisfy(k8serrors.IsForbidden))
-		statusError := &k8serrors.StatusError{}
-		Expect(errors.As(err, &statusError)).To(BeTrue())
-		Expect(statusError.Status().Message).To(ContainSubstring("required device paths were deleted from the LVMCluster"))
 
 		Expect(k8sClient.Delete(ctx, resource)).To(Succeed())
 	})
 
-	It("optional device paths cannot be removed from device class in update", func(ctx SpecContext) {
+	It("cannot add devices if none were initially specified", func(ctx SpecContext) {
 		resource := defaultLVMClusterInUniqueNamespace(ctx)
-		resource.Spec.Storage.DeviceClasses[0].DeviceSelector = &DeviceSelector{OptionalPaths: []DevicePath{"/dev/newpath"}}
+		resource.Spec.Storage.DeviceClasses[0].DeviceSelector = nil
 		Expect(k8sClient.Create(ctx, resource)).To(Succeed())
 
+		// Should fail - trying to add devices when none were initially specified
 		updated := resource.DeepCopy()
-		updated.Spec.Storage.DeviceClasses[0].DeviceSelector.OptionalPaths = []DevicePath{"/dev/otherpath"}
-
+		updated.Spec.Storage.DeviceClasses[0].DeviceSelector = &DeviceSelector{
+			Paths: []DevicePath{"/dev/newpath"},
+		}
 		err := k8sClient.Update(ctx, updated)
 		Expect(err).To(HaveOccurred())
 		Expect(err).To(Satisfy(k8serrors.IsForbidden))
-		statusError := &k8serrors.StatusError{}
-		Expect(errors.As(err, &statusError)).To(BeTrue())
-		Expect(statusError.Status().Message).To(ContainSubstring("optional device paths were deleted from the LVMCluster"))
 
 		Expect(k8sClient.Delete(ctx, resource)).To(Succeed())
 	})

api/v1alpha1/lvmcluster_webhook.go

@@ -250,17 +250,12 @@ func (v *lvmClusterValidator) ValidateUpdate(_ context.Context, old, new runtime
 			return warnings, ErrForceWipeOptionCannotBeChanged
 		}
 
-		// Make sure a device path list was not added
-		if len(oldDevices) == 0 && len(newDevices) > 0 {
-			return warnings, ErrDevicePathsCannotBeAddedInUpdate
-		}
-
-		// Make sure an optionalPaths list was not added
-		if len(oldOptionalDevices) == 0 && len(newOptionalDevices) > 0 {
-			return warnings, ErrDevicePathsCannotBeAddedInUpdate
-		}
-
-		if len(newOptionalDevices)+len(newDevices) == 0 {
+		// If originally no devices were specified, prevent adding any devices
+		if len(oldDevices) == 0 && len(oldOptionalDevices) == 0 {
+			if len(newDevices) > 0 || len(newOptionalDevices) > 0 {
+				return warnings, ErrDevicePathsCannotBeAddedInUpdate
+			}
+		} else if len(newDevices)+len(newOptionalDevices) == 0 {
 			return warnings, ErrDevicesCantBeEmpty
 		}
 
@@ -288,25 +283,6 @@ func validateDeviceClassesStillExist(old, new []DeviceClass) error {
 	return nil
 }
 
-func validateDevicePathsStillExist(old, new []DevicePath) error {
-	deviceMap := make(map[DevicePath]struct{})
-
-	for _, device := range old {
-		deviceMap[device] = struct{}{}
-	}
-
-	for _, device := range new {
-		delete(deviceMap, device)
-	}
-
-	// if any old device is removed now
-	if len(deviceMap) != 0 {
-		return fmt.Errorf("devices can not be removed from the LVMCluster once added oldDevices:%s, newDevices:%s", old, new)
-	}
-
-	return nil
-}
-
 // ValidateDelete implements webhook.Validator so a webhook will be registered for the type
 func (v *lvmClusterValidator) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
 	l := obj.(*LVMCluster)

bundle/manifests/lvms-operator.clusterserviceversion.yaml

@@ -615,6 +615,17 @@ spec:
           - create
           - patch
           - update
+        - apiGroups:
+          - ""
+          resources:
+          - configmaps
+          verbs:
+          - create
+          - get
+          - list
+          - patch
+          - update
+          - watch
         serviceAccountName: vg-manager
     strategy: deployment
   installModes:

catalog/lvms-operator/v0.0.1.yaml

@@ -11,5 +11,5 @@ generatorOptions:
 
 images:
 - name: controller
-  newName: quay.io/bzamalut/lvms
+  newName: quay.io/lvms_dev/lvms-operator
   newTag: latest

config/manager/kustomization.yaml

@@ -170,7 +170,10 @@ func (r *Reconciler) reconcile(
 	removedDevices, err := r.deleteRemovedDevices(ctx, volumeGroup, vgs, resolver)
 	if err != nil {
 		r.WarningEvent(ctx, volumeGroup, EventReasonErrorDeviceRemovalFailed, err)
-		r.setVolumeGroupFailedStatus(ctx, volumeGroup, vgs, FilteredBlockDevices{}, err)
+		_, errStatus := r.setVolumeGroupFailedStatus(ctx, volumeGroup, vgs, FilteredBlockDevices{}, err)
+		if errStatus != nil {
+			logger.Error(err, "failed to set status to failed")
+		}
 		return ctrl.Result{RequeueAfter: 5 * time.Second}, fmt.Errorf("failed to check if device was removed: %w", err)
 	}
 

internal/controllers/vgmanager/controller.go

@@ -38,10 +38,9 @@ type DeviceRemovalTransaction struct {
 }
 
 type deviceOperation struct {
-	devicePath   string
-	originalPath string // The user-provided path (symlink) corresponding to devicePath
-	phase        operationPhase
-	rollback     func(ctx context.Context) error
+	devicePath string
+	phase      operationPhase
+	rollback   func(ctx context.Context) error
 }
 
 type operationPhase int

internal/controllers/vgmanager/device_removal_transaction.go

@@ -50,4 +50,4 @@ func (r *Reconciler) validateDeviceRemoval(ctx context.Context, devicePath, vgNa
 
 	logger.V(1).Info("device validated for safe removal")
 	return nil
-}
+}