Skip to content

Commit

Permalink
Introduce rbac controller to handle RoleBindings (#434)
Browse files Browse the repository at this point in the history
* Introduce rolebinding controller to handle rbacs

* Use SET_PIPELINE_RBAC configuration env

* Skip tests if SET_PIPELINE_RBAC is set to false

* Remove configmap and use a simple env in odh managers deployment

* Swap deletion before creation on rbac controller

* Add log error in case of error on the rbac reconciler

* Set proper the ownerReferences when we create a rolebinding
  • Loading branch information
atheo89 authored Nov 7, 2024
1 parent 9d606be commit 3f931d2
Show file tree
Hide file tree
Showing 6 changed files with 279 additions and 2 deletions.
11 changes: 9 additions & 2 deletions components/odh-notebook-controller/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,6 @@ spec:
requests:
cpu: 500m
memory: 256Mi
env:
- name: SET_PIPELINE_RBAC
value: "false"
23 changes: 23 additions & 0 deletions components/odh-notebook-controller/config/rbac/role.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,10 @@ import (
"crypto/x509"
"encoding/pem"
"errors"
"os"
"reflect"
"strconv"
"strings"
"time"

netv1 "k8s.io/api/networking/v1"
Expand All @@ -32,6 +34,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"
Expand Down Expand Up @@ -67,6 +70,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 {
Expand Down Expand Up @@ -184,6 +189,15 @@ func (r *OpenshiftNotebookReconciler) Reconcile(ctx context.Context, req ctrl.Re
return ctrl.Result{}, err
}

// Call the Rolebinding reconciler
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
}
}

if !ServiceMeshIsEnabled(notebook.ObjectMeta) {
// Create the objects required by the OAuth proxy sidecar (see notebook_oauth.go file)
if OAuthInjectionIsEnabled(notebook.ObjectMeta) {
Expand Down Expand Up @@ -445,6 +459,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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,16 @@ import (
"context"
"crypto/x509"
"encoding/pem"
"fmt"
"io/ioutil"
"os"
"strings"
"time"

"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"
Expand Down Expand Up @@ -163,6 +166,78 @@ var _ = Describe("The Openshift Notebook controller", func() {

})

// New test case for RoleBinding reconciliation
When("Reconcile RoleBindings 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

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()

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 (
Expand Down
154 changes: 154 additions & 0 deletions components/odh-notebook-controller/controllers/notebook_rbac.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
/*
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"
ctrl "sigs.k8s.io/controller-runtime"
)

// 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
}

// 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)

// 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)
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 {

roleBindingName := "elyra-pipelines-" + notebook.Name
// 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
}

0 comments on commit 3f931d2

Please sign in to comment.