From f689e8c1936ec71c560fbc70561441abe9bac501 Mon Sep 17 00:00:00 2001
From: Antonin Bas <abas@vmware.com>
Date: Wed, 9 Aug 2023 14:13:28 -0700
Subject: [PATCH] [k8s] Fix panic in WaitUntilDeploymentAvailable

The `func (err DeploymentNotAvailable) Error()` method should not assume
that `Status.Conditions[0]` is always valid for the Deployment.

Fixes #1329

Signed-off-by: Antonin Bas <abas@vmware.com>
---
 modules/k8s/deployment.go      | 17 ++++++----
 modules/k8s/deployment_test.go |  9 +++++
 modules/k8s/errors.go          | 16 +++++++--
 modules/k8s/errors_test.go     | 60 ++++++++++++++++++++++++++++++++++
 4 files changed, 92 insertions(+), 10 deletions(-)
 create mode 100644 modules/k8s/errors_test.go

diff --git a/modules/k8s/deployment.go b/modules/k8s/deployment.go
index 14225bb97..c713d0ff9 100644
--- a/modules/k8s/deployment.go
+++ b/modules/k8s/deployment.go
@@ -98,13 +98,16 @@ func WaitUntilDeploymentAvailableE(
 
 // IsDeploymentAvailable returns true if all pods within the deployment are ready and started
 func IsDeploymentAvailable(deploy *appsv1.Deployment) bool {
-	for _, dc := range deploy.Status.Conditions {
-		if dc.Type == appsv1.DeploymentProgressing &&
-			dc.Status == v1.ConditionTrue &&
-			dc.Reason == "NewReplicaSetAvailable" {
-			return true
+	dc := getDeploymentCondition(deploy, appsv1.DeploymentProgressing)
+	return dc != nil && dc.Status == v1.ConditionTrue && dc.Reason == "NewReplicaSetAvailable"
+}
+
+func getDeploymentCondition(deploy *appsv1.Deployment, cType appsv1.DeploymentConditionType) *appsv1.DeploymentCondition {
+	for idx := range deploy.Status.Conditions {
+		dc := &deploy.Status.Conditions[idx]
+		if dc.Type == cType {
+			return dc
 		}
 	}
-
-	return false
+	return nil
 }
diff --git a/modules/k8s/deployment_test.go b/modules/k8s/deployment_test.go
index 7dde12468..2327e2ec9 100644
--- a/modules/k8s/deployment_test.go
+++ b/modules/k8s/deployment_test.go
@@ -110,6 +110,15 @@ func TestTestIsDeploymentAvailable(t *testing.T) {
 			},
 			expectedResult: false,
 		},
+		{
+			title: "TestIsDeploymentAvailableWithoutProgressingCondition",
+			deploy: &appsv1.Deployment{
+				Status: appsv1.DeploymentStatus{
+					Conditions: []appsv1.DeploymentCondition{},
+				},
+			},
+			expectedResult: false,
+		},
 	}
 
 	for _, tc := range testCases {
diff --git a/modules/k8s/errors.go b/modules/k8s/errors.go
index da87bb640..1b098b2e3 100644
--- a/modules/k8s/errors.go
+++ b/modules/k8s/errors.go
@@ -69,11 +69,21 @@ type DeploymentNotAvailable struct {
 
 // Error is a simple function to return a formatted error message as a string
 func (err DeploymentNotAvailable) Error() string {
+	dc := getDeploymentCondition(err.deploy, appsv1.DeploymentProgressing)
+	if dc == nil {
+		return fmt.Sprintf(
+			"Deployment %s is not available, missing '%s' condition",
+			err.deploy.Name,
+			appsv1.DeploymentProgressing,
+		)
+	}
 	return fmt.Sprintf(
-		"Deployment %s is not available, reason: %s, message: %s",
+		"Deployment %s is not available as '%s' condition indicates that the Deployment is not complete, status: %v, reason: %s, message: %s",
 		err.deploy.Name,
-		err.deploy.Status.Conditions[0].Reason,
-		err.deploy.Status.Conditions[0].Message,
+		appsv1.DeploymentProgressing,
+		dc.Status,
+		dc.Reason,
+		dc.Message,
 	)
 }
 
diff --git a/modules/k8s/errors_test.go b/modules/k8s/errors_test.go
new file mode 100644
index 000000000..37304d34f
--- /dev/null
+++ b/modules/k8s/errors_test.go
@@ -0,0 +1,60 @@
+package k8s
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	appsv1 "k8s.io/api/apps/v1"
+	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestErrorDeploymentNotAvailable(t *testing.T) {
+	testCases := []struct {
+		title       string
+		deploy      *appsv1.Deployment
+		expectedErr string
+	}{
+		{
+			title: "NoProgressingCondition",
+			deploy: &appsv1.Deployment{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "foo",
+				},
+				Status: appsv1.DeploymentStatus{
+					Conditions: []appsv1.DeploymentCondition{},
+				},
+			},
+			expectedErr: "Deployment foo is not available, missing 'Progressing' condition",
+		},
+		{
+			title: "DeploymentNotComplete",
+			deploy: &appsv1.Deployment{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "foo",
+				},
+				Status: appsv1.DeploymentStatus{
+					Conditions: []appsv1.DeploymentCondition{
+						{
+							Type:    appsv1.DeploymentProgressing,
+							Status:  v1.ConditionTrue,
+							Reason:  "ReplicaSetUpdated",
+							Message: "bar",
+						},
+					},
+				},
+			},
+			expectedErr: "Deployment foo is not available as 'Progressing' condition indicates that the Deployment is not complete, status: True, reason: ReplicaSetUpdated, message: bar",
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.title, func(t *testing.T) {
+			t.Parallel()
+			err := NewDeploymentNotAvailableError(tc.deploy)
+			assert.EqualError(t, err, tc.expectedErr)
+		})
+	}
+}