✨ Adding a token getter to get service account tokens #1006

skattoju · 2024-07-02T14:49:06Z

Description

As part of #737 user provided service accounts will be used to manage cluster extensions, the token getter implemented in this PR enables this by providing an interface to fetch tokens given a service account. It also implements a cache where fetched tokens are stored saving requests to the api server for subsequent requests. Fixes #972

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

netlify · 2024-07-02T14:49:21Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`3b2949f`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/6686f15bcfe6650009a78b01
😎 Deploy Preview	https://deploy-preview-1006--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-07-02T20:24:22Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.10%. Comparing base (872b7f7) to head (a902abc).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1006      +/-   ##
==========================================
+ Coverage   77.19%   78.10%   +0.90%     
==========================================
  Files          17       18       +1     
  Lines        1206     1256      +50     
==========================================
+ Hits          931      981      +50     
  Misses        193      193              
  Partials       82       82

Flag	Coverage Δ
e2e	`56.54% <ø> (ø)`
unit	`53.82% <100.00%> (+1.91%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

everettraven

Overall I think this looks really good, great work! There are a few smaller changes I'd recommend

everettraven · 2024-07-02T20:28:19Z

internal/authentication/tokengetter.go

+		expireTime = token.ExpirationTimestamp.Time
+	}
+
+	fiveMinutesAfterNow := metav1.Now().Add(5 * time.Minute)


Any particular reason for five minutes being hard coded here?

What do you think about making this configurable via the options pattern? I think that this would make this code a bit more robust and allow for tweaking this value in various cases without having to change the underlying logic of the TokenGetter

I forgot about that.. That should be set to expiration seconds. The options pattern is a good idea, thanks for pointing it out.

everettraven · 2024-07-02T20:30:45Z

internal/authentication/tokengetter.go

+}
+
+func (t *TokenGetter) getToken(ctx context.Context, key types.NamespacedName) (*authv1.TokenRequestStatus, error) {
+	req, err := t.client.ServiceAccounts(key.Namespace).CreateToken(ctx, key.Name, &authv1.TokenRequest{Spec: authv1.TokenRequestSpec{ExpirationSeconds: ptr.To[int64](3600)}}, metav1.CreateOptions{})


Similar comment here, any particular reason for the hardcoded 3600 seconds for expiration? Maybe this would be another good configurable option

Also missed this, this should also be set to expiration seconds will fix 👍

everettraven · 2024-07-02T20:41:23Z

internal/authentication/tokengetter.go

+func NewTokenGetter(client corev1.ServiceAccountsGetter, expirationSeconds int64) *TokenGetter {
+	return &TokenGetter{
+		client:            client,
+		expirationSeconds: expirationSeconds,


I noticed this expirationSeconds field is unused (noticed after my other comments related to hardcoded values)

everettraven · 2024-07-02T20:59:50Z

internal/authentication/tokengetter.go

+}
+
+func (t *TokenGetter) getToken(ctx context.Context, key types.NamespacedName) (*authenticationv1.TokenRequestStatus, error) {
+	req, err := t.client.ServiceAccounts(key.Namespace).CreateToken(ctx, key.Name, &authenticationv1.TokenRequest{Spec: authenticationv1.TokenRequestSpec{ExpirationSeconds: ptr.To[int64](3600)}}, metav1.CreateOptions{})


Similar comment regarding hardcoded value for expiration seconds, I'd like to see this be a configurable option

(in case this seems like a duplicate my GH review UI seemed to have lost the comment so I'm potentially adding a repeat comment here)

everettraven · 2024-07-02T21:02:51Z

internal/authentication/tokengetter_test.go

+	ctest "k8s.io/client-go/testing"
+)
+
+func TestNewTokenGetter(t *testing.T) {


Looking through the steps of the test I think the logic looks good overall. One thing I will note is that most of the tests we write for operator-controller end up being table driven tests that end up sharing the same test logic where possible. I think this test in particular could be a bit more readable and maintainable utilizing the table driven approach.

I don't think it is absolutely necessary if folks feel strongly about not requiring that change, but something I would personally like to see to stay consistent with many of the other unit tests we have throughout operator-controller

Ill change it to a table driven style 👍

joelanford · 2024-07-03T17:46:16Z

internal/authentication/tokengetter.go

+func (k *keyLock[K]) getLock(key K) *sync.Mutex {
+	k.mu.Lock()
+	defer k.mu.Unlock()
+
+	lock, ok := k.locks[key]
+	if !ok {
+		lock = &sync.Mutex{}
+		k.locks[key] = lock
+	}
+	return lock
+}


The number of locks will grow unbounded here, right? Maybe this is unnecessary complexity to start off with. WDYT about skipping the keyLock altogether. For now, maybe it's fine to get a global lock in the Get method. That would allow only one token to be requested at a time. If that becomes a problem, we can introduce key locks later.

Looking back through this a second time I was wondering the same thing. I get why the key lock was created but agree that this is probably more complex than necessary for a first pass

ok ill remove the key locks for now

internal/authentication/tokengetter.go

everettraven · 2024-07-03T18:00:18Z

internal/authentication/tokengetter.go

+		expireTime = token.ExpirationTimestamp.Time
+	}
+
+	expirationSecondsAfterNow := metav1.Now().Add(time.Duration(t.expirationSeconds))


Using the expiration seconds field here feels unintuitive to me. Maybe we should add another field to the struct called something like rotationThreshold that is a time.Duration type?

Or maybe we make it some hardcoded fraction of expirationSeconds? Maybe if we're within 10% or something (i.e. if expiration is 100s, then we would get a new token anytime after 90s)

yeah i guess setting that to expirationSeconds may be too much.. I will add a rotationThreshold so its configurable..

So if expirationSeconds is 100s and we set rotationThreshold to 10s we we would create a new token if its been more than 90s since the token was created like you suggested 👍

everettraven · 2024-07-03T19:25:41Z

internal/authentication/tokengetter.go

+}
+
+// Get returns a token from the cache if available and not expiring, otherwise creates a new token
+func (t *TokenGetter) Get(ctx context.Context, key types.NamespacedName) (string, error) {


Apologies for not noticing this earlier in the review, but Joe's earlier comment on the tokenLocks potentially growing unbounded made me realize that the tokens themselves could potentially grow unbounded with the current implementation since we only have a Get function.

What do you think about adding a method that could be used to "prune" expired tokens from the cache?

I'm thinking this would be a method used as needed by a caller. I could imagine when we wire everything together we may want to ensure we prune any expired tokens that are older than X amount of time

@joelanford What do you think?

It appears that theres no way to invalidate tokens issued by the TokenRequest api 🤔 but for our purposes i guess we just remove keys i.e. namespaced names of the service accounts from the token getter cache

Yeah, I'm thinking that it could simply be a process that runs every X amount of time and runs the method to ensure we aren't storing expired tokens for an unreasonable amount of time and adding bloat to the memory usage

To clarify, I'm only recommending we implement the method that will delete the keys and tokens from the map that are expired and meet a certain criteria defined by the caller (like expired for more than 2 hours)

Thinking about this a bit more, we would only remove entries from the cache when extensions are removed, in case there are expired tokens they would belong to service accounts of currently installed extensions. Further more the expired tokens will be replaced by fresh ones whenever an attempt is made to use the tokens so in theory tokens should not accumulate 🤔

Do we need the complexity of a background goroutine? What if we write a reapExpiredTokens function and call it every time Get is called?

That would make it simpler. Ive added a reapExpiredTokens This also eliminates the need for Clean and TokenExists

joelanford · 2024-07-03T20:34:48Z

internal/authentication/tokengetter.go

+	for _, opt := range options {
+		opt(tokenGetter)
+	}
+


If we allow expiration seconds and rotation threshold to be independently configurable, we probably need to do some sanity validation to make sure they make sense.

But I'm not sure we actually need rotation threshold to be configurable if we just make it expirationDuration * 0.1 or whatever we think is a reasonable ratio. I feel like we may be over-engineering this.

joelanford · 2024-07-03T20:36:39Z

internal/authentication/tokengetter.go

+func NewTokenGetter(client corev1.ServiceAccountsGetter, options ...TokenGetterOption) *TokenGetter {
+	tokenGetter := &TokenGetter{
+		client:            client,
+		expirationSeconds: int64(5 * time.Minute), // default token ttl


We should change this to a time.Duration type and change the name to expirationDuration. The name ("Seconds" suffix) and value (integer number of nanoseconds in 5 minutes) lead a reader to an incorrect conclusion.

I guess that was int64 because the TokenRequest api takes an pointer to an int64.. its better as a time.Duration 👍

joelanford · 2024-07-03T20:40:59Z

internal/authentication/tokengetter.go

+func (t *TokenGetter) Clean(ctx context.Context, key types.NamespacedName) {
+	t.mu.RLock()
+	defer t.mu.RUnlock()
+	delete(t.tokens, key)
+}
+
+func (t *TokenGetter) TokenExists(ctx context.Context, key types.NamespacedName) bool {
+	t.mu.RLock()
+	defer t.mu.RUnlock()
+	_, ok := t.tokens[key]
+	return ok
+}


How do we envision Clean and TokenExists being used?

Clean would be to remove entries from the cache when extensions are uninstalled or unused items need to be cleaned up for some other reason. TokenExists is debatable, as I added to make it testable.

These are made redundant by reapExpiredTokens so I have removed them

skattoju requested a review from a team as a code owner July 2, 2024 14:49

skattoju force-pushed the token_getter branch 2 times, most recently from 46d5ddc to b3d079a Compare July 2, 2024 15:06

skattoju changed the title ~~Token getter for service account tokens~~ ✨ Adding a token getter to get service account tokens Jul 2, 2024

skattoju force-pushed the token_getter branch from b3d079a to 98d5db2 Compare July 2, 2024 18:08

skattoju force-pushed the token_getter branch from 98d5db2 to bb93045 Compare July 2, 2024 20:50

everettraven requested changes Jul 2, 2024

View reviewed changes

skattoju force-pushed the token_getter branch from bb93045 to b77e9cd Compare July 3, 2024 16:17

joelanford reviewed Jul 3, 2024

View reviewed changes

everettraven reviewed Jul 3, 2024

View reviewed changes

internal/authentication/tokengetter.go Show resolved Hide resolved

everettraven reviewed Jul 3, 2024

View reviewed changes

skattoju force-pushed the token_getter branch from b77e9cd to 6e82b02 Compare July 3, 2024 18:42

everettraven reviewed Jul 3, 2024

View reviewed changes

skattoju force-pushed the token_getter branch from 6e82b02 to a902abc Compare July 3, 2024 20:29

joelanford reviewed Jul 3, 2024

View reviewed changes

skattoju force-pushed the token_getter branch 4 times, most recently from 0f3c60d to d3c041f Compare July 4, 2024 16:57

Adding a token getter to get service account tokens

3b2949f

skattoju force-pushed the token_getter branch from d3c041f to 3b2949f Compare July 4, 2024 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Adding a token getter to get service account tokens #1006

✨ Adding a token getter to get service account tokens #1006

skattoju commented Jul 2, 2024 •

edited

Loading

netlify bot commented Jul 2, 2024 •

edited

Loading

codecov bot commented Jul 2, 2024 •

edited

Loading

everettraven left a comment

everettraven Jul 2, 2024

skattoju Jul 3, 2024

everettraven Jul 2, 2024

skattoju Jul 3, 2024

everettraven Jul 2, 2024

everettraven Jul 2, 2024

everettraven Jul 2, 2024

skattoju Jul 3, 2024

joelanford Jul 3, 2024

everettraven Jul 3, 2024

skattoju Jul 3, 2024

everettraven Jul 3, 2024

joelanford Jul 3, 2024 •

edited

Loading

skattoju Jul 3, 2024

skattoju Jul 3, 2024

everettraven Jul 3, 2024

everettraven Jul 3, 2024

skattoju Jul 3, 2024

everettraven Jul 3, 2024

everettraven Jul 3, 2024 •

edited

Loading

skattoju Jul 3, 2024

joelanford Jul 3, 2024 •

edited

Loading

skattoju Jul 4, 2024

joelanford Jul 3, 2024

joelanford Jul 3, 2024 •

edited

Loading

skattoju Jul 3, 2024

joelanford Jul 3, 2024

skattoju Jul 3, 2024

skattoju Jul 4, 2024

✨ Adding a token getter to get service account tokens #1006

Are you sure you want to change the base?

✨ Adding a token getter to get service account tokens #1006

Conversation

skattoju commented Jul 2, 2024 • edited Loading

Description

Reviewer Checklist

netlify bot commented Jul 2, 2024 • edited Loading

✅ Deploy Preview for olmv1 ready!

codecov bot commented Jul 2, 2024 • edited Loading

Codecov Report

everettraven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelanford Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

everettraven Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelanford Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelanford Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skattoju commented Jul 2, 2024 •

edited

Loading

netlify bot commented Jul 2, 2024 •

edited

Loading

codecov bot commented Jul 2, 2024 •

edited

Loading

joelanford Jul 3, 2024 •

edited

Loading

everettraven Jul 3, 2024 •

edited

Loading

joelanford Jul 3, 2024 •

edited

Loading

joelanford Jul 3, 2024 •

edited

Loading