Test and remove panic possibilities in newDashboardCompatibleRelease (#…

…1445)

cmd/kubeops/internal/handler/dashboardcompat.go

@@ -19,13 +19,23 @@ import (
 	h2 "k8s.io/helm/pkg/proto/hapi/release"
 )
 
+var (
+	// ErrUnableToConvertWithoutInfo indicates that the input release had nil Info or Chart.
+	ErrUnableToConvertWithoutInfo = fmt.Errorf("unable to convert release without info")
+	// ErrUnableToParseDeletionTime indicates that the deletion time of the h3 chart could not be parsed.
+	ErrFailedToParseDeletionTime = fmt.Errorf("failed to parse deletion time")
+)
+
 func newDashboardCompatibleRelease(h3r h3.Release) (h2.Release, error) {
+	if h3r.Info == nil || h3r.Chart == nil || h3r.Chart.Metadata == nil {
+		return h2.Release{}, ErrUnableToConvertWithoutInfo
+	}
 	var deleted *timestamp.Timestamp
 	if !h3r.Info.Deleted.IsZero() {
 		var err error
 		deleted, err = ptypes.TimestampProto(h3r.Info.Deleted.Time)
 		if err != nil {
-			return h2.Release{}, fmt.Errorf("Failed to parse deletion time %v", err)
+			return h2.Release{}, ErrFailedToParseDeletionTime
 		}
 	}
 	return h2.Release{

cmd/kubeops/internal/handler/dashboardcompat_test.go

@@ -2,19 +2,29 @@ package handler
 
 import (
 	"net/http/httptest"
-	"runtime/debug"
 
+	"github.com/golang/protobuf/ptypes/timestamp"
 	"github.com/google/go-cmp/cmp"
 	"github.com/kubeapps/common/response"
 	h3chart "helm.sh/helm/v3/pkg/chart"
 	h3 "helm.sh/helm/v3/pkg/release"
+	helmtime "helm.sh/helm/v3/pkg/time"
 	h2chart "k8s.io/helm/pkg/proto/hapi/chart"
 	h2 "k8s.io/helm/pkg/proto/hapi/release"
 
 	"testing"
 )
 
 func TestNewDashboardCompatibleRelease(t *testing.T) {
+	const (
+		validSeconds   = 1452902400
+		invalidSeconds = 253402300801
+	)
+	var (
+		validDeletedTime   = helmtime.Unix(validSeconds, 0)
+		invalidDeletedTime = helmtime.Unix(invalidSeconds, 0)
+	)
+
 	type testScenario struct {
 		// Scenario params
 		Description  string
@@ -25,6 +35,7 @@ func TestNewDashboardCompatibleRelease(t *testing.T) {
 		// and how to format the fields of a Helm 2 release.
 		// E.g. spew.Dumps is a valid marhsalling function.
 		MarshallingFunction func(h2.Release) string
+		ExpectedError       error
 	}
 	tests := []testScenario{
 		{
@@ -113,20 +124,93 @@ func TestNewDashboardCompatibleRelease(t *testing.T) {
 				},
 			},
 		},
+		{
+			Description:         "returns an error rather than panicking for a Helm 3 Release without Info",
+			MarshallingFunction: asResponse,
+			Helm3Release: h3.Release{
+				Name:  "Incomplete",
+				Chart: &h3chart.Chart{},
+			},
+			ExpectedError: ErrUnableToConvertWithoutInfo,
+		},
+		{
+			Description:         "returns an error for a Helm 3 Release without Chart",
+			MarshallingFunction: asResponse,
+			Helm3Release: h3.Release{
+				Name: "Incomplete",
+				Info: &h3.Info{},
+			},
+			ExpectedError: ErrUnableToConvertWithoutInfo,
+		},
+		{
+			Description:         "returns an error for a Helm 3 Release without Chart.Metadata",
+			MarshallingFunction: asResponse,
+			Helm3Release: h3.Release{
+				Name:  "Incomplete",
+				Info:  &h3.Info{},
+				Chart: &h3chart.Chart{},
+			},
+			ExpectedError: ErrUnableToConvertWithoutInfo,
+		},
+		{
+			Description:         "parses and includes the deleted time",
+			MarshallingFunction: asResponse,
+			Helm3Release: h3.Release{
+				Name: "Foo",
+				Info: &h3.Info{
+					Status:  h3.StatusDeployed,
+					Deleted: validDeletedTime,
+				},
+				Chart: &h3chart.Chart{
+					Metadata: &h3chart.Metadata{},
+					Values:   map[string]interface{}{},
+				},
+			},
+			Helm2Release: h2.Release{
+				Name: "Foo",
+				Info: &h2.Info{
+					Status: &h2.Status{
+						Code: h2.Status_DEPLOYED,
+					},
+					Deleted: &timestamp.Timestamp{Seconds: validSeconds},
+				},
+				Chart: &h2chart.Chart{
+					Metadata: &h2chart.Metadata{},
+					Values: &h2chart.Config{
+						Raw: "{}\n",
+					},
+				},
+				Config: &h2chart.Config{
+					Raw: "{}\n",
+				},
+			},
+		},
+		{
+			Description:         "returns an error if the deleted time cannot be parsed",
+			MarshallingFunction: asResponse,
+			Helm3Release: h3.Release{
+				Name: "Foo",
+				Info: &h3.Info{
+					Status:  h3.StatusDeployed,
+					Deleted: invalidDeletedTime,
+				},
+				Chart: &h3chart.Chart{
+					Metadata: &h3chart.Metadata{},
+				},
+			},
+			ExpectedError: ErrFailedToParseDeletionTime,
+		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.Description, func(t *testing.T) {
-			// Capture the panic and report it in an orderly fashion
-			defer func() {
-				if r := recover(); r != nil {
-					t.Errorf("Got a panic: %v. \nStacktrace: \n%s", r, string(debug.Stack()))
-				}
-			}()
 			// Perform conversion
 			compatibleH3rls, err := newDashboardCompatibleRelease(test.Helm3Release)
+			if got, want := err, test.ExpectedError; got != want {
+				t.Errorf("got: %v, want: %v", got, want)
+			}
 			if err != nil {
-				t.Fatalf("Unexpected error: %v", err)
+				return
 			}
 			// Marshall both: Compatible H3Release and H2Release
 			h3Marshalled := test.MarshallingFunction(compatibleH3rls)
@@ -135,7 +219,8 @@ func TestNewDashboardCompatibleRelease(t *testing.T) {
 			t.Logf("Marshalled Helm 2 Release %s", h2Marshalled)
 			// Check result
 			if h3Marshalled != h2Marshalled {
-				t.Errorf("Not equal: %s", cmp.Diff(h3Marshalled, h2Marshalled))
+				t.Errorf("Not equal:\nMarshalled diff: %s\nUnMarshalled diff: %s",
+					cmp.Diff(h3Marshalled, h2Marshalled), cmp.Diff(compatibleH3rls, test.Helm2Release))
 			}
 		})
 	}

cmd/kubeops/internal/handler/handler_test.go

@@ -6,6 +6,7 @@ import (
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
+	"sort"
 	"strings"
 	"testing"
 	"time"
@@ -312,6 +313,14 @@ func TestActions(t *testing.T) {
 			if err != nil {
 				t.Fatalf("%+v", err)
 			}
+			// The Helm memory driver does not appear to have consistent ordering.
+			// See https://github.com/helm/helm/issues/7263
+			// Just sort by "name.version.namespace" which is good enough here.
+			sort.Slice(releases, func(i, j int) bool {
+				iKey := fmt.Sprintf("%s.%d.%s", releases[i].Name, releases[i].Version, releases[i].Namespace)
+				jKey := fmt.Sprintf("%s.%d.%s", releases[j].Name, releases[j].Version, releases[j].Namespace)
+				return iKey < jKey
+			})
 			if !cmp.Equal(releases, test.RemainingReleases, releaseComparer) {
 				t.Errorf("Unexpected remaining releases. Diff:\n%s", cmp.Diff(releases, test.RemainingReleases, releaseComparer))
 			}