fix(e2e-framework): return last error from SshRunCommand instead of (… (#54536)

…nil, nil)

## What does this PR do?

Fixes an `err` variable-shadowing bug in `SshRunCommand` (`test/e2e-framework/testing/utils/ssh/ssh.go`) that made the retry helper return `(nil, nil)` — no output **and** no error — whenever a command failed on every retry.

```go
var err error
for range NR_SSH_COMMAND_RETRIES {
    sshSession, err := sshClient.NewSession()      // := re-declares err in loop scope
    ...
    output, err := sshSession.CombinedOutput(command)  // := again, loop-scoped
    if err == nil {
        return output, nil
    }
}
return nil, err   // outer err is always nil → returns (nil, nil) on persistent failure
```

Both `:=` statements declare a loop-scoped `err`, so the outer `err` is never assigned. The helper's own doc comment ("Returns the last error if retries exceeded") was effectively a lie — it couldn't.

The fix assigns to the outer `err` (`var` decls + `=`) so the last failure propagates, and closes the `ssh.Session` between retries (it was previously leaked on every attempt).

## Motivation

Regression introduced in #48951 (commit `7b455c21`) when the SSH helper was moved into the shared infra package. It was surfaced by #49708, which added the OpenShift teardown dump.

The only caller is the GCP dump path (`testing/provisioners/gcp/kubernetes/kubernetes_dump.go`):

```go
sshOutput, err := sshutils.SshRunCommand(sshClient, "cat .kube/config", &out)
if err != nil { ... return }              // never taken — err is nil
kubeConfig, err := clientcmd.Load(sshOutput)   // fed nil/empty bytes instead
```

When `cat .kube/config` over SSH fails, the error guard is skipped and empty bytes flow into `clientcmd.Load`, so OpenShift teardown diagnostics emit misleading garbage (`Forbidden: cannot list ...`) instead of a clean "SSH failed" error — which is exactly the confusing output seen on failing `new-e2e-ssi-openshift` runs (INPLAT-995).

## Describe how you validated your changes

- Added `ssh_test.go` with an in-memory SSH server that completes the handshake but fails every command; asserts `SshRunCommand` returns a non-nil error after exhausting retries.
- Verified the test **fails** (`An error is expected but got nil`) against the original buggy code and **passes** with the fix.
- Confirmed default `go vet` produces **no** shadow warning for the original code — this bug is invisible to standard tooling (follow-up: consider enabling a `shadow` linter).
- `bazel test //test/e2e-framework/testing/utils/ssh:ssh_test` → PASS. `BUILD.bazel` regenerated via gazelle.

## Additional Notes

Test-framework-only change (`test/e2e-framework/`), no user-facing Agent behavior — no release note.


Co-authored-by: frank.spano <frank.spano@datadoghq.com>

Author: frank-spano

test/e2e-framework/testing/utils/ssh/BUILD.bazel

@@ -1,4 +1,5 @@
 load("@rules_go//go:def.bzl", "go_library")
+load("//bazel/rules/go:dd_agent_go_test.bzl", "dd_agent_go_test")
 
 go_library(
     name = "ssh",
@@ -10,3 +11,13 @@ go_library(
         "@org_golang_x_crypto//ssh/agent",
     ],
 )
+
+dd_agent_go_test(
+    name = "ssh_test",
+    srcs = ["ssh_test.go"],
+    embed = [":ssh"],
+    deps = [
+        "@com_github_stretchr_testify//require",
+        "@org_golang_x_crypto//ssh",
+    ],
+)

test/e2e-framework/testing/utils/ssh/ssh.go

@@ -77,12 +77,18 @@ func SshRunCommand(sshClient *ssh.Client, command string, logger io.Writer) ([]b
 
 	var err error
 	for range NR_SSH_COMMAND_RETRIES {
-		sshSession, err := sshClient.NewSession()
+		var sshSession *ssh.Session
+		// Assign to the outer err (not ":=") so the last failure is returned
+		// once retries are exhausted; shadowing it here made this function
+		// return (nil, nil) on persistent failures.
+		sshSession, err = sshClient.NewSession()
 		if err != nil {
 			return nil, err
 		}
 
-		output, err := sshSession.CombinedOutput(command)
+		var output []byte
+		output, err = sshSession.CombinedOutput(command)
+		sshSession.Close()
 		if err == nil {
 			return output, nil
 		}

test/e2e-framework/testing/utils/ssh/ssh_test.go

@@ -0,0 +1,113 @@
+// Unless explicitly stated otherwise all files in this repository are licensed
+// under the Apache License Version 2.0.
+// This product includes software developed at Datadog (https://www.datadoghq.com/).
+// Copyright 2016-present Datadog, Inc.
+
+package ssh
+
+import (
+	"crypto/rand"
+	"crypto/rsa"
+	"io"
+	"net"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"golang.org/x/crypto/ssh"
+)
+
+// startFailingSSHServer starts an in-memory SSH server that accepts
+// connections and completes (no-auth) handshakes, but makes every command
+// execution fail with a non-zero exit status. It returns the listen address.
+func startFailingSSHServer(t *testing.T) string {
+	t.Helper()
+
+	key, err := rsa.GenerateKey(rand.Reader, 2048)
+	require.NoError(t, err)
+	signer, err := ssh.NewSignerFromKey(key)
+	require.NoError(t, err)
+
+	cfg := &ssh.ServerConfig{NoClientAuth: true}
+	cfg.AddHostKey(signer)
+
+	ln, err := net.Listen("tcp", "127.0.0.1:0")
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = ln.Close() })
+
+	go func() {
+		for {
+			conn, err := ln.Accept()
+			if err != nil {
+				return // listener closed on cleanup
+			}
+			go serveFailingConn(conn, cfg)
+		}
+	}()
+
+	return ln.Addr().String()
+}
+
+func serveFailingConn(nConn net.Conn, cfg *ssh.ServerConfig) {
+	sConn, chans, reqs, err := ssh.NewServerConn(nConn, cfg)
+	if err != nil {
+		return
+	}
+	defer sConn.Close()
+	go ssh.DiscardRequests(reqs)
+
+	for newChan := range chans {
+		if newChan.ChannelType() != "session" {
+			_ = newChan.Reject(ssh.UnknownChannelType, "unknown channel type")
+			continue
+		}
+		ch, chReqs, err := newChan.Accept()
+		if err != nil {
+			continue
+		}
+		go func() {
+			for req := range chReqs {
+				switch req.Type {
+				case "exec", "shell":
+					if req.WantReply {
+						_ = req.Reply(true, nil)
+					}
+					// Report a non-zero exit status so the client's
+					// CombinedOutput returns an *ssh.ExitError.
+					_, _ = ch.SendRequest("exit-status", false, ssh.Marshal(struct{ Status uint32 }{Status: 1}))
+					_ = ch.Close()
+				default:
+					if req.WantReply {
+						_ = req.Reply(false, nil)
+					}
+				}
+			}
+		}()
+	}
+}
+
+func dialSSHClient(t *testing.T, addr string) *ssh.Client {
+	t.Helper()
+	conn, err := net.Dial("tcp", addr)
+	require.NoError(t, err)
+	c, chans, reqs, err := ssh.NewClientConn(conn, addr, &ssh.ClientConfig{
+		User:            "test",
+		HostKeyCallback: ssh.InsecureIgnoreHostKey(),
+	})
+	require.NoError(t, err)
+	client := ssh.NewClient(c, chans, reqs)
+	t.Cleanup(func() { _ = client.Close() })
+	return client
+}
+
+// TestSshRunCommandReturnsErrorAfterRetries guards against the err
+// variable-shadowing regression that made SshRunCommand return (nil, nil)
+// when a command failed on every retry (see PR #48951 / commit 7b455c21).
+func TestSshRunCommandReturnsErrorAfterRetries(t *testing.T) {
+	addr := startFailingSSHServer(t)
+	client := dialSSHClient(t, addr)
+
+	output, err := SshRunCommand(client, "false", io.Discard)
+
+	require.Error(t, err, "SshRunCommand must return the command error after exhausting retries, not (nil, nil)")
+	require.Nil(t, output)
+}