harbor/make/photon/chartserver/454.patch

From 66cc2635880193ffb1226e3c790b36eef24cfd8b Mon Sep 17 00:00:00 2001
From: scnace <scbizu@gmail.com>
Date: Mon, 3 May 2021 15:09:44 +0800
Subject: [PATCH 1/2] pkg/chartmuseum/server: add tests for cover duplicate
 index entry cases

Signed-off-by: scnace <scbizu@gmail.com>
---
 pkg/chartmuseum/server/multitenant/api.go     |  9 +++++----
 .../server/multitenant/server_test.go         | 19 +++++++++++++++++++
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/pkg/chartmuseum/server/multitenant/api.go b/pkg/chartmuseum/server/multitenant/api.go
index afc2ab5c..2b03d5e3 100644
--- a/pkg/chartmuseum/server/multitenant/api.go
+++ b/pkg/chartmuseum/server/multitenant/api.go
@@ -95,15 +95,15 @@ func (server *MultiTenantServer) deleteChartVersion(log cm_logger.LoggingFn, rep
 	return nil
 }
 
-func (server *MultiTenantServer) uploadChartPackage(log cm_logger.LoggingFn, repo string, content []byte, force bool) (string, *HTTPError ){
+func (server *MultiTenantServer) uploadChartPackage(log cm_logger.LoggingFn, repo string, content []byte, force bool) (string, *HTTPError) {
 	filename, err := cm_repo.ChartPackageFilenameFromContent(content)
 	if err != nil {
-		return filename,&HTTPError{http.StatusInternalServerError, err.Error()}
+		return filename, &HTTPError{http.StatusInternalServerError, err.Error()}
 	}
 
 	if pathutil.Base(filename) != filename {
 		// Name wants to break out of current directory
-		return filename,&HTTPError{http.StatusBadRequest, fmt.Sprintf("%s is improperly formatted", filename)}
+		return filename, &HTTPError{http.StatusBadRequest, fmt.Sprintf("%s is improperly formatted", filename)}
 	}
 
 	if !server.AllowOverwrite && (!server.AllowForceOverwrite || !force) {
@@ -139,7 +139,8 @@ func (server *MultiTenantServer) uploadChartPackage(log cm_logger.LoggingFn, rep
 	)
 	err = server.StorageBackend.PutObject(pathutil.Join(repo, filename), content)
 	if err != nil {
-		return filename, &HTTPError{http.StatusInternalServerError, err.Error()}	}
+		return filename, &HTTPError{http.StatusInternalServerError, err.Error()}
+	}
 	return filename, nil
 }
 
diff --git a/pkg/chartmuseum/server/multitenant/server_test.go b/pkg/chartmuseum/server/multitenant/server_test.go
index 13050e25..138364f8 100644
--- a/pkg/chartmuseum/server/multitenant/server_test.go
+++ b/pkg/chartmuseum/server/multitenant/server_test.go
@@ -339,6 +339,7 @@ func (suite *MultiTenantServerTestSuite) SetupSuite() {
 		AllowOverwrite:         true,
 		ChartPostFormFieldName: "chart",
 		ProvPostFormFieldName:  "prov",
+		CacheInterval:          time.Duration(time.Second),
 	})
 	suite.NotNil(server)
 	suite.Nil(err, "no error creating new overwrite server")
@@ -627,6 +628,14 @@ func (suite *MultiTenantServerTestSuite) TestDisabledDeleteServer() {
 	suite.Equal(404, res.Status(), "404 DELETE /api/charts/mychart/0.1.0")
 }
 
+func (suite *MultiTenantServerTestSuite) extractRepoEntryFromInternalCache(repo string) *cacheEntry {
+	local, ok := suite.OverwriteServer.InternalCacheStore[repo]
+	if ok {
+		return local
+	}
+	return nil
+}
+
 func (suite *MultiTenantServerTestSuite) TestOverwriteServer() {
 	// Check if files can be overwritten
 	content, err := ioutil.ReadFile(testTarballPath)
@@ -638,6 +647,16 @@ func (suite *MultiTenantServerTestSuite) TestOverwriteServer() {
 	res = suite.doRequest("overwrite", "POST", "/api/charts", body, "")
 	suite.Equal(201, res.Status(), "201 POST /api/charts")
 
+	{
+		// waiting for the emit event
+		// the event is transferred via a channel , here do a simple wait for not changing the original structure
+		// only for testing purpose
+		time.Sleep(time.Second)
+		// depth: 0
+		e := suite.extractRepoEntryFromInternalCache("")
+		suite.Equal(1, len(e.RepoIndex.Entries), "overwrite entries validation")
+	}
+
 	content, err = ioutil.ReadFile(testProvfilePath)
 	suite.Nil(err, "no error opening test provenance file")
 	body = bytes.NewBuffer(content)

From cd2e286da8148a7c114cb45867bf5c7b09e29467 Mon Sep 17 00:00:00 2001
From: scnace <scbizu@gmail.com>
Date: Mon, 3 May 2021 15:42:00 +0800
Subject: [PATCH 2/2] pkg/chartmuseum/server/multitenant: fix the bad action
 type when upload package when overwrite option is set ,index entry addChart
 should be updateChart under the overwrite cases.

Signed-off-by: scnace <scbizu@gmail.com>
---
 pkg/chartmuseum/server/multitenant/api.go      | 18 +++++++++++++++---
 pkg/chartmuseum/server/multitenant/handlers.go | 18 +++++++++++++++---
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/pkg/chartmuseum/server/multitenant/api.go b/pkg/chartmuseum/server/multitenant/api.go
index 2b03d5e3..902ab7c6 100644
--- a/pkg/chartmuseum/server/multitenant/api.go
+++ b/pkg/chartmuseum/server/multitenant/api.go
@@ -106,11 +106,18 @@ func (server *MultiTenantServer) uploadChartPackage(log cm_logger.LoggingFn, rep
 		return filename, &HTTPError{http.StatusBadRequest, fmt.Sprintf("%s is improperly formatted", filename)}
 	}
 
-	if !server.AllowOverwrite && (!server.AllowForceOverwrite || !force) {
-		_, err = server.StorageBackend.GetObject(pathutil.Join(repo, filename))
-		if err == nil {
+	// we should ensure that whether chart is existed even if the `overwrite` option is set
+	// For `overwrite` option , here will increase one `storage.GetObject` than before ; others should be equalvarant with the previous version.
+	var found bool
+	_, err = server.StorageBackend.GetObject(pathutil.Join(repo, filename))
+	// found
+	if err == nil {
+		found = true
+		// For those no-overwrite servers, return the Conflict error.
+		if !server.AllowOverwrite && (!server.AllowForceOverwrite || !force) {
 			return filename, &HTTPError{http.StatusConflict, "file already exists"}
 		}
+		// continue with the `overwrite` servers
 	}
 
 	if server.EnforceSemver2 {
@@ -141,6 +148,11 @@ func (server *MultiTenantServer) uploadChartPackage(log cm_logger.LoggingFn, rep
 	if err != nil {
 		return filename, &HTTPError{http.StatusInternalServerError, err.Error()}
 	}
+	if found {
+		// here is a fake conflict error for outside call
+		// In order to not add another return `bool` check (API Compatibility)
+		return filename, &HTTPError{http.StatusConflict, ""}
+	}
 	return filename, nil
 }
 
diff --git a/pkg/chartmuseum/server/multitenant/handlers.go b/pkg/chartmuseum/server/multitenant/handlers.go
index 3e1f0602..c6c31b01 100644
--- a/pkg/chartmuseum/server/multitenant/handlers.go
+++ b/pkg/chartmuseum/server/multitenant/handlers.go
@@ -242,10 +242,22 @@ func (server *MultiTenantServer) postPackageRequestHandler(c *gin.Context) {
 	}
 	log := server.Logger.ContextLoggingFn(c)
 	_, force := c.GetQuery("force")
+	action := addChart
 	filename, err := server.uploadChartPackage(log, repo, content, force)
 	if err != nil {
-		c.JSON(err.Status, gin.H{"error": err.Message})
-		return
+		// here should check both err.Status and err.Message
+		// The http.StatusConflict status means the chart is existed but overwrite is not sed OR chart is existed and overwrite is set
+		// err.Status == http.StatusConflict only denotes for chart is existed now.
+		if err.Status == http.StatusConflict {
+			if err.Message != "" {
+				c.JSON(err.Status, gin.H{"error": err.Message})
+				return
+			}
+			action = updateChart
+		} else {
+			c.JSON(err.Status, gin.H{"error": err.Message})
+			return
+		}
 	}
 
 	chart, chartErr := cm_repo.ChartVersionFromStorageObject(cm_storage.Object{
@@ -255,7 +267,7 @@ func (server *MultiTenantServer) postPackageRequestHandler(c *gin.Context) {
 	if chartErr != nil {
 		log(cm_logger.ErrorLevel, "cannot get chart from content", zap.Error(chartErr), zap.Binary("content", content))
 	}
-	server.emitEvent(c, repo, addChart, chart)
+	server.emitEvent(c, repo, action, chart)
 
 	c.JSON(201, objectSavedResponse)
 }