Merge pull request #4118 from twz123/harden-etcd-cli

Harden etcd subcommand usage and validation

Author: twz123

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)
@@ -64,19 +73,38 @@ func etcdLeaveCmd() *cobra.Command {
 			if err := etcdClient.DeleteMember(ctx, peerID); err != nil {
 				logrus.
 					WithField("peerURL", peerURL).
-					WithField("peerID", peerID).
+					WithField("peerID", fmt.Sprintf("%x", peerID)).
 					Errorf("Failed to delete node from cluster")
 				return err
 			}
 
 			logrus.
-				WithField("peerID", peerID).
+				WithField("peerID", fmt.Sprintf("%x", peerID)).
 				Info("Successfully deleted")
 			return nil
 		},
 	}
 
-	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"`)
+	})
+}