luxolus
/
open-consul
2
0
Fork 0
Code Issues 1 Pull Requests Packages Releases Wiki Activity

Merge pull request #12885 from hashicorp/acl-err-cache 

Store and return RPC error in ACL cache entries
This commit is contained in:
Kyle Havlovitz 2022-05-03 10:44:22 -07:00 committed by GitHub
parent b1a1ddf78f 3bd001fb29
commit 369f4848e3
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 60 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
.changelog/12885.txt Normal file
View File

@ -0,0 +1,3 @@
```release-note:bug
acl: Fixed a bug where the ACL down policy wasn't being applied on remote errors from the primary datacenter.
```

24
agent/consul/acl.go
View File

@ -360,20 +360,20 @@ func (r *ACLResolver) fetchAndCacheIdentityFromToken(token string, cached *struc
err := r.backend.RPC("ACL.TokenRead", &req, &resp)
if err == nil {
if resp.Token == nil {
r.cache.PutIdentity(cacheID, nil)
r.cache.PutIdentity(cacheID, nil, nil)
return nil, acl.ErrNotFound
} else if resp.Token.Local && r.config.Datacenter != resp.SourceDatacenter {
r.cache.PutIdentity(cacheID, nil)
r.cache.PutIdentity(cacheID, nil, nil)
return nil, acl.PermissionDeniedError{Cause: fmt.Sprintf("This is a local token in datacenter %q", resp.SourceDatacenter)}
} else {
r.cache.PutIdentity(cacheID, resp.Token)
r.cache.PutIdentity(cacheID, resp.Token, nil)
return resp.Token, nil
}
}
if acl.IsErrNotFound(err) {
// Make sure to remove from the cache if it was deleted
r.cache.PutIdentity(cacheID, nil)
r.cache.PutIdentity(cacheID, nil, err)
return nil, acl.ErrNotFound
}
@ -381,11 +381,11 @@ func (r *ACLResolver) fetchAndCacheIdentityFromToken(token string, cached *struc
// some other RPC error
if cached != nil && (r.config.ACLDownPolicy == "extend-cache" || r.config.ACLDownPolicy == "async-cache") {
// extend the cache
r.cache.PutIdentity(cacheID, cached.Identity)
r.cache.PutIdentity(cacheID, cached.Identity, err)
return cached.Identity, nil
}
r.cache.PutIdentity(cacheID, nil)
r.cache.PutIdentity(cacheID, nil, err)
return nil, err
}
@ -402,7 +402,10 @@ func (r *ACLResolver) resolveIdentityFromToken(token string) (structs.ACLIdentit
cacheEntry := r.cache.GetIdentity(tokenSecretCacheID(token))
if cacheEntry != nil && cacheEntry.Age() <= r.config.ACLTokenTTL {
metrics.IncrCounter([]string{"acl", "token", "cache_hit"}, 1)
return cacheEntry.Identity, nil
if cacheEntry.Error != nil && !acl.IsErrNotFound(cacheEntry.Error) {
return cacheEntry.Identity, ACLRemoteError{Err: cacheEntry.Error}
}
return cacheEntry.Identity, cacheEntry.Error
}
metrics.IncrCounter([]string{"acl", "token", "cache_miss"}, 1)
@ -416,7 +419,10 @@ func (r *ACLResolver) resolveIdentityFromToken(token string) (structs.ACLIdentit
waitForResult := cacheEntry == nil || r.config.ACLDownPolicy != "async-cache"
if !waitForResult {
// waitForResult being false requires the cacheEntry to not be nil
return cacheEntry.Identity, nil
if cacheEntry.Error != nil && !acl.IsErrNotFound(cacheEntry.Error) {
return cacheEntry.Identity, ACLRemoteError{Err: cacheEntry.Error}
}
return cacheEntry.Identity, cacheEntry.Error
}
// block on the read here, this is why we don't need chan buffering
@ -549,7 +555,7 @@ func (r *ACLResolver) maybeHandleIdentityErrorDuringFetch(identity structs.ACLId
if acl.IsErrNotFound(err) {
// make sure to indicate that this identity is no longer valid within
// the cache
r.cache.PutIdentity(tokenSecretCacheID(identity.SecretToken()), nil)
r.cache.PutIdentity(tokenSecretCacheID(identity.SecretToken()), nil, err)
// Do not touch the cache. Getting a top level ACL not found error
// only indicates that the secret token used in the request

38
agent/consul/acl_test.go
View File

@ -1264,6 +1264,44 @@ func TestACLResolver_DownPolicy(t *testing.T) {
requireIdentityCached(t, r, tokenSecretCacheID("found"), false, "no longer cached")
})
t.Run("Cache-Error", func(t *testing.T) {
_, rawToken, _ := testIdentityForToken("found")
foundToken := rawToken.(*structs.ACLToken)
secretID := foundToken.SecretID
remoteErr := fmt.Errorf("network error")
delegate := &ACLResolverTestDelegate{
enabled: true,
datacenter: "dc1",
legacy: false,
localTokens: false,
localPolicies: false,
tokenReadFn: func(_ *structs.ACLTokenGetRequest, reply *structs.ACLTokenResponse) error {
return remoteErr
},
}
r := newTestACLResolver(t, delegate, func(config *ACLResolverConfig) {
config.Config.ACLDownPolicy = "deny"
})
// Attempt to resolve the token - this should set up the cache entry with a nil
// identity and permission denied error.
authz, err := r.ResolveToken(secretID)
require.NoError(t, err)
require.NotNil(t, authz)
entry := r.cache.GetIdentity(tokenSecretCacheID(secretID))
require.Nil(t, entry.Identity)
require.Equal(t, remoteErr, entry.Error)
// Attempt to resolve again to pull from the cache.
authz, err = r.ResolveToken(secretID)
require.NoError(t, err)
require.NotNil(t, authz)
entry = r.cache.GetIdentity(tokenSecretCacheID(secretID))
require.Nil(t, entry.Identity)
require.Equal(t, remoteErr, entry.Error)
})
t.Run("PolicyResolve-TokenNotFound", func(t *testing.T) {
_, rawToken, _ := testIdentityForToken("found")
foundToken := rawToken.(*structs.ACLToken)

5
agent/structs/acl_cache.go
View File

@ -27,6 +27,7 @@ type ACLCaches struct {
type IdentityCacheEntry struct {
Identity ACLIdentity
CacheTime time.Time
Error error
}
func (e *IdentityCacheEntry) Age() time.Duration {
@ -187,12 +188,12 @@ func (c *ACLCaches) GetRole(roleID string) *RoleCacheEntry {
}
// PutIdentity adds a new identity to the cache
func (c *ACLCaches) PutIdentity(id string, ident ACLIdentity) {
func (c *ACLCaches) PutIdentity(id string, ident ACLIdentity, err error) {
if c == nil || c.identities == nil {
return
}
c.identities.Add(id, &IdentityCacheEntry{Identity: ident, CacheTime: time.Now()})
c.identities.Add(id, &IdentityCacheEntry{Identity: ident, CacheTime: time.Now(), Error: err})
}
func (c *ACLCaches) PutPolicy(policyId string, policy *ACLPolicy) {

2
agent/structs/acl_cache_test.go
View File

@ -47,7 +47,7 @@ func TestStructs_ACLCaches(t *testing.T) {
require.NoError(t, err)
require.NotNil(t, cache)
cache.PutIdentity("foo", &ACLToken{})
cache.PutIdentity("foo", &ACLToken{}, nil)
entry := cache.GetIdentity("foo")
require.NotNil(t, entry)
require.NotNil(t, entry.Identity)