Upstream/harbor
1
0
Fork 0
mirror of https://github.com/goharbor/harbor.git synced 2024-11-14 22:35:36 +01:00
Code Issues Projects Releases Wiki Activity

Merge pull request #16029 from ninjadq/fix_chart_replication_issue

Browse Source 
Fix: chart replication dup files issue
This commit is contained in:
Qian Deng 2021-12-03 13:47:42 +08:00 committed by GitHub
commit 5fc4449450
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 413 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

179
make/photon/chartserver/454.patch Normal file
View File

@ -0,0 +1,179 @@
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)
}

234
make/photon/chartserver/492.patch Normal file
View File

@ -0,0 +1,234 @@
From 5dd7f0370f73cdffa76707e4f1f715ee4e209f3e Mon Sep 17 00:00:00 2001
From: DQ <dengq@vmware.com>
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 <dengq@vmware.com>
---
.../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 <dengq@vmware.com>
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 <dengq@vmware.com>
---
.../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