Merge pull request #460 from danielvegamyhre/k-val

Move JobSet webhook into same webhooks package as pod webhook

Makefile

@@ -82,9 +82,11 @@ help: ## Display this help.
 manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects.
 	$(CONTROLLER_GEN) \
 		rbac:roleName=manager-role output:rbac:artifacts:config=config/components/rbac\
-		crd:generateEmbeddedObjectMeta=true output:crd:artifacts:config=config/components/crd/bases\
+		paths="./api/..."
+	$(CONTROLLER_GEN) \
+		rbac:roleName=manager-role output:rbac:artifacts:config=config/components/rbac\
 		webhook output:webhook:artifacts:config=config/components/webhook\
-		paths="{./api/..., ./pkg/...}"
+		paths="./pkg/..."
 
 .PHONY: generate
 generate: controller-gen code-generator openapi-gen ## Generate code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations and client-go libraries.

go.mod

@@ -40,7 +40,7 @@ require (
 	github.com/google/gnostic-models v0.6.8 // indirect
 	github.com/google/gofuzz v1.2.0 // indirect
 	github.com/google/pprof v0.0.0-20210720184732-4bb14d4b1be1 // indirect
-	github.com/google/uuid v1.3.0 // indirect
+	github.com/google/uuid v1.3.1 // indirect
 	github.com/imdario/mergo v0.3.16 // indirect
 	github.com/josharian/intern v1.0.0 // indirect
 	github.com/json-iterator/go v1.1.12 // indirect
@@ -59,7 +59,7 @@ require (
 	go.uber.org/atomic v1.11.0 // indirect
 	go.uber.org/multierr v1.11.0 // indirect
 	go.uber.org/zap v1.26.0 // indirect
-	golang.org/x/exp v0.0.0-20220722155223-a9213eeb770e // indirect
+	golang.org/x/exp v0.0.0-20230905200255-921286631fa9 // indirect
 	golang.org/x/mod v0.14.0 // indirect
 	golang.org/x/net v0.20.0 // indirect
 	golang.org/x/oauth2 v0.12.0 // indirect

go.sum

@@ -51,8 +51,8 @@ github.com/google/gofuzz v1.2.0 h1:xRy4A+RhZaiKjJ1bPfwQ8sedCA+YS2YcCHW6ec7JMi0=
 github.com/google/gofuzz v1.2.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg=
 github.com/google/pprof v0.0.0-20210720184732-4bb14d4b1be1 h1:K6RDEckDVWvDI9JAJYCmNdQXq6neHJOYx3V6jnqNEec=
 github.com/google/pprof v0.0.0-20210720184732-4bb14d4b1be1/go.mod h1:kpwsk12EmLew5upagYY7GY0pfYCcupk39gWOCRROcvE=
-github.com/google/uuid v1.3.0 h1:t6JiXgmwXMjEs8VusXIJk2BXHsn+wx8BZdTaoZ5fu7I=
-github.com/google/uuid v1.3.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
+github.com/google/uuid v1.3.1 h1:KjJaJ9iWZ3jOFZIf1Lqf4laDRCasjl0BCmnEGxkdLb4=
+github.com/google/uuid v1.3.1/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
 github.com/ianlancetaylor/demangle v0.0.0-20200824232613-28f6c0f3b639/go.mod h1:aSSvb/t6k1mPoxDqO4vJh6VOCGPwU4O0C2/Eqndh1Sc=
 github.com/imdario/mergo v0.3.16 h1:wwQJbIsHYGMUyLSPrEq1CT16AhnhNJQ51+4fdHUnCl4=
 github.com/imdario/mergo v0.3.16/go.mod h1:WBLT9ZmE3lPoWsEzCh9LPo3TiwVN+ZKEjmz+hD27ysY=
@@ -128,8 +128,8 @@ go.uber.org/zap v1.26.0/go.mod h1:dtElttAiwGvoJ/vj4IwHBS/gXsEu/pZ50mUIRWuG0so=
 golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
 golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
 golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
-golang.org/x/exp v0.0.0-20220722155223-a9213eeb770e h1:+WEEuIdZHnUeJJmEUjyYC2gfUMj69yZXw17EnHg/otA=
-golang.org/x/exp v0.0.0-20220722155223-a9213eeb770e/go.mod h1:Kr81I6Kryrl9sr8s2FK3vxD90NdsKWRuOIl2O4CvYbA=
+golang.org/x/exp v0.0.0-20230905200255-921286631fa9 h1:GoHiUyI/Tp2nVkLI2mCxVkOjsbSXD66ic0XW0js0R9g=
+golang.org/x/exp v0.0.0-20230905200255-921286631fa9/go.mod h1:S2oDrQGGwySpoQPVqRShND87VCbxmc6bL1Yd2oYrm6k=
 golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
 golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
 golang.org/x/mod v0.14.0 h1:dGoOF9QVLYng8IHTm7BAyWqCqSheQ5pYWGhzW00YJr0=

main.go

@@ -158,15 +158,20 @@ func setupControllers(mgr ctrl.Manager, certsReady chan struct{}) {
 	}
 
 	// Set up JobSet validating/defaulting webhook.
-	if err := (&jobset.JobSet{}).SetupWebhookWithManager(mgr); err != nil {
+	jobSetWebHook, err := webhooks.NewJobSetWebhook(mgr.GetClient())
+	if err != nil {
 		setupLog.Error(err, "unable to create webhook", "webhook", "JobSet")
 		os.Exit(1)
 	}
+	if err := jobSetWebHook.SetupWebhookWithManager(mgr); err != nil {
+		setupLog.Error(err, "unable to set up webhook", "webhook", "JobSet")
+		os.Exit(1)
+	}
 
 	// Set up pod mutating and admission webhook.
-	podWebhook := webhooks.NewPodWebhook(mgr)
+	podWebhook := webhooks.NewPodWebhook(mgr.GetClient())
 	if err := podWebhook.SetupWebhookWithManager(mgr); err != nil {
-		setupLog.Error(err, "unable to create webhook", "webhook", "Pod")
+		setupLog.Error(err, "unable to set up webhook", "webhook", "Pod")
 		os.Exit(1)
 	}
 	//+kubebuilder:scaffold:builder

api/jobset/v1alpha2/jobset_webhook.go → pkg/webhooks/jobset_webhook.go

@@ -11,30 +11,33 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package v1alpha2
+package webhooks
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"math"
 	"strconv"
 	"strings"
 
+	batchv1 "k8s.io/api/batch/v1"
+	corev1 "k8s.io/api/core/v1"
 	apivalidation "k8s.io/apimachinery/pkg/api/validation"
+
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/utils/ptr"
 
 	ctrl "sigs.k8s.io/controller-runtime"
-	"sigs.k8s.io/controller-runtime/pkg/webhook"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	"sigs.k8s.io/jobset/pkg/util/collections"
 	"sigs.k8s.io/jobset/pkg/util/placement"
 
-	batchv1 "k8s.io/api/batch/v1"
-	corev1 "k8s.io/api/core/v1"
+	jobset "sigs.k8s.io/jobset/api/jobset/v1alpha2"
 )
 
 // maximum lnegth of the value of the managedBy field
@@ -58,24 +61,43 @@ const (
 	subdomainTooLongErrMsg = ".spec.network.subdomain is too long, must be less than 63 characters"
 )
 
-func (js *JobSet) SetupWebhookWithManager(mgr ctrl.Manager) error {
-	return ctrl.NewWebhookManagedBy(mgr).
-		For(js).
-		Complete()
+//+kubebuilder:webhook:path=/mutate-jobset-x-k8s-io-v1alpha2-jobset,mutating=true,failurePolicy=fail,sideEffects=None,groups=jobset.x-k8s.io,resources=jobsets,verbs=create;update,versions=v1alpha2,name=mjobset.kb.io,admissionReviewVersions=v1
+
+// jobSetWebhook for defaulting and admission.
+type jobSetWebhook struct {
+	client  client.Client
+	decoder *admission.Decoder
 }
 
-//+kubebuilder:webhook:path=/mutate-jobset-x-k8s-io-v1alpha2-jobset,mutating=true,failurePolicy=fail,sideEffects=None,groups=jobset.x-k8s.io,resources=jobsets,verbs=create;update,versions=v1alpha2,name=mjobset.kb.io,admissionReviewVersions=v1
+func NewJobSetWebhook(mgrClient client.Client) (*jobSetWebhook, error) {
+	return &jobSetWebhook{client: mgrClient}, nil
+}
+
+// InjectDecoder injects the decoder into the jobSetWebhook.
+func (j *jobSetWebhook) InjectDecoder(d *admission.Decoder) error {
+	j.decoder = d
+	return nil
+}
 
-var _ webhook.Defaulter = &JobSet{}
+func (j *jobSetWebhook) SetupWebhookWithManager(mgr ctrl.Manager) error {
+	return ctrl.NewWebhookManagedBy(mgr).
+		For(&jobset.JobSet{}).
+		WithDefaulter(j).
+		WithValidator(j).
+		Complete()
+}
 
-// Default implements webhook.Defaulter so a webhook will be registered for the type
-func (js *JobSet) Default() {
+func (j *jobSetWebhook) Default(ctx context.Context, obj runtime.Object) error {
+	js, ok := obj.(*jobset.JobSet)
+	if !ok {
+		return nil
+	}
 	// Default success policy to operator "All" targeting all replicatedJobs.
 	if js.Spec.SuccessPolicy == nil {
-		js.Spec.SuccessPolicy = &SuccessPolicy{Operator: OperatorAll}
+		js.Spec.SuccessPolicy = &jobset.SuccessPolicy{Operator: jobset.OperatorAll}
 	}
 	if js.Spec.StartupPolicy == nil {
-		js.Spec.StartupPolicy = &StartupPolicy{StartupPolicyOrder: AnyOrder}
+		js.Spec.StartupPolicy = &jobset.StartupPolicy{StartupPolicyOrder: jobset.AnyOrder}
 	}
 	for i := range js.Spec.ReplicatedJobs {
 		// Default job completion mode to indexed.
@@ -84,7 +106,7 @@ func (js *JobSet) Default() {
 		}
 		// Enable DNS hostnames by default.
 		if js.Spec.Network == nil {
-			js.Spec.Network = &Network{}
+			js.Spec.Network = &jobset.Network{}
 		}
 		if js.Spec.Network.EnableDNSHostnames == nil {
 			js.Spec.Network.EnableDNSHostnames = ptr.To(true)
@@ -97,16 +119,20 @@ func (js *JobSet) Default() {
 	}
 
 	if js.Spec.ManagedBy == nil {
-		js.Spec.ManagedBy = ptr.To(JobSetControllerName)
+		js.Spec.ManagedBy = ptr.To(jobset.JobSetControllerName)
 	}
+	return nil
 }
 
 //+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
 
-var _ webhook.Validator = &JobSet{}
-
 // ValidateCreate implements webhook.Validator so a webhook will be registered for the type
-func (js *JobSet) ValidateCreate() (admission.Warnings, error) {
+func (j *jobSetWebhook) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
+	js, ok := obj.(*jobset.JobSet)
+	if !ok {
+		return nil, fmt.Errorf("expected a JobSet but got a %T", obj)
+	}
+
 	var allErrs []error
 	// Validate that replicatedJobs listed in success policy are part of this JobSet.
 	validReplicatedJobs := replicatedJobNamesFromSpec(js)
@@ -128,6 +154,7 @@ func (js *JobSet) ValidateCreate() (admission.Warnings, error) {
 		}
 	}
 
+	// Validate the managedBy field used for multi-kueue support.
 	if js.Spec.ManagedBy != nil {
 		manager := *js.Spec.ManagedBy
 		fieldPath := field.NewPath("spec", "managedBy")
@@ -139,6 +166,7 @@ func (js *JobSet) ValidateCreate() (admission.Warnings, error) {
 		}
 	}
 
+	// Validate each replicatedJob.
 	for _, rjob := range js.Spec.ReplicatedJobs {
 		var parallelism int32 = 1
 		if rjob.Template.Spec.Parallelism != nil {
@@ -147,6 +175,7 @@ func (js *JobSet) ValidateCreate() (admission.Warnings, error) {
 		if int64(parallelism)*int64(rjob.Replicas) > math.MaxInt32 {
 			allErrs = append(allErrs, fmt.Errorf("the product of replicas and parallelism must not exceed %d for replicatedJob '%s'", math.MaxInt32, rjob.Name))
 		}
+
 		// Check that the generated job names for this replicated job will be DNS 1035 compliant.
 		// Use the largest job index as it will have the longest name.
 		longestJobName := placement.GenJobName(js.Name, rjob.Name, int(rjob.Replicas-1))
@@ -171,6 +200,8 @@ func (js *JobSet) ValidateCreate() (admission.Warnings, error) {
 			}
 		}
 	}
+
+	// Validate the success policy's target replicated jobs are valid.
 	for _, rjobName := range js.Spec.SuccessPolicy.TargetReplicatedJobs {
 		if !collections.Contains(validReplicatedJobs, rjobName) {
 			allErrs = append(allErrs, fmt.Errorf("invalid replicatedJob name '%s' does not appear in .spec.ReplicatedJobs", rjobName))
@@ -180,9 +211,16 @@ func (js *JobSet) ValidateCreate() (admission.Warnings, error) {
 }
 
 // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
-func (js *JobSet) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
+func (j *jobSetWebhook) ValidateUpdate(ctx context.Context, old, newObj runtime.Object) (admission.Warnings, error) {
+	js, ok := newObj.(*jobset.JobSet)
+	if !ok {
+		return nil, fmt.Errorf("expected a JobSet but got a %T", newObj)
+	}
+	oldJS, ok := old.(*jobset.JobSet)
+	if !ok {
+		return nil, fmt.Errorf("expected a JobSet from old object but got a %T", old)
+	}
 	mungedSpec := js.Spec.DeepCopy()
-	oldJS := old.(*JobSet)
 	if ptr.Deref(oldJS.Spec.Suspend, false) {
 		for index := range js.Spec.ReplicatedJobs {
 			mungedSpec.ReplicatedJobs[index].Template.Spec.Template.Spec.NodeSelector = oldJS.Spec.ReplicatedJobs[index].Template.Spec.Template.Spec.NodeSelector
@@ -195,15 +233,15 @@ func (js *JobSet) ValidateUpdate(old runtime.Object) (admission.Warnings, error)
 }
 
 // ValidateDelete implements webhook.Validator so a webhook will be registered for the type
-func (js *JobSet) ValidateDelete() (admission.Warnings, error) {
+func (j *jobSetWebhook) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
 	return nil, nil
 }
 
 func completionModePtr(mode batchv1.CompletionMode) *batchv1.CompletionMode {
 	return &mode
 }
 
-func replicatedJobNamesFromSpec(js *JobSet) []string {
+func replicatedJobNamesFromSpec(js *jobset.JobSet) []string {
 	names := []string{}
 	for _, rjob := range js.Spec.ReplicatedJobs {
 		names = append(names, rjob.Name)