-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
migrate auth_test to common #14041
migrate auth_test to common #14041
Conversation
@serathius Can I get some early feebacks if the common test framework auth implementation makes sense to you? Thanks! |
Codecov Report
@@ Coverage Diff @@
## main #14041 +/- ##
==========================================
- Coverage 75.80% 74.65% -1.16%
==========================================
Files 457 457
Lines 37421 37391 -30
==========================================
- Hits 28367 27914 -453
- Misses 7294 7747 +453
+ Partials 1760 1730 -30
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
cc @mitake |
@@ -179,6 +179,8 @@ type Cluster struct { | |||
|
|||
mu sync.Mutex | |||
clusterClient *clientv3.Client | |||
|
|||
clusterAuthClients []*clientv3.Client |
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.
There is already a lot of different types of clients stored just to call .Close()
, maybe we should just store list destructor functions? Even better would be to use t.Cleanup(func())
for call client.Close.
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.
maybe we should just store list destructor functions
I think that's a good idea. Maybe we could optimize it in a follow-up PR?
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 mean this PR could just avoid introducing clusterAuthClients []*clientv3.Client
, by using cleanup []func()
. We can migrate others in future.
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.
okay. Make sense.
a174737
to
394540f
Compare
The failed test is unrelated to the PR.
ref. https://github.com/etcd-io/etcd/runs/7667198807?check_suite_focus=true The symptom looks like similar to #14275. However, I think this type of flake is inevitable without embed server restart See this test causes |
Thanks @chaochn47 for the PR. More than 2K line of code change is too big, could you try to break down this PR into small ones? |
Sorry for the late response. Are you still working on this PR? |
The latest code is on top of #14626, once it is merged, rebase will make the code diff smaller. |
2810a71
to
4e312bc
Compare
Impacted by a flaky test case
|
Found that root cause and will file pr later. |
Signed-off-by: Chao Chen <[email protected]>
This PR is too big. So it is almost impossible to carefully check each line of code change. But overall looks clear and good to me. Please try to use Suggestion for similar PR in future: Please try to migrate the cases step by step. I am OK if other maintainers agree to merge this PR. So defer to @serathius and @spzala to merge or comment this PR. |
Another point is that please try to reuse the common functions in auth_util.go. If the existing functions do not meet some requirements, then please try to enhance the common functions, so that we can simplify the test cases as much as possible. |
} | ||
} | ||
|
||
func authCredWriteKeyTest(cx ctlCtx) { |
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.
Cant find this tests. Was it migrated under new name or removed?
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.
It was migrated to TestAuthWriteKey
.
assert.Equal(t, 3, len(memberList.Members), "want 3 member but got %d", len(memberList.Members)) | ||
id := memberList.Members[0].ID | ||
|
||
// 5 seconds is the minimum required amount of time peer is considered active |
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.
Where this come from? I don't see it in original authTestMemberRemove
code
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.
Looks like it's not needed.
func TestAuthLeaseTestTimeToLiveExpired(t *testing.T) { | ||
tcs := []struct { | ||
name string | ||
JWTEnabled bool |
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 see this used.
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 for catching that. Will fix this.
} | ||
} | ||
|
||
func TestAuthLeaseGrantLeases(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.
Note, there are couple of tests for leases that do not really tests authorization. They are normal lease testing scenarios with only difference that authorization is enabled.
We should investigate if we can expand standard lease tests and treat authorization as part of cluster setup. This way we will not need to duplicate test scenario (one for lease without auth, second with auth enabled).
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.
Agreed. I think we can move this post the migration to think about it in the long term.
t.Fatalf("want leaseID %v expired but not", leaseID) | ||
} | ||
gresp, err := rootAuthClient.Get(ctx, "key", config.GetOptions{}) | ||
if err != nil || len(gresp.Kvs) != 0 { |
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.
Note, no need to fix it in this PR. This checking for empty response will really benefit from rewrite to testify.
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.
ack. Will fix it.
authEnable(ctx, cc, t) | ||
rootAuthClient := testutils.MustClient(clus.Client(WithAuth("root", "root"))) | ||
authSetupDefaultTestUser(ctx, rootAuthClient, t) | ||
if _, err := rootAuthClient.RoleGet(ctx, "test-role"); err != nil { |
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.
Original test validated here contents of the role, like what keys it can write and what keys it can read.
expected := []string{
"Role test-role",
"KV Read:", "foo",
"KV Write:", "foo",
}
if err != nil { | ||
t.Fatal(err) | ||
} | ||
assert.ElementsMatch(t, resp.Roles, []string{"test-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.
Original test also validated user name in response.
if err != nil { | ||
t.Fatal(err) | ||
} | ||
assert.ElementsMatch(t, resp.Roles, []string{"test-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.
ditto
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.
Reviewed and left couple fo comments. I would highly encourage migrating one tests at a time to make review quick. This one review iteration took me over 30 minutes and we will need more iterations.
If you send a separate PR that just migrates one test, I expect that review would take 2 minutes and I good give more detailed feedback.
Thanks for the review, I will split those test cases into multiple incremental PRs. Each PR contains 2 test cases. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions. |
Related to #13637