feat: cache argo cd rbac (argoproj#7587)

feat: cache argo cd rbac (argoproj#7587) Part of: argoproj#4296 Signed-off-by: Jan Jansen <[email protected]> Signed-off-by: pashavictorovich <[email protected]> Signed-off-by: Alexander Matyushentsev <[email protected]>
tkasuz · Nov 3, 2021 · fe3cc72 · fe3cc72
1 parent 513d8a4
commit fe3cc72
Show file tree

Hide file tree

Showing 5 changed files with 168 additions and 50 deletions.
diff --git a/server/cluster/cluster_test.go b/server/cluster/cluster_test.go
@@ -39,7 +39,7 @@ func newServerInMemoryCache() *servercache.Cache {
 
 func newNoopEnforcer() *rbac.Enforcer {
 	enf := rbac.NewEnforcer(fake.NewSimpleClientset(test.NewFakeConfigMap()), test.FakeArgoCDNamespace, common.ArgoCDConfigMapName, nil)
-	enf.Enforcer.EnableEnforce(false)
+	enf.EnableEnforce(false)
 	return enf
 }
 

diff --git a/server/rbacpolicy/rbacpolicy.go b/server/rbacpolicy/rbacpolicy.go
@@ -105,16 +105,18 @@ func (p *RBACPolicyEnforcer) EnforceClaims(claims jwt.Claims, rvals ...interface
 	// Check if the request is for an application resource. We have special enforcement which takes
 	// into consideration the project's token and group bindings
 	var runtimePolicy string
+	var projName string
 	proj := p.getProjectFromRequest(rvals...)
 	if proj != nil {
 		if IsProjectSubject(subject) {
 			return p.enforceProjectToken(subject, proj, rvals...)
 		}
 		runtimePolicy = proj.ProjectPoliciesString()
+		projName = proj.Name
 	}
 
 	// NOTE: This calls prevent multiple creation of the wrapped enforcer
-	enforcer := p.enf.CreateEnforcerWithRuntimePolicy(runtimePolicy)
+	enforcer := p.enf.CreateEnforcerWithRuntimePolicy(projName, runtimePolicy)
 
 	// Check the subject. This is typically the 'admin' case.
 	// NOTE: the call to EnforceWithCustomEnforcer will also consider the default role
@@ -191,6 +193,5 @@ func (p *RBACPolicyEnforcer) enforceProjectToken(subject string, proj *v1alpha1.
 	}
 
 	vals := append([]interface{}{subject}, rvals[1:]...)
-	return p.enf.EnforceRuntimePolicy(proj.ProjectPoliciesString(), vals...)
-
+	return p.enf.EnforceRuntimePolicy(proj.Name, proj.ProjectPoliciesString(), vals...)
 }
diff --git a/server/rbacpolicy/rbacpolicy_test.go b/server/rbacpolicy/rbacpolicy_test.go
@@ -107,6 +107,33 @@ p, cam, applications, %s/argoproj.io/Rollout/resume, my-proj/*, allow
 	assert.False(t, enf.Enforce(claims, "applications", ActionAction+"/argoproj.io/Rollout/resume", "my-proj/my-app"))
 }
 
+func TestInvalidatedCache(t *testing.T) {
+	kubeclientset := fake.NewSimpleClientset(test.NewFakeConfigMap())
+	projLister := test.NewFakeProjLister(newFakeProj())
+	enf := rbac.NewEnforcer(kubeclientset, test.FakeArgoCDNamespace, common.ArgoCDConfigMapName, nil)
+	enf.EnableLog(true)
+	_ = enf.SetBuiltinPolicy(`p, alice, applications, create, my-proj/*, allow`)
+	_ = enf.SetUserPolicy(`p, bob, applications, create, my-proj/*, allow`)
+	rbacEnf := NewRBACPolicyEnforcer(enf, projLister)
+	enf.SetClaimsEnforcerFunc(rbacEnf.EnforceClaims)
+
+	claims := jwt.MapClaims{"sub": "alice"}
+	assert.True(t, enf.Enforce(claims, "applications", "create", "my-proj/my-app"))
+	claims = jwt.MapClaims{"sub": "bob"}
+	assert.True(t, enf.Enforce(claims, "applications", "create", "my-proj/my-app"))
+
+	_ = enf.SetBuiltinPolicy(`p, alice, applications, create, my-proj2/*, allow`)
+	_ = enf.SetUserPolicy(`p, bob, applications, create, my-proj2/*, allow`)
+	claims = jwt.MapClaims{"sub": "alice"}
+	assert.True(t, enf.Enforce(claims, "applications", "create", "my-proj2/my-app"))
+	claims = jwt.MapClaims{"sub": "bob"}
+	assert.True(t, enf.Enforce(claims, "applications", "create", "my-proj2/my-app"))
+	claims = jwt.MapClaims{"sub": "alice"}
+	assert.False(t, enf.Enforce(claims, "applications", "create", "my-proj/my-app"))
+	claims = jwt.MapClaims{"sub": "bob"}
+	assert.False(t, enf.Enforce(claims, "applications", "create", "my-proj/my-app"))
+}
+
 func TestGetScopes_DefaultScopes(t *testing.T) {
 	rbacEnforcer := NewRBACPolicyEnforcer(nil, nil)
 

diff --git a/util/rbac/rbac.go b/util/rbac/rbac.go
@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/argoproj/argo-cd/v2/util/assets"
@@ -16,6 +17,7 @@ import (
 	"github.com/casbin/casbin/model"
 	"github.com/casbin/casbin/util"
 	jwt "github.com/dgrijalva/jwt-go/v4"
+	gocache "github.com/patrickmn/go-cache"
 	log "github.com/sirupsen/logrus"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -35,18 +37,32 @@ const (
 	ConfigMapMatchModeKey     = "policy.matchMode"
 	GlobMatchMode             = "glob"
 	RegexMatchMode            = "regex"
-	defaultRBACSyncPeriod     = 10 * time.Minute
+
+	defaultRBACSyncPeriod = 10 * time.Minute
 )
 
+// CasbinEnforcer represents methods that must be implemented by a Casbin enforces
+type CasbinEnforcer interface {
+	EnableLog(bool)
+	Enforce(rvals ...interface{}) bool
+	LoadPolicy() error
+	EnableEnforce(bool)
+	AddFunction(name string, function func(args ...interface{}) (interface{}, error))
+	GetGroupingPolicy() [][]string
+}
+
 // Enforcer is a wrapper around an Casbin enforcer that:
 // * is backed by a kubernetes config map
 // * has a predefined RBAC model
 // * supports a built-in policy
 // * supports a user-defined policy
 // * supports a custom JWT claims enforce function
 type Enforcer struct {
-	*casbin.Enforcer
+	lock               sync.Mutex
+	enforcerCache      *gocache.Cache
 	adapter            *argocdAdapter
+	enableLog          bool
+	enabled            bool
 	clientset          kubernetes.Interface
 	namespace          string
 	configmap          string
@@ -56,11 +72,81 @@ type Enforcer struct {
 	matchMode          string
 }
 
+// cachedEnforcer holds the Casbin enforcer instances and optional custom project policy
+type cachedEnforcer struct {
+	enforcer CasbinEnforcer
+	policy   string
+}
+
+func (e *Enforcer) invalidateCache(actions ...func()) {
+	e.lock.Lock()
+	defer e.lock.Unlock()
+
+	for _, action := range actions {
+		action()
+	}
+	e.enforcerCache.Flush()
+}
+
+func (e *Enforcer) getCabinEnforcer(project string, policy string) CasbinEnforcer {
+	res, err := e.tryGetCabinEnforcer(project, policy)
+	if err != nil {
+		panic(err)
+	}
+	return res
+}
+
+// tryGetCabinEnforcer returns the cached enforcer for the given optional project and project policy.
+func (e *Enforcer) tryGetCabinEnforcer(project string, policy string) (CasbinEnforcer, error) {
+	e.lock.Lock()
+	defer e.lock.Unlock()
+	var cached *cachedEnforcer
+	val, ok := e.enforcerCache.Get(project)
+	if ok {
+		if c, ok := val.(*cachedEnforcer); ok && c.policy == policy {
+			cached = c
+		}
+	}
+	if cached != nil {
+		return cached.enforcer, nil
+	}
+	var err error
+	var enforcer CasbinEnforcer
+	if policy != "" {
+		if enforcer, err = newEnforcerSafe(e.model, newAdapter(e.adapter.builtinPolicy, e.adapter.userDefinedPolicy, policy)); err != nil {
+			// fallback to default policy if project policy is invalid
+			log.Errorf("Failed to load project '%s' policy", project)
+			enforcer, err = newEnforcerSafe(e.model, e.adapter)
+		}
+	} else {
+		enforcer, err = newEnforcerSafe(e.model, e.adapter)
+	}
+	if err != nil {
+		return nil, err
+	}
+
+	matchFunc := globMatchFunc
+	if e.matchMode == RegexMatchMode {
+		matchFunc = util.RegexMatchFunc
+	}
+	enforcer.AddFunction("globOrRegexMatch", matchFunc)
+	enforcer.EnableLog(e.enableLog)
+	enforcer.EnableEnforce(e.enabled)
+	e.enforcerCache.SetDefault(project, &cachedEnforcer{enforcer: enforcer, policy: policy})
+	return enforcer, nil
+}
+
 // ClaimsEnforcerFunc is func template to enforce a JWT claims. The subject is replaced
 type ClaimsEnforcerFunc func(claims jwt.Claims, rvals ...interface{}) bool
 
-func newEnforcerSafe(params ...interface{}) (e *casbin.Enforcer, err error) {
-	enfs, err := casbin.NewEnforcerSafe(params...)
+func newEnforcerSafe(params ...interface{}) (e CasbinEnforcer, err error) {
+	defer func() {
+		if r := recover(); r != nil {
+			err = fmt.Errorf("%v", r)
+			e = nil
+		}
+	}()
+	enfs := casbin.NewCachedEnforcer(params...)
 	if err != nil {
 		return nil, err
 	}
@@ -72,22 +158,38 @@ func newEnforcerSafe(params ...interface{}) (e *casbin.Enforcer, err error) {
 func NewEnforcer(clientset kubernetes.Interface, namespace, configmap string, claimsEnforcer ClaimsEnforcerFunc) *Enforcer {
 	adapter := newAdapter("", "", "")
 	builtInModel := newBuiltInModel()
-	enf, err := newEnforcerSafe(builtInModel, adapter)
-	if err != nil {
-		panic(err)
-	}
-	enf.EnableLog(false)
 	return &Enforcer{
-		Enforcer:           enf,
+		enforcerCache:      gocache.New(time.Hour, time.Hour),
 		adapter:            adapter,
 		clientset:          clientset,
 		namespace:          namespace,
 		configmap:          configmap,
 		model:              builtInModel,
 		claimsEnforcerFunc: claimsEnforcer,
+		enabled:            true,
 	}
 }
 
+// EnableLog executes casbin.Enforcer functionality.
+func (e *Enforcer) EnableLog(s bool) {
+	e.invalidateCache(func() {
+		e.enableLog = s
+	})
+}
+
+// EnableEnforce executes casbin.Enforcer functionality and will invalidate cache if required.
+func (e *Enforcer) EnableEnforce(s bool) {
+	e.invalidateCache(func() {
+		e.enabled = s
+	})
+}
+
+// LoadPolicy executes casbin.Enforcer functionality and will invalidate cache if required.
+func (e *Enforcer) LoadPolicy() error {
+	_, err := e.tryGetCabinEnforcer("", "")
+	return err
+}
+
 // Glob match func
 func globMatchFunc(args ...interface{}) (interface{}, error) {
 	if len(args) < 2 {
@@ -108,16 +210,11 @@ func globMatchFunc(args ...interface{}) (interface{}, error) {
 
 // SetMatchMode set match mode on runtime, glob match or regex match
 func (e *Enforcer) SetMatchMode(mode string) {
-	if mode == RegexMatchMode {
-		e.matchMode = RegexMatchMode
-	} else {
-		e.matchMode = GlobMatchMode
-	}
-	e.Enforcer.AddFunction("globOrRegexMatch", func(args ...interface{}) (interface{}, error) {
+	e.invalidateCache(func() {
 		if mode == RegexMatchMode {
-			return util.RegexMatchFunc(args...)
+			e.matchMode = RegexMatchMode
 		} else {
-			return globMatchFunc(args...)
+			e.matchMode = GlobMatchMode
 		}
 	})
 }
@@ -138,7 +235,7 @@ func (e *Enforcer) SetClaimsEnforcerFunc(claimsEnforcer ClaimsEnforcerFunc) {
 // Enforce is a wrapper around casbin.Enforce to additionally enforce a default role and a custom
 // claims function
 func (e *Enforcer) Enforce(rvals ...interface{}) bool {
-	return enforce(e.Enforcer, e.defaultRole, e.claimsEnforcerFunc, rvals...)
+	return enforce(e.getCabinEnforcer("", ""), e.defaultRole, e.claimsEnforcerFunc, rvals...)
 }
 
 // EnforceErr is a convenience helper to wrap a failed enforcement with a detailed error about the request
@@ -173,36 +270,25 @@ func (e *Enforcer) EnforceErr(rvals ...interface{}) error {
 // EnforceRuntimePolicy enforces a policy defined at run-time which augments the built-in and
 // user-defined policy. This allows any explicit denies of the built-in, and user-defined policies
 // to override the run-time policy. Runs normal enforcement if run-time policy is empty.
-func (e *Enforcer) EnforceRuntimePolicy(policy string, rvals ...interface{}) bool {
-	enf := e.CreateEnforcerWithRuntimePolicy(policy)
+func (e *Enforcer) EnforceRuntimePolicy(project string, policy string, rvals ...interface{}) bool {
+	enf := e.CreateEnforcerWithRuntimePolicy(project, policy)
 	return e.EnforceWithCustomEnforcer(enf, rvals...)
 }
 
 // CreateEnforcerWithRuntimePolicy creates an enforcer with a policy defined at run-time which augments the built-in and
 // user-defined policy. This allows any explicit denies of the built-in, and user-defined policies
 // to override the run-time policy. Runs normal enforcement if run-time policy is empty.
-func (e *Enforcer) CreateEnforcerWithRuntimePolicy(policy string) *casbin.Enforcer {
-	var enf *casbin.Enforcer
-	var err error
-	if policy == "" {
-		enf = e.Enforcer
-	} else {
-		enf, err = newEnforcerSafe(newBuiltInModel(), newAdapter(e.adapter.builtinPolicy, e.adapter.userDefinedPolicy, policy))
-		if err != nil {
-			log.Warnf("invalid runtime policy: %s", policy)
-			enf = e.Enforcer
-		}
-	}
-	return enf
+func (e *Enforcer) CreateEnforcerWithRuntimePolicy(project string, policy string) CasbinEnforcer {
+	return e.getCabinEnforcer(project, policy)
 }
 
 // EnforceWithCustomEnforcer wraps enforce with an custom enforcer
-func (e *Enforcer) EnforceWithCustomEnforcer(enf *casbin.Enforcer, rvals ...interface{}) bool {
+func (e *Enforcer) EnforceWithCustomEnforcer(enf CasbinEnforcer, rvals ...interface{}) bool {
 	return enforce(enf, e.defaultRole, e.claimsEnforcerFunc, rvals...)
 }
 
 // enforce is a helper to additionally check a default role and invoke a custom claims enforcement function
-func enforce(enf *casbin.Enforcer, defaultRole string, claimsEnforcerFunc ClaimsEnforcerFunc, rvals ...interface{}) bool {
+func enforce(enf CasbinEnforcer, defaultRole string, claimsEnforcerFunc ClaimsEnforcerFunc, rvals ...interface{}) bool {
 	// check the default role
 	if defaultRole != "" && len(rvals) >= 2 {
 		if enf.Enforce(append([]interface{}{defaultRole}, rvals[1:]...)...) {
@@ -231,13 +317,17 @@ func enforce(enf *casbin.Enforcer, defaultRole string, claimsEnforcerFunc Claims
 
 // SetBuiltinPolicy sets a built-in policy, which augments any user defined policies
 func (e *Enforcer) SetBuiltinPolicy(policy string) error {
-	e.adapter.builtinPolicy = policy
+	e.invalidateCache(func() {
+		e.adapter.builtinPolicy = policy
+	})
 	return e.LoadPolicy()
 }
 
 // SetUserPolicy sets a user policy, augmenting the built-in policy
 func (e *Enforcer) SetUserPolicy(policy string) error {
-	e.adapter.userDefinedPolicy = policy
+	e.invalidateCache(func() {
+		e.adapter.userDefinedPolicy = policy
+	})
 	return e.LoadPolicy()
 }
 

diff --git a/util/rbac/rbac_test.go b/util/rbac/rbac_test.go
@@ -236,9 +236,9 @@ func TestDefaultRoleWithRuntimePolicy(t *testing.T) {
 	err := enf.syncUpdate(fakeConfigMap(), noOpUpdate)
 	assert.Nil(t, err)
 	runtimePolicy := assets.BuiltinPolicyCSV
-	assert.False(t, enf.EnforceRuntimePolicy(runtimePolicy, "bob", "applications", "get", "foo/bar"))
+	assert.False(t, enf.EnforceRuntimePolicy("", runtimePolicy, "bob", "applications", "get", "foo/bar"))
 	enf.SetDefaultRole("role:readonly")
-	assert.True(t, enf.EnforceRuntimePolicy(runtimePolicy, "bob", "applications", "get", "foo/bar"))
+	assert.True(t, enf.EnforceRuntimePolicy("", runtimePolicy, "bob", "applications", "get", "foo/bar"))
 }
 
 // TestClaimsEnforcerFuncWithRuntimePolicy tests the ability for claims enforcer function to still
@@ -252,11 +252,11 @@ func TestClaimsEnforcerFuncWithRuntimePolicy(t *testing.T) {
 	claims := jwt.StandardClaims{
 		Subject: "foo",
 	}
-	assert.False(t, enf.EnforceRuntimePolicy(runtimePolicy, claims, "applications", "get", "foo/bar"))
+	assert.False(t, enf.EnforceRuntimePolicy("", runtimePolicy, claims, "applications", "get", "foo/bar"))
 	enf.SetClaimsEnforcerFunc(func(claims jwt.Claims, rvals ...interface{}) bool {
 		return true
 	})
-	assert.True(t, enf.EnforceRuntimePolicy(runtimePolicy, claims, "applications", "get", "foo/bar"))
+	assert.True(t, enf.EnforceRuntimePolicy("", runtimePolicy, claims, "applications", "get", "foo/bar"))
 }
 
 // TestInvalidRuntimePolicy tests when an invalid policy is supplied, it falls back to normal enforcement
@@ -267,11 +267,11 @@ func TestInvalidRuntimePolicy(t *testing.T) {
 	err := enf.syncUpdate(fakeConfigMap(), noOpUpdate)
 	assert.Nil(t, err)
 	_ = enf.SetBuiltinPolicy(assets.BuiltinPolicyCSV)
-	assert.True(t, enf.EnforceRuntimePolicy("", "admin", "applications", "update", "foo/bar"))
-	assert.False(t, enf.EnforceRuntimePolicy("", "role:readonly", "applications", "update", "foo/bar"))
+	assert.True(t, enf.EnforceRuntimePolicy("", "", "admin", "applications", "update", "foo/bar"))
+	assert.False(t, enf.EnforceRuntimePolicy("", "", "role:readonly", "applications", "update", "foo/bar"))
 	badPolicy := "this, is, not, a, good, policy"
-	assert.True(t, enf.EnforceRuntimePolicy(badPolicy, "admin", "applications", "update", "foo/bar"))
-	assert.False(t, enf.EnforceRuntimePolicy(badPolicy, "role:readonly", "applications", "update", "foo/bar"))
+	assert.True(t, enf.EnforceRuntimePolicy("", badPolicy, "admin", "applications", "update", "foo/bar"))
+	assert.False(t, enf.EnforceRuntimePolicy("", badPolicy, "role:readonly", "applications", "update", "foo/bar"))
 }
 
 func TestValidatePolicy(t *testing.T) {