-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
acl: demonstrate ResolveTokenForRequest #12168
Conversation
type ACLRequest interface { | ||
TokenSecret() string | ||
|
||
// Merge sets the fields in this request to the values from source, if the | ||
// request does not already have a value. | ||
// TODO: rename | ||
Merge(source *structs.EnterpriseMeta) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost every one of our existing request structs already implements this interface (as described in #11690). Some may not have a Merge
method if they don't have EnterpriseMeta, but we can easily implement a no-op version to embed in those structs.
// Merge sets the fields in this request to the values from source, if the | ||
// request does not already have a value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Merge sets the fields in this request to the values from source, if the | |
// request does not already have a value. | |
// Merge sets the fields in this request to the values from source, if the | |
// request does not already have a value for those fields. |
This change allows us to remove one of the last remaining duplicate resolve token methods (Server.ResolveToken). With this change we are down to only 2, where the second one also handles setting the default EnterpriseMeta from the token.
3785314
to
8ace474
Compare
c448e1b
to
292e9bc
Compare
e4d0b9f
to
343b6de
Compare
This pull request has been automatically flagged for inactivity because it has not been acted upon in the last 60 days. It will be closed if no new activity occurs in the next 30 days. Please feel free to re-open to resurrect the change if you feel this has happened by mistake. Thank you for your contributions. |
Closing due to inactivity. If you feel this was a mistake or you wish to re-open at any time in the future, please leave a comment and it will be re-surfaced for the maintainers to review. |
Branched from #12167, this PR demonstrates the proposed (in #11690) replacement for
ResolveTokenAndDefaultMeta
.This new method
ResolveTokenForRequest
accepts a single arg and handles setting the default enterprise meta from the token.It does not set those values in the
AuthorizerContext
. Very few calls toResolveTokenAndDefaultMeta
actually make use of thisAuthorizerContext
arg. As part of the migration we can populate theAuthorizerContext
from the caller, but in the future we should be able to remove the need forAuthorizerContext
by instead accepting an interface to the methods onAuthorizer
. An example of that can be seen in #12169.TODO:
ResolveTokenAndDefaultMeta
withResolveTokenForRequest