From 7f1afbdaa93591baefdc6ef29afe5a5f2ce5e95b Mon Sep 17 00:00:00 2001
From: Alexey Gorobets <agorobets@d2iq.com>
Date: Sun, 17 Apr 2022 08:52:41 -0400
Subject: [PATCH] Don't check if trial's metadata is in spec.parameters 
 (#1848)

* do not check trial parameter in experiment parameters if it's trial's metadata

* revert unnecessary change

* add handle Labels[label] and Annotations[annotation]

* fix test description
---
 pkg/controller.v1beta1/consts/const.go        | 10 ++++++
 .../v1beta1/experiment/validator/validator.go | 34 +++++++++++++++++--
 .../experiment/validator/validator_test.go    | 34 +++++++++++++++++++
 3 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/pkg/controller.v1beta1/consts/const.go b/pkg/controller.v1beta1/consts/const.go
index 0fd74a6789c..1a09081847d 100644
--- a/pkg/controller.v1beta1/consts/const.go
+++ b/pkg/controller.v1beta1/consts/const.go
@@ -179,4 +179,14 @@ var (
 	DefaultKatibDBManagerServiceIP = env.GetEnvOrDefault(DefaultKatibDBManagerServiceIPEnvName, "katib-db-manager")
 	// DefaultKatibDBManagerServicePort is the default Port of Katib DB Manager
 	DefaultKatibDBManagerServicePort = env.GetEnvOrDefault(DefaultKatibDBManagerServicePortEnvName, "6789")
+
+	// List of all valid keys of trial metadata for substitution in Trial template
+	TrialTemplateMetaKeys = []string{
+		TrialTemplateMetaKeyOfName,
+		TrialTemplateMetaKeyOfNamespace,
+		TrialTemplateMetaKeyOfKind,
+		TrialTemplateMetaKeyOfAPIVersion,
+		TrialTemplateMetaKeyOfAnnotations,
+		TrialTemplateMetaKeyOfLabels,
+	}
 )
diff --git a/pkg/webhook/v1beta1/experiment/validator/validator.go b/pkg/webhook/v1beta1/experiment/validator/validator.go
index c8990080833..68f56f100a4 100644
--- a/pkg/webhook/v1beta1/experiment/validator/validator.go
+++ b/pkg/webhook/v1beta1/experiment/validator/validator.go
@@ -311,8 +311,10 @@ func (g *DefaultValidator) validateTrialTemplate(instance *experimentsv1beta1.Ex
 
 		// 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)
+			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)
+				}
 			}
 		}
 
@@ -481,3 +483,31 @@ func (g *DefaultValidator) validateMetricsCollector(inst *experimentsv1beta1.Exp
 
 	return nil
 }
+
+func isMetaKey(parameter string) bool {
+	// Check if parameter is trial metadata reference as ${trailSpec.Name}, ${trialSpec.Labels[label]}, etc. used for substitution
+	match := regexp.MustCompile(consts.TrialTemplateMetaReplaceFormatRegex).FindStringSubmatch(parameter)
+	isMeta := false
+	if len(match) > 0 {
+		matchedKey := match[1]
+		if contains(consts.TrialTemplateMetaKeys, matchedKey) {
+			isMeta = true
+		} else {
+			// Check if it's Labels[label] or Annotations[annotation]
+			subMatch := regexp.MustCompile(consts.TrialTemplateMetaParseFormatRegex).FindStringSubmatch(matchedKey)
+			if len(subMatch) == 3 && contains(consts.TrialTemplateMetaKeys, subMatch[1]) {
+				isMeta = true
+			}
+		}
+	}
+	return isMeta
+}
+
+func contains(slice []string, item string) bool {
+	for _, s := range slice {
+		if s == item {
+			return true
+		}
+	}
+	return false
+}
diff --git a/pkg/webhook/v1beta1/experiment/validator/validator_test.go b/pkg/webhook/v1beta1/experiment/validator/validator_test.go
index 1f56088df15..85ea6226e64 100644
--- a/pkg/webhook/v1beta1/experiment/validator/validator_test.go
+++ b/pkg/webhook/v1beta1/experiment/validator/validator_test.go
@@ -415,6 +415,9 @@ spec:
 	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)
+	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)
 
 	missedParameterTemplate := p.EXPECT().GetTrialTemplate(gomock.Any()).Return(missedParameterJobStr, nil)
 	oddParameterTemplate := p.EXPECT().GetTrialTemplate(gomock.Any()).Return(oddParameterJobStr, nil)
@@ -431,6 +434,9 @@ spec:
 		validTemplate4,
 		validTemplate5,
 		validTemplate6,
+		validTemplate7,
+		validTemplate8,
+		validTemplate9,
 		missedParameterTemplate,
 		oddParameterTemplate,
 		invalidParameterTemplate,
@@ -570,6 +576,34 @@ spec:
 			Err:             false,
 			testDescription: "Trial template contains Trial parameters when spec.parameters is empty",
 		},
+		// Trial template contains Trial metadata parameter substitution
+		{
+			Instance: func() *experimentsv1beta1.Experiment {
+				i := newFakeInstance()
+				i.Spec.TrialTemplate.TrialParameters[1].Reference = "${trialSpec.Name}"
+				return i
+			}(),
+			Err:             false,
+			testDescription: "Trial template contains Trial metadata reference as parameter",
+		},
+		{
+			Instance: func() *experimentsv1beta1.Experiment {
+				i := newFakeInstance()
+				i.Spec.TrialTemplate.TrialParameters[1].Reference = "${trialSpec.Annotations[test-annotation]}"
+				return i
+			}(),
+			Err:             false,
+			testDescription: "Trial template contains Trial annotation reference as parameter",
+		},
+		{
+			Instance: func() *experimentsv1beta1.Experiment {
+				i := newFakeInstance()
+				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
 		// missedParameterTemplate case
 		{