refactor(webhook): change for mutating webhook to use Default to set ModelReg namespace

controllers/webhook/webhook.go

@@ -24,6 +24,7 @@ import (
 	operatorv1 "github.com/openshift/api/operator/v1"
 	admissionv1 "k8s.io/api/admission/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -129,42 +130,29 @@ func (w *OpenDataHubValidatingWebhook) Handle(ctx context.Context, req admission
 //+kubebuilder:webhook:path=/mutate-opendatahub-io-v1,mutating=true,failurePolicy=fail,sideEffects=None,groups=datasciencecluster.opendatahub.io,resources=datascienceclusters,verbs=create;update,versions=v1,name=mutate.operator.opendatahub.io,admissionReviewVersions=v1
 //nolint:lll
 
-// OpenDataHubMutatingWebhook is a mutating webhook
-// It currently only sets defaults for modelregiestry in datascienceclusters.
-type OpenDataHubMutatingWebhook struct {
-	Client  client.Client
-	Decoder *admission.Decoder
-}
+type DSCDefaulter struct{}
 
-func (w *OpenDataHubMutatingWebhook) SetupWithManager(mgr ctrl.Manager) {
-	hookServer := mgr.GetWebhookServer()
-	odhWebhook := &webhook.Admission{
-		Handler: w,
-	}
-	hookServer.Register("/mutate-opendatahub-io-v1", odhWebhook)
-}
+// just assert that DSCDefaulter implements webhook.CustomDefaulter.
+var _ webhook.CustomDefaulter = &DSCDefaulter{}
 
-func (w *OpenDataHubMutatingWebhook) Handle(ctx context.Context, req admission.Request) admission.Response {
-	var resp admission.Response
-	resp.Allowed = true // initialize Allowed to be true in case Operation falls into "default" case
+func (m *DSCDefaulter) SetupWithManager(mgr ctrl.Manager) {
+	mutateWebhook := admission.WithCustomDefaulter(mgr.GetScheme(), &dscv1.DataScienceCluster{}, m)
+	mgr.GetWebhookServer().Register("/mutate-opendatahub-io-v1", mutateWebhook)
+}
 
-	dsc := &dscv1.DataScienceCluster{}
-	err := w.Decoder.Decode(req, dsc)
-	if err != nil {
-		return admission.Denied(fmt.Sprintf("failed to decode request body: %s", err))
+// Implement admission.CustomDefaulter interface.
+// It currently only sets defaults for modelregiestry in datascienceclusters.
+func (m *DSCDefaulter) Default(_ context.Context, obj runtime.Object) error {
+	dsc, isDSC := obj.(*dscv1.DataScienceCluster)
+	if !isDSC {
+		return fmt.Errorf("expected DataScienceCluster but got a different type: %T", obj)
 	}
-	resp = w.setDSCDefaults(ctx, dsc)
-	return resp
-}
 
-func (w *OpenDataHubMutatingWebhook) setDSCDefaults(_ context.Context, dsc *dscv1.DataScienceCluster) admission.Response {
-	// set default registriesNamespace if empty
-	if len(dsc.Spec.Components.ModelRegistry.RegistriesNamespace) == 0 && dsc.Spec.Components.ModelRegistry.ManagementState == operatorv1.Managed {
-		return admission.Patched("Property registriesNamespace set to default value", webhook.JSONPatchOp{
-			Operation: "replace",
-			Path:      "spec.components.modelregistry.registriesNamespace",
-			Value:     modelregistry.DefaultModelRegistriesNamespace,
-		})
+	// set default registriesNamespace if empty "" but ModelRegistry is enabled
+	if dsc.Spec.Components.ModelRegistry.ManagementState == operatorv1.Managed {
+		if dsc.Spec.Components.ModelRegistry.RegistriesNamespace == "" {
+			dsc.Spec.Components.ModelRegistry.RegistriesNamespace = modelregistry.DefaultModelRegistriesNamespace
+		}
 	}
-	return admission.Allowed("")
+	return nil
 }

controllers/webhook/webhook_suite_test.go

@@ -61,6 +61,7 @@ import (
 const (
 	namespace = "webhook-test-ns"
 	nameBase  = "webhook-test"
+	mrNS      = "model-registry-namespace"
 )
 
 // These tests use Ginkgo (BDD-style Go testing framework). Refer to
@@ -138,10 +139,7 @@ var _ = BeforeSuite(func() {
 		Decoder: admission.NewDecoder(mgr.GetScheme()),
 	}).SetupWithManager(mgr)
 
-	(&webhook.OpenDataHubMutatingWebhook{
-		Client:  mgr.GetClient(),
-		Decoder: admission.NewDecoder(mgr.GetScheme()),
-	}).SetupWithManager(mgr)
+	(&webhook.DSCDefaulter{}).SetupWithManager(mgr)
 
 	// +kubebuilder:scaffold:webhook
 
@@ -173,7 +171,7 @@ var _ = AfterSuite(func() {
 	Expect(err).NotTo(HaveOccurred())
 })
 
-var _ = Describe("DSC/DSCI webhook", func() {
+var _ = Describe("DSC/DSCI validating webhook", func() {
 	It("Should not have more than one DSCI instance in the cluster", func(ctx context.Context) {
 		desiredDsci := newDSCI(nameBase + "-dsci-1")
 		Expect(k8sClient.Create(ctx, desiredDsci)).Should(Succeed())
@@ -209,13 +207,23 @@ var _ = Describe("DSC/DSCI webhook", func() {
 		Expect(k8sClient.Delete(ctx, dsciInstance)).Should(Succeed())
 	})
 
-	It("Should set defaults in DSC instance", func(ctx context.Context) {
-		dscInstance := newDSC(nameBase+"-dsc-1", namespace)
+})
+
+// mutating webhook tests for model registry.
+var _ = Describe("DSC mutating webhook", func() {
+	It("Should use defaults for DSC if empty string for MR namespace when MR is enabled", func(ctx context.Context) {
+		dscInstance := newMRDSC1(nameBase+"-dsc-mr1", "", operatorv1.Managed)
 		Expect(k8sClient.Create(ctx, dscInstance)).Should(Succeed())
 		Expect(dscInstance.Spec.Components.ModelRegistry.RegistriesNamespace).
 			Should(Equal(modelregistry.DefaultModelRegistriesNamespace))
 		Expect(clearInstance(ctx, dscInstance)).Should(Succeed())
 	})
+
+	It("Should create DSC if no MR is set (for upgrade case)", func(ctx context.Context) {
+		dscInstance := newMRDSC2(nameBase + "-dsc-mr2")
+		Expect(k8sClient.Create(ctx, dscInstance)).Should(Succeed())
+		Expect(clearInstance(ctx, dscInstance)).Should(Succeed())
+	})
 })
 
 func clearInstance(ctx context.Context, instance client.Object) error {
@@ -293,6 +301,48 @@ func newDSC(name string, namespace string) *dscv1.DataScienceCluster {
 						ManagementState: operatorv1.Removed,
 					},
 				},
+				ModelRegistry: modelregistry.ModelRegistry{
+					Component: components.Component{
+						ManagementState: operatorv1.Removed,
+					},
+				},
+			},
+		},
+	}
+}
+
+func newMRDSC1(name string, mrNamespace string, state operatorv1.ManagementState) *dscv1.DataScienceCluster {
+	return &dscv1.DataScienceCluster{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: "appNS",
+		},
+		Spec: dscv1.DataScienceClusterSpec{
+			Components: dscv1.Components{
+				ModelRegistry: modelregistry.ModelRegistry{
+					Component: components.Component{
+						ManagementState: state,
+					},
+					RegistriesNamespace: mrNamespace,
+				},
+			},
+		},
+	}
+}
+
+func newMRDSC2(name string) *dscv1.DataScienceCluster {
+	return &dscv1.DataScienceCluster{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: "appNS",
+		},
+		Spec: dscv1.DataScienceClusterSpec{
+			Components: dscv1.Components{
+				Workbenches: workbenches.Workbenches{
+					Component: components.Component{
+						ManagementState: operatorv1.Removed,
+					},
+				},
 			},
 		},
 	}

main.go

@@ -213,10 +213,7 @@ func main() { //nolint:funlen,maintidx
 		Decoder: admission.NewDecoder(mgr.GetScheme()),
 	}).SetupWithManager(mgr)
 
-	(&webhook.OpenDataHubMutatingWebhook{
-		Client:  mgr.GetClient(),
-		Decoder: admission.NewDecoder(mgr.GetScheme()),
-	}).SetupWithManager(mgr)
+	(&webhook.DSCDefaulter{}).SetupWithManager(mgr)
 
 	if err = (&dscictrl.DSCInitializationReconciler{
 		Client:                mgr.GetClient(),