From 8b6fc58875a9f5e057cea853763adf89caa2b3b3 Mon Sep 17 00:00:00 2001 From: Aleksei Gorobets Date: Mon, 11 Apr 2022 19:01:28 -0400 Subject: [PATCH 1/4] do not check trial parameter in experiment parameters if it's trial's metadata --- pkg/controller.v1beta1/consts/const.go | 10 ++ .../v1beta1/experiment/validator/validator.go | 18 +++- .../experiment/validator/validator_test.go | 94 ++++--------------- 3 files changed, 45 insertions(+), 77 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..5011e2e69f9 100644 --- a/pkg/webhook/v1beta1/experiment/validator/validator.go +++ b/pkg/webhook/v1beta1/experiment/validator/validator.go @@ -311,8 +311,13 @@ 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) + // Check if parameter is trial metadata + regex := regexp.MustCompile(consts.TrialTemplateMetaReplaceFormatRegex) + match := regex.FindStringSubmatch(parameter.Reference) + if !(len(match) > 0 && contains(consts.TrialTemplateMetaKeys, match[1])) { + 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 +486,12 @@ func (g *DefaultValidator) validateMetricsCollector(inst *experimentsv1beta1.Exp return nil } + +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..1cc103918f4 100644 --- a/pkg/webhook/v1beta1/experiment/validator/validator_test.go +++ b/pkg/webhook/v1beta1/experiment/validator/validator_test.go @@ -415,6 +415,7 @@ 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) missedParameterTemplate := p.EXPECT().GetTrialTemplate(gomock.Any()).Return(missedParameterJobStr, nil) oddParameterTemplate := p.EXPECT().GetTrialTemplate(gomock.Any()).Return(oddParameterJobStr, nil) @@ -431,6 +432,7 @@ spec: validTemplate4, validTemplate5, validTemplate6, + validTemplate7, missedParameterTemplate, oddParameterTemplate, invalidParameterTemplate, @@ -570,6 +572,16 @@ 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", + }, // Trial Template doesn't contain parameter from trialParameters // missedParameterTemplate case { @@ -798,8 +810,7 @@ func TestValidateMetricsCollector(t *testing.T) { }, Source: &commonv1beta1.SourceSpec{ FileSystemPath: &commonv1beta1.FileSystemPath{ - Path: "not/absolute/path", - Format: commonv1beta1.TextFormat, + Path: "not/absolute/path", }, }, } @@ -827,27 +838,6 @@ func TestValidateMetricsCollector(t *testing.T) { Err: true, testDescription: "Invalid path for TF event metrics collector", }, - // TfEventCollector invalid file format - { - Instance: func() *experimentsv1beta1.Experiment { - i := newFakeInstance() - i.Spec.MetricsCollectorSpec = &commonv1beta1.MetricsCollectorSpec{ - Collector: &commonv1beta1.CollectorSpec{ - Kind: commonv1beta1.TfEventCollector, - }, - Source: &commonv1beta1.SourceSpec{ - FileSystemPath: &commonv1beta1.FileSystemPath{ - Path: "/absolute/path", - Format: commonv1beta1.JsonFormat, - Kind: commonv1beta1.DirectoryKind, - }, - }, - } - return i - }(), - Err: true, - testDescription: "Invalid file format for TF event metrics collector", - }, // PrometheusMetricCollector invalid Port { Instance: func() *experimentsv1beta1.Experiment { @@ -942,9 +932,8 @@ func TestValidateMetricsCollector(t *testing.T) { }, }, FileSystemPath: &commonv1beta1.FileSystemPath{ - Path: "/absolute/path", - Kind: commonv1beta1.FileKind, - Format: commonv1beta1.TextFormat, + Path: "/absolute/path", + Kind: commonv1beta1.FileKind, }, }, } @@ -968,9 +957,8 @@ func TestValidateMetricsCollector(t *testing.T) { }, }, FileSystemPath: &commonv1beta1.FileSystemPath{ - Path: "/absolute/path", - Kind: commonv1beta1.FileKind, - Format: commonv1beta1.TextFormat, + Path: "/absolute/path", + Kind: commonv1beta1.FileKind, }, }, } @@ -979,49 +967,6 @@ func TestValidateMetricsCollector(t *testing.T) { Err: true, testDescription: "One subexpression in metrics format", }, - // FileMetricCollector invalid file format - { - Instance: func() *experimentsv1beta1.Experiment { - i := newFakeInstance() - i.Spec.MetricsCollectorSpec = &commonv1beta1.MetricsCollectorSpec{ - Collector: &commonv1beta1.CollectorSpec{ - Kind: commonv1beta1.FileCollector, - }, - Source: &commonv1beta1.SourceSpec{ - FileSystemPath: &commonv1beta1.FileSystemPath{ - Path: "/absolute/path", - Kind: commonv1beta1.FileKind, - Format: "invalid", - }, - }, - } - return i - }(), - Err: true, - testDescription: "Invalid file format for File metrics collector", - }, - // FileMetricCollector invalid metrics filter - { - Instance: func() *experimentsv1beta1.Experiment { - i := newFakeInstance() - i.Spec.MetricsCollectorSpec = &commonv1beta1.MetricsCollectorSpec{ - Collector: &commonv1beta1.CollectorSpec{ - Kind: commonv1beta1.FileCollector, - }, - Source: &commonv1beta1.SourceSpec{ - Filter: &commonv1beta1.FilterSpec{}, - FileSystemPath: &commonv1beta1.FileSystemPath{ - Path: "/absolute/path", - Kind: commonv1beta1.FileKind, - Format: commonv1beta1.JsonFormat, - }, - }, - } - return i - }(), - Err: true, - testDescription: "Invalid metrics filer for File metrics collector when file format is `JSON`", - }, // Valid FileMetricCollector { Instance: func() *experimentsv1beta1.Experiment { @@ -1032,9 +977,8 @@ func TestValidateMetricsCollector(t *testing.T) { }, Source: &commonv1beta1.SourceSpec{ FileSystemPath: &commonv1beta1.FileSystemPath{ - Path: "/absolute/path", - Kind: commonv1beta1.FileKind, - Format: commonv1beta1.JsonFormat, + Path: "/absolute/path", + Kind: commonv1beta1.FileKind, }, }, } From 6d4ce0f1101efc75eab40958f89965d0d25bdd11 Mon Sep 17 00:00:00 2001 From: Aleksei Gorobets Date: Mon, 11 Apr 2022 19:11:25 -0400 Subject: [PATCH 2/4] revert unnecessary change --- .../experiment/validator/validator_test.go | 82 +++++++++++++++++-- 1 file changed, 75 insertions(+), 7 deletions(-) diff --git a/pkg/webhook/v1beta1/experiment/validator/validator_test.go b/pkg/webhook/v1beta1/experiment/validator/validator_test.go index 1cc103918f4..67ae47e1558 100644 --- a/pkg/webhook/v1beta1/experiment/validator/validator_test.go +++ b/pkg/webhook/v1beta1/experiment/validator/validator_test.go @@ -810,7 +810,8 @@ func TestValidateMetricsCollector(t *testing.T) { }, Source: &commonv1beta1.SourceSpec{ FileSystemPath: &commonv1beta1.FileSystemPath{ - Path: "not/absolute/path", + Path: "not/absolute/path", + Format: commonv1beta1.TextFormat, }, }, } @@ -838,6 +839,27 @@ func TestValidateMetricsCollector(t *testing.T) { Err: true, testDescription: "Invalid path for TF event metrics collector", }, + // TfEventCollector invalid file format + { + Instance: func() *experimentsv1beta1.Experiment { + i := newFakeInstance() + i.Spec.MetricsCollectorSpec = &commonv1beta1.MetricsCollectorSpec{ + Collector: &commonv1beta1.CollectorSpec{ + Kind: commonv1beta1.TfEventCollector, + }, + Source: &commonv1beta1.SourceSpec{ + FileSystemPath: &commonv1beta1.FileSystemPath{ + Path: "/absolute/path", + Format: commonv1beta1.JsonFormat, + Kind: commonv1beta1.DirectoryKind, + }, + }, + } + return i + }(), + Err: true, + testDescription: "Invalid file format for TF event metrics collector", + }, // PrometheusMetricCollector invalid Port { Instance: func() *experimentsv1beta1.Experiment { @@ -932,8 +954,9 @@ func TestValidateMetricsCollector(t *testing.T) { }, }, FileSystemPath: &commonv1beta1.FileSystemPath{ - Path: "/absolute/path", - Kind: commonv1beta1.FileKind, + Path: "/absolute/path", + Kind: commonv1beta1.FileKind, + Format: commonv1beta1.TextFormat, }, }, } @@ -957,8 +980,9 @@ func TestValidateMetricsCollector(t *testing.T) { }, }, FileSystemPath: &commonv1beta1.FileSystemPath{ - Path: "/absolute/path", - Kind: commonv1beta1.FileKind, + Path: "/absolute/path", + Kind: commonv1beta1.FileKind, + Format: commonv1beta1.TextFormat, }, }, } @@ -967,6 +991,49 @@ func TestValidateMetricsCollector(t *testing.T) { Err: true, testDescription: "One subexpression in metrics format", }, + // FileMetricCollector invalid file format + { + Instance: func() *experimentsv1beta1.Experiment { + i := newFakeInstance() + i.Spec.MetricsCollectorSpec = &commonv1beta1.MetricsCollectorSpec{ + Collector: &commonv1beta1.CollectorSpec{ + Kind: commonv1beta1.FileCollector, + }, + Source: &commonv1beta1.SourceSpec{ + FileSystemPath: &commonv1beta1.FileSystemPath{ + Path: "/absolute/path", + Kind: commonv1beta1.FileKind, + Format: "invalid", + }, + }, + } + return i + }(), + Err: true, + testDescription: "Invalid file format for File metrics collector", + }, + // FileMetricCollector invalid metrics filter + { + Instance: func() *experimentsv1beta1.Experiment { + i := newFakeInstance() + i.Spec.MetricsCollectorSpec = &commonv1beta1.MetricsCollectorSpec{ + Collector: &commonv1beta1.CollectorSpec{ + Kind: commonv1beta1.FileCollector, + }, + Source: &commonv1beta1.SourceSpec{ + Filter: &commonv1beta1.FilterSpec{}, + FileSystemPath: &commonv1beta1.FileSystemPath{ + Path: "/absolute/path", + Kind: commonv1beta1.FileKind, + Format: commonv1beta1.JsonFormat, + }, + }, + } + return i + }(), + Err: true, + testDescription: "Invalid metrics filer for File metrics collector when file format is `JSON`", + }, // Valid FileMetricCollector { Instance: func() *experimentsv1beta1.Experiment { @@ -977,8 +1044,9 @@ func TestValidateMetricsCollector(t *testing.T) { }, Source: &commonv1beta1.SourceSpec{ FileSystemPath: &commonv1beta1.FileSystemPath{ - Path: "/absolute/path", - Kind: commonv1beta1.FileKind, + Path: "/absolute/path", + Kind: commonv1beta1.FileKind, + Format: commonv1beta1.JsonFormat, }, }, } From 07725af5758bdd48058f51e6926982c4ce3b149c Mon Sep 17 00:00:00 2001 From: Aleksei Gorobets Date: Fri, 15 Apr 2022 18:37:33 -0400 Subject: [PATCH 3/4] add handle Labels[label] and Annotations[annotation] --- .../v1beta1/experiment/validator/validator.go | 24 +++++++++++++++---- .../experiment/validator/validator_test.go | 22 +++++++++++++++++ 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/pkg/webhook/v1beta1/experiment/validator/validator.go b/pkg/webhook/v1beta1/experiment/validator/validator.go index 5011e2e69f9..68f56f100a4 100644 --- a/pkg/webhook/v1beta1/experiment/validator/validator.go +++ b/pkg/webhook/v1beta1/experiment/validator/validator.go @@ -311,10 +311,7 @@ func (g *DefaultValidator) validateTrialTemplate(instance *experimentsv1beta1.Ex // Check if parameter reference exist in experiment parameters if len(experimentParameterNames) > 0 { - // Check if parameter is trial metadata - regex := regexp.MustCompile(consts.TrialTemplateMetaReplaceFormatRegex) - match := regex.FindStringSubmatch(parameter.Reference) - if !(len(match) > 0 && contains(consts.TrialTemplateMetaKeys, match[1])) { + 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) } @@ -487,6 +484,25 @@ 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 { diff --git a/pkg/webhook/v1beta1/experiment/validator/validator_test.go b/pkg/webhook/v1beta1/experiment/validator/validator_test.go index 67ae47e1558..dca7c5a6166 100644 --- a/pkg/webhook/v1beta1/experiment/validator/validator_test.go +++ b/pkg/webhook/v1beta1/experiment/validator/validator_test.go @@ -416,6 +416,8 @@ spec: 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) @@ -433,6 +435,8 @@ spec: validTemplate5, validTemplate6, validTemplate7, + validTemplate8, + validTemplate9, missedParameterTemplate, oddParameterTemplate, invalidParameterTemplate, @@ -582,6 +586,24 @@ spec: 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 metadata 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 metadata reference as parameter", + }, // Trial Template doesn't contain parameter from trialParameters // missedParameterTemplate case { From 72db2a5dd625ecd32910334f5ed233614ae5d2c7 Mon Sep 17 00:00:00 2001 From: Aleksei Gorobets Date: Fri, 15 Apr 2022 18:40:29 -0400 Subject: [PATCH 4/4] fix test description --- pkg/webhook/v1beta1/experiment/validator/validator_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/webhook/v1beta1/experiment/validator/validator_test.go b/pkg/webhook/v1beta1/experiment/validator/validator_test.go index dca7c5a6166..85ea6226e64 100644 --- a/pkg/webhook/v1beta1/experiment/validator/validator_test.go +++ b/pkg/webhook/v1beta1/experiment/validator/validator_test.go @@ -593,7 +593,7 @@ spec: return i }(), Err: false, - testDescription: "Trial template contains Trial metadata reference as parameter", + testDescription: "Trial template contains Trial annotation reference as parameter", }, { Instance: func() *experimentsv1beta1.Experiment { @@ -602,7 +602,7 @@ spec: return i }(), Err: false, - testDescription: "Trial template contains Trial metadata reference as parameter", + testDescription: "Trial template contains Trial's label reference as parameter", }, // Trial Template doesn't contain parameter from trialParameters // missedParameterTemplate case