Store and return RPC error in ACL cache entries #12885
Conversation
kyhavlov
added
pr/no-metrics-test
pr/no-docs
agent/consul/acl_test.go
Outdated
| // 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, acl.ErrPermissionDenied, entry.Error) |
There was a problem hiding this comment.
The original context was that after this initial ResolveToken primes the cache, the followup call would fail on acl.ErrNotFound and not apply the ACLDownPolicy (bug since it should fail on ACLRemoteError and apply the ACLDownPolicy).
I think this test needs to repeat the above chunk (you'll see that it fails the second time around because ResolveToken returns the cached error)
There was a problem hiding this comment.
After applying the proposed change in acl.go the second ResolveToken call passes for me
There was a problem hiding this comment.
If I wasn't clear:
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, acl.ErrPermissionDenied, entry.Error)
authz, err = r.ResolveToken(secretID)
require.NoError(t, err) <- your implementation fails here but it shouldn't
require.NotNil(t, authz)
entry = r.cache.GetIdentity(tokenSecretCacheID(secretID))
require.Nil(t, entry.Identity)
require.Equal(t, acl.ErrPermissionDenied, entry.Error)
| @@ -416,7 +416,7 @@ 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 | |||
| return cacheEntry.Identity, cacheEntry.Error | |||
There was a problem hiding this comment.
Since the caller has special handling for ACLRemoteError, we need to make sure we wrap the error like we do in L430, otherwise we'll miss the ACLRemoteError codepath
| return cacheEntry.Identity, cacheEntry.Error | |
| if cacheEntry.Error != nil && !acl.IsErrNotFound(cacheEntry.Error) { | |
| return cacheEntry.Identity, ACLRemoteError{Err: cacheEntry.Error} | |
| } | |
| return cacheEntry.Identity, cacheEntry.Error |
There was a problem hiding this comment.
Thanks, I totally missed this part about needing the return the err wrapped in an ACLRemoteError - I thought the bug was only that we weren't caching the error at all and returning an ErrNotFound when we shouldn't be
agent/consul/acl_test.go
Outdated
| localTokens: false, | ||
| localPolicies: false, | ||
| tokenReadFn: func(_ *structs.ACLTokenGetRequest, reply *structs.ACLTokenResponse) error { | ||
| return acl.ErrPermissionDenied |
There was a problem hiding this comment.
slight nit but can we return something like testErr := fmt.Errorf("network error")?
The type of error returned here isn't too important but I think using an error from the acl package (and the fact that we have special handling for a bunch of ACL error types) makes it a bit confusing.
|
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/659162. |
This PR adds an
Errorfield to theIdentityCacheEntryto preserve the error returned when attempting to resolve a token. Currently we returnacl.ErrNotFoundin this case which bubbles up to the client agent that made the request and the down policy ends up not being respected if it's set toallow.