-
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
Give descriptive error if auth method not found #10163
Conversation
4f1f60f
to
984e50d
Compare
984e50d
to
2a75560
Compare
Previously during a `consul login -method=blah`, if the auth method was not found, the error returned would be "ACL not found". This is potentially confusing because there may be many different ACLs involved in a login: the ACL of the Consul client, perhaps the binding rule or the auth method. Now the error will be "auth method blah not found", which is much easier to debug.
2a75560
to
9dd9013
Compare
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.
Thanks @lkysow for working on improving the error message! Left some comments about how we can keep the 4xx status code.
command/login/login_test.go
Outdated
@@ -143,7 +143,7 @@ func TestLoginCommand(t *testing.T) { | |||
|
|||
code := cmd.Run(args) | |||
require.Equal(t, code, 1, "err: %s", ui.ErrorWriter.String()) | |||
require.Contains(t, ui.ErrorWriter.String(), "403 (ACL not found)") | |||
require.Contains(t, ui.ErrorWriter.String(), "500 (auth method \"test\" not found)") |
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.
I don't think we want a 500 response code for this. Either a 404 or 403 seems appropriate.
There's some related context in #8520 (comment) (and the associated issue).
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.
Totally agreed, didn't know how to do that but your suggestion works like a charm!
agent/consul/acl_endpoint.go
Outdated
@@ -2380,7 +2380,7 @@ func (a *ACL) Login(args *structs.ACLLoginRequest, reply *structs.ACLToken) erro | |||
if err != nil { | |||
return err | |||
} else if method == nil { | |||
return acl.ErrNotFound | |||
return fmt.Errorf("auth method %q not found", auth.AuthMethod) |
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.
Oops, forgot to submit this comment:
I think we'll need something like this:
return fmt.Errorf("auth method %q not found", auth.AuthMethod) | |
return fmt.Errorf("auth method %q: %w", auth.AuthMethod, acl.ErrNotFound) |
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.
I swapped it around so it reads ACL not found: auth method <blah> not found
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.
LGTM!
🍒 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/361410. |
🍒✅ Cherry pick of commit 8d6cbe7 onto |
* Give descriptive error if auth method not found Previously during a `consul login -method=blah`, if the auth method was not found, the error returned would be "ACL not found". This is potentially confusing because there may be many different ACLs involved in a login: the ACL of the Consul client, perhaps the binding rule or the auth method. Now the error will be "auth method blah not found", which is much easier to debug.
🍒✅ Cherry pick of commit 8d6cbe7 onto |
* Give descriptive error if auth method not found Previously during a `consul login -method=blah`, if the auth method was not found, the error returned would be "ACL not found". This is potentially confusing because there may be many different ACLs involved in a login: the ACL of the Consul client, perhaps the binding rule or the auth method. Now the error will be "auth method blah not found", which is much easier to debug.
Previously during a
consul login -method=blah
, if the auth method was not found, theerror returned would be "ACL not found". This is potentially confusing
because there may be many different ACLs involved in a login: the ACL of
the Consul client, perhaps the binding rule or the auth method.
Now the error will be "auth method blah not found", which is much easier
to debug.