-
Notifications
You must be signed in to change notification settings - Fork 174
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
fetch SAs from apiserver #242
base: master
Are you sure you want to change the base?
Conversation
5af4ac8
to
5020bec
Compare
ed8a585
to
0178602
Compare
} | ||
|
||
go func() { | ||
for req := range saFetchRequests { |
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 would mean we are making only one request at a time to apiserver 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.
Wrapped this in a goroutine - thanks for the catch 💯
pkg/cache/cache.go
Outdated
defer cancel() | ||
|
||
klog.V(5).Infof("fetching SA: %s", req.CacheKey()) | ||
saList, err := getter.ServiceAccounts(req.Namespace).List( |
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.
why List? why can't we use Get?
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 moved this to Get
a37069c
to
ccbb720
Compare
ccbb720
to
1b19f6f
Compare
2c19173
to
9a8d1ad
Compare
@@ -433,9 +433,10 @@ func (m *Modifier) buildPodPatchConfig(pod *corev1.Pod) *podPatchConfig { | |||
} | |||
|
|||
// Use the STS WebIdentity method if set | |||
request := cache.Request{Namespace: pod.Namespace, Name: pod.Spec.ServiceAccountName, RequestNotification: true} | |||
gracePeriodEnabled := m.saLookupGraceTime > 0 | |||
request := cache.Request{Namespace: pod.Namespace, Name: pod.Spec.ServiceAccountName, RequestNotification: gracePeriodEnabled} |
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 added this change to toggle RequestNotification
only when the grace period is enabled.
Previously it was basically a no-op when the feature is disabled and RequestNotification
is true, but now we're fetching from the API server when it's true. So we only want it set when the feature is enabled.
Issue #, if available:
#174
Description of changes:
Enhances the implementation introduced in #236 so that we can fetch missing service accounts from the apiserver. Retains the existing guarantees that we won't fetch multiple service accounts concurrently, minimizing load on the apiserver.
This feature is still in shadow mode (off by default).
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.