From d60781d7db6dce58fa4160f239fb88f82c657eaf Mon Sep 17 00:00:00 2001 From: Saravana Date: Wed, 3 Jul 2024 01:27:15 +0530 Subject: [PATCH 1/4] RHOAIENG-2974: Restrict DSCI deletion before DSC --- config/webhook/manifests.yaml | 1 + controllers/webhook/webhook.go | 16 +++++++++++++++- pkg/cluster/gvk/gvk.go | 6 ++++++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index 5595314bffb..2ec1249d1a8 100644 --- a/config/webhook/manifests.yaml +++ b/config/webhook/manifests.yaml @@ -23,6 +23,7 @@ webhooks: operations: - CREATE - UPDATE + - DELETE resources: - datascienceclusters - dscinitializations diff --git a/controllers/webhook/webhook.go b/controllers/webhook/webhook.go index 85c2fd04e48..d00e53fc15d 100644 --- a/controllers/webhook/webhook.go +++ b/controllers/webhook/webhook.go @@ -28,11 +28,13 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" ) var log = ctrl.Log.WithName("odh-controller-webhook") -//+kubebuilder:webhook:path=/validate-opendatahub-io-v1,mutating=false,failurePolicy=fail,sideEffects=None,groups=datasciencecluster.opendatahub.io;dscinitialization.opendatahub.io,resources=datascienceclusters;dscinitializations,verbs=create;update,versions=v1,name=operator.opendatahub.io,admissionReviewVersions=v1 +//+kubebuilder:webhook:path=/validate-opendatahub-io-v1,mutating=false,failurePolicy=fail,sideEffects=None,groups=datasciencecluster.opendatahub.io;dscinitialization.opendatahub.io,resources=datascienceclusters;dscinitializations,verbs=create;update;delete,versions=v1,name=operator.opendatahub.io,admissionReviewVersions=v1 //nolint:lll type OpenDataHubWebhook struct { @@ -101,12 +103,24 @@ func (w *OpenDataHubWebhook) checkDupCreation(ctx context.Context, req admission fmt.Sprintf("Only one instance of %s object is allowed", req.Kind.Kind)) } +func (w *OpenDataHubWebhook) checkDSCIDeletion(ctx context.Context, req admission.Request) admission.Response { + // Restrict deletion of DSCI if DSC exists + if req.Kind.Kind != "DSCInitialization" { + admission.Allowed("") + } + + return denyCountGtZero(ctx, w.client, gvk.DataScienceCluster, + fmt.Sprintf("Cannot delete %s object before %s", req.Kind.Kind, gvk.DataScienceCluster.Kind)) +} + func (w *OpenDataHubWebhook) Handle(ctx context.Context, req admission.Request) admission.Response { var resp admission.Response switch req.Operation { case admissionv1.Create: resp = w.checkDupCreation(ctx, req) + case admissionv1.Delete: + resp = w.checkDSCIDeletion(ctx, req) default: msg := fmt.Sprintf("No logic check by webhook is applied on %v request", req.Operation) log.Info(msg) diff --git a/pkg/cluster/gvk/gvk.go b/pkg/cluster/gvk/gvk.go index 5091283cf6f..19f113c6183 100644 --- a/pkg/cluster/gvk/gvk.go +++ b/pkg/cluster/gvk/gvk.go @@ -9,6 +9,12 @@ var ( Kind: "ClusterServiceVersion", } + DataScienceCluster = schema.GroupVersionKind{ + Group: "datasciencecluster.opendatahub.io", + Version: "v1", + Kind: "DataScienceCluster", + } + KnativeServing = schema.GroupVersionKind{ Group: "operator.knative.dev", Version: "v1beta1", From c6e8f45d4fda69ea906721139a0e9db31f9df8c4 Mon Sep 17 00:00:00 2001 From: Saravana Date: Wed, 3 Jul 2024 22:18:18 +0530 Subject: [PATCH 2/4] Added tests and minor enhancements --- controllers/webhook/webhook.go | 10 +++---- controllers/webhook/webhook_suite_test.go | 33 ++++++++++++++++++++--- 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/controllers/webhook/webhook.go b/controllers/webhook/webhook.go index d00e53fc15d..dc13106f6f6 100644 --- a/controllers/webhook/webhook.go +++ b/controllers/webhook/webhook.go @@ -103,14 +103,14 @@ func (w *OpenDataHubWebhook) checkDupCreation(ctx context.Context, req admission fmt.Sprintf("Only one instance of %s object is allowed", req.Kind.Kind)) } -func (w *OpenDataHubWebhook) checkDSCIDeletion(ctx context.Context, req admission.Request) admission.Response { - // Restrict deletion of DSCI if DSC exists +func (w *OpenDataHubWebhook) checkDeletion(ctx context.Context, req admission.Request) admission.Response { if req.Kind.Kind != "DSCInitialization" { - admission.Allowed("") + return admission.Allowed("") } + // Restrict deletion of DSCI if DSC exists return denyCountGtZero(ctx, w.client, gvk.DataScienceCluster, - fmt.Sprintf("Cannot delete %s object before %s", req.Kind.Kind, gvk.DataScienceCluster.Kind)) + fmt.Sprintln("Cannot delete DSCI object when DSC object still exists")) } func (w *OpenDataHubWebhook) Handle(ctx context.Context, req admission.Request) admission.Response { @@ -120,7 +120,7 @@ func (w *OpenDataHubWebhook) Handle(ctx context.Context, req admission.Request) case admissionv1.Create: resp = w.checkDupCreation(ctx, req) case admissionv1.Delete: - resp = w.checkDSCIDeletion(ctx, req) + resp = w.checkDeletion(ctx, req) default: msg := fmt.Sprintf("No logic check by webhook is applied on %v request", req.Operation) log.Info(msg) diff --git a/controllers/webhook/webhook_suite_test.go b/controllers/webhook/webhook_suite_test.go index 3888512116c..6662b6655eb 100644 --- a/controllers/webhook/webhook_suite_test.go +++ b/controllers/webhook/webhook_suite_test.go @@ -159,16 +159,41 @@ var _ = Describe("DSC/DSCI webhook", func() { Expect(k8sClient.Create(ctx, desiredDsci)).Should(Succeed()) desiredDsci2 := newDSCI(nameBase + "-dsci-2") Expect(k8sClient.Create(ctx, desiredDsci2)).ShouldNot(Succeed()) + Expect(clearInstance(ctx, desiredDsci)).Should(Succeed()) }) It("Should block creation of second DSC instance", func(ctx context.Context) { - dscSpec := newDSC(nameBase+"-dsc-1", namespace) - Expect(k8sClient.Create(ctx, dscSpec)).Should(Succeed()) - dscSpec = newDSC(nameBase+"-dsc-2", namespace) - Expect(k8sClient.Create(ctx, dscSpec)).ShouldNot(Succeed()) + dscSpec1 := newDSC(nameBase+"-dsc-1", namespace) + Expect(k8sClient.Create(ctx, dscSpec1)).Should(Succeed()) + dscSpec2 := newDSC(nameBase+"-dsc-2", namespace) + Expect(k8sClient.Create(ctx, dscSpec2)).ShouldNot(Succeed()) + Expect(clearInstance(ctx, dscSpec1)).Should(Succeed()) + }) + + It("Should block deletion of DSCI instance when DSC instance exist", func(ctx context.Context) { + dscInstance := newDSC(nameBase+"-dsc-1", namespace) + Expect(k8sClient.Create(ctx, dscInstance)).Should(Succeed()) + dsciInstance := newDSCI(nameBase + "-dsci-1") + Expect(k8sClient.Create(ctx, dsciInstance)).Should(Succeed()) + Expect(k8sClient.Delete(ctx, dsciInstance)).ShouldNot(Succeed()) + Expect(clearInstance(ctx, dscInstance)).Should(Succeed()) + Expect(clearInstance(ctx, dsciInstance)).Should(Succeed()) + }) + + It("Should allow deletion of DSCI instance when DSC instance does not exist", func(ctx context.Context) { + dscInstance := newDSC(nameBase+"-dsc-1", namespace) + Expect(k8sClient.Create(ctx, dscInstance)).Should(Succeed()) + dsciInstance := newDSCI(nameBase + "-dsci-1") + Expect(k8sClient.Create(ctx, dsciInstance)).Should(Succeed()) + Expect(k8sClient.Delete(ctx, dscInstance)).Should(Succeed()) + Expect(k8sClient.Delete(ctx, dsciInstance)).Should(Succeed()) }) }) +func clearInstance(ctx context.Context, instance client.Object) error { + return k8sClient.Delete(ctx, instance) +} + func newDSCI(appName string) *dsciv1.DSCInitialization { monitoringNS := "monitoring-namespace" return &dsciv1.DSCInitialization{ From 3432a5f542e7423257cbe7d93274085351bc2b14 Mon Sep 17 00:00:00 2001 From: Saravana Date: Thu, 4 Jul 2024 16:24:08 +0530 Subject: [PATCH 3/4] Admission allow DSC --- controllers/webhook/webhook.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/webhook/webhook.go b/controllers/webhook/webhook.go index dc13106f6f6..a86ed619e89 100644 --- a/controllers/webhook/webhook.go +++ b/controllers/webhook/webhook.go @@ -104,7 +104,7 @@ func (w *OpenDataHubWebhook) checkDupCreation(ctx context.Context, req admission } func (w *OpenDataHubWebhook) checkDeletion(ctx context.Context, req admission.Request) admission.Response { - if req.Kind.Kind != "DSCInitialization" { + if req.Kind.Kind == "DataScienceCluster" { return admission.Allowed("") } From 38503519c7ee097768a95989505e11bdac258922 Mon Sep 17 00:00:00 2001 From: Saravana Date: Thu, 4 Jul 2024 19:46:50 +0530 Subject: [PATCH 4/4] lint fix --- controllers/webhook/webhook_suite_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/controllers/webhook/webhook_suite_test.go b/controllers/webhook/webhook_suite_test.go index 6662b6655eb..a179055f304 100644 --- a/controllers/webhook/webhook_suite_test.go +++ b/controllers/webhook/webhook_suite_test.go @@ -171,7 +171,7 @@ var _ = Describe("DSC/DSCI webhook", func() { }) It("Should block deletion of DSCI instance when DSC instance exist", func(ctx context.Context) { - dscInstance := newDSC(nameBase+"-dsc-1", namespace) + dscInstance := newDSC(nameBase+"-dsc-1", "webhook-test-namespace") Expect(k8sClient.Create(ctx, dscInstance)).Should(Succeed()) dsciInstance := newDSCI(nameBase + "-dsci-1") Expect(k8sClient.Create(ctx, dsciInstance)).Should(Succeed()) @@ -181,7 +181,7 @@ var _ = Describe("DSC/DSCI webhook", func() { }) It("Should allow deletion of DSCI instance when DSC instance does not exist", func(ctx context.Context) { - dscInstance := newDSC(nameBase+"-dsc-1", namespace) + dscInstance := newDSC(nameBase+"-dsc-1", "webhook-test-namespace") Expect(k8sClient.Create(ctx, dscInstance)).Should(Succeed()) dsciInstance := newDSCI(nameBase + "-dsci-1") Expect(k8sClient.Create(ctx, dsciInstance)).Should(Succeed())