diff --git a/make/photon/chartserver/454.patch b/make/photon/chartserver/454.patch new file mode 100644 index 000000000..9f244c808 --- /dev/null +++ b/make/photon/chartserver/454.patch @@ -0,0 +1,179 @@ +From 66cc2635880193ffb1226e3c790b36eef24cfd8b Mon Sep 17 00:00:00 2001 +From: scnace +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 +--- + 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 +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 +--- + 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) + } diff --git a/make/photon/chartserver/492.patch b/make/photon/chartserver/492.patch new file mode 100644 index 000000000..1067b4d23 --- /dev/null +++ b/make/photon/chartserver/492.patch @@ -0,0 +1,234 @@ +From 5dd7f0370f73cdffa76707e4f1f715ee4e209f3e Mon Sep 17 00:00:00 2001 +From: DQ +Date: Fri, 24 Sep 2021 17:56:00 +0000 +Subject: [PATCH 1/2] Fix duplicate versions for same chart + +* The detailed issue is described in #450 +* And there is a PR #454 fixed one scenario of this issue +* But there is another ocassion in which users upload chart with prov +* in this PR is to handle this situation with the way similar with #454 + +Signed-off-by: DQ +--- + .../server/multitenant/handlers.go | 55 +++++++++++++------ + .../server/multitenant/server_test.go | 7 +++ + 2 files changed, 46 insertions(+), 16 deletions(-) + +diff --git a/pkg/chartmuseum/server/multitenant/handlers.go b/pkg/chartmuseum/server/multitenant/handlers.go +index c6c31b0..a39a00d 100644 +--- a/pkg/chartmuseum/server/multitenant/handlers.go ++++ b/pkg/chartmuseum/server/multitenant/handlers.go +@@ -299,8 +299,24 @@ func (server *MultiTenantServer) postPackageAndProvenanceRequestHandler(c *gin.C + _, force := c.GetQuery("force") + var chartContent []byte + var path string ++ // action used to determine what operation to emit ++ action := addChart + cpFiles, status, err := server.getChartAndProvFiles(c.Request, repo, force) +- if status != 200 { ++ if err != nil { ++ c.JSON(status, gin.H{"error": fmt.Sprintf("%s", err)}) ++ return ++ } ++ switch status { ++ case http.StatusOK: ++ case http.StatusConflict: ++ if !server.AllowOverwrite && (!server.AllowForceOverwrite || !force) { ++ c.JSON(status, gin.H{"error": fmt.Sprintf("%s", fmt.Errorf("chart already exists"))}) // conflict ++ return ++ } ++ log(cm_logger.DebugLevel, "chart already exists, but overwrite is allowed", zap.String("repo", repo)) ++ // update chart if chart already exists and overwrite is allowed ++ action = updateChart ++ default: + c.JSON(status, gin.H{"error": fmt.Sprintf("%s", err)}) + return + } +@@ -309,7 +325,7 @@ func (server *MultiTenantServer) postPackageAndProvenanceRequestHandler(c *gin.C + if len(c.Errors) > 0 { + return // this is a "request too large" + } +- c.JSON(400, gin.H{"error": fmt.Sprintf( ++ c.JSON(http.StatusBadRequest, gin.H{"error": fmt.Sprintf( + "no package or provenance file found in form fields %s and %s", + server.ChartPostFormFieldName, server.ProvPostFormFieldName), + }) +@@ -332,7 +348,7 @@ func (server *MultiTenantServer) postPackageAndProvenanceRequestHandler(c *gin.C + for _, ppf := range storedFiles { + server.StorageBackend.DeleteObject(ppf.filename) + } +- c.JSON(500, gin.H{"error": fmt.Sprintf("%s", err)}) ++ c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("%s", err)}) + return + } + if ppf.field == defaultFormField { +@@ -350,9 +366,9 @@ func (server *MultiTenantServer) postPackageAndProvenanceRequestHandler(c *gin.C + log(cm_logger.ErrorLevel, "cannot get chart from content", zap.Error(err), zap.Binary("content", chartContent)) + } + +- server.emitEvent(c, repo, addChart, chart) ++ server.emitEvent(c, repo, action, chart) + +- c.JSON(201, objectSavedResponse) ++ c.JSON(http.StatusCreated, objectSavedResponse) + } + + func (server *MultiTenantServer) getChartAndProvFiles(req *http.Request, repo string, force bool) (map[string]*chartOrProvenanceFile, int, error) { +@@ -368,29 +384,36 @@ func (server *MultiTenantServer) getChartAndProvFiles(req *http.Request, repo st + {server.ProvPostFormFieldName, cm_repo.ProvenanceFilenameFromContent}, + } + ++ validStatusCode := http.StatusOK + cpFiles := make(map[string]*chartOrProvenanceFile) + for _, ff := range ffp { + content, err := extractContentFromRequest(req, ff.field) + if err != nil { +- return nil, 500, err ++ return nil, http.StatusInternalServerError, err + } + if content == nil { + continue + } + filename, err := ff.fn(content) + if err != nil { +- return nil, 400, err ++ return nil, http.StatusBadRequest, err + } + if _, ok := cpFiles[filename]; ok { + continue + } +- if status, err := server.validateChartOrProv(repo, filename, force); err != nil { ++ status, err := server.validateChartOrProv(repo, filename, force) ++ if err != nil { + return nil, status, err + } ++ // return conflict status code if the file already exists ++ if status == http.StatusConflict && validStatusCode != http.StatusConflict { ++ validStatusCode = status ++ } + cpFiles[filename] = &chartOrProvenanceFile{filename, content, ff.field} + } + +- return cpFiles, 200, nil ++ // validState code can be 200 or 409. Returning 409 means that the chart already exists ++ return cpFiles, validStatusCode, nil + } + + func extractContentFromRequest(req *http.Request, field string) ([]byte, error) { +@@ -408,7 +431,7 @@ func extractContentFromRequest(req *http.Request, field string) ([]byte, error) + + func (server *MultiTenantServer) validateChartOrProv(repo, filename string, force bool) (int, error) { + if pathutil.Base(filename) != filename { +- return 400, fmt.Errorf("%s is improperly formatted", filename) // Name wants to break out of current directory ++ return http.StatusBadRequest, fmt.Errorf("%s is improperly formatted", filename) // Name wants to break out of current directory + } + + var f string +@@ -417,11 +440,11 @@ func (server *MultiTenantServer) validateChartOrProv(repo, filename string, forc + } else { + f = repo + "/" + filename + } +- if !server.AllowOverwrite && (!server.AllowForceOverwrite || !force) { +- _, err := server.StorageBackend.GetObject(f) +- if err == nil { +- return 409, fmt.Errorf("%s already exists", f) // conflict +- } ++ // conflict does not mean the file is invalid. ++ // for example, when overwite is allowed, it's valid ++ // so that the client can decide what to do and here we just return conflict with no error ++ if _, err := server.StorageBackend.GetObject(f); err == nil { ++ return http.StatusConflict, nil + } +- return 200, nil ++ return http.StatusOK, nil + } +diff --git a/pkg/chartmuseum/server/multitenant/server_test.go b/pkg/chartmuseum/server/multitenant/server_test.go +index 138364f..477f349 100644 +--- a/pkg/chartmuseum/server/multitenant/server_test.go ++++ b/pkg/chartmuseum/server/multitenant/server_test.go +@@ -672,6 +672,13 @@ func (suite *MultiTenantServerTestSuite) TestOverwriteServer() { + buf, w = suite.getBodyWithMultipartFormFiles([]string{"chart", "prov"}, []string{testTarballPath, testProvfilePath}) + res = suite.doRequest("overwrite", "POST", "/api/charts", buf, w.FormDataContentType()) + suite.Equal(201, res.Status(), "201 POST /api/charts") ++ { ++ // the same as chart only case above ++ time.Sleep(time.Second) ++ // depth: 0 ++ e := suite.extractRepoEntryFromInternalCache("") ++ suite.Equal(1, len(e.RepoIndex.Entries), "overwrite entries validation") ++ } + } + + func (suite *MultiTenantServerTestSuite) TestBadChartUpload() { + +From 1ecdaa5811178f4d4d6d1cc8077c354cc8f5859f Mon Sep 17 00:00:00 2001 +From: DQ +Date: Thu, 30 Sep 2021 13:54:30 +0000 +Subject: [PATCH 2/2] Enhance: optimize loop in `getChartAndProvFiles` + +* If conflict, it didn't need to do the left logic, just return the file +* move out file format check logic out of `validateChartOrProv` +* these changes are discussed in https://github.com/helm/chartmuseum/pull/492#discussion_r716032288 + +Signed-off-by: DQ +--- + .../server/multitenant/handlers.go | 22 ++++++++++++------- + 1 file changed, 14 insertions(+), 8 deletions(-) + +diff --git a/pkg/chartmuseum/server/multitenant/handlers.go b/pkg/chartmuseum/server/multitenant/handlers.go +index a39a00d..fbaf450 100644 +--- a/pkg/chartmuseum/server/multitenant/handlers.go ++++ b/pkg/chartmuseum/server/multitenant/handlers.go +@@ -384,7 +384,7 @@ func (server *MultiTenantServer) getChartAndProvFiles(req *http.Request, repo st + {server.ProvPostFormFieldName, cm_repo.ProvenanceFilenameFromContent}, + } + +- validStatusCode := http.StatusOK ++ validReturnStatusCode := http.StatusOK + cpFiles := make(map[string]*chartOrProvenanceFile) + for _, ff := range ffp { + content, err := extractContentFromRequest(req, ff.field) +@@ -401,19 +401,29 @@ func (server *MultiTenantServer) getChartAndProvFiles(req *http.Request, repo st + if _, ok := cpFiles[filename]; ok { + continue + } ++ // if the file already exists, we don't need to validate it again ++ if validReturnStatusCode == http.StatusConflict { ++ cpFiles[filename] = &chartOrProvenanceFile{filename, content, ff.field} ++ continue ++ } ++ // check filename ++ if pathutil.Base(filename) != filename { ++ return nil, http.StatusBadRequest, fmt.Errorf("%s is improperly formatted", filename) // Name wants to break out of current directory ++ } ++ // check existence + status, err := server.validateChartOrProv(repo, filename, force) + if err != nil { + return nil, status, err + } + // return conflict status code if the file already exists +- if status == http.StatusConflict && validStatusCode != http.StatusConflict { +- validStatusCode = status ++ if status == http.StatusConflict { ++ validReturnStatusCode = status + } + cpFiles[filename] = &chartOrProvenanceFile{filename, content, ff.field} + } + + // validState code can be 200 or 409. Returning 409 means that the chart already exists +- return cpFiles, validStatusCode, nil ++ return cpFiles, validReturnStatusCode, nil + } + + func extractContentFromRequest(req *http.Request, field string) ([]byte, error) { +@@ -430,10 +440,6 @@ func extractContentFromRequest(req *http.Request, field string) ([]byte, error) + } + + func (server *MultiTenantServer) validateChartOrProv(repo, filename string, force bool) (int, error) { +- if pathutil.Base(filename) != filename { +- return http.StatusBadRequest, fmt.Errorf("%s is improperly formatted", filename) // Name wants to break out of current directory +- } +- + var f string + if repo == "" { + f = filename