feat(quay): add e2e Playwright job with OIDC, LDAP, and Mailpit#80187
feat(quay): add e2e Playwright job with OIDC, LDAP, and Mailpit#80187jbpratt wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSplits Playwright e2e into a deploy step provisioning Keycloak/LDAP/Mailpit and a phased Playwright runner; updates Quay CI to enable mailing, replace test refs, rename the job, and increase timeout. ChangesQuay Playwright E2E Pipeline Restructure
Sequence DiagramsequenceDiagram
participant Deploy as quay-deploy-test-services
participant Runner as quay-test-playwright
participant Quay as Quay App
participant Keycloak
participant LDAP as 389DS
participant Mailpit
Deploy->>Keycloak: deploy dev-mode realm + Route
Deploy->>LDAP: deploy with init ConfigMap + postStart hook
Deploy->>Mailpit: deploy SMTP + HTTP services
Deploy->>LDAP: poll base DN until initialized
Deploy->>Keycloak: poll Route until host admitted
Deploy->>Runner: keycloak_route written to SHARED_DIR
Runner->>Quay: Phase 1: run database auth tests
Runner->>Quay: extract config.yaml + merge OIDC overlay
Quay->>Keycloak: restart app + handle OIDC requests
Runner->>Quay: Phase 2: run OIDC auth tests
Runner->>Quay: extract config.yaml + merge LDAP overlay
Quay->>LDAP: restart app + bind/search during auth
Runner->>Quay: Phase 3: run LDAP auth tests
Runner->>Mailpit: port-forward active during all phases
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jbpratt The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/pj-rehearse |
|
@jbpratt: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ci-operator/step-registry/quay/deploy-test-services/quay-deploy-test-services-commands.sh`:
- Around line 544-550: The current single read of `.status.ingress[0].host` into
KEYCLOAK_ROUTE is race-prone; replace it with a polling loop that repeatedly
runs the oc -n "${NAMESPACE}" get route keycloak -o
jsonpath='{.status.ingress[0].host}' until a non-empty host is returned or a
timeout/maximum attempts is reached, logging each attempt and sleeping between
retries; once KEYCLOAK_ROUTE is non-empty write "https://${KEYCLOAK_ROUTE}" to
"${SHARED_DIR}/keycloak_route" and echo it, and if the timeout is reached still
empty exit 1 with an error message.
- Around line 340-385: The ldapmodify and ldapadd calls in init-389ds.sh
currently swallow failures with "|| true", causing the init to succeed even if
schema or LDIF application fails; update the ldapmodify (using LDAPI_URI) and
ldapadd commands to remove the "|| true" suppression, capture their exit status,
and if non-zero log a descriptive error (including command output) and exit
non-zero so the container fails fast when schema (quayMemberOf/quayUser) or
base.ldif import fails; ensure any grep -v "^SASL" filtering preserves exit
detection (e.g., check the pipeline exit status) and apply the same fail-fast
handling for both ldapmodify and ldapadd invocations.
- Line 414: Replace nondeterministic :latest image tags with pinned, immutable
references: update occurrences of docker.io/389ds/dirsrv:latest and
docker.io/axllent/mailpit:latest in quay-deploy-test-services-commands.sh to
either a specific version tag or a content-addressable digest (e.g.,
`@sha256`:...) so CI uses a fixed image; also audit
quay-deploy-test-services-ref.yaml for keys like cli: latest and from_image.tag:
latest and pin those entries similarly. Ensure you fetch the correct stable tag
or digest from the upstream registry, replace the :latest strings in the deploy
script and the YAML, and run the deploy/test pipeline to confirm the pinned
images work as expected.
In
`@ci-operator/step-registry/quay/test-playwright/quay-test-playwright-commands.sh`:
- Around line 10-18: The script reads QUAY_ROUTE and already validates it; do
the same for KEYCLOAK_ROUTE: after assigning KEYCLOAK_ROUTE via cat
"${SHARED_DIR}/keycloak_route", add an explicit non-empty check (similar to the
QUAY_ROUTE block) that writes an error to stderr and exits non‑zero if
KEYCLOAK_ROUTE is missing or empty, and only echo "Keycloak route:
${KEYCLOAK_ROUTE}" after the validation; update the validation logic for
KEYCLOAK_ROUTE to mirror the QUAY_ROUTE pattern so OIDC config never runs with
an invalid value.
- Around line 35-36: The curl invocation that fetches and executes yq from the
"latest" GitHub URL should be changed to download a pinned YQ_VERSION and
perform integrity verification before making it executable: introduce a
YQ_VERSION variable (or pinned release tag), fetch the corresponding yq binary
and its known SHA256 (or the .sha256 file from the same release), verify the
checksum with sha256sum (or openssl) and abort if it doesn't match, then chmod
+x /tmp/yq only after verification; also replace the global set -x tracing and
any echo of "Quay route" / "Keycloak route" with either removal or wrap them
with set +x / set -x so sensitive URLs are not printed, and use curl flags like
-fSLo to fail loudly without leaking URL info.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: aa6eb3be-f0e4-4acf-932b-31e08e202797
⛔ Files ignored due to path filters (1)
ci-operator/jobs/quay/quay/quay-quay-master-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (11)
ci-operator/config/quay/quay/quay-quay-master.yamlci-operator/step-registry/quay-tests/test-quay-playwright/quay-tests-test-quay-playwright-commands.shci-operator/step-registry/quay-tests/test-quay-playwright/quay-tests-test-quay-playwright-ref.yamlci-operator/step-registry/quay/deploy-test-services/OWNERSci-operator/step-registry/quay/deploy-test-services/quay-deploy-test-services-commands.shci-operator/step-registry/quay/deploy-test-services/quay-deploy-test-services-ref.metadata.jsonci-operator/step-registry/quay/deploy-test-services/quay-deploy-test-services-ref.yamlci-operator/step-registry/quay/test-playwright/OWNERSci-operator/step-registry/quay/test-playwright/quay-test-playwright-commands.shci-operator/step-registry/quay/test-playwright/quay-test-playwright-ref.metadata.jsonci-operator/step-registry/quay/test-playwright/quay-test-playwright-ref.yaml
💤 Files with no reviewable changes (2)
- ci-operator/step-registry/quay-tests/test-quay-playwright/quay-tests-test-quay-playwright-ref.yaml
- ci-operator/step-registry/quay-tests/test-quay-playwright/quay-tests-test-quay-playwright-commands.sh
|
/pj-rehearse pull-ci-quay-quay-master-e2e @CodeRabbit resolve |
|
@jbpratt: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# List all review threads on the PR to find CodeRabbit's comments
gh api graphql -f query='
{
repository(owner: "openshift", name: "release") {
pullRequest(number: 80187) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 5) {
nodes {
author { login }
body
databaseId
}
}
}
}
}
}
}'Length of output: 18528 🏁 Script executed: #!/bin/bash
# Resolve the two unresolved CodeRabbit review threads
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOBHvUUM6HsqmV"}) {
thread { id isResolved }
}
}'
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOBHvUUM6HsqmW"}) {
thread { id isResolved }
}
}'Length of output: 1124
[resolve] |
|
/pj-rehearse pull-ci-quay-quay-master-e2e |
|
@jbpratt: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ci-operator/step-registry/quay/deploy-test-services/quay-deploy-test-services-commands.sh (1)
3-4: ⚡ Quick winDisable global xtrace by default in this step script.
set -xat Line 4 makes every command traced. Keep tracing opt-in (debug-only) to match repo policy and reduce accidental log exposure.Suggested patch
set -euo pipefail -set -x +[[ "${CI_DEBUG_TRACE:-false}" == "true" ]] && set -xAs per coding guidelines, step-registry scripts should default to
set -euo pipefailwithout-x, enabling tracing only when necessary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci-operator/step-registry/quay/deploy-test-services/quay-deploy-test-services-commands.sh` around lines 3 - 4, Remove the unconditional tracing flag by deleting the `set -x` line so the script defaults to `set -euo pipefail`; make tracing opt-in by enabling xtrace only when a debug env var is set (e.g., check `CI_DEBUG` or `DEBUG` and then enable `set -x`), ensuring you reference the existing `set -euo pipefail` and `set -x` symbols when editing the script.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ci-operator/step-registry/quay/deploy-test-services/quay-deploy-test-services-commands.sh`:
- Around line 118-147: The inline Deployment pod specs for the keycloak, ldap,
and mailpit containers must be hardened: add pod-level
automountServiceAccountToken: false, enforce a podSecurityContext with
runAsNonRoot: true and a non-zero runAsUser (or add container-level
securityContext with runAsNonRoot: true and runAsUser > 0), and for each
container (keycloak, ldap, mailpit) add a container securityContext that sets
allowPrivilegeEscalation: false, readOnlyRootFilesystem: true, and drops all
non-essential capabilities (drop: ["ALL"]); also add resource requests and
limits (cpu/memory) for each container to avoid unbounded resources; ensure
capabilities and securityContext are applied to the same container specs
referenced by the names keycloak, ldap, and mailpit in the Deployment manifests
mentioned.
---
Nitpick comments:
In
`@ci-operator/step-registry/quay/deploy-test-services/quay-deploy-test-services-commands.sh`:
- Around line 3-4: Remove the unconditional tracing flag by deleting the `set
-x` line so the script defaults to `set -euo pipefail`; make tracing opt-in by
enabling xtrace only when a debug env var is set (e.g., check `CI_DEBUG` or
`DEBUG` and then enable `set -x`), ensuring you reference the existing `set -euo
pipefail` and `set -x` symbols when editing the script.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 652a19b7-a8c9-43e4-89f7-f7f4f8b44184
⛔ Files ignored due to path filters (1)
ci-operator/jobs/quay/quay/quay-quay-master-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (11)
ci-operator/config/quay/quay/quay-quay-master.yamlci-operator/step-registry/quay-tests/test-quay-playwright/quay-tests-test-quay-playwright-commands.shci-operator/step-registry/quay-tests/test-quay-playwright/quay-tests-test-quay-playwright-ref.yamlci-operator/step-registry/quay/deploy-test-services/OWNERSci-operator/step-registry/quay/deploy-test-services/quay-deploy-test-services-commands.shci-operator/step-registry/quay/deploy-test-services/quay-deploy-test-services-ref.metadata.jsonci-operator/step-registry/quay/deploy-test-services/quay-deploy-test-services-ref.yamlci-operator/step-registry/quay/test-playwright/OWNERSci-operator/step-registry/quay/test-playwright/quay-test-playwright-commands.shci-operator/step-registry/quay/test-playwright/quay-test-playwright-ref.metadata.jsonci-operator/step-registry/quay/test-playwright/quay-test-playwright-ref.yaml
💤 Files with no reviewable changes (2)
- ci-operator/step-registry/quay-tests/test-quay-playwright/quay-tests-test-quay-playwright-ref.yaml
- ci-operator/step-registry/quay-tests/test-quay-playwright/quay-tests-test-quay-playwright-commands.sh
✅ Files skipped from review due to trivial changes (2)
- ci-operator/step-registry/quay/test-playwright/quay-test-playwright-ref.metadata.json
- ci-operator/step-registry/quay/deploy-test-services/quay-deploy-test-services-ref.metadata.json
🚧 Files skipped from review as they are similar to previous changes (2)
- ci-operator/step-registry/quay/deploy-test-services/quay-deploy-test-services-ref.yaml
- ci-operator/step-registry/quay/test-playwright/quay-test-playwright-commands.sh
| spec: | ||
| containers: | ||
| - name: keycloak | ||
| image: quay.io/keycloak/keycloak:26.2 | ||
| args: ["start-dev", "--import-realm"] | ||
| env: | ||
| - name: KC_HEALTH_ENABLED | ||
| value: "true" | ||
| - name: KC_HTTP_RELATIVE_PATH | ||
| value: "/" | ||
| - name: KEYCLOAK_ADMIN | ||
| value: "admin" | ||
| - name: KEYCLOAK_ADMIN_PASSWORD | ||
| value: "admin" | ||
| ports: | ||
| - containerPort: 8080 | ||
| readinessProbe: | ||
| httpGet: | ||
| path: /health/ready | ||
| port: 8080 | ||
| initialDelaySeconds: 30 | ||
| periodSeconds: 10 | ||
| volumeMounts: | ||
| - name: realm | ||
| mountPath: /opt/keycloak/data/import | ||
| volumes: | ||
| - name: realm | ||
| configMap: | ||
| name: keycloak-realm | ||
| --- |
There was a problem hiding this comment.
Harden the inline Deployments to the repo’s Kubernetes security baseline.
The generated keycloak, ldap, and mailpit pod specs currently rely on permissive defaults (no explicit non-root/read-only/priv-esc controls, no dropped capabilities, no explicit automountServiceAccountToken: false, and no container limits). Please add explicit hardening and limits in each container spec before merge.
As per coding guidelines, Kubernetes/OpenShift manifests should enforce non-root hardening (runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation: false, drop capabilities), disable service-account token automount unless needed, and define resource limits.
Also applies to: 404-430, 468-488
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ci-operator/step-registry/quay/deploy-test-services/quay-deploy-test-services-commands.sh`
around lines 118 - 147, The inline Deployment pod specs for the keycloak, ldap,
and mailpit containers must be hardened: add pod-level
automountServiceAccountToken: false, enforce a podSecurityContext with
runAsNonRoot: true and a non-zero runAsUser (or add container-level
securityContext with runAsNonRoot: true and runAsUser > 0), and for each
container (keycloak, ldap, mailpit) add a container securityContext that sets
allowPrivilegeEscalation: false, readOnlyRootFilesystem: true, and drops all
non-essential capabilities (drop: ["ALL"]); also add resource requests and
limits (cpu/memory) for each container to avoid unbounded resources; ensure
capabilities and securityContext are applied to the same container specs
referenced by the names keycloak, ldap, and mailpit in the Deployment manifests
mentioned.
Source: Coding guidelines
|
/pj-rehearse pull-ci-quay-quay-master-e2e |
|
@jbpratt: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
openshift-ci
Bot
added
the
needs-rebase
openshift-ci
Bot
removed
the
needs-rebase
|
/pj-rehearse pull-ci-quay-quay-master-e2e |
|
@jbpratt: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-quay-quay-master-e2e |
|
@jbpratt: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-quay-quay-master-e2e |
|
@jbpratt: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-quay-quay-master-e2e |
|
@jbpratt: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-quay-quay-master-e2e |
|
@jbpratt: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-quay-quay-master-e2e |
|
@jbpratt: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-quay-quay-master-e2e |
|
@jbpratt: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-quay-quay-master-e2e |
|
@jbpratt: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
Extend the Quay CI to run Playwright tests across three auth backends. A new deploy-test-services step provisions Keycloak (OIDC), 389ds (LDAP), and Mailpit (email) into the cluster. The test runner executes Database, OIDC, and LDAP auth phases sequentially with config-swap restarts between each phase. - Rename playwright-e2e job to e2e - Move playwright step from quay-tests/ to quay/ namespace - Delete old quay-tests-test-quay-playwright step Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Brady Pratt <bpratt@redhat.com>
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
/pj-rehearse pull-ci-quay-quay-master-e2e |
|
@jbpratt: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@jbpratt: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
1 participant
Extend the Quay CI to run Playwright tests across three auth backends. A new deploy-test-services step provisions Keycloak (OIDC), 389ds (LDAP), and Mailpit (email) into the cluster. The test runner executes Database, OIDC, and LDAP auth phases sequentially with config-swap restarts between each phase.
Summary by CodeRabbit
This PR modifies OpenShift CI configuration for Quay Playwright end-to-end tests to deploy test services in-cluster and run a multi-phase authentication matrix (Database, OIDC, LDAP).
What changed, in practical terms:
Net effect: