NO-JIRA: Add e2e test for authorization cache race condition#31247
NO-JIRA: Add e2e test for authorization cache race condition#31247sanchezl wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a Ginkgo end-to-end concurrency test that runs many concurrent Projects().List() readers while RoleBindings are rapidly created/deleted, and includes helpers to detect apiserver restarts and emit crash-log evidence. ChangesAuth Cache Race Condition Test
Sequence Diagram(s)sequenceDiagram
participant Test
participant ProjectsAPI
participant Authorization
participant OpenShiftAPIServer
Test->>ProjectsAPI: Create namespaces and grant user view access
Test->>OpenShiftAPIServer: Record baseline pod restart counts
par Concurrent Readers
ProjectsAPI->>OpenShiftAPIServer: Repeated Projects().List() queries
and Concurrent Writer
Authorization->>OpenShiftAPIServer: Create/Delete RoleBindings churn
end
Test->>OpenShiftAPIServer: Verify pod restart counts unchanged
Test->>ProjectsAPI: Verify final Projects().List() contains expected namespaces
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sanchezl The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
sanchezl
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@sanchezl: This pull request explicitly references no 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/extended/project/project.go (1)
709-716: 💤 Low valueConsider strengthening the pod disappearance check.
When a baseline pod no longer exists, the code logs and continues. However, if a pod crashes and is replaced during the 30-second window, this would manifest as the old pod disappearing and a new pod appearing (with a fresh restart count of 0). The current logic would miss this scenario.
For more robust crash detection, consider also checking if
currentRestartscontains pods that weren't inbaselineRestarts, which could indicate a replacement due to crash.🛡️ Suggested enhancement
g.By("checking openshift-apiserver pods did not restart") currentRestarts := getPodRestartCounts(ctx, oc, "openshift-apiserver") for podName, baseline := range baselineRestarts { current, exists := currentRestarts[podName] if !exists { - framework.Logf("pod %s no longer exists (may have been rescheduled)", podName) - continue + // Pod disappeared - could indicate crash with pod replacement + g.Fail(fmt.Sprintf("pod %s no longer exists (possible crash and replacement)", podName)) } o.Expect(current).To(o.Equal(baseline), fmt.Sprintf("pod %s restarted during test (before=%d, after=%d)", podName, baseline, current)) } +// Check for new pods that weren't in baseline (indicates replacement) +for podName := range currentRestarts { + if _, existed := baselineRestarts[podName]; !existed { + framework.Logf("warning: new pod %s appeared during test (possible replacement after crash)", podName) + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/project/project.go` around lines 709 - 716, The existing loop over baselineRestarts misses cases where a pod was replaced (old pod gone, new pod present with a fresh restart count); after the current loop, add a complementary check that iterates currentRestarts and for each podName not present in baselineRestarts flag/fail the test (use the same o.Expect or framework.Logf pattern) with a clear message like "pod %s appeared during test (possible replacement/crash)"; reference the maps baselineRestarts and currentRestarts and place this new iteration after the existing for podName, baseline := range baselineRestarts block so replacements are detected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/extended/project/project.go`:
- Around line 709-716: The existing loop over baselineRestarts misses cases
where a pod was replaced (old pod gone, new pod present with a fresh restart
count); after the current loop, add a complementary check that iterates
currentRestarts and for each podName not present in baselineRestarts flag/fail
the test (use the same o.Expect or framework.Logf pattern) with a clear message
like "pod %s appeared during test (possible replacement/crash)"; reference the
maps baselineRestarts and currentRestarts and place this new iteration after the
existing for podName, baseline := range baselineRestarts block so replacements
are detected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b19ec8e3-a5d2-4190-9ef0-71f686996029
📒 Files selected for processing (1)
test/extended/project/project.go
|
Scheduling required tests: |
|
/pipeline required |
|
Scheduling required tests: |
|
/retest required |
sanchezl
force-pushed
the
auth-cache-race-e2e
branch
from
56c8f1f to
e8261c7
Compare
|
/test all |
sanchezl
force-pushed
the
auth-cache-race-e2e
branch
from
e8261c7 to
1ccd213
Compare
|
/test all |
|
Scheduling required tests: |
Stress test concurrent project List and Watch requests during sustained RBAC churn to surface races in the openshift-apiserver authorization cache.
sanchezl
force-pushed
the
auth-cache-race-e2e
branch
from
1ccd213 to
0785aeb
Compare
|
Scheduling required tests: |
|
@sanchezl: 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. |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 0785aeb
New tests seen in this PR at sha: 0785aeb
|
|
As expected, Once openshift/openshift-apiserver#642 is merged and lands in the payload, the test should pass and this PR can be merged. |
2 participants
NO-JIRA:
Summary
TestProjectAuthCacheRaceConditione2e test that exercises concurrentProjects().List()calls under RoleBinding churn to detect crashes, API errors, and latency regressions caused by unsafe map access inAuthorizationCacheTest plan
Summary by CodeRabbit