Skip to content

feat: Add WithMinimumCacheTTL option #176

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: Add WithMinimumCacheTTL option #176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 20 additions & 11 deletions authn/token_exchange.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ type TokenExchanger interface {
Exchange(ctx context.Context, r TokenExchangeRequest) (*TokenExchangeResponse, error)
}

const defaultCacheTTL = 15 * time.Second

var _ TokenExchanger = &TokenExchangeClient{}

// ExchangeClientOpts allows setting custom parameters during construction.
Expand All @@ -40,6 +42,14 @@ func WithTokenExchangeClientCache(cache cache.Cache) ExchangeClientOpts {
}
}

// WithMinimumCacheTTL allows setting the minimum amount of time that a cache
// entry must be valid for in order for it to be reused.
func WithMinimumCacheTTL(ttl time.Duration) ExchangeClientOpts {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious to hear your use case for making this configurable. The 15-second leeway acts as way to enforce that a token is purged from the cache a few seconds before its actual expiration. Would you actually like it to be purged sooner/later?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spoke to @mem offline and got some context around this. I understand the reasoning behind it and think it's a reasonable change.

I did share some minor concern about the naming here as WithMinimumCacheTTL doesn't quite convey what this is meant to achieve. That said, I'm not sure I can provide a substantially better alternative. Some ideas:

  • WithExpirationBuffer
  • WithEvictionThreshold

return func(c *TokenExchangeClient) {
c.minimumTTL = ttl
}
}

func NewTokenExchangeClient(cfg TokenExchangeConfig, opts ...ExchangeClientOpts) (*TokenExchangeClient, error) {
if cfg.Token == "" {
return nil, fmt.Errorf("%w: missing required token", ErrMissingConfig)
Expand All @@ -50,9 +60,10 @@ func NewTokenExchangeClient(cfg TokenExchangeConfig, opts ...ExchangeClientOpts)
}

c := &TokenExchangeClient{
cache: nil, // See below.
cfg: cfg,
singlef: singleflight.Group{},
cache: nil, // See below.
minimumTTL: defaultCacheTTL,
cfg: cfg,
singlef: singleflight.Group{},
}

for _, opt := range opts {
Expand All @@ -77,14 +88,14 @@ func NewTokenExchangeClient(cfg TokenExchangeConfig, opts ...ExchangeClientOpts)
}

return c, nil

}

type TokenExchangeClient struct {
cache cache.Cache
cfg TokenExchangeConfig
client *http.Client
singlef singleflight.Group
cache cache.Cache
minimumTTL time.Duration // Minimum time that token must be valid to be reused.
cfg TokenExchangeConfig
client *http.Client
singlef singleflight.Group
}

type TokenExchangeRequest struct {
Expand Down Expand Up @@ -207,8 +218,6 @@ func (c *TokenExchangeClient) getCache(ctx context.Context, key string) (string,
}

func (c *TokenExchangeClient) setCache(ctx context.Context, token string, key string) error {
const cacheLeeway = 15 * time.Second

parsed, err := jwt.ParseSigned(token)
if err != nil {
return fmt.Errorf("failed to parse token: %v", err)
Expand All @@ -219,7 +228,7 @@ func (c *TokenExchangeClient) setCache(ctx context.Context, token string, key st
return fmt.Errorf("failed to extract claims from the token: %v", err)
}

return c.cache.Set(ctx, key, []byte(token), time.Until(claims.Expiry.Time())-cacheLeeway)
return c.cache.Set(ctx, key, []byte(token), time.Until(claims.Expiry.Time())-c.minimumTTL)
}

var _ TokenExchanger = StaticTokenExchanger{}
Expand Down
22 changes: 22 additions & 0 deletions authn/token_exchange_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,28 @@ func Test_TokenExchangeClient_Exchange(t *testing.T) {
})
}

func Test_WithMinimumCacheTTL(t *testing.T) {
cfg := TokenExchangeConfig{
Token: "some-token",
TokenExchangeURL: "http://localhost",
}

t.Run("not using WithMinimumCacheTTL should use the default", func(t *testing.T) {
client, err := NewTokenExchangeClient(cfg)
require.NoError(t, err)
require.NotNil(t, client)
assert.Equal(t, defaultCacheTTL, client.minimumTTL)
})

t.Run("using WithMinimumCacheTTL should modify the value", func(t *testing.T) {
customTTL := 42 * time.Second
client, err := NewTokenExchangeClient(cfg, WithMinimumCacheTTL(customTTL))
require.NoError(t, err)
require.NotNil(t, client)
assert.Equal(t, customTTL, client.minimumTTL)
})
}

func signAccessToken(t *testing.T, expiresIn time.Duration) string {
signer, err := jose.NewSigner(jose.SigningKey{
Algorithm: jose.HS256,
Expand Down
Loading