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

zerodayz · 2021-03-09T22:48:29Z

Re-opening closed PR #11775 which was originaly authored by benmoss.

The mustClientForCmd function is responsible for parsing environment
variables and flags into configuration data. A change was made in #9382
to call Fatal if a flag is provided multiple times. This means that we
cannot call the mustClientForCmd function more than once,
since it will think that flags parsed the first time are now
being redefined and error out.

Some people have commented about this in #8380 but I don't think
there's an open issue for it.

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

zerodayz · 2021-03-09T22:49:51Z

From @ptabor in #11775 working on the test

Could you, please, add a regression test.
Probably this file is a convenient place and a template:

./tests/e2e/ctl_v3_move_leader_test.go

zerodayz · 2021-03-10T05:08:08Z

@ptabor I continued from where Ben Moss finished #11775 . Please can you review?

I noticed that etcd would fail to move-leader using the env variables set. If using CmdArgs it works.

This patch fixed that and also added a test which would fail without this patch, what it does it's setting up the Env Variables and executes move-leader.

I can't loop thru many tests there, as the tc.prefixes are pre-configuring the variables, meaning the last test values would always overwrite the previous one.

I couldn't figure out if I can pass the cx.envMap as part of the test, however this test showed the fix actually works and should prevent from regression in the future.

Thanks!

tests/e2e/ctl_v3_move_leader_test.go

ptabor · 2021-03-10T08:20:51Z

tests/e2e/ctl_v3_move_leader_test.go

+		envMap:      map[string]struct{}{},
+	}
+
+	tests := []struct {


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

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.

Whenever tc.prefixArgs is called it sets an env.

During populating tests struct it overwrites the Env.

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

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

ptabor · 2021-03-11T08:42:11Z

Oh... that's weird. The tests should not modify the testing environment of test-process at all:

etcd/tests/e2e/ctl_v3_test.go

Line 283 in 3ead91c

os.Setenv(ek, v)

instead it should pass its own environment map straight to:

etcd/tests/e2e/etcd_spawn_nocov.go

Line 31 in 3ead91c

return expect.NewExpectWithEnv(ctlBinPath, args[1:], env)

side by side to args []string

Thank you for catching it.

zerodayz · 2021-03-11T09:30:14Z

Oh... that's weird. The tests should not modify the testing environment of test-process at all:

etcd/tests/e2e/ctl_v3_test.go

Line 283 in 3ead91c

os.Setenv(ek, v)

instead it should pass its own environment map straight to:

etcd/tests/e2e/etcd_spawn_nocov.go

Line 31 in 3ead91c

return expect.NewExpectWithEnv(ctlBinPath, args[1:], env)

side by side to args []string

Thank you for catching it.

The code you point to is right but note that prefixArgs is called in

etcd/tests/e2e/ctl_v3_move_leader_test.go

Line 104 in 3ead91c

cx.prefixArgs([]string{cx.epc.EndpointsV3()[(leadIdx+1)%3]}),

and

etcd/tests/e2e/ctl_v3_move_leader_test.go

Line 108 in 3ead91c

cx.prefixArgs([]string{cx.epc.EndpointsV3()[leadIdx]}),

the second one overwrites the env in

etcd/tests/e2e/ctl_v3_test.go

Line 283 in 3ead91c

os.Setenv(ek, v)

And then it goes thru the loop and spawn the process for first test in

etcd/tests/e2e/ctl_v3_move_leader_test.go

Line 114 in 3ead91c

if err := spawnWithExpect(cmdArgs, tc.expect); err != nil {

but it already has overwritten env variables.

zerodayz · 2021-03-11T09:31:34Z

I have done some modifications, it still doesn't work due to issue with env, but let me push it so you can take a look.

zerodayz · 2021-04-05T23:08:38Z

@ptabor Did you have any time to look at those failing tests with Env Variables? I think the way the tests are written simply doesn't make it possible to use SetEnv. It will always get overwritten.

zhangguanzhang · 2021-06-08T11:45:42Z

any update?

zhangguanzhang · 2021-06-11T09:36:39Z

@gyuho any uddate?

lilic · 2021-06-11T12:06:00Z

@zerodayz do you mind rebasing so we can retrigger the tests you mentioned failed, thank you!

zerodayz · 2021-06-11T22:52:26Z

Hi yeah sure I will do it today and see if the tests are fixed.

zerodayz · 2021-06-14T10:08:32Z

Hi @lilic I rebased on main etcd-io. Also tried the Tests again with:

	testCtlV3MoveLeader(t, withCfg(*newConfigNoTLS()), withFlagByEnv())

And

		envMap:      map[string]struct{}{},

It's just that the tests simply overwrite each other when execute during initialization...

	tests := []struct {
		prefixes []string
		expect   string

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

Feels like it does the prefixArgs first on both tests to populate the OS Env Variables. Then executes the first test, and then second.

So the first test is executed with Env variables from the second test..

expected: no leader endpoint given at
got: Leadership transferred from c61f33e98087dec2 to d70245c72180528a

Do you see what I mean ? ^

lilic · 2021-06-14T10:10:23Z

Did the test fail, as you force pushed I could not see the previous failures, do you mind including them next time here, thanks!

zerodayz · 2021-06-14T10:11:34Z

Including tests.

Re-opening closed PR etcd-io#11775 which was originaly authored by benmoss. The mustClientForCmd function is responsible for parsing environment variables and flags into configuration data. A change was made in etcd-io#9382 to call Fatal if a flag is provided multiple times. This means that we cannot call the mustClientForCmd function more than once, since it will think that flags parsed the first time are now being redefined and error out. Some people have commented about this in etcd-io#8380 but I don't think there's an open issue for it.

zerodayz · 2021-06-14T10:13:05Z

@lilic Included updated tests, please try to debug in your Env. And you will see what I mean.

zerodayz · 2021-06-14T10:21:03Z

This is only failing of course if the tests with Env contains two tests, if I add test one by one separately, this works. So I just think the way how the tests are written, the OS Env variables are being overwritten before the tests are actually executed. So last lest variables are used for the first test.

lilic

I left a suggestion, there is also an issue with gofmt see the failing test for that.

lilic · 2021-06-14T11:40:08Z

tests/e2e/ctl_v3_move_leader_test.go

@@ -28,25 +28,34 @@ import (
 )

 func TestCtlV3MoveLeaderSecure(t *testing.T) {
-	testCtlV3MoveLeader(t, *newConfigTLS())
+	testCtlV3MoveLeader(t, withCfg(*newConfigTLS()))


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?

lilic · 2021-06-14T11:45:45Z

This is only failing of course if the tests with Env contains two tests, if I add test one by one separately, this works. So I just think the way how the tests are written, the OS Env variables are being overwritten before the tests are actually executed. So last lest variables are used for the first test.

By two tests you mean where you call testCtlV3MoveLeader twice? I left a suggestion asking why this is needed even for the existing tests?

zhangguanzhang · 2021-06-18T03:40:24Z

any update?

zhangguanzhang · 2021-06-24T03:18:56Z

any update?

lqhandsome · 2021-06-24T03:36:38Z

any update?

hong108 · 2021-06-24T03:38:35Z

any update?

zhangguanzhang · 2021-07-08T10:55:39Z

any update? 2021/07-08

lilic · 2021-07-13T11:52:52Z

@zerodayz hey 👋 would you prefer someone else to take over the PR with your commits maybe to finish this up?

zhangguanzhang · 2021-08-05T01:42:10Z

any update? 2021/08-05

lilic · 2021-08-05T12:01:25Z

Since its been some time, i would suggest someone else take this over with @zerodayz commits of course. @zhangguanzhang since you are interested in this, do you want to take the commits and open a new PR and fix the tests?

stale · 2021-11-04T05:33:38Z

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.

Re-opening closed PR etcd-io#11775 which was originaly authored by benmoss. Then again opened PR etcd-io#12757 which was authored by zerodayz. The mustClientForCmd function is responsible for parsing environment variables and flags into configuration data. A change was made in etcd-io#9382 to call Fatal if a flag is provided multiple times. This means that we cannot call the mustClientForCmd function more than once, since it will think that flags parsed the first time are now being redefined and error out. Some people have commented about this in etcd-io#8380 but I don't think there's an open issue for it. Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>

…ndpoints Re-opening closed PR etcd-io#11775 which was originaly authored by benmoss. Then again opened PR etcd-io#12757 which was authored by zerodayz. The mustClientForCmd function is responsible for parsing environment variables and flags into configuration data. A change was made in etcd-io#9382 to call Fatal if a flag is provided multiple times. This means that we cannot call the mustClientForCmd function more than once, since it will think that flags parsed the first time are now being redefined and error out. Some people have commented about this in etcd-io#8380 but I don't think there's an open issue for it. Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>

zerodayz force-pushed the fix-move-leader branch from e50f07e to 6bbdea7 Compare March 10, 2021 05:04

zerodayz force-pushed the fix-move-leader branch from 6bbdea7 to 510f9f8 Compare March 10, 2021 06:06

ptabor suggested changes Mar 10, 2021

View reviewed changes

zerodayz force-pushed the fix-move-leader branch from 510f9f8 to 6e37a72 Compare March 11, 2021 09:32

zerodayz force-pushed the fix-move-leader branch 2 times, most recently from af2ed4e to 10873b1 Compare June 14, 2021 09:48

zerodayz force-pushed the fix-move-leader branch from 10873b1 to 04f7fd9 Compare June 14, 2021 10:12

lilic reviewed Jun 14, 2021

View reviewed changes

ardaguclu mentioned this pull request Sep 13, 2021

Decouple prefixArgs from os.Env dependency #13343

Merged

stale bot added the stale label Nov 4, 2021

stale bot closed this Nov 25, 2021

tjungblu mentioned this pull request Aug 3, 2022

etcdctl: allow move-leader to connect to multiple endpoints #14307

Closed

tjungblu mentioned this pull request Sep 7, 2022

[release-3.5]etcdctl: allow move-leader to connect to multiple endpoints #14434

Merged

tjungblu mentioned this pull request Sep 7, 2022

UPSTREAM <carry>: etcdctl: allow move-leader to connect to multiple e… openshift/etcd#146

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zerodayz commented Mar 9, 2021

zerodayz commented Mar 9, 2021

zerodayz commented Mar 10, 2021

ptabor Mar 10, 2021

zerodayz Mar 11, 2021

zerodayz Mar 11, 2021

zerodayz Mar 11, 2021

zerodayz Mar 11, 2021

ptabor commented Mar 11, 2021 •

edited

Loading

zerodayz commented Mar 11, 2021

zerodayz commented Mar 11, 2021

zerodayz commented Apr 5, 2021

zhangguanzhang commented Jun 8, 2021

zhangguanzhang commented Jun 11, 2021

lilic commented Jun 11, 2021

zerodayz commented Jun 11, 2021

zerodayz commented Jun 14, 2021 •

edited

Loading

lilic commented Jun 14, 2021

zerodayz commented Jun 14, 2021

zerodayz commented Jun 14, 2021

zerodayz commented Jun 14, 2021

lilic left a comment

lilic Jun 14, 2021

lilic commented Jun 14, 2021 •

edited

Loading

zhangguanzhang commented Jun 18, 2021

zhangguanzhang commented Jun 24, 2021

lqhandsome commented Jun 24, 2021

hong108 commented Jun 24, 2021

zhangguanzhang commented Jul 8, 2021

lilic commented Jul 13, 2021

zhangguanzhang commented Aug 5, 2021

lilic commented Aug 5, 2021 •

edited

Loading

stale bot commented Nov 4, 2021

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

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

Conversation

zerodayz commented Mar 9, 2021

zerodayz commented Mar 9, 2021

zerodayz commented Mar 10, 2021

ptabor Mar 10, 2021

Choose a reason for hiding this comment

zerodayz Mar 11, 2021

Choose a reason for hiding this comment

zerodayz Mar 11, 2021

Choose a reason for hiding this comment

zerodayz Mar 11, 2021

Choose a reason for hiding this comment

zerodayz Mar 11, 2021

Choose a reason for hiding this comment

ptabor commented Mar 11, 2021 • edited Loading

zerodayz commented Mar 11, 2021

zerodayz commented Mar 11, 2021

zerodayz commented Apr 5, 2021

zhangguanzhang commented Jun 8, 2021

zhangguanzhang commented Jun 11, 2021

lilic commented Jun 11, 2021

zerodayz commented Jun 11, 2021

zerodayz commented Jun 14, 2021 • edited Loading

lilic commented Jun 14, 2021

zerodayz commented Jun 14, 2021

zerodayz commented Jun 14, 2021

zerodayz commented Jun 14, 2021

lilic left a comment

Choose a reason for hiding this comment

lilic Jun 14, 2021

Choose a reason for hiding this comment

lilic commented Jun 14, 2021 • edited Loading

zhangguanzhang commented Jun 18, 2021

zhangguanzhang commented Jun 24, 2021

lqhandsome commented Jun 24, 2021

hong108 commented Jun 24, 2021

zhangguanzhang commented Jul 8, 2021

lilic commented Jul 13, 2021

zhangguanzhang commented Aug 5, 2021

lilic commented Aug 5, 2021 • edited Loading

stale bot commented Nov 4, 2021

ptabor commented Mar 11, 2021 •

edited

Loading

zerodayz commented Jun 14, 2021 •

edited

Loading

lilic commented Jun 14, 2021 •

edited

Loading

lilic commented Aug 5, 2021 •

edited

Loading