pkg/repo,pkg/chartmuseum/server: per-limit-chart chart expiration removes prefix mathching for chartpath preparing (#721)

Fix #714

Signed-off-by: scbizu <scbizu@gmail.com>

pkg/chartmuseum/server/multitenant/api.go

@@ -230,7 +230,8 @@ func extractFromChart(content []byte) (name string, version string, err error) {
 }
 
 func (server *MultiTenantServer) PutWithLimit(ctx *gin.Context, log cm_logger.LoggingFn, repo string,
-	filename string, content []byte) error {
+	filename string, content []byte,
+) error {
 	if server.ChartLimits == nil {
 		log(cm_logger.DebugLevel, "PutWithLimit: per-chart-limit not set")
 		return server.StorageBackend.PutObject(pathutil.Join(repo, filename), content)
@@ -251,7 +252,8 @@ func (server *MultiTenantServer) PutWithLimit(ctx *gin.Context, log cm_logger.Lo
 	}
 	var newObjs []storage.Object
 	for _, obj := range objs {
-		if !strings.HasPrefix(obj.Path, name) || strings.HasSuffix(obj.Path, ".prov") {
+		n, _ := cm_repo.GetExactChartNameVersion(obj.Path)
+		if strings.Compare(n, name) != 0 || strings.HasSuffix(obj.Path, ".prov") {
 			continue
 		}
 		log(cm_logger.DebugLevel, "PutWithLimit", "current object name", obj.Path)

pkg/chartmuseum/server/multitenant/server_test.go

@@ -42,14 +42,17 @@ import (
 var maxUploadSize = 1024 * 1024 * 20
 
 // These are generated from scripts/setup-test-environment.sh
-var testTarballPath = "../../../../testdata/charts/mychart/mychart-0.1.0.tgz"
-var testTarballPathV2 = "../../../../testdata/charts/mychart/mychart-0.2.0.tgz"
-var testTarballPathV0 = "../../../../testdata/charts/mychart/mychart-0.0.1.tgz"
-var testProvfilePath = "../../../../testdata/charts/mychart/mychart-0.1.0.tgz.prov"
-var otherTestTarballPath = "../../../../testdata/charts/otherchart/otherchart-0.1.0.tgz"
-var otherTestProvfilePath = "../../../../testdata/charts/otherchart/otherchart-0.1.0.tgz.prov"
-var badTestTarballPath = "../../../../testdata/badcharts/mybadchart/mybadchart-1.0.0.tgz"
-var badTestProvfilePath = "../../../../testdata/badcharts/mybadchart/mybadchart-1.0.0.tgz.prov"
+var (
+	testTarballPath          = "../../../../testdata/charts/mychart/mychart-0.1.0.tgz"
+	testTarballPathV2        = "../../../../testdata/charts/mychart/mychart-0.2.0.tgz"
+	testTarballPathV0        = "../../../../testdata/charts/mychart/mychart-0.0.1.tgz"
+	testServiceTarballPathV0 = "../../../../testdata/charts/mychart-service/mychart-service-0.0.1.tgz"
+	testProvfilePath         = "../../../../testdata/charts/mychart/mychart-0.1.0.tgz.prov"
+	otherTestTarballPath     = "../../../../testdata/charts/otherchart/otherchart-0.1.0.tgz"
+	otherTestProvfilePath    = "../../../../testdata/charts/otherchart/otherchart-0.1.0.tgz.prov"
+	badTestTarballPath       = "../../../../testdata/badcharts/mybadchart/mybadchart-1.0.0.tgz"
+	badTestProvfilePath      = "../../../../testdata/badcharts/mybadchart/mybadchart-1.0.0.tgz.prov"
+)
 
 type MultiTenantServerTestSuite struct {
 	suite.Suite
@@ -676,7 +679,7 @@ entries:
     - charts/acs-engine-autoscaler-2.1.2.tgz
     version: 2.1.2
 generated: "2018-05-23T15:14:46-05:00"`)
-	err = os.WriteFile(indexCacheFilePath, content, 0644)
+	err = os.WriteFile(indexCacheFilePath, content, 0o644)
 	suite.Nil(err, "no error creating test index-cache.yaml")
 	defer os.Remove(indexCacheFilePath)
 
@@ -699,7 +702,7 @@ generated: "2018-05-23T15:14:46-05:00"`)
 	// invalid, unparsable index-cache.yaml
 	indexCacheFilePath = pathutil.Join(suite.TempDirectory, repo.StatefileFilename)
 	content = []byte(`is this valid yaml? maybe. but its definitely not a valid index.yaml!`)
-	err = os.WriteFile(indexCacheFilePath, content, 0644)
+	err = os.WriteFile(indexCacheFilePath, content, 0o644)
 	suite.Nil(err, "no error creating test index-cache.yaml")
 
 	NewMultiTenantServer(MultiTenantServerOptions{
@@ -917,27 +920,25 @@ func (suite *MultiTenantServerTestSuite) TestMaxObjectsServer() {
 
 func (suite *MultiTenantServerTestSuite) TestPerChartLimit() {
 	ns := "per-chart-limit"
-	content, err := os.ReadFile(testTarballPathV0)
-	suite.Nil(err, "no error opening test tarball")
-	body := bytes.NewBuffer(content)
-	res := suite.doRequest(ns, "POST", "/api/charts", body, "")
-	suite.Equal(201, res.Status(), "201 POST /api/charts")
 
-	content, err = os.ReadFile(testTarballPathV2)
-	suite.Nil(err, "no error opening test tarball")
-	body = bytes.NewBuffer(content)
-	res = suite.doRequest(ns, "POST", "/api/charts", body, "")
-	suite.Equal(201, res.Status(), "201 POST /api/charts")
+	expectUploadFiles := []string{
+		testServiceTarballPathV0,
+		testTarballPathV0,
+		testTarballPathV2,
+		testTarballPath,
+	}
 
-	content, err = os.ReadFile(testTarballPath)
-	suite.Nil(err, "no error opening test tarball")
-	body = bytes.NewBuffer(content)
-	res = suite.doRequest(ns, "POST", "/api/charts", body, "")
-	suite.Equal(201, res.Status(), "201 POST /api/charts")
+	for _, f := range expectUploadFiles {
+		content, err := os.ReadFile(f)
+		suite.Nil(err, "no error opening test tarball")
+		body := bytes.NewBuffer(content)
+		res := suite.doRequest(ns, "POST", "/api/charts", body, "")
+		suite.Equal(201, res.Status(), "201 POST /api/charts")
+	}
 
 	time.Sleep(time.Second)
 
-	res = suite.doRequest(ns, "GET", "/api/charts/mychart/0.2.0", nil, "")
+	res := suite.doRequest(ns, "GET", "/api/charts/mychart/0.2.0", nil, "")
 	suite.Equal(200, res.Status(), "200 GET /api/charts/mychart-0.2.0")
 
 	res = suite.doRequest(ns, "GET", "/api/charts/mychart/0.1.0", nil, "")
@@ -945,6 +946,9 @@ func (suite *MultiTenantServerTestSuite) TestPerChartLimit() {
 
 	res = suite.doRequest(ns, "GET", "/api/charts/mychart/0.0.1", nil, "")
 	suite.Equal(404, res.Status(), "200 GET /api/charts/mychart-0.0.1")
+
+	res = suite.doRequest(ns, "GET", "/api/charts/mychart-service/0.0.1", nil, "")
+	suite.Equal(200, res.Status(), "200 GET /api/charts/mychart-service-0.0.1")
 }
 
 func (suite *MultiTenantServerTestSuite) TestMaxUploadSizeServer() {
@@ -967,7 +971,6 @@ func (suite *MultiTenantServerTestSuite) TestMaxUploadSizeServer() {
 }
 
 func (suite *MultiTenantServerTestSuite) TestMetrics() {
-
 	apiPrefix := pathutil.Join("/api", "a")
 
 	content, err := os.ReadFile(testTarballPath)
@@ -1300,7 +1303,6 @@ func (suite *MultiTenantServerTestSuite) testAllRoutes(repo string, depth int) {
 	buf, w = suite.getBodyWithMultipartFormFiles([]string{"prov"}, []string{testTarballPath})
 	res = suite.doRequest(stype, "POST", fmt.Sprintf("%s/charts", apiPrefix), buf, w.FormDataContentType())
 	suite.Equal(400, res.Status(), fmt.Sprintf("400 POST %s/charts", apiPrefix))
-
 }
 
 func (suite *MultiTenantServerTestSuite) getBodyWithMultipartFormFiles(fields []string, filenames []string) (io.Reader, *multipart.Writer) {

pkg/repo/chart.go

@@ -113,23 +113,29 @@ func chartFromContent(content []byte) (*helm_chart.Chart, error) {
 
 func emptyChartVersionFromPackageFilename(filename string) *helm_repo.ChartVersion {
 	noExt := strings.TrimSuffix(pathutil.Base(filename), fmt.Sprintf(".%s", ChartPackageFileExtension))
-	parts := strings.Split(noExt, "-")
-	lastIndex := len(parts) - 1
-	name := parts[0]
-	version := ""
 
-	for idx := lastIndex; idx >= 1; idx-- {
-		if _, err := strconv.Atoi(string(parts[idx][0])); err == nil { // see if this part looks like a version (starts w int)
-			version = strings.Join(parts[idx:], "-")
-			name = strings.Join(parts[:idx], "-")
-			break
-		}
-	}
-	if version == "" { // no parts looked like a real version, just take everything after last hyphen
-		name = strings.Join(parts[:lastIndex], "-")
-		version = parts[lastIndex]
-	}
+	name, version := GetExactChartNameVersion(noExt)
 
 	metadata := &helm_chart.Metadata{Name: name, Version: version}
 	return &helm_repo.ChartVersion{Metadata: metadata}
 }
+
+func GetExactChartNameVersion(path string) (string, string) {
+	parts := strings.Split(path, "-")
+	lastIndex := len(parts) - 1
+	chartName := parts[0]
+	chartVersion := ""
+
+	for idx := lastIndex; idx >= 1; idx-- {
+		if _, err := strconv.Atoi(string(parts[idx][0])); err == nil { // see if this part looks like a version (starts w int)
+			chartVersion = strings.Join(parts[idx:], "-")
+			chartName = strings.Join(parts[:idx], "-")
+			break
+		}
+	}
+	if chartVersion == "" { // no parts looked like a real version, just take everything after last hyphen
+		chartName = strings.Join(parts[:lastIndex], "-")
+		chartVersion = parts[lastIndex]
+	}
+	return chartName, chartVersion
+}

scripts/setup-test-environment.sh

@@ -51,6 +51,8 @@ package_test_charts() {
     helm package --sign --key helm-test --keyring ../pgp/helm-test-key.secret --version 0.2.0 -d mychart/ mychart/.
     # add another version for per chart limit test
     helm package --sign --key helm-test --keyring ../pgp/helm-test-key.secret --version 0.0.1 -d mychart/ mychart/.
+
+    helm package --sign --key helm-test --keyring ../pgp/helm-test-key.secret --version 0.0.1 -d mychart-service/ mychart-service/.
     popd
 
     pushd testdata/badcharts/

testdata/charts/mychart-service/.helmignore

@@ -0,0 +1,2 @@
+*.tgz
+*.prov

testdata/charts/mychart-service/Chart.yaml

@@ -0,0 +1,2 @@
+name: mychart-service
+version: 0.1.0

testdata/charts/mychart-service/templates/pod.yaml

@@ -0,0 +1,9 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: '{{- printf "%s-%s" .Release.Name .Chart.Name | trunc 63 | trimSuffix "-" -}}'
+spec:
+  containers:
+  - image: busybox
+    name: '{{ .Chart.Name }}'
+    command: ['/bin/sh', '-c', 'while true; do echo {{ .Release.Name }}; sleep 5; done']