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/code-quality.yaml b/.github/workflows/code-quality.yaml index 342f506e11e..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: @@ -87,5 +90,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 }} diff --git a/.github/workflows/create-release.yaml b/.github/workflows/create-release.yaml new file mode 100644 index 00000000000..78a0b9a8751 --- /dev/null +++ b/.github/workflows/create-release.yaml @@ -0,0 +1,44 @@ +--- +name: Create release + +on: + workflow_dispatch: + inputs: + release_tag: + description: "Release Tag:" + type: string + target_branch: + description: "Target Branch:" + type: string + workflow_call: + inputs: + release_tag: + description: "Release Tag:" + type: string + target_branch: + description: "Target Branch:" + type: string + +env: + RELEASE_TAG: ${{ inputs.release_tag }} + TARGET_BRANCH: ${{ inputs.target_branch }} + +permissions: + contents: write + +jobs: + release: + name: Create opendatahub-io/kubeflow release + runs-on: ubuntu-22.04 + steps: + - 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 new file mode 100644 index 00000000000..59f738f771e --- /dev/null +++ b/.github/workflows/kubeflow-release.yaml @@ -0,0 +1,149 @@ +--- +name: Kubeflow Release Pipeline +on: + workflow_dispatch: + inputs: + release-type: + description: "Select the type of action to perform" + required: true + default: "Sync" + type: choice + options: + - Sync + - Release +env: + CREATE_NEW_RELEASE: ${{ inputs.release-type == '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}" + ) + + # 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 + 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" + secrets: + GH_TOKEN: ${{ secrets.GHA_SECRET_PAT }} + + # 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=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)..." + 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 + create-release: + needs: [update-release-images, check-pr-merged] + 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" diff --git a/.github/workflows/notebook-controller-images-updater.yaml b/.github/workflows/notebook-controller-images-updater.yaml index 3811956a764..0ebe160f74a 100644 --- a/.github/workflows/notebook-controller-images-updater.yaml +++ b/.github/workflows/notebook-controller-images-updater.yaml @@ -1,22 +1,47 @@ --- -# 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: + 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" + 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 }} + GH_TOKEN: ${{ secrets.GH_TOKEN }} jobs: update-notebook-controller-images: @@ -26,6 +51,20 @@ 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" + + # 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: @@ -33,21 +72,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 }} - - - name: Configure Git - run: | - git config --global user.email "github-actions[bot]@users.noreply.github.com" - git config --global user.name "GitHub Actions" + 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: 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 +92,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 +103,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 + 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 @@ -82,15 +117,15 @@ 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 "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" \ @@ -98,12 +133,20 @@ jobs: --head $REPO_OWNER:${{ env.TEMP_UPDATER_BRANCH }} \ --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 }}" + 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`. 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 + + - 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/notebook_controller_integration_test.yaml b/.github/workflows/notebook_controller_integration_test.yaml index dd8caffe5fa..94a38f14ec2 100644 --- a/.github/workflows/notebook_controller_integration_test.yaml +++ b/.github/workflows/notebook_controller_integration_test.yaml @@ -2,7 +2,11 @@ name: Notebook Controller Integration Test on: push: pull_request: + branches: + - main + - v1.9-branch paths: + - .github/workflows/notebook_controller_integration_test.yaml - components/notebook-controller/** workflow_dispatch: @@ -89,7 +93,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 <> $GITHUB_OUTPUT - - - name: Create pull request - uses: peter-evans/create-pull-request@c5a7806660adbe173f04e3e038b0ccdcd758773c # v6.1.0 - 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. diff --git a/.github/workflows/sync-branches.yaml b/.github/workflows/sync-branches.yaml new file mode 100644 index 00000000000..80bd5e1e52a --- /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 --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 + 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 }} 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 diff --git a/components/common/reconcilehelper/util.go b/components/common/reconcilehelper/util.go index ded19b08ab8..da2d2e1ab25 100644 --- a/components/common/reconcilehelper/util.go +++ b/components/common/reconcilehelper/util.go @@ -16,6 +16,7 @@ import ( // Deployment reconciles a k8s deployment object. func Deployment(ctx context.Context, r client.Client, deployment *appsv1.Deployment, log logr.Logger) error { + print("reconcilehelper/util.go") foundDeployment := &appsv1.Deployment{} justCreated := false if err := r.Get(ctx, types.NamespacedName{Name: deployment.Name, Namespace: deployment.Namespace}, foundDeployment); err != nil { 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/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/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/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 diff --git a/components/notebook-controller/config/overlays/openshift/params.env b/components/notebook-controller/config/overlays/openshift/params.env index d6f49dd6c9a..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:1.9-1769b4b +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/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 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/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 diff --git a/components/odh-notebook-controller/Makefile b/components/odh-notebook-controller/Makefile index 7fabc6e1107..7ca6bb961fd 100644 --- a/components/odh-notebook-controller/Makefile +++ b/components/odh-notebook-controller/Makefile @@ -1,15 +1,17 @@ +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 ?= 1.9-1769b4b +KF_TAG ?= $(KF_TAG) 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 @@ -86,11 +88,18 @@ 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-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 @@ -125,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. @@ -144,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 @@ -234,7 +238,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 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 2c5e256891d..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:1.9-1769b4b +odh-notebook-controller-image=quay.io/opendatahub/odh-notebook-controller:main 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 8e83dd2c273..e8b77ace91f 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" @@ -54,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 @@ -67,6 +71,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 +190,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) { @@ -269,6 +284,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" @@ -304,7 +322,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"))), }, } @@ -445,6 +463,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 87369dd78ef..2908b7fae9b 100644 --- a/components/odh-notebook-controller/controllers/notebook_controller_test.go +++ b/components/odh-notebook-controller/controllers/notebook_controller_test.go @@ -17,19 +17,22 @@ package controllers import ( "context" - "io/ioutil" - "strings" + "crypto/x509" + "encoding/pem" + "fmt" + "os" "time" "github.com/go-logr/logr" - "github.com/onsi/gomega/format" - netv1 "k8s.io/api/networking/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" @@ -97,7 +100,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() { @@ -115,7 +118,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() { @@ -128,7 +131,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() { @@ -161,6 +164,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 ( @@ -181,9 +256,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 @@ -230,6 +307,12 @@ var _ = Describe("The Openshift Notebook controller", func() { } // Check if the volume is present and matches the expected one Expect(notebook.Spec.Template.Spec.Volumes).To(ContainElement(expectedVolume)) + + // Check the content in workbench-trusted-ca-bundle matches what we expect: + // - have 2 certificates there in ca-bundle.crt + // - both certificates are valid + configMapName := "workbench-trusted-ca-bundle" + checkCertConfigMap(ctx, notebook.Namespace, configMapName, "ca-bundle.crt", 2) }) }) @@ -280,8 +363,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()) @@ -329,6 +412,12 @@ var _ = Describe("The Openshift Notebook controller", func() { }, } Expect(notebook.Spec.Template.Spec.Volumes).To(ContainElement(expectedVolume)) + + // Check the content in workbench-trusted-ca-bundle matches what we expect: + // - have 2 certificates there in ca-bundle.crt + // - both certificates are valid + configMapName := "workbench-trusted-ca-bundle" + checkCertConfigMap(ctx, notebook.Namespace, configMapName, "ca-bundle.crt", 2) }) }) @@ -341,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{ @@ -371,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, @@ -406,7 +490,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 { @@ -414,7 +498,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() { @@ -432,7 +516,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() { @@ -445,7 +530,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() { @@ -578,7 +664,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() { @@ -591,7 +677,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() { @@ -607,7 +693,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{} @@ -619,7 +705,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() { @@ -632,7 +719,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{} @@ -663,7 +751,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() { @@ -676,7 +764,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{} @@ -739,7 +827,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() { @@ -752,7 +840,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() { @@ -770,7 +858,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() { @@ -1039,3 +1127,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) +} 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_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 +} diff --git a/components/odh-notebook-controller/controllers/notebook_webhook.go b/components/odh-notebook-controller/controllers/notebook_webhook.go index bd91ae05470..5b0db331f81 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" @@ -48,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 @@ -232,6 +235,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{} @@ -252,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) } @@ -275,8 +279,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(ctx, req, notebook) + if err != nil { + return admission.Errored(http.StatusInternalServerError, err) + } + updatePendingAnnotation := "notebooks.opendatahub.io/update-pending" + if needsRestart != NoPendingUpdates { + mutatedNotebook.ObjectMeta.Annotations[updatePendingAnnotation] = needsRestart.Reason + } else { + delete(mutatedNotebook.ObjectMeta.Annotations, updatePendingAnnotation) + } + // 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 +307,68 @@ 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(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 { + 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") { + 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") { + 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 { + 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 { + 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) { + 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) { + 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, &UpdatesPending{Reason: diff}, 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 { @@ -459,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 { @@ -498,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/notebook_webhook_utils.go b/components/odh-notebook-controller/controllers/notebook_webhook_utils.go new file mode 100644 index 00000000000..e70657ccda5 --- /dev/null +++ b/components/odh-notebook-controller/controllers/notebook_webhook_utils.go @@ -0,0 +1,80 @@ +/* + +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 go-cmp 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 +} + +// 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) + + // 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/controllers/suite_test.go b/components/odh-notebook-controller/controllers/suite_test.go index 16efc780537..1cdc8ce65a4 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,17 +57,21 @@ 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{} ) 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) { @@ -74,7 +80,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 +89,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 +106,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 +147,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,14 +163,21 @@ 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()) // 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()) @@ -141,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, }, @@ -156,6 +201,7 @@ var _ = BeforeSuite(func() { go func() { defer GinkgoRecover() err = mgr.Start(ctx) + managerStopped <- struct{}{} Expect(err).ToNot(HaveOccurred(), "Failed to run manager") }() @@ -172,7 +218,6 @@ var _ = BeforeSuite(func() { }).Should(Succeed()) // Verify kubernetes client is working - cli = mgr.GetClient() Expect(cli).ToNot(BeNil()) for _, namespace := range testNamespaces { @@ -187,7 +232,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/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 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/go.mod b/components/odh-notebook-controller/go.mod index d9b788e28c5..aee387549a4 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 @@ -34,9 +35,8 @@ 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 github.com/google/uuid v1.3.0 // indirect github.com/imdario/mergo v0.3.12 // 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= 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, }, 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 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))