Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce rbac controller to handle RoleBindings #434

Merged
merged 7 commits into from
Nov 7, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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.
atheo89 marked this conversation as resolved.
Show resolved Hide resolved
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
atheo89 marked this conversation as resolved.
Show resolved Hide resolved

// 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
atheo89 marked this conversation as resolved.
Show resolved Hide resolved
if strings.ToLower(strings.TrimSpace(os.Getenv("SET_PIPELINE_RBAC"))) == "true" {
atheo89 marked this conversation as resolved.
Show resolved Hide resolved
jiridanek marked this conversation as resolved.
Show resolved Hide resolved
err = r.ReconcileRoleBindings(notebook, ctx)
if err != nil {
log.Error(err, "Unable to Reconcile Rolebinding")
return ctrl.Result{}, err
atheo89 marked this conversation as resolved.
Show resolved Hide resolved
}
}

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
166 changes: 166 additions & 0 deletions components/odh-notebook-controller/controllers/notebook_rbac.go
Original file line number Diff line number Diff line change
@@ -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)
atheo89 marked this conversation as resolved.
Show resolved Hide resolved

// 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)
atheo89 marked this conversation as resolved.
Show resolved Hide resolved
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) {
atheo89 marked this conversation as resolved.
Show resolved Hide resolved
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
// If the notebook is marked for deletion, remove the associated RoleBinding
if !notebook.ObjectMeta.DeletionTimestamp.IsZero() {
atheo89 marked this conversation as resolved.
Show resolved Hide resolved
atheo89 marked this conversation as resolved.
Show resolved Hide resolved
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
}