Migrate aescbc to ote - #2115
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gangwgr 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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughDelegates AES‑CBC encryption test to a shared helper and adds a Ginkgo suite; rewrites the bound SA token e2e test to require continuous ClusterOperator stability, wait for SA token key propagation and kubelet refresh, and replace single pod-bounce with a retry-capable, rate‑limited bounce mechanism. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner
participant Cluster as ClusterOperators
participant ConfigMap as SA Token ConfigMap
participant Kubelet as Kubelets
participant Pods as Pods
Test->>Cluster: waitForExtendedClusterStability(5m)
Cluster-->>Test: sustained stable conditions
Test->>ConfigMap: waitForSATokenKeysOnAllNodes()
ConfigMap-->>Test: ConfigMap exists with keys and stable resourceVersion
Test->>Kubelet: sleep kubeletTokenRefreshGracePeriod (5m)
Kubelet-->>Test: grace period elapsed
rect rgba(200,100,100,0.5)
Test->>Pods: bounceCrashLoopingPodsWithRetry(timeout)
Pods-->>Test: list crash-looping pods with restart counts
Test->>Pods: delete eligible pods (namespace-prioritized, rate-limited, max-bounces)
Pods-->>Test: pods restart/recover
Test->>Pods: repeat until healthy or timeout
end
Test->>ConfigMap: verify ConfigMap rollout/cleanup
ConfigMap-->>Test: keys present on all nodes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/encryption_test.go`:
- Line 13: The call to testEncryptionTypeAESCBC(t) is undefined; add a
package-scope helper with the exact signature func testEncryptionTypeAESCBC(t
*testing.T) in the same package so the call compiles, or change the call to an
existing symbol if one already exists; implement the helper to accept
*testing.T, call t.Helper(), and perform the prior test logic (or delegate to
the existing test implementation) so the invocation resolves.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f2543868-b5ac-46d3-aece-7f5bb922866c
📒 Files selected for processing (1)
test/e2e/encryption_test.go
| TargetGRs: operatorencryption.DefaultTargetGRs, | ||
| AssertFunc: operatorencryption.AssertSecretsAndConfigMaps, | ||
| }) | ||
| testEncryptionTypeAESCBC(t) |
There was a problem hiding this comment.
Build blocker: undefined helper invocation.
Line 13 calls testEncryptionTypeAESCBC(t), but typecheck reports this symbol as undefined in package test/e2e. This will fail compilation of test/e2e.test. Please add/restore the helper in this package scope (or call the correct existing symbol).
🧰 Tools
🪛 golangci-lint (2.11.4)
[error] 13-13: : # github.com/openshift/cluster-kube-apiserver-operator/test/e2e [github.com/openshift/cluster-kube-apiserver-operator/test/e2e.test]
test/e2e/encryption_test.go:13:2: undefined: testEncryptionTypeAESCBC
(typecheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/encryption_test.go` at line 13, The call to
testEncryptionTypeAESCBC(t) is undefined; add a package-scope helper with the
exact signature func testEncryptionTypeAESCBC(t *testing.T) in the same package
so the call compiles, or change the call to an existing symbol if one already
exists; implement the helper to accept *testing.T, call t.Helper(), and perform
the prior test logic (or delegate to the existing test implementation) so the
invocation resolves.
a062ea3 to
d417fa6
Compare
openshift-ci
Bot
added
the
needs-rebase
d417fa6 to
59ef132
Compare
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/e2e/bound_sa_token.go (2)
250-276:require.NoErrorin deferred cleanup can mask the original test failure.Using
require.NoErrorinside a deferred function will callt.FailNow()if cleanup fails, which can mask the original test failure reason. Consider logging errors instead of failing, or usingassert.NoErrorwhich doesn't stop execution.♻️ Suggested approach for cleanup resilience
defer func() { t.Log("cleaning up: deleting public key configmap (triggers third rollout)") err := kubeClient.ConfigMaps(targetNamespace).Delete(ctx, tokenctl.PublicKeyConfigMapName, metav1.DeleteOptions{}) - require.NoError(t, err) + if err != nil && !apierrors.IsNotFound(err) { + t.Logf("cleanup warning: failed to delete configmap: %v", err) + return // Skip remaining cleanup steps + } t.Log("waiting for kube-apiserver pods to stabilize after cleanup rollout") err = libgotest.WaitForPodsToStabilizeOnTheSameRevision(t, kubeClient.Pods(targetNamespace), "apiserver=true", rolloutSuccessThreshold, rolloutSuccessInterval, rolloutPollInterval, rolloutTimeout) - require.NoError(t, err) + if err != nil { + t.Logf("cleanup warning: pods did not stabilize: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/bound_sa_token.go` around lines 250 - 276, The deferred cleanup currently uses require.NoError in the anonymous defer func (calls to require.NoError for kubeClient.ConfigMaps(...).Delete, WaitForPodsToStabilizeOnTheSameRevision, waitForSATokenKeysOnAllNodes, bounceCrashLoopingPodsWithRetry, waitForAllClusterOperatorsStable), which can call t.FailNow() and hide the original test failure; change those require.NoError calls to non-fatal handling (e.g., replace with assert.NoError from testify or log the error with t.Errorf/t.Logf) so cleanup errors are reported but do not abort/override the original test result, keeping the same calls and context names (defer func, kubeClient.ConfigMaps, WaitForPodsToStabilizeOnTheSameRevision, waitForSATokenKeysOnAllNodes, bounceCrashLoopingPodsWithRetry, waitForAllClusterOperatorsStable).
375-397: Consider using a set for priority namespace lookup efficiency.The priority namespace list is checked in an O(n) loop on lines 405-414 for each namespace. While acceptable for the current small list size, a map-based set would be cleaner.
♻️ Optional: Use a set for O(1) lookup
// Process namespaces in priority order: operators first, then infrastructure priorityNamespaces := []string{ "openshift-kube-apiserver-operator", // ... other namespaces } + prioritySet := make(map[string]struct{}, len(priorityNamespaces)) + for _, ns := range priorityNamespaces { + prioritySet[ns] = struct{}{} + } // First pass: bounce high-priority operator namespaces // ... // Second pass: bounce remaining openshift-* namespaces for _, ns := range namespaces.Items { if !strings.HasPrefix(ns.Name, "openshift-") { continue } - // Skip if already processed in priority pass - isPriority := false - for _, pns := range priorityNamespaces { - if ns.Name == pns { - isPriority = true - break - } - } - if isPriority { + if _, isPriority := prioritySet[ns.Name]; isPriority { continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/bound_sa_token.go` around lines 375 - 397, Replace the hard-coded slice priorityNamespaces with a map-based set (e.g., map[string]struct{}) or build a set from that slice and use the set for membership checks and iteration where you need O(1) lookups; update the loop that currently iterates priorityNamespaces (which calls bouncePodsInNamespace(ctx, t, kubeClient, nsName, bouncedPods, minTimeBetweenBounces)) to iterate over the set or use the set when testing namespace membership elsewhere in the file so lookups are constant-time while preserving the same namespaces and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/encryption.go`:
- Around line 20-29: The function testEncryptionTypeAESCBC currently performs an
unsafe type assertion on t (testing.TB) to *testing.T before calling
library.TestEncryptionTypeAESCBC; remove the cast and pass t directly to
library.TestEncryptionTypeAESCBC so the call uses the testing.TB interface (this
avoids panics when t is a ginkgo.GinkgoTBWrapper). Update the invocation in
testEncryptionTypeAESCBC to call library.TestEncryptionTypeAESCBC(t, ...)
without the type assertion.
---
Nitpick comments:
In `@test/e2e/bound_sa_token.go`:
- Around line 250-276: The deferred cleanup currently uses require.NoError in
the anonymous defer func (calls to require.NoError for
kubeClient.ConfigMaps(...).Delete, WaitForPodsToStabilizeOnTheSameRevision,
waitForSATokenKeysOnAllNodes, bounceCrashLoopingPodsWithRetry,
waitForAllClusterOperatorsStable), which can call t.FailNow() and hide the
original test failure; change those require.NoError calls to non-fatal handling
(e.g., replace with assert.NoError from testify or log the error with
t.Errorf/t.Logf) so cleanup errors are reported but do not abort/override the
original test result, keeping the same calls and context names (defer func,
kubeClient.ConfigMaps, WaitForPodsToStabilizeOnTheSameRevision,
waitForSATokenKeysOnAllNodes, bounceCrashLoopingPodsWithRetry,
waitForAllClusterOperatorsStable).
- Around line 375-397: Replace the hard-coded slice priorityNamespaces with a
map-based set (e.g., map[string]struct{}) or build a set from that slice and use
the set for membership checks and iteration where you need O(1) lookups; update
the loop that currently iterates priorityNamespaces (which calls
bouncePodsInNamespace(ctx, t, kubeClient, nsName, bouncedPods,
minTimeBetweenBounces)) to iterate over the set or use the set when testing
namespace membership elsewhere in the file so lookups are constant-time while
preserving the same namespaces and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ff70610d-6481-4e36-a0d3-f521fb8b180b
📒 Files selected for processing (3)
test/e2e/bound_sa_token.gotest/e2e/encryption.gotest/e2e/encryption_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/encryption_test.go
|
/pipeline required |
|
/pipeline-required |
|
/test e2e-gcp-operator-serial-ote |
|
/test e2e-gcp-operator-serial-ote |
1 similar comment
|
/test e2e-gcp-operator-serial-ote |
dcbcf83 to
6e6fbda
Compare
|
/test e2e-gcp-operator-serial-ote |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/bound_sa_token.go (1)
384-396: Consider extracting priority namespaces to a package-level constant.The priority namespace list is well-chosen for operator stability. For maintainability, consider extracting this to a
varat package level so it's easier to update and document.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/bound_sa_token.go` around lines 384 - 396, Extract the hard-coded priorityNamespaces slice into a package-level variable (e.g. var PriorityNamespaces = []string{...}) declared near the top of the test package and replace the local declaration in bound_sa_token.go with a reference to PriorityNamespaces; update any tests or functions that previously shadowed the local name to use the package-level symbol and add a brief comment/docstring explaining the purpose and ordering of PriorityNamespaces for maintainability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/bound_sa_token.go`:
- Around line 617-620: The API error branch in the polling loop leaves
lastResourceVersion and stableStart unchanged, which can let a prior partial
stability interval be counted toward the 30s requirement; update the error
handling inside the block that checks "if err != nil" (in the function that
polls for bound SA token stability in bound_sa_token.go) to reset
lastResourceVersion (e.g., set to empty string) and stableStart (e.g., zero
time) when an API error occurs, mirroring the behavior in
waitForExtendedClusterStability, and keep the existing log/return behavior.
---
Nitpick comments:
In `@test/e2e/bound_sa_token.go`:
- Around line 384-396: Extract the hard-coded priorityNamespaces slice into a
package-level variable (e.g. var PriorityNamespaces = []string{...}) declared
near the top of the test package and replace the local declaration in
bound_sa_token.go with a reference to PriorityNamespaces; update any tests or
functions that previously shadowed the local name to use the package-level
symbol and add a brief comment/docstring explaining the purpose and ordering of
PriorityNamespaces for maintainability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 472df73f-1d34-4ae9-914b-44b416ba85c1
📒 Files selected for processing (1)
test/e2e/bound_sa_token.go
| if err != nil { | ||
| t.Logf("failed to get configmap %s/%s: %v", namespace, configMapName, err) | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
Missing state reset on API error could incorrectly extend stability duration.
When an API error occurs, lastResourceVersion and stableStart are not reset. If the previous poll had partial stability progress, this error would be silently counted toward the 30-second stability requirement. Compare with waitForExtendedClusterStability which correctly resets stableStart on error (line 694).
🐛 Proposed fix
if err != nil {
t.Logf("failed to get configmap %s/%s: %v", namespace, configMapName, err)
+ lastResourceVersion = ""
+ stableStart = time.Time{}
return false, 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.
| if err != nil { | |
| t.Logf("failed to get configmap %s/%s: %v", namespace, configMapName, err) | |
| return false, nil | |
| } | |
| if err != nil { | |
| t.Logf("failed to get configmap %s/%s: %v", namespace, configMapName, err) | |
| lastResourceVersion = "" | |
| stableStart = time.Time{} | |
| return false, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/bound_sa_token.go` around lines 617 - 620, The API error branch in
the polling loop leaves lastResourceVersion and stableStart unchanged, which can
let a prior partial stability interval be counted toward the 30s requirement;
update the error handling inside the block that checks "if err != nil" (in the
function that polls for bound SA token stability in bound_sa_token.go) to reset
lastResourceVersion (e.g., set to empty string) and stableStart (e.g., zero
time) when an API error occurs, mirroring the behavior in
waitForExtendedClusterStability, and keep the existing log/return behavior.
|
/test e2e-gcp-operator-serial-ote |
6e6fbda to
9872fde
Compare
|
/test e2e-gcp-operator-serial-ote |
9872fde to
872d874
Compare
|
/test e2e-gcp-operator-serial-ote |
1 similar comment
|
/test e2e-gcp-operator-serial-ote |
|
@gangwgr: The following test 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. |
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. 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. |
1 participant
Summary by CodeRabbit