test: minor refactor of TestRunChromeSandbox

This change includes some minor cleanup and refactor of the chrome
sandbox test.

- return error instead of calling Fatal in nested helper functions to
  help the control flow
- use the testing logger so that logs are properly formatted
- consolidate test utilities and remove unused functions

Author: sdowell

test/e2e/chromesandbox_test.go

@@ -20,98 +20,96 @@ import (
 	"io"
 	"net/http"
 	"os"
-	"os/exec"
-	"strings"
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/klog/v2"
+	sandboxv1alpha1 "sigs.k8s.io/agent-sandbox/api/v1alpha1"
 	"sigs.k8s.io/agent-sandbox/test/e2e/framework"
+	"sigs.k8s.io/agent-sandbox/test/e2e/framework/predicates"
 )
 
+func chromeSandbox() *sandboxv1alpha1.Sandbox {
+	sandbox := &sandboxv1alpha1.Sandbox{}
+	sandbox.Name = "chrome-sandbox"
+	sandbox.Spec.PodTemplate = sandboxv1alpha1.PodTemplate{
+		Spec: corev1.PodSpec{
+			Containers: []corev1.Container{
+				{
+					Name: "chrome-sandbox",
+					// might be nice to remove the IMAGE_TAG env var so this is easier to run from IDE
+					Image:           fmt.Sprintf("kind.local/chrome-sandbox:%s", os.Getenv("IMAGE_TAG")),
+					ImagePullPolicy: corev1.PullIfNotPresent,
+				},
+			},
+		},
+	}
+	return sandbox
+}
+
 // TestRunChromeSandbox tests that we can run Chrome inside a Sandbox,
 // it also measures how long it takes for Chrome to start serving the CDP protocol.
 func TestRunChromeSandbox(t *testing.T) {
-	ctx := context.Background()
-	ctx, cancel := context.WithCancel(ctx)
-	defer cancel()
-
-	log := klog.FromContext(ctx)
-
-	h := framework.NewTestContext(t)
+	tc := framework.NewTestContext(t)
 
-	ns := fmt.Sprintf("chrome-sandbox-test-%d", time.Now().UnixNano())
-	h.CreateTempNamespace(ctx, ns)
+	ns := &corev1.Namespace{}
+	ns.Name = fmt.Sprintf("chrome-sandbox-test-%d", time.Now().UnixNano())
+	require.NoError(t, tc.CreateWithCleanup(t.Context(), ns))
 
 	startTime := time.Now()
-
-	manifest := `
-kind: Sandbox
-apiVersion: agents.x-k8s.io/v1alpha1
-metadata:
-  name: chrome-sandbox
-spec:
-  podTemplate:
-    spec:
-      containers:
-      - name: chrome-sandbox
-        image: kind.local/chrome-sandbox:latest
-        imagePullPolicy: IfNotPresent
-`
-
-	manifest = strings.ReplaceAll(manifest, ":latest", ":"+os.Getenv("IMAGE_TAG"))
-
-	h.Apply(ctx, ns, manifest)
-
-	sandboxID := types.NamespacedName{
-		Namespace: ns,
-		Name:      "chrome-sandbox",
-	}
-
-	h.WaitForSandboxReady(ctx, sandboxID)
+	sandboxObj := chromeSandbox()
+	sandboxObj.Namespace = ns.Name
+	require.NoError(t, tc.CreateWithCleanup(t.Context(), sandboxObj))
+	require.NoError(t, tc.WaitForObject(t.Context(), sandboxObj, predicates.ReadyConditionIsTrue))
 
 	podID := types.NamespacedName{
-		Namespace: ns,
+		Namespace: ns.Name,
 		Name:      "chrome-sandbox",
 	}
-
+	podObj := &corev1.Pod{}
+	podObj.Name = podID.Name
+	podObj.Namespace = podID.Namespace
 	// Wait for the pod to be ready
-	{
-		waitForPodReady := exec.CommandContext(ctx, "kubectl", "-n", ns, "wait", "pod/"+podID.Name, "--for=condition=Ready", "--timeout=60s")
-		log.Info("waiting for pod to be ready", "command", waitForPodReady.String())
-		waitForPodReady.Stdout = os.Stdout
-		waitForPodReady.Stderr = os.Stderr
-		if err := waitForPodReady.Run(); err != nil {
-			t.Fatalf("failed to wait-for-pod-ready: %v", err)
-		}
-	}
+	require.NoError(t, tc.WaitForObject(t.Context(), podObj, predicates.ReadyConditionIsTrue))
+	// Wait for chrome to be ready
+	require.NoError(t, waitForChromeReady(t.Context(), tc, podID))
+	duration := time.Since(startTime)
+	t.Logf("Test took %s", duration)
+}
 
+func waitForChromeReady(ctx context.Context, tc *framework.TestContext, podID types.NamespacedName) error {
+	tc.Helper()
 	// Loop until we can query chrome for its version via the debug port
+	pollDuration := 100 * time.Millisecond
 	for {
-		if ctx.Err() != nil {
-			t.Fatalf("context cancelled")
+		select {
+		case <-ctx.Done():
+			return fmt.Errorf("context cancelled")
+		default:
+			// We have to port-forward in the loop because port-forward exits when it sees an error
+			// https://github.com/kubernetes/kubectl/issues/1249
+			portForwardCtx, portForwardCancel := context.WithCancel(ctx)
+			if err := tc.PortForward(portForwardCtx, podID, 9222, 9222); err != nil {
+				tc.Errorf("failed to port forward: %s", err)
+				portForwardCancel()
+				time.Sleep(pollDuration)
+				continue
+			}
+
+			u := "http://localhost:9222/json/version"
+			info, err := getChromeInfo(ctx, u)
+			portForwardCancel()
+			if err != nil {
+				tc.Errorf("failed to get chrome info: %s", err)
+				time.Sleep(pollDuration)
+				continue
+			}
+			tc.Logf("Chrome is ready (%s). Response: %s", u, info)
+			return nil
 		}
-
-		// We have to port-forward in the loop because port-forward exits when it sees an error
-		// https://github.com/kubernetes/kubectl/issues/1249
-		portForwardCtx, portForwardCancel := context.WithCancel(ctx)
-		h.PortForward(portForwardCtx, podID, 9222, 9222)
-
-		u := "http://localhost:9222/json/version"
-		info, err := getChromeInfo(ctx, u)
-		portForwardCancel()
-		if err != nil {
-			log.Error(err, "failed to get Chrome info")
-			time.Sleep(100 * time.Millisecond)
-			continue
-		}
-		log.Info("Chrome is ready", "url", u, "response", info)
-		break
 	}
-
-	duration := time.Since(startTime)
-	log.Info("Test completed successfully", "duration", duration)
 }
 
 // getChromeInfo connects to the Chrome Debug Port and retrieves the version information.

test/e2e/framework/client.go

@@ -21,7 +21,6 @@ import (
 	"io"
 	"os"
 	"os/exec"
-	"path/filepath"
 	"strings"
 	"testing"
 	"time"
@@ -30,20 +29,15 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/client-go/rest"
-	"k8s.io/klog/v2"
 	"sigs.k8s.io/agent-sandbox/test/e2e/framework/predicates"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 // ClusterClient is an abstraction layer for test cases to interact with the cluster.
 type ClusterClient struct {
 	*testing.T
-	client     client.Client
-	restConfig *rest.Config
+	client client.Client
 }
 
 // Update an object that already exists on the cluster.
@@ -205,53 +199,18 @@ func (cl *ClusterClient) validateAgentSandboxInstallation(ctx context.Context) e
 	return nil
 }
 
-func (cl *ClusterClient) Apply(ctx context.Context, namespace string, manifest string) {
-	tempDir := cl.T.TempDir()
-	manifestPath := filepath.Join(tempDir, "manifest.yaml")
-	if err := os.WriteFile(manifestPath, []byte(manifest), 0644); err != nil {
-		cl.T.Fatalf("failed to write manifest file %q: %v", manifestPath, err)
-	}
-	cmd := exec.CommandContext(ctx, "kubectl", "apply", "-f", manifestPath, "-n", namespace)
-	cmd.Stdout = os.Stdout
-	cmd.Stderr = os.Stderr
-	if err := cmd.Run(); err != nil {
-		cl.T.Fatalf("failed to apply manifest %q: %v", manifestPath, err)
-	}
-}
-
-var sandboxGVK = schema.GroupVersionKind{
-	Group:   "agents.x-k8s.io",
-	Version: "v1alpha1",
-	Kind:    "Sandbox",
-}
-
-func (cl *ClusterClient) RESTConfig() *rest.Config {
-	return cl.restConfig
-}
-
-func (cl *ClusterClient) CreateTempNamespace(ctx context.Context, name string) {
-	ns := &unstructured.Unstructured{}
-	ns.SetAPIVersion("v1")
-	ns.SetKind("Namespace")
-	ns.SetName(name)
-
-	if err := cl.CreateWithCleanup(ctx, ns); err != nil {
-		cl.T.Fatalf("failed to create namespace %q: %v", name, err)
-	}
-}
-
-func (cl *ClusterClient) PortForward(ctx context.Context, pod types.NamespacedName, localPort, remotePort int) {
-	log := klog.FromContext(ctx)
-
+func (cl *ClusterClient) PortForward(ctx context.Context, pod types.NamespacedName, localPort, remotePort int) error {
+	cl.Helper()
 	// Set up a port-forward to the Chrome Debug Port
-	portForward := exec.CommandContext(ctx, "kubectl", "-n", pod.Namespace, "port-forward", "pod/"+pod.Name, fmt.Sprintf("%d:%d", localPort, remotePort))
-	log.Info("starting port-forward", "command", portForward.String())
+	portForward := exec.CommandContext(ctx, "kubectl", "-n", pod.Namespace,
+		"port-forward", "pod/"+pod.Name, fmt.Sprintf("%d:%d", localPort, remotePort))
+	cl.Logf("starting port-forward: %s", portForward.String())
 	var stdout bytes.Buffer
 	var stderr bytes.Buffer
 	portForward.Stdout = io.MultiWriter(os.Stdout, &stdout)
 	portForward.Stderr = io.MultiWriter(os.Stderr, &stderr)
 	if err := portForward.Start(); err != nil {
-		cl.T.Fatalf("failed to start port-forward: %v", err)
+		return fmt.Errorf("failed to start port-forward: %w", err)
 	}
 
 	stopProcess := func() {
@@ -260,18 +219,19 @@ func (cl *ClusterClient) PortForward(ctx context.Context, pod types.NamespacedNa
 				return
 			}
 		}
-		log.Info("killing port-forward")
+		cl.Log("killing port-forward")
 		if err := portForward.Process.Kill(); err != nil {
-			log.Error(err, "failed to kill port-forward")
+			cl.Errorf("failed to kill port-forward: %s", err)
 		}
 	}
 	cl.T.Cleanup(stopProcess)
 
 	go func() {
+		cl.Helper()
 		if err := portForward.Wait(); err != nil {
-			log.Error(err, "port-forward exited with error")
+			cl.Logf("port-forward exited with error: %s", err)
 		} else {
-			log.Info("port-forward exited")
+			cl.Log("port-forward exited")
 		}
 	}()
 
@@ -280,26 +240,16 @@ func (cl *ClusterClient) PortForward(ctx context.Context, pod types.NamespacedNa
 	for {
 		if portForward.ProcessState != nil {
 			if portForward.ProcessState.Exited() {
-				cl.T.Fatalf("port-forward process exited unexpectedly: stdout=%q stderr=%q", stdout.String(), stderr.String())
+				return fmt.Errorf("port-forward process exited unexpectedly: stdout=%q stderr=%q", stdout.String(), stderr.String())
 			}
 		}
 
 		// Check stdout for the "Forwarding from" message
 		if strings.Contains(stdout.String(), "Forwarding from") {
-			log.Info("port-forward is ready", "stdout", stdout.String(), "stderr", stderr.String())
+			cl.Logf("port-forward is ready\nstdout: %s\nstderr: %s", stdout.String(), stderr.String())
 			break
 		}
 		time.Sleep(5 * time.Millisecond)
 	}
-}
-
-func (cl *ClusterClient) WaitForSandboxReady(ctx context.Context, sandboxID types.NamespacedName) {
-	sandbox := &unstructured.Unstructured{}
-	sandbox.SetGroupVersionKind(sandboxGVK)
-	sandbox.SetName(sandboxID.Name)
-	sandbox.SetNamespace(sandboxID.Namespace)
-
-	if err := cl.WaitForObject(ctx, sandbox, predicates.ReadyConditionIsTrue); err != nil {
-		cl.T.Fatalf("waiting for sandbox to be ready: %v", err)
-	}
+	return nil
 }

test/e2e/framework/context.go

@@ -66,9 +66,8 @@ func NewTestContext(t *testing.T) *TestContext {
 		t.Fatal(err)
 	}
 	th.ClusterClient = ClusterClient{
-		T:          t,
-		client:     cl,
-		restConfig: restCfg,
+		T:      t,
+		client: cl,
 	}
 	t.Cleanup(func() {
 		t.Helper()