[WIP]CNTRLPLANE-2549:test/e2e: migrate refresh-CA test for OTE compatibility #306
Conversation
Migrates the refresh-CA test to the OTE (Openshift Test Extended)
Ginkgo framework while maintaining dual-compatibility with traditional
Go tests.
Changes:
- Add Ginkgo test context in e2e.go for OTE test discovery:
* refresh-CA - Tests CA regeneration when deleted and recreated
- Extract test logic into shared function with testing.TB interface:
* testRefreshCA() - Verifies serving certs and configmaps update
when CA secret is deleted and recreated
- Add helper functions in e2e.go:
* pollForCABundleInjectionConfigMapWithReturn() - Polls for configmap
* pollForCARecreation() - Polls for CA secret recreation
- Keep test runner in e2e_test.go that calls shared function
- Remove duplicate helper functions from e2e_test.go
Net change: +30 lines (132 added - 102 removed)
Both test frameworks continue to work:
- Standard Go test: go test -run "^TestE2E$/^refresh-CA$"
- OTE: Ginkgo test discovery via service-ca-operator-tests-ext
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request refactors the e2e CA refresh test by moving helper functions and test logic from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wangke19 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/e2e/e2e.go`:
- Around line 1230-1317: The test captures configmapCopy before verifying the
injected CA data and calls pollForSecretChangeGinkgo for the headless secret
with no keys (which returns immediately); fix by moving the configmapCopy =
configmap.DeepCopy() to after checkConfigMapCABundleInjectionData(adminClient,
testConfigMapName, ns.Name) so the baseline includes injected data, and call
pollForSecretChangeGinkgo(t, adminClient, headlessSecretCopy, v1.TLSCertKey,
v1.TLSPrivateKeyKey) (same keys used for the regular secret) so the headless
secret change is actually validated; locate symbols
pollForServiceServingSecretWithReturn, configmapCopy,
checkConfigMapCABundleInjectionData, pollForConfigMapChange, headlessSecretCopy,
and pollForSecretChangeGinkgo to make the edits.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
test/e2e/e2e.gotest/e2e/e2e_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/e2e/e2e.gotest/e2e/e2e_test.go
🧬 Code graph analysis (1)
test/e2e/e2e.go (1)
pkg/controller/api/api.go (1)
InjectionDataKey(29-29)
🔇 Additional comments (3)
test/e2e/e2e_test.go (1)
697-703: Clean delegation to shared refresh-CA test.Nice consolidation to the shared
testRefreshCApath for OTE compatibility.test/e2e/e2e.go (2)
106-111: refresh-CA Ginkgo context integration looks good.Clear test discovery wiring for OTE.
1319-1349: Polling helpers are straightforward and consistent.Timeout usage and error handling align with the other poll helpers.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // testRefreshCA verifies that when the CA secret is deleted and recreated, | ||
| // all serving certs and configmaps get updated with the new CA. | ||
| // | ||
| // This test uses testing.TB interface for dual-compatibility with both | ||
| // standard Go tests and Ginkgo tests. | ||
| // | ||
| // This situation is temporary until we test the new e2e jobs with OTE. | ||
| // Eventually all tests will be run only as part of the OTE framework. | ||
| func testRefreshCA(t testing.TB) { | ||
| adminClient, err := getKubeClient() | ||
| if err != nil { | ||
| t.Fatalf("error getting kube client: %v", err) | ||
| } | ||
|
|
||
| ns, cleanup, err := createTestNamespace(t, adminClient, "test-"+randSeq(5)) | ||
| if err != nil { | ||
| t.Fatalf("could not create test namespace: %v", err) | ||
| } | ||
| defer cleanup() | ||
|
|
||
| // create secrets | ||
| testServiceName := "test-service-" + randSeq(5) | ||
| testSecretName := "test-secret-" + randSeq(5) | ||
| testHeadlessServiceName := "test-headless-service-" + randSeq(5) | ||
| testHeadlessSecretName := "test-headless-secret-" + randSeq(5) | ||
|
|
||
| err = createServingCertAnnotatedService(adminClient, testSecretName, testServiceName, ns.Name, false) | ||
| if err != nil { | ||
| t.Fatalf("error creating annotated service: %v", err) | ||
| } | ||
| if err = createServingCertAnnotatedService(adminClient, testHeadlessSecretName, testHeadlessServiceName, ns.Name, true); err != nil { | ||
| t.Fatalf("error creating annotated headless service: %v", err) | ||
| } | ||
|
|
||
| secret, err := pollForServiceServingSecretWithReturn(adminClient, testSecretName, ns.Name) | ||
| if err != nil { | ||
| t.Fatalf("error fetching created serving cert secret: %v", err) | ||
| } | ||
| secretCopy := secret.DeepCopy() | ||
| headlessSecret, err := pollForServiceServingSecretWithReturn(adminClient, testHeadlessSecretName, ns.Name) | ||
| if err != nil { | ||
| t.Fatalf("error fetching created serving cert secret: %v", err) | ||
| } | ||
| headlessSecretCopy := headlessSecret.DeepCopy() | ||
|
|
||
| // create configmap | ||
| testConfigMapName := "test-configmap-" + randSeq(5) | ||
|
|
||
| err = createAnnotatedCABundleInjectionConfigMap(adminClient, testConfigMapName, ns.Name) | ||
| if err != nil { | ||
| t.Fatalf("error creating annotated configmap: %v", err) | ||
| } | ||
|
|
||
| configmap, err := pollForCABundleInjectionConfigMapWithReturn(adminClient, testConfigMapName, ns.Name) | ||
| if err != nil { | ||
| t.Fatalf("error fetching ca bundle injection configmap: %v", err) | ||
| } | ||
| configmapCopy := configmap.DeepCopy() | ||
| err = checkConfigMapCABundleInjectionData(adminClient, testConfigMapName, ns.Name) | ||
| if err != nil { | ||
| t.Fatalf("error when checking ca bundle injection configmap: %v", err) | ||
| } | ||
|
|
||
| // delete ca secret | ||
| err = adminClient.CoreV1().Secrets("openshift-service-ca").Delete(context.TODO(), "signing-key", metav1.DeleteOptions{}) | ||
| if err != nil { | ||
| t.Fatalf("error deleting signing key: %v", err) | ||
| } | ||
|
|
||
| // make sure it's recreated | ||
| err = pollForCARecreation(adminClient) | ||
| if err != nil { | ||
| t.Fatalf("signing key was not recreated: %v", err) | ||
| } | ||
|
|
||
| err = pollForConfigMapChange(t, adminClient, configmapCopy, api.InjectionDataKey) | ||
| if err != nil { | ||
| t.Fatalf("configmap bundle did not change: %v", err) | ||
| } | ||
|
|
||
| err = pollForSecretChangeGinkgo(t, adminClient, secretCopy, v1.TLSCertKey, v1.TLSPrivateKeyKey) | ||
| if err != nil { | ||
| t.Fatalf("secret cert did not change: %v", err) | ||
| } | ||
| if err := pollForSecretChangeGinkgo(t, adminClient, headlessSecretCopy); err != nil { | ||
| t.Fatalf("headless secret cert did not change: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix false-positive checks for configmap and headless secret changes.
configmapCopy is captured before ensuring injected data exists, so the post-rotation check can succeed even if the bundle didn’t change. Also, pollForSecretChangeGinkgo is called with no keys for the headless secret, which makes it return immediately. Both weaken the test’s assertion.
Proposed fix
configmap, err := pollForCABundleInjectionConfigMapWithReturn(adminClient, testConfigMapName, ns.Name)
if err != nil {
t.Fatalf("error fetching ca bundle injection configmap: %v", err)
}
- configmapCopy := configmap.DeepCopy()
err = checkConfigMapCABundleInjectionData(adminClient, testConfigMapName, ns.Name)
if err != nil {
t.Fatalf("error when checking ca bundle injection configmap: %v", err)
}
+ configmap, err = adminClient.CoreV1().ConfigMaps(ns.Name).Get(context.TODO(), testConfigMapName, metav1.GetOptions{})
+ if err != nil {
+ t.Fatalf("error fetching ca bundle injection configmap: %v", err)
+ }
+ configmapCopy := configmap.DeepCopy()
@@
- if err := pollForSecretChangeGinkgo(t, adminClient, headlessSecretCopy); err != nil {
+ if err := pollForSecretChangeGinkgo(t, adminClient, headlessSecretCopy, v1.TLSCertKey, v1.TLSPrivateKeyKey); err != nil {
t.Fatalf("headless secret cert did not change: %v", err)
}🤖 Prompt for AI Agents
In `@test/e2e/e2e.go` around lines 1230 - 1317, The test captures configmapCopy
before verifying the injected CA data and calls pollForSecretChangeGinkgo for
the headless secret with no keys (which returns immediately); fix by moving the
configmapCopy = configmap.DeepCopy() to after
checkConfigMapCABundleInjectionData(adminClient, testConfigMapName, ns.Name) so
the baseline includes injected data, and call pollForSecretChangeGinkgo(t,
adminClient, headlessSecretCopy, v1.TLSCertKey, v1.TLSPrivateKeyKey) (same keys
used for the regular secret) so the headless secret change is actually
validated; locate symbols pollForServiceServingSecretWithReturn, configmapCopy,
checkConfigMapCABundleInjectionData, pollForConfigMapChange, headlessSecretCopy,
and pollForSecretChangeGinkgo to make the edits.
wangke19
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@wangke19: This pull request references CNTRLPLANE-2549 which is a valid jira issue. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
wangke19
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
/test e2e-aws-operator-serial-ote |
2 similar comments
|
/test e2e-aws-operator-serial-ote |
|
/test e2e-aws-operator-serial-ote |
Move headless-stateful-serving-cert-secret-delete-data context to correct position. It should run after serving-cert-secret-delete-data and before ca-bundle tests. This fixes the test execution sequence to match e2e_test.go order: 1. serving-cert-secret-delete-data 2. headless-stateful-serving-cert-secret-delete-data 3. ca-bundle-injection-configmap 4. ca-bundle-injection-configmap-update 5. vulnerable-legacy-ca-bundle-injection-configmap 6. metrics 7. refresh-CA
Add g.Ordered decorator to Ginkgo Describe block to guarantee tests run in declaration order. Without g.Ordered, Ginkgo randomizes test execution order by default, even with Parallelism: 1. This causes OTE tests to fail because the tests have state dependencies and must run in the exact order defined in e2e_test.go. The g.Ordered decorator ensures tests execute in this sequence: 1. serving-cert-annotation 2. serving-cert-secret-modify-bad-tlsCert 3. serving-cert-secret-add-data 4. serving-cert-secret-delete-data 5. headless-stateful-serving-cert-secret-delete-data 6. ca-bundle-injection-configmap 7. ca-bundle-injection-configmap-update 8. vulnerable-legacy-ca-bundle-injection-configmap 9. metrics 10. refresh-CA This matches the execution order in traditional Go tests.
|
@wangke19: This pull request references CNTRLPLANE-2549 which is a valid jira issue. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@wangke19: The following tests 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. |
2 participants
Summary
Migrates the
refresh-CAtest to the OTE (OpenShift Test Extended) Ginkgo framework while maintaining dual-compatibility with traditional Go tests.This PR continues the systematic migration of e2e tests to support OTE test discovery, following the pattern established in previous PRs (#297-#305).
Critical Fix: Added g.Ordered for Test Execution Order
Problem: OTE's
BuildExtensionTestSpecsFromOpenShiftGinkgoSuite()does not preserveg.Orderedorg.Serialdecorators. Withoutg.Ordered, Ginkgo randomizes test execution order by default, even withParallelism: 1in the suite configuration.Impact: Tests with state dependencies fail in OTE CI because they run in random order instead of the intended sequential order.
Solution: Added
g.Ordereddecorator to the Describe block to guarantee tests execute in declaration order, matching the execution sequence in traditional Go tests.Changes
test/e2e/e2e.go
g.Ordereddecorator to enforce sequential test executionheadless-stateful-serving-cert-secret-delete-datato correct position (set .travis.yml golang to 1.9 #5, afterserving-cert-secret-delete-data)refresh-CA- Tests CA regeneration when deleted and recreatedtesting.TBinterface:testRefreshCA()- Verifies serving certs and configmaps update when CA secret is deleted and recreatedpollForCABundleInjectionConfigMapWithReturn()- Polls for CA bundle injection configmappollForCARecreation()- Polls for CA secret recreation after deletiontest/e2e/e2e_test.go
pollForCABundleInjectionConfigMapWithReturn()pollForCARecreation()Test Execution Order (Guaranteed by g.Ordered)
This order matches the execution sequence in
e2e_test.goand ensures state dependencies are preserved.Testing
Both test frameworks work correctly:
go test -run "^TestE2E$/^refresh-CA$"service-ca-operator-tests-ext run-test "refresh-CA"Technical Details
Why g.Ordered is Required:
Parallelism: 1only means "run one test at a time", NOT "run in declaration order"g.Orderedexplicitly enforces declaration order for all nested Context/It blocksRelated
g.Ordereddecorator