CNTRLPLANE-2499:test/e2e: migrate ca-bundle-injection-configmap-update test for OTE compatibility #302
Conversation
openshift-merge-robot
added
the
needs-rebase
WalkthroughMoves CA bundle injection test flows and helpers into Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (5)
Comment |
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: 0
🧹 Nitpick comments (2)
test/e2e/e2e.go (1)
430-431: Unnecessary variable initialization.The
ok := trueassignment is immediately overwritten on the next line.♻️ Suggested simplification
- ok := true - _, ok = cm.Data[api.InjectionDataKey] + _, ok := cm.Data[api.InjectionDataKey]test/e2e/e2e_test.go (1)
1081-1083: Missing OTE migration comment.The
ca-bundle-injection-configmap-updatetest is also duplicated in the OTE framework but lacks the explanatory comment present in other migrated tests.📝 Suggested comment addition
// test updated data in ca bundle injection configmap will be stomped on + // NOTE: This test is also available in the OTE framework (test/e2e/e2e.go). + // This duplication is temporary until we fully migrate to OTE and validate the new e2e jobs. + // Eventually, all tests will run only through the OTE framework. t.Run("ca-bundle-injection-configmap-update", func(t *testing.T) { testCABundleInjectionConfigMapUpdate(t) })
📜 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_test.gotest/e2e/e2e.go
🧬 Code graph analysis (1)
test/e2e/e2e.go (1)
pkg/controller/api/api.go (2)
InjectionDataKey(29-29)InjectCABundleAnnotationName(27-27)
🔇 Additional comments (9)
test/e2e/e2e.go (8)
57-67: LGTM!The new Ginkgo test contexts for CA bundle injection follow the established patterns in the file. The test names and descriptions are clear and consistent with other tests.
367-395: LGTM!The test function follows the established patterns in the codebase with proper namespace creation, cleanup via defer, and sequential validation steps.
397-407: LGTM!Clean helper function that creates an annotated ConfigMap for CA bundle injection testing.
409-420: LGTM!The polling function correctly waits for the ConfigMap to exist. The data validation is intentionally separated into
checkConfigMapCABundleInjectionData, which aligns with the test flow.
438-443: LGTM!Proper defensive handling of nil annotations map.
445-483: LGTM!The test correctly verifies that the controller stomps on user modifications to CA bundle injection ConfigMaps, which is the expected behavior being tested.
485-501: LGTM!The function correctly adds test data and waits for the controller to remove it, validating the stomp behavior.
503-517: LGTM!The polling function correctly detects when specified keys have changed from their original values.
test/e2e/e2e_test.go (1)
1073-1078: LGTM!The migration comment clearly explains the temporary duplication during OTE transition. The refactoring to call the shared
testCABundleInjectionConfigMapfunction aligns with the PR objectives.
wangke19
force-pushed
the
ote-migrate-ca-bundle-injection-configmap-update
branch
from
46535b8 to
e02a309
Compare
openshift-merge-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/e2e/e2e.go (3)
376-391: Unresolved git merge conflicts will cause compilation failure.The file contains unresolved git merge conflict markers (
<<<<<<< HEAD,=======,>>>>>>> 31e28d9b7) that will prevent compilation. This aligns with the "needs-rebase" label on the PR.Please resolve the merge conflicts by rebasing on top of PR #301 as mentioned in the PR description.
403-501: Additional unresolved merge conflict block.This section contains another set of merge conflict markers. The conflict appears to be between
testServingCertSecretDeleteDataand its supporting functions from HEAD versus the newtestCABundleInjectionConfigMapand helper functions from the feature branch.
506-622: Third unresolved merge conflict block.This conflict block includes
pollForResourceGinkgoand related functions from HEAD conflicting withcheckConfigMapCABundleInjectionData,setInjectionAnnotation, and other helper functions from the feature branch.
📜 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 (2)
pkg/version/version.go (1)
Get(24-32)pkg/controller/api/api.go (2)
InjectionDataKey(29-29)InjectCABundleAnnotationName(27-27)
🔇 Additional comments (3)
test/e2e/e2e_test.go (1)
1045-1055: LGTM!The refactoring to use shared test functions (
testCABundleInjectionConfigMapandtestCABundleInjectionConfigMapUpdate) is clean and consistent with the OTE migration pattern used for other tests in this file. The explanatory comments about the temporary duplication are helpful.test/e2e/e2e.go (2)
65-76: LGTM!The new Ginkgo context blocks for
ca-bundle-injection-configmapandca-bundle-injection-configmap-updateare properly structured and follow the same pattern as the existing test contexts.
625-697: Review pending conflict resolution.The
testCABundleInjectionConfigMapUpdate,editConfigMapCABundleInjectionData, andpollForConfigMapChangefunctions appear correctly structured, but a thorough review should be done after the merge conflicts are resolved to ensure the final code is correct.Once conflicts are resolved, verify:
editConfigMapCABundleInjectionDatacorrectly adds extraneous data to trigger the controller's reconciliationpollForConfigMapChangeproperly detects when the controller removes the extraneous key
wangke19
force-pushed
the
ote-migrate-ca-bundle-injection-configmap-update
branch
from
e02a309 to
e48abe7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/e2e/e2e.go (1)
615-616: Simplify redundant variable initialization.The
ok := trueassignment is immediately overwritten and serves no purpose.♻️ Suggested simplification
- ok := true - _, ok = cm.Data[api.InjectionDataKey] + _, ok := cm.Data[api.InjectionDataKey]
📜 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 (2)
InjectionDataKey(29-29)InjectCABundleAnnotationName(27-27)
🔇 Additional comments (9)
test/e2e/e2e.go (8)
65-76: LGTM!The Ginkgo test contexts follow the established pattern in the file, consistently using
g.GinkgoTB()for dual-framework compatibility.
552-580: LGTM!The test function follows the established pattern, properly manages cleanup with defer, and uses consistent error handling.
582-592: LGTM!Clean helper function following the established pattern.
594-605: LGTM!Polling pattern is consistent with
pollForServiceServingSecret.
623-628: LGTM!Correctly uses the
api.InjectCABundleAnnotationNameconstant and handles nil annotations map.
630-668: LGTM!The test correctly validates that the controller restores the CA bundle after user modifications. The flow is clear and follows the established test patterns.
670-686: LGTM!The function correctly follows the pattern of
editServingSecretDataGinkgo, adding test data and waiting for the controller to restore the expected state.
688-702: LGTM!Polling function is consistent with
pollForSecretChangeGinkgo, using the same intervals and testing.TB for dual-framework compatibility.test/e2e/e2e_test.go (1)
1045-1055: LGTM!The refactoring correctly delegates to shared helper functions for dual-framework compatibility. The migration comments clearly document the temporary duplication state.
wangke19
force-pushed
the
ote-migrate-ca-bundle-injection-configmap-update
branch
from
e48abe7 to
7c0c72b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @test/e2e/e2e.go:
- Around line 597-608: The pollForCABundleInjectionConfigMap function currently
only checks for ConfigMap existence, causing a race since the test creates the
ConfigMap itself; update pollForCABundleInjectionConfigMap to mirror
pollForConfigMapCAInjection behavior by polling until the ConfigMap both exists
and contains the injected CA bundle data (use the same logic as
checkConfigMapCABundleInjectionData to inspect the ConfigMap data fields),
returning true only when the CA bundle is present; reference the existing
pollForConfigMapCAInjection and checkConfigMapCABundleInjectionData functions to
copy the verification steps and error handling so the poll waits for injection
rather than mere creation.
📜 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 (2)
InjectionDataKey(29-29)InjectCABundleAnnotationName(27-27)
🔇 Additional comments (5)
test/e2e/e2e.go (4)
65-76: LGTM!The Ginkgo test wrappers are correctly structured and follow the established pattern used by other tests in this file.
554-583: Test structure looks good, pending the poll fix above.The test function correctly follows the pattern: create namespace, create annotated ConfigMap, poll, verify, and cleanup via defer. Once the polling race condition is addressed, this implementation will be solid.
633-671: Update test logic is correct.The test properly verifies that the controller "stomps on" changes by adding extra data and polling for its removal, then re-validating the ConfigMap has exactly one key with the injected CA bundle.
673-705: Helper functions are well-structured.Both
editConfigMapCABundleInjectionDataandpollForConfigMapChangecorrectly implement the mutation and observation pattern for testing controller reconciliation behavior.test/e2e/e2e_test.go (1)
1045-1055: LGTM!The refactoring correctly delegates to the shared test functions in e2e.go. The migration comments clearly explain the temporary duplication during OTE transition.
| func pollForCABundleInjectionConfigMap(client *kubernetes.Clientset, configMapName, namespace string) error { | ||
| return wait.PollImmediate(time.Second, 10*time.Second, func() (bool, error) { | ||
| _, err := client.CoreV1().ConfigMaps(namespace).Get(context.TODO(), configMapName, metav1.GetOptions{}) | ||
| if err != nil && errors.IsNotFound(err) { | ||
| return false, nil | ||
| } | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| return true, nil | ||
| }) | ||
| } |
There was a problem hiding this comment.
Race condition: poll should wait for CA bundle injection, not just ConfigMap existence.
Since the test creates the ConfigMap itself (line 569), it exists immediately. This poll returns right away before the controller has time to inject the CA bundle. The subsequent checkConfigMapCABundleInjectionData call may fail intermittently.
Compare to the existing pollForConfigMapCAInjection in e2e_test.go (lines 169-188) which correctly polls for both existence AND injection data.
🛠️ Suggested fix
func pollForCABundleInjectionConfigMap(client *kubernetes.Clientset, configMapName, namespace string) error {
return wait.PollImmediate(time.Second, 10*time.Second, func() (bool, error) {
- _, err := client.CoreV1().ConfigMaps(namespace).Get(context.TODO(), configMapName, metav1.GetOptions{})
+ cm, err := client.CoreV1().ConfigMaps(namespace).Get(context.TODO(), configMapName, metav1.GetOptions{})
if err != nil && errors.IsNotFound(err) {
return false, nil
}
if err != nil {
return false, err
}
+ if len(cm.Data) != 1 {
+ return false, nil
+ }
+ _, ok := cm.Data[api.InjectionDataKey]
+ if !ok {
+ return false, nil
+ }
return true, nil
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func pollForCABundleInjectionConfigMap(client *kubernetes.Clientset, configMapName, namespace string) error { | |
| return wait.PollImmediate(time.Second, 10*time.Second, func() (bool, error) { | |
| _, err := client.CoreV1().ConfigMaps(namespace).Get(context.TODO(), configMapName, metav1.GetOptions{}) | |
| if err != nil && errors.IsNotFound(err) { | |
| return false, nil | |
| } | |
| if err != nil { | |
| return false, err | |
| } | |
| return true, nil | |
| }) | |
| } | |
| func pollForCABundleInjectionConfigMap(client *kubernetes.Clientset, configMapName, namespace string) error { | |
| return wait.PollImmediate(time.Second, 10*time.Second, func() (bool, error) { | |
| cm, err := client.CoreV1().ConfigMaps(namespace).Get(context.TODO(), configMapName, metav1.GetOptions{}) | |
| if err != nil && errors.IsNotFound(err) { | |
| return false, nil | |
| } | |
| if err != nil { | |
| return false, err | |
| } | |
| if len(cm.Data) != 1 { | |
| return false, nil | |
| } | |
| _, ok := cm.Data[api.InjectionDataKey] | |
| if !ok { | |
| return false, nil | |
| } | |
| return true, nil | |
| }) | |
| } |
🤖 Prompt for AI Agents
In @test/e2e/e2e.go around lines 597 - 608, The
pollForCABundleInjectionConfigMap function currently only checks for ConfigMap
existence, causing a race since the test creates the ConfigMap itself; update
pollForCABundleInjectionConfigMap to mirror pollForConfigMapCAInjection behavior
by polling until the ConfigMap both exists and contains the injected CA bundle
data (use the same logic as checkConfigMapCABundleInjectionData to inspect the
ConfigMap data fields), returning true only when the CA bundle is present;
reference the existing pollForConfigMapCAInjection and
checkConfigMapCABundleInjectionData functions to copy the verification steps and
error handling so the poll waits for injection rather than mere creation.
7c0c72b to
e7c4bf5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/e2e/e2e_test.go (1)
1052-1055: Add OTE migration comment for consistency.This test block is missing the OTE migration comment that's present on all other migrated tests in this file. Since this is the primary test being migrated per the PR title, adding the comment would maintain documentation consistency.
Suggested change
// test updated data in ca bundle injection configmap will be stomped on + // NOTE: This test is also available in the OTE framework (test/e2e/e2e.go). + // This duplication is temporary until we fully migrate to OTE and validate the new e2e jobs. + // Eventually, all tests will run only through the OTE framework. t.Run("ca-bundle-injection-configmap-update", func(t *testing.T) { testCABundleInjectionConfigMapUpdate(t) })
📜 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
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/e2e.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_test.go
🔇 Additional comments (1)
test/e2e/e2e_test.go (1)
1045-1050: LGTM!The OTE migration comment and delegation to the shared helper function follow the established pattern used by other migrated tests in this file.
…ompatibility
This commit migrates the ca-bundle-injection-configmap-update test to work with
both standard Go testing and OTE (OpenShift Tests Extension) framework.
Changes:
- Add Ginkgo wrapper in e2e.go for OTE test discovery (6 lines)
- Extract inline test code into shared testCABundleInjectionConfigMapUpdate
function (39 lines)
- Move editConfigMapCABundleInjectionData and pollForConfigMapChange helper
functions from e2e_test.go to e2e.go (35 lines total)
- Change helper function signatures from *testing.T to testing.TB for dual
framework compatibility
- Refactor e2e_test.go to call shared function instead of inline code (-66 lines)
Net change: +15 lines
- The +15 line overhead is required because:
1. Ginkgo wrapper is necessary for OTE test discovery (6 lines)
2. Shared test function allows both frameworks to use same logic (39 lines)
3. Helper functions must be in e2e.go (not e2e_test.go) because *_test.go
files are only compiled during test builds, not when building the OTE
binary (35 lines)
This migration builds on top of the ca-bundle-injection-configmap migration
(PR openshift#301) and reuses the helper functions moved in that PR.
Both test frameworks continue to work:
- Standard Go test: go test -run "^TestE2E$/^ca-bundle-injection-configmap-update$"
- OTE: service-ca-operator-tests-ext list tests (test discovered)
wangke19
force-pushed
the
ote-migrate-ca-bundle-injection-configmap-update
branch
from
e7c4bf5 to
a82891f
Compare
|
Related CI job e2e-aws-operator-serial-ote has been run passed. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gangwgr, 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 |
|
@wangke19: all tests passed! 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. |
wangke19
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@wangke19: This pull request references CNTRLPLANE-2499 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. |
|
/verified by CI test |
openshift-ci-robot
added
the
verified
|
@wangke19: This PR has been marked as verified by 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. |
4 participants
Summary
This PR migrates the
ca-bundle-injection-configmap-updatetest to work with both standard Go testing and OTE (OpenShift Tests Extension) framework. This test validates that the service-ca-operator will stomp on updated data in CA bundle injection configmaps.Changes
e2e.gofor OTE test discoverytestCABundleInjectionConfigMapUpdatefunctioneditConfigMapCABundleInjectionDataandpollForConfigMapChangehelper functions frome2e_test.gotoe2e.go*testing.Ttotesting.TBfor dual framework compatibilitye2e_test.goto call shared function instead of inline codeLine Count Explanation
Net change: +15 lines (+81 in e2e.go, -66 in e2e_test.go)
This overhead is required because:
e2e.go(note2e_test.go) because*_test.gofiles are only compiled during test builds, not when building the OTE binaryTest Plan
make build./service-ca-operator-tests-ext list tests