acl: remove ResolveTokenToIdentity #12166

dnephin · 2022-01-22T19:49:03Z

Branched from #12165, continues the work described in #11690

So that we can duplicate duplicate methods.

By exposing the AccessorID from the primary ResolveToken method we can remove this duplication.

We already have an ACLResolveResult, so we can get the accessor ID from it.

dnephin · 2022-01-23T18:28:15Z

agent/consul/acl_server.go

+// TODO: remove
 func (s *Server) ResolveToken(token string) (acl.Authorizer, error) {


This is done in #12167

freddygv

This generally looks good, my only concern is that now some of the calls to get the accessor ID are more expensive. Left some comments below, let me know what you think.

freddygv · 2022-01-26T20:11:23Z

agent/local/state.go

@@ -1538,16 +1539,13 @@ func (l *State) notifyIfAliased(serviceID structs.ServiceID) {
 // critical purposes, such as logging. Therefore we interpret all errors as empty-string
 // so we can safely log it without handling non-critical errors at the usage site.
 func (l *State) aclAccessorID(secretID string) string {
-	ident, err := l.Delegate.ResolveTokenToIdentity(secretID)


Isn't this doing significantly more work now? Maybe not for this PR, but could we avoid building the authorizer here? This call is fairly expensive from what I've seen in some CPU profiles

Yes, good point!

We only call this method when an RPC call returns either:

acl.ErrNotFound

acl.ErrPermissionDenied

So it should be pretty rare, but we do have some options to make it less expensive.

I was going to suggest we could cache the AccessorID, but I noticed ACLResolver already caches the Authorizer. It seems like we cache up to 256 Authorizers (1024 on servers), so I wonder how likely it would be that we miss the cache.

We could restore the method to retrieve only the Identity from a token, but I'm not sure if these rare cases are enough to warrant that extra code.

We could also decide to add a new cache of secretID -> AccessorID that is local to only the agent/local.State.

I wonder if we could actually remove this call entirely soon. We are planning on adding the AccessorID to "permission denied" error. Presumably if the error is "acl not found", then there is no accessor ID to return (or if it's a policy that is not found, we could include the accessor ID in that error as well).

Given that we are planning on improve those errors in the next release, I wonder if we could leave this as-is, and remove this aclAccessorID method once we've enriched the errors on the server side. Does that sound like a good approach? I think that gives us better errors without having to do any kind of lookup for AccessorID.

That seems pretty reasonable; can we capture this work somewhere to make sure it isn't missed?

Good call! I opened #12233 to track this cleanup.

freddygv · 2022-01-26T20:12:20Z

agent/acl.go

@@ -15,18 +15,15 @@ import (
 // critical purposes, such as logging. Therefore we interpret all errors as empty-string
 // so we can safely log it without handling non-critical errors at the usage site.
 func (a *Agent) aclAccessorID(secretID string) string {
-	ident, err := a.delegate.ResolveTokenToIdentity(secretID)


Same concern here

I see this method being called in two places: Agent.filterMembers, and Agent.sendCoordinate.

Agent.sendCoordinate is similar to the other case in agent/local. It's only done when we receive a permission denied error, and we will be included the accessorID in that error soon, so we should be able to remove that call entirely.

The second call Agent.filterMembers already has an acl.Authorizer. I missed this on the first pass! I'll push a commit which removes this call, and uses authz.AccessorID() to get the value. (edit: Done in the latest commit)

freddygv · 2022-01-26T20:13:46Z

agent/consul/intention_endpoint.go


 			// If ACLs prevented any responses, error
 			if len(reply.Intentions) == 0 {
-				accessorID := s.aclAccessorID(args.Token)
+				accessorID := authz.AccessorID()


This is great, love that this avoids re-resolving the token

dnephin

Ya, good call! Most of the changes here reduce work, but there are 3 cases in agent (or agent/local) where we do more work. I'll comment inline on each of them. I suspect we'll be able to remove those calls soon.

dnephin · 2022-01-26T22:14:55Z

agent/local/state.go

@@ -1538,16 +1539,13 @@ func (l *State) notifyIfAliased(serviceID structs.ServiceID) {
 // critical purposes, such as logging. Therefore we interpret all errors as empty-string
 // so we can safely log it without handling non-critical errors at the usage site.
 func (l *State) aclAccessorID(secretID string) string {
-	ident, err := l.Delegate.ResolveTokenToIdentity(secretID)


Yes, good point!

We only call this method when an RPC call returns either:

acl.ErrNotFound

acl.ErrPermissionDenied

So it should be pretty rare, but we do have some options to make it less expensive.

I was going to suggest we could cache the AccessorID, but I noticed ACLResolver already caches the Authorizer. It seems like we cache up to 256 Authorizers (1024 on servers), so I wonder how likely it would be that we miss the cache.

We could restore the method to retrieve only the Identity from a token, but I'm not sure if these rare cases are enough to warrant that extra code.

We could also decide to add a new cache of secretID -> AccessorID that is local to only the agent/local.State.

I wonder if we could actually remove this call entirely soon. We are planning on adding the AccessorID to "permission denied" error. Presumably if the error is "acl not found", then there is no accessor ID to return (or if it's a policy that is not found, we could include the accessor ID in that error as well).

Given that we are planning on improve those errors in the next release, I wonder if we could leave this as-is, and remove this aclAccessorID method once we've enriched the errors on the server side. Does that sound like a good approach? I think that gives us better errors without having to do any kind of lookup for AccessorID.

dnephin · 2022-01-26T22:19:09Z

agent/acl.go

@@ -15,18 +15,15 @@ import (
 // critical purposes, such as logging. Therefore we interpret all errors as empty-string
 // so we can safely log it without handling non-critical errors at the usage site.
 func (a *Agent) aclAccessorID(secretID string) string {
-	ident, err := a.delegate.ResolveTokenToIdentity(secretID)


I see this method being called in two places: Agent.filterMembers, and Agent.sendCoordinate.

Agent.sendCoordinate is similar to the other case in agent/local. It's only done when we receive a permission denied error, and we will be included the accessorID in that error soon, so we should be able to remove that call entirely.

The second call Agent.filterMembers already has an acl.Authorizer. I missed this on the first pass! I'll push a commit which removes this call, and uses authz.AccessorID() to get the value. (edit: Done in the latest commit)

I missed this on the first pass, we no longer need to look up this ID, because we have it from the Authorizer.

dnephin · 2022-01-31T19:04:36Z

agent/consul/acl.go

 	}
-
-	defer metrics.MeasureSince([]string{"acl", "ResolveTokenToIdentity"}, time.Now())


I just noticed I am removing this metric. I'm going to push one more commit that documents that this metric is being removed. I'll add a changelog as well.

Also document the metric that was removed in a previous commit.

hc-github-team-consul-core · 2022-02-01T00:19:55Z

🍒 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/571786.

dnephin added 2 commits January 22, 2022 14:33

acl: return a resposne from ResolveToken that includes the ACLIdentity

a5e8af7

So that we can duplicate duplicate methods.

acl: remove ResolveTokenToIdentity

edca8d6

By exposing the AccessorID from the primary ResolveToken method we can remove this duplication.

dnephin added theme/acls ACL and token generation theme/internal-cleanup Used to identify tech debt, testing improvements, code refactoring, and non-impactful optimization pr/no-changelog PR does not need a corresponding .changelog entry labels Jan 22, 2022

github-actions bot removed the theme/acls ACL and token generation label Jan 22, 2022

dnephin changed the title ~~Dnephin/acl resolve token 2~~ acl: remove ResolveTokenToIdentity Jan 22, 2022

acl: remove calls to ResolveIdentityFromToken

e134e43

We already have an ACLResolveResult, so we can get the accessor ID from it.

vercel bot temporarily deployed to Preview – consul January 22, 2022 20:10 Inactive

vercel bot temporarily deployed to Preview – consul-ui-staging January 22, 2022 20:10 Inactive

dnephin requested a review from a team January 22, 2022 20:10

This was referenced Jan 22, 2022

acl: extract a backend type for the ACLResolverBackend #11221

Merged

acl: rename ResolveTokenToIdentityAndAuthorizer to ResolveToken #12167

Merged

dnephin commented Jan 23, 2022

View reviewed changes

freddygv reviewed Jan 26, 2022

View reviewed changes

dnephin commented Jan 26, 2022

View reviewed changes

acl: Remove a call to aclAccessorID

7a6e03c

I missed this on the first pass, we no longer need to look up this ID, because we have it from the Authorizer.

vercel bot temporarily deployed to Preview – consul January 26, 2022 22:22 Inactive

vercel bot temporarily deployed to Preview – consul-ui-staging January 26, 2022 22:22 Inactive

markan self-assigned this Jan 28, 2022

markan approved these changes Jan 29, 2022

View reviewed changes

Base automatically changed from dnephin/acl-resolve-token to main January 31, 2022 18:27

dnephin mentioned this pull request Jan 31, 2022

acl: remove aclAccessorID methods #12233

Open

dnephin commented Jan 31, 2022

View reviewed changes

dnephin removed the pr/no-changelog PR does not need a corresponding .changelog entry label Jan 31, 2022

dnephin requested a review from a team as a code owner January 31, 2022 19:22

vercel bot deployed to Preview – consul January 31, 2022 19:22 View deployment

vercel bot temporarily deployed to Preview – consul-ui-staging January 31, 2022 19:22 Inactive

dnephin added the pr/no-metrics-test label Jan 31, 2022

acl: remove unused methods on fakes, and add changelog

d363cc0

Also document the metric that was removed in a previous commit.

dnephin force-pushed the dnephin/acl-resolve-token-2 branch from e2dd510 to d363cc0 Compare January 31, 2022 22:54

vercel bot temporarily deployed to Preview – consul-ui-staging January 31, 2022 22:54 Inactive

vercel bot deployed to Preview – consul January 31, 2022 22:54 View deployment

dnephin merged commit 997bf1e into main Feb 1, 2022

dnephin deleted the dnephin/acl-resolve-token-2 branch February 1, 2022 00:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

acl: remove ResolveTokenToIdentity #12166

acl: remove ResolveTokenToIdentity #12166

dnephin commented Jan 22, 2022

dnephin Jan 23, 2022

freddygv left a comment

freddygv Jan 26, 2022

dnephin Jan 26, 2022

markan Jan 29, 2022

dnephin Jan 31, 2022

freddygv Jan 26, 2022

dnephin Jan 26, 2022 •

edited

Loading

freddygv Jan 26, 2022

dnephin left a comment

dnephin Jan 26, 2022

dnephin Jan 26, 2022 •

edited

Loading

dnephin Jan 31, 2022

hc-github-team-consul-core commented Feb 1, 2022

		// TODO: remove
		func (s *Server) ResolveToken(token string) (acl.Authorizer, error) {

		}

		defer metrics.MeasureSince([]string{"acl", "ResolveTokenToIdentity"}, time.Now())

acl: remove ResolveTokenToIdentity #12166

acl: remove ResolveTokenToIdentity #12166

Conversation

dnephin commented Jan 22, 2022

Choose a reason for hiding this comment

freddygv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnephin Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnephin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnephin Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hc-github-team-consul-core commented Feb 1, 2022

dnephin Jan 26, 2022 •

edited

Loading

dnephin Jan 26, 2022 •

edited

Loading