refactor jobset webhook (#646)

kubernetes-sigs · Aug 10, 2024 · f7f3b28 · f7f3b28
1 parent 8bade1e
commit f7f3b28
Showing 1 changed file with 70 additions and 64 deletions.
diff --git 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
+}