fix: check if parameter references exist in experiment parameters (#1726

)

* fix: check if parameter references exist in experiment parameters

* Fix validator test

* Update some comments and test descriptions

* Check trial parameter reference only when experiment parameters are not empty

* Add a test for the case 'spec.parameters' is mepty

pkg/webhook/v1beta1/experiment/validator/validator.go

@@ -265,6 +265,11 @@ func (g *DefaultValidator) validateTrialTemplate(instance *experimentsv1beta1.Ex
 		return fmt.Errorf("unable to parse spec.trialTemplate: %v", err)
 	}
 
+	experimentParameterNames := make(map[string]bool)
+	for _, parameter := range instance.Spec.Parameters {
+		experimentParameterNames[parameter.Name] = true
+	}
+
 	trialParametersNames := make(map[string]bool)
 	trialParametersRefs := make(map[string]bool)
 
@@ -286,6 +291,13 @@ func (g *DefaultValidator) validateTrialTemplate(instance *experimentsv1beta1.Ex
 		trialParametersNames[parameter.Name] = true
 		trialParametersRefs[parameter.Reference] = true
 
+		// Check if parameter reference exist in experiment parameters
+		if len(experimentParameterNames) > 0 {
+			if _, ok := experimentParameterNames[parameter.Reference]; !ok {
+				return fmt.Errorf("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)

pkg/webhook/v1beta1/experiment/validator/validator_test.go

@@ -392,6 +392,8 @@ spec:
 	validTemplate2 := p.EXPECT().GetTrialTemplate(gomock.Any()).Return(validJobStr, nil)
 	validTemplate3 := p.EXPECT().GetTrialTemplate(gomock.Any()).Return(validJobStr, nil)
 	validTemplate4 := p.EXPECT().GetTrialTemplate(gomock.Any()).Return(validJobStr, nil)
+	validTemplate5 := p.EXPECT().GetTrialTemplate(gomock.Any()).Return(validJobStr, nil)
+	validTemplate6 := 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)
@@ -406,6 +408,8 @@ spec:
 		validTemplate2,
 		validTemplate3,
 		validTemplate4,
+		validTemplate5,
+		validTemplate6,
 		missedParameterTemplate,
 		oddParameterTemplate,
 		invalidParameterTemplate,
@@ -524,6 +528,27 @@ spec:
 			Err:             true,
 			testDescription: "Duplicate reference in Trial parameters",
 		},
+		// Trial template contains Trial parameters which weren't referenced from spec.parameters
+		{
+			Instance: func() *experimentsv1beta1.Experiment {
+				i := newFakeInstance()
+				i.Spec.TrialTemplate.TrialParameters[1].Reference = "wrong-ref"
+				return i
+			}(),
+			Err:             true,
+			testDescription: "Trial template contains Trial parameters which weren't referenced from spec.parameters",
+		},
+		// Trial template contains Trial parameters when spec.parameters is empty
+		{
+			Instance: func() *experimentsv1beta1.Experiment {
+				i := newFakeInstance()
+				i.Spec.Parameters = nil
+				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 doesn't contain parameter from trialParameters
 		// missedParameterTemplate case
 		{
@@ -1019,13 +1044,15 @@ func newFakeInstance() *experimentsv1beta1.Experiment {
 			},
 			Parameters: []experimentsv1beta1.ParameterSpec{
 				{
+					Name:          "lr",
 					ParameterType: experimentsv1beta1.ParameterTypeInt,
 					FeasibleSpace: experimentsv1beta1.FeasibleSpace{
 						Max: "5",
 						Min: "1",
 					},
 				},
 				{
+					Name:          "num-layers",
 					ParameterType: experimentsv1beta1.ParameterTypeCategorical,
 					FeasibleSpace: experimentsv1beta1.FeasibleSpace{
 						List: []string{"1", "2", "3"},