-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
introduce accesses caching #46
Conversation
a85045d
to
505baac
Compare
Signed-off-by: Francesco Ilario <[email protected]>
Signed-off-by: Francesco Ilario <[email protected]>
505baac
to
ce753dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments, otherwise looks fine
Makefile
Outdated
@@ -61,6 +61,7 @@ test-perf: ## Run performance tests | |||
--name namespace-lister-perf-test $(PERF_CLUSTER_PROVIDER_FLAGS) | |||
KUBECONFIG=$(PERF_CLUSTER_KUBECONFIG) $(GINKGO) --label-filter='perf' \ | |||
--keep-going --procs=1 --flake-attempts 2 --output-dir=$(PERF_OUT_DIR) | |||
-$(PERF_CLUSTER_PROVIDER) delete cluster --name namespace-lister-perf-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is redundant, since we already delete the cluster when we start perf tests. If we want to delete after tests run, maybe we should only do so if tests passed? If tests failed for some reason, I'd prefer to keep the cluster around to make debugging easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I added this as I always forgot it running. Let me remove that line for this PR
pkg/auth/cache/auth_cache.go
Outdated
|
||
// stores data | ||
type AuthCache struct { | ||
data atomic.Pointer[map[rbacv1.Subject][]corev1.Namespace] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking we may need a sync.RWMutex
instead of an atomic pointer swap to synchronize access, but I'm not 100% sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
until access is readonly we should be fine without a mutex, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, but long-term I think it's what we should probably be using to synchronize accesses.
Signed-off-by: Francesco Ilario <[email protected]>
Signed-off-by: Francesco Ilario <[email protected]>
Signed-off-by: Francesco Ilario <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, I think we're safe to try to deploy/test this. We can always revert if we have issues.
Signed-off-by: Francesco Ilario [email protected]