From 90c2e837515cf94a6291bfb96e8bcc71771de63d Mon Sep 17 00:00:00 2001 From: Michael Nelson Date: Tue, 14 Jan 2020 06:47:58 +0000 Subject: [PATCH] Test and remove panic possibilities in newDashboardCompatibleRelease --- .../internal/handler/dashboardcompat.go | 12 +- .../internal/handler/dashboardcompat_test.go | 103 ++++++++++++++++-- cmd/kubeops/internal/handler/handler_test.go | 9 ++ 3 files changed, 114 insertions(+), 10 deletions(-) diff --git a/cmd/kubeops/internal/handler/dashboardcompat.go b/cmd/kubeops/internal/handler/dashboardcompat.go index d69a578a285..a1c34ba545d 100644 --- a/cmd/kubeops/internal/handler/dashboardcompat.go +++ b/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{ diff --git a/cmd/kubeops/internal/handler/dashboardcompat_test.go b/cmd/kubeops/internal/handler/dashboardcompat_test.go index b931d6d0d69..bbc299ab52e 100644 --- a/cmd/kubeops/internal/handler/dashboardcompat_test.go +++ b/cmd/kubeops/internal/handler/dashboardcompat_test.go @@ -2,12 +2,13 @@ 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" @@ -15,6 +16,15 @@ import ( ) 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: ×tamp.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)) } }) } diff --git a/cmd/kubeops/internal/handler/handler_test.go b/cmd/kubeops/internal/handler/handler_test.go index a2ad14252c7..43d90092521 100644 --- a/cmd/kubeops/internal/handler/handler_test.go +++ b/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)) }