feat: deprecate enforcesemver2 config option (#522)

* feat: deprecate enforcesemver2 config option Signed-off-by: Casey Buto <[email protected]> * refactor: skip getting the chart version Signed-off-by: Casey Buto <[email protected]>
helm · Jan 25, 2022 · c08bf65 · c08bf65
1 parent 8ebb204
commit c08bf65
Show file tree

Hide file tree

Showing 8 changed files with 20 additions and 54 deletions.
diff --git a/.gitignore b/.gitignore
@@ -22,4 +22,5 @@ mirror/
 testbin/
 cmd/chartmuseum/debug
 vendor
-_dist/
+_dist/
+.DS_Store
diff --git a/acceptance_tests/lib/ChartMuseum.py b/acceptance_tests/lib/ChartMuseum.py
@@ -18,7 +18,7 @@ def http_status_code_should_be(self, expected_status, actual_status):
     def start_chartmuseum(self, storage):
         self.stop_chartmuseum()
         os.chdir(self.rootdir)
-        cmd = 'KILLME=1 chartmuseum --debug --enforce-semver2 --port=%d --storage="%s" ' % (common.PORT, storage)
+        cmd = 'KILLME=1 chartmuseum --debug --port=%d --storage="%s" ' % (common.PORT, storage)
         if storage == 'local':
             shutil.rmtree(common.STORAGE_DIR, ignore_errors=True)
             cmd += '--storage-local-rootdir=%s >> %s 2>&1' % (common.STORAGE_DIR, common.LOGFILE)
@@ -106,13 +106,7 @@ def upload_bad_test_charts(self):
                     print(('HTTP STATUS: %s' % response.status_code))
                     print(('HTTP CONTENT: %s' % response.content))
 
-                    # TODO: See comment in "uploadChartPackage" method in api.go
-                    # self.http_status_code_should_be(400, response.status_code)
-
-                    try:
-                        self.http_status_code_should_be(400, response.status_code)
-                    except AssertionError as e:
-                        self.http_status_code_should_be(500, response.status_code)
+                    self.http_status_code_should_be(400, response.status_code)
             os.chdir('../')
 
     def upload_provenance_files(self):

diff --git a/cmd/chartmuseum/main.go b/cmd/chartmuseum/main.go
@@ -18,13 +18,14 @@ package main
 
 import (
 	"fmt"
+	"log"
+	"os"
+	"strings"
+
 	"github.com/chartmuseum/storage"
 	"helm.sh/chartmuseum/pkg/cache"
 	"helm.sh/chartmuseum/pkg/chartmuseum"
 	"helm.sh/chartmuseum/pkg/config"
-	"log"
-	"os"
-	"strings"
 
 	"github.com/urfave/cli"
 )

diff --git a/go.mod b/go.mod
@@ -9,7 +9,6 @@ replace (
 )
 
 require (
-	github.com/Masterminds/semver/v3 v3.1.1
 	github.com/alicebob/miniredis v2.5.0+incompatible
 	github.com/chartmuseum/auth v0.5.0
 	github.com/chartmuseum/storage v0.11.0
@@ -39,6 +38,7 @@ require (
 	github.com/Azure/go-autorest/autorest/date v0.3.0 // indirect
 	github.com/Azure/go-autorest/logger v0.2.1 // indirect
 	github.com/Azure/go-autorest/tracing v0.6.0 // indirect
+	github.com/Masterminds/semver/v3 v3.1.1 // indirect
 	github.com/Microsoft/go-winio v0.4.17 // indirect
 	github.com/Microsoft/hcsshim v0.8.21 // indirect
 	github.com/NetEase-Object-Storage/nos-golang-sdk v0.0.0-00010101000000-000000000000 // indirect

diff --git a/pkg/chartmuseum/server.go b/pkg/chartmuseum/server.go
@@ -67,8 +67,7 @@ type (
 		CORSAllowOrigin        string
 		ReadTimeout            int
 		WriteTimeout           int
-		// EnforceSemver2 represents if the museum server always accept the Chart with [valid semantic version 2](https://semver.org/)
-		// More refers to : https://github.com/helm/chartmuseum/issues/320
+		// EnforceSemver was deprecated, see https://github.com/helm/chartmuseum/issues/485 for more info
 		EnforceSemver2 bool
 		CacheInterval  time.Duration
 		Host           string
@@ -139,9 +138,11 @@ func NewServer(options ServerOptions) (Server, error) {
 		UseStatefiles:          options.UseStatefiles,
 		AllowOverwrite:         options.AllowOverwrite,
 		AllowForceOverwrite:    options.AllowForceOverwrite,
-		EnforceSemver2:         options.EnforceSemver2,
 		Version:                options.Version,
 		CacheInterval:          options.CacheInterval,
+		// Deprecated options
+		// EnforceSemver2 - see https://github.com/helm/chartmuseum/issues/485 for more info
+		EnforceSemver2: options.EnforceSemver2,
 	})
 
 	return server, err

diff --git a/pkg/chartmuseum/server/multitenant/api.go b/pkg/chartmuseum/server/multitenant/api.go
@@ -22,8 +22,6 @@ import (
 	pathutil "path/filepath"
 	"sort"
 
-	"github.com/Masterminds/semver/v3"
-	"github.com/chartmuseum/storage"
 	cm_logger "helm.sh/chartmuseum/pkg/chartmuseum/logger"
 	cm_repo "helm.sh/chartmuseum/pkg/repo"
 
@@ -96,10 +94,11 @@ func (server *MultiTenantServer) deleteChartVersion(log cm_logger.LoggingFn, rep
 }
 
 func (server *MultiTenantServer) uploadChartPackage(log cm_logger.LoggingFn, repo string, content []byte, force bool) (string, *HTTPError) {
+	var filename string
+
 	filename, err := cm_repo.ChartPackageFilenameFromContent(content)
 	if err != nil {
-		// TODO: this is an error if invalid semver, making "EnforceSemver2" useless...
-		return filename, &HTTPError{http.StatusInternalServerError, err.Error()}
+		return filename, &HTTPError{http.StatusBadRequest, err.Error()}
 	}
 
 	if pathutil.Base(filename) != filename {
@@ -121,20 +120,6 @@ func (server *MultiTenantServer) uploadChartPackage(log cm_logger.LoggingFn, rep
 		// continue with the `overwrite` servers
 	}
 
-	if server.EnforceSemver2 {
-		version, err := cm_repo.ChartVersionFromStorageObject(storage.Object{
-			Content: content,
-			// Since we only need content to check for the chart version
-			// left the others fields to be default
-		})
-		if err != nil {
-			return filename, &HTTPError{http.StatusBadRequest, err.Error()}
-		}
-		if _, err := semver.StrictNewVersion(version.Metadata.Version); err != nil {
-			return filename, &HTTPError{http.StatusBadRequest, fmt.Errorf("semver2 validation: %w", err).Error()}
-		}
-	}
-
 	limitReached, err := server.checkStorageLimit(repo, filename, force)
 	if err != nil {
 		return filename, &HTTPError{http.StatusInternalServerError, err.Error()}

diff --git a/pkg/chartmuseum/server/multitenant/server_test.go b/pkg/chartmuseum/server/multitenant/server_test.go
@@ -353,7 +353,6 @@ func (suite *MultiTenantServerTestSuite) SetupSuite() {
 		EnableAPI:              true,
 		AllowOverwrite:         true,
 		ChartPostFormFieldName: "chart",
-		EnforceSemver2:         true,
 	})
 	suite.NotNil(server)
 	suite.Nil(err, "no error creating semantic version server")
@@ -701,18 +700,6 @@ func (suite *MultiTenantServerTestSuite) TestBadChartUpload() {
 	suite.Equal(400, res.Status(), "400 POST /api/charts")
 }
 
-func (suite *MultiTenantServerTestSuite) TestSemver2Validation() {
-	content, err := ioutil.ReadFile(badTestSemver2Path)
-	suite.Nil(err, "no error opening test path")
-	body := bytes.NewBuffer(content)
-	res := suite.doRequest("semver2", "POST", "/api/charts", body, "")
-
-	// TODO: See comment in "uploadChartPackage" method in api.go
-	// suite.Equal(400, res.Status(), "400 POST /api/charts bad semver validation")
-
-	suite.Equal(500, res.Status(), "500 POST /api/charts bad semver validation")
-}
-
 func (suite *MultiTenantServerTestSuite) TestForceOverwriteServer() {
 	// Clear test repo to allow uploading again
 	res := suite.doRequest("forceoverwrite", "DELETE", "/api/charts/mychart/0.1.0", nil, "")
@@ -984,7 +971,7 @@ func (suite *MultiTenantServerTestSuite) testAllRoutes(repo string, depth int) {
 	// POST /api/:repo/charts
 	body := bytes.NewBuffer([]byte{})
 	res = suite.doRequest(stype, "POST", fmt.Sprintf("%s/charts", apiPrefix), body, "")
-	suite.Equal(500, res.Status(), fmt.Sprintf("500 POST %s/charts", apiPrefix))
+	suite.Equal(400, res.Status(), fmt.Sprintf("400 POST %s/charts", apiPrefix))
 
 	// POST /api/:repo/prov
 	body = bytes.NewBuffer([]byte{})
@@ -1008,16 +995,13 @@ func (suite *MultiTenantServerTestSuite) testAllRoutes(repo string, depth int) {
 	res = suite.doRequest(stype, "POST", fmt.Sprintf("%s/charts?force", apiPrefix), body, "")
 	suite.Equal(409, res.Status(), fmt.Sprintf("409 POST %s/charts?force", apiPrefix))
 
-	// with bad semver but without enforce semver2
+	// with bad semver
 	content, err = ioutil.ReadFile(badTestSemver2Path)
 	suite.Nil(err, "no error opening bad semver2 path")
 
 	body = bytes.NewBuffer(content)
 	res = suite.doRequest(stype, "POST", fmt.Sprintf("%s/charts", apiPrefix), body, "")
-
-	// TODO: See comment in "uploadChartPackage" method in api.go
-	// suite.Equal(201, res.Status(), fmt.Sprintf("201 POST %s/charts", apiPrefix))
-	suite.Equal(500, res.Status(), fmt.Sprintf("500 POST %s/charts", apiPrefix))
+	suite.Equal(400, res.Status(), fmt.Sprintf("400 POST %s/charts", apiPrefix))
 
 	// POST /api/:repo/prov
 	content, err = ioutil.ReadFile(testProvfilePath)

diff --git a/pkg/config/vars.go b/pkg/config/vars.go
@@ -753,7 +753,7 @@ var configVars = map[string]configVar{
 		Default: false,
 		CLIFlag: cli.BoolFlag{
 			Name:   "enforce-semver2",
-			Usage:  "enforce the chart museum server only accepts the valid chart version as Helm does",
+			Usage:  "(deprecated) enforce the chart museum server only accepts the valid chart version as Helm does",
 			EnvVar: "ENFORCE_SEMVER2",
 		},
 	},