WIP: OCPBUGS-57474: Authorization Cache V2 #530

jacobsee · 2025-06-24T04:47:42Z

Move existing (periodic-sync based) AuthorizationCache from cache.go to AuthorizationCacheV1 in cachev1.go (no longer instantiated anywhere currently)
Add generic interface for an AuthorizationCache to cache.go
Add AuthorizationCacheV2, which uses periodic cluster and namespace-specific hashing to track changes and invalidate the cache/notify watchers
Add ReactiveAuthorizationCacheV2, which wraps AuthorizationCacheV2 in event-handling so that heavy sync checks can happen less frequently and changes are still processed in a timely manner
Switch the Project API to instantiating a ReactiveAuthorizationCacheV2
Significant testing

Motivated by the fact that the current auth cache does not appear to be notifying properly when permissions have been removed during incremental synchronizations (https://issues.redhat.com/browse/OCPBUGS-57474), and it looks to be an old issue in a system that is rather difficult to reason around.

…orizationCacheV2 implementation and switch all internal usage to an interface. Move the original resource-version-based cache to cachev1.go, update wiring and tests, and ensure all consumers use the new interface type.

openshift-ci · 2025-06-24T04:47:46Z

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

jacobsee · 2025-06-24T04:47:53Z

/test all

openshift-ci · 2025-06-24T04:49:00Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Needs approval from an approver in each of these files:

OWNERS

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

jacobsee · 2025-06-24T05:17:28Z

/retest

…eV2 for event-driven cache updates

jacobsee · 2025-06-25T01:16:25Z

/retest

jacobsee · 2025-06-25T01:53:03Z

/retest

jacobsee · 2025-06-25T05:49:54Z

/retest

…uthorizationCacheV2 to use it. Switch to Workqueue instead of custom queueing logic. Use LastSyncResourceVersion in global rbac cache to short circuit hashing.

jacobsee · 2025-06-30T20:19:34Z

/retest

jacobsee · 2025-06-30T21:20:26Z

/retest

benluddy

Thanks for all your work on this, it looks really promising!

benluddy · 2025-06-30T20:57:09Z

pkg/project/auth/cachev1.go

@@ -0,0 +1,645 @@
+package auth


Putting the cache.go/cache_test.go renames in a clean commit would make it obvious that there are no little changes mixed in.

benluddy · 2025-06-30T21:01:06Z

pkg/project/auth/cachev2.go

+		informers.RoleBindings().Informer(),
+	}
+
+	globalRBACCache := NewGlobalRBACCache(scrLister, scrbLister)


Does it make sense to pass this in rather than constructing it here? You'd be able to write tests that simulate varying the global RBAC without setting up phony informers.

benluddy · 2025-06-30T21:05:24Z

pkg/project/auth/cachev2.go

+}
+
+// Run begins watching and synchronizing the cache
+func (ac *AuthorizationCacheV2) Run(period time.Duration) {


Can the periodic "syncs" be entirely removed in favor of the informer event handlers with periodic resync intervals?

benluddy · 2025-06-30T21:20:48Z

pkg/project/auth/cachev2_reactive.go

+	// Start periodic synchronization
+	go func() {
+		// Initial sync
+		rac.synchronize()
+
+		ticker := time.NewTicker(period)
+		defer ticker.Stop()
+
+		for {
+			select {
+			case <-ticker.C:
+				rac.synchronize()
+			case <-rac.stopCh:
+				return
+			}
+		}
+	}()


Why is this still needed?

benluddy · 2025-06-30T21:22:15Z

pkg/project/auth/cachev2_reactive.go

+const (
+	globalSyncKey      = "global"
+	namespaceKeyPrefix = "namespace:"
+)


IMO, it would be at least as clear to use "" as a sentinel key for all namespaces and get rid of the string prefixing.

benluddy · 2025-06-30T21:35:51Z

pkg/project/auth/cachev2_global.go

+		refs = append(refs, rbacResourceRef{
+			uid:             string(cr.UID),
+			resourceVersion: cr.ResourceVersion,
+			kind:            "ClusterRole",


Is kind necessary? We shouldn't have any two objects with the same UID.

benluddy · 2025-06-30T21:40:48Z

pkg/project/auth/cachev2_global.go

+	// If we don't have previous versions cached, consider it changed
+	if g.lastClusterRoleVersion == "" || g.lastClusterRoleBindingVersion == "" {
+		return true
+	}


This looks redundant. When would we return true from this path but not the path that immediately follows?

benluddy · 2025-06-30T21:50:52Z

pkg/project/auth/cachev2_reactive.go

+// GetQueueStatus returns information about the queue
+func (rac *ReactiveAuthorizationCacheV2) GetQueueStatus() (queued int) {
+	return rac.queue.Len()
+}


It seems brittle to have tests that directly depend on this... can't we test the observable results of the workqueue processor without knowing about it?

benluddy · 2025-06-30T22:03:54Z

pkg/project/auth/cachev2_global.go

+}
+
+// GetCurrentHash returns the current global RBAC hash
+func (g *GlobalRBACCache) GetCurrentHash() string {


Do callers need to be responsible for detecting that this hash might be stale and for updating it? I think correctness depends on always getting the latest hash. Caching based on the last sync RV makes sense as an optimization, but I think it's an implementation detail and not something callers care about.

benluddy · 2025-06-30T22:07:50Z

pkg/project/auth/cachev1_test.go

@@ -129,7 +54,7 @@ func TestSyncNamespace(t *testing.T) {
 	nsIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})
 	nsLister := corev1listers.NewNamespaceLister(nsIndexer)

-	authorizationCache := NewAuthorizationCache(


Are there tests that can be run against both implementations? Or a shim that runs both implementations for real and detects behavior differences?

openshift-ci-robot · 2025-07-01T02:01:22Z

@jacobsee: This pull request references Jira Issue OCPBUGS-57474, which is invalid:

expected the bug to target the "4.20.0" version, but no target version was set

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.

In response to this:

Move existing (periodic-sync based) AuthorizationCache from cache.go to AuthorizationCacheV1 in cachev1.go (no longer instantiated anywhere currently)

Add generic interface for an AuthorizationCache to cache.go

Add AuthorizationCacheV2, which uses periodic cluster and namespace-specific hashing to track changes and invalidate the cache/notify watchers

Add ReactiveAuthorizationCacheV2, which wraps AuthorizationCacheV2 in event-handling so that heavy sync checks can happen less frequently and changes are still processed in a timely manner

Switch the Project API to instantiating a ReactiveAuthorizationCacheV2

Significant testing

Motivated by the fact that the current auth cache does not appear to be notifying properly when permissions have been removed during incremental synchronizations (https://issues.redhat.com/browse/OCPBUGS-57474), and it looks to be an old issue in a system that is rather difficult to reason around.

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.

openshift-ci · 2025-07-16T13:12:48Z

@jacobsee: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-ovn-cmd	`e1ab0ec`	link	false	`/test e2e-aws-ovn-cmd`
ci/prow/e2e-aws-ovn-serial	`e1ab0ec`	link	true	`/test e2e-aws-ovn-serial`
ci/prow/e2e-aws-ovn-serial-1of2	`e1ab0ec`	link	true	`/test e2e-aws-ovn-serial-1of2`
ci/prow/e2e-aws-ovn-serial-2of2	`e1ab0ec`	link	true	`/test e2e-aws-ovn-serial-2of2`

Full PR test history. Your PR dashboard.

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 24, 2025

gofmt issue

9da9b62

Introduce ReactiveAuthorizationCacheV2, which wraps AuthorizationCach…

cc4cfd1

…eV2 for event-driven cache updates

jacobsee changed the title ~~WIP: Project authorization cache V2~~ WIP: Authorization Cache V2 Jun 25, 2025

give a little more time for sync to complete in tests

758e87b

Add hash-based GlobalRBACCache for cluster-scoped RBAC and refactor A…

f2cec47

…uthorizationCacheV2 to use it. Switch to Workqueue instead of custom queueing logic. Use LastSyncResourceVersion in global rbac cache to short circuit hashing.

gofmt issue

e1ab0ec

benluddy reviewed Jun 30, 2025

View reviewed changes

jacobsee changed the title ~~WIP: Authorization Cache V2~~ WIP: OCPBUGS-57474: Authorization Cache V2 Jul 1, 2025

openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Jul 1, 2025

sanchezl mentioned this pull request Aug 13, 2025

OCPBUGS-57611: Updating ose-oauth-apiserver-container image to be consistent with ART for 4.20 openshift/oauth-apiserver#137

Merged

openshift-ci-robot mentioned this pull request Sep 2, 2025

OCPBUGS-57474: ensure cache invalidation after a time #547

Merged

WIP: OCPBUGS-57474: Authorization Cache V2 #530

Are you sure you want to change the base?

WIP: OCPBUGS-57474: Authorization Cache V2 #530

Uh oh!

Conversation

jacobsee commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Jun 24, 2025

Uh oh!

jacobsee commented Jun 24, 2025

Uh oh!

openshift-ci bot commented Jun 24, 2025

Uh oh!

jacobsee commented Jun 24, 2025

Uh oh!

jacobsee commented Jun 25, 2025

Uh oh!

jacobsee commented Jun 25, 2025

Uh oh!

jacobsee commented Jun 25, 2025

Uh oh!

jacobsee commented Jun 30, 2025

Uh oh!

jacobsee commented Jun 30, 2025

Uh oh!

benluddy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Jul 1, 2025

Uh oh!

openshift-ci bot commented Jul 16, 2025

Uh oh!

Uh oh!

jacobsee commented Jun 24, 2025 •

edited

Loading