diff --git a/weed/s3api/filer_multipart.go b/weed/s3api/filer_multipart.go index 5010bce3d1..4bf4f39dc8 100644 --- a/weed/s3api/filer_multipart.go +++ b/weed/s3api/filer_multipart.go @@ -361,8 +361,12 @@ func (s3a *S3ApiServer) prepareMultipartCompletionState(r *http.Request, input * entries, _, err := s3a.list(uploadDirectory, "", "", false, s3_constants.MaxS3MultipartParts+1) if err != nil { glog.Errorf("completeMultipartUpload %s %s error: %v", *input.Bucket, *input.UploadId, err) - stats.S3HandlerCounter.WithLabelValues(stats.ErrorCompletedNoSuchUpload).Inc() - return nil, nil, s3err.ErrNoSuchUpload + if isFilerListNotFound(err) { + stats.S3HandlerCounter.WithLabelValues(stats.ErrorCompletedNoSuchUpload).Inc() + return nil, nil, s3err.ErrNoSuchUpload + } + // a store error must not read as "upload gone": the client would drop a fully-uploaded object + return nil, nil, s3err.ErrInternalError } if len(entries) == 0 { stats.S3HandlerCounter.WithLabelValues(stats.ErrorCompletedNoSuchUpload).Inc() @@ -372,8 +376,11 @@ func (s3a *S3ApiServer) prepareMultipartCompletionState(r *http.Request, input * pentry, err := s3a.getEntry(s3a.genUploadsFolder(*input.Bucket), *input.UploadId) if err != nil { glog.Errorf("completeMultipartUpload %s %s error: %v", *input.Bucket, *input.UploadId, err) - stats.S3HandlerCounter.WithLabelValues(stats.ErrorCompletedNoSuchUpload).Inc() - return nil, nil, s3err.ErrNoSuchUpload + if errors.Is(err, filer_pb.ErrNotFound) { + stats.S3HandlerCounter.WithLabelValues(stats.ErrorCompletedNoSuchUpload).Inc() + return nil, nil, s3err.ErrNoSuchUpload + } + return nil, nil, s3err.ErrInternalError } deleteEntries := make([]*filer_pb.Entry, 0) @@ -882,8 +889,10 @@ func (s3a *S3ApiServer) abortMultipartUpload(input *s3.AbortMultipartUploadInput exists, err := s3a.exists(s3a.genUploadsFolder(*input.Bucket), *input.UploadId, true) if err != nil { - glog.V(1).Infof("bucket %s abort upload %s: %v", *input.Bucket, *input.UploadId, err) - return nil, s3err.ErrNoSuchUpload + // filer_pb.Exists reports not-found as (false, nil), so an error here is + // always a store failure; answering NoSuchUpload would leak the parts. + glog.Errorf("bucket %s abort upload %s: %v", *input.Bucket, *input.UploadId, err) + return nil, s3err.ErrInternalError } if exists { err = s3a.rm(s3a.genUploadsFolder(*input.Bucket), *input.UploadId, true, true) @@ -930,8 +939,14 @@ func (s3a *S3ApiServer) listMultipartUploads(input *s3.ListMultipartUploadsInput entries, _, err := s3a.list(s3a.genUploadsFolder(*input.Bucket), "", *input.UploadIdMarker, false, math.MaxInt32) if err != nil { + // A missing .uploads folder normally lists as empty with no error; a + // store that reports it as not-found still means an empty list. + if isFilerListNotFound(err) { + return output, s3err.ErrNone + } + // surface a real store error; a masked empty 200 makes a resuming client treat the upload as gone glog.Errorf("listMultipartUploads %s error: %v", *input.Bucket, err) - return + return output, s3err.ErrInternalError } uploadsCount := int64(0) @@ -991,8 +1006,13 @@ func (s3a *S3ApiServer) listObjectParts(input *s3.ListPartsInput) (output *ListP entries, isLast, err := s3a.list(s3a.genUploadsFolder(*input.Bucket)+"/"+*input.UploadId, "", fmt.Sprintf("%04d%s", *input.PartNumberMarker, multipartExt), false, uint32(*input.MaxParts)) if err != nil { + // A store that reports the missing upload directory as not-found means + // the upload is gone (completed or aborted), not a store error. + if isFilerListNotFound(err) { + return nil, s3err.ErrNoSuchUpload + } glog.Errorf("listObjectParts %s %s error: %v", *input.Bucket, *input.UploadId, err) - return nil, s3err.ErrNoSuchUpload + return nil, s3err.ErrInternalError } // Note: The upload directory is sort of a marker of the existence of an multipart upload request. diff --git a/weed/s3api/s3_error_utils.go b/weed/s3api/s3_error_utils.go index 0f7b99fdbe..bd38e50b79 100644 --- a/weed/s3api/s3_error_utils.go +++ b/weed/s3api/s3_error_utils.go @@ -1,10 +1,22 @@ package s3api import ( + "errors" + "strings" + "github.com/seaweedfs/seaweedfs/weed/glog" + "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" ) +// isFilerListNotFound reports whether a filer list error is a not-found. +// Unlike lookups (normalized in filer_pb.LookupEntry), list errors cross gRPC +// as raw status errors, so the sentinel survives only as text. Callers must +// pass errors from paths without user-controlled segments that could spoof it. +func isFilerListNotFound(err error) bool { + return errors.Is(err, filer_pb.ErrNotFound) || strings.Contains(err.Error(), filer_pb.ErrNotFound.Error()) +} + // ErrorHandlers provide common error handling patterns for S3 API operations // handleMultipartError logs an error and returns the standard multipart error format diff --git a/weed/s3api/s3api_copy_source_classify_test.go b/weed/s3api/s3api_copy_source_classify_test.go new file mode 100644 index 0000000000..7a40a90510 --- /dev/null +++ b/weed/s3api/s3api_copy_source_classify_test.go @@ -0,0 +1,48 @@ +package s3api + +import ( + "errors" + "fmt" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +func TestClassifyCopySourceError(t *testing.T) { + deleteMarker := &filer_pb.Entry{ + Extended: map[string][]byte{s3_constants.ExtDeleteMarkerKey: []byte("true")}, + } + + tests := []struct { + name string + entry *filer_pb.Entry + err error + want s3err.ErrorCode + }{ + {"regular entry", &filer_pb.Entry{Name: "o"}, nil, s3err.ErrNone}, + {"nil entry", nil, nil, s3err.ErrInvalidCopySource}, + {"directory entry", &filer_pb.Entry{IsDirectory: true}, nil, s3err.ErrInvalidCopySource}, + {"delete marker entry", deleteMarker, nil, s3err.ErrNoSuchKey}, + {"not found sentinel", nil, filer_pb.ErrNotFound, s3err.ErrInvalidCopySource}, + {"wrapped not found", nil, fmt.Errorf("read %s: %w", "o", filer_pb.ErrNotFound), s3err.ErrInvalidCopySource}, + {"grpc not found", nil, status.Error(codes.NotFound, "no entry"), s3err.ErrInvalidCopySource}, + {"invalid version id", nil, errInvalidVersionID, s3err.ErrInvalidCopySource}, + {"delete marker error", nil, ErrDeleteMarker, s3err.ErrInvalidCopySource}, + {"transient unavailable", nil, status.Error(codes.Unavailable, "filer down"), s3err.ErrServiceUnavailable}, + {"store error", nil, errors.New("connection reset"), s3err.ErrInternalError}, + // A message merely mentioning the sentinel text must not downgrade a store error. + {"sentinel text in message", nil, fmt.Errorf("peer said: %s", filer_pb.ErrNotFound.Error()), s3err.ErrInternalError}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := classifyCopySourceError(tt.entry, tt.err); got != tt.want { + t.Errorf("classifyCopySourceError() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/weed/s3api/s3api_object_handlers_copy.go b/weed/s3api/s3api_object_handlers_copy.go index 3da0370de6..e15a3098b0 100644 --- a/weed/s3api/s3api_object_handlers_copy.go +++ b/weed/s3api/s3api_object_handlers_copy.go @@ -24,6 +24,8 @@ import ( "github.com/seaweedfs/seaweedfs/weed/security" "github.com/seaweedfs/seaweedfs/weed/util" util_http "github.com/seaweedfs/seaweedfs/weed/util/http" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" ) @@ -67,6 +69,31 @@ func hasPrefixFold(s, prefix string) bool { return len(s) >= len(prefix) && strings.EqualFold(s[:len(prefix)], prefix) } +// classifyCopySourceError maps a copy-source lookup to an S3 error: a missing, +// invalid, or directory source stays a client error, but a transient store +// error becomes a retryable 5xx so a resumable copy/commit survives a blip. +func classifyCopySourceError(entry *filer_pb.Entry, err error) s3err.ErrorCode { + if err == nil { + if entry == nil || entry.IsDirectory { + return s3err.ErrInvalidCopySource + } + // The latest-version pointer tracks delete markers too; copying one + // must be NoSuchKey like every other handler, not a copy of the stub. + if deleteMarker, ok := entry.Extended[s3_constants.ExtDeleteMarkerKey]; ok && string(deleteMarker) == "true" { + return s3err.ErrNoSuchKey + } + return s3err.ErrNone + } + if errors.Is(err, filer_pb.ErrNotFound) || status.Code(err) == codes.NotFound || + errors.Is(err, errInvalidVersionID) || errors.Is(err, ErrDeleteMarker) { + return s3err.ErrInvalidCopySource + } + if isTransientFilerError(err) { + return s3err.ErrServiceUnavailable + } + return s3err.ErrInternalError +} + func (s3a *S3ApiServer) CopyObjectHandler(w http.ResponseWriter, r *http.Request) { t := time.Now().UTC().Truncate(time.Millisecond) @@ -144,13 +171,19 @@ func (s3a *S3ApiServer) CopyObjectHandler(w http.ResponseWriter, r *http.Request srcVersioningState, err := s3a.getVersioningState(srcBucket) if err != nil { glog.Errorf("Error checking versioning state for source bucket %s: %v", srcBucket, err) - s3err.WriteErrorResponse(w, r, s3err.ErrInvalidCopySource) + // Only a missing bucket is the client's fault; a store error must stay + // retryable, matching the destination-bucket lookup. + if errors.Is(err, filer_pb.ErrNotFound) { + s3err.WriteErrorResponse(w, r, s3err.ErrInvalidCopySource) + return + } + s3err.WriteErrorResponse(w, r, s3err.ErrInternalError) return } entry, err := s3a.resolveCopySourceEntry(srcBucket, srcObject, srcVersionId, srcVersioningState) - if err != nil || entry.IsDirectory { - s3err.WriteErrorResponse(w, r, s3err.ErrInvalidCopySource) + if errCode := classifyCopySourceError(entry, err); errCode != s3err.ErrNone { + s3err.WriteErrorResponse(w, r, errCode) return } @@ -216,8 +249,8 @@ func (s3a *S3ApiServer) CopyObjectHandler(w http.ResponseWriter, r *http.Request routeInPlace := owner != "" && dstVersioningState == "" && !mimeChanged selfCopyBody := func() s3err.ErrorCode { currentEntry, currentErr := s3a.resolveCopySourceEntry(srcBucket, srcObject, srcVersionId, srcVersioningState) - if currentErr != nil || currentEntry.IsDirectory { - return s3err.ErrInvalidCopySource + if errCode := classifyCopySourceError(currentEntry, currentErr); errCode != s3err.ErrNone { + return errCode } if errCode := s3a.validateConditionalCopyHeaders(r, currentEntry); errCode != s3err.ErrNone { return errCode @@ -828,12 +861,22 @@ func (s3a *S3ApiServer) CopyObjectPartHandler(w http.ResponseWriter, r *http.Req s3err.WriteErrorResponse(w, r, s3err.ErrInvalidPart) return } + if partID < 1 { + s3err.WriteErrorResponse(w, r, s3err.ErrInvalidPart) + return + } // Get detailed versioning state for source bucket srcVersioningState, err := s3a.getVersioningState(srcBucket) if err != nil { glog.Errorf("Error checking versioning state for source bucket %s: %v", srcBucket, err) - s3err.WriteErrorResponse(w, r, s3err.ErrInvalidCopySource) + // Only a missing bucket is the client's fault; a store error must stay + // retryable, matching the destination-bucket lookup. + if errors.Is(err, filer_pb.ErrNotFound) { + s3err.WriteErrorResponse(w, r, s3err.ErrInvalidCopySource) + return + } + s3err.WriteErrorResponse(w, r, s3err.ErrInternalError) return } @@ -856,8 +899,8 @@ func (s3a *S3ApiServer) CopyObjectPartHandler(w http.ResponseWriter, r *http.Req entry, err = s3a.getEntry(dir, name) } - if err != nil || entry.IsDirectory { - s3err.WriteErrorResponse(w, r, s3err.ErrInvalidCopySource) + if errCode := classifyCopySourceError(entry, err); errCode != s3err.ErrNone { + s3err.WriteErrorResponse(w, r, errCode) return } diff --git a/weed/s3api/s3api_object_versioning.go b/weed/s3api/s3api_object_versioning.go index fce90a8250..bea7dfb48d 100644 --- a/weed/s3api/s3api_object_versioning.go +++ b/weed/s3api/s3api_object_versioning.go @@ -1849,7 +1849,7 @@ func (s3a *S3ApiServer) recoverLatestVersionWithoutPointer(bucket, normalizedObj return nil, healErr } - return nil, fmt.Errorf("no version metadata in .versions directory and no regular object found for %s/%s", bucket, normalizedObject) + return nil, fmt.Errorf("no version metadata in .versions directory and no regular object found for %s/%s: %w", bucket, normalizedObject, filer_pb.ErrNotFound) } // versionIdFromEntry returns a .versions child entry's version id, preferring