Skip to content
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 e2e auth tests to common #8 #15867

Merged
merged 1 commit into from
May 13, 2023
Merged

Conversation

chaochn47
Copy link
Member

Related to the ask from #14041 to split tests into smaller pieces. This PR is the last one and should close #14041.

So code review will be reviewer friendly and easier to reason about.

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

/cc @ahrtr @serathius @mitake @ptabor @fuweid

@@ -303,29 +296,6 @@ func authTestWatch(cx ctlCtx) {

}

func authTestDefrag(cx ctlCtx) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -420,83 +420,6 @@ func TestV3AuthOldRevConcurrent(t *testing.T) {
wg.Wait()
}

func TestV3AuthRestartMember(t *testing.T) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the same verification as authTestCacheReload in ctl_v3_auth_test.go and it was added in #14358

@@ -583,86 +506,3 @@ func TestV3AuthWatchErrorAndWatchId0(t *testing.T) {

<-watchEndCh
}

func TestV3AuthWithLeaseTimeToLive(t *testing.T) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the same verification of authTestLeaseTimeToLive in ctl_v3_auth_test.go and was added in #15656

Copy link
Contributor

@mitake mitake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks a lot @chaochn47

Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks @chaochn47

<-time.After(3 * time.Second)

// e2e test will generate a new token while
// integration test that re-uses the same etcd client will refresh the token on server failure.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we validate that we refreshed token?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it is feasible if

  1. etcd server log level is debug level
  2. capture the etcd server log output

etcd/server/auth/store.go

Lines 339 to 343 in 4da39e4

as.lg.Debug(
"authenticated a user",
zap.String("user-name", username),
zap.String("token", token),
)

clus.Members()[0].Stop()
require.NoError(t, clus.Members()[0].Start(ctx))

// nothing has changed, but it fails without refreshing cache after restart
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't understand this comment. How do we know it would fail?

Copy link
Member Author

@chaochn47 chaochn47 May 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this refreshRangePermCache in NewAuthStore on server restart,

as.refreshRangePermCache(tx)

permissions are not reloaded from disk. So this test will fail.

image

Copy link
Member

@serathius serathius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider my comments for followups.

@chaochn47 chaochn47 force-pushed the auth_test_split_8 branch from 0cdd36a to c846b08 Compare May 13, 2023 05:52
@ahrtr ahrtr merged commit 52dfd4b into etcd-io:main May 13, 2023
@chaochn47 chaochn47 deleted the auth_test_split_8 branch May 13, 2023 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants