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 <[email protected]>
(cherry picked from commit 477d354)
(cherry picked from commit 6a19b5f)

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 <this node's peer address>)",
+		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
+}

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 <this node's peer address>)")
+	})
+}

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())

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"`)
+	})
+}