Repository: kubeflow/training-operator (language: python)

Issue:
Introduce Webhook Validation
As described in https://github.com/kubeflow/training-operator/issues/1708#issuecomment-1661876525, we can not introduce CEL validations for fields under the `.spec.replicaSpec` due to the CEL cost budget violations. The root cause is that our `replicaSpec` is defined as a typed `map`, and then Kubernetes can not estimate how many maximum replicas will be held in the `.spec.replicas`.

Also, current validations aren't better for UX since the users can find validation errors only in the training-operator logs.

So, I would suggest introducing the webhook validations with internal certs mechanisms like Katib since validations help to find validation errors for users and the internal cert mechanisms could reduce installation costs.

/kind feature

- [x] PyTorch: #2035
- [x] TensorFlow: #2051
- [x] XGBoost: #2052
- [x] MXNet: #2058
- [x] PaddlePaddle: #2057
- [x] MPIJob: No Plan 



Discussion / hints:
/assign
I'm wondering if we should not implement the webhook validations for the MXJob because we will remove the MXJob.

@kubeflow/wg-training-leads WDYT?

> I'm wondering if we should not implement the webhook validations for the MXJob because we will remove the MXJob.
> 
> @kubeflow/wg-training-leads WDYT?

Based on the last community call, we decided not to implement webhook validations for MXJob, but we will implement webhook warnings to notify deprecations.

Yes, that's correct @tenzen-y!
Let's just add the warning for users via webhook.
@andreyvelich @johnugeorge For MPIJob v1, I'm wondering if we can skip to implement webhooks.
WDYT?
I am fine to not add them since we will be focus on V2 for the long-term

Thank you for checking this.
So, I will skip the webhook implementations for the MPIJob v1.


Fix patch:
diff --git a/manifests/base/webhook/manifests.yaml b/manifests/base/webhook/manifests.yaml
index c8a69845b7..3447065fa4 100644
--- a/manifests/base/webhook/manifests.yaml
+++ b/manifests/base/webhook/manifests.yaml
@@ -4,6 +4,27 @@ kind: ValidatingWebhookConfiguration
 metadata:
   name: validating-webhook-configuration
 webhooks:
+- admissionReviewVersions:
+  - v1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: system
+      path: /validate-kubeflow-org-v1-mxjob
+  failurePolicy: Fail
+  name: validator.mxjob.training-operator.kubeflow.org
+  rules:
+  - apiGroups:
+    - kubeflow.org
+    apiVersions:
+    - v1
+    operations:
+    - CREATE
+    - UPDATE
+    - DELETE
+    resources:
+    - mxjobs
+  sideEffects: None
 - admissionReviewVersions:
   - v1
   clientConfig:
diff --git a/manifests/base/webhook/patch.yaml b/manifests/base/webhook/patch.yaml
index a02b11bf1c..b103423df2 100644
--- a/manifests/base/webhook/patch.yaml
+++ b/manifests/base/webhook/patch.yaml
@@ -10,6 +10,9 @@
 - op: replace
   path: /webhooks/3/clientConfig/service/name
   value: training-operator
+- op: replace
+  path: /webhooks/4/clientConfig/service/name
+  value: training-operator
 - op: replace
   path: /metadata/name
   value: validator.training-operator.kubeflow.org
diff --git a/pkg/webhooks/mxnet/mxnet_webhook.go b/pkg/webhooks/mxnet/mxnet_webhook.go
new file mode 100644
index 0000000000..57643752dd
--- /dev/null
+++ b/pkg/webhooks/mxnet/mxnet_webhook.go
@@ -0,0 +1,71 @@
+/*
+Copyright 2024 The Kubeflow Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package mxnet
+
+import (
+	"context"
+	"fmt"
+
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/klog/v2"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/webhook"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
+
+	trainingoperator "github.com/kubeflow/training-operator/pkg/apis/kubeflow.org/v1"
+)
+
+type Webhook struct{}
+
+func SetupWebhook(mgr ctrl.Manager) error {
+	return ctrl.NewWebhookManagedBy(mgr).
+		For(&trainingoperator.MXJob{}).
+		WithValidator(&Webhook{}).
+		Complete()
+}
+
+// +kubebuilder:webhook:path=/validate-kubeflow-org-v1-mxjob,mutating=false,failurePolicy=fail,sideEffects=None,groups=kubeflow.org,resources=mxjobs,verbs=create;update;delete,versions=v1,name=validator.mxjob.training-operator.kubeflow.org,admissionReviewVersions=v1
+
+var _ webhook.CustomValidator = &Webhook{}
+
+func (w *Webhook) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
+	job := obj.(*trainingoperator.MXJob)
+	log := ctrl.LoggerFrom(ctx).WithName("mxjob-webhook")
+	log.V(5).Info("Validating create", "mxJob", klog.KObj(job))
+	return deprecatedWarning(), nil
+}
+
+func (w *Webhook) ValidateUpdate(ctx context.Context, _, newObj runtime.Object) (admission.Warnings, error) {
+	job := newObj.(*trainingoperator.MXJob)
+	log := ctrl.LoggerFrom(ctx).WithName("mxjob-webhook")
+	log.V(5).Info("Validating update", "mxJob", klog.KObj(job))
+	return deprecatedWarning(), nil
+}
+
+func (w *Webhook) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
+	job := obj.(*trainingoperator.MXJob)
+	log := ctrl.LoggerFrom(ctx).WithName("mxjob-webhook")
+	log.V(5).Info("Validating delete", "mxJob", klog.KObj(job))
+	return deprecatedWarning(), nil
+}
+
+func deprecatedWarning() admission.Warnings {
+	return admission.Warnings{
+		fmt.Sprintf("MXJob is deprecated, and the MXJob will be removed in the release v1.9.0.\n" +
+			"Please see https://github.com/kubeflow/training-operator/issues/1996 for more details"),
+	}
+}
diff --git a/pkg/webhooks/webhooks.go b/pkg/webhooks/webhooks.go
index 8bfb23c68f..81e9c0d8c7 100644
--- a/pkg/webhooks/webhooks.go
+++ b/pkg/webhooks/webhooks.go
@@ -20,6 +20,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 
 	trainingoperator "github.com/kubeflow/training-operator/pkg/apis/kubeflow.org/v1"
+	"github.com/kubeflow/training-operator/pkg/webhooks/mxnet"
 	"github.com/kubeflow/training-operator/pkg/webhooks/paddlepaddle"
 	"github.com/kubeflow/training-operator/pkg/webhooks/pytorch"
 	"github.com/kubeflow/training-operator/pkg/webhooks/tensorflow"
@@ -32,7 +33,7 @@ var (
 	SupportedSchemeWebhook = map[string]WebhookSetupFunc{
 		trainingoperator.PyTorchJobKind: pytorch.SetupWebhook,
 		trainingoperator.TFJobKind:      tensorflow.SetupWebhook,
-		trainingoperator.MXJobKind:      scaffold,
+		trainingoperator.MXJobKind:      mxnet.SetupWebhook,
 		trainingoperator.XGBoostJobKind: xgboost.SetupWebhook,
 		trainingoperator.MPIJobKind:     scaffold,
 		trainingoperator.PaddleJobKind:  paddlepaddle.SetupWebhook,


Test patch:
diff --git a/pkg/controller.v1/mxnet/suite_test.go b/pkg/controller.v1/mxnet/suite_test.go
index a91e6fb837..cccfe0ed27 100644
--- a/pkg/controller.v1/mxnet/suite_test.go
+++ b/pkg/controller.v1/mxnet/suite_test.go
@@ -16,11 +16,16 @@ package mxnet
 
 import (
 	"context"
+	"crypto/tls"
+	"fmt"
+	"net"
 	"path/filepath"
 	"testing"
+	"time"
 
 	kubeflowv1 "github.com/kubeflow/training-operator/pkg/apis/kubeflow.org/v1"
 	"github.com/kubeflow/training-operator/pkg/controller.v1/common"
+	mxnetwebhook "github.com/kubeflow/training-operator/pkg/webhooks/mxnet"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -31,6 +36,7 @@ import (
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 	metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server"
+	"sigs.k8s.io/controller-runtime/pkg/webhook"
 	"volcano.sh/apis/pkg/apis/scheduling/v1beta1"
 	//+kubebuilder:scaffold:imports
 )
@@ -60,6 +66,9 @@ var _ = BeforeSuite(func() {
 	testEnv = &envtest.Environment{
 		CRDDirectoryPaths:     []string{filepath.Join("..", "..", "..", "manifests", "base", "crds")},
 		ErrorIfCRDPathMissing: true,
+		WebhookInstallOptions: envtest.WebhookInstallOptions{
+			Paths: []string{filepath.Join("..", "..", "..", "manifests", "base", "webhook", "manifests.yaml")},
+		},
 	}
 
 	cfg, err := testEnv.Start()
@@ -81,6 +90,12 @@ var _ = BeforeSuite(func() {
 		Metrics: metricsserver.Options{
 			BindAddress: "0",
 		},
+		WebhookServer: webhook.NewServer(
+			webhook.Options{
+				Host:    testEnv.WebhookInstallOptions.LocalServingHost,
+				Port:    testEnv.WebhookInstallOptions.LocalServingPort,
+				CertDir: testEnv.WebhookInstallOptions.LocalServingCertDir,
+			}),
 	})
 	Expect(err).NotTo(HaveOccurred())
 
@@ -88,12 +103,21 @@ var _ = BeforeSuite(func() {
 	r := NewReconciler(mgr, gangSchedulingSetupFunc)
 
 	Expect(r.SetupWithManager(mgr, 1)).NotTo(HaveOccurred())
+	Expect(mxnetwebhook.SetupWebhook(mgr)).NotTo(HaveOccurred())
 
 	go func() {
 		defer GinkgoRecover()
 		err = mgr.Start(testCtx)
 		Expect(err).ToNot(HaveOccurred(), "failed to run manager")
 	}()
+
+	dialer := &net.Dialer{Timeout: time.Second}
+	addrPort := fmt.Sprintf("%s:%d", testEnv.WebhookInstallOptions.LocalServingHost, testEnv.WebhookInstallOptions.LocalServingPort)
+	Eventually(func(g Gomega) {
+		conn, err := tls.DialWithDialer(dialer, "tcp", addrPort, &tls.Config{InsecureSkipVerify: true})
+		g.Expect(err).NotTo(HaveOccurred())
+		g.Expect(conn.Close()).NotTo(HaveOccurred())
+	}).Should(Succeed())
 })
 
 var _ = AfterSuite(func() {