diff --git a/pkg/webhook/v1beta1/experiment/validation_webhook.go b/pkg/webhook/v1beta1/experiment/validation_webhook.go index 442af14412f..e8ea920f9b5 100644 --- a/pkg/webhook/v1beta1/experiment/validation_webhook.go +++ b/pkg/webhook/v1beta1/experiment/validation_webhook.go @@ -90,7 +90,8 @@ func (v *ExperimentValidator) Handle(ctx context.Context, req admission.Request) return admission.Errored(http.StatusBadRequest, err) } - err = v.ValidateExperiment(inst, oldInst) + allErrs := v.ValidateExperiment(inst, oldInst) + err = allErrs.ToAggregate() if err != nil { return admission.Errored(http.StatusBadRequest, err) } diff --git a/pkg/webhook/v1beta1/experiment/validator/validator.go b/pkg/webhook/v1beta1/experiment/validator/validator.go index 8da0c3e3ff9..218bf714b05 100644 --- a/pkg/webhook/v1beta1/experiment/validator/validator.go +++ b/pkg/webhook/v1beta1/experiment/validator/validator.go @@ -19,6 +19,7 @@ package validator import ( "encoding/json" "fmt" + "k8s.io/apimachinery/pkg/util/validation/field" "path/filepath" "regexp" "strconv" @@ -41,10 +42,23 @@ import ( mccommon "github.com/kubeflow/katib/pkg/metricscollector/v1beta1/common" ) -var log = logf.Log.WithName("experiment-validating-webhook") +var ( + log = logf.Log.WithName("experiment-validating-webhook") + + specPath = field.NewPath("spec") + objectivePath = specPath.Child("objective") + algorithmPath = specPath.Child("algorithm") + earlyStoppingPath = specPath.Child("earlyStopping") + resumePolicyPath = specPath.Child("resumePolicy") + parametersPath = specPath.Child("parameters") + trialTemplatePath = specPath.Child("trialTemplate") + trialParametersPath = trialTemplatePath.Child("trialParameters") + metricsCollectorPath = specPath.Child("metricsCollectorSpec") + metricsSourcePath = metricsCollectorPath.Child("source") +) type Validator interface { - ValidateExperiment(instance, oldInst *experimentsv1beta1.Experiment) error + ValidateExperiment(instance, oldInst *experimentsv1beta1.Experiment) field.ErrorList InjectClient(c client.Client) } @@ -64,34 +78,38 @@ func (g *DefaultValidator) InjectClient(c client.Client) { // ValidateExperiment validates experiment for the given instance. // oldInst is specified when experiment is edited. -func (g *DefaultValidator) ValidateExperiment(instance, oldInst *experimentsv1beta1.Experiment) error { +func (g *DefaultValidator) ValidateExperiment(instance, oldInst *experimentsv1beta1.Experiment) field.ErrorList { + var allErrs field.ErrorList + namingConvention, _ := regexp.Compile("^[a-z]([-a-z0-9]*[a-z0-9])?") if !namingConvention.MatchString(instance.Name) { msg := "name must consist of lower case alphanumeric characters or '-'," + " start with an alphabetic character, and end with an alphanumeric character" + " (e.g. 'my-name', or 'abc-123', regex used for validation is '^[a-z]([-a-z0-9]*[a-z0-9])?)'" - return fmt.Errorf(msg) + allErrs = append(allErrs, field.Invalid(field.NewPath("metadata").Child("name"), instance.Name, msg)) } if instance.Spec.MaxFailedTrialCount != nil && *instance.Spec.MaxFailedTrialCount < 0 { - return fmt.Errorf("spec.maxFailedTrialCount should not be less than 0") + allErrs = append(allErrs, field.Invalid(specPath.Child("maxFailedTrialCount"), *instance.Spec.MaxFailedTrialCount, "should not be less than 0")) } if instance.Spec.MaxTrialCount != nil && *instance.Spec.MaxTrialCount <= 0 { - return fmt.Errorf("spec.maxTrialCount must be greater than 0") + allErrs = append(allErrs, field.Invalid(specPath.Child("maxTrialCount"), *instance.Spec.MaxTrialCount, "must be greater than 0")) } if instance.Spec.ParallelTrialCount != nil && *instance.Spec.ParallelTrialCount <= 0 { - return fmt.Errorf("spec.parallelTrialCount must be greater than 0") + allErrs = append(allErrs, field.Invalid(specPath.Child("parallelTrialCount"), *instance.Spec.ParallelTrialCount, "must be greater than 0")) } if instance.Spec.MaxFailedTrialCount != nil && instance.Spec.MaxTrialCount != nil { if *instance.Spec.MaxFailedTrialCount > *instance.Spec.MaxTrialCount { - return fmt.Errorf("spec.maxFailedTrialCount should be less than or equal to spec.maxTrialCount") + allErrs = append(allErrs, field.Invalid(specPath.Child("maxFailedTrialCount"), *instance.Spec.MaxFailedTrialCount, + "should be less than or equal to spec.maxTrialCount")) } } if instance.Spec.ParallelTrialCount != nil && instance.Spec.MaxTrialCount != nil { if *instance.Spec.ParallelTrialCount > *instance.Spec.MaxTrialCount { - return fmt.Errorf("spec.paralelTrialCount should be less than or equal to spec.maxTrialCount") + allErrs = append(allErrs, field.Invalid(specPath.Child("parallelTrialCount"), *instance.Spec.ParallelTrialCount, + "should be less than or equal to spec.maxTrialCount")) } } @@ -108,104 +126,119 @@ func (g *DefaultValidator) ValidateExperiment(instance, oldInst *experimentsv1be msg := fmt.Sprintf("Experiment can be restarted if it is in succeeded state by reaching max trials and "+ "spec.resumePolicy = %v or spec.resumePolicy = %v, when experiment is completed", experimentsv1beta1.LongRunning, experimentsv1beta1.FromVolume) - return fmt.Errorf(msg) + allErrs = append(allErrs, field.Invalid(field.NewPath("spec").Child("resumePolicy"), instance.Spec.ResumePolicy, msg)) } if isRestarting && instance.Spec.MaxTrialCount != nil && *instance.Spec.MaxTrialCount <= oldInst.Status.Trials { - return fmt.Errorf("spec.maxTrialCount: %v must be greater than status.trials count: %v", - *instance.Spec.MaxTrialCount, oldInst.Status.Trials) + allErrs = append(allErrs, field.Invalid(specPath.Child("maxTrialCount"), *instance.Spec.MaxTrialCount, + "must be greater than status.trials count")) } oldInst.Spec.MaxFailedTrialCount = instance.Spec.MaxFailedTrialCount oldInst.Spec.MaxTrialCount = instance.Spec.MaxTrialCount oldInst.Spec.ParallelTrialCount = instance.Spec.ParallelTrialCount if !equality.Semantic.DeepEqual(instance.Spec, oldInst.Spec) { - return fmt.Errorf("only spec.parallelTrialCount, spec.maxTrialCount and spec.maxFailedTrialCount are editable") + allErrs = append(allErrs, field.Forbidden(specPath, "only spec.parallelTrialCount, spec.maxTrialCount and spec.maxFailedTrialCount are editable")) } } if err := g.validateObjective(instance.Spec.Objective); err != nil { + allErrs = append(allErrs, err...) return err } if err := g.validateAlgorithm(instance.Spec.Algorithm); err != nil { - return err + allErrs = append(allErrs, err...) } if err := g.validateEarlyStopping(instance.Spec.EarlyStopping); err != nil { - return err + allErrs = append(allErrs, err...) } if err := g.validateResumePolicy(instance.Spec.ResumePolicy); err != nil { - return err + allErrs = append(allErrs, err...) } if err := g.validateTrialTemplate(instance); err != nil { - return err + allErrs = append(allErrs, err...) } if len(instance.Spec.Parameters) == 0 && instance.Spec.NasConfig == nil { - return fmt.Errorf("spec.parameters or spec.nasConfig must be specified") + allErrs = append(allErrs, field.Required(field.NewPath("spec"), "spec.parameters or spec.nasConfig must be specified")) } if len(instance.Spec.Parameters) > 0 && instance.Spec.NasConfig != nil { - return fmt.Errorf("only one of spec.parameters and spec.nasConfig can be specified") + allErrs = append(allErrs, field.Invalid(field.NewPath("spec"), instance.Spec, + "only one of spec.parameters and spec.nasConfig can be specified")) } if len(instance.Spec.Parameters) > 0 { if err := g.validateParameters(instance.Spec.Parameters); err != nil { - return err + allErrs = append(allErrs, err...) } } if err := g.validateMetricsCollector(instance); err != nil { - return err + allErrs = append(allErrs, err...) } - return nil + return allErrs } -func (g *DefaultValidator) validateObjective(obj *commonapiv1beta1.ObjectiveSpec) error { +func (g *DefaultValidator) validateObjective(obj *commonapiv1beta1.ObjectiveSpec) field.ErrorList { + var allErrs field.ErrorList + if obj == nil { - return fmt.Errorf("no spec.objective specified") + allErrs = append(allErrs, field.Required(objectivePath, "must be specified")) + return allErrs } if obj.Type != commonapiv1beta1.ObjectiveTypeMinimize && obj.Type != commonapiv1beta1.ObjectiveTypeMaximize { - return fmt.Errorf("spec.objective.type must be %s or %s", commonapiv1beta1.ObjectiveTypeMinimize, commonapiv1beta1.ObjectiveTypeMaximize) + allErrs = append(allErrs, field.Invalid(objectivePath.Child("type"), obj.Type, + fmt.Sprintf("must be %s or %s", commonapiv1beta1.ObjectiveTypeMinimize, commonapiv1beta1.ObjectiveTypeMaximize))) } if obj.ObjectiveMetricName == "" { - return fmt.Errorf("no spec.objective.objectiveMetricName specified") + allErrs = append(allErrs, field.Required(objectivePath.Child("objectiveMetricName"), "must be specified")) } if contains(obj.AdditionalMetricNames, obj.ObjectiveMetricName) { - return fmt.Errorf("spec.objective.additionalMetricNames should not contain spec.objective.objectiveMetricName") + allErrs = append(allErrs, field.Invalid(objectivePath.Child("additionalMetricNames"), + obj.AdditionalMetricNames, "should not contain spec.objective.objectiveMetricName")) } - return nil + return allErrs } -func (g *DefaultValidator) validateAlgorithm(ag *commonapiv1beta1.AlgorithmSpec) error { +func (g *DefaultValidator) validateAlgorithm(ag *commonapiv1beta1.AlgorithmSpec) field.ErrorList { + var allErrs field.ErrorList + if ag == nil { - return fmt.Errorf("no spec.algorithm specified") + allErrs = append(allErrs, field.Required(algorithmPath, "must be specified")) + return allErrs } if ag.AlgorithmName == "" { - return fmt.Errorf("no spec.algorithm.name specified") + allErrs = append(allErrs, field.Required(algorithmPath.Child("algorithmName"), "must be specified")) } if _, err := g.GetSuggestionConfigData(ag.AlgorithmName); err != nil { - return fmt.Errorf("unable to get Suggestion config data for algorithm %s: %v", ag.AlgorithmName, err) + allErrs = append(allErrs, field.Invalid(algorithmPath.Child("algorithmName"), ag.AlgorithmName, + fmt.Sprintf("unable to get Suggestion config data for algorithm %s: %v", ag.AlgorithmName, err))) } - return nil + return allErrs } -func (g *DefaultValidator) validateEarlyStopping(es *commonapiv1beta1.EarlyStoppingSpec) error { +func (g *DefaultValidator) validateEarlyStopping(es *commonapiv1beta1.EarlyStoppingSpec) field.ErrorList { if es == nil { return nil } + + var allErrs field.ErrorList if es.AlgorithmName == "" { - return fmt.Errorf("no spec.earlyStopping.algorithmName specified") + allErrs = append(allErrs, field.Required(earlyStoppingPath.Child("algorithmName"), "must be specified")) } if _, err := g.GetEarlyStoppingConfigData(es.AlgorithmName); err != nil { - return fmt.Errorf("unable to get EarlyStopping config data for algorithm %s: %v", es.AlgorithmName, err) + allErrs = append(allErrs, field.Invalid(earlyStoppingPath.Child("algorithmName"), es.AlgorithmName, + fmt.Sprintf("unable to get EarlyStopping config data for algorithm %s: %v", es.AlgorithmName, err))) } - return nil + return allErrs } -func (g *DefaultValidator) validateResumePolicy(resume experimentsv1beta1.ResumePolicyType) error { +func (g *DefaultValidator) validateResumePolicy(resume experimentsv1beta1.ResumePolicyType) field.ErrorList { + var allErrs field.ErrorList validTypes := map[experimentsv1beta1.ResumePolicyType]string{ "": "", experimentsv1beta1.NeverResume: "", @@ -213,12 +246,13 @@ func (g *DefaultValidator) validateResumePolicy(resume experimentsv1beta1.Resume experimentsv1beta1.FromVolume: "", } if _, ok := validTypes[resume]; !ok { - return fmt.Errorf("invalid ResumePolicyType %s", resume) + allErrs = append(allErrs, field.Invalid(resumePolicyPath, resume, "invalid ResumePolicyType")) } - return nil + return allErrs } -func (g *DefaultValidator) validateParameters(parameters []experimentsv1beta1.ParameterSpec) error { +func (g *DefaultValidator) validateParameters(parameters []experimentsv1beta1.ParameterSpec) field.ErrorList { + var allErrs field.ErrorList for i, param := range parameters { if param.ParameterType != experimentsv1beta1.ParameterTypeInt && @@ -226,62 +260,68 @@ func (g *DefaultValidator) validateParameters(parameters []experimentsv1beta1.Pa param.ParameterType != experimentsv1beta1.ParameterTypeCategorical && param.ParameterType != experimentsv1beta1.ParameterTypeDiscrete && param.ParameterType != experimentsv1beta1.ParameterTypeUnknown { - return fmt.Errorf("parameterType: %v is not supported in spec.parameters[%v]: %v", param.ParameterType, i, param) + allErrs = append(allErrs, field.Invalid(parametersPath.Index(i).Child("parameterType"), + param.ParameterType, fmt.Sprintf("parameterType: %v is not supported in spec.parameters[%v]: %v", param.ParameterType, i, param))) } if equality.Semantic.DeepEqual(param.FeasibleSpace, experimentsv1beta1.FeasibleSpace{}) { - return fmt.Errorf("feasibleSpace must be specified in spec.parameters[%v]: %v", i, param) - } - - if param.ParameterType == experimentsv1beta1.ParameterTypeDouble || param.ParameterType == experimentsv1beta1.ParameterTypeInt { - if len(param.FeasibleSpace.List) > 0 { - return fmt.Errorf("feasibleSpace.list is not supported for parameterType: %v in spec.parameters[%v]: %v", param.ParameterType, i, param) - } - if param.FeasibleSpace.Max == "" && param.FeasibleSpace.Min == "" { - return fmt.Errorf("feasibleSpace.max or feasibleSpace.min must be specified for parameterType: %v in spec.parameters[%v]: %v", param.ParameterType, i, param) - } + allErrs = append(allErrs, field.Required(parametersPath.Index(i).Child("feasibleSpace"), + fmt.Sprintf("feasibleSpace must be specified in spec.parameters[%v]: %v", i, param))) + } else { + if param.ParameterType == experimentsv1beta1.ParameterTypeDouble || param.ParameterType == experimentsv1beta1.ParameterTypeInt { + if len(param.FeasibleSpace.List) > 0 { + allErrs = append(allErrs, field.Invalid(parametersPath.Index(i).Child("feasibleSpace").Child("list"), + param.FeasibleSpace.List, fmt.Sprintf("feasibleSpace.list is not supported for parameterType: %v in spec.parameters[%v]: %v", param.ParameterType, i, param))) + } + if param.FeasibleSpace.Max == "" && param.FeasibleSpace.Min == "" { + allErrs = append(allErrs, field.Required(parametersPath.Index(i).Child("feasibleSpace").Child("max"), + fmt.Sprintf("feasibleSpace.max or feasibleSpace.min must be specified for parameterType: %v in spec.parameters[%v]: %v", param.ParameterType, i, param))) + } - } else if param.ParameterType == experimentsv1beta1.ParameterTypeCategorical || param.ParameterType == experimentsv1beta1.ParameterTypeDiscrete { - if param.FeasibleSpace.Max != "" || param.FeasibleSpace.Min != "" || param.FeasibleSpace.Step != "" { - return fmt.Errorf("feasibleSpace .max, .min and .step is not supported for parameterType: %v in spec.parameters[%v]: %v", param.ParameterType, i, param) + } else if param.ParameterType == experimentsv1beta1.ParameterTypeCategorical || param.ParameterType == experimentsv1beta1.ParameterTypeDiscrete { + if param.FeasibleSpace.Max != "" || param.FeasibleSpace.Min != "" || param.FeasibleSpace.Step != "" { + allErrs = append(allErrs, field.Invalid(parametersPath.Index(i).Child("feasibleSpace"), + param.FeasibleSpace, fmt.Sprintf("feasibleSpace .max, .min and .step is not supported for parameterType: %v in spec.parameters[%v]: %v", param.ParameterType, i, param))) + } } } } - return nil + return allErrs } -func (g *DefaultValidator) validateTrialTemplate(instance *experimentsv1beta1.Experiment) error { - +func (g *DefaultValidator) validateTrialTemplate(instance *experimentsv1beta1.Experiment) field.ErrorList { + var allErrs field.ErrorList trialTemplate := instance.Spec.TrialTemplate if trialTemplate == nil { - return fmt.Errorf("spec.trialTemplate must be specified") + allErrs = append(allErrs, field.Required(trialTemplatePath, "must be specified")) + return allErrs } // Check if PrimaryContainerName is set if trialTemplate.PrimaryContainerName == "" { - return fmt.Errorf("spec.trialTemplate.primaryContainerName must be specified") + allErrs = append(allErrs, field.Required(trialTemplatePath.Child("primaryContainerName"), "must be specified")) } // Check if SuccessCondition and FailureCondition is set if trialTemplate.SuccessCondition == "" || trialTemplate.FailureCondition == "" { - return fmt.Errorf("spec.trialTemplate.successCondition and spec.trialTemplate.failureCondition must be specified") + allErrs = append(allErrs, field.Required(trialTemplatePath, "successCondition and failureCondition must be specified")) } // Check if trialParameters exists if trialTemplate.TrialParameters == nil { - return fmt.Errorf("spec.trialTemplate.trialParameters must be specified") + return append(allErrs, field.Required(trialTemplatePath.Child("trialParameters"), "must be specified")) } // Check if trialSpec or configMap exists if trialTemplate.TrialSource.TrialSpec == nil && trialTemplate.TrialSource.ConfigMap == nil { - return fmt.Errorf("spec.trialTemplate.trialSpec or spec.trialTemplate.configMap must be specified") + return append(allErrs, field.Required(trialTemplatePath.Child("TrialSource"), "spec.trialTemplate.trialSpec or spec.trialTemplate.configMap must be specified")) } // Check if trialSpec and configMap doesn't exist together if trialTemplate.TrialSource.TrialSpec != nil && trialTemplate.TrialSource.ConfigMap != nil { - return fmt.Errorf("only one of spec.trialTemplate.trialSpec or spec.trialTemplate.configMap can be specified") + return append(allErrs, field.Required(trialTemplatePath, "only one of spec.trialTemplate.trialSpec or spec.trialTemplate.configMap can be specified")) } // Check if configMap parameters are specified @@ -289,13 +329,14 @@ func (g *DefaultValidator) validateTrialTemplate(instance *experimentsv1beta1.Ex (trialTemplate.TrialSource.ConfigMap.ConfigMapName == "" || trialTemplate.TrialSource.ConfigMap.ConfigMapNamespace == "" || trialTemplate.TrialSource.ConfigMap.TemplatePath == "") { - return fmt.Errorf("for spec.trialTemplate.configMap .configMapName and .configMapNamespace and .templatePath must be specified") + return append(allErrs, field.Required(trialTemplatePath.Child("configMap"), "configMapName, configMapNamespace and templatePath must be specified")) } // Check if Trial template can be parsed to string trialTemplateStr, err := g.GetTrialTemplate(instance) if err != nil { - return fmt.Errorf("unable to parse spec.trialTemplate: %v", err) + allErrs = append(allErrs, field.Invalid(trialTemplatePath, "", fmt.Sprintf("unable to parse spec.trialTemplate: %v", err))) + return allErrs } experimentParameterNames := make(map[string]bool) @@ -306,20 +347,26 @@ func (g *DefaultValidator) validateTrialTemplate(instance *experimentsv1beta1.Ex trialParametersNames := make(map[string]bool) trialParametersRefs := make(map[string]bool) - for _, parameter := range trialTemplate.TrialParameters { + for i, parameter := range trialTemplate.TrialParameters { // Check if all trialParameters contain name and reference. Or name contains invalid character if parameter.Name == "" || parameter.Reference == "" || strings.Contains(parameter.Name, "{") || strings.Contains(parameter.Name, "}") { - return fmt.Errorf("invalid spec.trialTemplate.trialParameters: %v", parameter) + allErrs = append(allErrs, field.Invalid(trialParametersPath.Index(i), "", + "name and reference must be specified and name must not contain '{' or '}'")) + continue } // Check if parameter names are not duplicated if _, ok := trialParametersNames[parameter.Name]; ok { - return fmt.Errorf("parameter name %v can't be duplicated in spec.trialTemplate.trialParameters: %v", parameter.Name, trialTemplate.TrialParameters) + allErrs = append(allErrs, field.Invalid(trialParametersPath.Index(i).Child("name"), parameter.Name, + fmt.Sprintf("parameter name %v can't be duplicated in spec.trialTemplate.trialParameters: %v", parameter.Name, trialTemplate.TrialParameters))) + continue } // Check if parameter references are not duplicated if _, ok := trialParametersRefs[parameter.Reference]; ok { - return fmt.Errorf("parameter reference %v can't be duplicated in spec.trialTemplate.trialParameters: %v", parameter.Reference, trialTemplate.TrialParameters) + allErrs = append(allErrs, field.Invalid(trialParametersPath.Index(i).Child("reference"), parameter.Reference, + fmt.Sprintf("parameter reference %v can't be duplicated in spec.trialTemplate.trialParameters: %v", parameter.Reference, trialTemplate.TrialParameters))) + continue } trialParametersNames[parameter.Name] = true trialParametersRefs[parameter.Reference] = true @@ -328,14 +375,17 @@ func (g *DefaultValidator) validateTrialTemplate(instance *experimentsv1beta1.Ex if len(experimentParameterNames) > 0 { if !isMetaKey(parameter.Reference) { if _, ok := experimentParameterNames[parameter.Reference]; !ok { - return fmt.Errorf("parameter reference %v does not exist in spec.parameters: %v", parameter.Reference, instance.Spec.Parameters) + allErrs = append(allErrs, field.Invalid(trialParametersPath.Index(i).Child("reference"), parameter.Reference, + fmt.Sprintf("parameter reference %v does not exist in spec.parameters: %v", parameter.Reference, instance.Spec.Parameters))) } } } // Check if trialParameters contains all substitution for Trial template if !strings.Contains(trialTemplateStr, fmt.Sprintf(consts.TrialTemplateParamReplaceFormat, parameter.Name)) { - return fmt.Errorf("parameter name: %v in spec.trialParameters not found in spec.trialTemplate: %v", parameter.Name, trialTemplateStr) + allErrs = append(allErrs, field.Invalid(trialParametersPath.Index(i).Child("name"), parameter.Name, + fmt.Sprintf("parameter name: %v in spec.trialParameters not found in spec.trialTemplate: %v", parameter.Name, trialTemplateStr))) + return allErrs } trialTemplateStr = strings.Replace(trialTemplateStr, fmt.Sprintf(consts.TrialTemplateParamReplaceFormat, parameter.Name), "test-value", -1) @@ -345,32 +395,34 @@ func (g *DefaultValidator) validateTrialTemplate(instance *experimentsv1beta1.Ex substitutionRegex := regexp.MustCompile(consts.TrialTemplateParamReplaceFormatRegex) notReplacedParams := substitutionRegex.FindAllString(trialTemplateStr, -1) if len(notReplacedParams) != 0 { - return fmt.Errorf("parameters: %v in spec.trialTemplate not found in spec.trialParameters: %v", notReplacedParams, trialTemplate.TrialParameters) + allErrs = append(allErrs, field.Invalid(trialTemplatePath, "", + fmt.Sprintf("parameters: %v in spec.trialTemplate not found in spec.trialParameters: %v", notReplacedParams, trialTemplate.TrialParameters))) } // Check if Trial template can be converted to unstructured runSpec, err := util.ConvertStringToUnstructured(trialTemplateStr) if err != nil { - return fmt.Errorf("unable to convert spec.trialTemplate: %v to unstructured", trialTemplateStr) + allErrs = append(allErrs, field.Invalid(trialTemplatePath, "", fmt.Sprintf("unable to convert spec.trialTemplate: %v to unstructured", trialTemplateStr))) + return allErrs } // Check if metadata.name and metatdata.namespace is omittied if runSpec.GetName() != "" || runSpec.GetNamespace() != "" { - return fmt.Errorf("metadata.name and metadata.namespace in spec.trialTemplate must be omitted") + allErrs = append(allErrs, field.Invalid(trialTemplatePath, "", "metadata.name and metadata.namespace in spec.trialTemplate must be omitted")) } // Check if ApiVersion and Kind is specified if runSpec.GetAPIVersion() == "" || runSpec.GetKind() == "" { - return fmt.Errorf("APIVersion and Kind in spec.trialTemplate must be specified") + allErrs = append(allErrs, field.Required(trialTemplatePath, "APIVersion and Kind in spec.trialTemplate must be specified")) } // Check if Job can be converted to Batch Job // Other CRDs are not validated if err := g.validateTrialJob(runSpec); err != nil { - return fmt.Errorf("invalid spec.trialTemplate: %v", err) + allErrs = append(allErrs, field.Invalid(trialTemplatePath, "", fmt.Sprintf("invalid spec.trialTemplate: %v", err))) } - return nil + return allErrs } func (g *DefaultValidator) validateTrialJob(runSpec *unstructured.Unstructured) error { @@ -420,7 +472,8 @@ func validatePatchJob(runSpec *unstructured.Unstructured, job interface{}, jobTy return nil } -func (g *DefaultValidator) validateMetricsCollector(inst *experimentsv1beta1.Experiment) error { +func (g *DefaultValidator) validateMetricsCollector(inst *experimentsv1beta1.Experiment) field.ErrorList { + var allErrs field.ErrorList mcSpec := inst.Spec.MetricsCollectorSpec mcKind := mcSpec.Collector.Kind for _, mc := range mccommon.AutoInjectMetricsCollectorList { @@ -428,71 +481,84 @@ func (g *DefaultValidator) validateMetricsCollector(inst *experimentsv1beta1.Exp continue } if _, err := g.GetMetricsCollectorConfigData(mcKind); err != nil { - return fmt.Errorf("GetMetricsCollectorConfigData failed: %v", err) + allErrs = append(allErrs, field.Invalid(metricsCollectorPath.Child("collector").Child("kind"), + mcKind, fmt.Sprintf("GetMetricsCollectorConfigData failed: %v", err))) } break } // TODO(hougangliu): log warning message if some field will not be used for the metricsCollector kind switch mcKind { case commonapiv1beta1.NoneCollector, commonapiv1beta1.StdOutCollector: - return nil + return allErrs case commonapiv1beta1.FileCollector: if mcSpec.Source == nil || mcSpec.Source.FileSystemPath == nil || mcSpec.Source.FileSystemPath.Kind != commonapiv1beta1.FileKind || !filepath.IsAbs(mcSpec.Source.FileSystemPath.Path) { - return fmt.Errorf("file path where metrics file exists is required by .spec.metricsCollectorSpec.source.fileSystemPath.path") + allErrs = append(allErrs, field.Required(metricsSourcePath.Child("fileSystemPath").Child("path"), + "file path where metrics file exists is required by .spec.metricsCollectorSpec.source.fileSystemPath.path")) } // Format fileFormat := mcSpec.Source.FileSystemPath.Format if fileFormat != commonapiv1beta1.TextFormat && fileFormat != commonapiv1beta1.JsonFormat { - return fmt.Errorf("format of metrics file is required by .spec.metricsCollectorSpec.source.fileSystemPath.format") + allErrs = append(allErrs, field.Required(metricsSourcePath.Child("fileSystemPath").Child("format"), + "format of metrics file is required by .spec.metricsCollectorSpec.source.fileSystemPath.format")) } if fileFormat == commonapiv1beta1.JsonFormat && mcSpec.Source.Filter != nil { - return fmt.Errorf(".spec.metricsCollectorSpec.source.filter must be nil when format of metrics file is %v", commonapiv1beta1.JsonFormat) + allErrs = append(allErrs, field.Invalid(metricsSourcePath.Child("filter"), + "", ".spec.metricsCollectorSpec.source.filter must be nil when format of metrics file is json")) } case commonapiv1beta1.TfEventCollector: if mcSpec.Source == nil || mcSpec.Source.FileSystemPath == nil || mcSpec.Source.FileSystemPath.Kind != commonapiv1beta1.DirectoryKind || !filepath.IsAbs(mcSpec.Source.FileSystemPath.Path) { - return fmt.Errorf("directory path where tensorflow event files exist is required by .spec.metricsCollectorSpec.source.fileSystemPath.path") + allErrs = append(allErrs, field.Required(metricsSourcePath, + "directory path where tensorflow event files exist is required by .spec.metricsCollectorSpec.source.fileSystemPath.path")) } if mcSpec.Source.FileSystemPath.Format != "" { - return fmt.Errorf(".spec.metricsCollectorSpec.source.fileSystemPath.format must be empty") + allErrs = append(allErrs, field.Invalid(metricsSourcePath.Child("fileSystemPath").Child("format"), + mcSpec.Source.FileSystemPath.Format, ".spec.metricsCollectorSpec.source.fileSystemPath.format must be empty")) } case commonapiv1beta1.PrometheusMetricCollector: i, err := strconv.Atoi(mcSpec.Source.HttpGet.Port.String()) if err != nil || i <= 0 { - return fmt.Errorf(".spec.metricsCollectorSpec.source.httpGet.port must be a positive integer value for metrics collector kind: %v", mcKind) + allErrs = append(allErrs, field.Invalid(metricsSourcePath.Child("httpGet").Child("port"), + mcSpec.Source.HttpGet.Port.String(), fmt.Sprintf(".spec.metricsCollectorSpec.source.httpGet.port must be a positive integer value for metrics collector kind: %v", mcKind))) } if !strings.HasPrefix(mcSpec.Source.HttpGet.Path, "/") { - return fmt.Errorf(".spec.metricsCollectorSpec.source.httpGet.path is invalid for metrics collector kind: %v", mcKind) + allErrs = append(allErrs, field.Invalid(metricsSourcePath.Child("httpGet").Child("path"), + mcSpec.Source.HttpGet.Path, fmt.Sprintf(".spec.metricsCollectorSpec.source.httpGet.path is invalid for metrics collector kind: %v", mcKind))) } case commonapiv1beta1.CustomCollector: if mcSpec.Collector.CustomCollector == nil { - return fmt.Errorf(".spec.metricsCollectorSpec.collector.customCollector is required for metrics collector kind: %v", mcKind) + allErrs = append(allErrs, field.Required(metricsCollectorPath.Child("collector").Child("customCollector"), + fmt.Sprintf(".spec.metricsCollectorSpec.collector.customCollector is required for metrics collector kind: %v", mcKind))) } - if mcSpec.Source.FileSystemPath != nil { + if mcSpec.Source != nil && mcSpec.Source.FileSystemPath != nil { if !filepath.IsAbs(mcSpec.Source.FileSystemPath.Path) || (mcSpec.Source.FileSystemPath.Kind != commonapiv1beta1.DirectoryKind && mcSpec.Source.FileSystemPath.Kind != commonapiv1beta1.FileKind) { - return fmt.Errorf(".spec.metricsCollectorSpec.source is invalid") + allErrs = append(allErrs, field.Invalid(metricsSourcePath.Child("fileSystemPath"), + "", fmt.Sprintf(".spec.metricsCollectorSpec.source is invalid"))) } } default: - return fmt.Errorf("invalid metrics collector kind: %v", mcKind) + allErrs = append(allErrs, field.Invalid(metricsCollectorPath.Child("collector").Child("kind"), + mcKind, fmt.Sprintf("invalid metrics collector kind: %v", mcKind))) } if mcSpec.Source != nil && mcSpec.Source.Filter != nil && len(mcSpec.Source.Filter.MetricsFormat) > 0 { // the filter regular expression must have two top subexpressions, the first matched one will be taken as metric name, the second one as metric value mustTwoBracket, _ := regexp.Compile(`.*\(.*\).*\(.*\).*`) for _, mFormat := range mcSpec.Source.Filter.MetricsFormat { if _, err := regexp.Compile(mFormat); err != nil { - return fmt.Errorf("invalid %q in .spec.metricsCollectorSpec.source.filter: %v", mFormat, err) + allErrs = append(allErrs, field.Invalid(metricsSourcePath.Child("filter").Child("metricsFormat"), + mFormat, fmt.Sprintf("invalid %q in .spec.metricsCollectorSpec.source.filter: %v", mFormat, err))) } else { if !mustTwoBracket.MatchString(mFormat) { - return fmt.Errorf("invalid %q in .spec.metricsCollectorSpec.source.filter: two top subexpressions are required", mFormat) + allErrs = append(allErrs, field.Invalid(metricsSourcePath.Child("filter").Child("metricsFormat"), + mFormat, fmt.Sprintf("invalid %q in .spec.metricsCollectorSpec.source.filter: two top subexpressions are required", mFormat))) } } } } - return nil + return allErrs } func isMetaKey(parameter string) bool { diff --git a/pkg/webhook/v1beta1/experiment/validator/validator_test.go b/pkg/webhook/v1beta1/experiment/validator/validator_test.go index 0bc33acbb8d..a5fccbfce4c 100644 --- a/pkg/webhook/v1beta1/experiment/validator/validator_test.go +++ b/pkg/webhook/v1beta1/experiment/validator/validator_test.go @@ -18,6 +18,11 @@ package validator import ( "errors" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + experimentutil "github.com/kubeflow/katib/pkg/controller.v1beta1/experiment/util" + "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/apimachinery/pkg/util/validation/field" "testing" "github.com/golang/mock/gomock" @@ -25,14 +30,12 @@ import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/util/intstr" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" configv1beta1 "github.com/kubeflow/katib/pkg/apis/config/v1beta1" commonv1beta1 "github.com/kubeflow/katib/pkg/apis/controller/common/v1beta1" experimentsv1beta1 "github.com/kubeflow/katib/pkg/apis/controller/experiments/v1beta1" - experimentutil "github.com/kubeflow/katib/pkg/controller.v1beta1/experiment/util" "github.com/kubeflow/katib/pkg/controller.v1beta1/util" manifestmock "github.com/kubeflow/katib/pkg/mock/v1beta1/experiment/manifest" @@ -66,7 +69,7 @@ func TestValidateExperiment(t *testing.T) { tcs := []struct { instance *experimentsv1beta1.Experiment - err bool + wantErr field.ErrorList oldInstance *experimentsv1beta1.Experiment testDescription string }{ @@ -77,7 +80,9 @@ func TestValidateExperiment(t *testing.T) { i.Name = "1234-test" return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("metadata").Child("name"), "", ""), + }, testDescription: "Name is invalid", }, // Objective @@ -87,7 +92,9 @@ func TestValidateExperiment(t *testing.T) { i.Spec.Objective = nil return i }(), - err: true, + wantErr: field.ErrorList{ + field.Required(field.NewPath("spec").Child("objective"), ""), + }, testDescription: "Objective is nil", }, { @@ -96,7 +103,9 @@ func TestValidateExperiment(t *testing.T) { i.Spec.Objective.Type = commonv1beta1.ObjectiveTypeUnknown return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("objective").Child("type"), "", ""), + }, testDescription: "Objective type is unknown", }, { @@ -105,7 +114,9 @@ func TestValidateExperiment(t *testing.T) { i.Spec.Objective.ObjectiveMetricName = "" return i }(), - err: true, + wantErr: field.ErrorList{ + field.Required(field.NewPath("spec").Child("objective").Child("objectiveMetricName"), ""), + }, testDescription: "Objective metric name is empty", }, { @@ -115,7 +126,9 @@ func TestValidateExperiment(t *testing.T) { i.Spec.Objective.AdditionalMetricNames = []string{"objective", "objective-1"} return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("objective").Child("additionalMetricNames"), "", ""), + }, testDescription: "additionalMetricNames should not contain objective metric name", }, // Algorithm @@ -125,7 +138,9 @@ func TestValidateExperiment(t *testing.T) { i.Spec.Algorithm = nil return i }(), - err: true, + wantErr: field.ErrorList{ + field.Required(field.NewPath("spec").Child("algorithm"), ""), + }, testDescription: "Algorithm is nil", }, { @@ -134,7 +149,9 @@ func TestValidateExperiment(t *testing.T) { i.Spec.Algorithm.AlgorithmName = "" return i }(), - err: true, + wantErr: field.ErrorList{ + field.Required(field.NewPath("spec").Child("algorithm").Child("algorithmName"), ""), + }, testDescription: "Algorithm name is empty", }, // EarlyStopping @@ -144,7 +161,6 @@ func TestValidateExperiment(t *testing.T) { i.Spec.EarlyStopping = nil return i }(), - err: false, testDescription: "EarlyStopping is nil", }, { @@ -153,13 +169,14 @@ func TestValidateExperiment(t *testing.T) { i.Spec.EarlyStopping.AlgorithmName = "" return i }(), - err: true, + wantErr: field.ErrorList{ + field.Required(field.NewPath("spec").Child("earlyStopping").Child("algorithmName"), ""), + }, testDescription: "EarlyStopping AlgorithmName is empty", }, // Valid Experiment { instance: newFakeInstance(), - err: false, testDescription: "Run validator for correct experiment", }, { @@ -168,7 +185,9 @@ func TestValidateExperiment(t *testing.T) { i.Spec.MaxFailedTrialCount = &fakeNegativeInt return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("maxFailedTrialCount"), "", ""), + }, testDescription: "Max failed trial count is negative", }, { @@ -177,7 +196,9 @@ func TestValidateExperiment(t *testing.T) { i.Spec.MaxTrialCount = &fakeNegativeInt return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("maxTrialCount"), "", ""), + }, testDescription: "Max trial count is negative", }, { @@ -186,19 +207,23 @@ func TestValidateExperiment(t *testing.T) { i.Spec.ParallelTrialCount = &fakeNegativeInt return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("parallelTrialCount"), "", ""), + }, testDescription: "Parallel trial count is negative", }, // Validate Resume Experiment { instance: newFakeInstance(), - err: false, oldInstance: newFakeInstance(), testDescription: "Run validator to correct resume experiment", }, { instance: newFakeInstance(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("resumePolicy"), "", ""), + field.Forbidden(field.NewPath("spec"), ""), + }, oldInstance: func() *experimentsv1beta1.Experiment { i := newFakeInstance() i.MarkExperimentStatusSucceeded(experimentutil.ExperimentMaxTrialsReachedReason, "Experiment is succeeded") @@ -209,7 +234,9 @@ func TestValidateExperiment(t *testing.T) { }, { instance: newFakeInstance(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("maxTrialCount"), "", ""), + }, oldInstance: func() *experimentsv1beta1.Experiment { i := newFakeInstance() i.Status = experimentsv1beta1.ExperimentStatus{ @@ -223,7 +250,9 @@ func TestValidateExperiment(t *testing.T) { }, { instance: newFakeInstance(), - err: true, + wantErr: field.ErrorList{ + field.Forbidden(field.NewPath("spec"), ""), + }, oldInstance: func() *experimentsv1beta1.Experiment { i := newFakeInstance() i.Spec.Algorithm.AlgorithmName = "not-test" @@ -237,7 +266,9 @@ func TestValidateExperiment(t *testing.T) { i.Spec.ResumePolicy = "invalid-policy" return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("resumePolicy"), "", ""), + }, testDescription: "Invalid resume policy", }, // Validate NAS Config @@ -248,7 +279,9 @@ func TestValidateExperiment(t *testing.T) { i.Spec.NasConfig = nil return i }(), - err: true, + wantErr: field.ErrorList{ + field.Required(field.NewPath("spec"), ""), + }, testDescription: "Parameters and NAS config is nil", }, { @@ -263,7 +296,9 @@ func TestValidateExperiment(t *testing.T) { } return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec"), "", ""), + }, testDescription: "Parameters and NAS config is not nil", }, { @@ -272,7 +307,9 @@ func TestValidateExperiment(t *testing.T) { i.Spec.TrialTemplate = nil return i }(), - err: true, + wantErr: field.ErrorList{ + field.Required(field.NewPath("spec").Child("trialTemplate"), ""), + }, testDescription: "Trial template is nil", }, { @@ -281,7 +318,9 @@ func TestValidateExperiment(t *testing.T) { i.Spec.Parameters[1].FeasibleSpace.Max = "5" return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("parameters").Index(1).Child("feasibleSpace"), "", ""), + }, testDescription: "Invalid feasible space in parameters", }, { @@ -293,7 +332,9 @@ func TestValidateExperiment(t *testing.T) { i.Spec.MaxFailedTrialCount = &invalidMaxFailedTrialCount return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("maxFailedTrialCount"), "", ""), + }, testDescription: "maxFailedTrialCount greater than maxTrialCount", }, { @@ -305,7 +346,6 @@ func TestValidateExperiment(t *testing.T) { i.Spec.MaxFailedTrialCount = &validMaxFailedTrialCount return i }(), - err: false, testDescription: "maxFailedTrialCount equal to maxTrialCount", }, { @@ -317,7 +357,9 @@ func TestValidateExperiment(t *testing.T) { i.Spec.ParallelTrialCount = &invalidParallelTrialCount return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("parallelTrialCount"), "", ""), + }, testDescription: "parallelTrialCount greater than maxTrialCount", }, { @@ -329,17 +371,14 @@ func TestValidateExperiment(t *testing.T) { i.Spec.ParallelTrialCount = &validParallelTrialCount return i }(), - err: false, testDescription: "parallelTrialCount equal to maxTrialCount", }, } for _, tc := range tcs { - err := g.ValidateExperiment(tc.instance, tc.oldInstance) - if !tc.err && err != nil { - t.Errorf("Case: %v failed. Expected nil, got %v", tc.testDescription, err) - } else if tc.err && err == nil { - t.Errorf("Case: %v failed. Expected err, got nil", tc.testDescription) + gotError := g.ValidateExperiment(tc.instance, tc.oldInstance) + if diff := cmp.Diff(tc.wantErr, gotError, cmpopts.IgnoreFields(field.Error{}, "Detail", "BadValue")); len(diff) != 0 { + t.Errorf("Unexpected errors (-want,+got):\n%s", diff) } } } @@ -353,7 +392,7 @@ func TestValidateParameters(t *testing.T) { tcs := []struct { parameters []experimentsv1beta1.ParameterSpec - err bool + wantErr field.ErrorList testDescription string }{ { @@ -362,7 +401,9 @@ func TestValidateParameters(t *testing.T) { ps[0].ParameterType = "invalid-type" return ps }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("parameters").Index(0).Child("parameterType"), "", ""), + }, testDescription: "Invalid parameter type", }, { @@ -371,7 +412,9 @@ func TestValidateParameters(t *testing.T) { ps[0].FeasibleSpace = experimentsv1beta1.FeasibleSpace{} return ps }(), - err: true, + wantErr: field.ErrorList{ + field.Required(field.NewPath("spec").Child("parameters").Index(0).Child("feasibleSpace"), ""), + }, testDescription: "Feasible space is nil", }, { @@ -380,7 +423,9 @@ func TestValidateParameters(t *testing.T) { ps[0].FeasibleSpace.List = []string{"invalid-list"} return ps }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("parameters").Index(0).Child("feasibleSpace").Child("list"), "", ""), + }, testDescription: "Not empty list for int parameter type", }, { @@ -393,7 +438,9 @@ func TestValidateParameters(t *testing.T) { } return ps }(), - err: true, + wantErr: field.ErrorList{ + field.Required(field.NewPath("spec").Child("parameters").Index(0).Child("feasibleSpace").Child("max"), ""), + }, testDescription: "Empty max and min for int parameter type", }, { @@ -402,17 +449,17 @@ func TestValidateParameters(t *testing.T) { ps[1].FeasibleSpace.Max = "1" return ps }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("parameters").Index(1).Child("feasibleSpace"), "", ""), + }, testDescription: "Not empty max for categorical parameter type", }, } for _, tc := range tcs { - err := g.(*DefaultValidator).validateParameters(tc.parameters) - if !tc.err && err != nil { - t.Errorf("Case: %v failed. Expected nil, got %v", tc.testDescription, err) - } else if tc.err && err == nil { - t.Errorf("Case: %v failed. Expected err, got nil", tc.testDescription) + gotError := g.(*DefaultValidator).validateParameters(tc.parameters) + if diff := cmp.Diff(tc.wantErr, gotError, cmpopts.IgnoreFields(field.Error{}, "Detail", "BadValue")); len(diff) != 0 { + t.Errorf("Unexpected errors (-want,+got):\n%s", diff) } } } @@ -476,6 +523,8 @@ spec: validTemplate7 := p.EXPECT().GetTrialTemplate(gomock.Any()).Return(validJobStr, nil) validTemplate8 := p.EXPECT().GetTrialTemplate(gomock.Any()).Return(validJobStr, nil) validTemplate9 := p.EXPECT().GetTrialTemplate(gomock.Any()).Return(validJobStr, nil) + validTemplate10 := p.EXPECT().GetTrialTemplate(gomock.Any()).Return(validJobStr, nil) + validTemplate11 := p.EXPECT().GetTrialTemplate(gomock.Any()).Return(validJobStr, nil) missedParameterTemplate := p.EXPECT().GetTrialTemplate(gomock.Any()).Return(missedParameterJobStr, nil) oddParameterTemplate := p.EXPECT().GetTrialTemplate(gomock.Any()).Return(oddParameterJobStr, nil) @@ -501,21 +550,25 @@ spec: notEmptyMetadataTemplate, emptyAPIVersionTemplate, customJobTypeTemplate, + validTemplate10, + validTemplate11, ) tcs := []struct { instance *experimentsv1beta1.Experiment - err bool + wantErr field.ErrorList testDescription string }{ - // TrialParamters is nil + // TrialParameters is nil { instance: func() *experimentsv1beta1.Experiment { i := newFakeInstance() i.Spec.TrialTemplate.TrialParameters = nil return i }(), - err: true, + wantErr: field.ErrorList{ + field.Required(field.NewPath("spec").Child("trialTemplate").Child("trialParameters"), ""), + }, testDescription: "Trial parameters is nil", }, // TrialSpec and ConfigMap is nil @@ -525,7 +578,9 @@ spec: i.Spec.TrialTemplate.TrialSpec = nil return i }(), - err: true, + wantErr: field.ErrorList{ + field.Required(field.NewPath("spec").Child("trialTemplate").Child("TrialSource"), ""), + }, testDescription: "Trial spec nil", }, // TrialSpec and ConfigMap is not nil @@ -533,11 +588,15 @@ spec: instance: func() *experimentsv1beta1.Experiment { i := newFakeInstance() i.Spec.TrialTemplate.TrialSource.ConfigMap = &experimentsv1beta1.ConfigMapSource{ - ConfigMapName: "config-map-name", + ConfigMapName: "config-map-name", + ConfigMapNamespace: "config-map-namespace", + TemplatePath: "config-map-template-path", } return i }(), - err: true, + wantErr: field.ErrorList{ + field.Required(field.NewPath("spec").Child("trialTemplate"), ""), + }, testDescription: "Trial spec and ConfigMap is not nil", }, // ConfigMap missed template path @@ -552,7 +611,9 @@ spec: } return i }(), - err: true, + wantErr: field.ErrorList{ + field.Required(field.NewPath("spec").Child("trialTemplate").Child("configMap"), ""), + }, testDescription: "Missed template path in ConfigMap", }, // Wrong path in configMap @@ -570,7 +631,9 @@ spec: } return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("trialTemplate"), "", ""), + }, testDescription: "Wrong template path in ConfigMap", }, // Empty Reference or Name in trialParameters @@ -580,7 +643,10 @@ spec: i.Spec.TrialTemplate.TrialParameters[0].Reference = "" return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("trialTemplate").Child("trialParameters").Index(0), "", ""), + field.Invalid(field.NewPath("spec").Child("trialTemplate"), "", ""), + }, testDescription: "Empty reference or name in Trial parameters", }, // Wrong Name in trialParameters @@ -590,7 +656,10 @@ spec: i.Spec.TrialTemplate.TrialParameters[0].Name = "{invalid-name}" return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("trialTemplate").Child("trialParameters").Index(0), "", ""), + field.Invalid(field.NewPath("spec").Child("trialTemplate"), "", ""), + }, testDescription: "Wrong name in Trial parameters", }, // Duplicate Name in trialParameters @@ -600,7 +669,10 @@ spec: i.Spec.TrialTemplate.TrialParameters[1].Name = i.Spec.TrialTemplate.TrialParameters[0].Name return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("trialTemplate").Child("trialParameters").Index(1).Child("name"), "", ""), + field.Invalid(field.NewPath("spec").Child("trialTemplate"), "", ""), + }, testDescription: "Duplicate name in Trial parameters", }, // Duplicate Reference in trialParameters @@ -610,7 +682,10 @@ spec: i.Spec.TrialTemplate.TrialParameters[1].Reference = i.Spec.TrialTemplate.TrialParameters[0].Reference return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("trialTemplate").Child("trialParameters").Index(1).Child("reference"), "", ""), + field.Invalid(field.NewPath("spec").Child("trialTemplate"), "", ""), + }, testDescription: "Duplicate reference in Trial parameters", }, // Trial template contains Trial parameters which weren't referenced from spec.parameters @@ -620,7 +695,9 @@ spec: i.Spec.TrialTemplate.TrialParameters[1].Reference = "wrong-ref" return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("trialTemplate").Child("trialParameters").Index(1).Child("reference"), "", ""), + }, testDescription: "Trial template contains Trial parameters which weren't referenced from spec.parameters", }, // Trial template contains Trial parameters when spec.parameters is empty @@ -631,7 +708,6 @@ spec: i.Spec.TrialTemplate.TrialParameters[1].Reference = "wrong-ref" return i }(), - err: false, testDescription: "Trial template contains Trial parameters when spec.parameters is empty", }, // Trial template contains Trial metadata parameter substitution @@ -641,7 +717,6 @@ spec: i.Spec.TrialTemplate.TrialParameters[1].Reference = "${trialSpec.Name}" return i }(), - err: false, testDescription: "Trial template contains Trial metadata reference as parameter", }, { @@ -650,7 +725,6 @@ spec: i.Spec.TrialTemplate.TrialParameters[1].Reference = "${trialSpec.Annotations[test-annotation]}" return i }(), - err: false, testDescription: "Trial template contains Trial annotation reference as parameter", }, { @@ -659,7 +733,6 @@ spec: i.Spec.TrialTemplate.TrialParameters[1].Reference = "${trialSpec.Labels[test-label]}" return i }(), - err: false, testDescription: "Trial template contains Trial's label reference as parameter", }, // Trial Template doesn't contain parameter from trialParameters @@ -669,7 +742,9 @@ spec: i := newFakeInstance() return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("trialTemplate"), "", ""), + }, testDescription: "Trial template doesn't contain parameter from Trial parameters", }, // Trial Template contains extra parameter @@ -679,7 +754,9 @@ spec: i := newFakeInstance() return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("trialTemplate"), "", ""), + }, testDescription: "Trial template contains extra parameter", }, // Trial Template parameter is invalid after substitution @@ -690,7 +767,9 @@ spec: i := newFakeInstance() return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("trialTemplate").Child("trialParameters").Index(1).Child("name"), "", ""), + }, testDescription: "Trial template is unable to convert to unstructured after substitution", }, // Trial Template contains Name and Namespace @@ -702,7 +781,9 @@ spec: i.Spec.TrialTemplate.TrialSpec.SetNamespace("trial-namespace") return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("trialTemplate"), "", ""), + }, testDescription: "Trial template contains metadata.name or metadata.namespace", }, // Trial Template doesn't contain APIVersion or Kind @@ -712,7 +793,9 @@ spec: i := newFakeInstance() return i }(), - err: true, + wantErr: field.ErrorList{ + field.Required(field.NewPath("spec").Child("trialTemplate"), ""), + }, testDescription: "Trial template doesn't contain APIVersion or Kind", }, // Trial Template has custom Kind @@ -722,7 +805,6 @@ spec: i := newFakeInstance() return i }(), - err: false, testDescription: "Trial template has custom Kind", }, // Trial Template doesn't have PrimaryContainerName @@ -732,7 +814,9 @@ spec: i.Spec.TrialTemplate.PrimaryContainerName = "" return i }(), - err: true, + wantErr: field.ErrorList{ + field.Required(field.NewPath("spec").Child("trialTemplate").Child("primaryContainerName"), ""), + }, testDescription: "Trial template doesn't have PrimaryContainerName", }, // Trial Template doesn't have SuccessCondition @@ -742,17 +826,17 @@ spec: i.Spec.TrialTemplate.SuccessCondition = "" return i }(), - err: true, + wantErr: field.ErrorList{ + field.Required(field.NewPath("spec").Child("trialTemplate"), ""), + }, testDescription: "Trial template doesn't have SuccessCondition", }, } for _, tc := range tcs { - err := g.(*DefaultValidator).validateTrialTemplate(tc.instance) - if !tc.err && err != nil { - t.Errorf("Case: %v failed. Expected nil, got %v", tc.testDescription, err) - } else if tc.err && err == nil { - t.Errorf("Case: %v failed. Expected err, got nil", tc.testDescription) + gotError := g.(*DefaultValidator).validateTrialTemplate(tc.instance) + if diff := cmp.Diff(tc.wantErr, gotError, cmpopts.IgnoreFields(field.Error{}, "Detail", "BadValue")); len(diff) != 0 { + t.Errorf("Unexpected errors (-want,+got):\n%s", diff) } } } @@ -882,7 +966,7 @@ func TestValidateMetricsCollector(t *testing.T) { tcs := []struct { instance *experimentsv1beta1.Experiment - err bool + wantErr field.ErrorList testDescription string }{ // Invalid Metrics Collector Kind @@ -896,7 +980,9 @@ func TestValidateMetricsCollector(t *testing.T) { } return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("metricsCollectorSpec").Child("collector").Child("kind"), "", ""), + }, testDescription: "Invalid metrics collector Kind", }, // FileCollector invalid Path @@ -916,7 +1002,9 @@ func TestValidateMetricsCollector(t *testing.T) { } return i }(), - err: true, + wantErr: field.ErrorList{ + field.Required(field.NewPath("spec").Child("metricsCollectorSpec").Child("source").Child("fileSystemPath").Child("path"), ""), + }, testDescription: "Invalid path for File metrics collector", }, // TfEventCollector invalid Path @@ -935,7 +1023,9 @@ func TestValidateMetricsCollector(t *testing.T) { } return i }(), - err: true, + wantErr: field.ErrorList{ + field.Required(field.NewPath("spec").Child("metricsCollectorSpec").Child("source"), ""), + }, testDescription: "Invalid path for TF event metrics collector", }, // TfEventCollector invalid file format @@ -956,7 +1046,9 @@ func TestValidateMetricsCollector(t *testing.T) { } return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("metricsCollectorSpec").Child("source").Child("fileSystemPath").Child("format"), "", ""), + }, testDescription: "Invalid file format for TF event metrics collector", }, // PrometheusMetricCollector invalid Port @@ -977,8 +1069,11 @@ func TestValidateMetricsCollector(t *testing.T) { } return i }(), - err: true, - testDescription: "Invalid port for Prometheus metrics collector", + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("metricsCollectorSpec").Child("source").Child("httpGet").Child("port"), "", ""), + field.Invalid(field.NewPath("spec").Child("metricsCollectorSpec").Child("source").Child("httpGet").Child("path"), "", ""), + }, + testDescription: "Invalid port and path for Prometheus metrics collector", }, // PrometheusMetricCollector invalid Path { @@ -999,7 +1094,9 @@ func TestValidateMetricsCollector(t *testing.T) { } return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("metricsCollectorSpec").Child("source").Child("httpGet").Child("path"), "", ""), + }, testDescription: "Invalid path for Prometheus metrics collector", }, // CustomCollector empty CustomCollector @@ -1013,7 +1110,9 @@ func TestValidateMetricsCollector(t *testing.T) { } return i }(), - err: true, + wantErr: field.ErrorList{ + field.Required(field.NewPath("spec").Child("metricsCollectorSpec").Child("collector").Child("customCollector"), ""), + }, testDescription: "Empty container for Custom metrics collector", }, // CustomCollector invalid Path @@ -1035,7 +1134,9 @@ func TestValidateMetricsCollector(t *testing.T) { } return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("metricsCollectorSpec").Child("source").Child("fileSystemPath"), "", ""), + }, testDescription: "Invalid path for Custom metrics collector", }, // FileMetricCollector invalid regexp in metrics format @@ -1061,7 +1162,9 @@ func TestValidateMetricsCollector(t *testing.T) { } return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("metricsCollectorSpec").Child("source").Child("filter").Child("metricsFormat"), "", ""), + }, testDescription: "Invalid metrics format regex for File metrics collector", }, // FileMetricCollector one subexpression in metrics format @@ -1087,7 +1190,9 @@ func TestValidateMetricsCollector(t *testing.T) { } return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("metricsCollectorSpec").Child("source").Child("filter").Child("metricsFormat"), "", ""), + }, testDescription: "One subexpression in metrics format", }, // FileMetricCollector invalid file format @@ -1108,7 +1213,9 @@ func TestValidateMetricsCollector(t *testing.T) { } return i }(), - err: true, + wantErr: field.ErrorList{ + field.Required(field.NewPath("spec").Child("metricsCollectorSpec").Child("source").Child("fileSystemPath").Child("format"), ""), + }, testDescription: "Invalid file format for File metrics collector", }, // FileMetricCollector invalid metrics filter @@ -1130,7 +1237,9 @@ func TestValidateMetricsCollector(t *testing.T) { } return i }(), - err: true, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("metricsCollectorSpec").Child("source").Child("filter"), "", ""), + }, testDescription: "Invalid metrics filer for File metrics collector when file format is `JSON`", }, // Valid FileMetricCollector @@ -1151,17 +1260,14 @@ func TestValidateMetricsCollector(t *testing.T) { } return i }(), - err: false, testDescription: "Run validator for correct File metrics collector", }, } for _, tc := range tcs { - err := g.(*DefaultValidator).validateMetricsCollector(tc.instance) - if !tc.err && err != nil { - t.Errorf("Case: %v failed. Expected nil, got %v", tc.testDescription, err) - } else if tc.err && err == nil { - t.Errorf("Case: %v failed. Expected err, got nil", tc.testDescription) + gotError := g.(*DefaultValidator).validateMetricsCollector(tc.instance) + if diff := cmp.Diff(tc.wantErr, gotError, cmpopts.IgnoreFields(field.Error{}, "Detail", "BadValue")); len(diff) != 0 { + t.Errorf("Unexpected errors (-want,+got):\n%s", diff) } } @@ -1185,7 +1291,7 @@ func TestValidateConfigData(t *testing.T) { invalidConfigCall, ) - validEarlyStoppingConfigCall := p.EXPECT().GetEarlyStoppingConfigData(gomock.Any()).Return(configv1beta1.EarlyStoppingConfig{}, nil) + validEarlyStoppingConfigCall := p.EXPECT().GetEarlyStoppingConfigData(gomock.Any()).Return(configv1beta1.EarlyStoppingConfig{}, nil).Times(2) invalidEarlyStoppingConfigCall := p.EXPECT().GetEarlyStoppingConfigData(gomock.Any()).Return(configv1beta1.EarlyStoppingConfig{}, errors.New("GetEarlyStoppingConfigData failed")) gomock.InOrder( @@ -1193,7 +1299,7 @@ func TestValidateConfigData(t *testing.T) { invalidEarlyStoppingConfigCall, ) - p.EXPECT().GetMetricsCollectorConfigData(gomock.Any()).Return(configv1beta1.MetricsCollectorConfig{}, errors.New("GetMetricsCollectorConfigData failed")) + p.EXPECT().GetMetricsCollectorConfigData(gomock.Any()).Return(configv1beta1.MetricsCollectorConfig{}, errors.New("GetMetricsCollectorConfigData failed")).Times(3) batchJobStr := convertBatchJobToString(newFakeBatchJob()) p.EXPECT().GetTrialTemplate(gomock.Any()).Return(batchJobStr, nil).AnyTimes() @@ -1217,8 +1323,8 @@ func TestValidateConfigData(t *testing.T) { } for _, tc := range tcs { - err := g.ValidateExperiment(tc.instance, nil) - if err == nil { + gotError := g.ValidateExperiment(tc.instance, nil) + if gotError == nil || len(gotError) == 0 { t.Errorf("Case: %v failed. Expected err, got nil", tc.testDescription) } }