etcdctl: allow move-leader to connect to multiple endpoints #14307
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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>
lavacat
reviewed
Aug 3, 2022
| @@ -109,6 +109,10 @@ func testCtlV3MoveLeader(t *testing.T, cfg e2e.EtcdProcessClusterConfig) { | |||
| []string{cx.epc.EndpointsV3()[leadIdx]}, | |||
| fmt.Sprintf("Leadership transferred from %s to %s", types.ID(leaderID), types.ID(transferee)), | |||
| }, | |||
| { // request to all endpoints | |||
There was a problem hiding this comment.
nit: I've added this test case to unmodified main and both TestCtlV3MoveLeaderSecure and TestCtlV3MoveLeaderInsecure passed for me. I think it should fail without the actual code change.
There was a problem hiding this comment.
You need to set the environment variable ETCDCTL_ENDPOITNS to trigger the issue. Please update the test case accordingly.
There was a problem hiding this comment.
$ export ETCDCTL_ENDPOINTS=http://www.google.com:2379
$ echo $ETCDCTL_ENDPOINTS
http://www.google.com:2379
$ etcdctl move-leader --endpoints=http://127.0.0.1:2379,http://127.0.0.1:22379,http://127.0.0.1:32379 8211f1d0f64f3269
{"level":"fatal","ts":"2022-08-09T02:59:50.324+0800","caller":"flags/flag.go:85","msg":"conflicting environment variable is shadowed by corresponding command-line flag (either unset environment variable or disable flag))","environment-variable":"ETCDCTL_ENDPOINTS","stacktrace":"go.etcd.io/etcd/pkg/v3/flags.verifyEnv\n\t/Users/wachao/go/src/github.com/ahrtr/etcd/pkg/flags/flag.go:85\ngo.etcd.io/etcd/pkg/v3/flags.SetPflagsFromEnv\n\t/Users/wachao/go/src/github.com/ahrtr/etcd/pkg/flags/flag.go:63\ngo.etcd.io/etcd/etcdctl/v3/ctlv3/command.clientConfigFromCmd\n\t/Users/wachao/go/src/github.com/ahrtr/etcd/etcdctl/ctlv3/command/global.go:103\ngo.etcd.io/etcd/etcdctl/v3/ctlv3/command.mustClientFromCmd\n\t/Users/wachao/go/src/github.com/ahrtr/etcd/etcdctl/ctlv3/command/global.go:150\ngo.etcd.io/etcd/etcdctl/v3/ctlv3/command.transferLeadershipCommandFunc\n\t/Users/wachao/go/src/github.com/ahrtr/etcd/etcdctl/ctlv3/command/move_leader_command.go:46\ngithub.com/spf13/cobra.(*Command).execute\n\t/Users/wachao/go/gopath/pkg/mod/github.com/spf13/cobra@v1.4.0/command.go:860\ngithub.com/spf13/cobra.(*Command).ExecuteC\n\t/Users/wachao/go/gopath/pkg/mod/github.com/spf13/cobra@v1.4.0/command.go:974\ngithub.com/spf13/cobra.(*Command).Execute\n\t/Users/wachao/go/gopath/pkg/mod/github.com/spf13/cobra@v1.4.0/command.go:902\ngo.etcd.io/etcd/etcdctl/v3/ctlv3.Start\n\t/Users/wachao/go/src/github.com/ahrtr/etcd/etcdctl/ctlv3/ctl.go:113\ngo.etcd.io/etcd/etcdctl/v3/ctlv3.MustStart\n\t/Users/wachao/go/src/github.com/ahrtr/etcd/etcdctl/ctlv3/ctl.go:117\nmain.main\n\t/Users/wachao/go/src/github.com/ahrtr/etcd/etcdctl/main.go:32\nruntime.main\n\t/Users/wachao/software/go/src/runtime/proc.go:255"}
There was a problem hiding this comment.
Thanks @ahrtr I have refactored the test so it fails on that condition again. The issue seems deeper in pflags however, will come up with another fix here.
There are many other conflicting variables here: #8380 (comment)
Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
Codecov Report
@@ Coverage Diff @@
## main #14307 +/- ##
==========================================
- Coverage 75.67% 75.34% -0.34%
==========================================
Files 456 456
Lines 37039 37071 +32
==========================================
- Hits 28030 27931 -99
- Misses 7272 7378 +106
- Partials 1737 1762 +25
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 |
|
just to bring this back to a real case in OpenShift with etcd 3.5.3. Our operator does set those env variables properly for the customer to "just" use, however on etcdctl move-leader we find this behaviour: which should not happen, ideally. There was no actual flag supplied here, thus there was no env variable to be shadowed in the first place. The stacktrace pretty printed is: I still assume this is fixed in master, yet we would need this in 3.5... |
|
given this is only reproducible for 3.5, here's the real patch #14434. closing. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Re-opening closed PR #11775 which was originaly authored by benmoss.
Then again opened PR #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 #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.
Signed-off-by: Thomas Jungblut tjungblu@redhat.com
tracked under our BZ1918413