diff --git a/api/v1alpha1/lvmcluster_test.go b/api/v1alpha1/lvmcluster_test.go index f14c3e0ea..743336966 100644 --- a/api/v1alpha1/lvmcluster_test.go +++ b/api/v1alpha1/lvmcluster_test.go @@ -7,9 +7,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - "github.com/openshift/lvm-operator/pkg/cluster" - v1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -43,7 +41,7 @@ var _ = Describe("webhook acceptance tests", func() { }, } - It("minimum viable configuration", func(ctx SpecContext) { + defaultLVMClusterInUniqueNamespace := func(ctx SpecContext) *LVMCluster { generatedName := generateUniqueNameForTestCase(ctx) GinkgoT().Setenv(cluster.OperatorNamespaceEnvVar, generatedName) namespace := &v1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: generatedName}} @@ -51,11 +49,14 @@ var _ = Describe("webhook acceptance tests", func() { DeferCleanup(func(ctx SpecContext) { Expect(k8sClient.Delete(ctx, namespace)).To(Succeed()) }) - resource := defaultClusterTemplate.DeepCopy() resource.SetName(generatedName) resource.SetNamespace(namespace.GetName()) + return resource + } + It("minimum viable create", func(ctx SpecContext) { + resource := defaultLVMClusterInUniqueNamespace(ctx) Expect(k8sClient.Create(ctx, resource)).To(Succeed()) Expect(k8sClient.Delete(ctx, resource)).To(Succeed()) }) @@ -134,4 +135,270 @@ var _ = Describe("webhook acceptance tests", func() { Expect(statusError.Status().Message).To(ContainSubstring(ErrInvalidNamespace.Error())) }) + It("no device classes are not allowed", func(ctx SpecContext) { + resource := defaultLVMClusterInUniqueNamespace(ctx) + resource.Spec.Storage.DeviceClasses = nil + + err := k8sClient.Create(ctx, resource) + 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(ErrAtLeastOneDeviceClassRequired.Error())) + }) + + It("two default device classes are not allowed", func(ctx SpecContext) { + resource := defaultLVMClusterInUniqueNamespace(ctx) + resource.Spec.Storage.DeviceClasses = append(resource.Spec.Storage.DeviceClasses, DeviceClass{ + Name: "dupe", + Default: true, + ThinPoolConfig: resource.Spec.Storage.DeviceClasses[0].ThinPoolConfig.DeepCopy(), + }) + + err := k8sClient.Create(ctx, resource) + 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(ErrOnlyOneDefaultDeviceClassAllowed.Error())) + }) + + It("no default device classes is allowed but outputs a warning", func(ctx SpecContext) { + resource := defaultLVMClusterInUniqueNamespace(ctx) + resource.Spec.Storage.DeviceClasses[0].Default = false + Expect(k8sClient.Create(ctx, resource)).To(Succeed()) + Expect(k8sClient.Delete(ctx, resource)).To(Succeed()) + }) + + It("device selector without path is invalid", func(ctx SpecContext) { + resource := defaultLVMClusterInUniqueNamespace(ctx) + resource.Spec.Storage.DeviceClasses[0].DeviceSelector = &DeviceSelector{} + + err := k8sClient.Create(ctx, resource) + 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(ErrPathsOrOptionalPathsMandatoryWithNonNilDeviceSelector.Error())) + }) + + It("multiple device classes without path list are not allowed", func(ctx SpecContext) { + resource := defaultLVMClusterInUniqueNamespace(ctx) + resource.Spec.Storage.DeviceClasses = append(resource.Spec.Storage.DeviceClasses, DeviceClass{Name: "test", + ThinPoolConfig: resource.Spec.Storage.DeviceClasses[0].ThinPoolConfig.DeepCopy()}) + + err := k8sClient.Create(ctx, resource) + 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(ErrEmptyPathsWithMultipleDeviceClasses.Error())) + }) + + It("device selector with non-dev path is forbidden", func(ctx SpecContext) { + resource := defaultLVMClusterInUniqueNamespace(ctx) + resource.Spec.Storage.DeviceClasses[0].DeviceSelector = &DeviceSelector{Paths: []string{ + "some-random-path", + }} + + err := k8sClient.Create(ctx, resource) + 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("must be an absolute path to the device")) + }) + + It("device selector with non-dev optional path is forbidden", func(ctx SpecContext) { + resource := defaultLVMClusterInUniqueNamespace(ctx) + resource.Spec.Storage.DeviceClasses[0].DeviceSelector = &DeviceSelector{OptionalPaths: []string{ + "some-random-path", + }} + + err := k8sClient.Create(ctx, resource) + 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("must be an absolute path to the device")) + }) + + It("device selector with overlapping devices in paths is forbidden", func(ctx SpecContext) { + resource := defaultLVMClusterInUniqueNamespace(ctx) + resource.Spec.Storage.DeviceClasses[0].DeviceSelector = &DeviceSelector{Paths: []string{ + "/dev/test1", + }} + dupe := *resource.Spec.Storage.DeviceClasses[0].DeepCopy() + dupe.Name = "dupe" + dupe.Default = false + resource.Spec.Storage.DeviceClasses = append(resource.Spec.Storage.DeviceClasses, dupe) + + err := k8sClient.Create(ctx, resource) + 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("overlaps in two different deviceClasss")) + }) + + It("device selector with overlapping devices in optional paths is forbidden", func(ctx SpecContext) { + resource := defaultLVMClusterInUniqueNamespace(ctx) + resource.Spec.Storage.DeviceClasses[0].DeviceSelector = &DeviceSelector{OptionalPaths: []string{ + "/dev/test1", + }} + dupe := *resource.Spec.Storage.DeviceClasses[0].DeepCopy() + dupe.Name = "dupe" + dupe.Default = false + resource.Spec.Storage.DeviceClasses = append(resource.Spec.Storage.DeviceClasses, dupe) + + err := k8sClient.Create(ctx, resource) + 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("overlaps in two different deviceClasss")) + }) + + It("minimum viable update", func(ctx SpecContext) { + resource := defaultLVMClusterInUniqueNamespace(ctx) + Expect(k8sClient.Create(ctx, resource)).To(Succeed()) + + updated := resource.DeepCopy() + + Expect(k8sClient.Update(ctx, updated)).To(Succeed()) + + Expect(k8sClient.Delete(ctx, resource)).To(Succeed()) + }) + + It("updating ThinPoolConfig.Name is not allowed", func(ctx SpecContext) { + resource := defaultLVMClusterInUniqueNamespace(ctx) + Expect(k8sClient.Create(ctx, resource)).To(Succeed()) + + updated := resource.DeepCopy() + + updated.Spec.Storage.DeviceClasses[0].ThinPoolConfig.Name = "blub" + + 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(ErrThinPoolConfigCannotBeChanged.Error())) + + Expect(k8sClient.Delete(ctx, resource)).To(Succeed()) + }) + + It("updating ThinPoolConfig.SizePercent is not allowed", func(ctx SpecContext) { + resource := defaultLVMClusterInUniqueNamespace(ctx) + Expect(k8sClient.Create(ctx, resource)).To(Succeed()) + + updated := resource.DeepCopy() + + updated.Spec.Storage.DeviceClasses[0].ThinPoolConfig.SizePercent-- + + 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(ErrThinPoolConfigCannotBeChanged.Error())) + + Expect(k8sClient.Delete(ctx, resource)).To(Succeed()) + }) + + It("updating ThinPoolConfig.OverprovisionRatio is not allowed", func(ctx SpecContext) { + resource := defaultLVMClusterInUniqueNamespace(ctx) + Expect(k8sClient.Create(ctx, resource)).To(Succeed()) + + updated := resource.DeepCopy() + + updated.Spec.Storage.DeviceClasses[0].ThinPoolConfig.OverprovisionRatio-- + + 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(ErrThinPoolConfigCannotBeChanged.Error())) + + Expect(k8sClient.Delete(ctx, resource)).To(Succeed()) + }) + + It("device paths cannot be added to device class in update", func(ctx SpecContext) { + resource := defaultLVMClusterInUniqueNamespace(ctx) + Expect(k8sClient.Create(ctx, resource)).To(Succeed()) + + updated := resource.DeepCopy() + updated.Spec.Storage.DeviceClasses[0].DeviceSelector = &DeviceSelector{Paths: []string{"/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(ErrDevicePathsCannotBeAddedInUpdate.Error())) + + Expect(k8sClient.Delete(ctx, resource)).To(Succeed()) + }) + + It("optional device paths cannot be added to device class in update", func(ctx SpecContext) { + resource := defaultLVMClusterInUniqueNamespace(ctx) + Expect(k8sClient.Create(ctx, resource)).To(Succeed()) + + updated := resource.DeepCopy() + updated.Spec.Storage.DeviceClasses[0].DeviceSelector = &DeviceSelector{OptionalPaths: []string{"/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(ErrDevicePathsCannotBeAddedInUpdate.Error())) + + Expect(k8sClient.Delete(ctx, resource)).To(Succeed()) + }) + + It("device paths cannot be removed from device class in update", func(ctx SpecContext) { + resource := defaultLVMClusterInUniqueNamespace(ctx) + resource.Spec.Storage.DeviceClasses[0].DeviceSelector = &DeviceSelector{Paths: []string{"/dev/newpath"}} + Expect(k8sClient.Create(ctx, resource)).To(Succeed()) + + updated := resource.DeepCopy() + updated.Spec.Storage.DeviceClasses[0].DeviceSelector.Paths = []string{"/dev/otherpath"} + + 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("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) { + resource := defaultLVMClusterInUniqueNamespace(ctx) + resource.Spec.Storage.DeviceClasses[0].DeviceSelector = &DeviceSelector{OptionalPaths: []string{"/dev/newpath"}} + Expect(k8sClient.Create(ctx, resource)).To(Succeed()) + + updated := resource.DeepCopy() + updated.Spec.Storage.DeviceClasses[0].DeviceSelector.OptionalPaths = []string{"/dev/otherpath"} + + 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()) + }) }) diff --git a/api/v1alpha1/lvmcluster_webhook.go b/api/v1alpha1/lvmcluster_webhook.go index 928d4d6c5..d571d8ca8 100644 --- a/api/v1alpha1/lvmcluster_webhook.go +++ b/api/v1alpha1/lvmcluster_webhook.go @@ -23,7 +23,6 @@ import ( "strings" "github.com/openshift/lvm-operator/pkg/cluster" - "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -42,11 +41,16 @@ type lvmClusterValidator struct { var _ webhook.CustomValidator = &lvmClusterValidator{} var ( - ErrDeviceClassNotFound = errors.New("DeviceClass not found in the LVMCluster") - ErrThinPoolConfigNotSet = errors.New("ThinPoolConfig is not set for the DeviceClass") - ErrInvalidNamespace = errors.New("invalid namespace was supplied") - ErrNoValidLVMCluster = errors.New("object passed to lvmClusterValidator is not LVMCluster") - ErrDuplicateLVMCluster = errors.New("duplicate LVMClusters are not allowed, remove the old LVMCluster or work with the existing instance") + ErrDeviceClassNotFound = errors.New("DeviceClass not found in the LVMCluster") + ErrThinPoolConfigNotSet = errors.New("ThinPoolConfig is not set for the DeviceClass") + ErrInvalidNamespace = errors.New("invalid namespace was supplied") + ErrAtLeastOneDeviceClassRequired = errors.New("at least one deviceClass is required") + ErrOnlyOneDefaultDeviceClassAllowed = errors.New("only one default deviceClass is allowed") + ErrPathsOrOptionalPathsMandatoryWithNonNilDeviceSelector = errors.New("either paths or optionalPaths must be specified when DeviceSelector is specified") + ErrEmptyPathsWithMultipleDeviceClasses = errors.New("path list should not be empty when there are multiple deviceClasses") + ErrDuplicateLVMCluster = errors.New("duplicate LVMClusters are not allowed, remove the old LVMCluster or work with the existing instance") + ErrThinPoolConfigCannotBeChanged = errors.New("ThinPoolConfig can not be changed") + ErrDevicePathsCannotBeAddedInUpdate = errors.New("device paths can not be added after a device class has been initialized") ) //+kubebuilder:webhook:path=/validate-lvm-topolvm-io-v1alpha1-lvmcluster,mutating=false,failurePolicy=fail,sideEffects=None,groups=lvm.topolvm.io,resources=lvmclusters,verbs=create;update,versions=v1alpha1,name=vlvmcluster.kb.io,admissionReviewVersions=v1 @@ -60,10 +64,7 @@ func (l *LVMCluster) SetupWebhookWithManager(mgr ctrl.Manager) error { // ValidateCreate implements webhook.Validator so a webhook will be registered for the type func (v *lvmClusterValidator) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { - l, ok := obj.(*LVMCluster) - if !ok { - return nil, ErrNoValidLVMCluster - } + l := obj.(*LVMCluster) warnings := admission.Warnings{} lvmclusterlog.Info("validate create", "name", l.Name) @@ -115,11 +116,9 @@ func (v *lvmClusterValidator) ValidateCreate(ctx context.Context, obj runtime.Ob } // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type -func (v *lvmClusterValidator) ValidateUpdate(ctx context.Context, old, new runtime.Object) (admission.Warnings, error) { - l, ok := new.(*LVMCluster) - if !ok { - return nil, ErrNoValidLVMCluster - } +func (v *lvmClusterValidator) ValidateUpdate(_ context.Context, old, new runtime.Object) (admission.Warnings, error) { + l := new.(*LVMCluster) + lvmclusterlog.Info("validate update", "name", l.Name) warnings := admission.Warnings{} @@ -161,18 +160,18 @@ func (v *lvmClusterValidator) ValidateUpdate(ctx context.Context, old, new runti newThinPoolConfig = deviceClass.ThinPoolConfig oldThinPoolConfig, err = v.getThinPoolsConfigOfDeviceClass(oldLVMCluster, deviceClass.Name) - if (newThinPoolConfig != nil && oldThinPoolConfig == nil && err != ErrDeviceClassNotFound) || + if (newThinPoolConfig != nil && oldThinPoolConfig == nil && !errors.Is(err, ErrDeviceClassNotFound)) || (newThinPoolConfig == nil && oldThinPoolConfig != nil) { return warnings, fmt.Errorf("ThinPoolConfig can not be changed") } if newThinPoolConfig != nil && oldThinPoolConfig != nil { if newThinPoolConfig.Name != oldThinPoolConfig.Name { - return warnings, fmt.Errorf("ThinPoolConfig.Name can not be changed") + return warnings, fmt.Errorf("ThinPoolConfig.Name is invalid: %w", ErrThinPoolConfigCannotBeChanged) } else if newThinPoolConfig.SizePercent != oldThinPoolConfig.SizePercent { - return warnings, fmt.Errorf("ThinPoolConfig.SizePercent can not be changed") + return warnings, fmt.Errorf("ThinPoolConfig.SizePercent is invalid: %w", ErrThinPoolConfigCannotBeChanged) } else if newThinPoolConfig.OverprovisionRatio != oldThinPoolConfig.OverprovisionRatio { - return warnings, fmt.Errorf("ThinPoolConfig.OverprovisionRatio can not be changed") + return warnings, fmt.Errorf("ThinPoolConfig.OverprovisionRatio is invalid: %w", ErrThinPoolConfigCannotBeChanged) } } @@ -190,24 +189,24 @@ func (v *lvmClusterValidator) ValidateUpdate(ctx context.Context, old, new runti // Make sure a device path list was not added if len(oldDevices) == 0 && len(newDevices) > 0 { - return warnings, fmt.Errorf("invalid: device paths can not be added after a device class has been initialized") + return warnings, ErrDevicePathsCannotBeAddedInUpdate } // Make sure an optionalPaths list was not added if len(oldOptionalDevices) == 0 && len(newOptionalDevices) > 0 { - return warnings, fmt.Errorf("invalid: optional device paths can not be added after a device class has been initialized") + return warnings, ErrDevicePathsCannotBeAddedInUpdate } // Validate all the old paths still exist err := validateDevicePathsStillExist(oldDevices, newDevices) if err != nil { - return warnings, fmt.Errorf("invalid: required device paths were deleted from the LVMCluster: %v", err) + return warnings, fmt.Errorf("invalid: required device paths were deleted from the LVMCluster: %w", err) } // Validate all the old optional paths still exist err = validateDevicePathsStillExist(oldOptionalDevices, newOptionalDevices) if err != nil { - return warnings, fmt.Errorf("invalid: optional device paths were deleted from the LVMCluster: %v", err) + return warnings, fmt.Errorf("invalid: optional device paths were deleted from the LVMCluster: %w", err) } } @@ -235,10 +234,7 @@ func validateDevicePathsStillExist(old, new []string) error { // 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, ok := obj.(*LVMCluster) - if !ok { - return nil, ErrNoValidLVMCluster - } + l := obj.(*LVMCluster) lvmclusterlog.Info("validate delete", "name", l.Name) @@ -248,7 +244,7 @@ func (v *lvmClusterValidator) ValidateDelete(ctx context.Context, obj runtime.Ob func (v *lvmClusterValidator) verifyDeviceClass(l *LVMCluster) (admission.Warnings, error) { deviceClasses := l.Spec.Storage.DeviceClasses if len(deviceClasses) < 1 { - return nil, fmt.Errorf("at least one deviceClass is required") + return nil, ErrAtLeastOneDeviceClassRequired } countDefault := 0 for _, deviceClass := range deviceClasses { @@ -257,7 +253,7 @@ func (v *lvmClusterValidator) verifyDeviceClass(l *LVMCluster) (admission.Warnin } } if countDefault > 1 { - return nil, fmt.Errorf("only one default deviceClass is allowed. Currently, there are %d default deviceClasses", countDefault) + return nil, fmt.Errorf("%w. Currently, there are %d default deviceClasses", ErrOnlyOneDefaultDeviceClassAllowed, countDefault) } if countDefault == 0 { return admission.Warnings{"no default deviceClass was specified, it will be mandatory to specify the generated storage class in any PVC explicitly"}, nil @@ -272,14 +268,14 @@ func (v *lvmClusterValidator) verifyPathsAreNotEmpty(l *LVMCluster) error { for _, deviceClass := range l.Spec.Storage.DeviceClasses { if deviceClass.DeviceSelector != nil { if len(deviceClass.DeviceSelector.Paths) == 0 && len(deviceClass.DeviceSelector.OptionalPaths) == 0 { - return fmt.Errorf("either paths or optionalPaths must be specified when DeviceSelector is specified") + return ErrPathsOrOptionalPathsMandatoryWithNonNilDeviceSelector } } else { deviceClassesWithoutPaths = append(deviceClassesWithoutPaths, deviceClass.Name) } } if len(l.Spec.Storage.DeviceClasses) > 1 && len(deviceClassesWithoutPaths) > 0 { - return fmt.Errorf("path list should not be empty when there are multiple deviceClasses. Please specify device path(s) under deviceSelector.paths for %s deviceClass(es)", strings.Join(deviceClassesWithoutPaths, `,`)) + return fmt.Errorf("%w. Please specify device path(s) under deviceSelector.paths for %s deviceClass(es)", ErrEmptyPathsWithMultipleDeviceClasses, strings.Join(deviceClassesWithoutPaths, `,`)) } return nil