Merge pull request #444 from atheo89/sync-v1.9

Sync v1.9-branch branch with main branch

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

components/odh-notebook-controller/config/manager/manager.yaml

@@ -25,7 +25,7 @@ spec:
           imagePullPolicy: Always
           command:
             - /manager
-          args: ["--oauth-proxy-image", "registry.redhat.io/openshift4/ose-oauth-proxy@sha256:4bef31eb993feb6f1096b51b4876c65a6fb1f4401fee97fa4f4542b6b7c9bc46"]
+          args: ["--oauth-proxy-image", "registry.redhat.io/openshift4/ose-oauth-proxy@sha256:4f8d66597feeb32bb18699326029f9a71a5aca4a57679d636b876377c2e95695"]
           securityContext:
             allowPrivilegeEscalation: false
           ports:
@@ -54,3 +54,6 @@ spec:
             requests:
               cpu: 500m
               memory: 256Mi
+          env:
+            - name: SET_PIPELINE_RBAC
+              value: "false"

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:

components/odh-notebook-controller/controllers/notebook_controller.go

@@ -21,8 +21,10 @@ import (
 	"crypto/x509"
 	"encoding/pem"
 	"errors"
+	"os"
 	"reflect"
 	"strconv"
+	"strings"
 	"time"
 
 	netv1 "k8s.io/api/networking/v1"
@@ -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"
@@ -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 {
@@ -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) {
@@ -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

components/odh-notebook-controller/controllers/notebook_controller_test.go

@@ -17,13 +17,18 @@ package controllers
 
 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"
@@ -161,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 (
@@ -230,6 +307,12 @@ var _ = Describe("The Openshift Notebook controller", func() {
 			}
 			// Check if the volume is present and matches the expected one
 			Expect(notebook.Spec.Template.Spec.Volumes).To(ContainElement(expectedVolume))
+
+			// Check the content in workbench-trusted-ca-bundle matches what we expect:
+			//   - have 2 certificates there in ca-bundle.crt
+			//   - both certificates are valid
+			configMapName := "workbench-trusted-ca-bundle"
+			checkCertConfigMap(ctx, notebook.Namespace, configMapName, "ca-bundle.crt", 2)
 		})
 
 	})
@@ -329,6 +412,12 @@ var _ = Describe("The Openshift Notebook controller", func() {
 				},
 			}
 			Expect(notebook.Spec.Template.Spec.Volumes).To(ContainElement(expectedVolume))
+
+			// Check the content in workbench-trusted-ca-bundle matches what we expect:
+			//   - have 2 certificates there in ca-bundle.crt
+			//   - both certificates are valid
+			configMapName := "workbench-trusted-ca-bundle"
+			checkCertConfigMap(ctx, notebook.Namespace, configMapName, "ca-bundle.crt", 2)
 		})
 	})
 
@@ -594,21 +683,23 @@ var _ = Describe("The Openshift Notebook controller", func() {
 			}, duration, interval).Should(BeTrue())
 		})
 
-		It("Should reconcile the Notebook when modified", func() {
-			By("By simulating a manual Notebook modification")
-			notebook.Spec.Template.Spec.ServiceAccountName = "foo"
-			notebook.Spec.Template.Spec.Containers[1].Image = "bar"
-			notebook.Spec.Template.Spec.Volumes[1].VolumeSource = corev1.VolumeSource{}
-			Expect(cli.Update(ctx, notebook)).Should(Succeed())
-			time.Sleep(interval)
-
-			By("By checking that the webhook has restored the Notebook spec")
-			Eventually(func() error {
-				key := types.NamespacedName{Name: Name, Namespace: Namespace}
-				return cli.Get(ctx, key, notebook)
-			}, duration, interval).Should(Succeed())
-			Expect(CompareNotebooks(*notebook, expectedNotebook)).Should(BeTrue())
-		})
+		// RHOAIENG-14552: We *do not* reconcile OAuth in the notebook when it's modified
+
+		//It("Should reconcile the Notebook when modified", func() {
+		//	By("By simulating a manual Notebook modification")
+		//	notebook.Spec.Template.Spec.ServiceAccountName = "foo"
+		//	notebook.Spec.Template.Spec.Containers[1].Image = "bar"
+		//	notebook.Spec.Template.Spec.Volumes[1].VolumeSource = corev1.VolumeSource{}
+		//	Expect(cli.Update(ctx, notebook)).Should(Succeed())
+		//	time.Sleep(interval)
+		//
+		//	By("By checking that the webhook has restored the Notebook spec")
+		//	Eventually(func() error {
+		//		key := types.NamespacedName{Name: Name, Namespace: Namespace}
+		//		return cli.Get(ctx, key, notebook)
+		//	}, duration, interval).Should(Succeed())
+		//	Expect(CompareNotebooks(*notebook, expectedNotebook)).Should(BeTrue())
+		//})
 
 		serviceAccount := &corev1.ServiceAccount{}
 		expectedServiceAccount := createOAuthServiceAccount(Name, Namespace)
@@ -1039,3 +1130,32 @@ func createOAuthConfigmap(name, namespace string, label map[string]string, confi
 		Data: configMapData,
 	}
 }
+
+// checkCertConfigMap checks the content of a config map defined by the name and namespace
+// It triest to parse the given certFileName and checks that all certificates can be parsed there and that the number of the certificates matches what we expect.
+func checkCertConfigMap(ctx context.Context, namespace string, configMapName string, certFileName string, expNumberCerts int) {
+	configMap := &corev1.ConfigMap{}
+	key := types.NamespacedName{Namespace: namespace, Name: configMapName}
+	Expect(cli.Get(ctx, key, configMap)).Should(Succeed())
+
+	// Attempt to decode PEM encoded certificates so we are sure all are readable as expected
+	certData := configMap.Data[certFileName]
+	certDataByte := []byte(certData)
+	certificatesFound := 0
+	for len(certDataByte) > 0 {
+		block, remainder := pem.Decode(certDataByte)
+		certDataByte = remainder
+
+		if block == nil {
+			break
+		}
+
+		if block.Type == "CERTIFICATE" {
+			// Attempt to parse the certificate
+			_, err := x509.ParseCertificate(block.Bytes)
+			Expect(err).ShouldNot(HaveOccurred())
+			certificatesFound++
+		}
+	}
+	Expect(certificatesFound).Should(Equal(expNumberCerts), "Number of parsed certificates don't match expected one:\n"+certData)
+}

components/odh-notebook-controller/controllers/notebook_oauth.go

@@ -35,10 +35,10 @@ import (
 const (
 	OAuthServicePort     = 443
 	OAuthServicePortName = "oauth-proxy"
-	// OAuthProxyImage uses sha256 manifest list digest value of v4.8 image for AMD64 as default to be compatible with imagePullPolicy: IfNotPresent, overridable
-	// taken from https://catalog.redhat.com/software/containers/openshift4/ose-oauth-proxy/5cdb2133bed8bd5717d5ae64?image=6306f12280cc9b3291272668&architecture=amd64&container-tabs=overview
+	// OAuthProxyImage uses sha256 manifest list digest value of v4.14 image for AMD64 as default to be compatible with imagePullPolicy: IfNotPresent, overridable
+	// taken from https://catalog.redhat.com/software/containers/openshift4/ose-oauth-proxy/5cdb2133bed8bd5717d5ae64?image=66cefc14401df6ff4664ec43&architecture=amd64&container-tabs=overview
 	// and kept in sync with the manifests here and in ClusterServiceVersion metadata of opendatahub operator
-	OAuthProxyImage = "registry.redhat.io/openshift4/ose-oauth-proxy@sha256:4bef31eb993feb6f1096b51b4876c65a6fb1f4401fee97fa4f4542b6b7c9bc46"
+	OAuthProxyImage = "registry.redhat.io/openshift4/ose-oauth-proxy@sha256:4f8d66597feeb32bb18699326029f9a71a5aca4a57679d636b876377c2e95695"
 )
 
 type OAuthConfig struct {