chore(e2e-next): Migrate e2e_ha to e2e-next framework#3700
chore(e2e-next): Migrate e2e_ha to e2e-next framework#3700pascalbreuninger wants to merge 1 commit intomainfrom
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b4eaddf48
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Expect(certData.NotAfter.After(time.Now())).To(BeTrue(), | ||
| "CA cert NotAfter (%s) should be in the future", certData.NotAfter) |
There was a problem hiding this comment.
Assert certificate actually changes after rotation
This assertion only checks that NotAfter is in the future, which is already true for a freshly created vCluster certificate, so the test can pass even if vcluster certs rotate-ca (and similarly rotate in the second spec) does nothing. In CI this creates a false-positive signal for cert-rotation regressions because validity alone does not prove rotation happened; capture a pre-rotation fingerprint (for example serial number or NotBefore) and assert it changed after the command.
Useful? React with 👍 / 👎.
Summary
test/e2e_ha/(HA cert rotation tests) to thee2e-next/test_ha/frameworkItblocks (CA rotation + leaf rotation) withwaitForDeploymentReadyhelperHAVClustercluster definition with dedicated HA values YAML (3 replicas, external etcd)e2e_suite_test.gowith proper setup/cleanup wiringTest plan
go build ./e2e-next/...compiles cleanlygo vet ./e2e-next/...passestest/e2e_ha/files are fully removed🤖 Generated with Claude Code
📋 E2E Migration Validator Notes
Overall:⚠️ Changes requested
🔴 Must Fix
Itblocks assertcert.NotAfter.After(time.Now()), which passes even without rotation (a freshly installed vCluster already has valid certs). Record the cert's serial number (orNotBefore) before rotation, then assert it changed afterward.🟡 Should Fix
waitForDeploymentReadyassumes the vCluster workload is a Deployment. Withetcd.deploy.enabled: trueand no persistence, the chart renders a Deployment. If the HA config changes, this helper will silently fail. Add a comment documenting this assumption or generalize.🟢 Nice to Have
certs checkinvocation (even just verifying it does not error) to catch regressions in the check command's ability to connect to the HA vCluster.haVClusterNamespaceuses hardcoded"vcluster-"prefix — consistent with other tests but fragile.