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