bugfix(k8s): avoid InspectContainer panic where container is not Terminated (#308)

Author: cognifloyd

runtime/kubernetes/container.go

@@ -54,6 +54,24 @@ func (c *client) InspectContainer(ctx context.Context, ctn *pipeline.Container)
 			continue
 		}
 
+		// avoid a panic if the build ends without terminating all containers
+		if cst.State.Terminated == nil {
+			for _, container := range pod.Spec.Containers {
+				if cst.Name != container.Name {
+					continue
+				}
+
+				// steps that were not executed will still be "running" the pause image as expected.
+				if container.Image == pauseImage {
+					return nil
+				}
+
+				break
+			}
+
+			return fmt.Errorf("expected container %s to be terminated, got %v", ctn.ID, cst.State)
+		}
+
 		// set the step exit code
 		ctn.ExitCode = int(cst.State.Terminated.ExitCode)
 
@@ -122,7 +140,7 @@ func (c *client) SetupContainer(ctx context.Context, ctn *pipeline.Container) er
 		// the containers with the proper image.
 		//
 		// https://hub.docker.com/r/kubernetes/pause
-		Image:           image.Parse("kubernetes/pause:latest"),
+		Image:           image.Parse(pauseImage),
 		Env:             []v1.EnvVar{},
 		Stdin:           false,
 		StdinOnce:       false,

runtime/kubernetes/container_test.go

@@ -18,42 +18,71 @@ import (
 )
 
 func TestKubernetes_InspectContainer(t *testing.T) {
-	// setup types
-	_engine, err := NewMock(_pod)
-	if err != nil {
-		t.Errorf("unable to create runtime engine: %v", err)
-	}
-
 	// setup tests
 	tests := []struct {
+		name      string
 		failure   bool
+		pod       *v1.Pod
 		container *pipeline.Container
 	}{
 		{
+			name:      "build container",
 			failure:   false,
+			pod:       _pod,
 			container: _container,
 		},
 		{
+			name:      "empty build container",
 			failure:   false,
+			pod:       _pod,
 			container: new(pipeline.Container),
 		},
+		{
+			name:    "container not terminated",
+			failure: true,
+			pod: &v1.Pod{
+				ObjectMeta: _pod.ObjectMeta,
+				TypeMeta:   _pod.TypeMeta,
+				Spec:       _pod.Spec,
+				Status: v1.PodStatus{
+					Phase: v1.PodRunning,
+					ContainerStatuses: []v1.ContainerStatus{
+						{
+							Name: "step-github-octocat-1-clone",
+							State: v1.ContainerState{
+								Running: &v1.ContainerStateRunning{},
+							},
+						},
+					},
+				},
+			},
+			container: _container,
+		},
 	}
 
 	// run tests
 	for _, test := range tests {
-		err = _engine.InspectContainer(context.Background(), test.container)
-
-		if test.failure {
-			if err == nil {
-				t.Errorf("InspectContainer should have returned err")
+		t.Run(test.name, func(t *testing.T) {
+			// setup types
+			_engine, err := NewMock(test.pod)
+			if err != nil {
+				t.Errorf("unable to create runtime engine: %v", err)
 			}
 
-			continue
-		}
+			err = _engine.InspectContainer(context.Background(), test.container)
 
-		if err != nil {
-			t.Errorf("InspectContainer returned err: %v", err)
-		}
+			if test.failure {
+				if err == nil {
+					t.Errorf("InspectContainer should have returned err")
+				}
+
+				return // effectively "continue" to next test
+			}
+
+			if err != nil {
+				t.Errorf("InspectContainer returned err: %v", err)
+			}
+		})
 	}
 }
 

runtime/kubernetes/image.go

@@ -14,7 +14,9 @@ import (
 	"github.com/go-vela/types/pipeline"
 )
 
-const imagePatch = `
+const (
+	pauseImage = "kubernetes/pause:latest"
+	imagePatch = `
 {
   "spec": {
     "containers": [
@@ -26,6 +28,7 @@ const imagePatch = `
   }
 }
 `
+)
 
 // CreateImage creates the pipeline container image.
 func (c *client) CreateImage(ctx context.Context, ctn *pipeline.Container) error {