From 0400fc3bdb33d9e7c2a8b871221256b4a4b78659 Mon Sep 17 00:00:00 2001 From: Jiri Danek Date: Wed, 2 Oct 2024 11:43:33 +0200 Subject: [PATCH 01/50] Issue #362: feat(nbcs): build containers to be fips-ready This takes inspiration from: * The Notebooks 2.0 Dockerfile, which comes from a default recent Kubebuilder template, at https://github.com/kubeflow/notebooks/blob/notebooks-v2/workspaces/controller/Dockerfile * The Red Hat build Dockerfile (that's the Cachito part) in an internal repository. This change brings multiple improvements: 1. Dockerfiles are brought closer together, especially to the Red Hat build; previously, sourcing things in a stand-alone RUN command had no effect 2. The openssl fips-compatible library is linked into the manager binaries, to proactively address fips concerns --- components/notebook-controller/Dockerfile | 16 ++++++++-------- components/odh-notebook-controller/Dockerfile | 18 +++++++++--------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/components/notebook-controller/Dockerfile b/components/notebook-controller/Dockerfile index cd8e2db01e8..8c35069ab20 100644 --- a/components/notebook-controller/Dockerfile +++ b/components/notebook-controller/Dockerfile @@ -11,6 +11,8 @@ ARG GOLANG_VERSION=1.21 # Use ubi8/go-toolset as base image FROM registry.access.redhat.com/ubi8/go-toolset:${GOLANG_VERSION} as builder +ARG TARGETOS +ARG TARGETARCH ## Build args to be used at this step ARG SOURCE_CODE @@ -30,14 +32,12 @@ WORKDIR /workspace/notebook-controller ## Build the kf-notebook-controller USER root -RUN if [ -z ${CACHITO_ENV_FILE} ]; then \ - go mod download all; \ - else \ - source ${CACHITO_ENV_FILE}; \ - fi - -RUN CGO_ENABLED=0 GOOS=linux GO111MODULE=on go build -a -mod=mod \ - -o ./bin/manager main.go +# the GOARCH has not a default value to allow the binary be built according to the host where the command +# was called. For example, if we call make docker-build in a local env which has the Apple Silicon M1 SO +# the docker BUILDPLATFORM arg will be linux/arm64 when for Apple x86 it will be linux/amd64. Therefore, +# by leaving it empty we can ensure that the container and binary shipped on it will have the same platform. +RUN if [ -z ${CACHITO_ENV_FILE} ]; then go mod download; else source ${CACHITO_ENV_FILE}; fi && \ + CGO_ENABLED=1 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build -tags strictfipsruntime -a -o ./bin/manager main.go # Use ubi8/ubi-minimal as base image FROM registry.access.redhat.com/ubi8/ubi-minimal:latest diff --git a/components/odh-notebook-controller/Dockerfile b/components/odh-notebook-controller/Dockerfile index bb6ce4f2303..e370c35e578 100644 --- a/components/odh-notebook-controller/Dockerfile +++ b/components/odh-notebook-controller/Dockerfile @@ -11,6 +11,8 @@ ARG GOLANG_VERSION=1.21 # Use ubi8/go-toolset as base image FROM registry.access.redhat.com/ubi8/go-toolset:${GOLANG_VERSION} as builder +ARG TARGETOS +ARG TARGETARCH ## Build args to be used at this step ARG SOURCE_CODE @@ -28,14 +30,12 @@ WORKDIR /workspace/odh-notebook-controller ## Build the kf-notebook-controller USER root -RUN if [ -z ${CACHITO_ENV_FILE} ]; then \ - go mod download all; \ - else \ - source ${CACHITO_ENV_FILE}; \ - fi - -RUN go build \ - -o ./bin/manager main.go +# the GOARCH has not a default value to allow the binary be built according to the host where the command +# was called. For example, if we call make docker-build in a local env which has the Apple Silicon M1 SO +# the docker BUILDPLATFORM arg will be linux/arm64 when for Apple x86 it will be linux/amd64. Therefore, +# by leaving it empty we can ensure that the container and binary shipped on it will have the same platform. +RUN if [ -z ${CACHITO_ENV_FILE} ]; then go mod download; else source ${CACHITO_ENV_FILE}; fi && \ + CGO_ENABLED=1 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build -tags strictfipsruntime -a -o ./bin/manager main.go # Use ubi8/ubi-minimal as base image FROM registry.access.redhat.com/ubi8/ubi-minimal:latest @@ -50,7 +50,7 @@ RUN useradd --uid 1001 --create-home --user-group --system rhods ## Set workdir directory to user home WORKDIR /home/rhods -## Copy kf-notebook-controller-manager binary from builder stage +## Copy odh-notebook-controller-manager binary from builder stage COPY --from=builder /workspace/odh-notebook-controller/bin/manager /manager COPY --from=builder /workspace/odh-notebook-controller/third_party/license.txt third_party/license.txt From 5ddebb27235f71ae61973a0a1d0bd8523fecec93 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 7 Oct 2024 08:37:13 +0000 Subject: [PATCH 02/50] chore(gha): bump peter-evans/create-pull-request from 6.1.0 to 7.0.5 Bumps [peter-evans/create-pull-request](https://github.com/peter-evans/create-pull-request) from 6.1.0 to 7.0.5. - [Release notes](https://github.com/peter-evans/create-pull-request/releases) - [Commits](https://github.com/peter-evans/create-pull-request/compare/c5a7806660adbe173f04e3e038b0ccdcd758773c...5e914681df9dc83aa4e4905692ca88beb2f9e91f) --- updated-dependencies: - dependency-name: peter-evans/create-pull-request dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] --- .github/workflows/sync-branches-through-pr.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/sync-branches-through-pr.yaml b/.github/workflows/sync-branches-through-pr.yaml index d08ab05607f..f412be13f7f 100644 --- a/.github/workflows/sync-branches-through-pr.yaml +++ b/.github/workflows/sync-branches-through-pr.yaml @@ -35,7 +35,7 @@ jobs: echo "branch=$SYNC_BRANCH" >> $GITHUB_OUTPUT - name: Create pull request - uses: peter-evans/create-pull-request@c5a7806660adbe173f04e3e038b0ccdcd758773c # v6.1.0 + uses: peter-evans/create-pull-request@5e914681df9dc83aa4e4905692ca88beb2f9e91f # v7.0.5 with: branch: ${{ steps.prepare.outputs.branch }} title: "Sync `${{ github.event.inputs.target }}` branch with `${{ github.event.inputs.source }}` branch" From 92d07c38281710728e77eb1719d627ffcb165a29 Mon Sep 17 00:00:00 2001 From: GitHub Actions Date: Mon, 7 Oct 2024 08:39:38 +0000 Subject: [PATCH 03/50] Update odh and notebook-controller with image main-7986030 --- .../notebook-controller/config/overlays/openshift/params.env | 2 +- components/odh-notebook-controller/Makefile | 2 +- components/odh-notebook-controller/config/base/params.env | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/components/notebook-controller/config/overlays/openshift/params.env b/components/notebook-controller/config/overlays/openshift/params.env index d6f49dd6c9a..afc156ff326 100644 --- a/components/notebook-controller/config/overlays/openshift/params.env +++ b/components/notebook-controller/config/overlays/openshift/params.env @@ -1 +1 @@ -odh-kf-notebook-controller-image=quay.io/opendatahub/kubeflow-notebook-controller:1.9-1769b4b +odh-kf-notebook-controller-image=quay.io/opendatahub/kubeflow-notebook-controller:main-7986030 diff --git a/components/odh-notebook-controller/Makefile b/components/odh-notebook-controller/Makefile index 7fabc6e1107..2795b4e96ad 100644 --- a/components/odh-notebook-controller/Makefile +++ b/components/odh-notebook-controller/Makefile @@ -4,7 +4,7 @@ IMG ?= quay.io/opendatahub/odh-notebook-controller TAG ?= $(shell git describe --tags --always) KF_IMG ?= quay.io/opendatahub/kubeflow-notebook-controller -KF_TAG ?= 1.9-1769b4b +KF_TAG ?= main-7986030 CONTAINER_ENGINE ?= podman diff --git a/components/odh-notebook-controller/config/base/params.env b/components/odh-notebook-controller/config/base/params.env index 2c5e256891d..0cc0a43c330 100644 --- a/components/odh-notebook-controller/config/base/params.env +++ b/components/odh-notebook-controller/config/base/params.env @@ -1 +1 @@ -odh-notebook-controller-image=quay.io/opendatahub/odh-notebook-controller:1.9-1769b4b +odh-notebook-controller-image=quay.io/opendatahub/odh-notebook-controller:main-7986030 From 741dcfe01c196dab2a21455d1f0da6b3299e2af6 Mon Sep 17 00:00:00 2001 From: Jiri Danek Date: Tue, 15 Oct 2024 17:14:00 +0200 Subject: [PATCH 04/50] NO-JIRA: fix(gha): replace oc with kubectl in notebook_controller_integration_test.yaml We don't have oc preinstalled on the new GitHub Actions runners that we were autoupdated to. Best to stick to kubectl. --- .github/workflows/notebook_controller_integration_test.yaml | 2 +- .../workflows/odh_notebook_controller_integration_test.yaml | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/notebook_controller_integration_test.yaml b/.github/workflows/notebook_controller_integration_test.yaml index dd8caffe5fa..4482902ba6e 100644 --- a/.github/workflows/notebook_controller_integration_test.yaml +++ b/.github/workflows/notebook_controller_integration_test.yaml @@ -89,7 +89,7 @@ jobs: export PR_NOTEBOOK_IMG=localhost/${{env.IMG}}:${{env.TAG}} kustomize edit set image ${CURRENT_NOTEBOOK_IMG}=${PR_NOTEBOOK_IMG} - cat < params.env - cat < Date: Tue, 15 Oct 2024 17:41:52 +0200 Subject: [PATCH 05/50] NO-JIRA: fix(gha) improve robustness by checking for resource existence Without this, the gha is flaky and may fail with ``` notebook.kubeflow.org/minimal-notebook created Error from server (NotFound): statefulsets.apps "minimal-notebook" not found ``` --- .github/workflows/odh_notebook_controller_integration_test.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/odh_notebook_controller_integration_test.yaml b/.github/workflows/odh_notebook_controller_integration_test.yaml index 52ab71fa80b..3d451d0a33e 100644 --- a/.github/workflows/odh_notebook_controller_integration_test.yaml +++ b/.github/workflows/odh_notebook_controller_integration_test.yaml @@ -271,6 +271,8 @@ jobs: port: notebook-port EOF + # wait for the statefulset to be created + timeout 100 bash -c -- 'until kubectl get statefulset minimal-notebook; do sleep 1; done' # wait for the good result kubectl rollout status --watch statefulset minimal-notebook kubectl wait statefulset minimal-notebook --for=jsonpath='{.spec.replicas}'=1 --timeout=100s From 83fd2c8598614b4635d8551666929c50a2d0b1ff Mon Sep 17 00:00:00 2001 From: Jiri Danek Date: Wed, 16 Oct 2024 16:16:22 +0200 Subject: [PATCH 06/50] Revert "[v1.9-branch] RHOAIENG-10827: feat(nbcs): update ose-oauth-proxy image digest reference from 4.8 to the latest 4.14 version (#388)" We found at least one OCP cluster where this change causes restart of running notebooks when controller is updated from a previous version to a version with this fix present. To be safe, we'll roll this back and try again. We'll investigate that cluster to understand what's happening. This reverts commit 99e70bf1c44d3cc2e07983ded45d1e37a4ed5f42. --- .../odh-notebook-controller/config/manager/manager.yaml | 2 +- .../odh-notebook-controller/controllers/notebook_oauth.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/components/odh-notebook-controller/config/manager/manager.yaml b/components/odh-notebook-controller/config/manager/manager.yaml index 98f25094bd8..991878bfbf3 100644 --- a/components/odh-notebook-controller/config/manager/manager.yaml +++ b/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:4f8d66597feeb32bb18699326029f9a71a5aca4a57679d636b876377c2e95695"] + args: ["--oauth-proxy-image", "registry.redhat.io/openshift4/ose-oauth-proxy@sha256:4bef31eb993feb6f1096b51b4876c65a6fb1f4401fee97fa4f4542b6b7c9bc46"] securityContext: allowPrivilegeEscalation: false ports: diff --git a/components/odh-notebook-controller/controllers/notebook_oauth.go b/components/odh-notebook-controller/controllers/notebook_oauth.go index 81f78f94a5a..f289dfc2a27 100644 --- a/components/odh-notebook-controller/controllers/notebook_oauth.go +++ b/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.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 + // 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 // and kept in sync with the manifests here and in ClusterServiceVersion metadata of opendatahub operator - OAuthProxyImage = "registry.redhat.io/openshift4/ose-oauth-proxy@sha256:4f8d66597feeb32bb18699326029f9a71a5aca4a57679d636b876377c2e95695" + OAuthProxyImage = "registry.redhat.io/openshift4/ose-oauth-proxy@sha256:4bef31eb993feb6f1096b51b4876c65a6fb1f4401fee97fa4f4542b6b7c9bc46" ) type OAuthConfig struct { From 648689f8b009f373ce80ca458520b2a198688dc9 Mon Sep 17 00:00:00 2001 From: Jan Stourac Date: Thu, 17 Oct 2024 16:29:55 +0200 Subject: [PATCH 07/50] [RHOAIENG-11895] fix(odh-nbc): put a newline between certs in case input is missing a newline (#411) In case that the source of the certificates doesn't contain newline, this will assure that at least one new line is between the concatenated certificates. In case that the new line in the source is present already, then there will be two new lines now, but it shouldn't be a problem as it shouldn't break anything for us. This fixes [1]. * [1] https://issues.redhat.com/browse/RHOAIENG-11895 --- .../odh-notebook-controller/controllers/notebook_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/odh-notebook-controller/controllers/notebook_controller.go b/components/odh-notebook-controller/controllers/notebook_controller.go index 8e83dd2c273..cce2e87386f 100644 --- a/components/odh-notebook-controller/controllers/notebook_controller.go +++ b/components/odh-notebook-controller/controllers/notebook_controller.go @@ -304,7 +304,7 @@ func (r *OpenshiftNotebookReconciler) CreateNotebookCertConfigMap(notebook *nbv1 Labels: map[string]string{"opendatahub.io/managed-by": "workbenches"}, }, Data: map[string]string{ - "ca-bundle.crt": string(bytes.Join(rootCertPool, []byte{})), + "ca-bundle.crt": string(bytes.Join(rootCertPool, []byte("\n"))), }, } From 5d61120b23a8ca7b2c6470d69002586a2911ba7b Mon Sep 17 00:00:00 2001 From: GitHub Actions Date: Fri, 18 Oct 2024 08:49:45 +0000 Subject: [PATCH 08/50] Update odh and notebook-controller with image main-648689f --- .../notebook-controller/config/overlays/openshift/params.env | 2 +- components/odh-notebook-controller/Makefile | 2 +- components/odh-notebook-controller/config/base/params.env | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/components/notebook-controller/config/overlays/openshift/params.env b/components/notebook-controller/config/overlays/openshift/params.env index afc156ff326..aead3391138 100644 --- a/components/notebook-controller/config/overlays/openshift/params.env +++ b/components/notebook-controller/config/overlays/openshift/params.env @@ -1 +1 @@ -odh-kf-notebook-controller-image=quay.io/opendatahub/kubeflow-notebook-controller:main-7986030 +odh-kf-notebook-controller-image=quay.io/opendatahub/kubeflow-notebook-controller:main-648689f diff --git a/components/odh-notebook-controller/Makefile b/components/odh-notebook-controller/Makefile index 2795b4e96ad..8c223728285 100644 --- a/components/odh-notebook-controller/Makefile +++ b/components/odh-notebook-controller/Makefile @@ -4,7 +4,7 @@ IMG ?= quay.io/opendatahub/odh-notebook-controller TAG ?= $(shell git describe --tags --always) KF_IMG ?= quay.io/opendatahub/kubeflow-notebook-controller -KF_TAG ?= main-7986030 +KF_TAG ?= main-648689f CONTAINER_ENGINE ?= podman diff --git a/components/odh-notebook-controller/config/base/params.env b/components/odh-notebook-controller/config/base/params.env index 0cc0a43c330..a53394c9199 100644 --- a/components/odh-notebook-controller/config/base/params.env +++ b/components/odh-notebook-controller/config/base/params.env @@ -1 +1 @@ -odh-notebook-controller-image=quay.io/opendatahub/odh-notebook-controller:main-7986030 +odh-notebook-controller-image=quay.io/opendatahub/odh-notebook-controller:main-648689f From 9117408d7806bc953b308d8312c65f58faef7e3c Mon Sep 17 00:00:00 2001 From: Jan Stourac Date: Wed, 30 Oct 2024 18:36:16 +0100 Subject: [PATCH 09/50] [RHOAIENG-14687] Extend tests for certificate to assure that they are readable https://issues.redhat.com/browse/RHOAIENG-14687 --- .../controllers/notebook_controller_test.go | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/components/odh-notebook-controller/controllers/notebook_controller_test.go b/components/odh-notebook-controller/controllers/notebook_controller_test.go index 87369dd78ef..4d251af574d 100644 --- a/components/odh-notebook-controller/controllers/notebook_controller_test.go +++ b/components/odh-notebook-controller/controllers/notebook_controller_test.go @@ -17,6 +17,8 @@ package controllers import ( "context" + "crypto/x509" + "encoding/pem" "io/ioutil" "strings" "time" @@ -230,6 +232,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 +337,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) }) }) @@ -1039,3 +1053,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) +} From 4a185e08761da954ad15d7e02fdf69fa754dfc41 Mon Sep 17 00:00:00 2001 From: Jiri Danek Date: Mon, 4 Nov 2024 15:05:40 +0100 Subject: [PATCH 10/50] RHOAIENG-15335: feat(odh-nbc/webhook): only update oauth-proxy image in CREATE events --- .../controllers/notebook_controller_test.go | 32 ++++++++++--------- .../controllers/notebook_webhook.go | 18 +++++++---- 2 files changed, 28 insertions(+), 22 deletions(-) diff --git a/components/odh-notebook-controller/controllers/notebook_controller_test.go b/components/odh-notebook-controller/controllers/notebook_controller_test.go index 4d251af574d..5411440e3f8 100644 --- a/components/odh-notebook-controller/controllers/notebook_controller_test.go +++ b/components/odh-notebook-controller/controllers/notebook_controller_test.go @@ -608,21 +608,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) diff --git a/components/odh-notebook-controller/controllers/notebook_webhook.go b/components/odh-notebook-controller/controllers/notebook_webhook.go index bd91ae05470..49e2a009e48 100644 --- a/components/odh-notebook-controller/controllers/notebook_webhook.go +++ b/components/odh-notebook-controller/controllers/notebook_webhook.go @@ -265,13 +265,17 @@ func (w *NotebookWebhook) Handle(ctx context.Context, req admission.Request) adm } // Inject the OAuth proxy if the annotation is present but only if Service Mesh is disabled - if OAuthInjectionIsEnabled(notebook.ObjectMeta) { - if ServiceMeshIsEnabled(notebook.ObjectMeta) { - return admission.Denied(fmt.Sprintf("Cannot have both %s and %s set to true. Pick one.", AnnotationServiceMesh, AnnotationInjectOAuth)) - } - err = InjectOAuthProxy(notebook, w.OAuthConfig) - if err != nil { - return admission.Errored(http.StatusInternalServerError, err) + if OAuthInjectionIsEnabled(notebook.ObjectMeta) && ServiceMeshIsEnabled(notebook.ObjectMeta) { + return admission.Denied(fmt.Sprintf("Cannot have both %s and %s set to true. Pick one.", AnnotationServiceMesh, AnnotationInjectOAuth)) + } + // RHOAIENG-14552: Updating oauth-proxy in a running notebook may lead to notebook restart. Updating only + // on Create is safe as it cannot cause a restart in already running notebook when oauth-proxy image changes. + if req.Operation == admissionv1.Create { + if OAuthInjectionIsEnabled(notebook.ObjectMeta) { + err = InjectOAuthProxy(notebook, w.OAuthConfig) + if err != nil { + return admission.Errored(http.StatusInternalServerError, err) + } } } From 900f028b6314a54260bb7605fa48f023adab01fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jiri=20Dan=C4=9Bk?= Date: Wed, 6 Nov 2024 13:13:21 +0100 Subject: [PATCH 11/50] Revert "Revert "[v1.9-branch] RHOAIENG-10827: feat(nbcs): update ose-oauth-proxy image digest reference from 4.8 to the latest 4.14 version (#388)"" --- .../odh-notebook-controller/config/manager/manager.yaml | 2 +- .../odh-notebook-controller/controllers/notebook_oauth.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/components/odh-notebook-controller/config/manager/manager.yaml b/components/odh-notebook-controller/config/manager/manager.yaml index 991878bfbf3..98f25094bd8 100644 --- a/components/odh-notebook-controller/config/manager/manager.yaml +++ b/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: diff --git a/components/odh-notebook-controller/controllers/notebook_oauth.go b/components/odh-notebook-controller/controllers/notebook_oauth.go index f289dfc2a27..81f78f94a5a 100644 --- a/components/odh-notebook-controller/controllers/notebook_oauth.go +++ b/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 { From 3f931d21bcec377150ba01a8f32dde27339a3a7a Mon Sep 17 00:00:00 2001 From: aTheo Date: Thu, 7 Nov 2024 10:18:46 +0100 Subject: [PATCH 12/50] Introduce rbac controller to handle RoleBindings (#434) * 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 --- components/odh-notebook-controller/Makefile | 11 +- .../config/manager/manager.yaml | 3 + .../config/rbac/role.yaml | 23 +++ .../controllers/notebook_controller.go | 15 ++ .../controllers/notebook_controller_test.go | 75 +++++++++ .../controllers/notebook_rbac.go | 154 ++++++++++++++++++ 6 files changed, 279 insertions(+), 2 deletions(-) create mode 100644 components/odh-notebook-controller/controllers/notebook_rbac.go 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/config/manager/manager.yaml b/components/odh-notebook-controller/config/manager/manager.yaml index 98f25094bd8..89f450985d4 100644 --- a/components/odh-notebook-controller/config/manager/manager.yaml +++ b/components/odh-notebook-controller/config/manager/manager.yaml @@ -54,3 +54,6 @@ spec: requests: cpu: 500m memory: 256Mi + env: + - name: SET_PIPELINE_RBAC + value: "false" 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..3e667ed06cb 100644 --- a/components/odh-notebook-controller/controllers/notebook_controller.go +++ b/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 diff --git a/components/odh-notebook-controller/controllers/notebook_controller_test.go b/components/odh-notebook-controller/controllers/notebook_controller_test.go index 5411440e3f8..262f46c0646 100644 --- a/components/odh-notebook-controller/controllers/notebook_controller_test.go +++ b/components/odh-notebook-controller/controllers/notebook_controller_test.go @@ -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" @@ -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 ( 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..6055eceb974 --- /dev/null +++ b/components/odh-notebook-controller/controllers/notebook_rbac.go @@ -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 +} From 4ff9d77323e8dab6da00985366aa5885ad6824aa Mon Sep 17 00:00:00 2001 From: GitHub Actions Date: Thu, 7 Nov 2024 10:43:50 +0000 Subject: [PATCH 13/50] Update odh and notebook-controller with image main-3f931d2 --- .../notebook-controller/config/overlays/openshift/params.env | 2 +- components/odh-notebook-controller/Makefile | 2 +- components/odh-notebook-controller/config/base/params.env | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/components/notebook-controller/config/overlays/openshift/params.env b/components/notebook-controller/config/overlays/openshift/params.env index aead3391138..2d30cc53dba 100644 --- a/components/notebook-controller/config/overlays/openshift/params.env +++ b/components/notebook-controller/config/overlays/openshift/params.env @@ -1 +1 @@ -odh-kf-notebook-controller-image=quay.io/opendatahub/kubeflow-notebook-controller:main-648689f +odh-kf-notebook-controller-image=quay.io/opendatahub/kubeflow-notebook-controller:main-3f931d2 diff --git a/components/odh-notebook-controller/Makefile b/components/odh-notebook-controller/Makefile index 9fdb3af172b..93a0b9f6344 100644 --- a/components/odh-notebook-controller/Makefile +++ b/components/odh-notebook-controller/Makefile @@ -4,7 +4,7 @@ IMG ?= quay.io/opendatahub/odh-notebook-controller TAG ?= $(shell git describe --tags --always) KF_IMG ?= quay.io/opendatahub/kubeflow-notebook-controller -KF_TAG ?= main-648689f +KF_TAG ?= main-3f931d2 CONTAINER_ENGINE ?= podman diff --git a/components/odh-notebook-controller/config/base/params.env b/components/odh-notebook-controller/config/base/params.env index a53394c9199..f062a8b7f34 100644 --- a/components/odh-notebook-controller/config/base/params.env +++ b/components/odh-notebook-controller/config/base/params.env @@ -1 +1 @@ -odh-notebook-controller-image=quay.io/opendatahub/odh-notebook-controller:main-648689f +odh-notebook-controller-image=quay.io/opendatahub/odh-notebook-controller:main-3f931d2 From 0f3c7427f6aaf6762e886de52b17ec5ac03ad6f5 Mon Sep 17 00:00:00 2001 From: Jan Stourac Date: Thu, 24 Oct 2024 12:13:45 +0200 Subject: [PATCH 14/50] [Test] Update the k8s tool used for the test execution The version 1.23 is no longer supported [1] and only releases from 1.28 - 1.31 versions are supported. Based on the [2], the OpenShift 4.13 (oldest we care about right now) is based on Kubernetes in version 1.26, so let's allign to that version. * [1] https://kubernetes.io/releases/ * [2] https://access.redhat.com/solutions/4870701 --- components/odh-notebook-controller/Makefile | 2 +- components/profile-controller/Makefile | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/components/odh-notebook-controller/Makefile b/components/odh-notebook-controller/Makefile index 93a0b9f6344..2c97e351bb7 100644 --- a/components/odh-notebook-controller/Makefile +++ b/components/odh-notebook-controller/Makefile @@ -9,7 +9,7 @@ KF_TAG ?= main-3f931d2 CONTAINER_ENGINE ?= podman # ENVTEST_K8S_VERSION refers to the version of kubebuilder assets to be downloaded by envtest binary. -ENVTEST_K8S_VERSION = 1.23 +ENVTEST_K8S_VERSION = 1.26 # Kubernetes configuration K8S_NAMESPACE ?= odh-notebook-controller-system diff --git a/components/profile-controller/Makefile b/components/profile-controller/Makefile index 40b96cf9730..10f0ad4dc3c 100644 --- a/components/profile-controller/Makefile +++ b/components/profile-controller/Makefile @@ -4,7 +4,7 @@ TAG ?= $(shell git describe --tags --always --dirty) ARCH ?= linux/amd64 # ENVTEST_K8S_VERSION refers to the version of kubebuilder assets to be downloaded by envtest binary. -ENVTEST_K8S_VERSION = 1.23 +ENVTEST_K8S_VERSION = 1.26 # Get the currently used golang install path (in GOPATH/bin, unless GOBIN is set) ifeq (,$(shell go env GOBIN)) From 8dba73129b19ed0a575fc8f90d9315bc735427a3 Mon Sep 17 00:00:00 2001 From: Jan Stourac Date: Fri, 25 Oct 2024 15:59:37 +0200 Subject: [PATCH 15/50] [Fix] kustomize package version revision This is a followup of #391 (1b2dd4fa594937e81aeae886c476cecef19e6dc7). Without this, we can see the following failure during the e2e tests execution: ``` make run-ci-e2e-tests ... GOBIN=/home/jstourac/workspace/rhosai/odh/kubeflow/components/odh-notebook-controller/bin go install sigs.k8s.io/kustomize/v3/cmd/kustomize@v5.0.2 go: sigs.k8s.io/kustomize/v3/cmd/kustomize@v5.0.2: sigs.k8s.io/kustomize/v3/cmd/kustomize@v5.0.2: invalid version: unknown revision v3/cmd/kustomize/v5.0.2 make[1]: *** [Makefile:243: kustomize] Error 1 ... ``` This caused that all e2e tests failed. After this fix, some e2e tests pass. The other seem to fail on a problem unrelated to this one. --- components/notebook-controller/Makefile | 2 +- components/odh-notebook-controller/Makefile | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/components/notebook-controller/Makefile b/components/notebook-controller/Makefile index 019d1bd92eb..9d698c4bc67 100644 --- a/components/notebook-controller/Makefile +++ b/components/notebook-controller/Makefile @@ -148,7 +148,7 @@ controller-gen: ## Download controller-gen locally if necessary. KUSTOMIZE = $(shell pwd)/bin/kustomize .PHONY: kustomize kustomize: ## Download kustomize locally if necessary. - $(call go-get-tool,$(KUSTOMIZE),sigs.k8s.io/kustomize/v3/cmd/kustomize@v5.0.2) + $(call go-get-tool,$(KUSTOMIZE),sigs.k8s.io/kustomize/kustomize/v5@v5.0.2) ENVTEST = $(shell pwd)/bin/setup-envtest ENVTEST_VERSION?=v0.0.0-20240923090159-236e448db12c diff --git a/components/odh-notebook-controller/Makefile b/components/odh-notebook-controller/Makefile index 2c97e351bb7..610189de3f8 100644 --- a/components/odh-notebook-controller/Makefile +++ b/components/odh-notebook-controller/Makefile @@ -241,7 +241,7 @@ controller-gen: ## Download controller-gen locally if necessary. KUSTOMIZE = $(LOCALBIN)/kustomize .PHONY: kustomize kustomize: ## Download kustomize locally if necessary. - GOBIN=$(LOCALBIN) go install sigs.k8s.io/kustomize/v3/cmd/kustomize@v5.0.2 + GOBIN=$(LOCALBIN) go install sigs.k8s.io/kustomize/kustomize/v5@v5.0.2 ENVTEST = $(LOCALBIN)/setup-envtest .PHONY: envtest From 92683c10b4cb0080a9661c131eb078654319f919 Mon Sep 17 00:00:00 2001 From: Jiri Danek Date: Tue, 12 Nov 2024 08:30:08 +0100 Subject: [PATCH 16/50] Revert "RHOAIENG-15335: feat(odh-nbc/webhook): only update oauth-proxy image in CREATE events" We have a better solution of blocking updates that * are initiated by the odh-notebook-controller webhook itself * that would lead to pod restart * when the notebook is in running state and there is no external update that itself causes restart This reverts commit 4a185e08761da954ad15d7e02fdf69fa754dfc41. --- .../controllers/notebook_controller_test.go | 32 +++++++++---------- .../controllers/notebook_webhook.go | 18 ++++------- 2 files changed, 22 insertions(+), 28 deletions(-) diff --git a/components/odh-notebook-controller/controllers/notebook_controller_test.go b/components/odh-notebook-controller/controllers/notebook_controller_test.go index 262f46c0646..65e96d66da0 100644 --- a/components/odh-notebook-controller/controllers/notebook_controller_test.go +++ b/components/odh-notebook-controller/controllers/notebook_controller_test.go @@ -683,23 +683,21 @@ var _ = Describe("The Openshift Notebook controller", func() { }, duration, interval).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()) - //}) + 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) diff --git a/components/odh-notebook-controller/controllers/notebook_webhook.go b/components/odh-notebook-controller/controllers/notebook_webhook.go index 49e2a009e48..bd91ae05470 100644 --- a/components/odh-notebook-controller/controllers/notebook_webhook.go +++ b/components/odh-notebook-controller/controllers/notebook_webhook.go @@ -265,17 +265,13 @@ func (w *NotebookWebhook) Handle(ctx context.Context, req admission.Request) adm } // Inject the OAuth proxy if the annotation is present but only if Service Mesh is disabled - if OAuthInjectionIsEnabled(notebook.ObjectMeta) && ServiceMeshIsEnabled(notebook.ObjectMeta) { - return admission.Denied(fmt.Sprintf("Cannot have both %s and %s set to true. Pick one.", AnnotationServiceMesh, AnnotationInjectOAuth)) - } - // RHOAIENG-14552: Updating oauth-proxy in a running notebook may lead to notebook restart. Updating only - // on Create is safe as it cannot cause a restart in already running notebook when oauth-proxy image changes. - if req.Operation == admissionv1.Create { - if OAuthInjectionIsEnabled(notebook.ObjectMeta) { - err = InjectOAuthProxy(notebook, w.OAuthConfig) - if err != nil { - return admission.Errored(http.StatusInternalServerError, err) - } + if OAuthInjectionIsEnabled(notebook.ObjectMeta) { + if ServiceMeshIsEnabled(notebook.ObjectMeta) { + return admission.Denied(fmt.Sprintf("Cannot have both %s and %s set to true. Pick one.", AnnotationServiceMesh, AnnotationInjectOAuth)) + } + err = InjectOAuthProxy(notebook, w.OAuthConfig) + if err != nil { + return admission.Errored(http.StatusInternalServerError, err) } } From 2ded5b5aed437d6b411e2b6456474ecd75e2a192 Mon Sep 17 00:00:00 2001 From: Jiri Danek Date: Mon, 4 Nov 2024 08:43:48 +0100 Subject: [PATCH 17/50] RHOAIENG-15335: feat(odh-nbc/webhook): don't initiate updates that would restart a running pod --- .../controllers/notebook_webhook.go | 68 ++++++++++++++++++- 1 file changed, 67 insertions(+), 1 deletion(-) diff --git a/components/odh-notebook-controller/controllers/notebook_webhook.go b/components/odh-notebook-controller/controllers/notebook_webhook.go index bd91ae05470..ab79e2ab337 100644 --- a/components/odh-notebook-controller/controllers/notebook_webhook.go +++ b/components/odh-notebook-controller/controllers/notebook_webhook.go @@ -28,6 +28,7 @@ import ( "github.com/kubeflow/kubeflow/components/notebook-controller/pkg/culler" admissionv1 "k8s.io/api/admission/v1" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" @@ -275,8 +276,21 @@ func (w *NotebookWebhook) Handle(ctx context.Context, req admission.Request) adm } } + // RHOAIENG-14552: Running notebook cannot be updated carelessly, or we may end up restarting the pod when + // the webhook runs after e.g. the oauth-proxy image has been updated + mutatedNotebook, needsRestart, err := w.maybeRestartRunningNotebook(req, notebook) + if err != nil { + return admission.Errored(http.StatusInternalServerError, err) + } + updatePendingAnnotation := "notebooks.opendatahub.io/update-pending" + if needsRestart { + mutatedNotebook.ObjectMeta.Annotations[updatePendingAnnotation] = "true" + } else { + mutatedNotebook.ObjectMeta.Annotations[updatePendingAnnotation] = "false" + } + // Create the mutated notebook object - marshaledNotebook, err := json.Marshal(notebook) + marshaledNotebook, err := json.Marshal(mutatedNotebook) if err != nil { return admission.Errored(http.StatusInternalServerError, err) } @@ -290,6 +304,58 @@ func (w *NotebookWebhook) InjectDecoder(d *admission.Decoder) error { return nil } +// maybeRestartRunningNotebook evaluates whether the updates being made cause notebook pod to restart. +// If the restart is caused by updates made by the mutating webhook itself to already existing notebook, +// and the notebook is not stopped, then these updates will be blocked until the notebook is stopped. +// Returns the mutated notebook, a flag whether there's a pending restart (to apply blocked updates), and an error value. +func (w *NotebookWebhook) maybeRestartRunningNotebook(req admission.Request, mutatedNotebook *nbv1.Notebook) (*nbv1.Notebook, bool, error) { + var err error + + // Notebook that was just created can be updated + if req.Operation == admissionv1.Create { + return mutatedNotebook, false, nil + } + + // Stopped notebooks are ok to update + if metav1.HasAnnotation(mutatedNotebook.ObjectMeta, "kubeflow-resource-stopped") { + return mutatedNotebook, false, nil + } + + // Restarting notebooks are also ok to update + if metav1.HasAnnotation(mutatedNotebook.ObjectMeta, "notebooks.opendatahub.io/notebook-restart") { + return mutatedNotebook, false, nil + } + + // fetch the updated Notebook CR that was sent to the Webhook + updatedNotebook := &nbv1.Notebook{} + err = w.Decoder.Decode(req, updatedNotebook) + if err != nil { + return nil, false, err + } + + // fetch the original Notebook CR + oldNotebook := &nbv1.Notebook{} + err = w.Decoder.DecodeRaw(req.OldObject, oldNotebook) + if err != nil { + return nil, false, err + } + + // The externally issued update already causes a restart, so we will just let all changes proceed + if !equality.Semantic.DeepEqual(oldNotebook.Spec.Template.Spec, updatedNotebook.Spec.Template.Spec) { + return mutatedNotebook, false, nil + } + + // Nothing about the Pod definition is actually changing and we can proceed + if equality.Semantic.DeepEqual(oldNotebook.Spec.Template.Spec, mutatedNotebook.Spec.Template.Spec) { + return mutatedNotebook, false, nil + } + + // Now we know we have to block the update + // Keep the old values and mark the Notebook as UpdatesPending + mutatedNotebook.Spec.Template.Spec = updatedNotebook.Spec.Template.Spec + return mutatedNotebook, true, nil +} + // CheckAndMountCACertBundle checks if the odh-trusted-ca-bundle ConfigMap is present func CheckAndMountCACertBundle(ctx context.Context, cli client.Client, notebook *nbv1.Notebook, log logr.Logger) error { From 1a1894b8642f7b31492bcef07c6e626a5032fe76 Mon Sep 17 00:00:00 2001 From: Jiri Danek Date: Tue, 12 Nov 2024 08:23:39 +0100 Subject: [PATCH 18/50] RHOAIENG-15603: feat(odh-nbc/webhook): add logging, and a Notebook CR metadata value with pending-update reason also a little unittest is added --- .../controllers/notebook_webhook.go | 37 ++++++--- .../controllers/notebook_webhook_utils.go | 79 +++++++++++++++++++ .../notebook_webhook_utils_test.go | 64 +++++++++++++++ components/odh-notebook-controller/go.mod | 2 +- 4 files changed, 168 insertions(+), 14 deletions(-) create mode 100644 components/odh-notebook-controller/controllers/notebook_webhook_utils.go create mode 100644 components/odh-notebook-controller/controllers/notebook_webhook_utils_test.go diff --git a/components/odh-notebook-controller/controllers/notebook_webhook.go b/components/odh-notebook-controller/controllers/notebook_webhook.go index ab79e2ab337..0f3163bd758 100644 --- a/components/odh-notebook-controller/controllers/notebook_webhook.go +++ b/components/odh-notebook-controller/controllers/notebook_webhook.go @@ -233,6 +233,7 @@ func (w *NotebookWebhook) Handle(ctx context.Context, req admission.Request) adm // Initialize logger format log := w.Log.WithValues("notebook", req.Name, "namespace", req.Namespace) + ctx = logr.NewContext(ctx, log) notebook := &nbv1.Notebook{} @@ -278,15 +279,15 @@ func (w *NotebookWebhook) Handle(ctx context.Context, req admission.Request) adm // RHOAIENG-14552: Running notebook cannot be updated carelessly, or we may end up restarting the pod when // the webhook runs after e.g. the oauth-proxy image has been updated - mutatedNotebook, needsRestart, err := w.maybeRestartRunningNotebook(req, notebook) + mutatedNotebook, needsRestart, err := w.maybeRestartRunningNotebook(ctx, req, notebook) if err != nil { return admission.Errored(http.StatusInternalServerError, err) } updatePendingAnnotation := "notebooks.opendatahub.io/update-pending" - if needsRestart { - mutatedNotebook.ObjectMeta.Annotations[updatePendingAnnotation] = "true" + if needsRestart != NoPendingUpdates { + mutatedNotebook.ObjectMeta.Annotations[updatePendingAnnotation] = needsRestart.Reason } else { - mutatedNotebook.ObjectMeta.Annotations[updatePendingAnnotation] = "false" + delete(mutatedNotebook.ObjectMeta.Annotations, updatePendingAnnotation) } // Create the mutated notebook object @@ -308,52 +309,62 @@ func (w *NotebookWebhook) InjectDecoder(d *admission.Decoder) error { // If the restart is caused by updates made by the mutating webhook itself to already existing notebook, // and the notebook is not stopped, then these updates will be blocked until the notebook is stopped. // Returns the mutated notebook, a flag whether there's a pending restart (to apply blocked updates), and an error value. -func (w *NotebookWebhook) maybeRestartRunningNotebook(req admission.Request, mutatedNotebook *nbv1.Notebook) (*nbv1.Notebook, bool, error) { +func (w *NotebookWebhook) maybeRestartRunningNotebook(ctx context.Context, req admission.Request, mutatedNotebook *nbv1.Notebook) (*nbv1.Notebook, *UpdatesPending, error) { var err error + log := logr.FromContextOrDiscard(ctx) // Notebook that was just created can be updated if req.Operation == admissionv1.Create { - return mutatedNotebook, false, nil + log.Info("Not blocking update, notebook is being newly created") + return mutatedNotebook, NoPendingUpdates, nil } // Stopped notebooks are ok to update if metav1.HasAnnotation(mutatedNotebook.ObjectMeta, "kubeflow-resource-stopped") { - return mutatedNotebook, false, nil + log.Info("Not blocking update, notebook is (to be) stopped") + return mutatedNotebook, NoPendingUpdates, nil } // Restarting notebooks are also ok to update if metav1.HasAnnotation(mutatedNotebook.ObjectMeta, "notebooks.opendatahub.io/notebook-restart") { - return mutatedNotebook, false, nil + log.Info("Not blocking update, notebook is (to be) restarted") + return mutatedNotebook, NoPendingUpdates, nil } // fetch the updated Notebook CR that was sent to the Webhook updatedNotebook := &nbv1.Notebook{} err = w.Decoder.Decode(req, updatedNotebook) if err != nil { - return nil, false, err + log.Error(err, "Failed to fetch the updated Notebook CR") + return nil, NoPendingUpdates, err } // fetch the original Notebook CR oldNotebook := &nbv1.Notebook{} err = w.Decoder.DecodeRaw(req.OldObject, oldNotebook) if err != nil { - return nil, false, err + log.Error(err, "Failed to fetch the original Notebook CR") + return nil, NoPendingUpdates, err } // The externally issued update already causes a restart, so we will just let all changes proceed if !equality.Semantic.DeepEqual(oldNotebook.Spec.Template.Spec, updatedNotebook.Spec.Template.Spec) { - return mutatedNotebook, false, nil + log.Info("Not blocking update, the externally issued update already modifies pod template, causing a restart") + return mutatedNotebook, NoPendingUpdates, nil } // Nothing about the Pod definition is actually changing and we can proceed if equality.Semantic.DeepEqual(oldNotebook.Spec.Template.Spec, mutatedNotebook.Spec.Template.Spec) { - return mutatedNotebook, false, nil + log.Info("Not blocking update, the pod template is not being modified at all") + return mutatedNotebook, NoPendingUpdates, nil } // Now we know we have to block the update // Keep the old values and mark the Notebook as UpdatesPending + diff := getStructDiff(ctx, mutatedNotebook.Spec.Template.Spec, updatedNotebook.Spec.Template.Spec) + log.Info("Update blocked, notebook pod template would be changed by the webhook", "diff", diff) mutatedNotebook.Spec.Template.Spec = updatedNotebook.Spec.Template.Spec - return mutatedNotebook, true, nil + return mutatedNotebook, &UpdatesPending{Reason: diff}, nil } // CheckAndMountCACertBundle checks if the odh-trusted-ca-bundle ConfigMap is present diff --git a/components/odh-notebook-controller/controllers/notebook_webhook_utils.go b/components/odh-notebook-controller/controllers/notebook_webhook_utils.go new file mode 100644 index 00000000000..dcc9f06f052 --- /dev/null +++ b/components/odh-notebook-controller/controllers/notebook_webhook_utils.go @@ -0,0 +1,79 @@ +/* + +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" + "fmt" + "github.com/google/go-cmp/cmp" + + "github.com/go-logr/logr" +) + +// UpdatesPending is either NoPendingUpdates, or a new value providing a Reason for the update. +type UpdatesPending struct { + Reason string +} + +var ( + NoPendingUpdates = &UpdatesPending{} +) + +// FirstDifferenceReporter is a custom reporter that only records the first difference. +type FirstDifferenceReporter struct { + path cmp.Path + diff string +} + +func (r *FirstDifferenceReporter) PushStep(ps cmp.PathStep) { + r.path = append(r.path, ps) +} + +func (r *FirstDifferenceReporter) Report(rs cmp.Result) { + if r.diff == "" && !rs.Equal() { + vx, vy := r.path.Last().Values() + r.diff = fmt.Sprintf("%#v: %+v != %+v", r.path, vx, vy) + } +} + +func (r *FirstDifferenceReporter) PopStep() { + r.path = r.path[:len(r.path)-1] +} + +func (r *FirstDifferenceReporter) String() string { + return r.diff +} + +func getStructDiff(ctx context.Context, a any, b any) (result string) { + log := logr.FromContextOrDiscard(ctx) + + // calling cmp.Equal may panic, get ready for it + result = "failed to compute the reason for why there is a pending restart" + defer func() { + if r := recover(); r != nil { + log.Error(fmt.Errorf("failed to compute struct difference: %+v", r), "Cannot determine reason for restart") + } + }() + + var comparator FirstDifferenceReporter + eq := cmp.Equal(a, b, cmp.Reporter(&comparator)) + if eq { + log.Error(nil, "Unexpectedly attempted to diff structs that are actually equal") + } + result = comparator.String() + + return +} diff --git a/components/odh-notebook-controller/controllers/notebook_webhook_utils_test.go b/components/odh-notebook-controller/controllers/notebook_webhook_utils_test.go new file mode 100644 index 00000000000..8761f874abf --- /dev/null +++ b/components/odh-notebook-controller/controllers/notebook_webhook_utils_test.go @@ -0,0 +1,64 @@ +/* + +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" + "testing" + + v1 "k8s.io/api/core/v1" + + "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/assert" +) + +func TestFirstDifferenceReporter(t *testing.T) { + for _, tt := range []struct { + name string + a any + b any + diff string + }{ + {"", 42, 42, ""}, + {"", v1.Pod{Spec: v1.PodSpec{NodeName: "node1"}}, v1.Pod{Spec: v1.PodSpec{NodeName: "node2"}}, "{v1.Pod}.Spec.NodeName: node1 != node2"}, + } { + t.Run(tt.name, func(t *testing.T) { + var reporter FirstDifferenceReporter + eq := cmp.Equal(tt.a, tt.b, cmp.Reporter(&reporter)) + assert.Equal(t, tt.diff == "", eq) + assert.Equal(t, tt.diff, reporter.String()) + }) + } +} + +func TestGetStructDiff(t *testing.T) { + var tests = []struct { + name string + a any + b any + expected string + }{ + {"simple numbers", 42, 42, ""}, + {"differing pods", v1.Pod{Spec: v1.PodSpec{NodeName: "node1"}}, v1.Pod{Spec: v1.PodSpec{NodeName: "node2"}}, "{v1.Pod}.Spec.NodeName: node1 != node2"}, + } + + for _, v := range tests { + t.Run(v.name, func(t *testing.T) { + diff := getStructDiff(context.Background(), v.a, v.b) + assert.Equal(t, diff, v.expected) + }) + } +} diff --git a/components/odh-notebook-controller/go.mod b/components/odh-notebook-controller/go.mod index d9b788e28c5..60a7fe2f2ad 100644 --- a/components/odh-notebook-controller/go.mod +++ b/components/odh-notebook-controller/go.mod @@ -6,6 +6,7 @@ toolchain go1.21.9 require ( github.com/go-logr/logr v1.4.1 + github.com/google/go-cmp v0.6.0 github.com/kubeflow/kubeflow/components/notebook-controller v0.0.0-20220728153354-fc09bd1eefb8 github.com/onsi/ginkgo v1.16.5 github.com/onsi/gomega v1.30.0 @@ -36,7 +37,6 @@ require ( github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect github.com/golang/protobuf v1.5.3 // indirect github.com/google/gnostic-models v0.6.8 // indirect - github.com/google/go-cmp v0.6.0 // indirect github.com/google/gofuzz v1.2.0 // indirect github.com/google/uuid v1.3.0 // indirect github.com/imdario/mergo v0.3.12 // indirect From ce8b637e26035d703576969da0e6ecfbcdc35d6e Mon Sep 17 00:00:00 2001 From: Jiri Danek Date: Tue, 12 Nov 2024 13:16:40 +0100 Subject: [PATCH 19/50] add more comments for notebook_webhook_utils.go --- .../controllers/notebook_webhook_utils.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/components/odh-notebook-controller/controllers/notebook_webhook_utils.go b/components/odh-notebook-controller/controllers/notebook_webhook_utils.go index dcc9f06f052..e70657ccda5 100644 --- a/components/odh-notebook-controller/controllers/notebook_webhook_utils.go +++ b/components/odh-notebook-controller/controllers/notebook_webhook_utils.go @@ -32,7 +32,7 @@ var ( NoPendingUpdates = &UpdatesPending{} ) -// FirstDifferenceReporter is a custom reporter that only records the first difference. +// FirstDifferenceReporter is a custom go-cmp reporter that only records the first difference. type FirstDifferenceReporter struct { path cmp.Path diff string @@ -57,6 +57,7 @@ func (r *FirstDifferenceReporter) String() string { return r.diff } +// getStructDiff compares a and b, reporting the first difference it found in a human-readable single-line string. func getStructDiff(ctx context.Context, a any, b any) (result string) { log := logr.FromContextOrDiscard(ctx) From 9ebd04ee44663c71434fdd025efd05bb7cbc132d Mon Sep 17 00:00:00 2001 From: Jiri Danek Date: Thu, 14 Nov 2024 01:07:40 +0100 Subject: [PATCH 20/50] NO-JIRA: tests(odh-nbc): shorten the test certificates Shorter certificates fit better on the screen. --- .../controllers/notebook_controller_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/components/odh-notebook-controller/controllers/notebook_controller_test.go b/components/odh-notebook-controller/controllers/notebook_controller_test.go index 65e96d66da0..817ed7aa614 100644 --- a/components/odh-notebook-controller/controllers/notebook_controller_test.go +++ b/components/odh-notebook-controller/controllers/notebook_controller_test.go @@ -258,9 +258,11 @@ var _ = Describe("The Openshift Notebook controller", func() { map[string]string{ "config.openshift.io/inject-trusted-cabundle": "true", }, + // NOTE: use valid short CA certs and make them each be different + // $ openssl req -nodes -x509 -newkey ed25519 -days 365 -set_serial 1 -out /dev/stdout -subj "/" map[string]string{ - "ca-bundle.crt": "-----BEGIN CERTIFICATE-----\nMIIB8jCCAXigAwIBAgITBmyf18G7EEwpQ+Vxe3ssyBrBDjAKBggqhkjOPQQDAzA5MQswCQYDVQQGEwJVUzEPMA0GA1UEChMGQW1hem9uMRkwFwYDVQQDExBBbWF6b24gUm9vdCBDQSA0MB4XDTE1MDUyNjAwMDAwMFoXDTQwMDUyNjAwMDAwMFowOTELMAkGA1UEBhMCVVMxDzANBgNVBAoTBkFtYXpvbjEZMBcGA1UEAxMQQW1hem9uIFJvb3QgQ0EgNDB2MBAGByqGSM49AgEGBSuBBAAiA2IABNKrijdPo1MN/sGKe0uoe0ZLY7Bi9i0b2whxIdIA6GO9mif78DluXeo9pcmBqqNbIJhFXRbb/egQbeOc4OO9X4Ri83BkM6DLJC9wuoihKqB1+IGuYgbEgds5bimwHvouXKNCMEAwDwYDVR0TAQH/BAUwAwEB/zAOBgNVHQ8BAf8EBAMCAYYwHQYDVR0OBBYEFNPsxzplbszh2naaVvuc84ZtV+WBMAoGCCqGSM49BAMDA2gAMGUCMDqLIfG9fhGt0O9Yli/W651+kI0rz2ZVwyzjKKlwCkcO8DdZEv8tmZQoTipPNU0zWgIxAOp1AE47xDqUEpHJWEadIRNyp4iciuRMStuW1KyLa2tJElMzrdfkviT8tQp21KW8EA==\n-----END CERTIFICATE-----", - "odh-ca-bundle.crt": "-----BEGIN CERTIFICATE-----\nMIIB8jCCAXigAwIBAgITBmyf18G7EEwpQ+Vxe3ssyBrBDjAKBggqhkjOPQQDAzA5MQswCQYDVQQGEwJVUzEPMA0GA1UEChMGQW1hem9uMRkwFwYDVQQDExBBbWF6b24gUm9vdCBDQSA0MB4XDTE1MDUyNjAwMDAwMFoXDTQwMDUyNjAwMDAwMFowOTELMAkGA1UEBhMCVVMxDzANBgNVBAoTBkFtYXpvbjEZMBcGA1UEAxMQQW1hem9uIFJvb3QgQ0EgNDB2MBAGByqGSM49AgEGBSuBBAAiA2IABNKrijdPo1MN/sGKe0uoe0ZLY7Bi9i0b2whxIdIA6GO9mif78DluXeo9pcmBqqNbIJhFXRbb/egQbeOc4OO9X4Ri83BkM6DLJC9wuoihKqB1+IGuYgbEgds5bimwHvouXKNCMEAwDwYDVR0TAQH/BAUwAwEB/zAOBgNVHQ8BAf8EBAMCAYYwHQYDVR0OBBYEFNPsxzplbszh2naaVvuc84ZtV+WBMAoGCCqGSM49BAMDA2gAMGUCMDqLIfG9fhGt0O9Yli/W651+kI0rz2ZVwyzjKKlwCkcO8DdZEv8tmZQoTipPNU0zWgIxAOp1AE47xDqUEpHJWEadIRNyp4iciuRMStuW1KyLa2tJElMzrdfkviT8tQp21KW8EA==\n-----END CERTIFICATE-----", + "ca-bundle.crt": "-----BEGIN CERTIFICATE-----\nMIGrMF+gAwIBAgIBATAFBgMrZXAwADAeFw0yNDExMTMyMzI3MzdaFw0yNTExMTMy\nMzI3MzdaMAAwKjAFBgMrZXADIQDEMMlJ1P0gyxEV7A8PgpNosvKZgE4ttDDpu/w9\n35BHzjAFBgMrZXADQQDHT8ulalOcI6P5lGpoRcwLzpa4S/5pyqtbqw2zuj7dIJPI\ndNb1AkbARd82zc9bF+7yDkCNmLIHSlDORUYgTNEL\n-----END CERTIFICATE-----", + "odh-ca-bundle.crt": "-----BEGIN CERTIFICATE-----\nMIGrMF+gAwIBAgIBATAFBgMrZXAwADAeFw0yNDExMTMyMzI2NTlaFw0yNTExMTMy\nMzI2NTlaMAAwKjAFBgMrZXADIQB/v02zcoIIcuan/8bd7cvrBuCGTuVZBrYr1RdA\n0k58yzAFBgMrZXADQQBKsL1tkpOZ6NW+zEX3mD7bhmhxtODQHnANMXEXs0aljWrm\nAxDrLdmzsRRYFYxe23OdXhWqPs8SfO8EZWEvXoME\n-----END CERTIFICATE-----", }) // Create the ConfigMap @@ -363,8 +365,8 @@ var _ = Describe("The Openshift Notebook controller", func() { "config.openshift.io/inject-trusted-cabundle": "true", }, map[string]string{ - "ca-bundle.crt": "-----BEGIN CERTIFICATE-----\nMIIB8jCCAXigAwIBAgITBmyf18G7EEwpQ+Vxe3ssyBrBDjAKBggqhkjOPQQDAzA5MQswCQYDVQQGEwJVUzEPMA0GA1UEChMGQW1hem9uMRkwFwYDVQQDExBBbWF6b24gUm9vdCBDQSA0MB4XDTE1MDUyNjAwMDAwMFoXDTQwMDUyNjAwMDAwMFowOTELMAkGA1UEBhMCVVMxDzANBgNVBAoTBkFtYXpvbjEZMBcGA1UEAxMQQW1hem9uIFJvb3QgQ0EgNDB2MBAGByqGSM49AgEGBSuBBAAiA2IABNKrijdPo1MN/sGKe0uoe0ZLY7Bi9i0b2whxIdIA6GO9mif78DluXeo9pcmBqqNbIJhFXRbb/egQbeOc4OO9X4Ri83BkM6DLJC9wuoihKqB1+IGuYgbEgds5bimwHvouXKNCMEAwDwYDVR0TAQH/BAUwAwEB/zAOBgNVHQ8BAf8EBAMCAYYwHQYDVR0OBBYEFNPsxzplbszh2naaVvuc84ZtV+WBMAoGCCqGSM49BAMDA2gAMGUCMDqLIfG9fhGt0O9Yli/W651+kI0rz2ZVwyzjKKlwCkcO8DdZEv8tmZQoTipPNU0zWgIxAOp1AE47xDqUEpHJWEadIRNyp4iciuRMStuW1KyLa2tJElMzrdfkviT8tQp21KW8EA==\n-----END CERTIFICATE-----", - "odh-ca-bundle.crt": "-----BEGIN CERTIFICATE-----\nMIIB8jCCAXigAwIBAgITBmyf18G7EEwpQ+Vxe3ssyBrBDjAKBggqhkjOPQQDAzA5MQswCQYDVQQGEwJVUzEPMA0GA1UEChMGQW1hem9uMRkwFwYDVQQDExBBbWF6b24gUm9vdCBDQSA0MB4XDTE1MDUyNjAwMDAwMFoXDTQwMDUyNjAwMDAwMFowOTELMAkGA1UEBhMCVVMxDzANBgNVBAoTBkFtYXpvbjEZMBcGA1UEAxMQQW1hem9uIFJvb3QgQ0EgNDB2MBAGByqGSM49AgEGBSuBBAAiA2IABNKrijdPo1MN/sGKe0uoe0ZLY7Bi9i0b2whxIdIA6GO9mif78DluXeo9pcmBqqNbIJhFXRbb/egQbeOc4OO9X4Ri83BkM6DLJC9wuoihKqB1+IGuYgbEgds5bimwHvouXKNCMEAwDwYDVR0TAQH/BAUwAwEB/zAOBgNVHQ8BAf8EBAMCAYYwHQYDVR0OBBYEFNPsxzplbszh2naaVvuc84ZtV+WBMAoGCCqGSM49BAMDA2gAMGUCMDqLIfG9fhGt0O9Yli/W651+kI0rz2ZVwyzjKKlwCkcO8DdZEv8tmZQoTipPNU0zWgIxAOp1AE47xDqUEpHJWEadIRNyp4iciuRMStuW1KyLa2tJElMzrdfkviT8tQp21KW8EA==\n-----END CERTIFICATE-----", + "ca-bundle.crt": "-----BEGIN CERTIFICATE-----\nMIGrMF+gAwIBAgIBATAFBgMrZXAwADAeFw0yNDExMTMyMzI4MjZaFw0yNTExMTMy\nMzI4MjZaMAAwKjAFBgMrZXADIQD77pLvWIX0WmlkYthRZ79oIf7qrGO7yECf668T\nSB42vTAFBgMrZXADQQDs76j81LPh+lgnnf4L0ROUqB66YiBx9SyDTjm83Ya4KC+2\nLEP6Mw1//X2DX89f1chy7RxCpFS3eXb7U/p+GPwA\n-----END CERTIFICATE-----", + "odh-ca-bundle.crt": "-----BEGIN CERTIFICATE-----\nMIGrMF+gAwIBAgIBATAFBgMrZXAwADAeFw0yNDExMTMyMzI4NDJaFw0yNTExMTMy\nMzI4NDJaMAAwKjAFBgMrZXADIQAw01381TUVSxaCvjQckcw3RTcg+bsVMgNZU8eF\nXa/f3jAFBgMrZXADQQBeJZHSiMOYqa/tXUrQTfNIcklHuvieGyBRVSrX3bVUV2uM\nDBkZLsZt65rCk1A8NG+xkA6j3eIMAA9vBKJ0ht8F\n-----END CERTIFICATE-----", }) // Create the ConfigMap Expect(cli.Create(ctx, trustedCACertBundle)).Should(Succeed()) From 176a4287dd1288ed62f988a3e70bcd3883090a0b Mon Sep 17 00:00:00 2001 From: GitHub Actions Date: Thu, 14 Nov 2024 17:17:09 +0000 Subject: [PATCH 21/50] Update odh and notebook-controller with image main-363bcdb --- .../notebook-controller/config/overlays/openshift/params.env | 2 +- components/odh-notebook-controller/Makefile | 2 +- components/odh-notebook-controller/config/base/params.env | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/components/notebook-controller/config/overlays/openshift/params.env b/components/notebook-controller/config/overlays/openshift/params.env index 2d30cc53dba..752384aeaf7 100644 --- a/components/notebook-controller/config/overlays/openshift/params.env +++ b/components/notebook-controller/config/overlays/openshift/params.env @@ -1 +1 @@ -odh-kf-notebook-controller-image=quay.io/opendatahub/kubeflow-notebook-controller:main-3f931d2 +odh-kf-notebook-controller-image=quay.io/opendatahub/kubeflow-notebook-controller:main-363bcdb diff --git a/components/odh-notebook-controller/Makefile b/components/odh-notebook-controller/Makefile index 610189de3f8..e92d704bf3b 100644 --- a/components/odh-notebook-controller/Makefile +++ b/components/odh-notebook-controller/Makefile @@ -4,7 +4,7 @@ IMG ?= quay.io/opendatahub/odh-notebook-controller TAG ?= $(shell git describe --tags --always) KF_IMG ?= quay.io/opendatahub/kubeflow-notebook-controller -KF_TAG ?= main-3f931d2 +KF_TAG ?= main-363bcdb CONTAINER_ENGINE ?= podman diff --git a/components/odh-notebook-controller/config/base/params.env b/components/odh-notebook-controller/config/base/params.env index f062a8b7f34..664290753b9 100644 --- a/components/odh-notebook-controller/config/base/params.env +++ b/components/odh-notebook-controller/config/base/params.env @@ -1 +1 @@ -odh-notebook-controller-image=quay.io/opendatahub/odh-notebook-controller:main-3f931d2 +odh-notebook-controller-image=quay.io/opendatahub/odh-notebook-controller:main-363bcdb From 9f065c7ad8a767f0c1cc34df9caa3eeb451b64c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jiri=20Dan=C4=9Bk?= Date: Fri, 15 Nov 2024 09:26:31 +0100 Subject: [PATCH 22/50] RHOAIENG-15875: chore(gha): enable verbose govulncheck output (#456) Turns out that this "impacted but not vulnerable" prodsec thing is around to haunt us. This way we'll have full info from govulncheck so that we can better assess various reports. --- .github/workflows/code-quality.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/code-quality.yaml b/.github/workflows/code-quality.yaml index 342f506e11e..04d672c8934 100644 --- a/.github/workflows/code-quality.yaml +++ b/.github/workflows/code-quality.yaml @@ -87,5 +87,5 @@ jobs: run: | go install golang.org/x/vuln/cmd/govulncheck@latest go mod tidy - $(go env GOPATH)/bin/govulncheck ./... + $(go env GOPATH)/bin/govulncheck -show=verbose ./... working-directory: ${{ matrix.component }} From e7980b146e0d62779e019de95a6faabc45c439cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jiri=20Dan=C4=9Bk?= Date: Fri, 15 Nov 2024 10:43:31 +0100 Subject: [PATCH 23/50] RHOAIENG-15875: fix(deps): upgrade protobuf package to unaffected version (#457) --- components/notebook-controller/go.mod | 4 ++-- components/notebook-controller/go.sum | 12 ++++-------- components/odh-notebook-controller/go.mod | 4 ++-- components/odh-notebook-controller/go.sum | 12 ++++-------- 4 files changed, 12 insertions(+), 20 deletions(-) diff --git a/components/notebook-controller/go.mod b/components/notebook-controller/go.mod index 6bffd3cac06..ba566c17c53 100644 --- a/components/notebook-controller/go.mod +++ b/components/notebook-controller/go.mod @@ -30,7 +30,7 @@ require ( github.com/go-openapi/swag v0.22.3 // indirect github.com/gogo/protobuf v1.3.2 // indirect github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect - github.com/golang/protobuf v1.5.3 // indirect + github.com/golang/protobuf v1.5.4 // indirect github.com/google/gnostic-models v0.6.8 // indirect github.com/google/go-cmp v0.6.0 // indirect github.com/google/gofuzz v1.2.0 // indirect @@ -60,7 +60,7 @@ require ( golang.org/x/time v0.3.0 // indirect gomodules.xyz/jsonpatch/v2 v2.4.0 // indirect google.golang.org/appengine v1.6.7 // indirect - google.golang.org/protobuf v1.31.0 // indirect + google.golang.org/protobuf v1.33.0 // indirect gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect diff --git a/components/notebook-controller/go.sum b/components/notebook-controller/go.sum index 2e92ba70d73..d000791f4d6 100644 --- a/components/notebook-controller/go.sum +++ b/components/notebook-controller/go.sum @@ -42,15 +42,13 @@ github.com/golang/protobuf v1.4.0-rc.2/go.mod h1:LlEzMj4AhA7rCAGe4KMBDvJI+AwstrU github.com/golang/protobuf v1.4.0-rc.4.0.20200313231945-b860323f09d0/go.mod h1:WU3c8KckQ9AFe+yFwt9sWVRKCVIyN9cPHBJSNnbL67w= github.com/golang/protobuf v1.4.0/go.mod h1:jodUvKwWbYaEsadDk5Fwe5c77LiNKVO9IDvqG2KuDX0= github.com/golang/protobuf v1.4.2/go.mod h1:oDoupMAO8OvCJWAcko0GGGIgR6R6ocIYbsSw735rRwI= -github.com/golang/protobuf v1.5.0/go.mod h1:FsONVRAS9T7sI+LIUmWTfcYkHO4aIWwzhcaSAoJOfIk= -github.com/golang/protobuf v1.5.3 h1:KhyjKVUg7Usr/dYsdSqoFveMYd5ko72D+zANwlG1mmg= -github.com/golang/protobuf v1.5.3/go.mod h1:XVQd3VNwM+JqD3oG2Ue2ip4fOMUkwXdXDdiuN0vRsmY= +github.com/golang/protobuf v1.5.4 h1:i7eJL8qZTpSEXOPTxNKhASYpMn+8e5Q6AdndVa1dWek= +github.com/golang/protobuf v1.5.4/go.mod h1:lnTiLA8Wa4RWRcIUkrtSVa5nRhsEGBg48fD6rSs7xps= github.com/google/gnostic-models v0.6.8 h1:yo/ABAfM5IMRsS1VnXjTBvUb61tFIHozhlYvRgGre9I= github.com/google/gnostic-models v0.6.8/go.mod h1:5n7qKqH0f5wFt+aWF8CW6pZLLNOfYuF5OpfBSENuI8U= github.com/google/go-cmp v0.3.0/go.mod h1:8QqcDgzrUqlUb/G2PQTWiueGozuR1884gddMywk6iLU= github.com/google/go-cmp v0.3.1/go.mod h1:8QqcDgzrUqlUb/G2PQTWiueGozuR1884gddMywk6iLU= github.com/google/go-cmp v0.4.0/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= -github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= @@ -200,10 +198,8 @@ google.golang.org/protobuf v0.0.0-20200228230310-ab0ca4ff8a60/go.mod h1:cfTl7dwQ google.golang.org/protobuf v1.20.1-0.20200309200217-e05f789c0967/go.mod h1:A+miEFZTKqfCUM6K7xSMQL9OKL/b6hQv+e19PK+JZNE= google.golang.org/protobuf v1.21.0/go.mod h1:47Nbq4nVaFHyn7ilMalzfO3qCViNmqZ2kzikPIcrTAo= google.golang.org/protobuf v1.23.0/go.mod h1:EGpADcykh3NcUnDUJcl1+ZksZNG86OlYog2l/sGQquU= -google.golang.org/protobuf v1.26.0-rc.1/go.mod h1:jlhhOSvTdKEhbULTjvd4ARK9grFBp09yW+WbY/TyQbw= -google.golang.org/protobuf v1.26.0/go.mod h1:9q0QmTI4eRPtz6boOQmLYwt+qCgq0jsYwAQnmE0givc= -google.golang.org/protobuf v1.31.0 h1:g0LDEJHgrBl9N9r17Ru3sqWhkIx2NB67okBHPwC7hs8= -google.golang.org/protobuf v1.31.0/go.mod h1:HV8QOd/L58Z+nl8r43ehVNZIU/HEI6OcFqwMG9pJV4I= +google.golang.org/protobuf v1.33.0 h1:uNO2rsAINq/JlFpSdYEKIZ0uKD/R9cpdv0T+yoGwGmI= +google.golang.org/protobuf v1.33.0/go.mod h1:c6P6GXX6sHbq/GpV6MGZEdwhWPcYBgnhAHhKbcUYpos= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q= diff --git a/components/odh-notebook-controller/go.mod b/components/odh-notebook-controller/go.mod index 60a7fe2f2ad..aee387549a4 100644 --- a/components/odh-notebook-controller/go.mod +++ b/components/odh-notebook-controller/go.mod @@ -35,7 +35,7 @@ require ( github.com/go-openapi/swag v0.22.3 // indirect github.com/gogo/protobuf v1.3.2 // indirect github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect - github.com/golang/protobuf v1.5.3 // indirect + github.com/golang/protobuf v1.5.4 // indirect github.com/google/gnostic-models v0.6.8 // indirect github.com/google/gofuzz v1.2.0 // indirect github.com/google/uuid v1.3.0 // indirect @@ -64,7 +64,7 @@ require ( golang.org/x/time v0.3.0 // indirect gomodules.xyz/jsonpatch/v2 v2.4.0 // indirect google.golang.org/appengine v1.6.7 // indirect - google.golang.org/protobuf v1.31.0 // indirect + google.golang.org/protobuf v1.33.0 // indirect gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect diff --git a/components/odh-notebook-controller/go.sum b/components/odh-notebook-controller/go.sum index 73e73399756..5dd60504f7d 100644 --- a/components/odh-notebook-controller/go.sum +++ b/components/odh-notebook-controller/go.sum @@ -42,15 +42,13 @@ github.com/golang/protobuf v1.4.0-rc.2/go.mod h1:LlEzMj4AhA7rCAGe4KMBDvJI+AwstrU github.com/golang/protobuf v1.4.0-rc.4.0.20200313231945-b860323f09d0/go.mod h1:WU3c8KckQ9AFe+yFwt9sWVRKCVIyN9cPHBJSNnbL67w= github.com/golang/protobuf v1.4.0/go.mod h1:jodUvKwWbYaEsadDk5Fwe5c77LiNKVO9IDvqG2KuDX0= github.com/golang/protobuf v1.4.2/go.mod h1:oDoupMAO8OvCJWAcko0GGGIgR6R6ocIYbsSw735rRwI= -github.com/golang/protobuf v1.5.0/go.mod h1:FsONVRAS9T7sI+LIUmWTfcYkHO4aIWwzhcaSAoJOfIk= -github.com/golang/protobuf v1.5.3 h1:KhyjKVUg7Usr/dYsdSqoFveMYd5ko72D+zANwlG1mmg= -github.com/golang/protobuf v1.5.3/go.mod h1:XVQd3VNwM+JqD3oG2Ue2ip4fOMUkwXdXDdiuN0vRsmY= +github.com/golang/protobuf v1.5.4 h1:i7eJL8qZTpSEXOPTxNKhASYpMn+8e5Q6AdndVa1dWek= +github.com/golang/protobuf v1.5.4/go.mod h1:lnTiLA8Wa4RWRcIUkrtSVa5nRhsEGBg48fD6rSs7xps= github.com/google/gnostic-models v0.6.8 h1:yo/ABAfM5IMRsS1VnXjTBvUb61tFIHozhlYvRgGre9I= github.com/google/gnostic-models v0.6.8/go.mod h1:5n7qKqH0f5wFt+aWF8CW6pZLLNOfYuF5OpfBSENuI8U= github.com/google/go-cmp v0.3.0/go.mod h1:8QqcDgzrUqlUb/G2PQTWiueGozuR1884gddMywk6iLU= github.com/google/go-cmp v0.3.1/go.mod h1:8QqcDgzrUqlUb/G2PQTWiueGozuR1884gddMywk6iLU= github.com/google/go-cmp v0.4.0/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= -github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= @@ -202,10 +200,8 @@ google.golang.org/protobuf v0.0.0-20200228230310-ab0ca4ff8a60/go.mod h1:cfTl7dwQ google.golang.org/protobuf v1.20.1-0.20200309200217-e05f789c0967/go.mod h1:A+miEFZTKqfCUM6K7xSMQL9OKL/b6hQv+e19PK+JZNE= google.golang.org/protobuf v1.21.0/go.mod h1:47Nbq4nVaFHyn7ilMalzfO3qCViNmqZ2kzikPIcrTAo= google.golang.org/protobuf v1.23.0/go.mod h1:EGpADcykh3NcUnDUJcl1+ZksZNG86OlYog2l/sGQquU= -google.golang.org/protobuf v1.26.0-rc.1/go.mod h1:jlhhOSvTdKEhbULTjvd4ARK9grFBp09yW+WbY/TyQbw= -google.golang.org/protobuf v1.26.0/go.mod h1:9q0QmTI4eRPtz6boOQmLYwt+qCgq0jsYwAQnmE0givc= -google.golang.org/protobuf v1.31.0 h1:g0LDEJHgrBl9N9r17Ru3sqWhkIx2NB67okBHPwC7hs8= -google.golang.org/protobuf v1.31.0/go.mod h1:HV8QOd/L58Z+nl8r43ehVNZIU/HEI6OcFqwMG9pJV4I= +google.golang.org/protobuf v1.33.0 h1:uNO2rsAINq/JlFpSdYEKIZ0uKD/R9cpdv0T+yoGwGmI= +google.golang.org/protobuf v1.33.0/go.mod h1:c6P6GXX6sHbq/GpV6MGZEdwhWPcYBgnhAHhKbcUYpos= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q= From 3cc53abd298dccad96ba872603645b1a430ebb95 Mon Sep 17 00:00:00 2001 From: Jiri Danek Date: Thu, 14 Nov 2024 20:24:17 +0100 Subject: [PATCH 24/50] RHOAIENG-15743: fix(odh-nbc): trim ca-bundle certData to avoid logging scary messages --- .../odh-notebook-controller/controllers/notebook_controller.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/components/odh-notebook-controller/controllers/notebook_controller.go b/components/odh-notebook-controller/controllers/notebook_controller.go index 3e667ed06cb..daac2b87bce 100644 --- a/components/odh-notebook-controller/controllers/notebook_controller.go +++ b/components/odh-notebook-controller/controllers/notebook_controller.go @@ -283,6 +283,9 @@ func (r *OpenshiftNotebookReconciler) CreateNotebookCertConfigMap(notebook *nbv1 for _, certFile := range configMapFileNames[configMapName] { certData, ok := configMap.Data[certFile] + // RHOAIENG-15743: opendatahub-operator#1339 started adding '\n' unconditionally, which + // is breaking our `== ""` checks below. Trim the certData again. + certData = strings.TrimSpace(certData) // If ca-bundle.crt is not found in the ConfigMap odh-trusted-ca-bundle // no need to create the workbench-trusted-ca-bundle, as it is created // by annotation inject-ca-bundle: "true" From 57ce0179c30fb64fe9c4d5ebd5699705c690b70d Mon Sep 17 00:00:00 2001 From: atheo89 Date: Mon, 18 Nov 2024 10:36:40 +0100 Subject: [PATCH 25/50] Update gha to alling with makefile-vars.mk external file --- .../workflows/notebook-controller-images-updater.yaml | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/.github/workflows/notebook-controller-images-updater.yaml b/.github/workflows/notebook-controller-images-updater.yaml index 3811956a764..99b020e8fd0 100644 --- a/.github/workflows/notebook-controller-images-updater.yaml +++ b/.github/workflows/notebook-controller-images-updater.yaml @@ -73,7 +73,7 @@ jobs: echo "Updating files in VERSION=${VERSION} with COMMIT_ID=${COMMIT_ID}" sed -E "s/(odh-kf-notebook-controller-image=quay\.io\/opendatahub\/kubeflow-notebook-controller:)[^: -]+(-)[^ ]+/\1$VERSION\2$COMMIT_ID/" -i components/notebook-controller/config/overlays/openshift/params.env sed -E "s/(odh-notebook-controller-image=quay\.io\/opendatahub\/odh-notebook-controller:)[^: -]+(-)[^ ]+/\1$VERSION\2$COMMIT_ID/" -i components/odh-notebook-controller/config/base/params.env - sed -E "s/(KF_TAG \?= )[^\-]+(-)[^ ]+/\1$VERSION\2$COMMIT_ID/" -i components/odh-notebook-controller/Makefile + sed -E "s/(KF_TAG \?= )[^\-]+(-)[^ ]+/\1$VERSION\2$COMMIT_ID/" -i components/odh-notebook-controller/makefile-vars.mk git status if [[ $(git status --porcelain | wc -l) -gt 0 ]]; then @@ -82,8 +82,8 @@ jobs: git pull origin ${{ env.TEMP_UPDATER_BRANCH }} git add components/notebook-controller/config/overlays/openshift/params.env git add components/odh-notebook-controller/config/base/params.env - git add components/odh-notebook-controller/Makefile - git commit -m "Update odh and notebook-controller with image ${VERSION}-${COMMIT_ID}" + git add components/odh-notebook-controller/makefile-vars.mk + git commit -m ":robot: Update odh and notebook-controller with image ${VERSION}-${COMMIT_ID}" git push origin ${{ env.TEMP_UPDATER_BRANCH }} git log --oneline else @@ -106,4 +106,5 @@ jobs: Have been updated the following related files: - components/notebook-controller/config/overlays/openshift/params.env - components/odh-notebook-controller/config/base/params.env - - components/odh-notebook-controller/Makefile + - components/odh-notebook-controller/makefile-vars.mk + From f2fa34e6afa343aaa18f28a06234488365515b01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jiri=20Dan=C4=9Bk?= Date: Tue, 19 Nov 2024 11:22:34 +0100 Subject: [PATCH 26/50] RHOAIENG-15772: tests(nbcs): use the direct (uncached) client to drive tests (#461) This is in accordance with the recommendations in the Kubebuilder Book. https://book.kubebuilder.io/cronjob-tutorial/writing-tests --- components/notebook-controller/controllers/suite_test.go | 1 - components/odh-notebook-controller/controllers/suite_test.go | 5 ++--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/components/notebook-controller/controllers/suite_test.go b/components/notebook-controller/controllers/suite_test.go index 3093e1b5e97..4783f027a47 100644 --- a/components/notebook-controller/controllers/suite_test.go +++ b/components/notebook-controller/controllers/suite_test.go @@ -91,7 +91,6 @@ var _ = BeforeSuite(func() { Expect(err).ToNot(HaveOccurred(), "failed to run manager") }() - k8sClient = k8sManager.GetClient() Expect(k8sClient).ToNot(BeNil()) }, 60) diff --git a/components/odh-notebook-controller/controllers/suite_test.go b/components/odh-notebook-controller/controllers/suite_test.go index 16efc780537..b4bb91eca2d 100644 --- a/components/odh-notebook-controller/controllers/suite_test.go +++ b/components/odh-notebook-controller/controllers/suite_test.go @@ -83,7 +83,7 @@ var _ = BeforeSuite(func() { } logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseFlagOptions(&opts))) - // Initiliaze test environment: + // Initialize test environment: // https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/envtest#Environment.Start By("Bootstrapping test environment") envTest = &envtest.Environment{ @@ -110,7 +110,7 @@ var _ = BeforeSuite(func() { utilruntime.Must(netv1.AddToScheme(scheme)) // +kubebuilder:scaffold:scheme - // Initiliaze Kubernetes client + // Initialize Kubernetes client cli, err = client.New(cfg, client.Options{Scheme: scheme}) Expect(err).NotTo(HaveOccurred()) Expect(cli).NotTo(BeNil()) @@ -172,7 +172,6 @@ var _ = BeforeSuite(func() { }).Should(Succeed()) // Verify kubernetes client is working - cli = mgr.GetClient() Expect(cli).ToNot(BeNil()) for _, namespace := range testNamespaces { From b5741e1e57cf9033fd32b5c86b1e8a339db0236d Mon Sep 17 00:00:00 2001 From: aTheo Date: Tue, 19 Nov 2024 12:28:33 +0100 Subject: [PATCH 27/50] Update sync workflow to handle specific conflicts (#459) * Update sync workflow to hadle specific conflicts * Externalize KF_TAG variable to dedicated file outside of the makefile * Update makefile logic handling --- .../workflows/sync-branches-through-pr.yaml | 103 ++++++++++++------ components/odh-notebook-controller/Makefile | 4 +- .../odh-notebook-controller/makefile-vars.mk | 1 + 3 files changed, 72 insertions(+), 36 deletions(-) create mode 100644 components/odh-notebook-controller/makefile-vars.mk diff --git a/.github/workflows/sync-branches-through-pr.yaml b/.github/workflows/sync-branches-through-pr.yaml index f412be13f7f..6923981dc88 100644 --- a/.github/workflows/sync-branches-through-pr.yaml +++ b/.github/workflows/sync-branches-through-pr.yaml @@ -1,47 +1,80 @@ --- -name: Sync branches through Pull Request +name: Sync Branches -on: # yamllint disable-line rule:truthy +on: workflow_dispatch: inputs: source: - description: Source branch + description: "From:" required: true + type: string target: - description: Target branch + description: "To:" required: true + type: string jobs: - sync: - permissions: - contents: write - pull-requests: write + sync-branches: runs-on: ubuntu-latest + steps: - - name: Checkout repository - uses: actions/checkout@v4 - with: - ref: ${{ github.event.inputs.target }} - fetch-depth: 0 - - - name: Prepare sync branch - id: prepare - run: | - git fetch origin ${{ github.event.inputs.source }} - git reset --hard origin/${{ github.event.inputs.source }} - - TIMESTAMP=$(date +'%Y%m%d%H%M%S') - SYNC_BRANCH=sync__${{ github.event.inputs.source }}__${{ github.event.inputs.target }}__${TIMESTAMP} - echo "branch=$SYNC_BRANCH" >> $GITHUB_OUTPUT - - - name: Create pull request - uses: peter-evans/create-pull-request@5e914681df9dc83aa4e4905692ca88beb2f9e91f # v7.0.5 - with: - branch: ${{ steps.prepare.outputs.branch }} - title: "Sync `${{ github.event.inputs.target }}` branch with `${{ github.event.inputs.source }}` branch" - body: | - :robot: This is an automated Pull Request created by `/.github/workflows/sync-branches-through-pr.yml`. - - It merges all commits from `${{ github.event.inputs.source }}` branch into `${{ github.event.inputs.target }}` branch. - - :warning: **IMPORTANT NOTE**: Remember to delete the `${{ steps.prepare.outputs.branch }}` branch after merging the changes. + - name: Checkout repository + uses: actions/checkout@v4 + with: + fetch-depth: 0 # Ensure full history is fetched for merging + + - name: Set up Git + run: | + git config user.name "github-actions[bot]" + git config user.email "github-actions[bot]@users.noreply.github.com" + + - name: Merge source branch into target with conflict resolution + id: merge + run: | + set -e + SOURCE_BRANCH="${{ github.event.inputs.source }}" + TARGET_BRANCH="${{ github.event.inputs.target }}" + + git fetch origin ${TARGET_BRANCH}:${TARGET_BRANCH} + git checkout ${TARGET_BRANCH} + + git fetch origin ${SOURCE_BRANCH}:${SOURCE_BRANCH} + git merge --no-commit origin/${SOURCE_BRANCH} || true + + # Resolve conflicts for specific files + FILES=( + "components/notebook-controller/config/overlays/openshift/params.env" + "components/odh-notebook-controller/config/base/params.env" + "components/odh-notebook-controller/makefile-vars.mk" + ) + + for FILE in "${FILES[@]}"; do + if [[ -f "$FILE" && "$(git status --porcelain=v1 2>/dev/null | grep -c "$FILE")" -gt 0 ]]; then + echo "Resolving conflict for $FILE by keeping target branch version." + git checkout --ours "$FILE" + git add "$FILE" + fi + done + + # Commit the merge changes + git commit -m "Merge ${SOURCE_BRANCH} into ${TARGET_BRANCH} with resolved conflicts" || echo "Nothing to commit" + + # Create a new branch for the sync + TIMESTAMP=$(date +'%Y%m%d%H%M%S') + SYNC_BRANCH="sync__${SOURCE_BRANCH}__${TARGET_BRANCH}__${TIMESTAMP}" + git checkout -b $SYNC_BRANCH + git push origin $SYNC_BRANCH + echo "branch=$SYNC_BRANCH" >> $GITHUB_OUTPUT + + - name: Create a Pull Request + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + SOURCE_BRANCH="${{ github.event.inputs.source }}" + TARGET_BRANCH="${{ github.event.inputs.target }}" + SYNC_BRANCH=$(echo "${{ steps.merge.outputs.branch }}") + gh pr create \ + --title "Sync ${SOURCE_BRANCH} into ${TARGET_BRANCH}" \ + --body ":robot: This is an automated PR" \ + --base "${{ github.event.inputs.target }}" \ + --head "${SYNC_BRANCH}" diff --git a/components/odh-notebook-controller/Makefile b/components/odh-notebook-controller/Makefile index e92d704bf3b..31ef73c671f 100644 --- a/components/odh-notebook-controller/Makefile +++ b/components/odh-notebook-controller/Makefile @@ -1,10 +1,12 @@ +include makefile-vars.mk + # Image URL to use all building/pushing image targets IMG ?= quay.io/opendatahub/odh-notebook-controller TAG ?= $(shell git describe --tags --always) KF_IMG ?= quay.io/opendatahub/kubeflow-notebook-controller -KF_TAG ?= main-363bcdb +KF_TAG ?= $(KF_TAG) CONTAINER_ENGINE ?= podman diff --git a/components/odh-notebook-controller/makefile-vars.mk b/components/odh-notebook-controller/makefile-vars.mk new file mode 100644 index 00000000000..827d0ea7820 --- /dev/null +++ b/components/odh-notebook-controller/makefile-vars.mk @@ -0,0 +1 @@ +KF_TAG ?= main-363bcdb From 658c81d8103aed2624003b7d0f0179cb1fabf9dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jiri=20Dan=C4=9Bk?= Date: Tue, 19 Nov 2024 13:48:16 +0100 Subject: [PATCH 28/50] RHOAIENG-15772: tests(odh-nbc): fix unintended shadowing of `cfg` variable (#462) This is a nasty Go gotcha, and we aren't the first to have trouble with it https://temporal.io/blog/go-shadowing-bad-choices#the-bug --- components/notebook-controller/controllers/suite_test.go | 3 ++- components/odh-notebook-controller/controllers/suite_test.go | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/components/notebook-controller/controllers/suite_test.go b/components/notebook-controller/controllers/suite_test.go index 4783f027a47..8f5a1f8a9a2 100644 --- a/components/notebook-controller/controllers/suite_test.go +++ b/components/notebook-controller/controllers/suite_test.go @@ -58,7 +58,8 @@ var _ = BeforeSuite(func() { ErrorIfCRDPathMissing: true, } - cfg, err := testEnv.Start() + var err error + cfg, err = testEnv.Start() Expect(err).NotTo(HaveOccurred()) Expect(cfg).NotTo(BeNil()) diff --git a/components/odh-notebook-controller/controllers/suite_test.go b/components/odh-notebook-controller/controllers/suite_test.go index b4bb91eca2d..f11db338dd3 100644 --- a/components/odh-notebook-controller/controllers/suite_test.go +++ b/components/odh-notebook-controller/controllers/suite_test.go @@ -98,7 +98,8 @@ var _ = BeforeSuite(func() { }, } - cfg, err := envTest.Start() + var err error + cfg, err = envTest.Start() Expect(err).NotTo(HaveOccurred()) Expect(cfg).NotTo(BeNil()) From 9e32dfd8e28430a215d9f27554ba2ea63c3768c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jiri=20Dan=C4=9Bk?= Date: Tue, 19 Nov 2024 14:59:46 +0100 Subject: [PATCH 29/50] RHOAIENG-15772: tests(odh-nbc): disable graceful shutdown period for envtest (#464) * RHOAIENG-15772: tests(odh-nbc): disable graceful shutdown period for envtest There is no benefit to doing graceful shutdown. We don't assert anything about the shutdown process, so it is best to be done with it quickly, so that we don't waste time and also avoid some meaningless log messages the shutdown may print out. * fixup also pass in test context --- .../odh-notebook-controller/controllers/suite_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/components/odh-notebook-controller/controllers/suite_test.go b/components/odh-notebook-controller/controllers/suite_test.go index f11db338dd3..8df6546d714 100644 --- a/components/odh-notebook-controller/controllers/suite_test.go +++ b/components/odh-notebook-controller/controllers/suite_test.go @@ -18,6 +18,7 @@ import ( "context" "crypto/tls" "fmt" + "k8s.io/utils/ptr" "net" "path/filepath" "testing" @@ -127,6 +128,12 @@ var _ = BeforeSuite(func() { Port: webhookInstallOptions.LocalServingPort, CertDir: webhookInstallOptions.LocalServingCertDir, }), + // Issue#429: waiting in tests only wastes time and prints pointless context-cancelled errors + GracefulShutdownTimeout: ptr.To(time.Duration(0)), + // pass in test context because why not + BaseContext: func() context.Context { + return ctx + }, }) Expect(err).NotTo(HaveOccurred()) From adc6db1dd99827b854c5481f311d71b715862823 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jiri=20Dan=C4=9Bk?= Date: Tue, 19 Nov 2024 17:28:19 +0100 Subject: [PATCH 30/50] RHOAIENG-15772: tests(odh-nbc): wait for controller manager to stop before stopping envtest (#463) Otherwise, there will be some errors printed in the output. It was not breaking anything, but it was unpleasant to look at. The notebook-controller does not seem to be suffering from this. Probably because it does not use a webhook. Example of the error message that's prevented by this ``` 2024-11-17T20:19:57+01:00 ERROR controller-runtime.certwatcher error re-reading certificate {"error": "open /var/folders/f1/3m518k5d34l72v_9nqyjzqm80000gn/T/envtest-serving-certs-737480808/tls.crt: no such file or directory"} ``` --- .../controllers/suite_test.go | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/components/odh-notebook-controller/controllers/suite_test.go b/components/odh-notebook-controller/controllers/suite_test.go index 8df6546d714..9c166bd7b4f 100644 --- a/components/odh-notebook-controller/controllers/suite_test.go +++ b/components/odh-notebook-controller/controllers/suite_test.go @@ -56,11 +56,14 @@ import ( // +kubebuilder:docs-gen:collapse=Imports var ( - cfg *rest.Config - cli client.Client - envTest *envtest.Environment + cfg *rest.Config + cli client.Client + envTest *envtest.Environment + ctx context.Context cancel context.CancelFunc + managerStopped = make(chan struct{}) + testNamespaces = []string{} ) @@ -75,7 +78,7 @@ func TestAPIs(t *testing.T) { } var _ = BeforeSuite(func() { - ctx, cancel = context.WithCancel(context.TODO()) + ctx, cancel = context.WithCancel(context.Background()) // Initialize logger opts := zap.Options{ @@ -164,6 +167,7 @@ var _ = BeforeSuite(func() { go func() { defer GinkgoRecover() err = mgr.Start(ctx) + managerStopped <- struct{}{} Expect(err).ToNot(HaveOccurred(), "Failed to run manager") }() @@ -194,7 +198,10 @@ var _ = BeforeSuite(func() { }, 60) var _ = AfterSuite(func() { + By("Stopping the manager") cancel() + <-managerStopped // Issue#429: waiting to avoid shutdown errors being logged + By("Tearing down the test environment") // TODO: Stop cert controller-runtime.certwatcher before manager err := envTest.Stop() From ed444be153ff0c679768d249f7c90161f1689328 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jiri=20Dan=C4=9Bk?= Date: Tue, 19 Nov 2024 18:48:06 +0100 Subject: [PATCH 31/50] RHOAIENG-15772: tests(odh-nbc): write envtest kubeconfig to disk upon request (#450) * RHOAIENG-15772: tests(odh-nbc): write envtest kubeconfig to disk upon request This helps with debugging because it's then possible to do e.g. ``` KUBECONFIG=/tmp/envtest.kubeconfig k9s ``` and investigate the cluster while the test is paused on a breakpoint. * fixup docs and logging * fixup move user creation as early as possible, and explain test verbosity * Update components/odh-notebook-controller/README.md Co-authored-by: Jan Stourac --------- Co-authored-by: Jan Stourac --- components/odh-notebook-controller/README.md | 15 +++++++++++++++ .../controllers/suite_test.go | 14 ++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/components/odh-notebook-controller/README.md b/components/odh-notebook-controller/README.md index d170be23cd2..4d75d803725 100644 --- a/components/odh-notebook-controller/README.md +++ b/components/odh-notebook-controller/README.md @@ -78,6 +78,21 @@ Run the following command to execute them: make test ``` +Useful options for running tests (that are already included in the above `make` command) are + +| Option | Description | +|----------------------------|-----------------------------------------------------------------------------------------------| +| KUBEBUILDER_ASSETS | Environment variable that specifies path to where Kubebuilder has downloaded envtest binaries | +| -ginkgo.v -ginkgo.progress | Enables verbose output from Ginkgo | +| -test.v | Enables verbose output from Go tests (*testing.T) | + +The following environment variables are used to enable additional debug options for the tests + +| Environment variable | Description | +|------------------------|----------------------------------------------------------------------------------------------------------------------------------------------| +| DEBUG_WRITE_KUBECONFIG | Writes a Kubeconfig file to disk. It can be used with `kubectl` or `k9s` to examine the envtest cluster when test is paused on a breakpoint. | +| | | + ### Run locally Install the `notebooks.kubeflow.org` CRD from the [Kubeflow notebook diff --git a/components/odh-notebook-controller/controllers/suite_test.go b/components/odh-notebook-controller/controllers/suite_test.go index 9c166bd7b4f..40b30c6a1aa 100644 --- a/components/odh-notebook-controller/controllers/suite_test.go +++ b/components/odh-notebook-controller/controllers/suite_test.go @@ -20,6 +20,7 @@ import ( "fmt" "k8s.io/utils/ptr" "net" + "os" "path/filepath" "testing" "time" @@ -107,6 +108,19 @@ var _ = BeforeSuite(func() { Expect(err).NotTo(HaveOccurred()) Expect(cfg).NotTo(BeNil()) + if kubeconfigPath, found := os.LookupEnv("DEBUG_WRITE_KUBECONFIG"); found { + user := envtest.User{Name: "MasterOfTheSystems", Groups: []string{"system:masters"}} + authedUser, err := envTest.ControlPlane.AddUser(user, nil) + Expect(err).NotTo(HaveOccurred()) + config, err := authedUser.KubeConfig() + Expect(err).NotTo(HaveOccurred()) + err = os.WriteFile(kubeconfigPath, config, 0600) + Expect(err).NotTo(HaveOccurred()) + GinkgoT().Logf("DEBUG_WRITE_KUBECONFIG is set, writing system:masters' Kubeconfig to %s", kubeconfigPath) + } else { + GinkgoT().Logf("DEBUG_WRITE_KUBECONFIG environment variable was not provided") + } + // Register API objects scheme := runtime.NewScheme() utilruntime.Must(clientgoscheme.AddToScheme(scheme)) From b4646e7d84beaa11f7ca062d546f03535469c03b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jiri=20Dan=C4=9Bk?= Date: Wed, 20 Nov 2024 11:02:34 +0100 Subject: [PATCH 32/50] RHOAIENG-15772: tests(odh-nbc): write auditlogs from envtest tests to disk upon request (#451) * RHOAIENG-15772: tests(odh-nbc): write auditlogs from envtest tests to disk upon request This helps with debugging because it's then possible to do e.g. ``` cat /tmp/audit.log | jq | ... ``` and investigate what happened when a test was running. * fixup logging and order and few comments * fixup readme and tweaks --- components/odh-notebook-controller/README.md | 2 +- .../controllers/suite_test.go | 17 +++++++++++++++++ .../envtest-audit-policy.yaml | 16 ++++++++++++++++ 3 files changed, 34 insertions(+), 1 deletion(-) create mode 100644 components/odh-notebook-controller/envtest-audit-policy.yaml diff --git a/components/odh-notebook-controller/README.md b/components/odh-notebook-controller/README.md index 4d75d803725..5320cbe7e0f 100644 --- a/components/odh-notebook-controller/README.md +++ b/components/odh-notebook-controller/README.md @@ -91,7 +91,7 @@ The following environment variables are used to enable additional debug options | Environment variable | Description | |------------------------|----------------------------------------------------------------------------------------------------------------------------------------------| | DEBUG_WRITE_KUBECONFIG | Writes a Kubeconfig file to disk. It can be used with `kubectl` or `k9s` to examine the envtest cluster when test is paused on a breakpoint. | -| | | +| DEBUG_WRITE_AUDITLOG | Writes kube-apiserver auditlogs to disk. The config is in `envtest-audit-policy.yaml`, set the namespace of interest there. | ### Run locally diff --git a/components/odh-notebook-controller/controllers/suite_test.go b/components/odh-notebook-controller/controllers/suite_test.go index 40b30c6a1aa..a9c93337db4 100644 --- a/components/odh-notebook-controller/controllers/suite_test.go +++ b/components/odh-notebook-controller/controllers/suite_test.go @@ -92,6 +92,9 @@ var _ = BeforeSuite(func() { // https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/envtest#Environment.Start By("Bootstrapping test environment") envTest = &envtest.Environment{ + ControlPlane: envtest.ControlPlane{ + APIServer: &envtest.APIServer{}, + }, CRDInstallOptions: envtest.CRDInstallOptions{ Paths: []string{filepath.Join("..", "config", "crd", "external")}, ErrorIfPathMissing: true, @@ -102,6 +105,19 @@ var _ = BeforeSuite(func() { IgnoreErrorIfPathMissing: false, }, } + if auditLogPath, found := os.LookupEnv("DEBUG_WRITE_AUDITLOG"); found { + envTest.ControlPlane.APIServer.Configure(). + // https://kubernetes.io/docs/tasks/debug/debug-cluster/audit/#log-backend + Append("audit-log-maxage", "1"). + Append("audit-log-maxbackup", "5"). + Append("audit-log-maxsize", "100"). // in MiB + Append("audit-log-format", "json"). + Append("audit-policy-file", filepath.Join("..", "envtest-audit-policy.yaml")). + Append("audit-log-path", auditLogPath) + GinkgoT().Logf("DEBUG_WRITE_AUDITLOG is set, writing `envtest-audit-policy.yaml` auditlog to %s", auditLogPath) + } else { + GinkgoT().Logf("DEBUG_WRITE_AUDITLOG environment variable was not provided") + } var err error cfg, err = envTest.Start() @@ -109,6 +125,7 @@ var _ = BeforeSuite(func() { Expect(cfg).NotTo(BeNil()) if kubeconfigPath, found := os.LookupEnv("DEBUG_WRITE_KUBECONFIG"); found { + // https://github.com/rancher/fleet/blob/main/integrationtests/utils/kubeconfig.go user := envtest.User{Name: "MasterOfTheSystems", Groups: []string{"system:masters"}} authedUser, err := envTest.ControlPlane.AddUser(user, nil) Expect(err).NotTo(HaveOccurred()) diff --git a/components/odh-notebook-controller/envtest-audit-policy.yaml b/components/odh-notebook-controller/envtest-audit-policy.yaml new file mode 100644 index 00000000000..70b8551ddf4 --- /dev/null +++ b/components/odh-notebook-controller/envtest-audit-policy.yaml @@ -0,0 +1,16 @@ +# https://kubernetes.io/docs/tasks/debug/debug-cluster/audit/#audit-policy +# This is extremely verbose kube-apiserver logging that may be enabled for debugging of envtest-based tests +--- +apiVersion: audit.k8s.io/v1 +kind: Policy +rules: + # Log all requests in `developer` namespace at the RequestResponse (maximum verbosity) level. + - level: RequestResponse + namespaces: ["developer"] + +# Use jq to analyze the log file this produces. For example: + +# jq 'select((.objectRef.apiGroup == "dscinitialization.opendatahub.io" +# or .objectRef.apiGroup == "datasciencecluster.opendatahub.io") +# and .user.username != "system:serviceaccount:redhat-ods-operator:redhat-ods-operator-controller-manager" +# and .verb != "get" and .verb != "watch" and .verb != "list")' < /tmp/kube-apiserver-audit.log From 30afc1574e05bb05b21565db64e3b0ba5d6b9060 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jiri=20Dan=C4=9Bk?= Date: Thu, 21 Nov 2024 17:40:38 +0100 Subject: [PATCH 33/50] RHOAIENG-15772: tests(odh-nbc): output struct diffs in gomega failure messages (#454) Now it will be possible to see why the check failed, instead of getting the meaningless ``` Expected : false to be true ``` --- .../controllers/notebook_controller_test.go | 48 ++++++++++--------- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/components/odh-notebook-controller/controllers/notebook_controller_test.go b/components/odh-notebook-controller/controllers/notebook_controller_test.go index 817ed7aa614..41e35ee800a 100644 --- a/components/odh-notebook-controller/controllers/notebook_controller_test.go +++ b/components/odh-notebook-controller/controllers/notebook_controller_test.go @@ -26,15 +26,15 @@ import ( "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/google/go-cmp/cmp" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + routev1 "github.com/openshift/api/route/v1" corev1 "k8s.io/api/core/v1" + netv1 "k8s.io/api/networking/v1" + rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" @@ -102,7 +102,7 @@ var _ = Describe("The Openshift Notebook controller", func() { key := types.NamespacedName{Name: Name, Namespace: Namespace} return cli.Get(ctx, key, route) }, duration, interval).Should(Succeed()) - Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrue()) + Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrueBecause(cmp.Diff(*route, expectedRoute))) }) It("Should reconcile the Route when modified", func() { @@ -120,7 +120,7 @@ var _ = Describe("The Openshift Notebook controller", func() { } return route.Spec.To.Name, nil }, duration, interval).Should(Equal(Name)) - Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrue()) + Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrueBecause(cmp.Diff(*route, expectedRoute))) }) It("Should recreate the Route when deleted", func() { @@ -133,7 +133,7 @@ var _ = Describe("The Openshift Notebook controller", func() { key := types.NamespacedName{Name: Name, Namespace: Namespace} return cli.Get(ctx, key, route) }, duration, interval).Should(Succeed()) - Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrue()) + Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrueBecause(cmp.Diff(*route, expectedRoute))) }) It("Should delete the Openshift Route", func() { @@ -497,7 +497,7 @@ var _ = Describe("The Openshift Notebook controller", func() { key := types.NamespacedName{Name: Name + "-ctrl-np", Namespace: Namespace} return cli.Get(ctx, key, notebookNetworkPolicy) }, duration, interval).Should(Succeed()) - Expect(CompareNotebookNetworkPolicies(*notebookNetworkPolicy, expectedNotebookNetworkPolicy)).Should(BeTrue()) + Expect(CompareNotebookNetworkPolicies(*notebookNetworkPolicy, expectedNotebookNetworkPolicy)).Should(BeTrueBecause(cmp.Diff(*notebookNetworkPolicy, expectedNotebookNetworkPolicy))) By("By checking that the controller has created Network policy to allow all requests on OAuth port") Eventually(func() error { @@ -505,7 +505,7 @@ var _ = Describe("The Openshift Notebook controller", func() { return cli.Get(ctx, key, notebookOAuthNetworkPolicy) }, duration, interval).Should(Succeed()) Expect(CompareNotebookNetworkPolicies(*notebookOAuthNetworkPolicy, expectedNotebookOAuthNetworkPolicy)). - To(BeTrue(), "Expected :%v\n, Got: %v", format.Object(expectedNotebookOAuthNetworkPolicy, 1), format.Object(notebookOAuthNetworkPolicy, 1)) + To(BeTrueBecause(cmp.Diff(*notebookOAuthNetworkPolicy, expectedNotebookOAuthNetworkPolicy))) }) It("Should reconcile the Network policies when modified", func() { @@ -523,7 +523,8 @@ var _ = Describe("The Openshift Notebook controller", func() { } return string(notebookNetworkPolicy.Spec.PolicyTypes[0]), nil }, duration, interval).Should(Equal("Ingress")) - Expect(CompareNotebookNetworkPolicies(*notebookNetworkPolicy, expectedNotebookNetworkPolicy)).Should(BeTrue()) + Expect(CompareNotebookNetworkPolicies(*notebookNetworkPolicy, expectedNotebookNetworkPolicy)).Should( + BeTrueBecause(cmp.Diff(*notebookNetworkPolicy, expectedNotebookNetworkPolicy))) }) It("Should recreate the Network Policy when deleted", func() { @@ -536,7 +537,8 @@ var _ = Describe("The Openshift Notebook controller", func() { key := types.NamespacedName{Name: Name + "-oauth-np", Namespace: Namespace} return cli.Get(ctx, key, notebookOAuthNetworkPolicy) }, duration, interval).Should(Succeed()) - Expect(CompareNotebookNetworkPolicies(*notebookOAuthNetworkPolicy, expectedNotebookOAuthNetworkPolicy)).Should(BeTrue()) + Expect(CompareNotebookNetworkPolicies(*notebookOAuthNetworkPolicy, expectedNotebookOAuthNetworkPolicy)).Should( + BeTrueBecause(cmp.Diff(*notebookOAuthNetworkPolicy, expectedNotebookOAuthNetworkPolicy))) }) It("Should delete the Network Policies", func() { @@ -669,7 +671,7 @@ var _ = Describe("The Openshift Notebook controller", func() { time.Sleep(interval) By("By checking that the webhook has injected the sidecar container") - Expect(CompareNotebooks(*notebook, expectedNotebook)).Should(BeTrue()) + Expect(CompareNotebooks(*notebook, expectedNotebook)).Should(BeTrueBecause(cmp.Diff(*notebook, expectedNotebook))) }) It("Should remove the reconciliation lock annotation", func() { @@ -682,7 +684,7 @@ var _ = Describe("The Openshift Notebook controller", func() { return false } return CompareNotebooks(*notebook, expectedNotebook) - }, duration, interval).Should(BeTrue()) + }, duration, interval).Should(BeTrueBecause(cmp.Diff(*notebook, expectedNotebook))) }) It("Should reconcile the Notebook when modified", func() { @@ -698,7 +700,7 @@ var _ = Describe("The Openshift Notebook controller", func() { key := types.NamespacedName{Name: Name, Namespace: Namespace} return cli.Get(ctx, key, notebook) }, duration, interval).Should(Succeed()) - Expect(CompareNotebooks(*notebook, expectedNotebook)).Should(BeTrue()) + Expect(CompareNotebooks(*notebook, expectedNotebook)).Should(BeTrueBecause(cmp.Diff(*notebook, expectedNotebook))) }) serviceAccount := &corev1.ServiceAccount{} @@ -710,7 +712,8 @@ var _ = Describe("The Openshift Notebook controller", func() { key := types.NamespacedName{Name: Name, Namespace: Namespace} return cli.Get(ctx, key, serviceAccount) }, duration, interval).Should(Succeed()) - Expect(CompareNotebookServiceAccounts(*serviceAccount, expectedServiceAccount)).Should(BeTrue()) + Expect(CompareNotebookServiceAccounts(*serviceAccount, expectedServiceAccount)).Should( + BeTrueBecause(cmp.Diff(*serviceAccount, expectedServiceAccount))) }) It("Should recreate the Service Account when deleted", func() { @@ -723,7 +726,8 @@ var _ = Describe("The Openshift Notebook controller", func() { key := types.NamespacedName{Name: Name, Namespace: Namespace} return cli.Get(ctx, key, serviceAccount) }, duration, interval).Should(Succeed()) - Expect(CompareNotebookServiceAccounts(*serviceAccount, expectedServiceAccount)).Should(BeTrue()) + Expect(CompareNotebookServiceAccounts(*serviceAccount, expectedServiceAccount)).Should( + BeTrueBecause(cmp.Diff(*serviceAccount, expectedServiceAccount))) }) service := &corev1.Service{} @@ -754,7 +758,7 @@ var _ = Describe("The Openshift Notebook controller", func() { key := types.NamespacedName{Name: Name + "-tls", Namespace: Namespace} return cli.Get(ctx, key, service) }, duration, interval).Should(Succeed()) - Expect(CompareNotebookServices(*service, expectedService)).Should(BeTrue()) + Expect(CompareNotebookServices(*service, expectedService)).Should(BeTrueBecause(cmp.Diff(*service, expectedService))) }) It("Should recreate the Service when deleted", func() { @@ -767,7 +771,7 @@ var _ = Describe("The Openshift Notebook controller", func() { key := types.NamespacedName{Name: Name + "-tls", Namespace: Namespace} return cli.Get(ctx, key, service) }, duration, interval).Should(Succeed()) - Expect(CompareNotebookServices(*service, expectedService)).Should(BeTrue()) + Expect(CompareNotebookServices(*service, expectedService)).Should(BeTrueBecause(cmp.Diff(*service, expectedService))) }) secret := &corev1.Secret{} @@ -830,7 +834,7 @@ var _ = Describe("The Openshift Notebook controller", func() { key := types.NamespacedName{Name: Name, Namespace: Namespace} return cli.Get(ctx, key, route) }, duration, interval).Should(Succeed()) - Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrue()) + Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrueBecause(cmp.Diff(*route, expectedRoute))) }) It("Should recreate the Route when deleted", func() { @@ -843,7 +847,7 @@ var _ = Describe("The Openshift Notebook controller", func() { key := types.NamespacedName{Name: Name, Namespace: Namespace} return cli.Get(ctx, key, route) }, duration, interval).Should(Succeed()) - Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrue()) + Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrueBecause(cmp.Diff(*route, expectedRoute))) }) It("Should reconcile the Route when modified", func() { @@ -861,7 +865,7 @@ var _ = Describe("The Openshift Notebook controller", func() { } return route.Spec.To.Name, nil }, duration, interval).Should(Equal(Name + "-tls")) - Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrue()) + Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrueBecause(cmp.Diff(*route, expectedRoute))) }) It("Should delete the OAuth proxy objects", func() { From 9294927291754b6bcd91efcf70173a2583278a3e Mon Sep 17 00:00:00 2001 From: Jan Stourac Date: Mon, 25 Nov 2024 14:46:25 +0100 Subject: [PATCH 34/50] [Fix] update bash scripts for E2E tests run to propagate failures (#433) * [Fix] update bash scripts for E2E tests run to propagate failures We haven't been propagating the test failures in our E2E CI jobs because we weren't checking for the return codes of the commands run in the bash scripts. Also, this fixes setup* targets in Makefile so that expected images are deployed to the testing cluster. * asdf --- components/odh-notebook-controller/Makefile | 15 ++---- .../run-e2e-test-service-mesh.sh | 51 +++++++++++++------ .../odh-notebook-controller/run-e2e-test.sh | 51 +++++++++++++------ 3 files changed, 77 insertions(+), 40 deletions(-) diff --git a/components/odh-notebook-controller/Makefile b/components/odh-notebook-controller/Makefile index 31ef73c671f..90de33e4e54 100644 --- a/components/odh-notebook-controller/Makefile +++ b/components/odh-notebook-controller/Makefile @@ -134,16 +134,13 @@ endif setup-kf: kustomize ## Replace Kustomize manifests with your environment configuration. sed -i'' -e 's,namespace: .*,namespace: '"${K8S_NAMESPACE}"',' \ ../notebook-controller/config/overlays/openshift/kustomization.yaml - sed -i'' -e 's,newName: .*,newName: '"${KF_IMG}"',' \ - ../notebook-controller/config/overlays/openshift/kustomization.yaml - sed -i'' -e 's,newTag: .*,newTag: '"${KF_TAG}"',' \ - ../notebook-controller/config/overlays/openshift/kustomization.yaml + sed -i'' -e "s,odh-kf-notebook-controller-image=.*,odh-kf-notebook-controller-image=${KF_IMG}:${KF_TAG}," \ + ../notebook-controller/config/overlays/openshift/params.env .PHONY: setup setup: manifests kustomize ## Replace Kustomize manifests with your environment configuration. sed -i'' -e 's,namespace: .*,namespace: '"${K8S_NAMESPACE}"',' ./config/default/kustomization.yaml - sed -i'' -e 's,newName: .*,newName: '"${IMG}"',' ./config/base/kustomization.yaml - sed -i'' -e 's,newTag: .*,newTag: '"${TAG}"',' ./config/base/kustomization.yaml + sed -i'' -e "s,odh-notebook-controller-image=.*,odh-notebook-controller-image=${IMG}:${TAG}," ./config/base/params.env .PHONY: setup-service-mesh setup-service-mesh: kustomize ## Replace Kustomize manifests with your environment configuration. @@ -153,10 +150,8 @@ setup-service-mesh: kustomize ## Replace Kustomize manifests with your environme ../notebook-controller/config/overlays/standalone-service-mesh/kustomization.yaml sed -i'' -e 's,ISTIO_GATEWAY=.*,ISTIO_GATEWAY='"${K8S_NAMESPACE}/odh-gateway"',' \ ../notebook-controller/config/overlays/standalone-service-mesh/kustomization.yaml - sed -i'' -e 's,newName: .*,newName: '"${KF_IMG}"',' \ - ../notebook-controller/config/overlays/standalone-service-mesh/kustomization.yaml - sed -i'' -e 's,newTag: .*,newTag: '"${KF_TAG}"',' \ - ../notebook-controller/config/overlays/standalone-service-mesh/kustomization.yaml + sed -i'' -e "s,odh-kf-notebook-controller-image=.*,odh-kf-notebook-controller-image=${KF_IMG}:${KF_TAG}," \ + ../notebook-controller/config/overlays/openshift/params.env sed -i'' -e 's,host: .*,host: opendatahub.'"$(shell kubectl get ingress.config.openshift.io cluster -o 'jsonpath={.spec.domain}')"',' \ ../notebook-controller/config/overlays/standalone-service-mesh/gateway-route.yaml diff --git a/components/odh-notebook-controller/run-e2e-test-service-mesh.sh b/components/odh-notebook-controller/run-e2e-test-service-mesh.sh index db4c6d0eb4a..1e431a8a53b 100755 --- a/components/odh-notebook-controller/run-e2e-test-service-mesh.sh +++ b/components/odh-notebook-controller/run-e2e-test-service-mesh.sh @@ -1,23 +1,44 @@ #!/usr/bin/env bash +# https://vaneyckt.io/posts/safer_bash_scripts_with_set_euxo_pipefail/ +set -Eeuxo pipefail + +echo "Running the ${0} setup" + TEST_NAMESPACE="odh-notebook-controller-system" -# setup and deploy the controller -oc new-project $TEST_NAMESPACE -n $TEST_NAMESPACE --skip-config-write +# Following variables are optional - if not set, the default values in relevant params.env +# will be used for both images. As such, if you want to run tests against your custom changes, +# be sure to perform a docker build and set these variables accordingly! +ODH_NOTEBOOK_CONTROLLER_IMAGE="${ODH_NOTEBOOK_CONTROLLER_IMAGE:-}" +KF_NOTEBOOK_CONTROLLER="${KF_NOTEBOOK_CONTROLLER:-}" -IFS=':' read -r -a CTRL_IMG <<< "${ODH_NOTEBOOK_CONTROLLER_IMAGE}" -export IMG="${CTRL_IMG[0]}" -export TAG="${CTRL_IMG[1]}" -IFS=':' read -r -a KF_NBC_IMG <<< "${KF_NOTEBOOK_CONTROLLER}" -export KF_IMG="${KF_NBC_IMG[0]}" -export KF_TAG="${KF_NBC_IMG[1]}" -export K8S_NAMESPACE=$TEST_NAMESPACE -make deploy-service-mesh deploy-with-mesh +if test -n "${ODH_NOTEBOOK_CONTROLLER_IMAGE}"; then + IFS=':' read -r -a CTRL_IMG <<< "${ODH_NOTEBOOK_CONTROLLER_IMAGE}" + export IMG="${CTRL_IMG[0]}" + export TAG="${CTRL_IMG[1]}" +fi -# run e2e tests -make e2e-test-service-mesh +if test -n "${KF_NOTEBOOK_CONTROLLER}"; then + IFS=':' read -r -a KF_NBC_IMG <<< "${KF_NOTEBOOK_CONTROLLER}" + export KF_IMG="${KF_NBC_IMG[0]}" + export KF_TAG="${KF_NBC_IMG[1]}" +fi + +export K8S_NAMESPACE="${TEST_NAMESPACE}" -# cleanup deployment -make undeploy-with-mesh undeploy-service-mesh -oc delete project $TEST_NAMESPACE -n $TEST_NAMESPACE +# From now on we want to be sure that undeploy and testing project deletion are called + +function cleanup() { + echo "Performing deployment cleanup of the ${0}" + make undeploy-with-mesh undeploy-service-mesh && oc delete project "${TEST_NAMESPACE}" +} +trap cleanup EXIT + +# setup and deploy the controller +oc new-project "${TEST_NAMESPACE}" --skip-config-write + +# deploy and run e2e tests +make deploy-service-mesh deploy-with-mesh +make e2e-test-service-mesh diff --git a/components/odh-notebook-controller/run-e2e-test.sh b/components/odh-notebook-controller/run-e2e-test.sh index bf6c6a42e0e..0dc75e400ba 100755 --- a/components/odh-notebook-controller/run-e2e-test.sh +++ b/components/odh-notebook-controller/run-e2e-test.sh @@ -1,23 +1,44 @@ #!/usr/bin/env bash +# https://vaneyckt.io/posts/safer_bash_scripts_with_set_euxo_pipefail/ +set -Eeuxo pipefail + +echo "Running the ${0} setup" + TEST_NAMESPACE="odh-notebook-controller-system" -# setup and deploy the controller -oc new-project $TEST_NAMESPACE -n $TEST_NAMESPACE --skip-config-write +# Following variables are optional - if not set, the default values in relevant params.env +# will be used for both images. As such, if you want to run tests against your custom changes, +# be sure to perform a docker build and set these variables accordingly! +ODH_NOTEBOOK_CONTROLLER_IMAGE="${ODH_NOTEBOOK_CONTROLLER_IMAGE:-}" +KF_NOTEBOOK_CONTROLLER="${KF_NOTEBOOK_CONTROLLER:-}" -IFS=':' read -r -a CTRL_IMG <<< "${ODH_NOTEBOOK_CONTROLLER_IMAGE}" -export IMG="${CTRL_IMG[0]}" -export TAG="${CTRL_IMG[1]}" -IFS=':' read -r -a KF_NBC_IMG <<< "${KF_NOTEBOOK_CONTROLLER}" -export KF_IMG="${KF_NBC_IMG[0]}" -export KF_TAG="${KF_NBC_IMG[1]}" -export K8S_NAMESPACE=$TEST_NAMESPACE -make deploy +if test -n "${ODH_NOTEBOOK_CONTROLLER_IMAGE}"; then + IFS=':' read -r -a CTRL_IMG <<< "${ODH_NOTEBOOK_CONTROLLER_IMAGE}" + export IMG="${CTRL_IMG[0]}" + export TAG="${CTRL_IMG[1]}" +fi -# run e2e tests -make e2e-test +if test -n "${KF_NOTEBOOK_CONTROLLER}"; then + IFS=':' read -r -a KF_NBC_IMG <<< "${KF_NOTEBOOK_CONTROLLER}" + export KF_IMG="${KF_NBC_IMG[0]}" + export KF_TAG="${KF_NBC_IMG[1]}" +fi + +export K8S_NAMESPACE="${TEST_NAMESPACE}" -# cleanup deployment -make undeploy -oc delete project $TEST_NAMESPACE -n $TEST_NAMESPACE +# From now on we want to be sure that undeploy and testing project deletion are called + +function cleanup() { + echo "Performing deployment cleanup of the ${0}" + make undeploy && oc delete project "${TEST_NAMESPACE}" +} +trap cleanup EXIT + +# setup and deploy the controller +oc new-project "${TEST_NAMESPACE}" + +# deploy and run e2e tests +make deploy +make e2e-test From f1be2a4bbf0677ae9be97c1d4aedebe96627a4b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jiri=20Dan=C4=9Bk?= Date: Mon, 25 Nov 2024 18:20:39 +0100 Subject: [PATCH 35/50] RHOAIENG-15748: chore(release): set the main tag to `main`, so we need not update it with digest updater from now on (#468) --- .../notebook-controller/config/overlays/openshift/params.env | 2 +- components/odh-notebook-controller/config/base/params.env | 2 +- components/odh-notebook-controller/makefile-vars.mk | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/components/notebook-controller/config/overlays/openshift/params.env b/components/notebook-controller/config/overlays/openshift/params.env index 752384aeaf7..7fe7b775d19 100644 --- a/components/notebook-controller/config/overlays/openshift/params.env +++ b/components/notebook-controller/config/overlays/openshift/params.env @@ -1 +1 @@ -odh-kf-notebook-controller-image=quay.io/opendatahub/kubeflow-notebook-controller:main-363bcdb +odh-kf-notebook-controller-image=quay.io/opendatahub/kubeflow-notebook-controller:main diff --git a/components/odh-notebook-controller/config/base/params.env b/components/odh-notebook-controller/config/base/params.env index 664290753b9..f20ffe232a8 100644 --- a/components/odh-notebook-controller/config/base/params.env +++ b/components/odh-notebook-controller/config/base/params.env @@ -1 +1 @@ -odh-notebook-controller-image=quay.io/opendatahub/odh-notebook-controller:main-363bcdb +odh-notebook-controller-image=quay.io/opendatahub/odh-notebook-controller:main diff --git a/components/odh-notebook-controller/makefile-vars.mk b/components/odh-notebook-controller/makefile-vars.mk index 827d0ea7820..fcd1af1d6bc 100644 --- a/components/odh-notebook-controller/makefile-vars.mk +++ b/components/odh-notebook-controller/makefile-vars.mk @@ -1 +1 @@ -KF_TAG ?= main-363bcdb +KF_TAG ?= main From 989d636e6dde8d616ae2ae819d0583e72db6b375 Mon Sep 17 00:00:00 2001 From: Diamond Bryant Date: Tue, 26 Nov 2024 09:57:59 -0500 Subject: [PATCH 36/50] Create a .yaml file with kubeflow version --- .../config/component_metadata.yaml | 4 + .../generate-metadata-yaml.sh | 165 ++++++++++++++++++ 2 files changed, 169 insertions(+) create mode 100644 components/notebook-controller/config/component_metadata.yaml create mode 100644 components/notebook-controller/generate-metadata-yaml.sh diff --git a/components/notebook-controller/config/component_metadata.yaml b/components/notebook-controller/config/component_metadata.yaml new file mode 100644 index 00000000000..20a75ddd24c --- /dev/null +++ b/components/notebook-controller/config/component_metadata.yaml @@ -0,0 +1,4 @@ +releases: + - name: Kubeflow Notebook + version: 1.9.0 + repoUrl: https://github.com/kubeflow/kubeflow diff --git a/components/notebook-controller/generate-metadata-yaml.sh b/components/notebook-controller/generate-metadata-yaml.sh new file mode 100644 index 00000000000..3a48c4b3b13 --- /dev/null +++ b/components/notebook-controller/generate-metadata-yaml.sh @@ -0,0 +1,165 @@ +#!/usr/bin/env bash + +set -uo pipefail + +function trap_exit() { + rc=$? + + set +x + + exit $rc +} + +trap "trap_exit" EXIT + +_derive_metadata() +{ + # inspired from https://stackoverflow.com/a/29835459 + current_dir=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd) + + kf_project_file="${current_dir}/PROJECT" + if [ -e "${kf_project_file}" ]; then + + if [ -z "${metadata_repo_url}" ]; then + project_repo_reference=$(yq -e '.repo' "${kf_project_file}") + project_repo_parts=( $(printf "%s" ${project_repo_reference##https://} | tr '/' ' ') ) + github_host="${project_repo_parts[0]}" + github_owner="${project_repo_parts[1]}" + github_repo="${project_repo_parts[2]}" + + metadata_repo_url=$(printf "https://%s/%s/%s" "${github_host}" "${github_owner}" "${github_repo}") + fi + + if [ -z "${metadata_name}" ]; then + project_domain=$(yq -e '.domain' "${kf_project_file}") + project_name=$(yq -e '.projectName' "${kf_project_file}") + metadata_name="${project_domain} ${project_name}" + fi + + fi + + if [ -z "${metadata_version}" ]; then + repo_root=$(git rev-parse --show-toplevel) + version_file="${repo_root}/releasing/version/VERSION" + + metadata_version=$(cat "${version_file}" | head -n 1) + fi +} + +_fallback_to_existing_values() +{ + if [ -n "${existing_fallback}" ]; then + if [ -z "${metadata_repo_url}" ]; then + metadata_repo_url=$(yq -e '.releases[0].repoUrl' "${output_file}") + fi + + if [ -z "${metadata_version}" ]; then + metadata_version=$(yq -e '.releases[0].version' "${output_file}") + fi + + if [ -z "${metadata_name}" ]; then + metadata_name=$(yq -e '.releases[0].name' "${output_file}") + fi + fi +} + +_check_for_missing_data() +{ + missing_data= + + if [ -z "${metadata_repo_url}" ]; then + printf "%s\n" "repoUrl attribute not specified and unable to be inferred" + missing_data=1 + fi + + if [ -z "${metadata_version}" ]; then + printf "%s\n" "version attribute not specified and unable to be inferred" + missing_data=1 + fi + + if [ -z "${metadata_name}" ]; then + printf "%s\n" "name attribute not specified and unable to be inferred" + missing_data=1 + fi + + if [ -n "${missing_data}" ]; then + exit 1 + fi +} + +_handle_metadata_file() +{ + + _derive_metadata + + _fallback_to_existing_values + + _check_for_missing_data + + # NOTE: Does not handle multiple entries!! + yq_env_arg="${metadata_name}" yq -i '.releases[0].name = strenv(yq_env_arg)' "${output_file}" + yq_env_arg="${metadata_version}" yq -i '.releases[0].version = strenv(yq_env_arg)' "${output_file}" + yq_env_arg="${metadata_repo_url}" yq -i '.releases[0].repoUrl = strenv(yq_env_arg)' "${output_file}" +} + +_usage() +{ + printf "%s\n" "Usage: $(basename $0) -o [-n ] [-v ] [-r ] [-p] [-x] [-h]" +} + +_parse_opts() +{ + local OPTIND + + while getopts ':o:n:v:r:exh' OPTION; do + case "${OPTION}" in + o ) + output_file="${OPTARG}" + + if ! [ -e "${output_file}" ]; then + touch "${output_file}" + fi + ;; + n ) + metadata_name="${OPTARG}" + ;; + v ) + metadata_version="${OPTARG}" + ;; + r ) + metadata_repo_url="${OPTARG}" + ;; + e ) + existing_fallback="t" + ;; + h) + _usage + exit + ;; + * ) + _usage + exit 1 + ;; + esac + done +} + +output_file= +metadata_repo_url= +metadata_version= +metadata_name= +existing_fallback= + +if ! yq --version &> /dev/null; then + printf "%s" "yq not installed... aborting script." + exit 1 +fi + +_parse_opts "$@" + +if [ -z "${output_file}" ]; then + printf "%s" "-o argument is required" + exit 1 +fi + +_handle_metadata_file \ No newline at end of file From bb2dd26fd114081d67170c8e6117c08a1a7f846a Mon Sep 17 00:00:00 2001 From: Sven Thoms <21118431+shalberd@users.noreply.github.com> Date: Wed, 27 Nov 2024 16:31:05 +0100 Subject: [PATCH 37/50] RHOAIENG-8390: feat(odh-nbc): search for imagestreams only in the namespace the controller runs in (#375) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [Fix] RHOAIENG-8390: feat(odh-nbc): search for imagestreams only in the namespace the controller runs in (#375) * [Fix] search for imagestreams only in the namespace the controller runs in. - Take namespace that the controller runs in - Log that dynamically-determined namespace once at startup. - Use hard-coded namespace for unit tests. * review suggestion, fix comment in tests --------- Co-authored-by: Jiri Daněk --- .../controllers/notebook_controller.go | 5 +- .../controllers/notebook_controller_test.go | 15 +-- .../controllers/notebook_network.go | 21 +---- .../controllers/notebook_webhook.go | 93 +++++++++---------- .../controllers/suite_test.go | 19 ++-- components/odh-notebook-controller/main.go | 33 +++++-- 6 files changed, 93 insertions(+), 93 deletions(-) diff --git a/components/odh-notebook-controller/controllers/notebook_controller.go b/components/odh-notebook-controller/controllers/notebook_controller.go index daac2b87bce..e8b77ace91f 100644 --- a/components/odh-notebook-controller/controllers/notebook_controller.go +++ b/components/odh-notebook-controller/controllers/notebook_controller.go @@ -57,8 +57,9 @@ const ( // OpenshiftNotebookReconciler holds the controller configuration. type OpenshiftNotebookReconciler struct { client.Client - Scheme *runtime.Scheme - Log logr.Logger + Namespace string + Scheme *runtime.Scheme + Log logr.Logger } // ClusterRole permissions diff --git a/components/odh-notebook-controller/controllers/notebook_controller_test.go b/components/odh-notebook-controller/controllers/notebook_controller_test.go index 41e35ee800a..2908b7fae9b 100644 --- a/components/odh-notebook-controller/controllers/notebook_controller_test.go +++ b/components/odh-notebook-controller/controllers/notebook_controller_test.go @@ -20,9 +20,7 @@ import ( "crypto/x509" "encoding/pem" "fmt" - "io/ioutil" "os" - "strings" "time" "github.com/go-logr/logr" @@ -432,12 +430,7 @@ var _ = Describe("The Openshift Notebook controller", func() { notebook := createNotebook(Name, Namespace) npProtocol := corev1.ProtocolTCP - testPodNamespace := "redhat-ods-applications" - if data, err := ioutil.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace"); err == nil { - if ns := strings.TrimSpace(string(data)); len(ns) > 0 { - testPodNamespace = ns - } - } + testPodNamespace := odhNotebookControllerTestNamespace expectedNotebookNetworkPolicy := netv1.NetworkPolicy{ ObjectMeta: metav1.ObjectMeta{ @@ -462,9 +455,9 @@ var _ = Describe("The Openshift Notebook controller", func() { }, From: []netv1.NetworkPolicyPeer{ { - // Since for unit tests we do not have context, - // namespace will fallback to test pod namespace - // if run in CI or `redhat-ods-applications` if run locally + // Since for unit tests the controller does not run in a cluster pod, + // it cannot detect its own pod's namespace. Therefore, we define it + // to be `redhat-ods-applications` (in suite_test.go) NamespaceSelector: &metav1.LabelSelector{ MatchLabels: map[string]string{ "kubernetes.io/metadata.name": testPodNamespace, diff --git a/components/odh-notebook-controller/controllers/notebook_network.go b/components/odh-notebook-controller/controllers/notebook_network.go index 4a42e5ceacf..47169d84e9c 100644 --- a/components/odh-notebook-controller/controllers/notebook_network.go +++ b/components/odh-notebook-controller/controllers/notebook_network.go @@ -19,10 +19,9 @@ import ( "context" corev1 "k8s.io/api/core/v1" netv1 "k8s.io/api/networking/v1" - "os" "reflect" - "strings" + "github.com/go-logr/logr" nbv1 "github.com/kubeflow/kubeflow/components/notebook-controller/api/v1" apierrs "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -44,7 +43,7 @@ func (r *OpenshiftNotebookReconciler) ReconcileAllNetworkPolicies(notebook *nbv1 log := r.Log.WithValues("notebook", notebook.Name, "namespace", notebook.Namespace) // Generate the desired Network Policies - desiredNotebookNetworkPolicy := NewNotebookNetworkPolicy(notebook) + desiredNotebookNetworkPolicy := NewNotebookNetworkPolicy(notebook, log, r.Namespace) // Create Network Policies if they do not already exist err := r.reconcileNetworkPolicy(desiredNotebookNetworkPolicy, ctx, notebook) @@ -129,11 +128,11 @@ func CompareNotebookNetworkPolicies(np1 netv1.NetworkPolicy, np2 netv1.NetworkPo } // NewNotebookNetworkPolicy defines the desired network policy for Notebook port -func NewNotebookNetworkPolicy(notebook *nbv1.Notebook) *netv1.NetworkPolicy { +func NewNotebookNetworkPolicy(notebook *nbv1.Notebook, log logr.Logger, namespace string) *netv1.NetworkPolicy { npProtocol := corev1.ProtocolTCP namespaceSel := metav1.LabelSelector{ MatchLabels: map[string]string{ - "kubernetes.io/metadata.name": getControllerNamespace(), + "kubernetes.io/metadata.name": namespace, }, } // Create a Kubernetes NetworkPolicy resource that allows all traffic to the oauth port of a notebook @@ -209,15 +208,3 @@ func NewOAuthNetworkPolicy(notebook *nbv1.Notebook) *netv1.NetworkPolicy { }, } } - -func getControllerNamespace() string { - // TODO:Add env variable that stores namespace for both controllers. - if data, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace"); err == nil { - if ns := strings.TrimSpace(string(data)); len(ns) > 0 { - return ns - } - } - - // Fallback to default namespace, keep default as redhat-ods-applications - return "redhat-ods-applications" -} diff --git a/components/odh-notebook-controller/controllers/notebook_webhook.go b/components/odh-notebook-controller/controllers/notebook_webhook.go index 0f3163bd758..5b0db331f81 100644 --- a/components/odh-notebook-controller/controllers/notebook_webhook.go +++ b/components/odh-notebook-controller/controllers/notebook_webhook.go @@ -49,6 +49,8 @@ type NotebookWebhook struct { Config *rest.Config Decoder *admission.Decoder OAuthConfig OAuthConfig + // controller namespace + Namespace string } // InjectReconciliationLock injects the kubeflow notebook controller culling @@ -254,7 +256,7 @@ func (w *NotebookWebhook) Handle(ctx context.Context, req admission.Request) adm // Check Imagestream Info both on create and update operations if req.Operation == admissionv1.Create || req.Operation == admissionv1.Update { // Check Imagestream Info - err = SetContainerImageFromRegistry(ctx, w.Config, notebook, log) + err = SetContainerImageFromRegistry(ctx, w.Config, notebook, log, w.Namespace) if err != nil { return admission.Errored(http.StatusInternalServerError, err) } @@ -536,7 +538,7 @@ func InjectCertConfig(notebook *nbv1.Notebook, configMapName string) error { // If an internal registry is detected, it uses the default values specified in the Notebook Custom Resource (CR). // Otherwise, it checks the last-image-selection annotation to find the image stream and fetches the image from status.dockerImageReference, // assigning it to the container.image value. -func SetContainerImageFromRegistry(ctx context.Context, config *rest.Config, notebook *nbv1.Notebook, log logr.Logger) error { +func SetContainerImageFromRegistry(ctx context.Context, config *rest.Config, notebook *nbv1.Notebook, log logr.Logger, namespace string) error { // Create a dynamic client dynamicClient, err := dynamic.NewForConfig(config) if err != nil { @@ -575,63 +577,56 @@ func SetContainerImageFromRegistry(ctx context.Context, config *rest.Config, not return fmt.Errorf("invalid image selection format") } - // Specify the namespaces to search in - namespaces := []string{"opendatahub", "redhat-ods-applications"} imagestreamFound := false - for _, namespace := range namespaces { - // List imagestreams in the specified namespace - imagestreams, err := dynamicClient.Resource(ims).Namespace(namespace).List(ctx, metav1.ListOptions{}) - if err != nil { - log.Info("Cannot list imagestreams", "error", err) - continue - } + // List imagestreams in the specified namespace + imagestreams, err := dynamicClient.Resource(ims).Namespace(namespace).List(ctx, metav1.ListOptions{}) + if err != nil { + log.Info("Cannot list imagestreams", "error", err) + continue + } - // Iterate through the imagestreams to find matches - for _, item := range imagestreams.Items { - metadata := item.Object["metadata"].(map[string]interface{}) - name := metadata["name"].(string) - - // Match with the ImageStream name - if name == imageSelected[0] { - status := item.Object["status"].(map[string]interface{}) - - // Match to the corresponding tag of the image - tags := status["tags"].([]interface{}) - for _, t := range tags { - tagMap := t.(map[string]interface{}) - tagName := tagMap["tag"].(string) - if tagName == imageSelected[1] { - items := tagMap["items"].([]interface{}) - if len(items) > 0 { - // Sort items by creationTimestamp to get the most recent one - sort.Slice(items, func(i, j int) bool { - iTime := items[i].(map[string]interface{})["created"].(string) - jTime := items[j].(map[string]interface{})["created"].(string) - return iTime > jTime // Lexicographical comparison of RFC3339 timestamps - }) - imageHash := items[0].(map[string]interface{})["dockerImageReference"].(string) - // Update the Containers[i].Image value - notebook.Spec.Template.Spec.Containers[i].Image = imageHash - // Update the JUPYTER_IMAGE environment variable with the image selection for example "jupyter-datascience-notebook:2023.2" - for i, envVar := range container.Env { - if envVar.Name == "JUPYTER_IMAGE" { - container.Env[i].Value = imageSelection - break - } + // Iterate through the imagestreams to find matches + for _, item := range imagestreams.Items { + metadata := item.Object["metadata"].(map[string]interface{}) + name := metadata["name"].(string) + + // Match with the ImageStream name + if name == imageSelected[0] { + status := item.Object["status"].(map[string]interface{}) + + // Match to the corresponding tag of the image + tags := status["tags"].([]interface{}) + for _, t := range tags { + tagMap := t.(map[string]interface{}) + tagName := tagMap["tag"].(string) + if tagName == imageSelected[1] { + items := tagMap["items"].([]interface{}) + if len(items) > 0 { + // Sort items by creationTimestamp to get the most recent one + sort.Slice(items, func(i, j int) bool { + iTime := items[i].(map[string]interface{})["created"].(string) + jTime := items[j].(map[string]interface{})["created"].(string) + return iTime > jTime // Lexicographical comparison of RFC3339 timestamps + }) + imageHash := items[0].(map[string]interface{})["dockerImageReference"].(string) + // Update the Containers[i].Image value + notebook.Spec.Template.Spec.Containers[i].Image = imageHash + // Update the JUPYTER_IMAGE environment variable with the image selection for example "jupyter-datascience-notebook:2023.2" + for i, envVar := range container.Env { + if envVar.Name == "JUPYTER_IMAGE" { + container.Env[i].Value = imageSelection + break } - imagestreamFound = true - break } + imagestreamFound = true + break } } } } - if imagestreamFound { - break - } } if !imagestreamFound { - log.Error(nil, "Imagestream not found in any of the specified namespaces", "imageSelected", imageSelected[0], "tag", imageSelected[1]) + log.Error(nil, "Imagestream not found in main controller namespace", "imageSelected", imageSelected[0], "tag", imageSelected[1], "namespace", namespace) } } } diff --git a/components/odh-notebook-controller/controllers/suite_test.go b/components/odh-notebook-controller/controllers/suite_test.go index a9c93337db4..1cdc8ce65a4 100644 --- a/components/odh-notebook-controller/controllers/suite_test.go +++ b/components/odh-notebook-controller/controllers/suite_test.go @@ -69,8 +69,9 @@ var ( ) const ( - timeout = time.Second * 10 - interval = time.Second * 2 + timeout = time.Second * 10 + interval = time.Second * 2 + odhNotebookControllerTestNamespace = "redhat-ods-applications" ) func TestAPIs(t *testing.T) { @@ -173,9 +174,10 @@ var _ = BeforeSuite(func() { // Setup notebook controller err = (&OpenshiftNotebookReconciler{ - Client: mgr.GetClient(), - Log: ctrl.Log.WithName("controllers").WithName("notebook-controller"), - Scheme: mgr.GetScheme(), + Client: mgr.GetClient(), + Log: ctrl.Log.WithName("controllers").WithName("notebook-controller"), + Scheme: mgr.GetScheme(), + Namespace: odhNotebookControllerTestNamespace, }).SetupWithManager(mgr) Expect(err).ToNot(HaveOccurred()) @@ -183,9 +185,10 @@ var _ = BeforeSuite(func() { hookServer := mgr.GetWebhookServer() notebookWebhook := &webhook.Admission{ Handler: &NotebookWebhook{ - Log: ctrl.Log.WithName("controllers").WithName("notebook-controller"), - Client: mgr.GetClient(), - Config: mgr.GetConfig(), + Log: ctrl.Log.WithName("controllers").WithName("notebook-controller"), + Client: mgr.GetClient(), + Config: mgr.GetConfig(), + Namespace: odhNotebookControllerTestNamespace, OAuthConfig: OAuthConfig{ ProxyImage: OAuthProxyImage, }, diff --git a/components/odh-notebook-controller/main.go b/components/odh-notebook-controller/main.go index 698bca75a88..85fa78e0801 100644 --- a/components/odh-notebook-controller/main.go +++ b/components/odh-notebook-controller/main.go @@ -17,6 +17,7 @@ package main import ( "flag" + "fmt" "os" "time" @@ -59,6 +60,17 @@ func init() { //+kubebuilder:scaffold:scheme } +func getControllerNamespace() (string, error) { + // Try to get the namespace from the service account secret + if data, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace"); err == nil { + if ns := string(data); len(ns) > 0 { + return ns, nil + } + } + + return "", fmt.Errorf("unable to determine the namespace") +} + func main() { var metricsAddr, probeAddr, oauthProxyImage string var webhookPort int @@ -104,10 +116,18 @@ func main() { } // Setup notebook controller + // determine and set the controller namespace + namespace, err := getControllerNamespace() + if err != nil { + setupLog.Error(err, "Error during determining controller / main namespace") + os.Exit(1) + } + setupLog.Info("Controller is running in namespace", "namespace", namespace) if err = (&controllers.OpenshiftNotebookReconciler{ - Client: mgr.GetClient(), - Log: ctrl.Log.WithName("controllers").WithName("Notebook"), - Scheme: mgr.GetScheme(), + Client: mgr.GetClient(), + Log: ctrl.Log.WithName("controllers").WithName("Notebook"), + Namespace: namespace, + Scheme: mgr.GetScheme(), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "Notebook") os.Exit(1) @@ -117,9 +137,10 @@ func main() { hookServer := mgr.GetWebhookServer() notebookWebhook := &webhook.Admission{ Handler: &controllers.NotebookWebhook{ - Log: ctrl.Log.WithName("controllers").WithName("Notebook"), - Client: mgr.GetClient(), - Config: mgr.GetConfig(), + Log: ctrl.Log.WithName("controllers").WithName("Notebook"), + Client: mgr.GetClient(), + Config: mgr.GetConfig(), + Namespace: namespace, OAuthConfig: controllers.OAuthConfig{ ProxyImage: oauthProxyImage, }, From fc5ffd48ee283f94f94a0e7c65152523820a45fd Mon Sep 17 00:00:00 2001 From: Jan Stourac Date: Wed, 27 Nov 2024 19:05:04 +0100 Subject: [PATCH 38/50] [RHOAIENG-15141] Fix failing E2E tests (#473) There were two E2E tests in failure: --- FAIL: TestE2ENotebookController/update/thoth-minimal-oauth-notebook/Notebook_Statefulset_Validation_After_Update (60.28s) --- FAIL: TestE2ENotebookController/update/thoth-minimal-oauth-notebook/Verify_Notebook_Traffic_After_Update (10.75s) The reason was that after the culling test, the culling configuration is reverted, but the Notebook CR isn't updated and the annotation that was added by the notebook controller: `kubeflow-resource-stopped` stays in that CR and as such, the workbench instance isn't started back. This change deletes this annotation at the end of the culler test and the workbench is up and running and ready for the followup tests. Another issue there was that the update test updated the workbench to a non-existent image link, which resulted in a ImagePullBackOff Error in the end. This change updates this link to some existing image. --- .../e2e/helper_test.go | 46 +++++++++++++++++-- .../e2e/notebook_creation_test.go | 21 +-------- .../e2e/notebook_update_test.go | 5 +- 3 files changed, 48 insertions(+), 24 deletions(-) diff --git a/components/odh-notebook-controller/e2e/helper_test.go b/components/odh-notebook-controller/e2e/helper_test.go index 792187e4a69..9fe72ec2bc9 100644 --- a/components/odh-notebook-controller/e2e/helper_test.go +++ b/components/odh-notebook-controller/e2e/helper_test.go @@ -3,7 +3,6 @@ package e2e import ( "crypto/tls" "fmt" - netv1 "k8s.io/api/networking/v1" "log" "net/http" "time" @@ -12,9 +11,11 @@ import ( routev1 "github.com/openshift/api/route/v1" appsv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" + netv1 "k8s.io/api/networking/v1" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/wait" "sigs.k8s.io/controller-runtime/pkg/client" @@ -130,18 +131,57 @@ func (tc *testContext) rolloutDeployment(depMeta metav1.ObjectMeta) error { return nil } -func (tc *testContext) revertCullingConfiguration(cmMeta metav1.ObjectMeta, depMeta metav1.ObjectMeta) { +func (tc *testContext) waitForStatefulSet(nbMeta *metav1.ObjectMeta, availableReplicas int32, readyReplicas int32) error { + // Verify StatefulSet is running expected number of replicas + err := wait.Poll(tc.resourceRetryInterval, tc.resourceCreationTimeout, func() (done bool, err error) { + notebookStatefulSet, err1 := tc.kubeClient.AppsV1().StatefulSets(tc.testNamespace).Get(tc.ctx, + nbMeta.Name, metav1.GetOptions{}) + + if err1 != nil { + if errors.IsNotFound(err1) { + return false, nil + } else { + log.Printf("Failed to get %s statefulset", nbMeta.Name) + return false, err1 + } + } + if notebookStatefulSet.Status.AvailableReplicas == availableReplicas && + notebookStatefulSet.Status.ReadyReplicas == readyReplicas { + return true, nil + } + return false, nil + }) + return err +} + +func (tc *testContext) revertCullingConfiguration(cmMeta metav1.ObjectMeta, depMeta metav1.ObjectMeta, nbMeta *metav1.ObjectMeta) { // Delete the culling configuration Configmap once the test is completed err := tc.kubeClient.CoreV1().ConfigMaps(tc.testNamespace).Delete(tc.ctx, cmMeta.Name, metav1.DeleteOptions{}) if err != nil { - log.Printf("error creating configmap notebook-controller-culler-config: %v ", err) + log.Printf("error deleting configmap notebook-controller-culler-config: %v ", err) } // Roll out the controller deployment err = tc.rolloutDeployment(depMeta) if err != nil { log.Printf("error rolling out the deployment %v: %v ", depMeta.Name, err) } + + testNotebook := &nbv1.Notebook{ + ObjectMeta: *nbMeta, + } + // The NBC added the annotation to stop the idle workbench: kubeflow-resource-stopped: '2024-11-26T17:20:42Z' + // To make the workbench running again, we need to also remove that annotation. + patch := client.RawPatch(types.JSONPatchType, []byte(`[{"op": "remove", "path": "/metadata/annotations/kubeflow-resource-stopped"}]`)) + + if err := tc.customClient.Patch(tc.ctx, testNotebook, patch); err != nil { + log.Printf("failed to patch Notebook CR removing the kubeflow-resource-stopped annotation: %v ", err) + } + // now we should wait for pod to start again + err = tc.waitForStatefulSet(nbMeta, 1, 1) + if err != nil { + log.Printf("notebook StatefulSet: %s isn't ready as expected: %s", nbMeta.Name, err) + } } func (tc *testContext) scaleDeployment(depMeta metav1.ObjectMeta, desiredReplicas int32) error { diff --git a/components/odh-notebook-controller/e2e/notebook_creation_test.go b/components/odh-notebook-controller/e2e/notebook_creation_test.go index e066a36ad3a..c6bea3b0a9c 100644 --- a/components/odh-notebook-controller/e2e/notebook_creation_test.go +++ b/components/odh-notebook-controller/e2e/notebook_creation_test.go @@ -198,24 +198,7 @@ func (tc *testContext) ensureNetworkPolicyAllowingAccessToOnlyNotebookController func (tc *testContext) testNotebookValidation(nbMeta *metav1.ObjectMeta) error { // Verify StatefulSet is running - err := wait.Poll(tc.resourceRetryInterval, tc.resourceCreationTimeout, func() (done bool, err error) { - notebookStatefulSet, err1 := tc.kubeClient.AppsV1().StatefulSets(tc.testNamespace).Get(tc.ctx, - nbMeta.Name, metav1.GetOptions{}) - - if err1 != nil { - if errors.IsNotFound(err1) { - return false, nil - } else { - log.Printf("Failed to get %s statefulset", nbMeta.Name) - return false, err1 - } - } - if notebookStatefulSet.Status.AvailableReplicas == 1 && - notebookStatefulSet.Status.ReadyReplicas == 1 { - return true, nil - } - return false, nil - }) + err := tc.waitForStatefulSet(nbMeta, 1, 1) if err != nil { return fmt.Errorf("error validating notebook StatefulSet: %s", err) } @@ -283,7 +266,7 @@ func (tc *testContext) testNotebookCulling(nbMeta *metav1.ObjectMeta) error { return fmt.Errorf("error getting deployment %v: %v", controllerDeployment.Name, err) } - defer tc.revertCullingConfiguration(cullingConfigMap.ObjectMeta, controllerDeployment.ObjectMeta) + defer tc.revertCullingConfiguration(cullingConfigMap.ObjectMeta, controllerDeployment.ObjectMeta, nbMeta) err = tc.rolloutDeployment(controllerDeployment.ObjectMeta) if err != nil { diff --git a/components/odh-notebook-controller/e2e/notebook_update_test.go b/components/odh-notebook-controller/e2e/notebook_update_test.go index 6bd0a94b9d3..a88713544dc 100644 --- a/components/odh-notebook-controller/e2e/notebook_update_test.go +++ b/components/odh-notebook-controller/e2e/notebook_update_test.go @@ -65,7 +65,8 @@ func (tc *testContext) testNotebookUpdate(nbContext notebookContext) error { } // Example update: Change the Notebook image - updatedNotebook.Spec.Template.Spec.Containers[0].Image = "new-image:latest" + newImage := "quay.io/opendatahub/workbench-images:jupyter-minimal-ubi9-python-3.11-20241119-3ceb400" + updatedNotebook.Spec.Template.Spec.Containers[0].Image = newImage err = tc.customClient.Update(tc.ctx, updatedNotebook) if err != nil { @@ -79,7 +80,7 @@ func (tc *testContext) testNotebookUpdate(nbContext notebookContext) error { if err != nil { return false, err } - if note.Spec.Template.Spec.Containers[0].Image == "new-image:latest" { + if note.Spec.Template.Spec.Containers[0].Image == newImage { return true, nil } return false, nil From feae52783cce6399aadfc1220db4c41f5e48e4d9 Mon Sep 17 00:00:00 2001 From: aTheo Date: Thu, 28 Nov 2024 13:25:51 +0100 Subject: [PATCH 39/50] Kubeflow sync release pipeline (#469) * Modify notebook-controller-images-updater gha to be callable action and also to automerge or not the changes * Update sync branches action to automerge the changes to the target branch and changed its name * Introduce create-release gha used as mockup to unblock the development untill be ready * Introduce kubeflow-release action that orchestrates all previous manual steps * little tweeks * fix typos * Add an option create-new-release to create or not release, for extra flexibility * Apply suggestions from the review --- .github/workflows/create-release.yaml | 24 +++ .github/workflows/kubeflow-release.yaml | 138 ++++++++++++++++++ .../notebook-controller-images-updater.yaml | 88 +++++++---- .../workflows/sync-branches-through-pr.yaml | 80 ---------- .github/workflows/sync-branches.yaml | 88 +++++++++++ 5 files changed, 308 insertions(+), 110 deletions(-) create mode 100644 .github/workflows/create-release.yaml create mode 100644 .github/workflows/kubeflow-release.yaml delete mode 100644 .github/workflows/sync-branches-through-pr.yaml create mode 100644 .github/workflows/sync-branches.yaml diff --git a/.github/workflows/create-release.yaml b/.github/workflows/create-release.yaml new file mode 100644 index 00000000000..b6e674df046 --- /dev/null +++ b/.github/workflows/create-release.yaml @@ -0,0 +1,24 @@ +--- +name: Create Release +on: + workflow_dispatch: + inputs: + input_var: + description: "Say Hi!" + required: true + workflow_call: + inputs: + input_var: + type: string + required: true + +env: + INPUT_VAR: ${{ inputs.input_var }} + +jobs: + say-hello: + runs-on: ubuntu-latest + + steps: + - name: Say Hello + run: echo "${{ env.INPUT_VAR }}, World!" diff --git a/.github/workflows/kubeflow-release.yaml b/.github/workflows/kubeflow-release.yaml new file mode 100644 index 00000000000..f94bc80798d --- /dev/null +++ b/.github/workflows/kubeflow-release.yaml @@ -0,0 +1,138 @@ +--- +name: Kubeflow Release Pipeline +on: + workflow_dispatch: + inputs: + create-new-release: + description: "Create a new release?" + required: true + default: "true" +env: + CREATE_NEW_RELEASE: ${{ inputs.create-new-release }} + REPO_OWNER: opendatahub-io + REPO_NAME: kubeflow + BRANCH_NAME: v1.9-branch + +jobs: + # 1. Sync changes to opendatahub:v1.9-branch from opendatahub:main + sync-main-to-release-branch: + uses: opendatahub-io/kubeflow/.github/workflows/sync-branches.yaml@main + with: + source: "main" + target: "v1.9-branch" + + # 2. Poll for images to be available on quay.io the readiness of the images usually takes ~10 mins + wait-images-are-ready-on-quay: + needs: sync-main-to-release-branch + runs-on: ubuntu-latest + outputs: + images_ready: ${{ steps.check-images.outputs.images_ready }} + steps: + - name: Poll for images availability + id: check-images + run: | + # Install required tools + sudo apt-get update + sudo apt-get install -y skopeo jq curl + + # Get the latest Git hash from the target branch + PAYLOAD=$(curl --silent -H 'Accept: application/vnd.github.v4.raw' https://api.github.com/repos/$REPO_OWNER/$REPO_NAME/commits?sha=$BRANCH_NAME&per_page=1) + GIT_HASH=$(echo "$PAYLOAD" | jq -r '.[0].sha' | cut -c 1-7) + echo "GIT_HASH=$GIT_HASH" + + # Images to check + IMAGES=( + "quay.io/opendatahub/kubeflow-notebook-controller:1.9-${GIT_HASH}" + "quay.io/opendatahub/odh-notebook-controller:1.9-${GIT_HASH}" + ) + # Poll for image readiness total timeout=15m + MAX_ATTEMPTS=10 + SLEEP_DURATION=90 + for image in "${IMAGES[@]}"; do + for (( i=1; i<=MAX_ATTEMPTS; i++ )); do + echo "Checking availability of $image (Attempt $i/$MAX_ATTEMPTS)..." + if skopeo inspect docker://$image &>/dev/null; then + echo "$image is available!" + break + fi + if [[ $i -eq $MAX_ATTEMPTS ]]; then + echo "Timed out waiting for $image to become available." + exit 1 + fi + sleep $SLEEP_DURATION + done + done + echo "images_ready=true" >> $GITHUB_ENV + echo "images_ready=true" >> $GITHUB_OUTPUT + + - name: Images are ready + if: ${{ env.images_ready == 'true' }} + run: echo "All images are ready. Proceeding to the next step." + + # 3. Once Images are availble then updates the notebook controllers’ image tags + update-release-images: + needs: wait-images-are-ready-on-quay + if: ${{ needs.wait-images-are-ready-on-quay.outputs.images_ready == 'true' }} + uses: opendatahub-io/kubeflow/.github/workflows/notebook-controller-images-updater.yaml@main + with: + branch-name: "v1.9-branch" + organization: "opendatahub-io" + generate-pr: "true" + + # 4. Check PR merged status + check-pr-merged: + needs: update-release-images + runs-on: ubuntu-latest + outputs: + pr_merged: ${{ steps.check.outputs.pr_merged }} + steps: + - name: Check out repository + uses: actions/checkout@v4 + + - name: Check if the PR is merged + id: check + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + # PR to look for + PR_TITLE="[GHA-${{ github.run_id }}]" + # Fetch matching PRs + PR_NUMBER=$(gh pr list --repo "$REPO_OWNER/$REPO_NAME" --state all --search "$PR_TITLE" --json number,title | jq -r '.[0].number') + echo "PR number: $PR_NUMBER" + + if [ -z "$PR_NUMBER" ]; then + echo "No matching PR found." + exit 1 + fi + + # Polling loop to wait for the PR to be merged total timeout=1h + MAX_ATTEMPTS=10 + SLEEP_DURATION=360 + + for (( i=1; i<=MAX_ATTEMPTS; i++ )); do + echo "Checking if PR #$PR_NUMBER is merged (Attempt $i/$MAX_ATTEMPTS)..." + PR_STATE=$(gh pr view --repo "$REPO_OWNER/$REPO_NAME" $PR_NUMBER --json mergedAt --jq '.mergedAt') + + if [ "$PR_STATE" = "null" ] || [ -z "$PR_STATE" ]; then + echo "PR #$PR_NUMBER is not merged yet. Waiting..." + sleep $SLEEP_DURATION + else + echo "PR #$PR_NUMBER is merged!" + echo "pr_merged=true" >> $GITHUB_ENV + echo "pr_merged=true" >> $GITHUB_OUTPUT + exit 0 + fi + done + + echo "Timed out waiting for PR #$PR_NUMBER to be merged." + echo "pr_merged=false" >> $GITHUB_ENV + echo "pr_merged=false" >> $GITHUB_OUTPUT + exit 1 + + # 5. Create a release (Mock-Up workflow it will be fullfill by RHOAIENG-15391) + create-release: + needs: [update-release-images, check-pr-merged] + if: ${{ needs.check-pr-merged.outputs.pr_merged == 'true' && inputs.create-new-release == 'true' }} + uses: opendatahub-io/kubeflow/.github/workflows/create-release.yaml@main + with: + input_var: "Eyo" diff --git a/.github/workflows/notebook-controller-images-updater.yaml b/.github/workflows/notebook-controller-images-updater.yaml index 99b020e8fd0..42f7c4281f1 100644 --- a/.github/workflows/notebook-controller-images-updater.yaml +++ b/.github/workflows/notebook-controller-images-updater.yaml @@ -1,22 +1,42 @@ --- -# This is a gha updates automaticaly the notebook controller images. Can be run on demand before a new release -name: Update Notebook Controller Images With Latest Commit ID -on: # yamllint disable-line rule:truthy +# This workflow automatically updates the notebook controllers' image tags +name: Update Notebook Controllers' Image Tags +on: workflow_dispatch: inputs: branch-name: - description: "Provide name of the branch, ex: v1.9-branch" + description: "Provide name of the branch, used to commit changes" required: true - default: "v1.9-branch" + default: "main" organization: required: true - description: "Owner of origin notebooks repository used to open a PR" + description: "Owner of origin kubeflow repository" default: "opendatahub-io" + generate-pr: + description: "Create PR?" + required: true + default: "true" + workflow_call: + inputs: + branch-name: + description: "Provide name of the branch, used to commit changes" + required: true + type: string + organization: + description: "Owner of origin kubeflow repository" + required: true + type: string + generate-pr: + description: "Create PR?" + required: true + type: string + env: - REPO_OWNER: ${{ github.event.inputs.organization }} + REPO_OWNER: ${{ inputs.organization }} + TEMP_UPDATER_BRANCH: temp-${{ inputs.branch-name }}-${{ github.run_id }} + BRANCH_NAME: ${{ inputs.branch-name }} + GENERATE_PR: ${{ inputs.generate-pr }} REPO_NAME: kubeflow - TEMP_UPDATER_BRANCH: temp-${{ github.run_id }} - BRANCH_NAME: ${{ github.event.inputs.branch-name }} jobs: update-notebook-controller-images: @@ -26,6 +46,11 @@ jobs: pull-requests: write steps: + - name: Configure Git + run: | + git config --global user.email "github-actions[bot]@users.noreply.github.com" + git config --global user.name "GitHub Actions" + - name: Checkout branch uses: actions/checkout@v4 with: @@ -33,21 +58,17 @@ jobs: - name: Checkout new branch run: | - echo ${{ env.TEMP_UPDATER_BRANCH }} - git checkout -b ${{ env.TEMP_UPDATER_BRANCH }} - git push --set-upstream origin ${{ env.TEMP_UPDATER_BRANCH }} + echo ${{ env.TEMP_UPDATER_BRANCH }} + git fetch origin + git checkout -b ${{ env.TEMP_UPDATER_BRANCH }} origin/${{ env.BRANCH_NAME }} + git push --set-upstream origin ${{ env.TEMP_UPDATER_BRANCH }} - - name: Configure Git - run: | - git config --global user.email "github-actions[bot]@users.noreply.github.com" - git config --global user.name "GitHub Actions" - - - name: Retrive latest commit + - name: Retrieve latest commit id: commit-id shell: bash run: | PAYLOAD=$(curl --silent -H 'Accept: application/vnd.github.v4.raw' https://api.github.com/repos/$REPO_OWNER/$REPO_NAME/commits?sha=$BRANCH_NAME&per_page=1) - echo "COMMIT_ID=$(echo $PAYLOAD | jq -r '.[0].sha[0:7]')" >> ${GITHUB_OUTPUT} + echo "GIT_HASH=$(echo $PAYLOAD | jq -r '.[0].sha[0:7]')" >> ${GITHUB_OUTPUT} - name: Extract version from branch-name id: version @@ -57,7 +78,7 @@ jobs: else VERSION=$(echo "${{ env.BRANCH_NAME }}" | sed -E 's/^v([0-9]+\.[0-9]+)-.*/\1/') - # Check if VERSION is empty, then, assign the full branch name + # Check if VERSION is empty, then assign the full branch name if [[ -z "$VERSION" ]]; then VERSION="${{ env.BRANCH_NAME }}" fi @@ -68,12 +89,12 @@ jobs: - name: Update related files id: apply-changes run: | - COMMIT_ID=${{ steps.commit-id.outputs.COMMIT_ID }} + GIT_HASH=${{ steps.commit-id.outputs.GIT_HASH }} VERSION=${{ steps.version.outputs.VERSION }} - echo "Updating files in VERSION=${VERSION} with COMMIT_ID=${COMMIT_ID}" - sed -E "s/(odh-kf-notebook-controller-image=quay\.io\/opendatahub\/kubeflow-notebook-controller:)[^: -]+(-)[^ ]+/\1$VERSION\2$COMMIT_ID/" -i components/notebook-controller/config/overlays/openshift/params.env - sed -E "s/(odh-notebook-controller-image=quay\.io\/opendatahub\/odh-notebook-controller:)[^: -]+(-)[^ ]+/\1$VERSION\2$COMMIT_ID/" -i components/odh-notebook-controller/config/base/params.env - sed -E "s/(KF_TAG \?= )[^\-]+(-)[^ ]+/\1$VERSION\2$COMMIT_ID/" -i components/odh-notebook-controller/makefile-vars.mk + echo "Updating files in VERSION=${VERSION} with GIT_HASH=${GIT_HASH}" + sed -E "s/(odh-kf-notebook-controller-image=quay\.io\/opendatahub\/kubeflow-notebook-controller:)[^: -]+(-)[^ ]+/\1$VERSION\2$GIT_HASH/" -i components/notebook-controller/config/overlays/openshift/params.env + sed -E "s/(odh-notebook-controller-image=quay\.io\/opendatahub\/odh-notebook-controller:)[^: -]+(-)[^ ]+/\1$VERSION\2$GIT_HASH/" -i components/odh-notebook-controller/config/base/params.env + sed -E "s/(KF_TAG \?= )[^\-]+(-)[^ ]+/\1$VERSION\2$GIT_HASH/" -i components/odh-notebook-controller/makefile-vars.mk git status if [[ $(git status --porcelain | wc -l) -gt 0 ]]; then @@ -83,14 +104,14 @@ jobs: git add components/notebook-controller/config/overlays/openshift/params.env git add components/odh-notebook-controller/config/base/params.env git add components/odh-notebook-controller/makefile-vars.mk - git commit -m ":robot: Update odh and notebook-controller with image ${VERSION}-${COMMIT_ID}" + git commit -m "Update odh and notebook-controller with image ${VERSION}-${GIT_HASH}" git push origin ${{ env.TEMP_UPDATER_BRANCH }} - git log --oneline else echo "There were no changes detected on ${{ env.BRANCH_NAME }}" fi - - - name: Create Pull Request + + - name: PR Cretation + if: ${{ env.GENERATE_PR == 'true' }} run: | gh pr create --repo https://github.com/$REPO_OWNER/$REPO_NAME.git \ --title "$pr_title" \ @@ -99,7 +120,7 @@ jobs: --base ${{ env.BRANCH_NAME }} env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - pr_title: "[GHA] Update odh and notebook-controller with image ${{ steps.version.outputs.VERSION }}-${{ steps.commit-id.outputs.COMMIT_ID }}" + pr_title: "[GHA-${{ github.run_id }}] Update odh and notebook-controller with image ${{ steps.version.outputs.VERSION }}-${{ steps.commit-id.outputs.GIT_HASH }}" pr_body: | :robot: This is an automated Pull Request created by `/.github/workflows/notebook-controller-images-updater.yaml`. @@ -108,3 +129,10 @@ jobs: - components/odh-notebook-controller/config/base/params.env - components/odh-notebook-controller/makefile-vars.mk + - name: Auto Merge Changes to Target Branch + if: ${{ env.GENERATE_PR != 'true' }} + run: | + git fetch origin + git checkout ${{ env.BRANCH_NAME }} + git merge --no-ff ${{ env.TEMP_UPDATER_BRANCH }} -m ":robot: Update odh and notebook-controller with image ${VERSION}-${GIT_HASH} auto-merged changes from ${{ env.TEMP_UPDATER_BRANCH }}" + git push origin ${{ env.BRANCH_NAME }} diff --git a/.github/workflows/sync-branches-through-pr.yaml b/.github/workflows/sync-branches-through-pr.yaml deleted file mode 100644 index 6923981dc88..00000000000 --- a/.github/workflows/sync-branches-through-pr.yaml +++ /dev/null @@ -1,80 +0,0 @@ ---- -name: Sync Branches - -on: - workflow_dispatch: - inputs: - source: - description: "From:" - required: true - type: string - target: - description: "To:" - required: true - type: string - -jobs: - sync-branches: - runs-on: ubuntu-latest - - steps: - - name: Checkout repository - uses: actions/checkout@v4 - with: - fetch-depth: 0 # Ensure full history is fetched for merging - - - name: Set up Git - run: | - git config user.name "github-actions[bot]" - git config user.email "github-actions[bot]@users.noreply.github.com" - - - name: Merge source branch into target with conflict resolution - id: merge - run: | - set -e - SOURCE_BRANCH="${{ github.event.inputs.source }}" - TARGET_BRANCH="${{ github.event.inputs.target }}" - - git fetch origin ${TARGET_BRANCH}:${TARGET_BRANCH} - git checkout ${TARGET_BRANCH} - - git fetch origin ${SOURCE_BRANCH}:${SOURCE_BRANCH} - git merge --no-commit origin/${SOURCE_BRANCH} || true - - # Resolve conflicts for specific files - FILES=( - "components/notebook-controller/config/overlays/openshift/params.env" - "components/odh-notebook-controller/config/base/params.env" - "components/odh-notebook-controller/makefile-vars.mk" - ) - - for FILE in "${FILES[@]}"; do - if [[ -f "$FILE" && "$(git status --porcelain=v1 2>/dev/null | grep -c "$FILE")" -gt 0 ]]; then - echo "Resolving conflict for $FILE by keeping target branch version." - git checkout --ours "$FILE" - git add "$FILE" - fi - done - - # Commit the merge changes - git commit -m "Merge ${SOURCE_BRANCH} into ${TARGET_BRANCH} with resolved conflicts" || echo "Nothing to commit" - - # Create a new branch for the sync - TIMESTAMP=$(date +'%Y%m%d%H%M%S') - SYNC_BRANCH="sync__${SOURCE_BRANCH}__${TARGET_BRANCH}__${TIMESTAMP}" - git checkout -b $SYNC_BRANCH - git push origin $SYNC_BRANCH - echo "branch=$SYNC_BRANCH" >> $GITHUB_OUTPUT - - - name: Create a Pull Request - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: | - SOURCE_BRANCH="${{ github.event.inputs.source }}" - TARGET_BRANCH="${{ github.event.inputs.target }}" - SYNC_BRANCH=$(echo "${{ steps.merge.outputs.branch }}") - gh pr create \ - --title "Sync ${SOURCE_BRANCH} into ${TARGET_BRANCH}" \ - --body ":robot: This is an automated PR" \ - --base "${{ github.event.inputs.target }}" \ - --head "${SYNC_BRANCH}" diff --git a/.github/workflows/sync-branches.yaml b/.github/workflows/sync-branches.yaml new file mode 100644 index 00000000000..c925ef21ae5 --- /dev/null +++ b/.github/workflows/sync-branches.yaml @@ -0,0 +1,88 @@ +name: Sync Branches + +on: + workflow_dispatch: + inputs: + source: + description: "From:" + required: true + type: string + target: + description: "To:" + required: true + type: string + workflow_call: + inputs: + source: + description: "From:" + required: true + type: string + target: + description: "To:" + required: true + type: string +env: + SOURCE_BRANCH: ${{ inputs.source }} + TARGET_BRANCH: ${{ inputs.target }} + +jobs: + sync-branches: + runs-on: ubuntu-latest + + steps: + - name: Set up Git + run: | + git config user.name "github-actions[bot]" + git config user.email "github-actions[bot]@users.noreply.github.com" + + - name: Checkout repository + uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Merge source branch into target + run: | + set -e + + # Fetch and checkout target branch + git fetch origin ${{ env.TARGET_BRANCH }}:${{ env.TARGET_BRANCH }} + git checkout ${{ env.TARGET_BRANCH }} + + # Fetch and merge source branch + git fetch origin ${{ env.SOURCE_BRANCH }}:${{ env.SOURCE_BRANCH }} + git merge --no-commit origin/${{ env.SOURCE_BRANCH }} || true + + # Known files to resolve the conflicts + FILES=( + "components/notebook-controller/config/overlays/openshift/params.env" + "components/odh-notebook-controller/config/base/params.env" + "components/odh-notebook-controller/makefile-vars.mk" + ) + + # Resolve conflicts for known files + for FILE in "${FILES[@]}"; do + if [[ -f "$FILE" && "$(git status --porcelain=v1 2>/dev/null | grep -c "$FILE")" -gt 0 ]]; then + echo "Resolving conflict for $FILE by keeping target branch version." + git checkout --ours "$FILE" + git add "$FILE" + fi + done + + # Check for potential unresolved conflicts + if [[ -n "$(git ls-files -u)" ]]; then + echo "Unresolved conflicts detected in the following files:" + git ls-files -u + echo "Aborting merge due to unexpected conflicts." + exit 1 + fi + + # Commit changes if any + if [[ -n "$(git status --porcelain)" ]]; then + git commit -m "Merge ${{ env.SOURCE_BRANCH }} into ${{ env.TARGET_BRANCH }} with known resolved conflicts" + else + echo "No changes to commit. Skipping push." + exit 0 + fi + + # Push changes directly to target branch + git push origin ${{ env.TARGET_BRANCH }} From 0eefd59ca280d9c0248f37a9f5f06e770af98af6 Mon Sep 17 00:00:00 2001 From: atheo89 Date: Thu, 28 Nov 2024 16:30:56 +0100 Subject: [PATCH 40/50] Change gitconfig to use --global option --- .github/workflows/notebook-controller-images-updater.yaml | 2 +- .github/workflows/sync-branches.yaml | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/notebook-controller-images-updater.yaml b/.github/workflows/notebook-controller-images-updater.yaml index 42f7c4281f1..eb3c6d6d1a9 100644 --- a/.github/workflows/notebook-controller-images-updater.yaml +++ b/.github/workflows/notebook-controller-images-updater.yaml @@ -50,7 +50,7 @@ jobs: run: | git config --global user.email "github-actions[bot]@users.noreply.github.com" git config --global user.name "GitHub Actions" - + - name: Checkout branch uses: actions/checkout@v4 with: diff --git a/.github/workflows/sync-branches.yaml b/.github/workflows/sync-branches.yaml index c925ef21ae5..80bd5e1e52a 100644 --- a/.github/workflows/sync-branches.yaml +++ b/.github/workflows/sync-branches.yaml @@ -32,8 +32,8 @@ jobs: steps: - name: Set up Git run: | - git config user.name "github-actions[bot]" - git config user.email "github-actions[bot]@users.noreply.github.com" + git config --global user.name "github-actions[bot]" + git config --global user.email "github-actions[bot]@users.noreply.github.com" - name: Checkout repository uses: actions/checkout@v4 From 4efd65f3296198aa10f9fb063b3d5a71b2e433ac Mon Sep 17 00:00:00 2001 From: atheo89 Date: Thu, 28 Nov 2024 18:04:32 +0100 Subject: [PATCH 41/50] Increase timeouts as they were to tight --- .github/workflows/kubeflow-release.yaml | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/.github/workflows/kubeflow-release.yaml b/.github/workflows/kubeflow-release.yaml index f94bc80798d..ee96258208d 100644 --- a/.github/workflows/kubeflow-release.yaml +++ b/.github/workflows/kubeflow-release.yaml @@ -45,8 +45,13 @@ jobs: "quay.io/opendatahub/kubeflow-notebook-controller:1.9-${GIT_HASH}" "quay.io/opendatahub/odh-notebook-controller:1.9-${GIT_HASH}" ) - # Poll for image readiness total timeout=15m - MAX_ATTEMPTS=10 + + # Sleep for 5 minutes before starting polling + echo "Sleeping for 5 minutes before starting polling..." + sleep 300 + + # Poll for image readiness total timeout=20m + MAX_ATTEMPTS=13 SLEEP_DURATION=90 for image in "${IMAGES[@]}"; do for (( i=1; i<=MAX_ATTEMPTS; i++ )); do @@ -105,9 +110,9 @@ jobs: exit 1 fi - # Polling loop to wait for the PR to be merged total timeout=1h - MAX_ATTEMPTS=10 - SLEEP_DURATION=360 + # Polling loop to wait for the PR to be merged total timeout=5h + MAX_ATTEMPTS=30 + SLEEP_DURATION=600 for (( i=1; i<=MAX_ATTEMPTS; i++ )); do echo "Checking if PR #$PR_NUMBER is merged (Attempt $i/$MAX_ATTEMPTS)..." From 9116a09c52f8a6ccc6ca2e2c9e0dc721de00ca59 Mon Sep 17 00:00:00 2001 From: atheo89 Date: Tue, 3 Dec 2024 11:52:15 +0100 Subject: [PATCH 42/50] Enable GHAs for v1.9-branch --- .github/workflows/code-quality.yaml | 3 +++ .github/workflows/notebook_controller_integration_test.yaml | 3 +++ .github/workflows/notebook_controller_unit_test.yaml | 3 +++ .../workflows/odh_notebook_controller_integration_test.yaml | 3 +++ .github/workflows/odh_notebook_controller_unit_test.yaml | 3 +++ 5 files changed, 15 insertions(+) diff --git a/.github/workflows/code-quality.yaml b/.github/workflows/code-quality.yaml index 04d672c8934..70e4569b4b0 100644 --- a/.github/workflows/code-quality.yaml +++ b/.github/workflows/code-quality.yaml @@ -3,6 +3,9 @@ name: Code static analysis "on": push: pull_request: + branches: + - main + - v1.9-branch workflow_dispatch: permissions: diff --git a/.github/workflows/notebook_controller_integration_test.yaml b/.github/workflows/notebook_controller_integration_test.yaml index 4482902ba6e..07999d87924 100644 --- a/.github/workflows/notebook_controller_integration_test.yaml +++ b/.github/workflows/notebook_controller_integration_test.yaml @@ -2,6 +2,9 @@ name: Notebook Controller Integration Test on: push: pull_request: + branches: + - main + - v1.9-branch paths: - components/notebook-controller/** workflow_dispatch: diff --git a/.github/workflows/notebook_controller_unit_test.yaml b/.github/workflows/notebook_controller_unit_test.yaml index 39b8e770b8d..91e7968cce6 100644 --- a/.github/workflows/notebook_controller_unit_test.yaml +++ b/.github/workflows/notebook_controller_unit_test.yaml @@ -2,6 +2,9 @@ name: Run Notebook Controller unit tests on: push: pull_request: + branches: + - main + - v1.9-branch paths: - components/notebook-controller/** workflow_dispatch: diff --git a/.github/workflows/odh_notebook_controller_integration_test.yaml b/.github/workflows/odh_notebook_controller_integration_test.yaml index 3d451d0a33e..b7f58adcc79 100644 --- a/.github/workflows/odh_notebook_controller_integration_test.yaml +++ b/.github/workflows/odh_notebook_controller_integration_test.yaml @@ -2,6 +2,9 @@ name: ODH Notebook Controller Integration Test on: push: pull_request: + branches: + - main + - v1.9-branch paths: - .github/workflows/odh_notebook_controller_integration_test.yaml - components/notebook-controller/** diff --git a/.github/workflows/odh_notebook_controller_unit_test.yaml b/.github/workflows/odh_notebook_controller_unit_test.yaml index a8bbf5a5fe5..ec7f647528a 100644 --- a/.github/workflows/odh_notebook_controller_unit_test.yaml +++ b/.github/workflows/odh_notebook_controller_unit_test.yaml @@ -2,6 +2,9 @@ name: Run ODH Notebook Controller unit tests on: push: pull_request: + branches: + - main + - v1.9-branch paths: - components/odh-notebook-controller/** workflow_dispatch: From bf047382c5085b27185ea6bb2d6566b3e9101621 Mon Sep 17 00:00:00 2001 From: Andy Stoneberg Date: Tue, 3 Dec 2024 14:17:15 -0500 Subject: [PATCH 43/50] feat(automation): Add GitHub Action to handle creating a new release This commit replaces the "placeholder" create-release.yaml file to include functionality to actually create a opendatahub-io/kubeflow release following the original manual process. The GHA can be invoked manually as a workflow from GitHub as well as invoked as part of the kubeflow-release action. `kubeflow-release.yaml` has been modified to pass in the target branch name for cutting the release. The release/tag name is **NOT** provided as the `create-release.sh` script will auto-compute what the proper tag name should be. - Please note that the tag name **can** be provided if necessary in the future... at which point the tag name would be honored as is (i.e. no "auto-incrementing" logic is performed if the tag name provided as input) `create-release.sh` will also handle auto-updating the release tag for future scenarios where we change the target branch (ex: `v1.9-branch` to `v2.0-branch`). Please see the (extensive) comments in `.github/scripts/create-release.sh` for deeper explanation on implementation. - Please note, in what may be a "controversial" decision, I have extracted the script into its own file as opposed to embedding it within the `yaml` of the action. I find it more pleasant to read/develop when the script is in its own file - and also allows code editors to be smarter in offering hints on the source. Related-to: https://issues.redhat.com/browse/RHOAIENG-15391 --- .github/scripts/create-release.sh | 194 ++++++++++++++++++++++++ .github/workflows/create-release.yaml | 44 ++++-- .github/workflows/kubeflow-release.yaml | 4 +- 3 files changed, 228 insertions(+), 14 deletions(-) create mode 100755 .github/scripts/create-release.sh diff --git a/.github/scripts/create-release.sh b/.github/scripts/create-release.sh new file mode 100755 index 00000000000..e8e6db0eb12 --- /dev/null +++ b/.github/scripts/create-release.sh @@ -0,0 +1,194 @@ +#! /usr/bin/env bash + +## Description: +## +## This script automates the process involved in creating a new release for the opendatahub-io/kubeflow repository as +## documented in https://issues.redhat.com/browse/RHOAIENG-15391. +## +## Usage: +## +## create-release.sh +## - Intended to be invoked as part of a GitHub Action workflow. +## - Accepts no arguments. Information is passed to the script in the form of environment variables (see below) +## +## +## Required Environment Variables: +## +## TARGET_BRANCH [Optional] Existing branch on the repository used to construct the release. If unset, the branch with most recent commit will be used. +## RELEASE_TAG [Optional] Tag to create that identifies the release. If unset, latest existing tag will be incremented. +## GITHUB_REPOSITORY [Required] Repository to target for the release. Set automatically as part of GitHub Actions execution. +## + +set -euo pipefail + +# Description: +# Computes the branch to use when creating the release. If no argument provided for $1, the remote branch matching the naming convention of */v* that has the most +# recent commit will be used. +# git branch -r returns branches in the form of / +# examples: +# origin/v1.9-branch : match +# origin/main : no match +# +# Arguments: +# $1 : Release branch to use if provided. +# +# Returns: +# Name of branch to use when creating the release +_get_release_branch() +{ + local release_branch="${1:-}" + + if [ -z "${release_branch}" ]; then + local raw_branch=$(git branch -r --sort=-committerdate --list "*/v*" | head -n 1) + local trimmed_branch="${raw_branch#${raw_branch%%[![:space:]]*}}" + release_branch="${trimmed_branch#origin/}" + fi + + printf "%s" "${release_branch}" +} + +# Description: +# Retrieves the tag used for the most recent published release. Draft and Pre-Release releases are excluded. +# +# gh release list, by default, sorts releases based on the date of the most recent commit. To ensure the most recent published release is returned, a jq filter +# is used on the results to enforce ordering by publish time. Under most circumstances, this ordering should be identical. +# +# Returns: +# Name of tag used for the most recent published release +_get_latest_release_tag() +{ + gh release list \ + --repo "$GITHUB_REPOSITORY" \ + --exclude-drafts \ + --exclude-pre-releases \ + --json tagName,publishedAt \ + --jq 'sort_by(.publishedAt) | reverse | .[0].tagName' +} + +# Description: +# Determines if the most recent release tag is related to the given release branch. A release branch is "related" to the most recent release tag if the tag name starts +# with the branch prefix. +# +# This determination is critical in being able to properly auto-increment the release name. See comments on _get_target_release_json() for further details. +# +# Arguments: +# $1 : Branch prefix that is being used to create the new release. kubeflow names branches like 'v1.9-branch'. The branch prefix would then be expected to be 'v1.9' +# $2 : Tag name corresponding to the most recent published release +# +# Returns: +# 0 if the release branch name is aligned with the latest release tag +# 1 otherwise +_same_branch_as_prior_release() +{ + local release_branch_prefix="${1:-}" + local latest_release_tag="${2:-}" + + case "${latest_release_tag}" in + "${release_branch_prefix}"*) + true + ;; + *) + false + ;; + esac +} + +# Description: +# Determines the name of the release (which is also the tag name) to identify the to-be-created release. Additionally returns the tag name of the most recent +# published release to use in automated notes generation. +# As both these data points can be interdependent and require querying for the latest published release, the computation is combined in this single function +# +# Release names are expected to be in the form: v{major}.{minor}.{patch}-{release} +# +# If a release name is provided in the arguments, it is used as-is without any further computation. However, if a release name is not provided, this function will +# analyze the state of the release branch as well as the most recent published release tag, to determine the appropriate auto-incrementing strategy. Consider the +# following scenarios: +# _get_target_release_json "v1.9-branch" "" +# In this case, the most recent published release is retrieved, and, based on _same_branch_as_prior_release(): +# - if the most recent published release is associated with the release branch, the {release} segment of the name is incremented by 1 (ex: v1.9.0-5 to v1.9.0-6) +# - if the most recent published release is not associated with the release branch, release name is "reset" based on the branch (ex: v1.9.0-1) +# +# _get_target_release_json "v1.9-branch" "v1.9.0-5" +# In this case, the release name is simply the provided 'v1.9.0-5' argument. If this release already exists, the script will subsequently error. +# +# Generally speaking, it should not be necessary to provide the $2 argument unless under extraordinary circumstances. +# +# Arguments: +# $1 : Name of the branch being used to create the new release. +# $2 : Name of the release (and related tag) +# +# Returns: +# Stringified JSON of the form: '{notes_start_tag: , release_name: }' +_get_target_release_json() +{ + local release_branch="${1:-}" + local release_name="${2:-}" + + local latest_release_tag=$(_get_latest_release_tag) + local notes_start_tag="${latest_release_tag}" + if [ -z "${release_name}" ]; then + local release_base="${release_branch%-branch}" + if ! _same_branch_as_prior_release "${release_base}" "${latest_release_tag}"; then + latest_release_tag="${release_base}.0-0" + + if [ -z "${latest_release_tag}" ]; then + notes_start_tag= + fi + fi + + tag_parts=($(printf "%s" "${latest_release_tag}" | tr '-' ' ')) + release_prefix="${tag_parts[0]}" + release_id="$(( ${tag_parts[1]} + 1 ))" + release_name="${release_prefix}-${release_id}" + fi + + jq -n --arg notes_start_tag "${notes_start_tag}" --arg release_name "${release_name}" '{notes_start_tag: $notes_start_tag, release_name: $release_name}' +} + +# Description: +# Invokes the GH CLI to create a release. Release notes are automatically generated. If the $3 argument is not provided, the --notes-start-tag option is not +# provided to the GH CLI invocation. +# +# Expects GITHUB_REPOSITORY to be defined as an environment variable in the shell session. +# +# Arguments: +# $1 : Name of the branch being used to create the new release. +# $2 : Name of the release (and related tag) +# $3 : Name of the tag to use, if provided, for the --notes-start-tag parameter of the 'gh release create' command +_create_release() +{ + local release_branch="${1:-}" + local release_name="${2:-}" + local notes_start_tag="${3:-}" + + gh release create "${release_name}" \ + --repo "$GITHUB_REPOSITORY" \ + --title "${release_name}" \ + --target "${release_branch}" \ + --generate-notes \ + ${notes_start_tag:+ --notes-start-tag ${notes_start_tag}} +} + +# Description: +# Orchestration logic that accomplishes the intent of the script. Diagnostic messages are also output to aid in understanding the outcome. +# +# Will honor TARGET_BRANCH and RELEASE_TAG environment variables if defined in the shell session. +main() +{ + release_branch=$( _get_release_branch "${TARGET_BRANCH}" ) + + echo "Using branch '${release_branch}'" + + target_release_json=$( _get_target_release_json "${release_branch}" "${RELEASE_TAG}" ) + release_name=$( jq -r '.release_name' <<< "${target_release_json}" ) + notes_start_tag=$( jq -r '.notes_start_tag' <<< "${target_release_json}" ) + + echo "Using release name '${release_name}' ${notes_start_tag:+with a start tag of '${notes_start_tag}' for notes generation}" + + _create_release "${release_branch}" "${release_name}" "${notes_start_tag}" +} + + +main + + diff --git a/.github/workflows/create-release.yaml b/.github/workflows/create-release.yaml index b6e674df046..78a0b9a8751 100644 --- a/.github/workflows/create-release.yaml +++ b/.github/workflows/create-release.yaml @@ -1,24 +1,44 @@ --- -name: Create Release +name: Create release + on: workflow_dispatch: inputs: - input_var: - description: "Say Hi!" - required: true + release_tag: + description: "Release Tag:" + type: string + target_branch: + description: "Target Branch:" + type: string workflow_call: inputs: - input_var: + release_tag: + description: "Release Tag:" type: string - required: true + target_branch: + description: "Target Branch:" + type: string env: - INPUT_VAR: ${{ inputs.input_var }} + RELEASE_TAG: ${{ inputs.release_tag }} + TARGET_BRANCH: ${{ inputs.target_branch }} -jobs: - say-hello: - runs-on: ubuntu-latest +permissions: + contents: write +jobs: + release: + name: Create opendatahub-io/kubeflow release + runs-on: ubuntu-22.04 steps: - - name: Say Hello - run: echo "${{ env.INPUT_VAR }}, World!" + - name: Check out the repository to the runner + uses: actions/checkout@v4 + with: + fetch-depth: 0 + - name: Invoke script to handle creating release + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + working-directory: ${{env.GITHUB_WORKSPACE}} + run: | + ./.github/scripts/create-release.sh + diff --git a/.github/workflows/kubeflow-release.yaml b/.github/workflows/kubeflow-release.yaml index ee96258208d..3d5ddf2ed66 100644 --- a/.github/workflows/kubeflow-release.yaml +++ b/.github/workflows/kubeflow-release.yaml @@ -134,10 +134,10 @@ jobs: echo "pr_merged=false" >> $GITHUB_OUTPUT exit 1 - # 5. Create a release (Mock-Up workflow it will be fullfill by RHOAIENG-15391) + # 5. Create a release create-release: needs: [update-release-images, check-pr-merged] if: ${{ needs.check-pr-merged.outputs.pr_merged == 'true' && inputs.create-new-release == 'true' }} uses: opendatahub-io/kubeflow/.github/workflows/create-release.yaml@main with: - input_var: "Eyo" + target_branch: "v1.9-branch" From 8dc9aed80fafb861c2dda4a35620e46832df599f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jiri=20Dan=C4=9Bk?= Date: Thu, 5 Dec 2024 10:30:49 +0100 Subject: [PATCH 44/50] RHOAIENG-7610: fix(nbc): deduplicate culling-related configmap keys (#480) --- components/notebook-controller/config/manager/params.env | 3 --- 1 file changed, 3 deletions(-) diff --git a/components/notebook-controller/config/manager/params.env b/components/notebook-controller/config/manager/params.env index 232af1d5685..cf6f8d1e30f 100644 --- a/components/notebook-controller/config/manager/params.env +++ b/components/notebook-controller/config/manager/params.env @@ -2,6 +2,3 @@ USE_ISTIO=true ISTIO_GATEWAY=kubeflow/kubeflow-gateway ISTIO_HOST=* CLUSTER_DOMAIN=cluster.local -ENABLE_CULLING=false -CULL_IDLE_TIME=1440 -IDLENESS_CHECK_PERIOD=1 From b2ebcd7fadc8deb9ef50e87f78425cf7fa9801b0 Mon Sep 17 00:00:00 2001 From: atheo89 Date: Mon, 9 Dec 2024 11:05:59 +0100 Subject: [PATCH 45/50] Add a dropdown to choose Sync or Release action --- .github/workflows/kubeflow-release.yaml | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/.github/workflows/kubeflow-release.yaml b/.github/workflows/kubeflow-release.yaml index 3d5ddf2ed66..10538d6c0bf 100644 --- a/.github/workflows/kubeflow-release.yaml +++ b/.github/workflows/kubeflow-release.yaml @@ -3,12 +3,16 @@ name: Kubeflow Release Pipeline on: workflow_dispatch: inputs: - create-new-release: - description: "Create a new release?" + release-type: + description: "Select the type of action to perform" required: true - default: "true" + default: "Sync" + type: choice + options: + - Sync + - Release env: - CREATE_NEW_RELEASE: ${{ inputs.create-new-release }} + CREATE_NEW_RELEASE: ${{ inputs.release-type == 'Release' }} REPO_OWNER: opendatahub-io REPO_NAME: kubeflow BRANCH_NAME: v1.9-branch @@ -134,10 +138,10 @@ jobs: echo "pr_merged=false" >> $GITHUB_OUTPUT exit 1 - # 5. Create a release + # 5. Create a release create-release: needs: [update-release-images, check-pr-merged] - if: ${{ needs.check-pr-merged.outputs.pr_merged == 'true' && inputs.create-new-release == 'true' }} + if: ${{ needs.check-pr-merged.outputs.pr_merged == 'true' && inputs.release-type == 'Release' }} uses: opendatahub-io/kubeflow/.github/workflows/create-release.yaml@main with: target_branch: "v1.9-branch" From adcb3a8b3f5438edce0dfbe676c0ac299dc9e44b Mon Sep 17 00:00:00 2001 From: aTheo Date: Tue, 10 Dec 2024 11:08:42 +0100 Subject: [PATCH 46/50] Set secret token to enable GHA tests to run when the automated PR opens (#489) --- .github/workflows/kubeflow-release.yaml | 2 ++ .../notebook-controller-images-updater.yaml | 16 +++++++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/.github/workflows/kubeflow-release.yaml b/.github/workflows/kubeflow-release.yaml index 3d5ddf2ed66..10b1849c3d8 100644 --- a/.github/workflows/kubeflow-release.yaml +++ b/.github/workflows/kubeflow-release.yaml @@ -83,6 +83,8 @@ jobs: branch-name: "v1.9-branch" organization: "opendatahub-io" generate-pr: "true" + secrets: + GH_TOKEN: ${{ secrets.GHA_SECRET_PAT }} # 4. Check PR merged status check-pr-merged: diff --git a/.github/workflows/notebook-controller-images-updater.yaml b/.github/workflows/notebook-controller-images-updater.yaml index eb3c6d6d1a9..0ebe160f74a 100644 --- a/.github/workflows/notebook-controller-images-updater.yaml +++ b/.github/workflows/notebook-controller-images-updater.yaml @@ -17,6 +17,10 @@ on: required: true default: "true" workflow_call: + secrets: + GH_TOKEN: + description: 'A token passed from the caller workflow' + required: true inputs: branch-name: description: "Provide name of the branch, used to commit changes" @@ -37,6 +41,7 @@ env: BRANCH_NAME: ${{ inputs.branch-name }} GENERATE_PR: ${{ inputs.generate-pr }} REPO_NAME: kubeflow + GH_TOKEN: ${{ secrets.GH_TOKEN }} jobs: update-notebook-controller-images: @@ -51,6 +56,15 @@ jobs: git config --global user.email "github-actions[bot]@users.noreply.github.com" git config --global user.name "GitHub Actions" + # The GHA_SECRET_PAT has been generated by the ide-developer account. + # If a user runs this GHA on their repository and wants the GHA tests enabled, they should configure their personal GHA_SECRET_PAT in their fork. + - name: Set GH_TOKEN for standalone execution + id: set-gh-token + run: | + if [ -z "${{ env.GH_TOKEN }}" ]; then + echo "GH_TOKEN=${{ secrets.GHA_SECRET_PAT }}" >> $GITHUB_ENV + fi + - name: Checkout branch uses: actions/checkout@v4 with: @@ -119,7 +133,7 @@ jobs: --head $REPO_OWNER:${{ env.TEMP_UPDATER_BRANCH }} \ --base ${{ env.BRANCH_NAME }} env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GH_TOKEN: ${{ env.GH_TOKEN }} pr_title: "[GHA-${{ github.run_id }}] Update odh and notebook-controller with image ${{ steps.version.outputs.VERSION }}-${{ steps.commit-id.outputs.GIT_HASH }}" pr_body: | :robot: This is an automated Pull Request created by `/.github/workflows/notebook-controller-images-updater.yaml`. From 0ed590414e5754fc599a41c0e04109899f3875f8 Mon Sep 17 00:00:00 2001 From: Andy Stoneberg Date: Tue, 10 Dec 2024 07:12:25 -0500 Subject: [PATCH 47/50] chore(test): Integration Codecov coverage reporting for odh-notebook-controller (#492) This commit integrates Codecov into our testing process by publishing our coverage report generated during `make test` to Codecov via the official `codecov/codecov-action` GHA. The `CODECOV_TOKEN` was previously uploaded to our GH repository as a secret (and copied to the Dependabot Secrets as well) by a repo admin: - https://redhat-internal.slack.com/archives/C060A5FJEAD/p1733334721701649 Please be aware of functionality **NOT** included in this initial PR (that may be added in the future): - unit test results are not being shipped to `Codecov` (even though that capability is supported) - a `.codecov.yml` file is presently not included. this could come as a fast-follow after the team gets a "feel" for `Codecov` - but I think its a bit premature to take my personal opinions to create such a file. Example `odh-dashboard` config can be seen here: - https://github.com/opendatahub-io/odh-dashboard/blob/main/.codecov.yml The `Codecov` integration was added to the `odh_notebook_controller_unit_test.yaml` GHA - which is already configured to execute on `pull_request` and `push` triggers. As such, no additional modification was necessary. :warning: It should be noted that the `make test` target was modifed to ensure the coverage files for the 2 respective invocations of `go test` wrote to unique output files to prevent information loss. `*.out` files are already ignored via `.gitignore`, so no problems there. Also, Codecov claims to support report merging automatically - so this should cause no issues: - https://docs.codecov.com/docs/merging-reports Related-to: https://issues.redhat.com/browse/RHOAIENG-11142 --- .github/workflows/odh_notebook_controller_unit_test.yaml | 7 +++++++ components/odh-notebook-controller/Makefile | 4 ++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/.github/workflows/odh_notebook_controller_unit_test.yaml b/.github/workflows/odh_notebook_controller_unit_test.yaml index ec7f647528a..09affa2c596 100644 --- a/.github/workflows/odh_notebook_controller_unit_test.yaml +++ b/.github/workflows/odh_notebook_controller_unit_test.yaml @@ -27,3 +27,10 @@ jobs: run: | cd components/odh-notebook-controller make test + + - name: Upload results to Codecov + uses: codecov/codecov-action@v5 + with: + token: ${{ secrets.CODECOV_TOKEN }} + files: ./components/odh-notebook-controller/cover-rbac-false.out,./components/odh-notebook-controller/cover-rbac-true.out + disable_search: true \ No newline at end of file diff --git a/components/odh-notebook-controller/Makefile b/components/odh-notebook-controller/Makefile index 90de33e4e54..7ca6bb961fd 100644 --- a/components/odh-notebook-controller/Makefile +++ b/components/odh-notebook-controller/Makefile @@ -94,12 +94,12 @@ 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 + go test ./controllers/... -ginkgo.v -ginkgo.progress -test.v -coverprofile cover-rbac-false.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 + go test ./controllers/... -ginkgo.v -ginkgo.progress -test.v -coverprofile cover-rbac-true.out ##@ Build From 3cae697c20e555bb9aa1de3600e1ecd0aeea5200 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jiri=20Dan=C4=9Bk?= Date: Thu, 12 Dec 2024 10:47:44 +0100 Subject: [PATCH 48/50] NO-ISSUE: chore(gha): run tests in a gha when gha itself is modified in a pr (#491) --- .github/workflows/notebook_controller_integration_test.yaml | 1 + .github/workflows/notebook_controller_unit_test.yaml | 1 + .github/workflows/odh_notebook_controller_unit_test.yaml | 1 + 3 files changed, 3 insertions(+) diff --git a/.github/workflows/notebook_controller_integration_test.yaml b/.github/workflows/notebook_controller_integration_test.yaml index 07999d87924..94a38f14ec2 100644 --- a/.github/workflows/notebook_controller_integration_test.yaml +++ b/.github/workflows/notebook_controller_integration_test.yaml @@ -6,6 +6,7 @@ on: - main - v1.9-branch paths: + - .github/workflows/notebook_controller_integration_test.yaml - components/notebook-controller/** workflow_dispatch: diff --git a/.github/workflows/notebook_controller_unit_test.yaml b/.github/workflows/notebook_controller_unit_test.yaml index 91e7968cce6..6c21ea018f1 100644 --- a/.github/workflows/notebook_controller_unit_test.yaml +++ b/.github/workflows/notebook_controller_unit_test.yaml @@ -6,6 +6,7 @@ on: - main - v1.9-branch paths: + - .github/workflows/notebook_controller_unit_test.yaml - components/notebook-controller/** workflow_dispatch: diff --git a/.github/workflows/odh_notebook_controller_unit_test.yaml b/.github/workflows/odh_notebook_controller_unit_test.yaml index 09affa2c596..300bc3e41d7 100644 --- a/.github/workflows/odh_notebook_controller_unit_test.yaml +++ b/.github/workflows/odh_notebook_controller_unit_test.yaml @@ -6,6 +6,7 @@ on: - main - v1.9-branch paths: + - .github/workflows/odh_notebook_controller_unit_test.yaml - components/odh-notebook-controller/** workflow_dispatch: From 15706de9e30b4c1a3d383cacfd48b0048277b8b1 Mon Sep 17 00:00:00 2001 From: Andy Stoneberg Date: Fri, 13 Dec 2024 09:22:40 -0500 Subject: [PATCH 49/50] chore(owners): add myself as reviewer/approver (#495) Adding my own GH handle (`andyatmiami`) to the `approvers` and `reviewers` section of `OWNERS`, honor alphanumeric sorting, so I can be more self-sufficient in working issues alongside the team. --- OWNERS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/OWNERS b/OWNERS index 238c8a85a9b..47a5b8f0254 100644 --- a/OWNERS +++ b/OWNERS @@ -1,4 +1,5 @@ approvers: + - andyatmiami - atheo89 - caponetto - harshad16 @@ -7,6 +8,7 @@ approvers: - paulovmr reviewers: + - andyatmiami - atheo89 - caponetto - dibryant From 0ae60aa05510272785f84c9b37aba5ed805d94d9 Mon Sep 17 00:00:00 2001 From: Andy Stoneberg Date: Fri, 13 Dec 2024 10:34:26 -0500 Subject: [PATCH 50/50] chore(test): Integration Codecov coverage reporting for notebook-controller (#490) This commit integrates Codecov into our testing process by publishing our coverage report generated during `make test` to Codecov via the official `codecov/codecov-action` GHA. The `CODECOV_TOKEN` was previously uploaded to our GH repository as a secret (and copied to the Dependabot Secrets as well) by a repo admin: - https://redhat-internal.slack.com/archives/C060A5FJEAD/p1733334721701649 Please be aware of functionality **NOT** included in this initial PR (that may be added in the future): - unit test results are not being shipped to `Codecov` (even though that capability is supported) - a `.codecov.yml` file is presently not included. this could come as a fast-follow after the team gets a "feel" for `Codecov` - but I think its a bit premature to take my personal opinions to create such a file. Example `odh-dashboard` config can be seen here: - https://github.com/opendatahub-io/odh-dashboard/blob/main/.codecov.yml The `Codecov` integration was added to the `notebook_controller_unit_test.yaml` GHA - which is already configured to execute on `pull_request` and `push` triggers. As such, no additional modification was necessary. :warning: It should be noted that the `make test` target as configured in upstream is presently incorrect. The 2 different `go test` executions are redundant - and the 2nd invocation (targetting only the `./controller/...` scope, actually overwrites the `cover.out` file initially generated by the 1st invocation. However, the fix for this should come from upstream.. I will open up a PR against upstream at which point it will flow through to `odh` whenever its merged and we sync. Related-to: https://issues.redhat.com/browse/RHOAIENG-11142 --- .github/workflows/notebook_controller_unit_test.yaml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.github/workflows/notebook_controller_unit_test.yaml b/.github/workflows/notebook_controller_unit_test.yaml index 6c21ea018f1..db1ac8d06ba 100644 --- a/.github/workflows/notebook_controller_unit_test.yaml +++ b/.github/workflows/notebook_controller_unit_test.yaml @@ -28,3 +28,10 @@ jobs: run: | cd components/notebook-controller make test + + - name: Upload results to Codecov + uses: codecov/codecov-action@v5 + with: + token: ${{ secrets.CODECOV_TOKEN }} + files: ./components/notebook-controller/cover.out + disable_search: true