From f7f3b2839a26f2dbcf5f5bb4591ac7758ad525f4 Mon Sep 17 00:00:00 2001 From: Daniel Vega-Myhre <105610547+danielvegamyhre@users.noreply.github.com> Date: Sat, 10 Aug 2024 15:55:58 -0700 Subject: [PATCH] refactor jobset webhook (#646) --- pkg/webhooks/jobset_webhook.go | 134 +++++++++++++++++---------------- 1 file changed, 70 insertions(+), 64 deletions(-) diff --git a/pkg/webhooks/jobset_webhook.go b/pkg/webhooks/jobset_webhook.go index 9678fcc6..8c7588de 100644 --- a/pkg/webhooks/jobset_webhook.go +++ b/pkg/webhooks/jobset_webhook.go @@ -151,62 +151,6 @@ func (j *jobSetWebhook) Default(ctx context.Context, obj runtime.Object) error { //+kubebuilder:webhook:path=/validate-jobset-x-k8s-io-v1alpha2-jobset,mutating=false,failurePolicy=fail,sideEffects=None,groups=jobset.x-k8s.io,resources=jobsets,verbs=create;update,versions=v1alpha2,name=vjobset.kb.io,admissionReviewVersions=v1 -const minRuleNameLength = 1 -const maxRuleNameLength = 128 -const ruleNameFmt = "^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$" - -var ruleNameRegexp = regexp.MustCompile(ruleNameFmt) - -// validateFailurePolicy performs validation for jobset failure policies and returns all errors detected. -func validateFailurePolicy(failurePolicy *jobset.FailurePolicy, validReplicatedJobs []string) []error { - var allErrs []error - if failurePolicy == nil { - return allErrs - } - - // ruleNameToRulesWithName is used to verify that rule names are unique - ruleNameToRulesWithName := make(map[string][]int) - for index, rule := range failurePolicy.Rules { - // Check that the rule name meets the minimum length - nameLen := len(rule.Name) - if !(minRuleNameLength <= nameLen && nameLen <= maxRuleNameLength) { - err := fmt.Errorf("invalid failure policy rule name of length %v, the rule name must be at least %v characters long and at most %v characters long", nameLen, minRuleNameLength, maxRuleNameLength) - allErrs = append(allErrs, err) - } - - ruleNameToRulesWithName[rule.Name] = append(ruleNameToRulesWithName[rule.Name], index) - - if !ruleNameRegexp.MatchString(rule.Name) { - err := fmt.Errorf("invalid failure policy rule name '%v', a failure policy rule name must start with an alphabetic character, optionally followed by a string of alphanumeric characters or '_,:', and must end with an alphanumeric character or '_'", rule.Name) - allErrs = append(allErrs, err) - } - - // Validate the rules target replicated jobs are valid - for _, rjobName := range rule.TargetReplicatedJobs { - if !collections.Contains(validReplicatedJobs, rjobName) { - allErrs = append(allErrs, fmt.Errorf("invalid replicatedJob name '%s' in failure policy does not appear in .spec.ReplicatedJobs", rjobName)) - } - } - - // Validate the rules on job failure reasons are valid - for _, failureReason := range rule.OnJobFailureReasons { - if !collections.Contains(validOnJobFailureReasons, failureReason) { - allErrs = append(allErrs, fmt.Errorf("invalid job failure reason '%s' in failure policy is not a recognized job failure reason", failureReason)) - } - } - } - - // Checking that rule names are unique - for ruleName, rulesWithName := range ruleNameToRulesWithName { - if len(rulesWithName) > 1 { - err := fmt.Errorf("rule names are not unique, rules with indices %v all have the same name '%v'", rulesWithName, ruleName) - allErrs = append(allErrs, err) - } - } - - return allErrs -} - // ValidateCreate implements webhook.Validator so a webhook will be registered for the type func (j *jobSetWebhook) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { js, ok := obj.(*jobset.JobSet) @@ -340,16 +284,64 @@ func (j *jobSetWebhook) ValidateDelete(ctx context.Context, obj runtime.Object) return nil, nil } -func completionModePtr(mode batchv1.CompletionMode) *batchv1.CompletionMode { - return &mode -} +// Failure policy constants. +const ( + minRuleNameLength = 1 + maxRuleNameLength = 128 + ruleNameFmt = "^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$" +) -func replicatedJobNamesFromSpec(js *jobset.JobSet) []string { - names := []string{} - for _, rjob := range js.Spec.ReplicatedJobs { - names = append(names, rjob.Name) +// ruleNameRegexp is the regular expression that failure policy rules must match. +var ruleNameRegexp = regexp.MustCompile(ruleNameFmt) + +// validateFailurePolicy performs validation for jobset failure policies and returns all errors detected. +func validateFailurePolicy(failurePolicy *jobset.FailurePolicy, validReplicatedJobs []string) []error { + var allErrs []error + if failurePolicy == nil { + return allErrs } - return names + + // ruleNameToRulesWithName is used to verify that rule names are unique + ruleNameToRulesWithName := make(map[string][]int) + for index, rule := range failurePolicy.Rules { + // Check that the rule name meets the minimum length + nameLen := len(rule.Name) + if !(minRuleNameLength <= nameLen && nameLen <= maxRuleNameLength) { + err := fmt.Errorf("invalid failure policy rule name of length %v, the rule name must be at least %v characters long and at most %v characters long", nameLen, minRuleNameLength, maxRuleNameLength) + allErrs = append(allErrs, err) + } + + ruleNameToRulesWithName[rule.Name] = append(ruleNameToRulesWithName[rule.Name], index) + + if !ruleNameRegexp.MatchString(rule.Name) { + err := fmt.Errorf("invalid failure policy rule name '%v', a failure policy rule name must start with an alphabetic character, optionally followed by a string of alphanumeric characters or '_,:', and must end with an alphanumeric character or '_'", rule.Name) + allErrs = append(allErrs, err) + } + + // Validate the rules target replicated jobs are valid + for _, rjobName := range rule.TargetReplicatedJobs { + if !collections.Contains(validReplicatedJobs, rjobName) { + allErrs = append(allErrs, fmt.Errorf("invalid replicatedJob name '%s' in failure policy does not appear in .spec.ReplicatedJobs", rjobName)) + } + } + + // Validate the rules on job failure reasons are valid + for _, failureReason := range rule.OnJobFailureReasons { + if !collections.Contains(validOnJobFailureReasons, failureReason) { + allErrs = append(allErrs, fmt.Errorf("invalid job failure reason '%s' in failure policy is not a recognized job failure reason", failureReason)) + } + } + } + + // Checking that rule names are unique + for ruleName, rulesWithName := range ruleNameToRulesWithName { + if len(rulesWithName) > 1 { + err := fmt.Errorf("rule names are not unique, rules with indices %v all have the same name '%v'", rulesWithName, ruleName) + allErrs = append(allErrs, err) + } + } + + return allErrs } // validateCoordinator validates the following: @@ -390,3 +382,17 @@ func replicatedJobByName(js *jobset.JobSet, replicatedJob string) *jobset.Replic } return nil } + +// replicatedJobNamesFromSpec parses the JobSet spec and returns a list of +// the replicatedJob names. +func replicatedJobNamesFromSpec(js *jobset.JobSet) []string { + names := []string{} + for _, rjob := range js.Spec.ReplicatedJobs { + names = append(names, rjob.Name) + } + return names +} + +func completionModePtr(mode batchv1.CompletionMode) *batchv1.CompletionMode { + return &mode +}