Skip to content

Commit 73f8106

Browse files
committed
Merge branch 'main' of https://github.com/opendatahub-io/kubeflow into sync-v1.9
2 parents e5b3eeb + 3f931d2 commit 73f8106

File tree

8 files changed

+354
-28
lines changed

8 files changed

+354
-28
lines changed

components/odh-notebook-controller/Makefile

+9-2
Original file line numberDiff line numberDiff line change
@@ -86,8 +86,15 @@ fmt: ## Run go fmt against code.
8686
vet: ## Run go vet against code.
8787
go vet ./...
8888

89-
.PHONY: test
90-
test: manifests generate fmt vet envtest ## Run tests.
89+
.PHONY: test test-with-rbac-false test-with-rbac-true
90+
test: test-with-rbac-false test-with-rbac-true
91+
test-with-rbac-false: manifests generate fmt vet envtest ## Run tests.
92+
export SET_PIPELINE_RBAC=false && \
93+
ACK_GINKGO_DEPRECATIONS=1.16.5 \
94+
KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path)" \
95+
go test ./controllers/... -ginkgo.v -ginkgo.progress -test.v -coverprofile cover.out
96+
test-with-rbac-true: manifests generate fmt vet envtest ## Run tests.
97+
export SET_PIPELINE_RBAC=true && \
9198
ACK_GINKGO_DEPRECATIONS=1.16.5 \
9299
KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path)" \
93100
go test ./controllers/... -ginkgo.v -ginkgo.progress -test.v -coverprofile cover.out

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

+4-1
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ spec:
2525
imagePullPolicy: Always
2626
command:
2727
- /manager
28-
args: ["--oauth-proxy-image", "registry.redhat.io/openshift4/ose-oauth-proxy@sha256:4bef31eb993feb6f1096b51b4876c65a6fb1f4401fee97fa4f4542b6b7c9bc46"]
28+
args: ["--oauth-proxy-image", "registry.redhat.io/openshift4/ose-oauth-proxy@sha256:4f8d66597feeb32bb18699326029f9a71a5aca4a57679d636b876377c2e95695"]
2929
securityContext:
3030
allowPrivilegeEscalation: false
3131
ports:
@@ -54,3 +54,6 @@ spec:
5454
requests:
5555
cpu: 500m
5656
memory: 256Mi
57+
env:
58+
- name: SET_PIPELINE_RBAC
59+
value: "false"

components/odh-notebook-controller/config/rbac/role.yaml

+23
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,29 @@ rules:
5858
- patch
5959
- update
6060
- watch
61+
- apiGroups:
62+
- rbac.authorization.k8s.io
63+
resources:
64+
- rolebindings
65+
verbs:
66+
- create
67+
- delete
68+
- get
69+
- list
70+
- patch
71+
- update
72+
- watch
73+
- apiGroups:
74+
- rbac.authorization.k8s.io
75+
resources:
76+
- roles
77+
verbs:
78+
- create
79+
- get
80+
- list
81+
- patch
82+
- update
83+
- watch
6184
- apiGroups:
6285
- route.openshift.io
6386
resources:

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

+15
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,10 @@ import (
2121
"crypto/x509"
2222
"encoding/pem"
2323
"errors"
24+
"os"
2425
"reflect"
2526
"strconv"
27+
"strings"
2628
"time"
2729

2830
netv1 "k8s.io/api/networking/v1"
@@ -32,6 +34,7 @@ import (
3234
"github.com/kubeflow/kubeflow/components/notebook-controller/pkg/culler"
3335
routev1 "github.com/openshift/api/route/v1"
3436
corev1 "k8s.io/api/core/v1"
37+
rbacv1 "k8s.io/api/rbac/v1"
3538
apierrs "k8s.io/apimachinery/pkg/api/errors"
3639
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3740
"k8s.io/apimachinery/pkg/runtime"
@@ -67,6 +70,8 @@ type OpenshiftNotebookReconciler struct {
6770
// +kubebuilder:rbac:groups="",resources=services;serviceaccounts;secrets;configmaps,verbs=get;list;watch;create;update;patch
6871
// +kubebuilder:rbac:groups=config.openshift.io,resources=proxies,verbs=get;list;watch
6972
// +kubebuilder:rbac:groups=networking.k8s.io,resources=networkpolicies,verbs=get;list;watch;create;update;patch
73+
// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles,verbs=get;list;watch;create;update;patch
74+
// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=rolebindings,verbs=get;list;watch;create;update;patch;delete
7075

7176
// CompareNotebooks checks if two notebooks are equal, if not return false.
7277
func CompareNotebooks(nb1 nbv1.Notebook, nb2 nbv1.Notebook) bool {
@@ -184,6 +189,15 @@ func (r *OpenshiftNotebookReconciler) Reconcile(ctx context.Context, req ctrl.Re
184189
return ctrl.Result{}, err
185190
}
186191

192+
// Call the Rolebinding reconciler
193+
if strings.ToLower(strings.TrimSpace(os.Getenv("SET_PIPELINE_RBAC"))) == "true" {
194+
err = r.ReconcileRoleBindings(notebook, ctx)
195+
if err != nil {
196+
log.Error(err, "Unable to Reconcile Rolebinding")
197+
return ctrl.Result{}, err
198+
}
199+
}
200+
187201
if !ServiceMeshIsEnabled(notebook.ObjectMeta) {
188202
// Create the objects required by the OAuth proxy sidecar (see notebook_oauth.go file)
189203
if OAuthInjectionIsEnabled(notebook.ObjectMeta) {
@@ -445,6 +459,7 @@ func (r *OpenshiftNotebookReconciler) SetupWithManager(mgr ctrl.Manager) error {
445459
Owns(&corev1.Service{}).
446460
Owns(&corev1.Secret{}).
447461
Owns(&netv1.NetworkPolicy{}).
462+
Owns(&rbacv1.RoleBinding{}).
448463

449464
// Watch for all the required ConfigMaps
450465
// odh-trusted-ca-bundle, kube-root-ca.crt, workbench-trusted-ca-bundle

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

+135-15
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,18 @@ package controllers
1717

1818
import (
1919
"context"
20+
"crypto/x509"
21+
"encoding/pem"
22+
"fmt"
2023
"io/ioutil"
24+
"os"
2125
"strings"
2226
"time"
2327

2428
"github.com/go-logr/logr"
2529
"github.com/onsi/gomega/format"
2630
netv1 "k8s.io/api/networking/v1"
31+
rbacv1 "k8s.io/api/rbac/v1"
2732
"k8s.io/apimachinery/pkg/api/resource"
2833

2934
. "github.com/onsi/ginkgo"
@@ -161,6 +166,78 @@ var _ = Describe("The Openshift Notebook controller", func() {
161166

162167
})
163168

169+
// New test case for RoleBinding reconciliation
170+
When("Reconcile RoleBindings is called for a Notebook", func() {
171+
const (
172+
name = "test-notebook-rolebinding"
173+
namespace = "default"
174+
)
175+
notebook := createNotebook(name, namespace)
176+
177+
// Define the role and role-binding names and types used in the reconciliation
178+
roleRefName := "ds-pipeline-user-access-dspa"
179+
roleBindingName := "elyra-pipelines-" + name
180+
181+
BeforeEach(func() {
182+
// Skip the tests if SET_PIPELINE_RBAC is not set to "true"
183+
fmt.Printf("SET_PIPELINE_RBAC is: %s\n", os.Getenv("SET_PIPELINE_RBAC"))
184+
if os.Getenv("SET_PIPELINE_RBAC") != "true" {
185+
Skip("Skipping RoleBinding reconciliation tests as SET_PIPELINE_RBAC is not set to 'true'")
186+
}
187+
})
188+
189+
It("Should create a RoleBinding when the referenced Role exists", func() {
190+
ctx := context.Background()
191+
192+
By("Creating a Notebook and ensuring the Role exists")
193+
Expect(cli.Create(ctx, notebook)).Should(Succeed())
194+
time.Sleep(interval)
195+
196+
// Simulate the Role required by RoleBinding
197+
role := &rbacv1.Role{
198+
ObjectMeta: metav1.ObjectMeta{
199+
Name: roleRefName,
200+
Namespace: namespace,
201+
},
202+
}
203+
Expect(cli.Create(ctx, role)).Should(Succeed())
204+
defer func() {
205+
if err := cli.Delete(ctx, role); err != nil {
206+
GinkgoT().Logf("Failed to delete Role: %v", err)
207+
}
208+
}()
209+
210+
By("Checking that the RoleBinding is created")
211+
roleBinding := &rbacv1.RoleBinding{}
212+
Eventually(func() error {
213+
return cli.Get(ctx, types.NamespacedName{Name: roleBindingName, Namespace: namespace}, roleBinding)
214+
}, duration, interval).Should(Succeed())
215+
216+
Expect(roleBinding.RoleRef.Name).To(Equal(roleRefName))
217+
Expect(roleBinding.Subjects[0].Name).To(Equal(name))
218+
Expect(roleBinding.Subjects[0].Kind).To(Equal("ServiceAccount"))
219+
})
220+
221+
It("Should delete the RoleBinding when the Notebook is deleted", func() {
222+
ctx := context.Background()
223+
224+
By("Ensuring the RoleBinding exists")
225+
roleBinding := &rbacv1.RoleBinding{}
226+
Eventually(func() error {
227+
return cli.Get(ctx, types.NamespacedName{Name: roleBindingName, Namespace: namespace}, roleBinding)
228+
}, duration, interval).Should(Succeed())
229+
230+
By("Deleting the Notebook")
231+
Expect(cli.Delete(ctx, notebook)).Should(Succeed())
232+
233+
By("Ensuring the RoleBinding is deleted")
234+
Eventually(func() error {
235+
return cli.Get(ctx, types.NamespacedName{Name: roleBindingName, Namespace: namespace}, roleBinding)
236+
}, duration, interval).Should(Succeed())
237+
})
238+
239+
})
240+
164241
// New test case for notebook creation
165242
When("Creating a Notebook, test certificate is mounted", func() {
166243
const (
@@ -230,6 +307,12 @@ var _ = Describe("The Openshift Notebook controller", func() {
230307
}
231308
// Check if the volume is present and matches the expected one
232309
Expect(notebook.Spec.Template.Spec.Volumes).To(ContainElement(expectedVolume))
310+
311+
// Check the content in workbench-trusted-ca-bundle matches what we expect:
312+
// - have 2 certificates there in ca-bundle.crt
313+
// - both certificates are valid
314+
configMapName := "workbench-trusted-ca-bundle"
315+
checkCertConfigMap(ctx, notebook.Namespace, configMapName, "ca-bundle.crt", 2)
233316
})
234317

235318
})
@@ -329,6 +412,12 @@ var _ = Describe("The Openshift Notebook controller", func() {
329412
},
330413
}
331414
Expect(notebook.Spec.Template.Spec.Volumes).To(ContainElement(expectedVolume))
415+
416+
// Check the content in workbench-trusted-ca-bundle matches what we expect:
417+
// - have 2 certificates there in ca-bundle.crt
418+
// - both certificates are valid
419+
configMapName := "workbench-trusted-ca-bundle"
420+
checkCertConfigMap(ctx, notebook.Namespace, configMapName, "ca-bundle.crt", 2)
332421
})
333422
})
334423

@@ -594,21 +683,23 @@ var _ = Describe("The Openshift Notebook controller", func() {
594683
}, duration, interval).Should(BeTrue())
595684
})
596685

597-
It("Should reconcile the Notebook when modified", func() {
598-
By("By simulating a manual Notebook modification")
599-
notebook.Spec.Template.Spec.ServiceAccountName = "foo"
600-
notebook.Spec.Template.Spec.Containers[1].Image = "bar"
601-
notebook.Spec.Template.Spec.Volumes[1].VolumeSource = corev1.VolumeSource{}
602-
Expect(cli.Update(ctx, notebook)).Should(Succeed())
603-
time.Sleep(interval)
604-
605-
By("By checking that the webhook has restored the Notebook spec")
606-
Eventually(func() error {
607-
key := types.NamespacedName{Name: Name, Namespace: Namespace}
608-
return cli.Get(ctx, key, notebook)
609-
}, duration, interval).Should(Succeed())
610-
Expect(CompareNotebooks(*notebook, expectedNotebook)).Should(BeTrue())
611-
})
686+
// RHOAIENG-14552: We *do not* reconcile OAuth in the notebook when it's modified
687+
688+
//It("Should reconcile the Notebook when modified", func() {
689+
// By("By simulating a manual Notebook modification")
690+
// notebook.Spec.Template.Spec.ServiceAccountName = "foo"
691+
// notebook.Spec.Template.Spec.Containers[1].Image = "bar"
692+
// notebook.Spec.Template.Spec.Volumes[1].VolumeSource = corev1.VolumeSource{}
693+
// Expect(cli.Update(ctx, notebook)).Should(Succeed())
694+
// time.Sleep(interval)
695+
//
696+
// By("By checking that the webhook has restored the Notebook spec")
697+
// Eventually(func() error {
698+
// key := types.NamespacedName{Name: Name, Namespace: Namespace}
699+
// return cli.Get(ctx, key, notebook)
700+
// }, duration, interval).Should(Succeed())
701+
// Expect(CompareNotebooks(*notebook, expectedNotebook)).Should(BeTrue())
702+
//})
612703

613704
serviceAccount := &corev1.ServiceAccount{}
614705
expectedServiceAccount := createOAuthServiceAccount(Name, Namespace)
@@ -1039,3 +1130,32 @@ func createOAuthConfigmap(name, namespace string, label map[string]string, confi
10391130
Data: configMapData,
10401131
}
10411132
}
1133+
1134+
// checkCertConfigMap checks the content of a config map defined by the name and namespace
1135+
// 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.
1136+
func checkCertConfigMap(ctx context.Context, namespace string, configMapName string, certFileName string, expNumberCerts int) {
1137+
configMap := &corev1.ConfigMap{}
1138+
key := types.NamespacedName{Namespace: namespace, Name: configMapName}
1139+
Expect(cli.Get(ctx, key, configMap)).Should(Succeed())
1140+
1141+
// Attempt to decode PEM encoded certificates so we are sure all are readable as expected
1142+
certData := configMap.Data[certFileName]
1143+
certDataByte := []byte(certData)
1144+
certificatesFound := 0
1145+
for len(certDataByte) > 0 {
1146+
block, remainder := pem.Decode(certDataByte)
1147+
certDataByte = remainder
1148+
1149+
if block == nil {
1150+
break
1151+
}
1152+
1153+
if block.Type == "CERTIFICATE" {
1154+
// Attempt to parse the certificate
1155+
_, err := x509.ParseCertificate(block.Bytes)
1156+
Expect(err).ShouldNot(HaveOccurred())
1157+
certificatesFound++
1158+
}
1159+
}
1160+
Expect(certificatesFound).Should(Equal(expNumberCerts), "Number of parsed certificates don't match expected one:\n"+certData)
1161+
}

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

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

4444
type OAuthConfig struct {

0 commit comments

Comments
 (0)