From 04214a278db46f97e33468c575ce7de3eaebc3b3 Mon Sep 17 00:00:00 2001 From: atheo89 Date: Mon, 28 Oct 2024 09:21:03 +0100 Subject: [PATCH 1/7] Introduce rolebinding controller to handle rbacs --- .../config/rbac/role.yaml | 23 +++ .../controllers/notebook_controller.go | 10 ++ .../controllers/notebook_controller_test.go | 65 +++++++ .../controllers/notebook_rbac.go | 166 ++++++++++++++++++ 4 files changed, 264 insertions(+) create mode 100644 components/odh-notebook-controller/controllers/notebook_rbac.go diff --git a/components/odh-notebook-controller/config/rbac/role.yaml b/components/odh-notebook-controller/config/rbac/role.yaml index 1551e236714..94be8c5e262 100644 --- a/components/odh-notebook-controller/config/rbac/role.yaml +++ b/components/odh-notebook-controller/config/rbac/role.yaml @@ -58,6 +58,29 @@ rules: - patch - update - watch +- apiGroups: + - rbac.authorization.k8s.io + resources: + - rolebindings + verbs: + - create + - delete + - get + - list + - patch + - update + - watch +- apiGroups: + - rbac.authorization.k8s.io + resources: + - roles + verbs: + - create + - get + - list + - patch + - update + - watch - apiGroups: - route.openshift.io resources: diff --git a/components/odh-notebook-controller/controllers/notebook_controller.go b/components/odh-notebook-controller/controllers/notebook_controller.go index cce2e87386f..f584678a0a0 100644 --- a/components/odh-notebook-controller/controllers/notebook_controller.go +++ b/components/odh-notebook-controller/controllers/notebook_controller.go @@ -32,6 +32,7 @@ import ( "github.com/kubeflow/kubeflow/components/notebook-controller/pkg/culler" routev1 "github.com/openshift/api/route/v1" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" apierrs "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -67,6 +68,8 @@ type OpenshiftNotebookReconciler struct { // +kubebuilder:rbac:groups="",resources=services;serviceaccounts;secrets;configmaps,verbs=get;list;watch;create;update;patch // +kubebuilder:rbac:groups=config.openshift.io,resources=proxies,verbs=get;list;watch // +kubebuilder:rbac:groups=networking.k8s.io,resources=networkpolicies,verbs=get;list;watch;create;update;patch +// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles,verbs=get;list;watch;create;update;patch +// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=rolebindings,verbs=get;list;watch;create;update;patch;delete // CompareNotebooks checks if two notebooks are equal, if not return false. func CompareNotebooks(nb1 nbv1.Notebook, nb2 nbv1.Notebook) bool { @@ -184,6 +187,12 @@ func (r *OpenshiftNotebookReconciler) Reconcile(ctx context.Context, req ctrl.Re return ctrl.Result{}, err } + // Call the Rolebinding reconciler + err = r.ReconcileRoleBindings(notebook, ctx) + if err != nil { + return ctrl.Result{}, err + } + if !ServiceMeshIsEnabled(notebook.ObjectMeta) { // Create the objects required by the OAuth proxy sidecar (see notebook_oauth.go file) if OAuthInjectionIsEnabled(notebook.ObjectMeta) { @@ -445,6 +454,7 @@ func (r *OpenshiftNotebookReconciler) SetupWithManager(mgr ctrl.Manager) error { Owns(&corev1.Service{}). Owns(&corev1.Secret{}). Owns(&netv1.NetworkPolicy{}). + Owns(&rbacv1.RoleBinding{}). // Watch for all the required ConfigMaps // odh-trusted-ca-bundle, kube-root-ca.crt, workbench-trusted-ca-bundle diff --git a/components/odh-notebook-controller/controllers/notebook_controller_test.go b/components/odh-notebook-controller/controllers/notebook_controller_test.go index 4d251af574d..b411562f4ac 100644 --- a/components/odh-notebook-controller/controllers/notebook_controller_test.go +++ b/components/odh-notebook-controller/controllers/notebook_controller_test.go @@ -26,6 +26,7 @@ import ( "github.com/go-logr/logr" "github.com/onsi/gomega/format" netv1 "k8s.io/api/networking/v1" + rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/resource" . "github.com/onsi/ginkgo" @@ -163,6 +164,70 @@ var _ = Describe("The Openshift Notebook controller", func() { }) + // New test case for RoleBinding reconciliation + When("ReconcileRoleBindings is called for a Notebook", func() { + const ( + name = "test-notebook-rolebinding" + namespace = "default" + ) + notebook := createNotebook(name, namespace) + + // Define the role and role-binding names and types used in the reconciliation + roleRefName := "ds-pipeline-user-access-dspa" + roleBindingName := "elyra-pipelines-" + name + + It("Should create a RoleBinding when the referenced Role exists", func() { + ctx := context.Background() + + By("Creating a Notebook and ensuring the Role exists") + Expect(cli.Create(ctx, notebook)).Should(Succeed()) + time.Sleep(interval) + + // Simulate the Role required by RoleBinding + role := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: roleRefName, + Namespace: namespace, + }, + } + Expect(cli.Create(ctx, role)).Should(Succeed()) + defer func() { + if err := cli.Delete(ctx, role); err != nil { + GinkgoT().Logf("Failed to delete Role: %v", err) + } + }() + + By("Checking that the RoleBinding is created") + roleBinding := &rbacv1.RoleBinding{} + Eventually(func() error { + return cli.Get(ctx, types.NamespacedName{Name: roleBindingName, Namespace: namespace}, roleBinding) + }, duration, interval).Should(Succeed()) + + Expect(roleBinding.RoleRef.Name).To(Equal(roleRefName)) + Expect(roleBinding.Subjects[0].Name).To(Equal(name)) + Expect(roleBinding.Subjects[0].Kind).To(Equal("ServiceAccount")) + }) + + It("Should delete the RoleBinding when the Notebook is deleted", func() { + ctx := context.Background() + + By("Ensuring the RoleBinding exists") + roleBinding := &rbacv1.RoleBinding{} + Eventually(func() error { + return cli.Get(ctx, types.NamespacedName{Name: roleBindingName, Namespace: namespace}, roleBinding) + }, duration, interval).Should(Succeed()) + + By("Deleting the Notebook") + Expect(cli.Delete(ctx, notebook)).Should(Succeed()) + + By("Ensuring the RoleBinding is deleted") + Eventually(func() error { + return cli.Get(ctx, types.NamespacedName{Name: roleBindingName, Namespace: namespace}, roleBinding) + }, duration, interval).Should(Succeed()) + }) + + }) + // New test case for notebook creation When("Creating a Notebook, test certificate is mounted", func() { const ( diff --git a/components/odh-notebook-controller/controllers/notebook_rbac.go b/components/odh-notebook-controller/controllers/notebook_rbac.go new file mode 100644 index 00000000000..73a2d98bb63 --- /dev/null +++ b/components/odh-notebook-controller/controllers/notebook_rbac.go @@ -0,0 +1,166 @@ +/* + +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 controllers + +import ( + "context" + "reflect" + + nbv1 "github.com/kubeflow/kubeflow/components/notebook-controller/api/v1" + rbacv1 "k8s.io/api/rbac/v1" + apierrs "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" +) + +// NewRoleBinding defines the desired RoleBinding or ClusterRoleBinding object. +// Parameters: +// - notebook: The Notebook resource instance for which the RoleBinding or ClusterRoleBinding is being created. +// - rolebindingName: The name to assign to the RoleBinding or ClusterRoleBinding object. +// - roleRefKind: The kind of role reference to bind to, which can be either Role or ClusterRole. +// - roleRefName: The name of the Role or ClusterRole to reference. +func NewRoleBinding(notebook *nbv1.Notebook, rolebindingName, roleRefKind, roleRefName string) *rbacv1.RoleBinding { + return &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: rolebindingName, + Namespace: notebook.Namespace, + Labels: map[string]string{ + "notebook-name": notebook.Name, + }, + }, + Subjects: []rbacv1.Subject{ + { + Kind: "ServiceAccount", + Name: notebook.Name, + Namespace: notebook.Namespace, + }, + }, + RoleRef: rbacv1.RoleRef{ + Kind: roleRefKind, + Name: roleRefName, + APIGroup: "rbac.authorization.k8s.io", + }, + } +} + +// checkRoleExists checks if a Role or ClusterRole exists in the namespace. +func (r *OpenshiftNotebookReconciler) checkRoleExists(ctx context.Context, roleRefKind, roleRefName, namespace string) (bool, error) { + if roleRefKind == "ClusterRole" { + // Check ClusterRole if roleRefKind is ClusterRole + clusterRole := &rbacv1.ClusterRole{} + err := r.Get(ctx, types.NamespacedName{Name: roleRefName}, clusterRole) + if err != nil { + if apierrs.IsNotFound(err) { + // ClusterRole not found + return false, nil + } + return false, err // Some other error occurred + } + } else { + // Check Role if roleRefKind is Role + role := &rbacv1.Role{} + err := r.Get(ctx, types.NamespacedName{Name: roleRefName, Namespace: namespace}, role) + if err != nil { + if apierrs.IsNotFound(err) { + // Role not found + return false, nil + } + return false, err // Some other error occurred + } + } + return true, nil // Role or ClusterRole exists +} + +// Helper function to delete the RoleBinding if the notebooks deleted +func (r *OpenshiftNotebookReconciler) deleteRoleBinding(ctx context.Context, rolebindingName, namespace string) error { + roleBinding := &rbacv1.RoleBinding{} + err := r.Get(ctx, types.NamespacedName{Name: rolebindingName, Namespace: namespace}, roleBinding) + if err != nil { + if apierrs.IsNotFound(err) { + return nil // RoleBinding not found, nothing to delete + } + return err // Some other error occurred + } + + // Delete the RoleBinding + return r.Delete(ctx, roleBinding) +} + +// reconcileRoleBinding manages creation, update, and deletion of RoleBindings and ClusterRoleBindings +func (r *OpenshiftNotebookReconciler) reconcileRoleBinding( + notebook *nbv1.Notebook, ctx context.Context, rolebindingName, roleRefKind, roleRefName string) error { + + log := r.Log.WithValues("notebook", types.NamespacedName{Name: notebook.Name, Namespace: notebook.Namespace}) + + // Check if the Role or ClusterRole exists before proceeding + roleExists, err := r.checkRoleExists(ctx, roleRefKind, roleRefName, notebook.Namespace) + if err != nil { + log.Error(err, "Error checking if Role exists", "Role.Kind", roleRefKind, "Role.Name", roleRefName) + return err + } + if !roleExists { + return nil // Skip if dspa Role is not found on the namespace + } + + // Create a new RoleBinding based on provided parameters + roleBinding := NewRoleBinding(notebook, rolebindingName, roleRefKind, roleRefName) + + // Check if the RoleBinding already exists + found := &rbacv1.RoleBinding{} + err = r.Get(ctx, types.NamespacedName{Name: rolebindingName, Namespace: notebook.Namespace}, found) + if err != nil && apierrs.IsNotFound(err) { + log.Info("Creating RoleBinding", "RoleBinding.Namespace", roleBinding.Namespace, "RoleBinding.Name", roleBinding.Name) + err = r.Create(ctx, roleBinding) + if err != nil { + log.Error(err, "Failed to create RoleBinding", "RoleBinding.Namespace", roleBinding.Namespace, "RoleBinding.Name", roleBinding.Name) + return err + } + return nil + } else if err != nil { + log.Error(err, "Failed to get RoleBinding") + return err + } + + // Update RoleBinding if the subjects differ + if !reflect.DeepEqual(roleBinding.Subjects, found.Subjects) { + log.Info("Updating RoleBinding", "RoleBinding.Namespace", roleBinding.Namespace, "RoleBinding.Name", roleBinding.Name) + err = r.Update(ctx, roleBinding) + if err != nil { + log.Error(err, "Failed to update RoleBinding", "RoleBinding.Namespace", roleBinding.Namespace, "RoleBinding.Name", roleBinding.Name) + return err + } + } + + return nil +} + +// ReconcileRoleBindings will manage multiple RoleBinding and ClusterRoleBinding reconciliations +func (r *OpenshiftNotebookReconciler) ReconcileRoleBindings( + notebook *nbv1.Notebook, ctx context.Context) error { + + // Reconcile a RoleBinding for pipelines for the notebook service account + roleBindingName := "elyra-pipelines-" + notebook.Name + if err := r.reconcileRoleBinding(notebook, ctx, roleBindingName, "Role", "ds-pipeline-user-access-dspa"); err != nil { + return err + } + + // If the notebook is marked for deletion, remove the associated RoleBinding + if !notebook.ObjectMeta.DeletionTimestamp.IsZero() { + return r.deleteRoleBinding(ctx, roleBindingName, notebook.Namespace) + } + + return nil +} From 198cb10e455a70c0851b82c86dbb3b2b34a3bbc5 Mon Sep 17 00:00:00 2001 From: atheo89 Date: Thu, 31 Oct 2024 12:36:31 +0100 Subject: [PATCH 2/7] Use SET_PIPELINE_RBAC configuration env --- .../config/manager/kustomization.yaml | 4 ++++ .../odh-notebook-controller/config/manager/manager.yaml | 6 ++++++ .../odh-notebook-controller/config/manager/params.env | 1 + .../controllers/notebook_controller.go | 9 ++++++--- 4 files changed, 17 insertions(+), 3 deletions(-) create mode 100644 components/odh-notebook-controller/config/manager/params.env diff --git a/components/odh-notebook-controller/config/manager/kustomization.yaml b/components/odh-notebook-controller/config/manager/kustomization.yaml index 316613372ea..0fc53ccf42b 100644 --- a/components/odh-notebook-controller/config/manager/kustomization.yaml +++ b/components/odh-notebook-controller/config/manager/kustomization.yaml @@ -6,3 +6,7 @@ resources: - service.yaml configurations: - params.yaml +configMapGenerator: +- name: config + envs: + - params.env diff --git a/components/odh-notebook-controller/config/manager/manager.yaml b/components/odh-notebook-controller/config/manager/manager.yaml index 991878bfbf3..0acc9473d20 100644 --- a/components/odh-notebook-controller/config/manager/manager.yaml +++ b/components/odh-notebook-controller/config/manager/manager.yaml @@ -54,3 +54,9 @@ spec: requests: cpu: 500m memory: 256Mi + env: + - name: SET_PIPELINE_RBAC + valueFrom: + configMapKeyRef: + name: config + key: SET_PIPELINE_RBAC diff --git a/components/odh-notebook-controller/config/manager/params.env b/components/odh-notebook-controller/config/manager/params.env new file mode 100644 index 00000000000..e86533b7a65 --- /dev/null +++ b/components/odh-notebook-controller/config/manager/params.env @@ -0,0 +1 @@ +SET_PIPELINE_RBAC=false diff --git a/components/odh-notebook-controller/controllers/notebook_controller.go b/components/odh-notebook-controller/controllers/notebook_controller.go index f584678a0a0..ea7a6a34257 100644 --- a/components/odh-notebook-controller/controllers/notebook_controller.go +++ b/components/odh-notebook-controller/controllers/notebook_controller.go @@ -21,6 +21,7 @@ import ( "crypto/x509" "encoding/pem" "errors" + "os" "reflect" "strconv" "time" @@ -188,9 +189,11 @@ func (r *OpenshiftNotebookReconciler) Reconcile(ctx context.Context, req ctrl.Re } // Call the Rolebinding reconciler - err = r.ReconcileRoleBindings(notebook, ctx) - if err != nil { - return ctrl.Result{}, err + if os.Getenv("SET_PIPELINE_RBAC") == "true" { + err = r.ReconcileRoleBindings(notebook, ctx) + if err != nil { + return ctrl.Result{}, err + } } if !ServiceMeshIsEnabled(notebook.ObjectMeta) { From 3878f3be19a21887487579fb0867eec226c81982 Mon Sep 17 00:00:00 2001 From: atheo89 Date: Thu, 31 Oct 2024 13:12:14 +0100 Subject: [PATCH 3/7] Skip tests if SET_PIPELINE_RBAC is set to false --- components/odh-notebook-controller/Makefile | 11 +++++++++-- .../controllers/notebook_controller_test.go | 12 +++++++++++- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/components/odh-notebook-controller/Makefile b/components/odh-notebook-controller/Makefile index 8c223728285..9fdb3af172b 100644 --- a/components/odh-notebook-controller/Makefile +++ b/components/odh-notebook-controller/Makefile @@ -86,8 +86,15 @@ fmt: ## Run go fmt against code. vet: ## Run go vet against code. go vet ./... -.PHONY: test -test: manifests generate fmt vet envtest ## Run tests. +.PHONY: test test-with-rbac-false test-with-rbac-true +test: test-with-rbac-false test-with-rbac-true +test-with-rbac-false: manifests generate fmt vet envtest ## Run tests. + export SET_PIPELINE_RBAC=false && \ + ACK_GINKGO_DEPRECATIONS=1.16.5 \ + KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path)" \ + go test ./controllers/... -ginkgo.v -ginkgo.progress -test.v -coverprofile cover.out +test-with-rbac-true: manifests generate fmt vet envtest ## Run tests. + export SET_PIPELINE_RBAC=true && \ ACK_GINKGO_DEPRECATIONS=1.16.5 \ KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path)" \ go test ./controllers/... -ginkgo.v -ginkgo.progress -test.v -coverprofile cover.out diff --git a/components/odh-notebook-controller/controllers/notebook_controller_test.go b/components/odh-notebook-controller/controllers/notebook_controller_test.go index b411562f4ac..65e96d66da0 100644 --- a/components/odh-notebook-controller/controllers/notebook_controller_test.go +++ b/components/odh-notebook-controller/controllers/notebook_controller_test.go @@ -19,7 +19,9 @@ import ( "context" "crypto/x509" "encoding/pem" + "fmt" "io/ioutil" + "os" "strings" "time" @@ -165,7 +167,7 @@ var _ = Describe("The Openshift Notebook controller", func() { }) // New test case for RoleBinding reconciliation - When("ReconcileRoleBindings is called for a Notebook", func() { + When("Reconcile RoleBindings is called for a Notebook", func() { const ( name = "test-notebook-rolebinding" namespace = "default" @@ -176,6 +178,14 @@ var _ = Describe("The Openshift Notebook controller", func() { roleRefName := "ds-pipeline-user-access-dspa" roleBindingName := "elyra-pipelines-" + name + BeforeEach(func() { + // Skip the tests if SET_PIPELINE_RBAC is not set to "true" + fmt.Printf("SET_PIPELINE_RBAC is: %s\n", os.Getenv("SET_PIPELINE_RBAC")) + if os.Getenv("SET_PIPELINE_RBAC") != "true" { + Skip("Skipping RoleBinding reconciliation tests as SET_PIPELINE_RBAC is not set to 'true'") + } + }) + It("Should create a RoleBinding when the referenced Role exists", func() { ctx := context.Background() From 5e6c77552b5c8cc836d9fc22965c924aad08a8f6 Mon Sep 17 00:00:00 2001 From: atheo89 Date: Wed, 6 Nov 2024 09:16:23 +0100 Subject: [PATCH 4/7] Remove configmap and use a simple env in odh managers deployment --- .../config/manager/kustomization.yaml | 4 ---- .../odh-notebook-controller/config/manager/manager.yaml | 5 +---- components/odh-notebook-controller/config/manager/params.env | 1 - .../controllers/notebook_controller.go | 3 ++- 4 files changed, 3 insertions(+), 10 deletions(-) delete mode 100644 components/odh-notebook-controller/config/manager/params.env diff --git a/components/odh-notebook-controller/config/manager/kustomization.yaml b/components/odh-notebook-controller/config/manager/kustomization.yaml index 0fc53ccf42b..316613372ea 100644 --- a/components/odh-notebook-controller/config/manager/kustomization.yaml +++ b/components/odh-notebook-controller/config/manager/kustomization.yaml @@ -6,7 +6,3 @@ resources: - service.yaml configurations: - params.yaml -configMapGenerator: -- name: config - envs: - - params.env diff --git a/components/odh-notebook-controller/config/manager/manager.yaml b/components/odh-notebook-controller/config/manager/manager.yaml index 0acc9473d20..36f35727e3f 100644 --- a/components/odh-notebook-controller/config/manager/manager.yaml +++ b/components/odh-notebook-controller/config/manager/manager.yaml @@ -56,7 +56,4 @@ spec: memory: 256Mi env: - name: SET_PIPELINE_RBAC - valueFrom: - configMapKeyRef: - name: config - key: SET_PIPELINE_RBAC + value: "false" diff --git a/components/odh-notebook-controller/config/manager/params.env b/components/odh-notebook-controller/config/manager/params.env deleted file mode 100644 index e86533b7a65..00000000000 --- a/components/odh-notebook-controller/config/manager/params.env +++ /dev/null @@ -1 +0,0 @@ -SET_PIPELINE_RBAC=false diff --git a/components/odh-notebook-controller/controllers/notebook_controller.go b/components/odh-notebook-controller/controllers/notebook_controller.go index ea7a6a34257..614a6b31d41 100644 --- a/components/odh-notebook-controller/controllers/notebook_controller.go +++ b/components/odh-notebook-controller/controllers/notebook_controller.go @@ -24,6 +24,7 @@ import ( "os" "reflect" "strconv" + "strings" "time" netv1 "k8s.io/api/networking/v1" @@ -189,7 +190,7 @@ func (r *OpenshiftNotebookReconciler) Reconcile(ctx context.Context, req ctrl.Re } // Call the Rolebinding reconciler - if os.Getenv("SET_PIPELINE_RBAC") == "true" { + if strings.ToLower(strings.TrimSpace(os.Getenv("SET_PIPELINE_RBAC"))) == "true" { err = r.ReconcileRoleBindings(notebook, ctx) if err != nil { return ctrl.Result{}, err From 573e592f002c92a07f2e823b4aae4483e71445cf Mon Sep 17 00:00:00 2001 From: atheo89 Date: Wed, 6 Nov 2024 16:02:49 +0100 Subject: [PATCH 5/7] Swap deletion before creation on rbac controller --- .../controllers/notebook_rbac.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/components/odh-notebook-controller/controllers/notebook_rbac.go b/components/odh-notebook-controller/controllers/notebook_rbac.go index 73a2d98bb63..6f7ede54ac0 100644 --- a/components/odh-notebook-controller/controllers/notebook_rbac.go +++ b/components/odh-notebook-controller/controllers/notebook_rbac.go @@ -151,16 +151,16 @@ func (r *OpenshiftNotebookReconciler) reconcileRoleBinding( func (r *OpenshiftNotebookReconciler) ReconcileRoleBindings( notebook *nbv1.Notebook, ctx context.Context) error { - // Reconcile a RoleBinding for pipelines for the notebook service account roleBindingName := "elyra-pipelines-" + notebook.Name - if err := r.reconcileRoleBinding(notebook, ctx, roleBindingName, "Role", "ds-pipeline-user-access-dspa"); err != nil { - return err - } - // If the notebook is marked for deletion, remove the associated RoleBinding if !notebook.ObjectMeta.DeletionTimestamp.IsZero() { return r.deleteRoleBinding(ctx, roleBindingName, notebook.Namespace) } + // Reconcile a RoleBinding for pipelines for the notebook service account + if err := r.reconcileRoleBinding(notebook, ctx, roleBindingName, "Role", "ds-pipeline-user-access-dspa"); err != nil { + return err + } + return nil } From d937dece9695e43896763147b9447a97e8bd55c7 Mon Sep 17 00:00:00 2001 From: atheo89 Date: Wed, 6 Nov 2024 16:08:07 +0100 Subject: [PATCH 6/7] Add log error in case of error on the rbac reconciler --- .../odh-notebook-controller/controllers/notebook_controller.go | 1 + 1 file changed, 1 insertion(+) diff --git a/components/odh-notebook-controller/controllers/notebook_controller.go b/components/odh-notebook-controller/controllers/notebook_controller.go index 614a6b31d41..3e667ed06cb 100644 --- a/components/odh-notebook-controller/controllers/notebook_controller.go +++ b/components/odh-notebook-controller/controllers/notebook_controller.go @@ -193,6 +193,7 @@ func (r *OpenshiftNotebookReconciler) Reconcile(ctx context.Context, req ctrl.Re if strings.ToLower(strings.TrimSpace(os.Getenv("SET_PIPELINE_RBAC"))) == "true" { err = r.ReconcileRoleBindings(notebook, ctx) if err != nil { + log.Error(err, "Unable to Reconcile Rolebinding") return ctrl.Result{}, err } } From 53b65aa25f33305f6e580be46984d1ba82c27dae Mon Sep 17 00:00:00 2001 From: atheo89 Date: Thu, 7 Nov 2024 09:24:17 +0100 Subject: [PATCH 7/7] Set proper the ownerReferences when we create a rolebinding --- .../controllers/notebook_rbac.go | 28 ++++++------------- 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/components/odh-notebook-controller/controllers/notebook_rbac.go b/components/odh-notebook-controller/controllers/notebook_rbac.go index 6f7ede54ac0..6055eceb974 100644 --- a/components/odh-notebook-controller/controllers/notebook_rbac.go +++ b/components/odh-notebook-controller/controllers/notebook_rbac.go @@ -24,6 +24,7 @@ import ( apierrs "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" ) // NewRoleBinding defines the desired RoleBinding or ClusterRoleBinding object. @@ -84,21 +85,6 @@ func (r *OpenshiftNotebookReconciler) checkRoleExists(ctx context.Context, roleR return true, nil // Role or ClusterRole exists } -// Helper function to delete the RoleBinding if the notebooks deleted -func (r *OpenshiftNotebookReconciler) deleteRoleBinding(ctx context.Context, rolebindingName, namespace string) error { - roleBinding := &rbacv1.RoleBinding{} - err := r.Get(ctx, types.NamespacedName{Name: rolebindingName, Namespace: namespace}, roleBinding) - if err != nil { - if apierrs.IsNotFound(err) { - return nil // RoleBinding not found, nothing to delete - } - return err // Some other error occurred - } - - // Delete the RoleBinding - return r.Delete(ctx, roleBinding) -} - // reconcileRoleBinding manages creation, update, and deletion of RoleBindings and ClusterRoleBindings func (r *OpenshiftNotebookReconciler) reconcileRoleBinding( notebook *nbv1.Notebook, ctx context.Context, rolebindingName, roleRefKind, roleRefName string) error { @@ -123,6 +109,13 @@ func (r *OpenshiftNotebookReconciler) reconcileRoleBinding( err = r.Get(ctx, types.NamespacedName{Name: rolebindingName, Namespace: notebook.Namespace}, found) if err != nil && apierrs.IsNotFound(err) { log.Info("Creating RoleBinding", "RoleBinding.Namespace", roleBinding.Namespace, "RoleBinding.Name", roleBinding.Name) + + // Add .metatada.ownerReferences to the Rolebinding to be deleted by + // the Kubernetes garbage collector if the notebook is deleted + if err := ctrl.SetControllerReference(notebook, roleBinding, r.Scheme); err != nil { + log.Error(err, "Failed to set controller reference for RoleBinding") + return err + } err = r.Create(ctx, roleBinding) if err != nil { log.Error(err, "Failed to create RoleBinding", "RoleBinding.Namespace", roleBinding.Namespace, "RoleBinding.Name", roleBinding.Name) @@ -152,11 +145,6 @@ func (r *OpenshiftNotebookReconciler) ReconcileRoleBindings( notebook *nbv1.Notebook, ctx context.Context) error { roleBindingName := "elyra-pipelines-" + notebook.Name - // If the notebook is marked for deletion, remove the associated RoleBinding - if !notebook.ObjectMeta.DeletionTimestamp.IsZero() { - return r.deleteRoleBinding(ctx, roleBindingName, notebook.Namespace) - } - // Reconcile a RoleBinding for pipelines for the notebook service account if err := r.reconcileRoleBinding(notebook, ctx, roleBindingName, "Role", "ds-pipeline-user-access-dspa"); err != nil { return err