chore(e2e-next): Migrate e2e_certs to e2e-next framework#3698
chore(e2e-next): Migrate e2e_certs to e2e-next framework#3698pascalbreuninger wants to merge 1 commit into
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ccdc1d518
ℹ️ 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".
| os.Remove(tmpPath) | ||
| } | ||
|
|
||
| options := &cli.ConnectOptions{ | ||
| BackgroundProxy: true, |
There was a problem hiding this comment.
connectVCluster always turns on BackgroundProxy before calling cli.ConnectHelm, which starts a detached Docker proxy container on hosts where Docker is available, but the returned cleanup only removes the temp kubeconfig file. Because no corresponding proxy teardown is performed, this test leaves a stale vcluster connect proxy container behind after execution, which can consume CI runner resources and interfere with later runs; cleanup should also stop the background proxy (or avoid using BackgroundProxy here).
Useful? React with 👍 / 👎.
| caCertBefore *x509.Certificate | ||
| ) | ||
|
|
||
| BeforeAll(func(ctx context.Context) { |
There was a problem hiding this comment.
BeforeAll does not return context.Context. Inside an Ordered Describe, BeforeAll should return the enriched context so that It blocks receive any context modifications. In this case the hostClient is a captured var so it works, but the signature is inconsistent with the framework convention.
| }, | ||
| ) | ||
|
|
||
| // waitForVClusterReady polls until all vCluster pods are running with all containers ready. |
There was a problem hiding this comment.
Missing GinkgoHelper() - waitForVClusterReady is a shared assertion helper that should call GinkgoHelper() so failure stack traces point to the call site.
Also it can be moved to some common global helpers so other tests can use it.
|
|
||
| // connectVCluster establishes a fresh connection to the vCluster using a background proxy. | ||
| // Returns a rest.Config and a cleanup function that removes the temp kubeconfig. | ||
| func connectVCluster(ctx context.Context, name, namespace string) (*rest.Config, func()) { |
There was a problem hiding this comment.
Missing GinkgoHelper(), also can be moved to some Helper file
| tmpFile.Close() | ||
|
|
||
| cleanupFn := func() { | ||
| os.Remove(tmpPath) |
There was a problem hiding this comment.
Background proxy container leak - cleanupFn only removes the temp kubeconfig file via os.Remove(tmpPath). cli.ConnectHelm with BackgroundProxy: true starts a Docker container. The cleanup
never stops it. This function is called up to 4 times per run, potentially leaking 4 Docker containers.
Fix (option A - stop the container):
func connectVCluster(ctx context.Context, name, namespace string) (*rest.Config, func(context.Context)) {
// ... setup ...
err = cli.ConnectHelm(ctx, options, globalFlags, name, nil, loftlog.Discard)
Expect(err).NotTo(HaveOccurred())
cleanupFn := func(ctx context.Context) {
// Stop the background proxy container
disconnectOptions := &cli.DisconnectOptions{}
_ = cli.DisconnectHelm(ctx, disconnectOptions, name)
os.Remove(tmpPath)
}
// ...
return cfg, cleanupFn
}
or avoid containers entirely:
options := &cli.ConnectOptions{
BackgroundProxy: false,
KubeConfig: tmpPath,
}
| @@ -0,0 +1,411 @@ | |||
| package test_certs | |||
There was a problem hiding this comment.
Dropped "certs check" subcommand coverage - old test called certs check twice
Explanation:
The old test in test/e2e_certs/certs/rotate.go called vcluster certs check twice via the CLI subcommand:
- Before rotation (line 703): certsCmd.SetArgs([]string{"check", f.VClusterName}) - inside the It("checking current validity date of CA cert of vCluster") block
- After rotation (line 797): certsCmd.SetArgs([]string{"check", f.VClusterName}) - inside the It("priniting new expiry date and time of vCluster CA cert") block (note the typo "priniting" in the original)
Both of these instantiate certscmd.NewCertsCmd(...), set args to ["check", vclusterName], call Execute(), and assert no error. This exercises the vcluster certs check CLI subcommand - verifying it can inspect a vCluster's certificates and
report them as valid.
The new migrated test in e2e-next/test_certs/test_rotate.go dropped both of these calls entirely. It validates certificate fingerprints and expiry dates directly by reading Kubernetes secrets, but it never invokes the certs check CLI
subcommand.
| // contexts depend on the state left by prior ones. | ||
| var _ = Describe("Certificate Rotation", | ||
| Ordered, | ||
| labels.PR, |
There was a problem hiding this comment.
labels.PR on a slow/destructive suite (multiple rotations + pod restarts) - maybe this should be in nightly build only?
2 participants
Summary
test/e2e_certs/(certificate rotation tests) to thee2e-next/test_certs/frameworkDescribeblocks into a singleOrderedDescribewith fourContextblocksCertsVClustercluster definition with dedicated values YAMLe2e_suite_test.gowith proper setup/cleanup wiringTest plan
go build ./e2e-next/...compiles cleanlygo vet ./e2e-next/...passestest/e2e_certs/files are fully removed🤖 Generated with Claude Code
📋 E2E Migration Validator Notes
Overall:⚠️ Changes requested
🔴 Must Fix
connectVCluster(lines 342-387):cli.ConnectHelmwithBackgroundProxy: truestarts a Docker container.The
cleanupFnonly callsos.Remove(tmpPath)on the kubeconfig file — it never stopsthe Docker container. This function is called 4 times, potentially leaking 4 containers
per test run. Fix: add container cleanup to the cleanup function, or use
BackgroundProxy: false.🟡 Should Fix
Dropped "certs check" coverage: The old test called
certs check <name>to verify the check subcommand works both before and after rotation. Add at least oneItthat calls the check command and verifies it succeeds.labels.PRon a slow, destructive test (line 40): This suite performs multiple cert rotations with pod-readiness waits (2+ minutes). Consider removinglabels.PRand running only withlabels.Certsin a dedicated job.🟢 Nice to Have
os.Setenvdefer scope (lines 161-166): Thedeferinside theBy()closure fires at the end of that anonymous function, not at the end of theIt(). A comment would prevent future confusion.vclusterNSandvclusterNamecould beconstinstead ofvar.connectVClusterhelper (45 lines) could be moved to a shared helper package if reused.