Merge pull request #877 from owncloud/proxy-caching

Caching in Proxy

changelog/unreleased/proxy-cache-userinfo.md

@@ -0,0 +1,7 @@
+Enhancement: Cache userinfo in proxy
+
+Tags: proxy
+
+We introduced caching for the userinfo response. The token expiration is used for cache invalidation if available. Otherwise we fall back to a preconfigured TTL (default 10 seconds).
+
+https://github.com/owncloud/ocis/pull/877

konnectd/assets/identifier-registration.yaml

@@ -4,9 +4,8 @@
 clients:
   - id: phoenix
     name: ownCloud web app
-    application_type: web
-    insecure: yes
     trusted: yes
+    insecure: yes
     redirect_uris:
       - https://localhost:9200/
       - https://localhost:9200/oidc-callback.html
@@ -19,9 +18,8 @@ clients:
       - http://localhost:9100
 
   - id: ocis-explorer.js
-    name: OCIS Graph Explorer
+    name: oCIS Graph Explorer
     trusted: yes
-    application_type: web
     insecure: yes
 
   - id: xdXOt13JKxym1B1QcEncf2XDkLAexMBFwiT9j6EfhhHFJhs2KM9jbjTmf8JBXE69

proxy/pkg/cache/cache.go

@@ -1,19 +1,19 @@
 package cache
 
 import (
-	"fmt"
 	"sync"
+	"time"
 )
 
 // Entry represents an entry on the cache. You can type assert on V.
 type Entry struct {
-	V     interface{}
-	Valid bool
+	V          interface{}
+	expiration time.Time
 }
 
 // Cache is a barebones cache implementation.
 type Cache struct {
-	entries map[string]map[string]Entry
+	entries map[string]*Entry
 	size    int
 	m       sync.Mutex
 }
@@ -24,78 +24,55 @@ func NewCache(o ...Option) Cache {
 
 	return Cache{
 		size:    opts.size,
-		entries: map[string]map[string]Entry{},
+		entries: map[string]*Entry{},
 	}
 }
 
-// Get gets an entry on a service `svcKey` by a give `key`.
-func (c *Cache) Get(svcKey, key string) (*Entry, error) {
-	var value Entry
-	ok := true
-
+// Get gets a role-bundle by a given `roleID`.
+func (c *Cache) Get(k string) *Entry {
 	c.m.Lock()
 	defer c.m.Unlock()
 
-	if value, ok = c.entries[svcKey][key]; !ok {
-		return nil, fmt.Errorf("invalid service key: `%v`", key)
+	if _, ok := c.entries[k]; ok {
+		if c.expired(c.entries[k]) {
+			delete(c.entries, k)
+			return nil
+		}
+		return c.entries[k]
 	}
-
-	return &value, nil
+	return nil
 }
 
-// Set sets a key / value. It lets a service add entries on a request basis.
-func (c *Cache) Set(svcKey, key string, val interface{}) error {
+// Set sets a roleID / role-bundle.
+func (c *Cache) Set(k string, val interface{}, expiration time.Time) {
 	c.m.Lock()
 	defer c.m.Unlock()
 
 	if !c.fits() {
-		return fmt.Errorf("cache is full")
-	}
-
-	if _, ok := c.entries[svcKey]; !ok {
-		c.entries[svcKey] = map[string]Entry{}
-	}
-
-	if _, ok := c.entries[svcKey][key]; ok {
-		return fmt.Errorf("key `%v` already exists", key)
+		c.evict()
 	}
 
-	c.entries[svcKey][key] = Entry{
-		V:     val,
-		Valid: true,
+	c.entries[k] = &Entry{
+		val,
+		expiration,
 	}
-
-	return nil
-}
-
-// Invalidate invalidates a cache Entry by key.
-func (c *Cache) Invalidate(svcKey, key string) error {
-	r, err := c.Get(svcKey, key)
-	if err != nil {
-		return err
-	}
-
-	r.Valid = false
-	c.entries[svcKey][key] = *r
-	return nil
 }
 
-// Evict frees memory from the cache by removing invalid keys. It is a noop.
-func (c *Cache) Evict() {
-	for _, v := range c.entries {
-		for k, svcEntry := range v {
-			if !svcEntry.Valid {
-				delete(v, k)
-			}
+// evict frees memory from the cache by removing entries that exceeded the cache TTL.
+func (c *Cache) evict() {
+	for i := range c.entries {
+		if c.expired(c.entries[i]) {
+			delete(c.entries, i)
 		}
 	}
 }
 
-// Length returns the amount of entries per service key.
-func (c *Cache) Length(k string) int {
-	return len(c.entries[k])
+// expired checks if an entry is expired
+func (c *Cache) expired(e *Entry) bool {
+	return e.expiration.Before(time.Now())
 }
 
+// fits returns whether the cache fits more entries.
 func (c *Cache) fits() bool {
-	return c.size >= len(c.entries)
+	return c.size > len(c.entries)
 }

proxy/pkg/command/server.go

@@ -15,12 +15,12 @@ import (
 	"github.com/coreos/go-oidc"
 	"github.com/justinas/alice"
 	"github.com/micro/cli/v2"
-	"github.com/owncloud/ocis/ocis-pkg/service/grpc"
 	"github.com/oklog/run"
 	openzipkin "github.com/openzipkin/zipkin-go"
 	zipkinhttp "github.com/openzipkin/zipkin-go/reporter/http"
 	acc "github.com/owncloud/ocis/accounts/pkg/proto/v0"
 	"github.com/owncloud/ocis/ocis-pkg/log"
+	"github.com/owncloud/ocis/ocis-pkg/service/grpc"
 	"github.com/owncloud/ocis/proxy/pkg/config"
 	"github.com/owncloud/ocis/proxy/pkg/cs3"
 	"github.com/owncloud/ocis/proxy/pkg/flagset"
@@ -281,6 +281,8 @@ func loadMiddlewares(ctx context.Context, l log.Logger, cfg *config.Config) alic
 			}),
 			middleware.HTTPClient(oidcHTTPClient),
 			middleware.OIDCIss(cfg.OIDC.Issuer),
+			middleware.TokenCacheSize(cfg.OIDC.UserinfoCache.Size),
+			middleware.TokenCacheTTL(time.Second*time.Duration(cfg.OIDC.UserinfoCache.TTL)),
 		),
 		middleware.BasicAuth(
 			middleware.Logger(l),

proxy/pkg/config/config.go

@@ -83,6 +83,12 @@ type Reva struct {
 	Address string
 }
 
+// Cache is a TTL cache configuration.
+type Cache struct {
+	Size int
+	TTL  int
+}
+
 // Config combines all available configuration parts.
 type Config struct {
 	File                  string
@@ -105,8 +111,9 @@ type Config struct {
 // OIDC is the config for the OpenID-Connect middleware. If set the proxy will try to authenticate every request
 // with the configured oidc-provider
 type OIDC struct {
-	Issuer   string
-	Insecure bool
+	Issuer        string
+	Insecure      bool
+	UserinfoCache Cache
 }
 
 // PolicySelector is the toplevel-configuration for different selectors

proxy/pkg/flagset/flagset.go

@@ -202,6 +202,20 @@ func ServerWithConfig(cfg *config.Config) []cli.Flag {
 			EnvVars:     []string{"PROXY_OIDC_INSECURE"},
 			Destination: &cfg.OIDC.Insecure,
 		},
+		&cli.IntFlag{
+			Name:        "oidc-userinfo-cache-tll",
+			Value:       10,
+			Usage:       "Fallback TTL in seconds for caching userinfo, when no token lifetime can be identified",
+			EnvVars:     []string{"PROXY_OIDC_USERINFO_CACHE_TTL"},
+			Destination: &cfg.OIDC.UserinfoCache.TTL,
+		},
+		&cli.IntFlag{
+			Name:        "oidc-userinfo-cache-size",
+			Value:       1024,
+			Usage:       "Max entries for caching userinfo",
+			EnvVars:     []string{"PROXY_OIDC_USERINFO_CACHE_SIZE"},
+			Destination: &cfg.OIDC.UserinfoCache.Size,
+		},
 
 		&cli.BoolFlag{
 			Name:        "autoprovision-accounts",

proxy/pkg/middleware/account_resolver_test.go

@@ -3,33 +3,31 @@ package middleware
 import (
 	"context"
 	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
 	"github.com/micro/go-micro/v2/client"
 	"github.com/owncloud/ocis/accounts/pkg/proto/v0"
 	"github.com/owncloud/ocis/ocis-pkg/log"
 	"github.com/owncloud/ocis/ocis-pkg/oidc"
 	"github.com/owncloud/ocis/proxy/pkg/config"
 	settings "github.com/owncloud/ocis/settings/pkg/proto/v0"
-	"net/http"
-	"net/http/httptest"
-	"testing"
 )
 
 func TestGetAccountSuccess(t *testing.T) {
-	svcCache.Invalidate(AccountsKey, "success")
 	if _, status := getAccount(log.NewLogger(), mockAccountResolverMiddlewareAccSvc(false, true), "mail eq 'success'"); status != 0 {
 		t.Errorf("expected an account")
 	}
 }
 
 func TestGetAccountInternalError(t *testing.T) {
-	svcCache.Invalidate(AccountsKey, "failure")
 	if _, status := getAccount(log.NewLogger(), mockAccountResolverMiddlewareAccSvc(true, false), "mail eq 'failure'"); status != http.StatusInternalServerError {
 		t.Errorf("expected an internal server error")
 	}
 }
 
 func TestAccountResolverMiddleware(t *testing.T) {
-	svcCache.Invalidate(AccountsKey, "success")
 	next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})
 	m := AccountResolver(
 		Logger(log.NewLogger()),
@@ -50,7 +48,6 @@ func TestAccountResolverMiddleware(t *testing.T) {
 }
 
 func TestAccountResolverMiddlewareWithDisabledAccount(t *testing.T) {
-	svcCache.Invalidate(AccountsKey, "failure")
 	next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})
 	m := AccountResolver(
 		Logger(log.NewLogger()),

proxy/pkg/middleware/middleware.go → proxy/pkg/middleware/errors.go

@@ -12,5 +12,5 @@ var (
 	ErrUnauthorized = errors.New("unauthorized")
 
 	// ErrInternal is returned if something went wrong
-	ErrInternal     = errors.New("internal error")
+	ErrInternal = errors.New("internal error")
 )