Refactor pkg/kubernetes (#57)

This PR refactos pkg/kubernetes to make it a bit more Goish™, and also tries add some missing unit tests. I think the full files view should be enough, but in any case each commit is trying to be as small and scoped as possible so it might be easier to review that way.

* Refactor Kubernetes.GetPod

This function wasn't being used, only tested. Here we refactor it so
we can eventually encapsulate this logic in this struct, and add
proper testing.

* Use Kubernetes.GetPods for finding pods

* Hide low-level Kubernetes fields

These aren't necessary for proper utilization of this struct.

* Rename kubernetes.GetKubernetes to kubernetes.New

* Refactor kubernetes.New

* Remove unused field

* Refactor chooseTargetContainer

* Add getEphemeralContainerExitCode unit test

* Add isEphemeralContainerTerminated unit test

* Revert get cluster config logic

See [note] on pull request, we should probably fix this.

[note]: #57 (comment)

Author: inkel

cmd/runner/executetest.go

@@ -14,7 +14,6 @@ import (
 	"github.com/grafana/nethax/pkg/kubernetes"
 	"github.com/spf13/cobra"
 	corev1 "k8s.io/api/core/v1"
-	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 // ExecuteTest returns the execute-test command
@@ -44,7 +43,7 @@ func ExecuteTest() *cobra.Command {
 				common.ExitConfigError()
 			}
 
-			k, err := kubernetes.GetKubernetes("")
+			k, err := kubernetes.New("")
 			if err != nil {
 				cmd.Printf("Error creating Kubernetes client: %v\n", err)
 				common.ExitConfigError()
@@ -171,19 +170,12 @@ func isPodReady(pod *corev1.Pod) bool {
 }
 
 func findPods(ctx context.Context, k *kubernetes.Kubernetes, mode, namespace, selector string) ([]corev1.Pod, error) {
-	// Find pods matching the selector
-	pods, err := k.Client.CoreV1().Pods(namespace).List(ctx, v1.ListOptions{
-		LabelSelector: selector,
-	})
+	pods, err := k.GetPods(ctx, namespace, selector)
 	if err != nil {
 		return nil, fmt.Errorf("failed to find pods: %w", err)
 	}
 
-	if len(pods.Items) == 0 {
-		return nil, fmt.Errorf("no pods found matching selector %s", selector)
-	}
-
-	return selectPods(mode, pods.Items)
+	return selectPods(mode, pods)
 }
 
 var (

pkg/kubernetes/kubernetes.go

@@ -3,8 +3,8 @@ package kubernetes
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
-	"log"
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
@@ -24,64 +24,76 @@ var (
 	ProbeImageVersion = "latest"
 )
 
-func GetKubernetes(context string) (*Kubernetes, error) {
+// New returns a new Kubernetes object, connected to the given
+// context, or to the in-cluster API if blank.
+func New(context string) (*Kubernetes, error) {
+	config, err := getClusterConfig(context)
+	if err != nil {
+		return nil, fmt.Errorf("fetching Kubernetes configuration: %w", err)
+	}
+
+	client, err := kubernetes.NewForConfig(config)
+	if err != nil {
+		return nil, fmt.Errorf("creating Kubernetes client: %w", err)
+	}
+
+	return &Kubernetes{
+		client: client,
+	}, nil
+}
+
+func getClusterConfig(kontext string) (*rest.Config, error) {
 	// attempt to use config from pod service account
-	config, err := rest.InClusterConfig()
+	cfg, err := rest.InClusterConfig()
 	if err != nil {
 		// Can be overridden by KUBECONFIG variable
 		loadingRules := clientcmd.NewDefaultClientConfigLoadingRules()
-		configOverride := &clientcmd.ConfigOverrides{}
-		if context != "" {
-			configOverride.CurrentContext = context
+		configOverride := &clientcmd.ConfigOverrides{
+			CurrentContext: kontext,
 		}
 
-		config, err = clientcmd.NewNonInteractiveDeferredLoadingClientConfig(
+		cfg, err = clientcmd.NewNonInteractiveDeferredLoadingClientConfig(
 			loadingRules,
 			configOverride,
 		).ClientConfig()
-
 		if err != nil {
-			return nil, fmt.Errorf("fetching Kubernetes configuration: %w", err)
+			return nil, fmt.Errorf("loading Kubernetes configuration: %w", err)
 		}
 	}
 
-	client, err := kubernetes.NewForConfig(config)
-	if err != nil {
-		return nil, fmt.Errorf("creating Kubernetes client: %w", err)
-	}
-
-	return &Kubernetes{
-		Client: client,
-		Config: config,
-	}, nil
+	return cfg, nil
 }
 
 type Kubernetes struct {
-	Config *rest.Config
-	Client kubernetes.Interface
+	client kubernetes.Interface
 }
 
-func (k *Kubernetes) GetPods(ctx context.Context, namespace string) []string {
-	pods, err := k.Client.CoreV1().Pods(namespace).List(
-		ctx,
-		metav1.ListOptions{})
+var (
+	errNoPodsFound       = errors.New("no pods found")
+	errNoContainersInPod = errors.New("no containers in pod")
+)
 
+func (k *Kubernetes) GetPods(ctx context.Context, namespace, selector string) ([]corev1.Pod, error) {
+	pods, err := k.client.CoreV1().Pods(namespace).List(ctx, metav1.ListOptions{
+		LabelSelector: selector,
+	})
 	if err != nil {
-		panic(err.Error())
+		return nil, fmt.Errorf("listing pods for namespace %s and selector %q: %w", namespace, selector, err)
 	}
-	podNames := []string{}
-	for _, pod := range pods.Items {
-		podNames = append(podNames, pod.Name)
+
+	if len(pods.Items) == 0 {
+		return nil, fmt.Errorf("%w: namespace %s, selector %q", errNoPodsFound, namespace, selector)
 	}
-	return podNames
+
+	return pods.Items, nil
 }
 
-func chooseTargetContainer(pod *corev1.Pod) string {
+func chooseTargetContainer(pod *corev1.Pod) (string, error) {
 	// TODO add capability to pick container by name (currently assume 0th container)
 	if len(pod.Spec.Containers) == 0 {
-		log.Fatalf("Error: No containers in pod.")
+		return "", errNoContainersInPod
 	}
-	return pod.Spec.Containers[0].Name
+	return pod.Spec.Containers[0].Name, nil
 }
 
 func (k *Kubernetes) LaunchEphemeralContainer(ctx context.Context, pod *corev1.Pod, command []string, args []string) (*corev1.Pod, string, error) {
@@ -92,14 +104,19 @@ func (k *Kubernetes) LaunchEphemeralContainer(ctx context.Context, pod *corev1.P
 
 	ephemeralName := fmt.Sprintf("nethax-probe-%v", time.Now().UnixNano())
 
+	targetContainer, err := chooseTargetContainer(pod)
+	if err != nil {
+		return nil, "", fmt.Errorf("choosing target container: %w", err)
+	}
+
 	debugContainer := &corev1.EphemeralContainer{
 		EphemeralContainerCommon: corev1.EphemeralContainerCommon{
 			Name:    ephemeralName,
 			Image:   fmt.Sprintf("nethax-probe:%s", ProbeImageVersion),
 			Command: command,
 			Args:    args,
 		},
-		TargetContainerName: chooseTargetContainer(pod),
+		TargetContainerName: targetContainer,
 	}
 
 	debugPod := pod.DeepCopy()
@@ -115,7 +132,7 @@ func (k *Kubernetes) LaunchEphemeralContainer(ctx context.Context, pod *corev1.P
 		return nil, ephemeralName, fmt.Errorf("error creating patch to add debug container: %v", err)
 	}
 
-	pods := k.Client.CoreV1().Pods(pod.Namespace)
+	pods := k.client.CoreV1().Pods(pod.Namespace)
 	result, err := pods.Patch(ctx, pod.Name, types.StrategicMergePatchType, patch, metav1.PatchOptions{}, "ephemeralcontainers")
 	if err != nil {
 		return nil, ephemeralName, fmt.Errorf("error patching pod with debug container: %v", err)
@@ -125,7 +142,7 @@ func (k *Kubernetes) LaunchEphemeralContainer(ctx context.Context, pod *corev1.P
 }
 
 func (k *Kubernetes) getEphemeralContainerExitStatus(ctx context.Context, pod *corev1.Pod, ephemeralContainerName string) (int32, error) {
-	pod, err := k.Client.CoreV1().Pods(pod.Namespace).Get(ctx, pod.Name, metav1.GetOptions{})
+	pod, err := k.client.CoreV1().Pods(pod.Namespace).Get(ctx, pod.Name, metav1.GetOptions{})
 	if err != nil {
 		return -1, err
 	}