From ff33c565050a4f4475980640d27e2416f5aed183 Mon Sep 17 00:00:00 2001 From: Tom Wieczorek Date: Fri, 23 Feb 2024 16:47:17 +0100 Subject: [PATCH] Harden etcd subcommand usage and validation The etcd leave subcommand can be used to remove k0s nodes from the etcd cluster. If no IP address is specified, the default is to remove the current node. The IP of the node to be removed can be specified with the --peer-address flag. However, it's quite tempting to just pass the IP as an argument, like this `k0s etcd drop 192.168.0.0.3`. This extra argument will simply be ignored, and the leave subcommand will use its defaults. This can be _very_ confusing, and also quite dangerous, as it may remove _different_ nodes than intended. Improve the subcommand by making it fail if arguments are passed to it. Add validation to the --peer-address flag and improve the usage strings. While at it, make the etcd member-list subcommand reject any args as well. Signed-off-by: Tom Wieczorek (cherry picked from commit 477d354c1aa85b9712baee7149fc79bd0a79849f) (cherry picked from commit 6a19b5fed2ab196f8b68d4283cdc3d9da68612ae) --- cmd/etcd/leave.go | 46 +++++++++++++++++++++++++++++++++--------- cmd/etcd/leave_test.go | 46 ++++++++++++++++++++++++++++++++++++++++++ cmd/etcd/list.go | 3 ++- cmd/etcd/list_test.go | 32 +++++++++++++++++++++++++++++ 4 files changed, 117 insertions(+), 10 deletions(-) create mode 100644 cmd/etcd/leave_test.go create mode 100644 cmd/etcd/list_test.go diff --git a/cmd/etcd/leave.go b/cmd/etcd/leave.go index 9c9381b47bd6..45fea772232c 100644 --- a/cmd/etcd/leave.go +++ b/cmd/etcd/leave.go @@ -17,21 +17,27 @@ limitations under the License. package etcd import ( + "errors" "fmt" + "net" + "net/url" "github.com/k0sproject/k0s/pkg/config" "github.com/k0sproject/k0s/pkg/etcd" + "github.com/asaskevich/govalidator" "github.com/sirupsen/logrus" "github.com/spf13/cobra" + "github.com/spf13/pflag" ) func etcdLeaveCmd() *cobra.Command { - var etcdPeerAddress string + var peerAddressArg string cmd := &cobra.Command{ Use: "leave", - Short: "Sign off a given etc node from etcd cluster", + Short: "Leave the etcd cluster, or remove a specific peer", + Args: cobra.NoArgs, // accept peer address via flag, not via arg RunE: func(cmd *cobra.Command, args []string) error { opts, err := config.GetCmdOpts(cmd) if err != nil { @@ -42,14 +48,17 @@ func etcdLeaveCmd() *cobra.Command { return err } ctx := cmd.Context() - if etcdPeerAddress == "" { - etcdPeerAddress = nodeConfig.Spec.Storage.Etcd.PeerAddress - } - if etcdPeerAddress == "" { - return fmt.Errorf("can't leave etcd cluster: peer address is empty, check the config file or use cli argument") + + peerAddress := nodeConfig.Spec.Storage.Etcd.PeerAddress + if peerAddressArg == "" { + if peerAddress == "" { + return fmt.Errorf("can't leave etcd cluster: this node doesn't have an etcd peer address, check the k0s configuration or use --peer-address") + } + } else { + peerAddress = peerAddressArg } - peerURL := fmt.Sprintf("https://%s:2380", etcdPeerAddress) + peerURL := (&url.URL{Scheme: "https", Host: net.JoinHostPort(peerAddress, "2380")}).String() etcdClient, err := etcd.NewClient(opts.K0sVars.CertRootDir, opts.K0sVars.EtcdCertDir, nodeConfig.Spec.Storage.Etcd) if err != nil { return fmt.Errorf("can't connect to the etcd: %w", err) @@ -76,7 +85,26 @@ func etcdLeaveCmd() *cobra.Command { }, } - cmd.Flags().StringVar(&etcdPeerAddress, "peer-address", "", "etcd peer address") + cmd.Flags().AddFlag(&pflag.Flag{ + Name: "peer-address", + Usage: "etcd peer address to remove (default )", + Value: (*ipOrDNSName)(&peerAddressArg), + }) + cmd.PersistentFlags().AddFlagSet(config.GetPersistentFlagSet()) return cmd } + +type ipOrDNSName string + +func (i *ipOrDNSName) Type() string { return "ip-or-dns-name" } +func (i *ipOrDNSName) String() string { return string(*i) } + +func (i *ipOrDNSName) Set(value string) error { + if !govalidator.IsIP(value) && !govalidator.IsDNSName(value) { + return errors.New("neither an IP address nor a DNS name") + } + + *i = ipOrDNSName(value) + return nil +} diff --git a/cmd/etcd/leave_test.go b/cmd/etcd/leave_test.go new file mode 100644 index 000000000000..0aea6580814c --- /dev/null +++ b/cmd/etcd/leave_test.go @@ -0,0 +1,46 @@ +/* +Copyright 2024 k0s authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package etcd + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestEtcdLeaveCmd(t *testing.T) { + t.Run("rejects_IP_address_as_arg", func(t *testing.T) { + leaveCmd := etcdLeaveCmd() + leaveCmd.SetArgs([]string{"255.255.255.255"}) + err := leaveCmd.Execute() + assert.ErrorContains(t, err, `unknown command "255.255.255.255" for "leave"`) + }) + + t.Run("rejects_invalid_peer_addresses", func(t *testing.T) { + leaveCmd := etcdLeaveCmd() + leaveCmd.SetArgs([]string{"--peer-address=neither/ip/nor/name"}) + err := leaveCmd.Execute() + assert.ErrorContains(t, err, `invalid argument "neither/ip/nor/name" for "--peer-address" flag: neither an IP address nor a DNS name`) + }) + + t.Run("peer_address_usage_string", func(t *testing.T) { + leaveCmd := etcdLeaveCmd() + usageLines := strings.Split(leaveCmd.UsageString(), "\n") + assert.Contains(t, usageLines, " --peer-address ip-or-dns-name etcd peer address to remove (default )") + }) +} diff --git a/cmd/etcd/list.go b/cmd/etcd/list.go index 2c622719014d..8de8322f83bd 100644 --- a/cmd/etcd/list.go +++ b/cmd/etcd/list.go @@ -30,7 +30,8 @@ import ( func etcdListCmd() *cobra.Command { cmd := &cobra.Command{ Use: "member-list", - Short: "Returns etcd cluster members list", + Short: "List etcd cluster members (JSON encoded)", + Args: cobra.NoArgs, PreRun: func(cmd *cobra.Command, args []string) { // ensure logs don't mess up the output logrus.SetOutput(cmd.ErrOrStderr()) diff --git a/cmd/etcd/list_test.go b/cmd/etcd/list_test.go new file mode 100644 index 000000000000..f8d7d95a595d --- /dev/null +++ b/cmd/etcd/list_test.go @@ -0,0 +1,32 @@ +/* +Copyright 2024 k0s authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package etcd + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestEtcdListCmd(t *testing.T) { + t.Run("rejects_args", func(t *testing.T) { + leaveCmd := etcdListCmd() + leaveCmd.SetArgs([]string{"bogus"}) + err := leaveCmd.Execute() + assert.ErrorContains(t, err, `unknown command "bogus" for "member-list"`) + }) +}