Skip to content

Commit ec65767

Browse files
fix(cli): fix nil map panic and SSH command quoting
C1: Add nil check for env.Labels before writing provisioned label in update --reprovision. Previously panicked when no --label flags were provided and the cached environment had no labels. C2: Replace incorrect containsSpace+fmt.Sprintf("%q") quoting with simple strings.Join. SSH session.Run always passes through the remote shell, so Go-style quoting was wrong. Document this behavior. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
1 parent 57d6d80 commit ec65767

File tree

4 files changed

+23
-44
lines changed

4 files changed

+23
-44
lines changed

cmd/cli/ssh/ssh.go

Lines changed: 5 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"fmt"
2121
"os"
2222
"os/exec"
23+
"strings"
2324
"time"
2425

2526
"golang.org/x/crypto/ssh"
@@ -229,27 +230,17 @@ func (m command) runCommand(client *ssh.Client, cmd []string) error {
229230
session.Stdout = os.Stdout
230231
session.Stderr = os.Stderr
231232

232-
// Build command string
233-
cmdStr := ""
234-
for i, c := range cmd {
235-
if i > 0 {
236-
cmdStr += " "
237-
}
238-
// Quote arguments with spaces
239-
if containsSpace(c) {
240-
cmdStr += fmt.Sprintf("%q", c)
241-
} else {
242-
cmdStr += c
243-
}
244-
}
233+
// SSH remote execution always passes the command through the remote shell,
234+
// so we join arguments with spaces. Users who need literal quoting should
235+
// wrap arguments in shell quotes themselves (e.g., -- 'echo "hello"').
236+
cmdStr := strings.Join(cmd, " ")
245237

246238
return session.Run(cmdStr)
247239
}
248240

249241
// runInteractiveSystemSSH uses the system's ssh command for interactive sessions
250242
// This provides better terminal support (colors, window resize, etc.)
251243
func (m command) runInteractiveSystemSSH(keyPath, userName, hostUrl string) error {
252-
// Build SSH command arguments
253244
args := []string{
254245
"-i", keyPath,
255246
"-o", "StrictHostKeyChecking=no",
@@ -258,20 +249,10 @@ func (m command) runInteractiveSystemSSH(keyPath, userName, hostUrl string) erro
258249
fmt.Sprintf("%s@%s", userName, hostUrl),
259250
}
260251

261-
// Execute system SSH
262252
cmd := exec.Command("ssh", args...)
263253
cmd.Stdin = os.Stdin
264254
cmd.Stdout = os.Stdout
265255
cmd.Stderr = os.Stderr
266256

267257
return cmd.Run()
268258
}
269-
270-
func containsSpace(s string) bool {
271-
for _, c := range s {
272-
if c == ' ' || c == '\t' {
273-
return true
274-
}
275-
}
276-
return false
277-
}

cmd/cli/ssh/ssh_test.go

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -134,23 +134,3 @@ func TestGetHostURL_NoProperties(t *testing.T) {
134134
}
135135
}
136136

137-
func TestContainsSpace(t *testing.T) {
138-
tests := []struct {
139-
input string
140-
expected bool
141-
}{
142-
{"hello", false},
143-
{"hello world", true},
144-
{"hello\tworld", true},
145-
{"", false},
146-
{"nospaces", false},
147-
{" leading", true},
148-
{"trailing ", true},
149-
}
150-
151-
for _, tt := range tests {
152-
if got := containsSpace(tt.input); got != tt.expected {
153-
t.Errorf("containsSpace(%q) = %v, want %v", tt.input, got, tt.expected)
154-
}
155-
}
156-
}

cmd/cli/update/update.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -324,6 +324,9 @@ func (m *command) run(instanceID string) error {
324324
}
325325

326326
// Mark as provisioned
327+
if env.Labels == nil {
328+
env.Labels = make(map[string]string)
329+
}
327330
env.Labels[instances.InstanceProvisionedLabelKey] = "true"
328331
data, err := jyaml.MarshalYAML(env)
329332
if err != nil {

cmd/cli/update/update_test.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,3 +169,18 @@ func TestLabelApplication(t *testing.T) {
169169
t.Errorf("expected env=test, got %s", env.Labels["env"])
170170
}
171171
}
172+
173+
func TestProvisionedLabel_NilLabelsMap(t *testing.T) {
174+
// Regression test for C1: writing to nil Labels map should not panic
175+
env := &v1alpha1.Environment{}
176+
177+
// Simulate what update.go does after provisioning
178+
if env.Labels == nil {
179+
env.Labels = make(map[string]string)
180+
}
181+
env.Labels["holodeck-instance-provisioned"] = "true"
182+
183+
if env.Labels["holodeck-instance-provisioned"] != "true" {
184+
t.Error("expected provisioned label to be set")
185+
}
186+
}

0 commit comments

Comments
 (0)