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.

Sign up for GitHub

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

Jump to bottom

etcdctl: allow move-leader to connect to multiple endpoints with TLS #12757

Closed
wants to merge 1 commit into from
Closed

etcdctl: allow move-leader to connect to multiple endpoints with TLS #12757

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions etcdctl/ctlv3/command/move_leader_command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ func transferLeadershipCommandFunc(cmd *cobra.Command, args []string) {
cobrautl.ExitWithError(cobrautl.ExitBadArgs, err)
}

c := mustClientFromCmd(cmd)
cfg := clientConfigFromCmd(cmd)
c := cfg.mustClient()
eps := c.Endpoints()
c.Close()

Expand All @@ -53,7 +54,6 @@ func transferLeadershipCommandFunc(cmd *cobra.Command, args []string) {
var leaderCli *clientv3.Client
var leaderID uint64
for _, ep := range eps {
cfg := clientConfigFromCmd(cmd)
cfg.endpoints = []string{ep}
cli := cfg.mustClient()
resp, serr := cli.Status(ctx, ep)
Expand Down
21 changes: 16 additions & 5 deletions tests/e2e/ctl_v3_move_leader_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,25 +28,34 @@ import (
)

func TestCtlV3MoveLeaderSecure(t *testing.T) {
testCtlV3MoveLeader(t, *newConfigTLS())
testCtlV3MoveLeader(t, withCfg(*newConfigTLS()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why calling this twice is needed change? I would suggest keeping the existing tests as is and add a new test case that tests the different scenarios you have, e.g. endpoints connection. This way failures would be more evident. Unless I am missing something here?

testCtlV3MoveLeader(t, withCfg(*newConfigTLS()), withFlagByEnv())
}

func TestCtlV3MoveLeaderInsecure(t *testing.T) {
testCtlV3MoveLeader(t, *newConfigNoTLS())
testCtlV3MoveLeader(t, withCfg(*newConfigNoTLS()))
testCtlV3MoveLeader(t, withCfg(*newConfigNoTLS()), withFlagByEnv())
}

func testCtlV3MoveLeader(t *testing.T, cfg etcdProcessClusterConfig) {
func testCtlV3MoveLeader(t *testing.T, opts ...ctlOption) {
BeforeTest(t)

epc := setupEtcdctlTest(t, &cfg, true)
ret := ctlCtx{
t: t,
cfg: *newConfigAutoTLS(),
dialTimeout: 7 * time.Second,
}
ret.applyOpts(opts)

epc := setupEtcdctlTest(t, &ret.cfg, true)
defer func() {
if errC := epc.Close(); errC != nil {
t.Fatalf("error closing etcd processes (%v)", errC)
}
}()

var tcfg *tls.Config
if cfg.clientTLS == clientTLS {
if ret.cfg.clientTLS == clientTLS {
tinfo := transport.TLSInfo{
CertFile: certPath,
KeyFile: privateKeyPath,
Expand Down Expand Up @@ -94,11 +103,13 @@ func testCtlV3MoveLeader(t *testing.T, cfg etcdProcessClusterConfig) {
cfg: *newConfigNoTLS(),
dialTimeout: 7 * time.Second,
epc: epc,
envMap: map[string]struct{}{},
ptabor marked this conversation as resolved.
Show resolved Hide resolved
}

tests := []struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we need to have different testcases for different tests... let's expose tests as this method parameter.

Copy link
Author

Choose a reason for hiding this comment

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

I think we should have exact same test cases. I am executing:

func TestCtlV3MoveLeaderSecure(t *testing.T) {
	testCtlV3MoveLeader(t, withCfg(*newConfigTLS()))
	testCtlV3MoveLeader(t, withCfg(*newConfigTLS()), withFlagByEnv())
}

func TestCtlV3MoveLeaderInsecure(t *testing.T) {
	testCtlV3MoveLeader(t, withCfg(*newConfigNoTLS()))
	testCtlV3MoveLeader(t, withCfg(*newConfigNoTLS()), withFlagByEnv())
}

Move leader with and without env, which is correct way we need to test both. At the moment I am only fighting with:

	{
		{ // request to non-leader
			ret.prefixArgs([]string{ret.epc.EndpointsV3()[(leadIdx+1)%3]}),
			"no leader endpoint given at ",
		},
		{ // request to leader
			ret.prefixArgs([]string{ret.epc.EndpointsV3()[leadIdx]}),
			fmt.Sprintf("Leadership transferred from %s to %s", types.ID(leaderID), types.ID(transferee)),
		},
	}

The first test sets the leader to e.g. 2010 and connects to 2005, for example, so it should fail with:

no leader endpoint given at ",

But the second sets it to point to leader, with the CmdArgs it works, but with Env, it just gets overwritten and the first test is executed with Envs from the second test. I am trying to find out how to prevent that overwrites.

Copy link
Author

Choose a reason for hiding this comment

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

Whenever tc.prefixArgs is called it sets an env.

During populating tests struct it overwrites the Env.

Copy link
Author

Choose a reason for hiding this comment

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

https://github.com/etcd-io/etcd/blob/master/tests/e2e/ctl_v3_test.go#L283

Copy link
Author

Choose a reason for hiding this comment

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

I think the best would be to add env into each test struct, what do you think @ptabor ?

prefixes []string
expect string

}{
{ // request to non-leader
cx.prefixArgs([]string{cx.epc.EndpointsV3()[(leadIdx+1)%3]}),
Expand Down