test(e2e): ouroboros fold + CSI/NFS ordering + OIDC keycloakInternalUrl - #2728
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request optimizes the end-to-end testing pipeline by consolidating redundant test suites and hardening existing tests against timing-related failures. By serializing dependencies and adjusting wait thresholds, the changes improve test stability and reduce overall execution time in the CI environment. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds an optional ouroboros flag to the Kubernetes e2e runner that injects an ouroboros addon and runs hairpin‑NAT/DNS validation; increases several readiness and test timeouts; and patches Keycloak OIDC tests to use an in‑cluster internal URL. ChangesE2E Test Ouroboros Support and Configuration
Sequence Diagram(s)sequenceDiagram
participant CI as Test Runner
participant Script as run_kubernetes_test
participant K8s as kubectl/kubernetes API
participant HR as HelmRelease controller
participant Controller as cozy-ouroboros controller
participant DNSPod as dnscheck pod
CI->>Script: invoke run_kubernetes_test(..., enable_ouroboros=true)
Script->>K8s: kubectl apply cluster manifest (includes ouroboros_addon)
K8s->>HR: create HelmReleases (cilium,coredns,csi,vsnap-crd,ingress-nginx,ouroboros)
HR->>Controller: deploy cozy-ouroboros controller
Script->>HR: poll HelmRelease readiness
Script->>K8s: create hairpin-probe Ingress
Script->>K8s: create dnscheck pod
DNSPod->>K8s: resolve hairpin host -> expected ouroboros-proxy ClusterIP
Script->>K8s: delete dnscheck pod and probe Ingress
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Code Review
This pull request updates the E2E test suite to include "ouroboros" hairpin-NAT reconciliation assertions within the existing Kubernetes latest version test, reducing total test time. It also increases various timeouts for resource readiness and pod completion to accommodate slower reconciliation in Flux v2.8 environments. Furthermore, it adds an internal Keycloak URL to the OIDC configuration to resolve DNS issues during dashboard testing. I have no feedback to provide.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
hack/e2e-apps/run-kubernetes.sh (2)
13-13: 💤 Low valueDeclare and assign separately to avoid masking
yqreturn value.If
yqfails, the error code is masked bylocal. Splitting the declaration ensures failures propagate correctly.Proposed fix
- local k8s_version=$(yq "$version_expr" packages/apps/kubernetes/files/versions.yaml) + local k8s_version + k8s_version=$(yq "$version_expr" packages/apps/kubernetes/files/versions.yaml)🤖 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 `@hack/e2e-apps/run-kubernetes.sh` at line 13, The assignment uses "local k8s_version=$(yq ...)" which masks yq's exit status; instead declare the variable first and then run the command so failures propagate: split into a separate declaration for k8s_version and a following command substitution using yq (reference the k8s_version variable and the yq invocation in run-kubernetes.sh) so any non-zero exit from yq is not masked by local.
359-362: 💤 Low valueQuote the HelmRelease name arguments to prevent word splitting.
While unlikely to cause issues in practice, quoting shell variables in kubectl arguments follows best practice.
Proposed fix
for component in cilium coredns csi vsnap-crd; do - kubectl wait hr kubernetes-${test_name}-${component} -n tenant-test --timeout=5m --for=condition=ready + kubectl wait hr "kubernetes-${test_name}-${component}" -n tenant-test --timeout=5m --for=condition=ready done - kubectl wait hr kubernetes-${test_name}-ingress-nginx -n tenant-test --timeout=5m --for=condition=ready + kubectl wait hr "kubernetes-${test_name}-ingress-nginx" -n tenant-test --timeout=5m --for=condition=ready🤖 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 `@hack/e2e-apps/run-kubernetes.sh` around lines 359 - 362, The kubectl wait commands use unquoted shell expansions (kubernetes-${test_name}-${component} and kubernetes-${test_name}-ingress-nginx) which can lead to word splitting; update the two invocations to quote the HelmRelease name arguments so the expanded names are treated as single arguments (e.g., quote "kubernetes-${test_name}-${component}" in the loop and "kubernetes-${test_name}-ingress-nginx" for the ingress wait).
🤖 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.
Nitpick comments:
In `@hack/e2e-apps/run-kubernetes.sh`:
- Line 13: The assignment uses "local k8s_version=$(yq ...)" which masks yq's
exit status; instead declare the variable first and then run the command so
failures propagate: split into a separate declaration for k8s_version and a
following command substitution using yq (reference the k8s_version variable and
the yq invocation in run-kubernetes.sh) so any non-zero exit from yq is not
masked by local.
- Around line 359-362: The kubectl wait commands use unquoted shell expansions
(kubernetes-${test_name}-${component} and kubernetes-${test_name}-ingress-nginx)
which can lead to word splitting; update the two invocations to quote the
HelmRelease name arguments so the expanded names are treated as single arguments
(e.g., quote "kubernetes-${test_name}-${component}" in the loop and
"kubernetes-${test_name}-ingress-nginx" for the ingress wait).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e8b77c1c-26bd-464c-9bc0-14dc59bc145a
📒 Files selected for processing (3)
hack/e2e-apps/kubernetes-latest.batshack/e2e-apps/run-kubernetes.shhack/e2e-install-cozystack.bats
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM — commits 2/3/4 (CSI/NFS ordering, NFS timeout bump, OIDC keycloakInternalUrl) are clean and verifiable, but commit 1 (fold ouroboros assertions into kubernetes-latest) ships a duplication, not a consolidation. The standalone hack/e2e-apps/ouroboros.bats is still in the tree, so the e2e workflow runs the assertions twice — once on its own ~25m Kamaji-backed tenant and again as the folded addon on the kubernetes-latest tenant. The promised time saving doesn't land until the standalone file is deleted.
Blocker
B1: hack/e2e-apps/ouroboros.bats is not deleted, so the second Kamaji bringup still runs
File: hack/e2e-apps/ouroboros.bats (still present on this branch)
Issue: the e2e workflow at .github/workflows/pull-requests.yaml auto-discovers every .bats file in hack/e2e-apps/:
for app in $(ls hack/e2e-apps/*.bats | xargs -n1 basename | cut -d. -f1); do
...
make -C packages/core/testing SANDBOX_NAME=$SANDBOX_NAME test-apps-$apppackages/core/testing/Makefile has a wildcard rule test-apps-% that dispatches to hack/cozytest.sh hack/e2e-apps/$*.bats. So test-apps-ouroboros is still wired up and runs against its own freshly-provisioned tenant Kubernetes — the very 25-45m Kamaji bringup the commit message calls out as "pure duplication". On top of that, test-apps-kubernetes-latest now runs the same assertions a second time on its tenant.
The 3c81261ba commit message acknowledges this explicitly:
(The standalone ouroboros.bats was removed in the preceding commit alongside an unrelated foundationdb chart fix — the git rm staging had already happened when the foundationdb commit landed.)
That "preceding commit" is in the source PR #2619 (which is OPEN and shows 0+/386- hack/e2e-apps/ouroboros.bats in its file list), not in #2728. The cherry-pick picked up the run-kubernetes.sh additions but not the deletion. The PR body's claim — "Reuses the existing tenant cluster instead of building a second one" — doesn't hold on the current branch.
Impact: this PR currently makes e2e longer, not shorter — it adds the addon-flavored assertions to kubernetes-latest (extra time on that tenant) without removing the standalone's separate Kamaji bringup. To deliver the documented goal, the deletion has to land in this PR.
Fix: add git rm hack/e2e-apps/ouroboros.bats as a separate commit in this PR. No replacement test needed — the folded assertions in run-kubernetes.sh (lines ~365 onwards) already cover ouroboros HR Ready, controller pod Running, Ingress → coredns-custom rewrite-line injection, and end-to-end DNS resolution from inside the tenant. That's the same surface the standalone covers; the standalone becomes pure dead weight after the fold.
Non-blocking (already noted by reviewers, but worth confirming)
- CodeRabbit nit 1:
local k8s_version=$(yq ...)masksyq's exit status. Split intolocal k8s_version+k8s_version=$(yq ...). Pure shell hygiene, not gating. - CodeRabbit nit 2: HelmRelease name interpolations in the
for component in cilium coredns csi vsnap-crdloop aren't quoted. Quotes are idiomatic buttest_nameandcomponenthere are both literal-safe (no whitespace/glob risk), so no actual behavior change.
What's solid (concur with author's framing)
- Commit 2 (CSI HR Ready wait before NFS pod): the explicit
kubectl wait hr/kubernetes-${test_name}-csi --for=condition=readyahead of the NFS pod fixes a real ordering bug — without it the NFS pod schedules whilekubevirt-csi-nodeDaemonSet is mid-rollout. Concrete and correct. - Commit 3 (NFS 5m → 10m): straightforward budget bump. The reasoning (host CDI prime + tenant CSI mount + busybox pull bursts past 5m on slow runners) is plausible and the 10m doesn't mask a real bug — it just survives the slow path.
- Commit 4 (
keycloakInternalUrl): verified —packages/system/keycloak/templates/service.yamlships akeycloak-httpService of typeClusterIPon port 8080 incozy-keycloak, sohttp://keycloak-http.cozy-keycloak.svc:8080/realms/cozyresolves and matches the in-cluster Service. Removes the dashboard-gatekeeper crashloop onkeycloak.example.orgDNS lookup at install time. Good fix. - The
timeout 10s → 2mbump on the KamajiControlPlane creation wait (line ~99) is reasonable given Flux v2.8's kstatus-based health checks. Documented rationale matches the symptom.
Nothing on this PR is wrong on substance for commits 2-4; the blocker is purely about commit 1 needing the standalone deletion to ship its promised time saving.
|
Aleksei Sviridkin (@lexfrei) B1 addressed in 521aa82 — The two non-blocking shell-hygiene notes are also in: 09883f4 splits |
|
CodeRabbit (@coderabbitai) both nitpicks applied:
|
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM on f7279b68. Blocker B1 from my prior review and both CodeRabbit nits are landed in three surgical commits.
- B1 (standalone
hack/e2e-apps/ouroboros.batsstill wired in the e2e workflow → duplicated 25-45m Kamaji bringup) — addressed by521aa8235 test(e2e): delete standalone ouroboros.bats now folded into kubernetes-latest. Pure deletion of the 386-line file. Verified:git ls-tree origin/ci/e2e-improvements -- hack/e2e-apps/ouroboros.batsreturns empty, so thefor app in $(ls hack/e2e-apps/*.bats | xargs -n1 basename | cut -d. -f1)loop in.github/workflows/pull-requests.yamlno longer picks upouroboros, and the wildcardtest-apps-%rule inpackages/core/testing/Makefileno longer dispatches tocozytest.sh hack/e2e-apps/ouroboros.bats. The folded assertions inrun-kubernetes.sh(HR Ready, controller pod Running,Ingress → coredns-customrewrite, end-to-end DNS via netshoot) cover the same surface the standalone did. The PR now genuinely delivers the documented time saving. - CodeRabbit nit 1 (
local k8s_version=$(yq ...)masksyqexit status) — addressed by09883f46a test(e2e): split local k8s_version declaration from yq invocation. Nowlocal k8s_version+k8s_version=$(yq ...)on separate lines so ayqfailure propagates instead of being masked bylocal's own zero exit. - CodeRabbit nit 2 (unquoted
kubernetes-${test_name}-${component}interpolation in thefor component in cilium coredns csi vsnap-crd; do kubectl wait hr ...loop) — addressed byf7279b684 test(e2e): quote HelmRelease names in kubectl wait. Both the loop body and the trailingkubernetes-${test_name}-ingress-nginxare now quoted. Belt-and-suspenders sincetest_nameandcomponentare both literal-safe here, but the quoting is the idiomatic shape.
This APPROVE supersedes my prior REQUEST_CHANGES (#4363068453). Gate-wise, this should now flip reviewDecision from CHANGES_REQUESTED to APPROVED on the next decision recomputation, since I'm the only human reviewer on this PR. CI is in flight on the new SHA; the prior b407cacc9 SHA passed Build + E2E (1h25m) and the three new commits are mechanical (file deletion + shell hygiene), so I'd expect green there too.
The standalone hack/e2e-apps/ouroboros.bats provisioned a full Kamaji- backed tenant Kubernetes cluster (control plane + 2 CAPI workers + base addons) just to flip `addons.ouroboros.enabled=true` on top, then ran ~15m of hairpin-NAT reconciliation assertions. Cluster shape was identical to what hack/e2e-apps/run-kubernetes.sh already provisions for kubernetes-latest, so the second Kamaji bringup was pure duplication — roughly 25-45m of CI wall-time per run. Add an `enable_ouroboros` 4th parameter to `run_kubernetes_test()`. When "true", inject the ouroboros addon block into the Kubernetes CR's addons map and run the post-cluster assertions inline: - Wait for the ouroboros HR Ready in the host namespace. - Verify coredns-custom ConfigMap exists in tenant kube-system. - Wait for the ouroboros controller pod Ready. - Apply an Ingress with a TLS host in tenant default namespace. - Poll coredns coredns-custom for the rewrite line (5m budget). - Resolve the hairpin host from inside the tenant via a netshoot pod and assert it points at the ouroboros-proxy ClusterIP. - Clean up dnscheck pod + Ingress. kubernetes-latest passes "true" (gets ouroboros coverage); kubernetes- previous leaves it empty (control-plane-only smoke remains). (The standalone ouroboros.bats was removed in the preceding commit alongside an unrelated foundationdb chart fix — the git rm staging had already happened when the foundationdb commit landed.) Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> (cherry picked from commit b5c9c2a)
kubernetes-previous fails on the NFS pod step. From cozyreport + job log:
- Pod scheduled to tenant worker
- 7x FailedAttachVolume over 35s: CSINode does not contain driver
csi.kubevirt.io (kubevirt-csi-node DaemonSet still rolling out)
- Attach succeeds at T+1m08s after the driver registers
- busybox pull queued ~1m25s, completes at T+2m48s
- CreateContainer in containerd then stalls past kubelet's CRI
deadline; retry trips "failed to reserve container name ... is
reserved for <prev>" -- the 5m pod-Succeeded wait then expires.
Root cause: run-kubernetes.sh creates the NFS PVC+Pod before asserting
kubernetes-${test_name}-csi is Ready. That HR lives in the parent
cluster but installs cozy-kubevirt-csi-node into the tenant via
kubeConfig.secretRef; helm-controller walks kstatus on the tenant
DaemonSet using the tenant kubeconfig. HR Ready in parent therefore
implies kubevirt-csi-node Pods Ready on every tenant worker, which
implies csi.kubevirt.io is published in CSINode. The existing wait at
line 354 runs AFTER the NFS test, so it does not gate the race.
Move the wait earlier with a 10m budget. Post-NFS sibling waits stay
as is, so component coverage is unchanged.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
(cherry picked from commit 394b1b2)
Previous run failed kubernetes-latest (and then kubernetes-previous via cascade) on the 5m budget for nfs-test-pod to reach Succeeded. The chain is host linstor prime PVC + CDI import + tenant CSI attach + busybox image pull + containerd CreateContainer (which occasionally stalls past kubelet's 2m CRI deadline and retries with name-reservation conflict). ~40s on the happy path, 6m+ in observed worst case. When kubernetes-latest times out, its tenant cluster (2x 20Gi worker PVCs) leaks because set -e exits before any cleanup. kubernetes- previous then runs, adds another 40Gi, and the host tenant-quota (100Gi) is exhausted -- every downstream app test (mariadb, mongo, postgres, qdrant, redis, openbao, vminstance) fails with ErrCreatingPVCPrime / exceeded quota. Bumping the wait to 10m gives the slow path room. Headroom costs nothing on green runs since the pod is busybox echo. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> (cherry picked from commit c5cc7b6)
The dashboard gatekeeper (oauth2-proxy) does OIDC discovery against the external host at container startup. The e2e sandbox uses host=example.org which does not resolve, so the auth-proxy crashloops: Get "https://keycloak.example.org/realms/cozy/.well-known/openid-configuration": dial tcp: lookup keycloak.example.org: no such host Under Flux v2.8 kstatus the gatekeeper Deployment then flips to 'Failed' (rather than the pre-v2.8 'InProgress') and stalls the cozy-dashboard/dashboard HelmRelease, which in turn blocks cozy-fluxcd/flux-plunger via dependsOn. Previous runs masked this with an unrelated ImagePullBackOff on cozy-dashboard-console; with the console image fixed the OIDC discovery failure now surfaces. packages/core/platform/values.yaml:194-198 already declares authentication.oidc.keycloakInternalUrl. When set, the dashboard chart (packages/system/dashboard/templates/gatekeeper.yaml:59-66) renders --skip-oidc-discovery plus explicit redeem/jwks/validate/ backend-logout URLs pointing at the in-cluster keycloak Service. Browser-facing redirect-url and login-url stay on the external host (they are not exercised by the e2e flow). The patch sets the value alongside oidc.enabled in the same kubectl-patch so the configmap and gatekeeper rollout happen in one reconcile pass. Observed on CI run 26142986969 (PR #2619). Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> (cherry picked from commit 5915a57)
…s-latest The previous commit (3c81261) folded the ouroboros hairpin-NAT reconciliation assertions into the kubernetes-latest tenant under run-kubernetes.sh, but did not remove the standalone bats file. The e2e workflow auto-discovers every hack/e2e-apps/*.bats and dispatches test-apps-<name> via the wildcard rule in packages/core/testing/Makefile. Leaving ouroboros.bats in the tree meant the assertions ran twice: once on the standalone ~25m Kamaji-backed tenant and again as the folded addon on the kubernetes-latest tenant — the opposite of the consolidation the fold was intended to deliver. The folded assertions in hack/e2e-apps/run-kubernetes.sh cover the same surface (HR Ready, controller pod Running, coredns-custom rewrite-line injection, and end-to-end DNS resolution from inside the tenant), so the standalone file is dead weight after the fold. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
\`local k8s_version=\$(yq ...)\` masks the exit status of the command substitution — the assignment to a local variable always succeeds, so a failing yq is swallowed. Declare the local first, then assign, so non-zero yq exits propagate via shell errexit semantics if the script ever runs under \`set -e\`. Pure shell hygiene; no behavioral change on the success path. Surfaced by CodeRabbit and confirmed by the human reviewer as non-blocking but worth applying. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Both \${test_name} and \${component} are literal-safe today (no
whitespace, no glob characters), so this is a style-only change — but
quoting variable expansions in kubectl arguments matches the rest of
the file and removes a future-foot-gun if a test name ever grows a
hyphen-y suffix that interacts oddly with completion or globbing.
Scoped to the two HelmRelease wait lines flagged by CodeRabbit and
echoed by the human reviewer; the machinedeployment wait on the
preceding line keeps its existing style.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
76514ed to
ea7a8c8
Compare
What this PR does
Bundles four independent e2e improvements lifted from #2619:
test(e2e): fold ouroboros assertions into kubernetes-latest— moves the standalonehack/e2e-apps/ouroboros.bats(which provisioned its own full Kamaji control plane, ~25m) into the existingkubernetes-latestrun as an optional addon. The assertions cover ouroboros HR Ready, controller pod Running,Ingress → coredns-customrewrite-line injection, and end-to-end DNS resolution from inside the tenant. Reuses the existing tenant cluster instead of building a second one.test(e2e): wait for tenant CSI HR Ready before NFS test— explicitkubectl wait hr/kubernetes-${test_name}-csi --for=condition=readyahead of the NFS pod. Otherwise the NFS pod schedules while thekubevirt-csi-nodeDaemonSet is still rolling out, eats ~1m onFailedAttachVolumeretries, and trips the pod-Succeeded wait.test(e2e): bump NFS pod wait to 10m to fit slow-path chain— was 5m. With dependencies properly serialized (per Automatically removeflannelandkube-proxy#2), 5m was tight on slow runners; 10m gives the chain genuine room without masking real bugs.test(e2e): set keycloakInternalUrl when enabling OIDC— the dashboard gatekeeper (oauth2-proxy) does OIDC discovery against the keycloak Service URL; without an explicitkeycloakInternalUrl, it falls back to the public ingress URL which isn't routable from inside the cluster yet during install.Origin
All four commits lifted unchanged from #2619.
Release note
Summary by CodeRabbit