diff --git a/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server.go b/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server.go index 9880d91c7af..f71bf9fdac9 100644 --- a/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server.go +++ b/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server.go @@ -329,7 +329,14 @@ func (s *Server) GetAvailablePackageDetail(ctx context.Context, request *connect return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("Chart returned without any versions: %+v", chart)) } if version == "" { - version = chart.ChartVersions[0].Version + sortedVersions, err := pkgutils.SortByPackageVersion(chart.ChartVersions) + if err != nil { + // If there was an error parsing a version as semver, fall back to ChartVersions[0] + log.Errorf("error parsing versions as semver: %w", err) + version = chart.ChartVersions[0].Version + } else { + version = sortedVersions[0].Version.String() + } } fileID := fileIDForChart(unescapedChartID, version) chartFiles, err := s.manager.GetChartFiles(namespace, fileID) @@ -419,11 +426,22 @@ func AvailablePackageDetailFromChart(chart *models.Chart, chartFiles *models.Cha pkg.AvailablePackageRef.Context = &corev1.Context{Namespace: chart.Repo.Namespace} } - // We assume that chart.ChartVersions[0] will always contain either: the latest version or the specified version + // We assume that sortedVersions[0] will always contain either: the latest version or the specified version if chart.ChartVersions != nil || len(chart.ChartVersions) != 0 { - pkg.Version = &corev1.PackageAppVersion{ - PkgVersion: chart.ChartVersions[0].Version, - AppVersion: chart.ChartVersions[0].AppVersion, + sortedVersions, err := pkgutils.SortByPackageVersion(chart.ChartVersions) + if err != nil { + // If there was an error parsing a version as semver, fall back to ChartVersions[0] + log.Errorf("error parsing versions as semver: %w", err) + version = chart.ChartVersions[0].Version + pkg.Version = &corev1.PackageAppVersion{ + PkgVersion: chart.ChartVersions[0].Version, + AppVersion: chart.ChartVersions[0].AppVersion, + } + } else { + pkg.Version = &corev1.PackageAppVersion{ + PkgVersion: sortedVersions[0].Version.String(), + AppVersion: sortedVersions[0].AppVersion, + } } pkg.Readme = chartFiles.Readme pkg.DefaultValues = chartFiles.DefaultValues @@ -523,10 +541,21 @@ func (s *Server) GetInstalledPackageSummaries(ctx context.Context, request *conn // TODO(agamez): deal with multiple matches, perhaps returning []AvailablePackageRef ? // Example: global + namespaced repo including an overlapping subset of packages. if len(charts) > 0 && len(charts[0].ChartVersions) > 0 { - installedPkgSummaries[i].LatestVersion = &corev1.PackageAppVersion{ - PkgVersion: charts[0].ChartVersions[0].Version, - AppVersion: charts[0].ChartVersions[0].AppVersion, + sortedVersions, err := pkgutils.SortByPackageVersion(charts[0].ChartVersions) + if err != nil { + // If there was an error parsing a version as semver, fall back to ChartVersions[0] + log.Errorf("error parsing versions as semver: %w", err) + installedPkgSummaries[i].LatestVersion = &corev1.PackageAppVersion{ + PkgVersion: charts[0].ChartVersions[0].Version, + AppVersion: charts[0].ChartVersions[0].AppVersion, + } + } else { + installedPkgSummaries[i].LatestVersion = &corev1.PackageAppVersion{ + PkgVersion: sortedVersions[0].Version.String(), + AppVersion: sortedVersions[0].AppVersion, + } } + } installedPkgSummaries[i].Status = &corev1.InstalledPackageStatus{ Ready: rel.Info.Status == release.StatusDeployed, @@ -650,11 +679,21 @@ func (s *Server) GetInstalledPackageDetail(ctx context.Context, request *connect } } if len(charts[0].ChartVersions) > 0 { - cv := charts[0].ChartVersions[0] - installedPkgDetail.LatestVersion = &corev1.PackageAppVersion{ - PkgVersion: cv.Version, - AppVersion: cv.AppVersion, + sortedVersions, err := pkgutils.SortByPackageVersion(charts[0].ChartVersions) + if err != nil { + // If there was an error parsing a version as semver, fall back to ChartVersions[0] + log.Errorf("error parsing versions as semver: %w", err) + installedPkgDetail.LatestVersion = &corev1.PackageAppVersion{ + PkgVersion: charts[0].ChartVersions[0].Version, + AppVersion: charts[0].ChartVersions[0].AppVersion, + } + } else { + installedPkgDetail.LatestVersion = &corev1.PackageAppVersion{ + PkgVersion: sortedVersions[0].Version.String(), + AppVersion: sortedVersions[0].AppVersion, + } } + } } @@ -901,10 +940,9 @@ func (s *Server) fetchChartWithRegistrySecrets(ctx context.Context, headers http return nil, nil, connect.NewError(connect.CodeInternal, fmt.Errorf("Unable to fetch the chart %s (version %s) from the namespace %q: %w", chartID, chartDetails.Version, chartDetails.AppRepositoryResourceNamespace, err)) } var tarballURL string + // If the chart is cached, we can use the tarball URL from the cache, + // we assume cachedChart.ChartVersions only contains 1 element if cachedChart.ChartVersions != nil && len(cachedChart.ChartVersions) == 1 && cachedChart.ChartVersions[0].URLs != nil { - // The tarball URL will always be the first URL in the repo.chartVersions: - // https://helm.sh/docs/topics/chart_repository/#the-index-file - // /~https://github.com/helm/helm/blob/v3.7.1/cmd/helm/search/search_test.go#L63 tarballURL = chartTarballURL(cachedChart.Repo, cachedChart.ChartVersions[0]) log.InfoS("using chart tarball", "url", tarballURL) } @@ -935,6 +973,9 @@ func (s *Server) fetchChartWithRegistrySecrets(ctx context.Context, headers http } func chartTarballURL(r *models.Repo, cv models.ChartVersion) string { + // The tarball URL will always be the first URL, ie. URL[0], in the repo.chartVersions[i]: + // https://helm.sh/docs/topics/chart_repository/#the-index-file + // /~https://github.com/helm/helm/blob/v3.7.1/cmd/helm/search/search_test.go#L63 source := strings.TrimSpace(cv.URLs[0]) parsedUrl, err := url.ParseRequestURI(source) if err != nil || parsedUrl.Scheme == "" { diff --git a/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server_test.go b/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server_test.go index 7be4658afd3..83ea1dff5ac 100644 --- a/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server_test.go +++ b/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server_test.go @@ -410,9 +410,9 @@ func TestGetAvailablePackageSummaries(t *testing.T) { }, expectDBQueryNamespace: globalPackagingNamespace, charts: []*models.Chart{ - makeChart("chart-1", "repo-1", "http://chart-1", "my-ns", []string{"3.0.0"}, DefaultChartCategory), - makeChart("chart-2", "repo-1", "http://chart-2", "my-ns", []string{"2.0.0"}, DefaultChartCategory), - makeChart("chart-3-global", "repo-1", "http://chart-3", globalPackagingNamespace, []string{"2.0.0"}, DefaultChartCategory), + makeChart("chart-1", "repo-1", "http://chart-1", "my-ns", []string{"2.0.0", "3.0.0"}, DefaultChartCategory), + makeChart("chart-2", "repo-1", "http://chart-2", "my-ns", []string{"1.0.0", "2.0.0"}, DefaultChartCategory), + makeChart("chart-3-global", "repo-1", "http://chart-3", globalPackagingNamespace, []string{"1.0.0", "2.0.0"}, DefaultChartCategory), }, expectedResponse: &corev1.GetAvailablePackageSummariesResponse{ AvailablePackageSummaries: []*corev1.AvailablePackageSummary{ @@ -478,8 +478,8 @@ func TestGetAvailablePackageSummaries(t *testing.T) { }, expectDBQueryNamespace: "my-ns", charts: []*models.Chart{ - makeChart("chart-1", "repo-1", "http://chart-1", "my-ns", []string{"3.0.0"}, DefaultChartCategory), - makeChart("chart-2", "repo-1", "http://chart-2", "my-ns", []string{"2.0.0"}, DefaultChartCategory), + makeChart("chart-1", "repo-1", "http://chart-1", "my-ns", []string{"2.0.0", "3.0.0"}, DefaultChartCategory), + makeChart("chart-2", "repo-1", "http://chart-2", "my-ns", []string{"1.0.0", "2.0.0"}, DefaultChartCategory), }, expectedResponse: &corev1.GetAvailablePackageSummariesResponse{ AvailablePackageSummaries: []*corev1.AvailablePackageSummary{ @@ -530,8 +530,8 @@ func TestGetAvailablePackageSummaries(t *testing.T) { }, expectDBQueryNamespace: globalPackagingNamespace, charts: []*models.Chart{ - makeChart("chart-1", "repo-1", "http://chart-1", "my-ns", []string{"3.0.0"}, DefaultChartCategory), - makeChart("chart-2", "repo-1", "http://chart-2", "my-ns", []string{"2.0.0"}, DefaultChartCategory), + makeChart("chart-1", "repo-1", "http://chart-1", "my-ns", []string{"2.0.0", "3.0.0"}, DefaultChartCategory), + makeChart("chart-2", "repo-1", "http://chart-2", "my-ns", []string{"1.0.0", "2.0.0"}, DefaultChartCategory), }, expectedResponse: &corev1.GetAvailablePackageSummariesResponse{ AvailablePackageSummaries: []*corev1.AvailablePackageSummary{ @@ -621,8 +621,8 @@ func TestGetAvailablePackageSummaries(t *testing.T) { }, expectDBQueryNamespace: globalPackagingNamespace, charts: []*models.Chart{ - makeChart("chart-1", "repo-1", "http://chart-1", "my-ns", []string{"3.0.0"}, DefaultChartCategory), - makeChart("chart-2", "repo-1", "http://chart-2", "my-ns", []string{"2.0.0"}, DefaultChartCategory), + makeChart("chart-1", "repo-1", "http://chart-1", "my-ns", []string{"2.0.0", "3.0.0"}, DefaultChartCategory), + makeChart("chart-2", "repo-1", "http://chart-2", "my-ns", []string{"1.0.0", "2.0.0"}, DefaultChartCategory), makeChart("chart-3", "repo-1", "http://chart-3", "my-ns", []string{"1.0.0"}, DefaultChartCategory), }, expectedResponse: &corev1.GetAvailablePackageSummariesResponse{ @@ -665,8 +665,8 @@ func TestGetAvailablePackageSummaries(t *testing.T) { }, expectDBQueryNamespace: globalPackagingNamespace, charts: []*models.Chart{ - makeChart("chart-1", "repo-1", "http://chart-1", "my-ns", []string{"3.0.0"}, DefaultChartCategory), - makeChart("chart-2", "repo-1", "http://chart-2", "my-ns", []string{"2.0.0"}, DefaultChartCategory), + makeChart("chart-1", "repo-1", "http://chart-1", "my-ns", []string{"2.0.0", "3.0.0"}, DefaultChartCategory), + makeChart("chart-2", "repo-1", "http://chart-2", "my-ns", []string{"1.0.0", "2.0.0"}, DefaultChartCategory), makeChart("chart-3", "repo-1", "http://chart-3", "my-ns", []string{"1.0.0"}, DefaultChartCategory), }, expectedResponse: &corev1.GetAvailablePackageSummariesResponse{ @@ -718,8 +718,8 @@ func TestGetAvailablePackageSummaries(t *testing.T) { }, expectDBQueryNamespace: "my-ns", charts: []*models.Chart{ - makeChart("chart-1", "repo-1", "http://chart-1", "my-ns", []string{"3.0.0"}, "foo"), - makeChart("chart-2", "repo-1", "http://chart-2", "my-ns", []string{"2.0.0"}, "bar"), + makeChart("chart-1", "repo-1", "http://chart-1", "my-ns", []string{"2.0.0", "3.0.0"}, "foo"), + makeChart("chart-2", "repo-1", "http://chart-2", "my-ns", []string{"1.0.0", "2.0.0"}, "bar"), makeChart("chart-3", "repo-1", "http://chart-3", "my-ns", []string{"1.0.0"}, "bar"), }, expectedResponse: &corev1.GetAvailablePackageSummariesResponse{ @@ -852,7 +852,7 @@ func TestAvailablePackageDetailFromChart(t *testing.T) { }{ { name: "it returns AvailablePackageDetail if the chart is correct", - chart: makeChart("foo", "repo-1", "http://foo", "my-ns", []string{"3.0.0"}, DefaultChartCategory), + chart: makeChart("foo", "repo-1", "http://foo", "my-ns", []string{"2.0.0", "3.0.0"}, DefaultChartCategory), chartFiles: &models.ChartFiles{ Readme: "chart readme", DefaultValues: "chart values", @@ -885,7 +885,7 @@ func TestAvailablePackageDetailFromChart(t *testing.T) { }, { name: "it includes additional values files in AvailablePackageDetail when available", - chart: makeChart("foo", "repo-1", "http://foo", "my-ns", []string{"3.0.0"}, DefaultChartCategory), + chart: makeChart("foo", "repo-1", "http://foo", "my-ns", []string{"2.0.0", "3.0.0"}, DefaultChartCategory), chartFiles: &models.ChartFiles{ Readme: "chart readme", DefaultValues: "chart values", @@ -972,7 +972,7 @@ func TestGetAvailablePackageDetail(t *testing.T) { Identifier: "repo-1%2Ffoo", }, }, - charts: []*models.Chart{makeChart("foo", "repo-1", "http://foo", "my-ns", []string{"3.0.0"}, DefaultChartCategory)}, + charts: []*models.Chart{makeChart("foo", "repo-1", "http://foo", "my-ns", []string{"2.0.0", "3.0.0"}, DefaultChartCategory)}, expectedPackage: &corev1.AvailablePackageDetail{ Name: "foo", DisplayName: "foo", @@ -1007,7 +1007,7 @@ func TestGetAvailablePackageDetail(t *testing.T) { }, PkgVersion: "1.0.0", }, - charts: []*models.Chart{makeChart("foo", "repo-1", "http://foo", "my-ns", []string{"3.0.0", "2.0.0", "1.0.0"}, DefaultChartCategory)}, + charts: []*models.Chart{makeChart("foo", "repo-1", "http://foo", "my-ns", []string{"1.0.0", "2.0.0", "3.0.0"}, DefaultChartCategory)}, expectedPackage: &corev1.AvailablePackageDetail{ Name: "foo", DisplayName: "foo", @@ -1204,7 +1204,7 @@ func TestGetAvailablePackageVersions(t *testing.T) { }, { name: "it returns the package version summary", - charts: []*models.Chart{makeChart("apache", "bitnami", "http://apache", "kubeapps", []string{"3.0.0", "2.0.0", "1.0.0"}, DefaultChartCategory)}, + charts: []*models.Chart{makeChart("apache", "bitnami", "http://apache", "kubeapps", []string{"2.0.0", "3.0.0", "1.0.0"}, DefaultChartCategory)}, request: &corev1.GetAvailablePackageVersionsRequest{ AvailablePackageRef: &corev1.AvailablePackageReference{ Context: &corev1.Context{ diff --git a/cmd/kubeapps-apis/plugins/pkg/pkgutils/pkgutils.go b/cmd/kubeapps-apis/plugins/pkg/pkgutils/pkgutils.go index 3121f8e793b..d3a9ce6f446 100644 --- a/cmd/kubeapps-apis/plugins/pkg/pkgutils/pkgutils.go +++ b/cmd/kubeapps-apis/plugins/pkg/pkgutils/pkgutils.go @@ -54,13 +54,13 @@ func GetDefaultVersionsInSummary() VersionsInSummary { return defaultVersionsInSummary } -type packageSemVersion struct { +type PackageSemVersion struct { *semver.Version - appVersion string + AppVersion string } -func sortByPackageVersion(versions []models.ChartVersion) ([]*packageSemVersion, error) { - var sortedVersions []*packageSemVersion +func SortByPackageVersion(versions []models.ChartVersion) ([]*PackageSemVersion, error) { + var sortedVersions []*PackageSemVersion for _, v := range versions { version, err := semver.NewVersion(v.Version) if err != nil { @@ -70,9 +70,9 @@ func sortByPackageVersion(versions []models.ChartVersion) ([]*packageSemVersion, return nil, fmt.Errorf("Chart version %q is not semver", v.Version) } - sortedVersions = append(sortedVersions, &packageSemVersion{ + sortedVersions = append(sortedVersions, &PackageSemVersion{ Version: version, - appVersion: v.AppVersion, + AppVersion: v.AppVersion, }) } sort.Slice(sortedVersions, func(i, j int) bool { @@ -87,7 +87,7 @@ func PackageAppVersionsSummary(versions []models.ChartVersion, versionInSummary var pav []*corev1.PackageAppVersion // Sort versions - sortedVersions, err := sortByPackageVersion(versions) + sortedVersions, err := SortByPackageVersion(versions) if err != nil { // If there was an error parsing a version as semver, we log the error // and simply return the versions, as Helm does. @@ -127,7 +127,7 @@ func PackageAppVersionsSummary(versions []models.ChartVersion, versionInSummary // Include the version and update the version map. pav = append(pav, &corev1.PackageAppVersion{ PkgVersion: version.Version.String(), - AppVersion: version.appVersion, + AppVersion: version.AppVersion, }) if _, ok := versionMap[version.Major()]; !ok { @@ -154,7 +154,7 @@ func IsValidChart(chart *models.Chart) (bool, error) { return false, connect.NewError(connect.CodeInternal, fmt.Errorf("required field .Repo not found on helm chart: %v", chart)) } if chart.ChartVersions == nil || len(chart.ChartVersions) == 0 { - return false, connect.NewError(connect.CodeInternal, fmt.Errorf("required field .chart.ChartVersions[0] not found on helm chart: %v", chart)) + return false, connect.NewError(connect.CodeInternal, fmt.Errorf("required field .chart.ChartVersions not found on helm chart or is empty: %v", chart)) } else { for _, chartVersion := range chart.ChartVersions { if chartVersion.Version == "" { @@ -197,15 +197,26 @@ func AvailablePackageSummaryFromChart(chart *models.Chart, plugin *plugins.Plugi pkg.AvailablePackageRef.Context = &corev1.Context{Namespace: chart.Repo.Namespace} if chart.ChartVersions != nil || len(chart.ChartVersions) != 0 { - pkg.LatestVersion = &corev1.PackageAppVersion{ - PkgVersion: chart.ChartVersions[0].Version, - AppVersion: chart.ChartVersions[0].AppVersion, + sortedVersions, err := SortByPackageVersion(chart.ChartVersions) + if err != nil { + // If there was an error parsing a version as semver, fall back to ChartVersions[0] + log.Errorf("error parsing versions as semver: %w", err) + pkg.LatestVersion = &corev1.PackageAppVersion{ + PkgVersion: chart.ChartVersions[0].Version, + AppVersion: chart.ChartVersions[0].AppVersion, + } + } else { + pkg.LatestVersion = &corev1.PackageAppVersion{ + PkgVersion: sortedVersions[0].Version.String(), + AppVersion: sortedVersions[0].AppVersion, + } } } return pkg, nil } +// TODO(agamez): I have replaced chart.ChartVersions[0] with SortByPackageVersion(...)[0] // TODO @gfichtenholt: I really wanted to put helm plugin's implementation of AvailablePackageDetailFromChart() // here, and use it in flux plugin as well. But I found out a couple of flaws in the implementation and decided // against it. Namely: diff --git a/cmd/kubeapps-apis/plugins/pkg/pkgutils/pkgutils_test.go b/cmd/kubeapps-apis/plugins/pkg/pkgutils/pkgutils_test.go index a211d6df5a7..5682bc05eb0 100644 --- a/cmd/kubeapps-apis/plugins/pkg/pkgutils/pkgutils_test.go +++ b/cmd/kubeapps-apis/plugins/pkg/pkgutils/pkgutils_test.go @@ -542,9 +542,9 @@ func TestAvailablePackageSummaryFromChart(t *testing.T) { }, Maintainers: []chart.Maintainer{{Name: "me", Email: "me@me.me"}}, ChartVersions: []models.ChartVersion{ - {Version: "3.0.0", AppVersion: DefaultAppVersion, Readme: "chart readme", DefaultValues: "chart values", Schema: "chart schema"}, - {Version: "2.0.0", AppVersion: DefaultAppVersion, Readme: "chart readme", DefaultValues: "chart values", Schema: "chart schema"}, {Version: "1.0.0", AppVersion: DefaultAppVersion, Readme: "chart readme", DefaultValues: "chart values", Schema: "chart schema"}, + {Version: "2.0.0", AppVersion: DefaultAppVersion, Readme: "chart readme", DefaultValues: "chart values", Schema: "chart schema"}, + {Version: "3.0.0", AppVersion: DefaultAppVersion, Readme: "chart readme", DefaultValues: "chart values", Schema: "chart schema"}, }, }, expected: &corev1.AvailablePackageSummary{