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 + 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/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/notebook-controller/controllers/suite_test.go b/components/notebook-controller/controllers/suite_test.go index 3093e1b5e97..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()) @@ -91,7 +92,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/Makefile b/components/odh-notebook-controller/Makefile index e92d704bf3b..90de33e4e54 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 @@ -132,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. @@ -151,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/README.md b/components/odh-notebook-controller/README.md index d170be23cd2..5320cbe7e0f 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. | +| 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 Install the `notebooks.kubeflow.org` CRD from the [Kubeflow notebook 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/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" diff --git a/components/odh-notebook-controller/controllers/notebook_controller_test.go b/components/odh-notebook-controller/controllers/notebook_controller_test.go index e9c9c6b0941..b3ed46bf3ef 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() { @@ -498,7 +498,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 { @@ -506,7 +506,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() { @@ -524,7 +524,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() { @@ -537,7 +538,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() { @@ -670,7 +672,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() { @@ -683,7 +685,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() { @@ -699,7 +701,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{} @@ -711,7 +713,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() { @@ -724,7 +727,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{} @@ -755,7 +759,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() { @@ -768,7 +772,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{} @@ -831,7 +835,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() { @@ -844,7 +848,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() { @@ -862,7 +866,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() { diff --git a/components/odh-notebook-controller/controllers/suite_test.go b/components/odh-notebook-controller/controllers/suite_test.go index 16efc780537..a9c93337db4 100644 --- a/components/odh-notebook-controller/controllers/suite_test.go +++ b/components/odh-notebook-controller/controllers/suite_test.go @@ -18,7 +18,9 @@ import ( "context" "crypto/tls" "fmt" + "k8s.io/utils/ptr" "net" + "os" "path/filepath" "testing" "time" @@ -55,11 +57,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{} ) @@ -74,7 +79,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{ @@ -83,10 +88,13 @@ 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{ + ControlPlane: envtest.ControlPlane{ + APIServer: &envtest.APIServer{}, + }, CRDInstallOptions: envtest.CRDInstallOptions{ Paths: []string{filepath.Join("..", "config", "crd", "external")}, ErrorIfPathMissing: true, @@ -97,11 +105,39 @@ 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") + } - cfg, err := envTest.Start() + var err error + cfg, err = envTest.Start() Expect(err).NotTo(HaveOccurred()) 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()) + 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)) @@ -110,7 +146,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()) @@ -126,6 +162,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()) @@ -156,6 +198,7 @@ var _ = BeforeSuite(func() { go func() { defer GinkgoRecover() err = mgr.Start(ctx) + managerStopped <- struct{}{} Expect(err).ToNot(HaveOccurred(), "Failed to run manager") }() @@ -172,7 +215,6 @@ var _ = BeforeSuite(func() { }).Should(Succeed()) // Verify kubernetes client is working - cli = mgr.GetClient() Expect(cli).ToNot(BeNil()) for _, namespace := range testNamespaces { @@ -187,7 +229,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() 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 diff --git a/components/odh-notebook-controller/makefile-vars.mk b/components/odh-notebook-controller/makefile-vars.mk new file mode 100644 index 00000000000..fcd1af1d6bc --- /dev/null +++ b/components/odh-notebook-controller/makefile-vars.mk @@ -0,0 +1 @@ +KF_TAG ?= main 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