Token cache #19
Token cache #19
Conversation
Could do with some polish, but this will cut down on the auth spam. Cursory tests show significant throughput/latency gains. - Cache auth token - Read max-age Cache-Control header & set expiration if possible, default to 10 minutes, or honor no-cache/no-store directives - Refresh within 30 seconds of expiration
stargate/pkg/auth/client.go
Outdated
| @@ -34,6 +37,11 @@ type authRequest struct { | |||
| Password string `json:"password"` | |||
| } | |||
|
|
|||
| var cacheEnabled = true | |||
There was a problem hiding this comment.
Do you think these can be variables of an instance of tableBasedTokenProvider?
There was a problem hiding this comment.
Sensible. I'm not totally sure I got it right, but I think I'm close. Works, in any case.
|
I'm not sure this will work. There seems to be an outside possibility that the server could forget the token is valid. Currently, the token refresh mechanism is nested in an implementation of the GetRequestMetadata interface in the grpc package, so I can't see a way to trigger a manual refresh from the code that sees the error. I thought about sticking the token on the context, but there are calls that don't use context... open to ideas. |
|
Good call. I don't remember all the details on how token auth work in Stargate (I think it might be on a sliding window based on usage). I need a bit of time to refresh my mind on how that work to provide good feedback there. It's almost like we need the token to only be regenerated when the service becomes unauthorized, but this might not be needed if it's on a sliding window. Give a couple days to think about it and I'll get back to you. If you have time you could try checking out the token-auth component in Stargate. |
| } | ||
|
|
||
| // cache expiration default of 10min (arbitrary). | ||
| ccExpSec := 600 |
There was a problem hiding this comment.
This shouldn't cache it if there's not a valid Cache-Control. I don't think this should default to an arbitrary value.
|
It looks like the |
|
Here are my suggested changes. The cache logic could be simplified and it shouldn't cache if there's not a cache control header. Also, I think that the provider can be user on multiple threads so any shared data should be protected with a mutex or they should use atomics (more challenging to get correct). diff --git a/stargate/pkg/auth/client.go b/stargate/pkg/auth/client.go
index d486a8f..a66e75e 100644
--- a/stargate/pkg/auth/client.go
+++ b/stargate/pkg/auth/client.go
@@ -10,6 +10,7 @@ import (
"regexp"
"strconv"
"strings"
+ "sync"
"time"
log "github.com/sirupsen/logrus"
@@ -17,13 +18,13 @@ import (
)
type tableBasedTokenProvider struct {
- cacheEnabled bool
token string
tokenExpiresAt time.Time
client *client
username string
password string
requireTransportSecurity bool
+ mu sync.RWMutex // I believe that the token provider can be access by multiple threads, protect shared data.
}
type client struct {
@@ -47,7 +48,6 @@ var ccMaxAgeRegex, _ = regexp.Compile(`\d+`)
// with the returned token.
func NewTableBasedTokenProvider(serviceURL, username, password string) credentials.PerRPCCredentials {
return &tableBasedTokenProvider{
- cacheEnabled: true,
client: getClient(serviceURL),
username: username,
password: password,
@@ -59,7 +59,6 @@ func NewTableBasedTokenProvider(serviceURL, username, password string) credentia
// to false for environments where transport security it not in use.
func NewTableBasedTokenProviderUnsafe(serviceURL, username, password string) credentials.PerRPCCredentials {
return &tableBasedTokenProvider{
- cacheEnabled: true,
client: getClient(serviceURL),
username: username,
password: password,
@@ -81,12 +80,14 @@ func (t *tableBasedTokenProvider) GetRequestMetadata(ctx context.Context, uri ..
}
func (t *tableBasedTokenProvider) getToken(ctx context.Context) (string, error) {
- // If we have a cached token and it won't expire for at least 30 seconds, use it
- if t.cacheEnabled &&
- t.token != "" &&
+ // If we have a cached token that doesn't expire for at least 30 seconds, use it
+ t.mu.RLock()
+ if t.token != "" &&
t.tokenExpiresAt.Add(-30*time.Second).After(time.Now().UTC()) {
+ t.mu.RUnlock()
return t.token, nil
}
+ t.mu.RUnlock()
authReq := authRequest{
Username: t.username,
@@ -126,13 +127,7 @@ func (t *tableBasedTokenProvider) getToken(ctx context.Context) (string, error)
return "", fmt.Errorf("error unmarshalling response body: %v", err)
}
- // If the server asks, skip the cache.
- if !t.cacheEnabled {
- return ar.AuthToken, nil
- }
-
- // cache expiration default of 10min (arbitrary).
- ccExpSec := 600
+ ccExpSec := 0 // Don't cache by default
// Try to read the server's reported cache expiration, or no-cache (unlikely).
ccValue := response.Header.Get("Cache-Control")
@@ -142,26 +137,29 @@ func (t *tableBasedTokenProvider) getToken(ctx context.Context) (string, error)
strings.Contains(val, "s-maxage") {
res := ccMaxAgeRegex.Find([]byte(ccValue))
ccSec, ccErr := strconv.Atoi(string(res))
- if ccErr == nil {
+ if ccErr != nil {
ccExpSec = ccSec
}
break
}
if strings.Contains(val, "no-cache") ||
strings.Contains(val, "no-store") {
- t.cacheEnabled = false
- return ar.AuthToken, nil
+ ccExpSec = 0
+ break
}
}
}
- // Cache the token
- t.token = ar.AuthToken
-
- // Set expiration
- t.tokenExpiresAt = time.Now().UTC().Add(time.Second * time.Duration(ccExpSec))
+ t.mu.Lock()
+ if ccExpSec > 0 {
+ t.token = ar.AuthToken
+ t.tokenExpiresAt = time.Now().UTC().Add(time.Second * time.Duration(ccExpSec))
+ } else {
+ t.token = ""
+ }
+ t.mu.Unlock()
- return t.token, nil
+ return ar.AuthToken, nil
}
func getClient(serviceURL string) *client { |
Could do with some polish, but this will cut down on the auth spam. Cursory tests show significant throughput/latency gains.
What this PR does:
Which issue(s) this PR fixes:
Fixes #18
Checklist