Revert "OCPBUGS-84534: fix concurrent map race in project authorization cache"

[bertinatto]

Reverts #642.

Just checking if this caused failures in TP clusters.

Summary by CodeRabbit

• Refactor

  • Simplified authorization cache synchronization logic and data structure management, eliminating intermediate wrapper layers and reducing overall complexity.

• Tests

  • Removed obsolete benchmarks related to deprecated caching patterns to streamline the test suite.

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[openshift-ci-robot]

@bertinatto: This pull request references Jira Issue OCPBUGS-84534, which is invalid:

• expected the bug to be in one of the following states: NEW, ASSIGNED, POST, but it is Verified instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Reverts #642.

Just checking if this caused failures in TP clusters.

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]

Walkthrough

AuthorizationCache refactors away atomic pointer-based store swapping in favor of direct store fields, and removes copy-on-write branching from all synchronization paths. Synchronization now always updates subject records in-place. Related benchmarks and race-condition tests are removed.

Changes

AuthorizationCache Store and Sync Refactoring

Layer / File(s)	Summary
Cache store structure and imports pkg/project/auth/cache.go	AuthorizationCache struct replaces atomic.Pointer[authorizationCacheStores] with direct fields reviewRecordStore, userSubjectRecordStore, and groupSubjectRecordStore; sync/atomic import is removed.
Constructor initialization pkg/project/auth/cache.go	NewAuthorizationCache directly initializes the three store fields on the struct instance via cache.NewStore().
Synchronization methods pkg/project/auth/cache.go	syncHandler, synchronizeNamespaces, synchronizePolicies, and synchronizeRoleBindings remove the copyOnWrite parameter; synchronize() no longer snapshots atomic pointers or swaps stores, instead using local references and updating instance fields directly after full rebuild.
Subject record mutation helpers pkg/project/auth/cache.go	syncRequest contract removes copyOnWrite parameter; deleteNamespaceFromSubjects and addSubjectsToNamespace eliminate copy-on-write branching and unconditionally mutate records in-store.
List method direct store access pkg/project/auth/cache.go	List() reads subject-to-namespace mappings directly from instance store fields instead of snapshotting an atomic pointer.
Test cleanup and atomic/race removal pkg/project/auth/cache_test.go	Removes sync import and deletes benchmarks and race tests: BenchmarkFullCacheInvalidation, BenchmarkIncrementalSyncDuplicateSubjects, TestAuthorizationCacheRace, BenchmarkAddSubjectsToNamespace, and helper type fakeVersioner.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• openshift/openshift-apiserver#642: Prior PR that introduced the atomic-swap and copy-on-write patterns now being removed by this refactoring.

Suggested reviewers

• deads2k
• derekwaynecarr

🚥 Pre-merge checks | ✅ 15

✅ Passed checks (15 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: reverting a previous bugfix commit. It clearly states what is being reverted and provides the bugfix reference (OCPBUGS-84534).
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	The modified test files contain only standard Go tests (func Test*), not Ginkgo tests. No Ginkgo test names to evaluate against the stability requirement.
Test Structure And Quality	✅ Passed	The check requires reviewing Ginkgo test code, but the PR only modifies standard Go tests (testing.T), not Ginkgo tests. The check is not applicable.
Microshift Test Compatibility	✅ Passed	PR reverts cache implementation; only modifies unit tests (removes them, doesn't add), not Ginkgo e2e tests. Check applicability requires new Ginkgo e2e tests—not present here.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	No new Ginkgo e2e tests are added in this PR. The changes only involve unit tests in the Go testing framework within cache.go and cache_test.go, with tests being removed rather than added.
Topology-Aware Scheduling Compatibility	✅ Passed	This PR modifies only internal authorization cache logic, not deployment manifests, operator code, or controllers. No scheduling constraints or topology-aware configurations are affected.
Ote Binary Stdout Contract	✅ Passed	PR reverts authorization cache fix, introducing klog.V(5).Info in library code. This is not process-level code (main/init/TestMain/suite setup) so does not violate OTE Binary Stdout Contract.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	PR adds no new Ginkgo e2e tests; only standard Go unit tests removed. Check applies to new Ginkgo e2e tests only.
No-Weak-Crypto	✅ Passed	No weak cryptography found. PR contains authorization cache refactoring with standard Go/Kubernetes libraries; no MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB, or custom crypto implementations present.
Container-Privileges	✅ Passed	PR modifies only Go source files (cache.go and cache_test.go), not container/K8s manifests. No privileged settings or containers found.
No-Sensitive-Data-In-Logs	✅ Passed	This PR reverts architectural changes without introducing new logging statements. Existing logging statements contain no new sensitive data exposure.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch revert-642-bugfix/project-auth-cache-race

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign derekwaynecarr for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• pkg/project/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[bertinatto]

/payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview
/payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-gcp-ovn-techpreview
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-vsphere-ovn-techpreview

[openshift-ci]

@bertinatto: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

• periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview
• periodic-ci-openshift-release-main-ci-5.0-e2e-gcp-ovn-techpreview
• periodic-ci-openshift-release-main-nightly-5.0-e2e-vsphere-ovn-techpreview

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/ea3312a0-61d5-11f1-9ac7-7cc707b143ed-0

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

pkg/project/auth/cache.go (1)

437-460: ⚠️ Potential issue | 🔴 Critical

Critical: cache race from mutating subjectRecord.namespaces while List() iterates + unsafe store swapping

AuthorizationCache.Run() starts synchronize() in a background goroutine, and synchronize() reuses the live cache.Store instances (Lines ~437-455). On the incremental path, deleteNamespaceFromSubjects / addSubjectsToNamespace mutate subjectRecord.namespaces in place (Lines ~603-624), where namespaces is a sets.String (map-backed). List() simultaneously reads the same objects and iterates via subjectRecord.namespaces.List() (Lines ~514-524), which can trigger concurrent map iteration and map write and data races. This violates the k8s ThreadSafeStore contract (“you must not modify anything returned by Get or List”; returned pointers are not copied).

On full invalidation, synchronize() also swaps ac.userSubjectRecordStore / ac.groupSubjectRecordStore / ac.reviewRecordStore via plain field assignment (Lines ~458-460) while List() reads them without synchronization, introducing a separate data race on the store references.

Restore copy-on-write/atomic snapshot semantics (clone subjectRecord/namespaces on updates, and swap snapshots atomically), or guard synchronize()/List() with shared synchronization so readers never observe concurrently-mutated objects.

🤖 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 `@pkg/project/auth/cache.go` around lines 437 - 460, The synchronize() path
mutates shared subjectRecord.namespaces and swaps store fields unsafely causing
concurrent map-iteration and store-reference races; fix by making updates
copy-on-write and swapping snapshots atomically: when modifying subjects (in
deleteNamespaceFromSubjects and addSubjectsToNamespace) clone the subjectRecord
and create a new namespaces sets.String copy before modifying so List() never
sees in-place mutations, and for full invalidation (invalidateCache) stop
assigning ac.userSubjectRecordStore / ac.groupSubjectRecordStore /
ac.reviewRecordStore directly — instead publish the rebuilt stores via a single
atomic snapshot (e.g. an atomic.Value or a protected struct) or hold a RWMutex
around List() and synchronize() so readers see a consistent snapshot; ensure
AuthorizationCache.Run, synchronize(), and List() use the same snapshot
mechanism.

🤖 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.

Outside diff comments:
In `@pkg/project/auth/cache.go`:
- Around line 437-460: The synchronize() path mutates shared
subjectRecord.namespaces and swaps store fields unsafely causing concurrent
map-iteration and store-reference races; fix by making updates copy-on-write and
swapping snapshots atomically: when modifying subjects (in
deleteNamespaceFromSubjects and addSubjectsToNamespace) clone the subjectRecord
and create a new namespaces sets.String copy before modifying so List() never
sees in-place mutations, and for full invalidation (invalidateCache) stop
assigning ac.userSubjectRecordStore / ac.groupSubjectRecordStore /
ac.reviewRecordStore directly — instead publish the rebuilt stores via a single
atomic snapshot (e.g. an atomic.Value or a protected struct) or hold a RWMutex
around List() and synchronize() so readers see a consistent snapshot; ensure
AuthorizationCache.Run, synchronize(), and List() use the same snapshot
mechanism.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: f0a7b0f3-b61b-488b-80d2-07329538b8ad

📥 Commits

Reviewing files that changed from the base of the PR and between 22f07f6 and 03ed637.

📒 Files selected for processing (2)

• pkg/project/auth/cache.go
• pkg/project/auth/cache_test.go

💤 Files with no reviewable changes (1)

• pkg/project/auth/cache_test.go

[openshift-ci-robot]

@bertinatto: This pull request references Jira Issue OCPBUGS-84534. The bug has been updated to no longer refer to the pull request using the external bug tracker.

Details

In response to this:

Reverts #642.

Just checking if this caused failures in TP clusters.

Summary by CodeRabbit

• Refactor

• Simplified authorization cache synchronization logic and data structure management, eliminating intermediate wrapper layers and reducing overall complexity.

• Tests

• Removed obsolete benchmarks related to deprecated caching patterns to streamline the test suite.

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.