-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
v2http: allow empty role for GET '/users' #5265
Conversation
Previous:
Now it returns
Please review. Thanks. |
writeError(w, r, err) | ||
return | ||
if err != nil { // in case, role does not exist | ||
plog.Warningf("GetRole %s error (%v)", roleName, err) |
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.
should this continue
instead of falling through and appending an empty role?
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.
Right, will fix. Thanks.
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.
Just fixed. PTAL. Thanks.
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 guess we should just remove the warning. this is nothing actually... etcd-users can assign a non-existing role to a user.
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.
Sure, will remove.
3c1c0a8
to
5d358a3
Compare
lgtm, deferring to @xiang90 |
@@ -116,6 +116,29 @@ func TestV2CurlIssue5182(t *testing.T) { | |||
} | |||
} | |||
|
|||
func TestV2CurlIssue5246(t *testing.T) { |
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.
this test does not really belong here. we should add a test in client_auth_test.go. e2e test is to ensure the correctness of etcdctl/etcd interaction, not to ensure the correctness of the server itself.
@xiang90 I moved the test into |
@xiang90 Now test only tests the PTAL |
t.Fatal(err) | ||
} | ||
for _, u := range uc.Users { | ||
if u.User == "root" { |
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.
first we need to ensure the user root does exist.
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.
first we need to ensure the user root does exist.
Can you explain more? Current output is:
{"users":[{"user":"root","roles":[{"role":"root","permissions":{"kv":{"read":["/"],"write":["/"]}}}]}]}
I thought we want to return empty role just for the non-existing foo
?
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.
Ok nvm. You said does exist
... :) Will fix.
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.
this test does not ensure user root exist. it only fails if root
exists and foo
exists. it should also fail if root
does not exist.
All addressed. PTAL. |
t.Fatal(err) | ||
} | ||
rootExist := false | ||
for _, u := range uc.Users { |
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.
just remove this range. get user root directly.
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.
if len(us.Users) != 1 {t.fatal(...)}
u := us.Users[0]
if u.User != "root" {t.fatal()}
...
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.
ok will fix.
02ba10a
to
433b25c
Compare
@xiang90 Actually, we can't check the user number because we use that mockStore for other tests, so AllUsers method returns 3 users now... https://github.com/coreos/etcd/blob/master/etcdserver/api/v2http/client_auth_test.go#L46 |
Can we fix it? I do not really care about that 3 actually. I should just get the user out of the users and do what we need to do instead of ranging over every users. That makes the code more readable. |
@xiang90 We can have another type of mock store for this test, or remove test cases using those 3 users. Which one do we prefer? |
@gyuho Which one is simpler. Or just keep the 3 there but get the root user out of them. |
@xiang90 I fixed the AllUsers for mock store to make it work for our new test. Works now. PTAL. Thanks! |
LGTM |
Thanks. Merging in greens. |
Fix #5246.