diff --git a/internal/webhook/apps/v1alpha1/nimservice_webhook.go b/internal/webhook/apps/v1alpha1/nimservice_webhook.go index 000b1e58b..10898b83b 100644 --- a/internal/webhook/apps/v1alpha1/nimservice_webhook.go +++ b/internal/webhook/apps/v1alpha1/nimservice_webhook.go @@ -95,13 +95,13 @@ func (v *NIMServiceCustomValidator) ValidateCreate(_ context.Context, obj runtim fldPath := field.NewPath("nimservice").Child("spec") // Perform comprehensive spec validation via helper. - errList := validateNIMServiceSpec(&nimservice.Spec, fldPath, v.k8sVersion) + warningList, errList := validateNIMServiceSpec(&nimservice.Spec, fldPath, v.k8sVersion) if len(errList) > 0 { - return nil, errList.ToAggregate() + return warningList, errList.ToAggregate() } - return nil, nil + return warningList, nil } // ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type NIMService. @@ -114,7 +114,7 @@ func (v *NIMServiceCustomValidator) ValidateUpdate(_ context.Context, oldObj, ne fldPath := field.NewPath("nimservice").Child("spec") // Start with structural validation to ensure the updated object is well formed. - errList := validateNIMServiceSpec(&nimservice.Spec, fldPath, v.k8sVersion) + warningList, errList := validateNIMServiceSpec(&nimservice.Spec, fldPath, v.k8sVersion) // All fields of NIMService.Spec are mutable, except for: // - Spec.MultiNode @@ -128,14 +128,19 @@ func (v *NIMServiceCustomValidator) ValidateUpdate(_ context.Context, oldObj, ne return nil, fmt.Errorf("expected a NIMService object for newObj but got %T", newObj) } - errList = append(errList, validateMultiNodeImmutability(oldNIMService, newNIMService, field.NewPath("spec").Child("multiNode"))...) - errList = append(errList, validatePVCImmutability(oldNIMService, newNIMService, field.NewPath("spec").Child("storage").Child("pvc"))...) + wList, eList := validateMultiNodeImmutability(oldNIMService, newNIMService, field.NewPath("spec").Child("multiNode")) + warningList = append(warningList, wList...) + errList = append(errList, eList...) + + wList, eList = validatePVCImmutability(oldNIMService, newNIMService, field.NewPath("spec").Child("storage").Child("pvc")) + warningList = append(warningList, wList...) + errList = append(errList, eList...) if len(errList) > 0 { - return nil, errList.ToAggregate() + return warningList, errList.ToAggregate() } - return nil, nil + return warningList, nil } func (v *NIMServiceCustomValidator) ValidateDelete(_ context.Context, obj runtime.Object) (admission.Warnings, error) { diff --git a/internal/webhook/apps/v1alpha1/nimservice_webhook_validation_helper.go b/internal/webhook/apps/v1alpha1/nimservice_webhook_validation_helper.go index 4889cf98e..49a43ffb4 100644 --- a/internal/webhook/apps/v1alpha1/nimservice_webhook_validation_helper.go +++ b/internal/webhook/apps/v1alpha1/nimservice_webhook_validation_helper.go @@ -24,6 +24,7 @@ import ( networkingv1 "k8s.io/api/networking/v1" "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/util/validation/field" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" appsv1alpha1 "github.com/NVIDIA/k8s-nim-operator/api/apps/v1alpha1" "github.com/NVIDIA/k8s-nim-operator/internal/utils" @@ -36,7 +37,8 @@ var validPVCAccessModeStrs = []corev1.PersistentVolumeAccessMode{ corev1.ReadWriteOncePod, } -func validateImageConfiguration(image *appsv1alpha1.Image, fldPath *field.Path) field.ErrorList { +func validateImageConfiguration(image *appsv1alpha1.Image, fldPath *field.Path) (admission.Warnings, field.ErrorList) { + warningList := admission.Warnings{} errList := field.ErrorList{} if image.Repository == "" { errList = append(errList, field.Required(fldPath.Child("repository"), "is required")) @@ -44,10 +46,11 @@ func validateImageConfiguration(image *appsv1alpha1.Image, fldPath *field.Path) if image.Tag == "" { errList = append(errList, field.Required(fldPath.Child("tag"), "is required")) } - return errList + return warningList, errList } -func validateServiceStorageConfiguration(storage *appsv1alpha1.NIMServiceStorage, fldPath *field.Path) field.ErrorList { +func validateServiceStorageConfiguration(storage *appsv1alpha1.NIMServiceStorage, fldPath *field.Path) (admission.Warnings, field.ErrorList) { + warningList := admission.Warnings{} errList := field.ErrorList{} // If size limit is defined, it must be greater than 0 if storage.SharedMemorySizeLimit != nil { @@ -98,10 +101,11 @@ func validateServiceStorageConfiguration(storage *appsv1alpha1.NIMServiceStorage errList = append(errList, validatePVCConfiguration(&storage.PVC, fldPath.Child("pvc"))...) } - return errList + return warningList, errList } -func validateDRAResourcesConfiguration(spec *appsv1alpha1.NIMServiceSpec, fldPath *field.Path, k8sVersion string) field.ErrorList { +func validateDRAResourcesConfiguration(spec *appsv1alpha1.NIMServiceSpec, fldPath *field.Path, k8sVersion string) (admission.Warnings, field.ErrorList) { + warningList := admission.Warnings{} errList := field.ErrorList{} draResourcesPath := fldPath.Child("draResources") @@ -151,19 +155,21 @@ func validateDRAResourcesConfiguration(spec *appsv1alpha1.NIMServiceSpec, fldPat } } } - return errList + return warningList, errList } -func validateAuthSecret(authSecret *string, fldPath *field.Path) field.ErrorList { +func validateAuthSecret(authSecret *string, fldPath *field.Path) (admission.Warnings, field.ErrorList) { + warningList := admission.Warnings{} errList := field.ErrorList{} if *authSecret == "" { errList = append(errList, field.Required(fldPath, "is required")) } - return errList + return warningList, errList } // If Spec.Expose.Ingress.Enabled is true, Spec.Expose.Ingress.Spec must be non-nil. -func validateExposeConfiguration(expose *appsv1alpha1.Expose, fldPath *field.Path) field.ErrorList { +func validateExposeConfiguration(expose *appsv1alpha1.Expose, fldPath *field.Path) (admission.Warnings, field.ErrorList) { + warningList := admission.Warnings{} errList := field.ErrorList{} if expose.Ingress.Enabled != nil && *expose.Ingress.Enabled { if reflect.DeepEqual(expose.Ingress.Spec, networkingv1.IngressSpec{}) { @@ -176,73 +182,102 @@ func validateExposeConfiguration(expose *appsv1alpha1.Expose, fldPath *field.Pat errList = append(errList, field.Required(fldPath.Child("spec"), fmt.Sprintf("must be defined if %s is true", fldPath.Child("enabled")))) } } - return errList + return warningList, errList } // If Spec.Metrics.Enabled is true, Spec.Metrics.ServiceMonitor must not be empty. -func validateMetricsConfiguration(metrics *appsv1alpha1.Metrics, fldPath *field.Path) field.ErrorList { +func validateMetricsConfiguration(metrics *appsv1alpha1.Metrics, fldPath *field.Path) (admission.Warnings, field.ErrorList) { + warningList := admission.Warnings{} errList := field.ErrorList{} if metrics.Enabled != nil && *metrics.Enabled { if reflect.DeepEqual(metrics.ServiceMonitor, appsv1alpha1.ServiceMonitor{}) { errList = append(errList, field.Required(fldPath.Child("serviceMonitor"), fmt.Sprintf("must be defined if %s is true", fldPath.Child("enabled")))) } } - return errList + return warningList, errList } // If Spec.Scale.Enabled is true, Spec.Scale.HPA must be non-empty HorizontalPodAutoScaler. -func validateScaleConfiguration(scale *appsv1alpha1.Autoscaling, fldPath *field.Path) field.ErrorList { +func validateScaleConfiguration(scale *appsv1alpha1.Autoscaling, fldPath *field.Path) (admission.Warnings, field.ErrorList) { + warningList := admission.Warnings{} errList := field.ErrorList{} if scale.Enabled != nil && *scale.Enabled { if reflect.DeepEqual(scale.HPA, appsv1alpha1.HorizontalPodAutoscalerSpec{}) { errList = append(errList, field.Required(fldPath.Child("hpa"), fmt.Sprintf("must be defined if %s is true", fldPath.Child("enabled")))) } } - return errList + return warningList, errList } // Spec.Resources.Claims must be empty. -func validateResourcesConfiguration(resources *corev1.ResourceRequirements, fldPath *field.Path) field.ErrorList { +func validateResourcesConfiguration(resources *corev1.ResourceRequirements, fldPath *field.Path) (admission.Warnings, field.ErrorList) { + warningList := admission.Warnings{} errList := field.ErrorList{} if resources != nil { if resources.Claims != nil || len(resources.Claims) != 0 { errList = append(errList, field.Forbidden(fldPath.Child("claims"), "must be empty")) } } - return errList + return warningList, errList } // validateNIMServiceSpec aggregates all structural validation checks for a NIMService // object. It is intended to be invoked by both ValidateCreate and ValidateUpdate to // ensure the resource is well-formed before any other validation (e.g. immutability) // is performed. -func validateNIMServiceSpec(spec *appsv1alpha1.NIMServiceSpec, fldPath *field.Path, kubeVersion string) field.ErrorList { +func validateNIMServiceSpec(spec *appsv1alpha1.NIMServiceSpec, fldPath *field.Path, kubeVersion string) (admission.Warnings, field.ErrorList) { errList := field.ErrorList{} + warningList := admission.Warnings{} - // Validate individual sections of the spec using existing helper functions. - errList = append(errList, validateImageConfiguration(&spec.Image, fldPath.Child("image"))...) - errList = append(errList, validateAuthSecret(&spec.AuthSecret, fldPath.Child("authSecret"))...) - errList = append(errList, validateServiceStorageConfiguration(&spec.Storage, fldPath.Child("storage"))...) - errList = append(errList, validateExposeConfiguration(&spec.Expose, fldPath.Child("expose").Child("ingress"))...) - errList = append(errList, validateMetricsConfiguration(&spec.Metrics, fldPath.Child("metrics"))...) - errList = append(errList, validateScaleConfiguration(&spec.Scale, fldPath.Child("scale"))...) - errList = append(errList, validateResourcesConfiguration(spec.Resources, fldPath.Child("resources"))...) - errList = append(errList, validateDRAResourcesConfiguration(spec, fldPath, kubeVersion)...) - - return errList + // TODO abstract all validation functions with a single signature validateFunc(*appsv1alpha1.NIMServiceSpec, *field.Path) (admission.Warnings, field.ErrorList) + w, err := validateImageConfiguration(&spec.Image, fldPath.Child("image")) + warningList = append(warningList, w...) + errList = append(errList, err...) + + w, err = validateAuthSecret(&spec.AuthSecret, fldPath.Child("authSecret")) + warningList = append(warningList, w...) + errList = append(errList, err...) + + w, err = validateServiceStorageConfiguration(&spec.Storage, fldPath.Child("storage")) + warningList = append(warningList, w...) + errList = append(errList, err...) + + w, err = validateExposeConfiguration(&spec.Expose, fldPath.Child("expose").Child("ingress")) + warningList = append(warningList, w...) + errList = append(errList, err...) + + w, err = validateMetricsConfiguration(&spec.Metrics, fldPath.Child("metrics")) + warningList = append(warningList, w...) + errList = append(errList, err...) + + w, err = validateScaleConfiguration(&spec.Scale, fldPath.Child("scale")) + warningList = append(warningList, w...) + errList = append(errList, err...) + + w, err = validateResourcesConfiguration(spec.Resources, fldPath.Child("resources")) + warningList = append(warningList, w...) + errList = append(errList, err...) + + w, err = validateDRAResourcesConfiguration(spec, fldPath, kubeVersion) + warningList = append(warningList, w...) + errList = append(errList, err...) + + return warningList, errList } // validateMultiNodeImmutability ensures that the MultiNode field remains unchanged after creation. -func validateMultiNodeImmutability(oldNs, newNs *appsv1alpha1.NIMService, fldPath *field.Path) field.ErrorList { +func validateMultiNodeImmutability(oldNs, newNs *appsv1alpha1.NIMService, fldPath *field.Path) (admission.Warnings, field.ErrorList) { + warningList := admission.Warnings{} errList := field.ErrorList{} if !equality.Semantic.DeepEqual(oldNs.Spec.MultiNode, newNs.Spec.MultiNode) { errList = append(errList, field.Forbidden(fldPath, "is immutable once the resource is created")) } - return errList + return warningList, errList } // validatePVCImmutability verifies that once a PVC is created with create: true, no further modifications are allowed. -func validatePVCImmutability(oldNs, newNs *appsv1alpha1.NIMService, fldPath *field.Path) field.ErrorList { +func validatePVCImmutability(oldNs, newNs *appsv1alpha1.NIMService, fldPath *field.Path) (admission.Warnings, field.ErrorList) { + warningList := admission.Warnings{} errList := field.ErrorList{} oldPVC := oldNs.Spec.Storage.PVC newPVC := newNs.Spec.Storage.PVC @@ -251,5 +286,5 @@ func validatePVCImmutability(oldNs, newNs *appsv1alpha1.NIMService, fldPath *fie errList = append(errList, field.Forbidden(fldPath, fmt.Sprintf("is immutable once it is created with %s = true", fldPath.Child("create")))) } } - return errList + return warningList, errList } diff --git a/internal/webhook/apps/v1alpha1/nimservice_webhook_validation_helper_test.go b/internal/webhook/apps/v1alpha1/nimservice_webhook_validation_helper_test.go index 8188f1217..d1b677835 100644 --- a/internal/webhook/apps/v1alpha1/nimservice_webhook_validation_helper_test.go +++ b/internal/webhook/apps/v1alpha1/nimservice_webhook_validation_helper_test.go @@ -32,41 +32,45 @@ func TestValidateImageConfiguration(t *testing.T) { fldPath := field.NewPath("spec").Child("image") tests := []struct { - name string - image *appsv1alpha1.Image - wantErrs int + name string + image *appsv1alpha1.Image + wantErrs int + wantWarnings int }{ { - name: "valid image", - image: &appsv1alpha1.Image{Repository: "repo", Tag: "latest"}, - wantErrs: 0, + name: "valid image", + image: &appsv1alpha1.Image{Repository: "repo", Tag: "latest"}, + wantErrs: 0, + wantWarnings: 0, }, { - name: "missing repository", - image: &appsv1alpha1.Image{Tag: "v1"}, - wantErrs: 1, + name: "missing repository", + image: &appsv1alpha1.Image{Tag: "v1"}, + wantErrs: 1, + wantWarnings: 0, }, { - name: "missing tag", - image: &appsv1alpha1.Image{Repository: "repo"}, - wantErrs: 1, + name: "missing tag", + image: &appsv1alpha1.Image{Repository: "repo"}, + wantErrs: 1, + wantWarnings: 0, }, { - name: "missing both", - image: &appsv1alpha1.Image{}, - wantErrs: 2, + name: "missing both", + image: &appsv1alpha1.Image{}, + wantErrs: 2, + wantWarnings: 0, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - errs := validateImageConfiguration(tc.image, fldPath) - if got := len(errs); got != tc.wantErrs { + w, errs := validateImageConfiguration(tc.image, fldPath) + gotErrs := len(errs) + gotWarnings := len(w) + if gotErrs != tc.wantErrs || gotWarnings != tc.wantWarnings { t.Logf("Validation errors:") - for i, err := range errs { - t.Logf(" %d: %s", i+1, err.Error()) - } - t.Fatalf("got %d errs, want %d", got, tc.wantErrs) + t.Fatalf("got %d errs, %d warnings, want %d errs, %d warnings", gotErrs, gotWarnings, tc.wantErrs, tc.wantWarnings) } }) } @@ -91,14 +95,16 @@ func TestValidateServiceStorageConfiguration(t *testing.T) { sizeNeg := resource.MustParse("0") tests := []struct { - name string - modify func(*appsv1alpha1.NIMService) - wantErrs int + name string + modify func(*appsv1alpha1.NIMService) + wantErrs int + wantWarnings int }{ { - name: "missing both nimCache and pvc", - modify: func(ns *appsv1alpha1.NIMService) {}, - wantErrs: 1, + name: "missing both nimCache and pvc", + modify: func(ns *appsv1alpha1.NIMService) {}, + wantErrs: 1, + wantWarnings: 0, }, { name: "both nimCache and pvc defined", @@ -106,14 +112,16 @@ func TestValidateServiceStorageConfiguration(t *testing.T) { ns.Spec.Storage.NIMCache = appsv1alpha1.NIMCacheVolSpec{Name: "cache", Profile: "p"} ns.Spec.Storage.PVC = appsv1alpha1.PersistentVolumeClaim{Name: "pvc"} }, - wantErrs: 1, + wantErrs: 1, + wantWarnings: 0, }, { name: "nimCache missing name", modify: func(ns *appsv1alpha1.NIMService) { ns.Spec.Storage.NIMCache = appsv1alpha1.NIMCacheVolSpec{Profile: "p"} }, - wantErrs: 1, + wantErrs: 1, + wantWarnings: 0, }, { name: "pvc create false name empty", @@ -122,7 +130,8 @@ func TestValidateServiceStorageConfiguration(t *testing.T) { Create: &falseVal, } }, - wantErrs: 1, + wantErrs: 1, + wantWarnings: 0, }, { name: "pvc invalid volumeaccessmode", @@ -132,7 +141,8 @@ func TestValidateServiceStorageConfiguration(t *testing.T) { VolumeAccessMode: "BadMode", } }, - wantErrs: 1, + wantErrs: 1, + wantWarnings: 0, }, { name: "shared memory size <=0", @@ -140,14 +150,16 @@ func TestValidateServiceStorageConfiguration(t *testing.T) { ns.Spec.Storage.PVC = appsv1alpha1.PersistentVolumeClaim{Name: "pvc"} ns.Spec.Storage.SharedMemorySizeLimit = &sizeNeg }, - wantErrs: 1, + wantErrs: 1, + wantWarnings: 0, }, { name: "valid nimCache", modify: func(ns *appsv1alpha1.NIMService) { ns.Spec.Storage.NIMCache = appsv1alpha1.NIMCacheVolSpec{Name: "cache", Profile: "default"} }, - wantErrs: 0, + wantErrs: 0, + wantWarnings: 0, }, { name: "valid pvc", @@ -160,7 +172,8 @@ func TestValidateServiceStorageConfiguration(t *testing.T) { VolumeAccessMode: corev1.ReadWriteOnce, } }, - wantErrs: 0, + wantErrs: 0, + wantWarnings: 0, }, } @@ -168,13 +181,12 @@ func TestValidateServiceStorageConfiguration(t *testing.T) { t.Run(tc.name, func(t *testing.T) { ns := baseNIMService() tc.modify(ns) - errs := validateServiceStorageConfiguration(&ns.Spec.Storage, fldPath) - if got := len(errs); got != tc.wantErrs { + w, errs := validateServiceStorageConfiguration(&ns.Spec.Storage, fldPath) + gotErrs := len(errs) + gotWarnings := len(w) + if gotErrs != tc.wantErrs || gotWarnings != tc.wantWarnings { t.Logf("Validation errors:") - for i, err := range errs { - t.Logf(" %d: %s", i+1, err.Error()) - } - t.Fatalf("got %d errs, want %d", got, tc.wantErrs) + t.Fatalf("got %d errs, %d warnings, want %d errs, %d warnings", gotErrs, gotWarnings, tc.wantErrs, tc.wantWarnings) } }) } @@ -186,16 +198,18 @@ func TestValidateDRAResourcesConfiguration(t *testing.T) { fld := field.NewPath("spec") cases := []struct { - name string - modify func(*appsv1alpha1.NIMService) - k8sVersion string - wantErrs int + name string + modify func(*appsv1alpha1.NIMService) + k8sVersion string + wantErrs int + wantWarnings int }{ { - name: "no dra resources", - modify: func(ns *appsv1alpha1.NIMService) {}, - k8sVersion: "v1.34.0", - wantErrs: 0, + name: "no dra resources", + modify: func(ns *appsv1alpha1.NIMService) {}, + k8sVersion: "v1.34.0", + wantErrs: 0, + wantWarnings: 0, }, { name: "unsupported k8s version", @@ -204,8 +218,9 @@ func TestValidateDRAResourcesConfiguration(t *testing.T) { ResourceClaimTemplateName: ptr.To("tmpl1"), }} }, - k8sVersion: "v1.32.0", // below MinSupportedClusterVersionForDRA - wantErrs: 1, + k8sVersion: "v1.32.0", // below MinSupportedClusterVersionForDRA + wantErrs: 1, + wantWarnings: 0, }, { name: "both name and template provided", @@ -215,16 +230,18 @@ func TestValidateDRAResourcesConfiguration(t *testing.T) { ResourceClaimTemplateName: ptr.To("tmpl1"), }} }, - k8sVersion: "v1.34.0", - wantErrs: 1, + k8sVersion: "v1.34.0", + wantErrs: 1, + wantWarnings: 0, }, { name: "neither name nor template provided", modify: func(ns *appsv1alpha1.NIMService) { ns.Spec.DRAResources = []appsv1alpha1.DRAResource{{}} }, - k8sVersion: "v1.34.0", - wantErrs: 1, + k8sVersion: "v1.34.0", + wantErrs: 1, + wantWarnings: 0, }, { name: "resourceClaimName with replicas>1", @@ -234,8 +251,9 @@ func TestValidateDRAResourcesConfiguration(t *testing.T) { ResourceClaimName: ptr.To("claim1"), }} }, - k8sVersion: "v1.34.0", - wantErrs: 1, + k8sVersion: "v1.34.0", + wantErrs: 1, + wantWarnings: 0, }, { name: "resourceClaimName with autoscaling enabled", @@ -246,8 +264,9 @@ func TestValidateDRAResourcesConfiguration(t *testing.T) { ResourceClaimName: ptr.To("claim1"), }} }, - k8sVersion: "v1.34.0", - wantErrs: 1, + k8sVersion: "v1.34.0", + wantErrs: 1, + wantWarnings: 0, }, { name: "duplicate resourceClaimNames", @@ -257,8 +276,9 @@ func TestValidateDRAResourcesConfiguration(t *testing.T) { {ResourceClaimName: ptr.To("dup")}, } }, - k8sVersion: "v1.34.0", - wantErrs: 1, + k8sVersion: "v1.34.0", + wantErrs: 1, + wantWarnings: 0, }, { name: "valid template", @@ -267,8 +287,9 @@ func TestValidateDRAResourcesConfiguration(t *testing.T) { ResourceClaimTemplateName: ptr.To("tmpl1"), }} }, - k8sVersion: "v1.34.0", - wantErrs: 0, + k8sVersion: "v1.34.0", + wantErrs: 0, + wantWarnings: 0, }, { name: "valid multiple templates", @@ -279,8 +300,9 @@ func TestValidateDRAResourcesConfiguration(t *testing.T) { ResourceClaimTemplateName: ptr.To("tmpl2"), }} }, - k8sVersion: "v1.34.0", - wantErrs: 0, + k8sVersion: "v1.34.0", + wantErrs: 0, + wantWarnings: 0, }, } @@ -288,13 +310,12 @@ func TestValidateDRAResourcesConfiguration(t *testing.T) { t.Run(tc.name, func(t *testing.T) { ns := baseNIMService() tc.modify(ns) - errs := validateDRAResourcesConfiguration(&ns.Spec, fld, tc.k8sVersion) - if got := len(errs); got != tc.wantErrs { + w, errs := validateDRAResourcesConfiguration(&ns.Spec, fld, tc.k8sVersion) + gotErrs := len(errs) + gotWarnings := len(w) + if gotErrs != tc.wantErrs || gotWarnings != tc.wantWarnings { t.Logf("Validation errors:") - for i, err := range errs { - t.Logf(" %d: %s", i+1, err.Error()) - } - t.Fatalf("got %d errs, want %d", got, tc.wantErrs) + t.Fatalf("got %d errs, %d warnings, want %d errs, %d warnings", gotErrs, gotWarnings, tc.wantErrs, tc.wantWarnings) } }) } @@ -304,22 +325,22 @@ func TestValidateDRAResourcesConfiguration(t *testing.T) { func TestValidateAuthSecret(t *testing.T) { fld := field.NewPath("spec").Child("authSecret") cases := []struct { - name string - value string - wantErrs int + name string + value string + wantErrs int + wantWarnings int }{ - {"non-empty", "secret", 0}, - {"empty", "", 1}, + {"non-empty", "secret", 0, 0}, + {"empty", "", 1, 0}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - errs := validateAuthSecret(&tc.value, fld) - if got := len(errs); got != tc.wantErrs { + w, errs := validateAuthSecret(&tc.value, fld) + gotErrs := len(errs) + gotWarnings := len(w) + if gotErrs != tc.wantErrs || gotWarnings != tc.wantWarnings { t.Logf("Validation errors:") - for i, err := range errs { - t.Logf(" %d: %s", i+1, err.Error()) - } - t.Fatalf("got %d errs, want %d", got, tc.wantErrs) + t.Fatalf("got %d errs, %d warnings, want %d errs, %d warnings", gotErrs, gotWarnings, tc.wantErrs, tc.wantWarnings) } }) } @@ -332,21 +353,24 @@ func TestValidateExposeIngressConfiguration(t *testing.T) { class := "nginx" cases := []struct { - name string - modify func(*appsv1alpha1.NIMService) - wantErrs int + name string + modify func(*appsv1alpha1.NIMService) + wantErrs int + wantWarnings int }{ { - name: "ingress disabled", - modify: func(ns *appsv1alpha1.NIMService) {}, - wantErrs: 0, + name: "ingress disabled", + modify: func(ns *appsv1alpha1.NIMService) {}, + wantErrs: 0, + wantWarnings: 0, }, { name: "enabled empty spec", modify: func(ns *appsv1alpha1.NIMService) { ns.Spec.Expose.Ingress.Enabled = &enabled }, - wantErrs: 1, + wantErrs: 1, + wantWarnings: 0, }, { name: "enabled with spec", @@ -354,20 +378,20 @@ func TestValidateExposeIngressConfiguration(t *testing.T) { ns.Spec.Expose.Ingress.Enabled = &enabled ns.Spec.Expose.Ingress.Spec.IngressClassName = &class }, - wantErrs: 0, + wantErrs: 0, + wantWarnings: 0, }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { ns := baseNIMService() tc.modify(ns) - errs := validateExposeConfiguration(&ns.Spec.Expose, fld) - if got := len(errs); got != tc.wantErrs { + w, errs := validateExposeConfiguration(&ns.Spec.Expose, fld) + gotErrs := len(errs) + gotWarnings := len(w) + if gotErrs != tc.wantErrs || gotWarnings != tc.wantWarnings { t.Logf("Validation errors:") - for i, err := range errs { - t.Logf(" %d: %s", i+1, err.Error()) - } - t.Fatalf("got %d errs, want %d", got, tc.wantErrs) + t.Fatalf("got %d errs, %d warnings, want %d errs, %d warnings", gotErrs, gotWarnings, tc.wantErrs, tc.wantWarnings) } }) } @@ -379,21 +403,24 @@ func TestValidateMetricsConfiguration(t *testing.T) { enabled := true cases := []struct { - name string - modify func(*appsv1alpha1.NIMService) - wantErrs int + name string + modify func(*appsv1alpha1.NIMService) + wantErrs int + wantWarnings int }{ { - name: "metrics disabled", - modify: func(ns *appsv1alpha1.NIMService) {}, - wantErrs: 0, + name: "metrics disabled", + modify: func(ns *appsv1alpha1.NIMService) {}, + wantErrs: 0, + wantWarnings: 0, }, { name: "enabled empty monitor", modify: func(ns *appsv1alpha1.NIMService) { ns.Spec.Metrics.Enabled = &enabled }, - wantErrs: 1, + wantErrs: 1, + wantWarnings: 0, }, { name: "enabled valid", @@ -401,20 +428,20 @@ func TestValidateMetricsConfiguration(t *testing.T) { ns.Spec.Metrics.Enabled = &enabled ns.Spec.Metrics.ServiceMonitor.Interval = "30s" }, - wantErrs: 0, + wantErrs: 0, + wantWarnings: 0, }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { ns := baseNIMService() tc.modify(ns) - errs := validateMetricsConfiguration(&ns.Spec.Metrics, fld) - if got := len(errs); got != tc.wantErrs { + w, errs := validateMetricsConfiguration(&ns.Spec.Metrics, fld) + gotErrs := len(errs) + gotWarnings := len(w) + if gotErrs != tc.wantErrs || gotWarnings != tc.wantWarnings { t.Logf("Validation errors:") - for i, err := range errs { - t.Logf(" %d: %s", i+1, err.Error()) - } - t.Fatalf("got %d errs, want %d", got, tc.wantErrs) + t.Fatalf("got %d errs, %d warnings, want %d errs, %d warnings", gotErrs, gotWarnings, tc.wantErrs, tc.wantWarnings) } }) } @@ -426,25 +453,25 @@ func TestValidateScaleConfiguration(t *testing.T) { enabled := true cases := []struct { - name string - modify func(*appsv1alpha1.NIMService) - wantErrs int + name string + modify func(*appsv1alpha1.NIMService) + wantErrs int + wantWarnings int }{ - {"autoscaling disabled", func(ns *appsv1alpha1.NIMService) {}, 0}, - {"enabled empty HPA", func(ns *appsv1alpha1.NIMService) { ns.Spec.Scale.Enabled = &enabled }, 1}, - {"enabled valid HPA", func(ns *appsv1alpha1.NIMService) { ns.Spec.Scale.Enabled = &enabled; ns.Spec.Scale.HPA.MaxReplicas = 3 }, 0}, + {"autoscaling disabled", func(ns *appsv1alpha1.NIMService) {}, 0, 0}, + {"enabled empty HPA", func(ns *appsv1alpha1.NIMService) { ns.Spec.Scale.Enabled = &enabled }, 1, 0}, + {"enabled valid HPA", func(ns *appsv1alpha1.NIMService) { ns.Spec.Scale.Enabled = &enabled; ns.Spec.Scale.HPA.MaxReplicas = 3 }, 0, 0}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { ns := baseNIMService() tc.modify(ns) - errs := validateScaleConfiguration(&ns.Spec.Scale, fld) - if got := len(errs); got != tc.wantErrs { + w, errs := validateScaleConfiguration(&ns.Spec.Scale, fld) + gotErrs := len(errs) + gotWarnings := len(w) + if gotErrs != tc.wantErrs || gotWarnings != tc.wantWarnings { t.Logf("Validation errors:") - for i, err := range errs { - t.Logf(" %d: %s", i+1, err.Error()) - } - t.Fatalf("got %d errs, want %d", got, tc.wantErrs) + t.Fatalf("got %d errs, %d warnings, want %d errs, %d warnings", gotErrs, gotWarnings, tc.wantErrs, tc.wantWarnings) } }) } @@ -455,26 +482,26 @@ func TestValidateResourcesConfiguration(t *testing.T) { fld := field.NewPath("spec").Child("resources") cases := []struct { - name string - modify func(*appsv1alpha1.NIMService) - wantErrs int + name string + modify func(*appsv1alpha1.NIMService) + wantErrs int + wantWarnings int }{ - {"nil resources", func(ns *appsv1alpha1.NIMService) {}, 0}, + {"nil resources", func(ns *appsv1alpha1.NIMService) {}, 0, 0}, {"with claims", func(ns *appsv1alpha1.NIMService) { ns.Spec.Resources = &corev1.ResourceRequirements{Claims: []corev1.ResourceClaim{{Name: "c1"}}} - }, 1}, + }, 1, 0}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { ns := baseNIMService() tc.modify(ns) - errs := validateResourcesConfiguration(ns.Spec.Resources, fld) - if got := len(errs); got != tc.wantErrs { + w, errs := validateResourcesConfiguration(ns.Spec.Resources, fld) + gotErrs := len(errs) + gotWarnings := len(w) + if gotErrs != tc.wantErrs || gotWarnings != tc.wantWarnings { t.Logf("Validation errors:") - for i, err := range errs { - t.Logf(" %d: %s", i+1, err.Error()) - } - t.Fatalf("got %d errs, want %d", got, tc.wantErrs) + t.Fatalf("got %d errs, %d warnings, want %d errs, %d warnings", gotErrs, gotWarnings, tc.wantErrs, tc.wantWarnings) } }) } @@ -487,26 +514,30 @@ func TestValidateMultiNodeImmutability(t *testing.T) { old.Spec.MultiNode = &appsv1alpha1.NimServiceMultiNodeConfig{Size: 1} cases := []struct { - name string - newObj *appsv1alpha1.NIMService - wantErrs int + name string + newObj *appsv1alpha1.NIMService + wantErrs int + wantWarnings int }{ {"unchanged", func() *appsv1alpha1.NIMService { n := baseNIMService() n.Spec.MultiNode = &appsv1alpha1.NimServiceMultiNodeConfig{Size: 1} return n - }(), 0}, + }(), 0, 0}, {"changed", func() *appsv1alpha1.NIMService { n := baseNIMService() n.Spec.MultiNode = &appsv1alpha1.NimServiceMultiNodeConfig{Size: 2} return n - }(), 1}, + }(), 1, 0}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - errs := validateMultiNodeImmutability(old, c.newObj, fld) - if got := len(errs); got != c.wantErrs { - t.Fatalf("got %d errs, want %d", got, c.wantErrs) + w, errs := validateMultiNodeImmutability(old, c.newObj, fld) + gotErrs := len(errs) + gotWarnings := len(w) + if gotErrs != c.wantErrs || gotWarnings != c.wantWarnings { + t.Logf("Validation errors:") + t.Fatalf("got %d errs, %d warnings, want %d errs, %d warnings", gotErrs, gotWarnings, c.wantErrs, c.wantWarnings) } }) } @@ -520,30 +551,30 @@ func TestValidatePVCImmutability(t *testing.T) { old.Spec.Storage.PVC = appsv1alpha1.PersistentVolumeClaim{Create: &trueVal, Size: "10Gi"} cases := []struct { - name string - newObj *appsv1alpha1.NIMService - wantErrs int + name string + newObj *appsv1alpha1.NIMService + wantErrs int + wantWarnings int }{ {"no change", func() *appsv1alpha1.NIMService { n := baseNIMService() n.Spec.Storage.PVC = old.Spec.Storage.PVC return n - }(), 0}, + }(), 0, 0}, {"changed size", func() *appsv1alpha1.NIMService { n := baseNIMService() n.Spec.Storage.PVC = appsv1alpha1.PersistentVolumeClaim{Create: &trueVal, Size: "20Gi"} return n - }(), 1}, + }(), 1, 0}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - errs := validatePVCImmutability(old, tc.newObj, fld) - if got := len(errs); got != tc.wantErrs { + w, errs := validatePVCImmutability(old, tc.newObj, fld) + gotErrs := len(errs) + gotWarnings := len(w) + if gotErrs != tc.wantErrs || gotWarnings != tc.wantWarnings { t.Logf("Validation errors:") - for i, err := range errs { - t.Logf(" %d: %s", i+1, err.Error()) - } - t.Fatalf("got %d errs, want %d", got, tc.wantErrs) + t.Fatalf("got %d errs, %d warnings, want %d errs, %d warnings", gotErrs, gotWarnings, tc.wantErrs, tc.wantWarnings) } }) }