✨ 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.07%. Comparing base (872b7f7) to head (3b2949f).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1006      +/-   ##
==========================================
+ Coverage   77.19%   78.07%   +0.87%     
==========================================
  Files          17       18       +1     
  Lines        1206     1254      +48     
==========================================
+ Hits          931      979      +48     
  Misses        193      193              
  Partials       82       82

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

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

internal/authentication/tokengetter.go

internal/authentication/tokengetter_test.go

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 👍

internal/authentication/tokengetter.go

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

everettraven

Thanks for bearing with us on all the reviews @skattoju - I think this is in a good state now!

joelanford · 2024-07-05T16:09:40Z

internal/authentication/tokengetter.go

+		if metav1.Now().Sub(token.ExpirationTimestamp.Time) > expiredDuration {
+			delete(t.tokens, key)
+		}


Why would we keep expired tokens around? Seems like they could be reaped as soon as they expire.

Now that you mention it, it seems like we can get rid of that expiredDuration parameter 🤔

joelanford · 2024-07-05T16:14:02Z

internal/authentication/tokengetter.go

+	}
+
+	// Create a new token if the cached token expires within DurationPercentage of expirationDuration from now
+	rotationThresholdAfterNow := metav1.Now().Add(t.expirationDuration * (RotationThresholdPercentage / 100))


Will rotationThresholdAfterNow always evaluate to 0 when RotationThresholdPercentage < 100? Seems like (RotationThresholdPercentage / 100) is an integer division.

RotationThresholdPercentage should probably be a float64 so that we can do something like this:

Suggested change

rotationThresholdAfterNow := metav1.Now().Add(t.expirationDuration * (RotationThresholdPercentage / 100))

rotationThresholdAfterNow := metav1.Now().Add(time.Duration(float64(t.expirationDuration) * RotationThresholdPercentage)

also, any reason for RotationThresholdPercentage to be exported? Seems like we could unexport it.

joelanford · 2024-07-05T16:32:04Z

internal/authentication/tokengetter.go

+	req, err := t.client.ServiceAccounts(key.Namespace).CreateToken(ctx,
+		key.Name,
+		&authenticationv1.TokenRequest{
+			Spec: authenticationv1.TokenRequestSpec{ExpirationSeconds: ptr.To[int64](int64(t.expirationDuration))},


t.expirationDuration needs to be converted to an integer number of seconds to be used in the TokenRequest API.

Suggested change

Spec: authenticationv1.TokenRequestSpec{ExpirationSeconds: ptr.To[int64](int64(t.expirationDuration))},

Spec: authenticationv1.TokenRequestSpec{ExpirationSeconds: ptr.To[int64](int64(t.expirationDuration/time.Second))},

…rk#1006)

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

internal/authentication/tokengetter.go Show resolved Hide resolved

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

everettraven approved these changes Jul 5, 2024

View reviewed changes

everettraven enabled auto-merge July 5, 2024 13:46

everettraven added this pull request to the merge queue Jul 5, 2024

Merged via the queue into operator-framework:main with commit 5c83e91 Jul 5, 2024
17 checks passed

joelanford reviewed Jul 5, 2024

View reviewed changes

skattoju mentioned this pull request Jul 5, 2024

✨ Fixes and Improvements for TokenGetter #1014

Merged

4 tasks

perdasilva pushed a commit to LalatenduMohanty/operator-controller that referenced this pull request Aug 13, 2024

Adding a token getter to get service account tokens (operator-framewo…

9c6d110

…rk#1006)

skattoju mentioned this pull request Sep 9, 2024

🌱 Descriptive Upgrade Constraint Policy constants #1233

Merged

skattoju mentioned this pull request Sep 25, 2024

✨ Remove unpacked status #1309

Merged

4 tasks

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

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

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

everettraven left a comment

joelanford Jul 5, 2024

skattoju Jul 5, 2024

joelanford Jul 5, 2024

joelanford Jul 5, 2024

joelanford Jul 5, 2024

joelanford Jul 5, 2024

	rotationThresholdAfterNow := metav1.Now().Add(t.expirationDuration * (RotationThresholdPercentage / 100))
	rotationThresholdAfterNow := metav1.Now().Add(time.Duration(float64(t.expirationDuration) * RotationThresholdPercentage)

	Spec: authenticationv1.TokenRequestSpec{ExpirationSeconds: ptr.To[int64](int64(t.expirationDuration))},
	Spec: authenticationv1.TokenRequestSpec{ExpirationSeconds: ptr.To[int64](int64(t.expirationDuration/time.Second))},

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

✨ 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

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

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

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

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

joelanford Jul 3, 2024 •

edited

Loading