From 1166dead006eaf95dc4a542b62e9f3d3e96b1d58 Mon Sep 17 00:00:00 2001 From: shichanglin5 Date: Fri, 13 May 2022 18:14:39 +0800 Subject: [PATCH 1/2] fix the problem of metadata and tagging loss when files are copied by adding processing of metadata and tagging in s3 api CopyObject (judging whether to copy or overwrite according to the directive header) --- weed/s3api/http/header.go | 9 +- weed/s3api/s3api_object_copy_handlers.go | 131 ++++++++++++++++++++++- 2 files changed, 132 insertions(+), 8 deletions(-) diff --git a/weed/s3api/http/header.go b/weed/s3api/http/header.go index d63d50443..30fc8eefa 100644 --- a/weed/s3api/http/header.go +++ b/weed/s3api/http/header.go @@ -28,11 +28,14 @@ const ( AmzStorageClass = "x-amz-storage-class" // S3 user-defined metadata - AmzUserMetaPrefix = "X-Amz-Meta-" + AmzUserMetaPrefix = "X-Amz-Meta-" + AmzUserMetaDirective = "X-Amz-Metadata-Directive" // S3 object tagging - AmzObjectTagging = "X-Amz-Tagging" - AmzTagCount = "x-amz-tagging-count" + AmzObjectTagging = "X-Amz-Tagging" + AmzObjectTaggingPrefix = "X-Amz-Tagging-" + AmzObjectTaggingDirective = "X-Amz-Tagging-Directive" + AmzTagCount = "x-amz-tagging-count" ) // Non-Standard S3 HTTP request constants diff --git a/weed/s3api/s3api_object_copy_handlers.go b/weed/s3api/s3api_object_copy_handlers.go index f62db9c31..ff1329ba4 100644 --- a/weed/s3api/s3api_object_copy_handlers.go +++ b/weed/s3api/s3api_object_copy_handlers.go @@ -3,9 +3,10 @@ package s3api import ( "fmt" "github.com/chrislusf/seaweedfs/weed/glog" + headers "github.com/chrislusf/seaweedfs/weed/s3api/http" xhttp "github.com/chrislusf/seaweedfs/weed/s3api/http" "github.com/chrislusf/seaweedfs/weed/s3api/s3err" - weed_server "github.com/chrislusf/seaweedfs/weed/server" + "modernc.org/strutil" "net/http" "net/url" "strconv" @@ -30,7 +31,9 @@ func (s3a *S3ApiServer) CopyObjectHandler(w http.ResponseWriter, r *http.Request glog.V(3).Infof("CopyObjectHandler %s %s => %s %s", srcBucket, srcObject, dstBucket, dstObject) - if (srcBucket == dstBucket && srcObject == dstObject || cpSrcPath == "") && isReplace(r) { + replaceMeta, replaceTagging := replaceDirective(r) + + if (srcBucket == dstBucket && srcObject == dstObject || cpSrcPath == "") && (replaceMeta || replaceTagging) { fullPath := util.FullPath(fmt.Sprintf("%s/%s%s", s3a.option.BucketsPath, dstBucket, dstObject)) dir, name := fullPath.DirAndName() entry, err := s3a.getEntry(dir, name) @@ -38,7 +41,7 @@ func (s3a *S3ApiServer) CopyObjectHandler(w http.ResponseWriter, r *http.Request s3err.WriteErrorResponse(w, r, s3err.ErrInvalidCopySource) return } - entry.Extended = weed_server.SaveAmzMetaData(r, entry.Extended, isReplace(r)) + entry.Extended = processMetadataBytes(r.Header, entry.Extended, replaceMeta, replaceTagging) err = s3a.touch(dir, name, entry) if err != nil { s3err.WriteErrorResponse(w, r, s3err.ErrInvalidCopySource) @@ -80,6 +83,11 @@ func (s3a *S3ApiServer) CopyObjectHandler(w http.ResponseWriter, r *http.Request } defer util.CloseResponse(resp) + tagErr := processMetadata(r.Header, resp.Header, replaceMeta, replaceTagging, s3a.getTags, dir, name) + if tagErr != nil { + s3err.WriteErrorResponse(w, r, s3err.ErrInvalidCopySource) + return + } glog.V(2).Infof("copy from %s to %s", srcUrl, dstUrl) etag, errCode := s3a.putToFiler(r, dstUrl, resp.Body) @@ -182,6 +190,119 @@ func (s3a *S3ApiServer) CopyObjectPartHandler(w http.ResponseWriter, r *http.Req } -func isReplace(r *http.Request) bool { - return r.Header.Get("X-Amz-Metadata-Directive") == "REPLACE" +func replaceDirective(r *http.Request) (replaceMeta, replaceTagging bool) { + return r.Header.Get(headers.AmzUserMetaDirective) == "REPLACE", r.Header.Get(headers.AmzObjectTaggingDirective) == "REPLACE" +} + +func processMetadata(reqHeader, existing http.Header, replaceMeta, replaceTagging bool, getTags func(parentDirectoryPath string, entryName string) (tags map[string]string, err error), dir, name string) (err error) { + if sc := reqHeader.Get(xhttp.AmzStorageClass); len(sc) == 0 { + if sc := existing[xhttp.AmzStorageClass]; len(sc) > 0 { + reqHeader[xhttp.AmzStorageClass] = sc + } + } + + if !replaceMeta { + for header, _ := range reqHeader { + if strings.HasPrefix(header, xhttp.AmzUserMetaPrefix) { + delete(reqHeader, header) + } + } + for k, v := range existing { + if strings.HasPrefix(k, xhttp.AmzUserMetaPrefix) { + reqHeader[k] = v + } + } + } + + if replaceTagging { + if tags := reqHeader.Get(xhttp.AmzObjectTagging); tags != "" { + for _, v := range strings.Split(tags, "&") { + tag := strings.Split(v, "=") + if len(tag) == 2 { + reqHeader[xhttp.AmzObjectTagging+"-"+tag[0]] = []string{tag[1]} + } else if len(tag) == 1 { + reqHeader[xhttp.AmzObjectTagging+"-"+tag[0]] = []string{} + } + } + } + delete(reqHeader, xhttp.AmzObjectTagging) + } else { + for header, _ := range reqHeader { + if strings.HasPrefix(header, xhttp.AmzObjectTagging) { + delete(reqHeader, header) + } + } + + found := false + for k, _ := range existing { + if strings.HasPrefix(k, xhttp.AmzObjectTaggingPrefix) { + found = true + break + } + } + + if found { + tags, err := getTags(dir, name) + if err != nil { + return err + } + + var tagArr []string + for k, v := range tags { + tagArr = append(tagArr, fmt.Sprintf("%s=%s", k, v)) + } + tagStr := strutil.JoinFields(tagArr, "&") + reqHeader.Set(xhttp.AmzObjectTagging, tagStr) + } + } + return +} + +func processMetadataBytes(reqHeader http.Header, existing map[string][]byte, replaceMeta, replaceTagging bool) (metadata map[string][]byte) { + metadata = make(map[string][]byte) + + if sc := existing[xhttp.AmzStorageClass]; len(sc) > 0 { + metadata[xhttp.AmzStorageClass] = sc + } + if sc := reqHeader.Get(xhttp.AmzStorageClass); len(sc) > 0 { + metadata[xhttp.AmzStorageClass] = []byte(sc) + } + + if replaceMeta { + for k, v := range existing { + if strings.HasPrefix(k, xhttp.AmzUserMetaPrefix) { + metadata[k] = v + } + } + } else { + for header, values := range reqHeader { + if strings.HasPrefix(header, xhttp.AmzUserMetaPrefix) { + for _, value := range values { + metadata[header] = []byte(value) + } + } + } + } + + if replaceTagging { + if tags := reqHeader.Get(xhttp.AmzObjectTagging); tags != "" { + for _, v := range strings.Split(tags, "&") { + tag := strings.Split(v, "=") + if len(tag) == 2 { + metadata[xhttp.AmzObjectTagging+"-"+tag[0]] = []byte(tag[1]) + } else if len(tag) == 1 { + metadata[xhttp.AmzObjectTagging+"-"+tag[0]] = nil + } + } + } + } else { + for k, v := range existing { + if strings.HasPrefix(k, xhttp.AmzObjectTagging) { + metadata[k] = v + } + } + delete(metadata, xhttp.AmzTagCount) + } + + return } From 688d55488ccebd2e3a97a0e4570c3bc8bbdceb8f Mon Sep 17 00:00:00 2001 From: shichanglin5 Date: Sat, 14 May 2022 10:40:29 +0800 Subject: [PATCH 2/2] test(s3api_object_copy_handlers_test.go): some unit tests have been added to the processMetadata & processMetadataBytes methods of s3api_object_copy_handlers.go --- weed/s3api/s3api_object_copy_handlers.go | 37 +- weed/s3api/s3api_object_copy_handlers_test.go | 426 ++++++++++++++++++ 2 files changed, 441 insertions(+), 22 deletions(-) create mode 100644 weed/s3api/s3api_object_copy_handlers_test.go diff --git a/weed/s3api/s3api_object_copy_handlers.go b/weed/s3api/s3api_object_copy_handlers.go index ff1329ba4..c44ca7ddf 100644 --- a/weed/s3api/s3api_object_copy_handlers.go +++ b/weed/s3api/s3api_object_copy_handlers.go @@ -16,6 +16,11 @@ import ( "github.com/chrislusf/seaweedfs/weed/util" ) +const ( + DirectiveCopy = "COPY" + DirectiveReplace = "REPLACE" +) + func (s3a *S3ApiServer) CopyObjectHandler(w http.ResponseWriter, r *http.Request) { dstBucket, dstObject := xhttp.GetBucketAndObject(r) @@ -31,7 +36,7 @@ func (s3a *S3ApiServer) CopyObjectHandler(w http.ResponseWriter, r *http.Request glog.V(3).Infof("CopyObjectHandler %s %s => %s %s", srcBucket, srcObject, dstBucket, dstObject) - replaceMeta, replaceTagging := replaceDirective(r) + replaceMeta, replaceTagging := replaceDirective(r.Header) if (srcBucket == dstBucket && srcObject == dstObject || cpSrcPath == "") && (replaceMeta || replaceTagging) { fullPath := util.FullPath(fmt.Sprintf("%s/%s%s", s3a.option.BucketsPath, dstBucket, dstObject)) @@ -190,8 +195,8 @@ func (s3a *S3ApiServer) CopyObjectPartHandler(w http.ResponseWriter, r *http.Req } -func replaceDirective(r *http.Request) (replaceMeta, replaceTagging bool) { - return r.Header.Get(headers.AmzUserMetaDirective) == "REPLACE", r.Header.Get(headers.AmzObjectTaggingDirective) == "REPLACE" +func replaceDirective(reqHeader http.Header) (replaceMeta, replaceTagging bool) { + return reqHeader.Get(headers.AmzUserMetaDirective) == DirectiveReplace, reqHeader.Get(headers.AmzObjectTaggingDirective) == DirectiveReplace } func processMetadata(reqHeader, existing http.Header, replaceMeta, replaceTagging bool, getTags func(parentDirectoryPath string, entryName string) (tags map[string]string, err error), dir, name string) (err error) { @@ -214,19 +219,7 @@ func processMetadata(reqHeader, existing http.Header, replaceMeta, replaceTaggin } } - if replaceTagging { - if tags := reqHeader.Get(xhttp.AmzObjectTagging); tags != "" { - for _, v := range strings.Split(tags, "&") { - tag := strings.Split(v, "=") - if len(tag) == 2 { - reqHeader[xhttp.AmzObjectTagging+"-"+tag[0]] = []string{tag[1]} - } else if len(tag) == 1 { - reqHeader[xhttp.AmzObjectTagging+"-"+tag[0]] = []string{} - } - } - } - delete(reqHeader, xhttp.AmzObjectTagging) - } else { + if !replaceTagging { for header, _ := range reqHeader { if strings.HasPrefix(header, xhttp.AmzObjectTagging) { delete(reqHeader, header) @@ -269,12 +262,6 @@ func processMetadataBytes(reqHeader http.Header, existing map[string][]byte, rep } if replaceMeta { - for k, v := range existing { - if strings.HasPrefix(k, xhttp.AmzUserMetaPrefix) { - metadata[k] = v - } - } - } else { for header, values := range reqHeader { if strings.HasPrefix(header, xhttp.AmzUserMetaPrefix) { for _, value := range values { @@ -282,6 +269,12 @@ func processMetadataBytes(reqHeader http.Header, existing map[string][]byte, rep } } } + } else { + for k, v := range existing { + if strings.HasPrefix(k, xhttp.AmzUserMetaPrefix) { + metadata[k] = v + } + } } if replaceTagging { diff --git a/weed/s3api/s3api_object_copy_handlers_test.go b/weed/s3api/s3api_object_copy_handlers_test.go new file mode 100644 index 000000000..d2c8e488b --- /dev/null +++ b/weed/s3api/s3api_object_copy_handlers_test.go @@ -0,0 +1,426 @@ +package s3api + +import ( + "fmt" + headers "github.com/chrislusf/seaweedfs/weed/s3api/http" + "net/http" + "reflect" + "sort" + "strings" + "testing" +) + +type H map[string]string + +func (h H) String() string { + pairs := make([]string, 0, len(h)) + for k, v := range h { + pairs = append(pairs, fmt.Sprintf("%s : %s", k, v)) + } + sort.Strings(pairs) + join := strings.Join(pairs, "\n") + return "\n" + join + "\n" +} + +var processMetadataTestCases = []struct { + caseId int + request H + existing H + getTags H + want H +}{ + { + 201, + H{ + "User-Agent": "firefox", + "X-Amz-Meta-My-Meta": "request", + "X-Amz-Tagging": "A=B&a=b&type=request", + }, + H{ + "X-Amz-Meta-My-Meta": "existing", + "X-Amz-Tagging-A": "B", + "X-Amz-Tagging-Type": "existing", + }, + H{ + "A": "B", + "a": "b", + "type": "existing", + }, + H{ + "User-Agent": "firefox", + "X-Amz-Meta-My-Meta": "existing", + "X-Amz-Tagging": "A=B&a=b&type=existing", + }, + }, + { + 202, + H{ + "User-Agent": "firefox", + "X-Amz-Meta-My-Meta": "request", + "X-Amz-Tagging": "A=B&a=b&type=request", + headers.AmzUserMetaDirective: DirectiveReplace, + }, + H{ + "X-Amz-Meta-My-Meta": "existing", + "X-Amz-Tagging-A": "B", + "X-Amz-Tagging-Type": "existing", + }, + H{ + "A": "B", + "a": "b", + "type": "existing", + }, + H{ + "User-Agent": "firefox", + "X-Amz-Meta-My-Meta": "request", + "X-Amz-Tagging": "A=B&a=b&type=existing", + headers.AmzUserMetaDirective: DirectiveReplace, + }, + }, + + { + 203, + H{ + "User-Agent": "firefox", + "X-Amz-Meta-My-Meta": "request", + "X-Amz-Tagging": "A=B&a=b&type=request", + headers.AmzObjectTaggingDirective: DirectiveReplace, + }, + H{ + "X-Amz-Meta-My-Meta": "existing", + "X-Amz-Tagging-A": "B", + "X-Amz-Tagging-Type": "existing", + }, + H{ + "A": "B", + "a": "b", + "type": "existing", + }, + H{ + "User-Agent": "firefox", + "X-Amz-Meta-My-Meta": "existing", + "X-Amz-Tagging": "A=B&a=b&type=request", + headers.AmzObjectTaggingDirective: DirectiveReplace, + }, + }, + + { + 204, + H{ + "User-Agent": "firefox", + "X-Amz-Meta-My-Meta": "request", + "X-Amz-Tagging": "A=B&a=b&type=request", + headers.AmzUserMetaDirective: DirectiveReplace, + headers.AmzObjectTaggingDirective: DirectiveReplace, + }, + H{ + "X-Amz-Meta-My-Meta": "existing", + "X-Amz-Tagging-A": "B", + "X-Amz-Tagging-a": "b", + "X-Amz-Tagging-Type": "existing", + }, + H{ + "A": "B", + "a": "b", + "type": "existing", + }, + H{ + "User-Agent": "firefox", + "X-Amz-Meta-My-Meta": "request", + "X-Amz-Tagging": "A=B&a=b&type=request", + headers.AmzUserMetaDirective: DirectiveReplace, + headers.AmzObjectTaggingDirective: DirectiveReplace, + }, + }, + + { + 205, + H{ + "User-Agent": "firefox", + "X-Amz-Meta-My-Meta": "request", + "X-Amz-Tagging": "A=B&a=b&type=request", + headers.AmzUserMetaDirective: DirectiveReplace, + headers.AmzObjectTaggingDirective: DirectiveReplace, + }, + H{}, + H{}, + H{ + "User-Agent": "firefox", + "X-Amz-Meta-My-Meta": "request", + "X-Amz-Tagging": "A=B&a=b&type=request", + headers.AmzUserMetaDirective: DirectiveReplace, + headers.AmzObjectTaggingDirective: DirectiveReplace, + }, + }, + + { + 206, + H{ + "User-Agent": "firefox", + headers.AmzUserMetaDirective: DirectiveReplace, + headers.AmzObjectTaggingDirective: DirectiveReplace, + }, + H{ + "X-Amz-Meta-My-Meta": "existing", + "X-Amz-Tagging-A": "B", + "X-Amz-Tagging-a": "b", + "X-Amz-Tagging-Type": "existing", + }, + H{ + "A": "B", + "a": "b", + "type": "existing", + }, + H{ + "User-Agent": "firefox", + headers.AmzUserMetaDirective: DirectiveReplace, + headers.AmzObjectTaggingDirective: DirectiveReplace, + }, + }, + + { + 207, + H{ + "User-Agent": "firefox", + "X-Amz-Meta-My-Meta": "request", + headers.AmzUserMetaDirective: DirectiveReplace, + headers.AmzObjectTaggingDirective: DirectiveReplace, + }, + H{ + "X-Amz-Meta-My-Meta": "existing", + "X-Amz-Tagging-A": "B", + "X-Amz-Tagging-a": "b", + "X-Amz-Tagging-Type": "existing", + }, + H{ + "A": "B", + "a": "b", + "type": "existing", + }, + H{ + "User-Agent": "firefox", + "X-Amz-Meta-My-Meta": "request", + headers.AmzUserMetaDirective: DirectiveReplace, + headers.AmzObjectTaggingDirective: DirectiveReplace, + }, + }, +} +var processMetadataBytesTestCases = []struct { + caseId int + request H + existing H + want H +}{ + { + 101, + H{ + "User-Agent": "firefox", + "X-Amz-Meta-My-Meta": "request", + "X-Amz-Tagging": "A=B&a=b&type=request", + }, + H{ + "X-Amz-Meta-My-Meta": "existing", + "X-Amz-Tagging-A": "B", + "X-Amz-Tagging-a": "b", + "X-Amz-Tagging-type": "existing", + }, + H{ + "X-Amz-Meta-My-Meta": "existing", + "X-Amz-Tagging-A": "B", + "X-Amz-Tagging-a": "b", + "X-Amz-Tagging-type": "existing", + }, + }, + + { + 102, + H{ + "User-Agent": "firefox", + "X-Amz-Meta-My-Meta": "request", + "X-Amz-Tagging": "A=B&a=b&type=request", + headers.AmzUserMetaDirective: DirectiveReplace, + }, + H{ + "X-Amz-Meta-My-Meta": "existing", + "X-Amz-Tagging-A": "B", + "X-Amz-Tagging-a": "b", + "X-Amz-Tagging-type": "existing", + }, + H{ + "X-Amz-Meta-My-Meta": "request", + "X-Amz-Tagging-A": "B", + "X-Amz-Tagging-a": "b", + "X-Amz-Tagging-type": "existing", + }, + }, + + { + 103, + H{ + "User-Agent": "firefox", + "X-Amz-Meta-My-Meta": "request", + "X-Amz-Tagging": "A=B&a=b&type=request", + headers.AmzObjectTaggingDirective: DirectiveReplace, + }, + H{ + "X-Amz-Meta-My-Meta": "existing", + "X-Amz-Tagging-A": "B", + "X-Amz-Tagging-a": "b", + "X-Amz-Tagging-type": "existing", + }, + H{ + "X-Amz-Meta-My-Meta": "existing", + "X-Amz-Tagging-A": "B", + "X-Amz-Tagging-a": "b", + "X-Amz-Tagging-type": "request", + }, + }, + + { + 104, + H{ + "User-Agent": "firefox", + "X-Amz-Meta-My-Meta": "request", + "X-Amz-Tagging": "A=B&a=b&type=request", + headers.AmzUserMetaDirective: DirectiveReplace, + headers.AmzObjectTaggingDirective: DirectiveReplace, + }, + H{ + "X-Amz-Meta-My-Meta": "existing", + "X-Amz-Tagging-A": "B", + "X-Amz-Tagging-a": "b", + "X-Amz-Tagging-type": "existing", + }, + H{ + "X-Amz-Meta-My-Meta": "request", + "X-Amz-Tagging-A": "B", + "X-Amz-Tagging-a": "b", + "X-Amz-Tagging-type": "request", + }, + }, + + { + 105, + H{ + "User-Agent": "firefox", + headers.AmzUserMetaDirective: DirectiveReplace, + headers.AmzObjectTaggingDirective: DirectiveReplace, + }, + H{ + "X-Amz-Meta-My-Meta": "existing", + "X-Amz-Tagging-A": "B", + "X-Amz-Tagging-a": "b", + "X-Amz-Tagging-type": "existing", + }, + H{}, + }, + + { + 107, + H{ + "User-Agent": "firefox", + "X-Amz-Meta-My-Meta": "request", + "X-Amz-Tagging": "A=B&a=b&type=request", + headers.AmzUserMetaDirective: DirectiveReplace, + headers.AmzObjectTaggingDirective: DirectiveReplace, + }, + H{}, + H{ + "X-Amz-Meta-My-Meta": "request", + "X-Amz-Tagging-A": "B", + "X-Amz-Tagging-a": "b", + "X-Amz-Tagging-type": "request", + }, + }, +} + +func TestProcessMetadata(t *testing.T) { + for _, tc := range processMetadataTestCases { + reqHeader := transferHToHeader(tc.request) + existing := transferHToHeader(tc.existing) + replaceMeta, replaceTagging := replaceDirective(reqHeader) + + err := processMetadata(reqHeader, existing, replaceMeta, replaceTagging, func(_ string, _ string) (tags map[string]string, err error) { + return tc.getTags, nil + }, "", "") + if err != nil { + t.Error(err) + } + + result := transferHeaderToH(reqHeader) + fmtTagging(result, tc.want) + + if !reflect.DeepEqual(result, tc.want) { + t.Error(fmt.Errorf("\n### CaseID: %d ###"+ + "\nRequest:%v"+ + "\nExisting:%v"+ + "\nGetTags:%v"+ + "\nWant:%v"+ + "\nActual:%v", + tc.caseId, tc.request, tc.existing, tc.getTags, tc.want, result)) + } + } +} + +func TestProcessMetadataBytes(t *testing.T) { + for _, tc := range processMetadataBytesTestCases { + reqHeader := transferHToHeader(tc.request) + existing := transferHToBytesArr(tc.existing) + replaceMeta, replaceTagging := replaceDirective(reqHeader) + extends := processMetadataBytes(reqHeader, existing, replaceMeta, replaceTagging) + + result := transferBytesArrToH(extends) + fmtTagging(result, tc.want) + + if !reflect.DeepEqual(result, tc.want) { + t.Error(fmt.Errorf("\n### CaseID: %d ###"+ + "\nRequest:%v"+ + "\nExisting:%v"+ + "\nWant:%v"+ + "\nActual:%v", + tc.caseId, tc.request, tc.existing, tc.want, result)) + } + } +} + +func fmtTagging(maps ...map[string]string) { + for _, m := range maps { + if tagging := m[headers.AmzObjectTagging]; len(tagging) > 0 { + split := strings.Split(tagging, "&") + sort.Strings(split) + m[headers.AmzObjectTagging] = strings.Join(split, "&") + } + } +} + +func transferHToHeader(data map[string]string) http.Header { + header := http.Header{} + for k, v := range data { + header.Add(k, v) + } + return header +} + +func transferHToBytesArr(data map[string]string) map[string][]byte { + m := make(map[string][]byte, len(data)) + for k, v := range data { + m[k] = []byte(v) + } + return m +} + +func transferBytesArrToH(data map[string][]byte) H { + m := make(map[string]string, len(data)) + for k, v := range data { + m[k] = string(v) + } + return m +} + +func transferHeaderToH(data map[string][]string) H { + m := make(map[string]string, len(data)) + for k, v := range data { + m[k] = v[len(v)-1] + } + return m +}