refactor: replace generic ShellEscape with shell-aware functions (#690)

* refactor: replace generic ShellEscape with shell-aware functions

* adapt tests

* make generate

Author: petersutter

docs/help/gardenctl_ssh.md

@@ -4,7 +4,7 @@ Establish an SSH connection to a node of a Shoot cluster
 
 ### Synopsis
 
-Establish an SSH connection to a node of a Shoot cluster by specifying its name. 
+Establish an SSH connection to a node of a Shoot cluster by specifying its name.
 
 A bastion is created to access the node and is automatically cleaned up afterwards.
 
@@ -59,6 +59,7 @@ gardenctl ssh --keep-bastion --bastion-name cli-xxxxxxxx --public-key-file /path
       --project string                            target the given project
       --public-key-file string                    Path to the file that contains a public SSH key. If not given, a temporary keypair will be generated.
       --seed string                               target the given seed cluster
+      --shell string                              Shell to use for escaping arguments when printing out the SSH command. If not provided, it defaults to the GCTL_SHELL environment variable or bash.
       --shoot string                              target the given shoot cluster
       --skip-availability-check                   Skip checking for SSH bastion host availability.
       --user string                               user is the name of the Shoot cluster node ssh login username. (default "gardener")

internal/util/strings.go

@@ -7,7 +7,6 @@ SPDX-License-Identifier: Apache-2.0
 package util
 
 import (
-	"fmt"
 	"strings"
 	"unicode"
 )
@@ -28,23 +27,6 @@ func FilterStringsByPrefix(prefix string, values []string) []string {
 	return filtered
 }
 
-// ShellEscape returns a shell-escaped version of the given string. It also removes non-printable characters.
-func ShellEscape(values ...interface{}) string {
-	out := make([]string, 0, len(values))
-
-	for _, v := range values {
-		if v != nil {
-			s := fmt.Sprintf("%v", v)
-			s = StripUnsafe(s)
-			s = strings.ReplaceAll(s, "'", "'\"'\"'")
-			s = "'" + s + "'"
-			out = append(out, s)
-		}
-	}
-
-	return strings.Join(out, " ")
-}
-
 // StripUnsafe remove non-printable characters from the string.
 func StripUnsafe(s string) string {
 	return strings.Map(func(r rune) rune {

internal/util/strings_test.go

@@ -20,16 +20,4 @@ var _ = Describe("String Utilities", func() {
 			Expect(util.FilterStringsByPrefix("x", []string{"xa", "yb", "xc", "zx"})).To(Equal([]string{"xa", "xc"}))
 		})
 	})
-
-	Describe("escaping shell strings", func() {
-		It("should escape a shell string", func() {
-			Expect(util.ShellEscape("$TOKEN")).To(Equal("'$TOKEN'"))
-			Expect(util.ShellEscape("'")).To(Equal("''\"'\"''"))
-			Expect(util.ShellEscape("\u0081")).To(Equal("''"))
-		})
-
-		It("should escape multiple shell strings", func() {
-			Expect(util.ShellEscape("a", "b")).To(Equal("'a' 'b'"))
-		})
-	})
 })

pkg/cmd/kubectlenv/options.go

@@ -47,7 +47,13 @@ func (o *options) Complete(f util.Factory, cmd *cobra.Command, _ []string) error
 	o.Shell = cmd.Name()
 	o.CmdPath = cmd.Parent().CommandPath()
 	o.GardenDir = f.GardenHomeDir()
-	o.Template = env.NewTemplate("helpers")
+
+	tmpl, err := env.NewTemplate(o.Shell, "helpers")
+	if err != nil {
+		return err
+	}
+
+	o.Template = tmpl
 
 	filename := filepath.Join(o.GardenDir, "templates", "kubernetes.tmpl")
 	if err := o.Template.ParseFiles(filename); err != nil {

pkg/cmd/kubectlenv/options_test.go

@@ -51,8 +51,10 @@ var _ = Describe("Env Commands - Options", func() {
 			manager = targetmocks.NewMockManager(ctrl)
 			options = kubectlenv.NewOptions()
 			cmdPath = "gardenctl provider-env"
-			baseTemplate = env.NewTemplate("helpers")
-			shell = "default"
+			shell = "bash"
+			var err error
+			baseTemplate, err = env.NewTemplate(shell, "helpers")
+			Expect(err).NotTo(HaveOccurred())
 			cfg = &config.Config{
 				LinkKubeconfig: ptr.To(false),
 				Gardens:        []config.Garden{{Name: "test"}},
@@ -78,7 +80,7 @@ var _ = Describe("Env Commands - Options", func() {
 			BeforeEach(func() {
 				root = &cobra.Command{Use: "root"}
 				parent = &cobra.Command{Use: "parent", Aliases: []string{"alias"}}
-				child = &cobra.Command{Use: "child"}
+				child = &cobra.Command{Use: "bash"}
 				parent.AddCommand(child)
 				root.AddCommand(parent)
 				factory.EXPECT().GardenHomeDir().Return(gardenHomeDir)
@@ -230,7 +232,9 @@ var _ = Describe("Env Commands - Options", func() {
 			JustBeforeEach(func() {
 				meta = options.GenerateMetadata()
 				targetFlags = kubectlenv.GetTargetFlags(t)
-				Expect(env.NewTemplate("helpers").ExecuteTemplate(options.IOStreams.Out, "usage-hint", meta)).To(Succeed())
+				tmpl, err := env.NewTemplate(shell, "helpers")
+				Expect(err).NotTo(HaveOccurred())
+				Expect(tmpl.ExecuteTemplate(options.IOStreams.Out, "usage-hint", meta)).To(Succeed())
 			})
 
 			Context("when configuring the shell", func() {

pkg/cmd/providerenv/options.go

@@ -88,7 +88,15 @@ func (o *options) Complete(f util.Factory, cmd *cobra.Command, _ []string) error
 
 	o.CmdPath = cmd.Parent().CommandPath()
 	o.GardenDir = f.GardenHomeDir()
-	o.Template = env.NewTemplate("helpers")
+
+	if o.Output == "" {
+		tmpl, err := env.NewTemplate(o.Shell, "helpers")
+		if err != nil {
+			return err
+		}
+
+		o.Template = tmpl
+	}
 
 	sessionID, err := f.GetSessionID()
 	if err != nil {
@@ -203,7 +211,13 @@ func (o *options) Validate() error {
 		return pflag.ErrHelp
 	}
 
-	// Usually, we would check and return an error if both shell and output are set (not empty). However, this is not required because the output flag is not set for the shell subcommands.
+	if o.Shell != "" && o.Output != "" {
+		return fmt.Errorf("internal error: both shell and output are set")
+	}
+
+	if o.Output != "" && o.Template != nil {
+		return fmt.Errorf("internal error: template is set when output is specified")
+	}
 
 	if o.Shell != "" {
 		s := env.Shell(o.Shell)

pkg/cmd/providerenv/options_test.go

@@ -65,8 +65,11 @@ var _ = Describe("Env Commands - Options", func() {
 			manager = targetmocks.NewMockManager(ctrl)
 			options = providerenv.NewOptions()
 			cmdPath = "gardenctl provider-env"
-			baseTemplate = env.NewTemplate("helpers")
-			shell = "default"
+			// Use bash as default shell for tests; helpers template itself is shell-agnostic
+			shell = "bash"
+			var err error
+			baseTemplate, err = env.NewTemplate(shell, "helpers")
+			Expect(err).NotTo(HaveOccurred())
 			output = ""
 			providerType = "aws"
 			cfg = &config.Config{
@@ -96,7 +99,7 @@ var _ = Describe("Env Commands - Options", func() {
 			BeforeEach(func() {
 				root = &cobra.Command{Use: "root"}
 				parent = &cobra.Command{Use: "parent", Aliases: []string{"alias"}}
-				child = &cobra.Command{Use: "child"}
+				child = &cobra.Command{Use: "bash"}
 				parent.AddCommand(child)
 				root.AddCommand(parent)
 				factory.EXPECT().GardenHomeDir().Return(gardenHomeDir)
@@ -159,6 +162,7 @@ var _ = Describe("Env Commands - Options", func() {
 			Context("when output is set", func() {
 				BeforeEach(func() {
 					shell = ""
+					baseTemplate = nil
 				})
 
 				It("should successfully validate the options", func() {
@@ -172,6 +176,12 @@ var _ = Describe("Env Commands - Options", func() {
 				})
 			})
 
+			It("should detect invalid state when both shell and output are set", func() {
+				options.Shell = "bash"
+				options.Output = "json"
+				Expect(options.Validate()).To(MatchError("internal error: both shell and output are set"))
+			})
+
 			It("should successfully validate the options", func() {
 				options.Shell = "bash"
 				Expect(options.Validate()).To(Succeed())
@@ -243,8 +253,10 @@ var _ = Describe("Env Commands - Options", func() {
 				factory.EXPECT().Context().Return(ctx).AnyTimes()
 				// Create a proper command hierarchy for Complete() to work
 				parentCmd := &cobra.Command{Use: "gardenctl"}
-				mockCmd = &cobra.Command{Use: "provider-env"}
-				parentCmd.AddCommand(mockCmd)
+				providerEnvCmd := &cobra.Command{Use: "provider-env"}
+				mockCmd = &cobra.Command{Use: "bash"}
+				providerEnvCmd.AddCommand(mockCmd)
+				parentCmd.AddCommand(providerEnvCmd)
 			})
 
 			JustBeforeEach(func() {
@@ -526,8 +538,10 @@ var _ = Describe("Env Commands - Options", func() {
 				options.Target = target.NewTarget("test", "project", "", shootName)
 				// Create a proper command hierarchy for Complete() to work
 				parentCmd := &cobra.Command{Use: "gardenctl"}
-				mockCmd = &cobra.Command{Use: "provider-env"}
-				parentCmd.AddCommand(mockCmd)
+				providerEnvCmd := &cobra.Command{Use: "provider-env"}
+				mockCmd = &cobra.Command{Use: "bash"}
+				providerEnvCmd.AddCommand(mockCmd)
+				parentCmd.AddCommand(providerEnvCmd)
 			})
 
 			JustBeforeEach(func() {
@@ -666,6 +680,7 @@ var _ = Describe("Env Commands - Options", func() {
 					manager.EXPECT().SessionDir().Return(sessionDir)
 					manager.EXPECT().Configuration().Return(cfg)
 					Expect(options.Complete(factory, mockCmd, nil)).To(Succeed())
+					options.Shell = shell // monkey patch
 				})
 
 				It("should fail to render the template with JSON parse error", func() {
@@ -1044,7 +1059,9 @@ var _ = Describe("Env Commands - Options", func() {
 				cli = providerenv.GetProviderCLI(providerType)
 				meta = options.GenerateMetadata(cli, "Secret")
 				targetFlags = providerenv.GetTargetFlags(t)
-				Expect(env.NewTemplate("helpers").ExecuteTemplate(options.IOStreams.Out, "usage-hint", meta)).To(Succeed())
+				tmpl, err := env.NewTemplate(shell, "helpers")
+				Expect(err).NotTo(HaveOccurred())
+				Expect(tmpl.ExecuteTemplate(options.IOStreams.Out, "usage-hint", meta)).To(Succeed())
 			})
 
 			Context("when configuring the shell", func() {

pkg/cmd/rc/options.go

@@ -40,7 +40,13 @@ type options struct {
 func (o *options) Complete(_ util.Factory, cmd *cobra.Command, _ []string) error {
 	o.Shell = cmd.Name()
 	o.CmdPath = cmd.Parent().CommandPath()
-	o.Template = env.NewTemplate("rc")
+
+	tmpl, err := env.NewTemplate(o.Shell, "rc")
+	if err != nil {
+		return err
+	}
+
+	o.Template = tmpl
 
 	return nil
 }
@@ -60,6 +66,10 @@ func (o *options) Validate() error {
 		return fmt.Errorf("prefix must start with an alphabetic character may be followed by alphanumeric characters, underscore or dash")
 	}
 
+	if len(o.Prefix) > 32 {
+		return fmt.Errorf("prefix is too long, maximum length is 32 characters")
+	}
+
 	return nil
 }
 

pkg/cmd/rc/rc_test.go

@@ -62,7 +62,8 @@ var _ = Describe("Env Commands", func() {
 		It("should execute the bash subcommand", func() {
 			cmd.SetArgs([]string{"bash"})
 			Expect(cmd.Execute()).To(Succeed())
-			Expect(out.String()).To(Equal(`if [ -z "$GCTL_SESSION_ID" ] && [ -z "$TERM_SESSION_ID" ]; then
+			Expect(out.String()).To(Equal(`export GCTL_SHELL=bash
+if [ -z "$GCTL_SESSION_ID" ] && [ -z "$TERM_SESSION_ID" ]; then
   export GCTL_SESSION_ID=$(uuidgen)
 fi
 alias g=gardenctl
@@ -81,7 +82,8 @@ gk
 		It("should execute the zsh subcommand", func() {
 			cmd.SetArgs([]string{"zsh"})
 			Expect(cmd.Execute()).To(Succeed())
-			Expect(out.String()).To(Equal(`if [ -z "$GCTL_SESSION_ID" ] && [ -z "$TERM_SESSION_ID" ]; then
+			Expect(out.String()).To(Equal(`export GCTL_SHELL=zsh
+if [ -z "$GCTL_SESSION_ID" ] && [ -z "$TERM_SESSION_ID" ]; then
   export GCTL_SESSION_ID=$(uuidgen)
 fi
 alias g=gardenctl
@@ -112,7 +114,8 @@ gk
 		It("should execute the fish subcommand", func() {
 			cmd.SetArgs([]string{"fish"})
 			Expect(cmd.Execute()).To(Succeed())
-			Expect(out.String()).To(Equal(`if [ -z "$GCTL_SESSION_ID" ] && [ -z "$TERM_SESSION_ID" ];
+			Expect(out.String()).To(Equal(`set -gx GCTL_SHELL fish
+if [ -z "$GCTL_SESSION_ID" ] && [ -z "$TERM_SESSION_ID" ];
   set -gx GCTL_SESSION_ID (uuidgen)
 end
 alias g=gardenctl
@@ -131,7 +134,8 @@ gk
 		It("should execute the powershell subcommand", func() {
 			cmd.SetArgs([]string{"powershell"})
 			Expect(cmd.Execute()).To(Succeed())
-			Expect(out.String()).To(Equal(`if ( !(Test-Path Env:GCTL_SESSION_ID) -and !(Test-Path Env:TERM_SESSION_ID) ) {
+			Expect(out.String()).To(Equal(`$Env:GCTL_SHELL = "powershell"
+if ( !(Test-Path Env:GCTL_SESSION_ID) -and !(Test-Path Env:TERM_SESSION_ID) ) {
   $Env:GCTL_SESSION_ID = [guid]::NewGuid().ToString()
 }
 Set-Alias -Name g -Value (get-command gardenctl).Path -Option AllScope -Force
@@ -229,5 +233,39 @@ gk
 			options.Prefix = "!"
 			Expect(options.Validate()).To(MatchError("prefix must start with an alphabetic character may be followed by alphanumeric characters, underscore or dash"))
 		})
+		It("should return an error when the prefix is too long", func() {
+			options.Prefix = "verylongprefixthatiswaymorethan32characters"
+			Expect(options.Validate()).To(MatchError("prefix is too long, maximum length is 32 characters"))
+		})
+
+		It("should allow valid prefix with alphanumeric characters", func() {
+			options.Prefix = "gctl123"
+			Expect(options.Validate()).To(Succeed())
+		})
+
+		It("should allow valid prefix with underscore", func() {
+			options.Prefix = "g_ctl"
+			Expect(options.Validate()).To(Succeed())
+		})
+
+		It("should allow valid prefix with dash", func() {
+			options.Prefix = "g-ctl"
+			Expect(options.Validate()).To(Succeed())
+		})
+
+		It("should return an error when prefix starts with a number", func() {
+			options.Prefix = "1gctl"
+			Expect(options.Validate()).To(MatchError("prefix must start with an alphabetic character may be followed by alphanumeric characters, underscore or dash"))
+		})
+
+		It("should return an error when prefix starts with underscore", func() {
+			options.Prefix = "_gctl"
+			Expect(options.Validate()).To(MatchError("prefix must start with an alphabetic character may be followed by alphanumeric characters, underscore or dash"))
+		})
+
+		It("should return an error when prefix starts with dash", func() {
+			options.Prefix = "-gctl"
+			Expect(options.Validate()).To(MatchError("prefix must start with an alphabetic character may be followed by alphanumeric characters, underscore or dash"))
+		})
 	})
 })