From 8d4f0f440fc9f777682291ea43ad8feb2cf6f045 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 17 May 2023 15:32:46 +0200 Subject: [PATCH] harden uploads (#3899) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- changelog/unreleased/harden-uploads.md | 5 +++ .../http/services/appprovider/appprovider.go | 35 +++++++++++-------- internal/http/services/owncloud/ocdav/tus.go | 6 ++++ pkg/rhttp/datatx/manager/tus/tus.go | 12 +++++-- .../utils/decomposedfs/upload/processing.go | 12 ++++--- 5 files changed, 50 insertions(+), 20 deletions(-) create mode 100644 changelog/unreleased/harden-uploads.md diff --git a/changelog/unreleased/harden-uploads.md b/changelog/unreleased/harden-uploads.md new file mode 100644 index 00000000000..c68cb9b7138 --- /dev/null +++ b/changelog/unreleased/harden-uploads.md @@ -0,0 +1,5 @@ +Bugfix: harden uploads + +Uploads now check response headers for a file id and omit a subsequent stat request which might land on a storage provider that does not yet see the new file due to latency, eg. when NFS caches direntries. + +https://github.com/cs3org/reva/pull/3899 \ No newline at end of file diff --git a/internal/http/services/appprovider/appprovider.go b/internal/http/services/appprovider/appprovider.go index 1f79b14f19a..c121657885a 100644 --- a/internal/http/services/appprovider/appprovider.go +++ b/internal/http/services/appprovider/appprovider.go @@ -33,6 +33,7 @@ import ( provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" typespb "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" "github.com/cs3org/reva/v2/internal/http/services/datagateway" + "github.com/cs3org/reva/v2/internal/http/services/owncloud/ocdav/net" ctxpkg "github.com/cs3org/reva/v2/pkg/ctx" "github.com/cs3org/reva/v2/pkg/rgrpc/status" "github.com/cs3org/reva/v2/pkg/rgrpc/todo/pool" @@ -283,26 +284,32 @@ func (s *svc) handleNew(w http.ResponseWriter, r *http.Request) { return } - // Stat the newly created file - statRes, err := client.Stat(ctx, statFileReq) - if err != nil { - writeError(w, r, appErrorServerError, "statting the created file failed", err) - return - } + var fileid string + if httpRes.Header.Get(net.HeaderOCFileID) != "" { + fileid = httpRes.Header.Get(net.HeaderOCFileID) + } else { + // Stat the newly created file + statRes, err := client.Stat(ctx, statFileReq) + if err != nil { + writeError(w, r, appErrorServerError, "statting the created file failed", err) + return + } - if statRes.Status.Code != rpc.Code_CODE_OK { - writeError(w, r, appErrorServerError, "statting the created file failed", nil) - return - } + if statRes.Status.Code != rpc.Code_CODE_OK { + writeError(w, r, appErrorServerError, "statting the created file failed", nil) + return + } - if statRes.Info.Type != provider.ResourceType_RESOURCE_TYPE_FILE { - writeError(w, r, appErrorInvalidParameter, "the given file id does not point to a file", nil) - return + if statRes.Info.Type != provider.ResourceType_RESOURCE_TYPE_FILE { + writeError(w, r, appErrorInvalidParameter, "the given file id does not point to a file", nil) + return + } + fileid = storagespace.FormatResourceID(*statRes.Info.Id) } js, err := json.Marshal( map[string]interface{}{ - "file_id": storagespace.FormatResourceID(*statRes.Info.Id), + "file_id": fileid, }, ) if err != nil { diff --git a/internal/http/services/owncloud/ocdav/tus.go b/internal/http/services/owncloud/ocdav/tus.go index af5ea00e63c..14a6cd2dea4 100644 --- a/internal/http/services/owncloud/ocdav/tus.go +++ b/internal/http/services/owncloud/ocdav/tus.go @@ -278,6 +278,12 @@ func (s *svc) handleTusPost(ctx context.Context, w http.ResponseWriter, r *http. if length == 0 || httpRes.Header.Get(net.HeaderUploadOffset) == r.Header.Get(net.HeaderUploadLength) { // get uploaded file metadata + if resid, err := storagespace.ParseID(httpRes.Header.Get(net.HeaderOCFileID)); err == nil { + sReq.Ref = &provider.Reference{ + ResourceId: &resid, + } + } + sRes, err := s.gwClient.Stat(ctx, sReq) if err != nil { log.Error().Err(err).Msg("error sending grpc stat request") diff --git a/pkg/rhttp/datatx/manager/tus/tus.go b/pkg/rhttp/datatx/manager/tus/tus.go index 0a5a626d666..f98ce8adb10 100644 --- a/pkg/rhttp/datatx/manager/tus/tus.go +++ b/pkg/rhttp/datatx/manager/tus/tus.go @@ -40,6 +40,7 @@ import ( "github.com/cs3org/reva/v2/pkg/rhttp/datatx/metrics" "github.com/cs3org/reva/v2/pkg/storage" "github.com/cs3org/reva/v2/pkg/storage/cache" + "github.com/cs3org/reva/v2/pkg/storagespace" "github.com/cs3org/reva/v2/pkg/utils" "github.com/mitchellh/mapstructure" ) @@ -144,6 +145,7 @@ func (m *manager) Handler(fs storage.FS) (http.Handler, error) { metrics.UploadsActive.Sub(1) }() // set etag, mtime and file id + setHeaders(fs, w, r) handler.PostFile(w, r) case "HEAD": handler.HeadFile(w, r) @@ -153,7 +155,7 @@ func (m *manager) Handler(fs storage.FS) (http.Handler, error) { metrics.UploadsActive.Sub(1) }() // set etag, mtime and file id - setExpiresHeader(fs, w, r) + setHeaders(fs, w, r) handler.PatchFile(w, r) case "DELETE": handler.DelFile(w, r) @@ -180,7 +182,7 @@ type composable interface { UseIn(composer *tusd.StoreComposer) } -func setExpiresHeader(fs storage.FS, w http.ResponseWriter, r *http.Request) { +func setHeaders(fs storage.FS, w http.ResponseWriter, r *http.Request) { ctx := r.Context() id := path.Base(r.URL.Path) datastore, ok := fs.(tusd.DataStore) @@ -202,4 +204,10 @@ func setExpiresHeader(fs storage.FS, w http.ResponseWriter, r *http.Request) { if expires != "" { w.Header().Set(net.HeaderTusUploadExpires, expires) } + resourceid := provider.ResourceId{ + StorageId: info.MetaData["providerID"], + SpaceId: info.Storage["SpaceRoot"], + OpaqueId: info.Storage["NodeId"], + } + w.Header().Set(net.HeaderOCFileID, storagespace.FormatResourceID(resourceid)) } diff --git a/pkg/storage/utils/decomposedfs/upload/processing.go b/pkg/storage/utils/decomposedfs/upload/processing.go index eab37f52097..3ba1a044519 100644 --- a/pkg/storage/utils/decomposedfs/upload/processing.go +++ b/pkg/storage/utils/decomposedfs/upload/processing.go @@ -154,6 +154,7 @@ func New(ctx context.Context, info tusd.FileInfo, lu *lookup.Lookup, tp Tree, p "BinPath": binPath, "NodeId": n.ID, + "NodeExists": "true", "NodeParentId": n.ParentID, "NodeName": n.Name, "SpaceRoot": spaceRoot, @@ -166,6 +167,11 @@ func New(ctx context.Context, info tusd.FileInfo, lu *lookup.Lookup, tp Tree, p "LogLevel": log.GetLevel().String(), } + if !n.Exists { + // fill future node info + info.Storage["NodeId"] = uuid.New().String() + info.Storage["NodeExists"] = "false" + } // Create binary file in the upload folder with no content log.Debug().Interface("info", info).Msg("Decomposedfs: built storage info") file, err := os.OpenFile(binPath, os.O_CREATE|os.O_WRONLY, defaultFilePerm) @@ -267,8 +273,8 @@ func CreateNodeForUpload(upload *Upload, initAttrs node.Attributes) (*node.Node, } var f *lockedfile.File - switch n.ID { - case "": + switch upload.Info.Storage["NodeExists"] { + case "false": f, err = initNewNode(upload, n, uint64(fsize)) default: f, err = updateExistingNode(upload, n, spaceID, uint64(fsize)) @@ -320,8 +326,6 @@ func CreateNodeForUpload(upload *Upload, initAttrs node.Attributes) (*node.Node, } func initNewNode(upload *Upload, n *node.Node, fsize uint64) (*lockedfile.File, error) { - n.ID = uuid.New().String() - // create folder structure (if needed) if err := os.MkdirAll(filepath.Dir(n.InternalPath()), 0700); err != nil { return nil, err