Skip to content

Conversation

@subhashkhileri
Copy link
Member

@subhashkhileri subhashkhileri commented Jan 20, 2026

Description

https://issues.redhat.com/browse/RHIDP-11483

Which issue(s) does this PR fix

  • Fixes #?

PR acceptance criteria

Please make sure that the following steps are complete:

  • GitHub Actions are completed and successful
  • Unit Tests are updated and passing
  • E2E Tests are updated and passing
  • Documentation is updated if necessary (requirement for new features)
  • Add a screenshot if the change is UX/UI related

How to test changes / Special notes to the reviewer

@openshift-ci openshift-ci bot requested a review from albarbaro January 20, 2026 07:22
@openshift-ci
Copy link

openshift-ci bot commented Jan 20, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign 04kash for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@christoph-jerolimov
Copy link
Member

/cc @jrichter1 @christoph-jerolimov

@christoph-jerolimov
Copy link
Member

@github-actions
Copy link
Contributor

The image is available at:

/test e2e-ocp-helm

@subhashkhileri subhashkhileri changed the title local test runner chore(e2e) : Add Local Test Runner support for E2E Tests for RHDH core. Jan 20, 2026
@github-actions
Copy link
Contributor

The image is available at:

/test e2e-ocp-helm

@github-actions
Copy link
Contributor

The image is available at:

/test e2e-ocp-helm

@gustavolira
Copy link
Member

@subhashkhileri subhashkhileri changed the title chore(e2e) : Add Local Test Runner support for E2E Tests for RHDH core. chore(e2e) : add Local Test Runner support for E2E Tests for RHDH core. Jan 21, 2026
@subhashkhileri subhashkhileri changed the title chore(e2e) : add Local Test Runner support for E2E Tests for RHDH core. chore : add Local Test Runner support for E2E Tests for RHDH core. Jan 21, 2026
@subhashkhileri subhashkhileri changed the title chore : add Local Test Runner support for E2E Tests for RHDH core. chore: add Local Test Runner support for E2E Tests for RHDH core. Jan 21, 2026
@github-actions
Copy link
Contributor

The image is available at:

/test e2e-ocp-helm

@zdrapela
Copy link
Member

/review
-i

@rhdh-qodo-merge
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

RHIDP-11483 - Partially compliant

Compliant requirements:

  • Support running tests in a container that mirrors the CI environment.
  • Support deploy-only, then run tests locally in headed browser mode for debugging.
  • Provide an interactive CLI to select job type, image repository, image tag, and run mode.
  • Persist configuration between runs.
  • Verify image existence on quay.io before deployment.
  • Fetch secrets from Vault and avoid writing them to disk (tmpfs usage inside container; local setup exports at runtime).
  • Detect CrashLoopBackOff early and fail fast.
  • Documentation includes usage and examples (at least for OCP; includes guidance for AKS/EKS/GKE via custom job patterns).
  • Prerequisites are documented and partially validated in script.

Non-compliant requirements:

  • Container drops into interactive shell on failure for debugging.

Requires further human verification:

  • Developer can deploy RHDH to any cluster they are logged into (OCP, AKS, EKS, GKE) end-to-end (scripts currently assume oc and OpenShift flows).
  • Documentation covers usage and examples for all supported clusters (verify accuracy for non-OCP clusters; current docs heavily OCP-centric).
  • Secrets are fetched from Vault securely and not stored on disk (verify no accidental persistence in artifacts/logs, and confirm Vault token handling expectations).
⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🔒 Security concerns

Privilege escalation / credential exposure:
e2e-tests/local-run.sh grants cluster-admin to a newly created service account and then exports/passes OC_TOKEN into a privileged container. If the host or container environment is compromised (or logs/process environment are inspected), this token can be abused with full cluster privileges.

Supply-chain risk: e2e-tests/container-init.sh downloads Vault binaries via curl and installs them without checksum/signature validation. This can enable tampering and execution of malicious binaries.

⚡ Recommended focus areas for review

Over-privilege

The runner creates a service account and grants it cluster-admin, then passes the token into the container. This is very powerful for local developer use and increases blast radius if the token is leaked (including via environment, logs, shell history, or process inspection). Consider scoping permissions to a namespace/required resources, offering an opt-in flag for cluster-admin, and cleaning up the service account/namespace after the run.

# Create service account in dedicated namespace and get token
log::section "Setting up cluster access"
SA_NAME="rhdh-local-tester"
SA_NAMESPACE="rhdh-local-test"
OC_SERVER=$(oc whoami --show-server)
oc create namespace "$SA_NAMESPACE" 2>/dev/null || log::info "Namespace already exists"
oc create serviceaccount "$SA_NAME" -n "$SA_NAMESPACE" 2>/dev/null || log::info "Service account already exists"
oc adm policy add-cluster-role-to-user cluster-admin "system:serviceaccount:${SA_NAMESPACE}:${SA_NAME}" 2>/dev/null || true
OC_TOKEN=$(oc create token "$SA_NAME" -n "$SA_NAMESPACE" --duration=48h)
log::info "K8S_CLUSTER_URL: $OC_SERVER"
Supply chain

The script downloads and installs Vault via curl without checksum/signature verification. This is a supply-chain risk (MITM/compromised artifact) and also makes debugging harder if the download is corrupted. Consider verifying SHA256 checksums (or using OS package manager / pinned image layer) before installing.

# Install vault if not present
if ! command -v vault &> /dev/null; then
    VAULT_VERSION="${VAULT_VERSION:-1.15.4}"
    log::info "Installing vault ${VAULT_VERSION}..."
    curl -fsSL "https://releases.hashicorp.com/vault/${VAULT_VERSION}/vault_${VAULT_VERSION}_linux_amd64.zip" -o /tmp/vault.zip
    unzip -q /tmp/vault.zip -d /usr/local/bin/
    rm /tmp/vault.zip
fi
📚 Focus areas based on broader codebase context

Strict Mode

The new script enables set -e but does not enable -u and -o pipefail, which can allow unset variables (e.g., OC_TOKEN, OC_SERVER) or failed piped commands to go unnoticed and produce confusing partial state. Consider switching to set -euo pipefail and explicitly guarding/validating required environment variables before use. (Ref 6)

#!/bin/bash

# Source logging library
# shellcheck source=../.ibm/pipelines/lib/log.sh
source "/tmp/rhdh/.ibm/pipelines/lib/log.sh"

# Trap errors and exit with error code
handle_error() {
    local exit_code=$?
    echo ""
    log::error "Container script failed! (exit code: $exit_code)"
    echo ""
    log::info "Check the logs above for details."
    log::info "Pod logs are saved to: .local-test/rhdh/.local-test/artifact_dir/"
    echo ""
    exit $exit_code
}
trap handle_error ERR

set -e

Reference reasoning: Existing bash utilities in the codebase use strict mode (set -euo pipefail) and required-variable checks to fail fast and avoid silent errors from unset variables or pipelines; aligning this script to that pattern reduces brittleness in CI/local runner flows.

📄 References
  1. redhat-developer/rhdh-operator/hack/validate-image-digests.sh [71-103]
  2. redhat-developer/rhdh-operator/hack/validate-image-digests.sh [104-145]
  3. redhat-developer/rhdh-operator/hack/validate-image-digests.sh [146-157]
  4. redhat-developer/rhdh-operator/hack/validate-image-digests.sh [159-190]
  5. redhat-developer/rhdh-operator/hack/validate-image-digests.sh [192-199]
  6. redhat-developer/rhdh-operator/hack/db_copy.sh [1-23]
  7. redhat-developer/rhdh-chart/hack/orchestrator-templates-setup.sh [351-386]
  8. redhat-developer/rhdh-chart/hack/orchestrator-templates-setup.sh [1-57]

Instead of hardcoding nodejs:18-ubi8, query the cluster for available
nodejs imagestream tags and select the latest UBI9 version. Falls back
to 18-ubi8 if no UBI9 tags are found.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@github-actions
Copy link
Contributor

🚫 Image Push Skipped.

The container image push was skipped because the build was skipped (either due to [skip-build] tag or no relevant changes with existing image)

Copy link
Member

@zdrapela zdrapela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong opinions here, but I have some thoughts on this 🤔

But great work overall!

Comment on lines +121 to +124
2) JOB_NAME="periodic-ci-ocp-helm-nightly" ;;
3) JOB_NAME="periodic-ci-ocp-operator-nightly" ;;
4) JOB_NAME="periodic-ci-ocp-helm-upgrade-nightly" ;;
5) JOB_NAME="periodic-ci-ocp-operator-auth-providers-nightly" ;;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: It won't have an effect, but we can align the job names with what's really used.

Suggested change
2) JOB_NAME="periodic-ci-ocp-helm-nightly" ;;
3) JOB_NAME="periodic-ci-ocp-operator-nightly" ;;
4) JOB_NAME="periodic-ci-ocp-helm-upgrade-nightly" ;;
5) JOB_NAME="periodic-ci-ocp-operator-auth-providers-nightly" ;;
2) JOB_NAME="periodic-ci-redhat-developer-rhdh-main-ocp-helm-nightly" ;;
3) JOB_NAME="periodic-ci-redhat-developer-rhdh-main-ocp-operator-nightly" ;;
4) JOB_NAME="periodic-ci-redhat-developer-rhdh-main-ocp-helm-upgrade-nightly" ;;
5) JOB_NAME="periodic-ci-redhat-developer-rhdh-main-ocp-operator-auth-providers-nightly" ;;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its release / branch independent. the main might confuse.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, we can keep it as it is

@@ -0,0 +1,335 @@
#!/bin/bash
Copy link
Member

@zdrapela zdrapela Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I prefer to have the option to use flags to configure the options that I want. The current setup with a walkthrough works fine, and it's great for a newcomer, etc. The option to rerun the same config is great. But for me, flags are unbeatable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool.

@@ -0,0 +1,141 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We currently have linters for bash scripts only in .ibm folder, and basically all the bash scripts related to e2e and CI are there. It can make sense to have those scripts in e2e-tests folder, but it's kind of mixing up TypeScript with Bash now 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are user-facing scripts that belong with the e2e-tests, not CI pipeline

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, but it would make sense to at least group them in a folder, maybe? So we can set up Prettier and ShellCheck (possibly in the future)

Co-authored-by: Zbyněk Drápela <61500440+zdrapela@users.noreply.github.com>
@github-actions
Copy link
Contributor

🚫 Image Push Skipped.

The container image push was skipped because the build was skipped (either due to [skip-build] tag or no relevant changes with existing image)

@github-actions
Copy link
Contributor

🚫 Image Push Skipped.

The container image push was skipped because the build was skipped (either due to [skip-build] tag or no relevant changes with existing image)

Add CLI flags (-j, -r, -t, -p, -s) to local-run.sh for automation and
quick runs without interactive prompts. Also improve container-init.sh
to pre-compute and save config before deployment so URLs are available
even if deployment fails.

Additional fixes:
- Fix secrets parsing in local-test-setup.sh to handle special chars
- Minor formatting fix in utils.sh for piped command

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@sonarqubecloud
Copy link

@github-actions
Copy link
Contributor

@openshift-ci
Copy link

openshift-ci bot commented Jan 22, 2026

@subhashkhileri: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-ocp-helm 82835ea link true /test e2e-ocp-helm

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants