From c13d9b17a2149ac00b110b2a278777b46f3a6e51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Tue, 10 Aug 2021 14:03:01 +0000 Subject: [PATCH] call CreateDir without splitting name from ref MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .drone.star | 62 ------------------- .../storageprovider/storageprovider.go | 47 ++++---------- pkg/storage/fs/owncloud/owncloud.go | 11 ++-- pkg/storage/fs/owncloudsql/owncloudsql.go | 18 +++--- pkg/storage/fs/s3/s3.go | 9 ++- pkg/storage/storage.go | 2 +- .../utils/decomposedfs/decomposedfs.go | 10 ++- .../decomposedfs_concurrency_test.go | 4 +- pkg/storage/utils/decomposedfs/lookup.go | 1 - .../utils/decomposedfs/testhelpers/helpers.go | 9 ++- pkg/storage/utils/eosfs/eosfs.go | 5 +- pkg/storage/utils/localfs/localfs.go | 5 +- 12 files changed, 50 insertions(+), 133 deletions(-) diff --git a/.drone.star b/.drone.star index af8d7a7f22..c11e9fac7e 100644 --- a/.drone.star +++ b/.drone.star @@ -103,7 +103,6 @@ def main(ctx): litmusOcisOldWebdav(), litmusOcisNewWebdav(), litmusOcisSpacesDav(), - localIntegrationTestsOcis(), ] + ocisIntegrationTests(6) + s3ngIntegrationTests(12) @@ -574,67 +573,6 @@ def litmusOcisSpacesDav(): ] }, ], - } - -def localIntegrationTestsOcis(): - return { - "kind": "pipeline", - "type": "docker", - "name": "local-integration-tests-ocis", - "platform": { - "os": "linux", - "arch": "amd64", - }, - "trigger": { - "event": { - "include": [ - "pull_request", - "tag", - ], - }, - }, - "steps": [ - makeStep("build-ci"), - { - "name": "revad-services", - "image": "registry.cern.ch/docker.io/library/golang:1.16", - "detach": True, - "commands": [ - "cd /drone/src/tests/oc-integration-tests/drone/", - "/drone/src/cmd/revad/revad -c frontend.toml &", - "/drone/src/cmd/revad/revad -c gateway.toml &", - "/drone/src/cmd/revad/revad -c shares.toml &", - "/drone/src/cmd/revad/revad -c storage-home-ocis.toml &", - "/drone/src/cmd/revad/revad -c storage-oc-ocis.toml &", - "/drone/src/cmd/revad/revad -c storage-publiclink-ocis.toml &", - "/drone/src/cmd/revad/revad -c ldap-users.toml", - ], - }, - cloneOc10TestReposStep(), - { - "name": "localAPIAcceptanceTestsOcisStorage", - "image": "registry.cern.ch/docker.io/owncloudci/php:7.4", - "commands": [ - "make test-acceptance-api", - ], - "environment": { - "TEST_SERVER_URL": "http://revad-services:20080", - "OCIS_REVA_DATA_ROOT": "/drone/src/tmp/reva/data/", - "DELETE_USER_DATA_CMD": "rm -rf /drone/src/tmp/reva/data/nodes/root/* /drone/src/tmp/reva/data/nodes/*-*-*-*", - "STORAGE_DRIVER": "OCIS", - "SKELETON_DIR": "/drone/src/tmp/testing/data/apiSkeleton", - "TEST_WITH_LDAP": "true", - "REVA_LDAP_HOSTNAME": "ldap", - "TEST_REVA": "true", - "SEND_SCENARIO_LINE_REFERENCES": "true", - "BEHAT_FILTER_TAGS": "~@skipOnOcis-OCIS-Storage", - "PATH_TO_CORE": "/drone/src/tmp/testrunner", - } - }, - ], - "services": [ - ldapService(), - ], } def ocisIntegrationTests(parallelRuns, skipExceptParts = []): diff --git a/internal/grpc/services/storageprovider/storageprovider.go b/internal/grpc/services/storageprovider/storageprovider.go index 0f8db73fee..52d5154415 100644 --- a/internal/grpc/services/storageprovider/storageprovider.go +++ b/internal/grpc/services/storageprovider/storageprovider.go @@ -500,27 +500,13 @@ func (s *service) DeleteStorageSpace(ctx context.Context, req *provider.DeleteSt } func (s *service) CreateContainer(ctx context.Context, req *provider.CreateContainerRequest) (*provider.CreateContainerResponse, error) { - var parentRef *provider.Reference - var name string - switch { - case utils.IsRelativeReference(req.Ref): - req.Ref.Path, name = path.Split(req.Ref.Path) - parentRef = req.Ref - case utils.IsAbsoluteReference(req.Ref): - ref, err := s.unwrap(ctx, req.Ref) - if err != nil { - return &provider.CreateContainerResponse{ - Status: status.NewInternal(ctx, err, "error unwrapping path"), - }, nil - } - parentRef = &provider.Reference{Path: path.Dir(ref.GetPath())} - name = path.Base(ref.GetPath()) - default: + newRef, err := s.unwrap(ctx, req.Ref) + if err != nil { return &provider.CreateContainerResponse{ - Status: status.NewInvalidArg(ctx, "invalid reference, name required"), + Status: status.NewInternal(ctx, err, "error unwrapping path"), }, nil } - if err := s.storage.CreateDir(ctx, parentRef, name); err != nil { + if err := s.storage.CreateDir(ctx, newRef); err != nil { var st *rpc.Status switch err.(type) { case errtypes.IsNotFound: @@ -590,13 +576,13 @@ func (s *service) Move(ctx context.Context, req *provider.MoveRequest) (*provide sourceRef, err := s.unwrap(ctx, req.Source) if err != nil { return &provider.MoveResponse{ - Status: status.NewInvalid(ctx, err.Error()), + Status: status.NewInternal(ctx, err, "error unwrapping source path"), }, nil } targetRef, err := s.unwrap(ctx, req.Destination) if err != nil { return &provider.MoveResponse{ - Status: status.NewInvalid(ctx, err.Error()), + Status: status.NewInternal(ctx, err, "error unwrapping destination path"), }, nil } @@ -630,23 +616,15 @@ func (s *service) Stat(ctx context.Context, req *provider.StatRequest) (*provide ) newRef, err := s.unwrap(ctx, req.Ref) - var st *rpc.Status if err != nil { - switch err.(type) { - case errtypes.IsNotFound: - st = status.NewNotFound(ctx, "path not found when unwrapping") - case errtypes.PermissionDenied: - st = status.NewPermissionDenied(ctx, err, "permission denied") - default: - st = status.NewInternal(ctx, err, "error unwrapping: "+req.String()) - } return &provider.StatResponse{ - Status: st, + Status: status.NewInternal(ctx, err, "error unwrapping path"), }, nil } md, err := s.storage.GetMD(ctx, newRef, req.ArbitraryMetadataKeys) if err != nil { + var st *rpc.Status switch err.(type) { case errtypes.IsNotFound: st = status.NewNotFound(ctx, "path not found when stating") @@ -1237,13 +1215,12 @@ func (s *service) unwrap(ctx context.Context, ref *provider.Reference) (*provide // there are two cases: // 1. absolute id references (resource_id is set, path is empty) // 2. relative references (resource_id is set, path starts with a `.`) - if ref.ResourceId != nil { + if ref.GetResourceId() != nil { return ref, nil } - // if the if !strings.HasPrefix(ref.GetPath(), "/") { - // abort, absolute path references must start with a `/`` + // abort, absolute path references must start with a `/` return nil, errtypes.BadRequest("ref is invalid: " + ref.String()) } @@ -1254,7 +1231,9 @@ func (s *service) unwrap(ctx context.Context, ref *provider.Reference) (*provide return nil, err } - return &provider.Reference{Path: fsfn}, nil + pathRef := &provider.Reference{Path: fsfn} + + return pathRef, nil } func (s *service) trimMountPrefix(fn string) (string, error) { diff --git a/pkg/storage/fs/owncloud/owncloud.go b/pkg/storage/fs/owncloud/owncloud.go index a383bc0efb..8eba17ca41 100644 --- a/pkg/storage/fs/owncloud/owncloud.go +++ b/pkg/storage/fs/owncloud/owncloud.go @@ -1152,13 +1152,12 @@ func (fs *ocfs) GetHome(ctx context.Context) (string, error) { return "", nil } -func (fs *ocfs) CreateDir(ctx context.Context, ref *provider.Reference, name string) (err error) { +func (fs *ocfs) CreateDir(ctx context.Context, ref *provider.Reference) (err error) { - dir, err := fs.resolve(ctx, ref) + ip, err := fs.resolve(ctx, ref) if err != nil { return err } - ip := filepath.Join(dir, name) // check permissions of parent dir if perm, err := fs.readPermissions(ctx, filepath.Dir(ip)); err == nil { @@ -1167,17 +1166,17 @@ func (fs *ocfs) CreateDir(ctx context.Context, ref *provider.Reference, name str } } else { if isNotFound(err) { - return errtypes.NotFound(fs.toStoragePath(ctx, filepath.Dir(ip))) + return errtypes.NotFound(ref.Path) } return errors.Wrap(err, "ocfs: error reading permissions") } if err = os.Mkdir(ip, 0700); err != nil { if os.IsNotExist(err) { - return errtypes.NotFound(fs.toStoragePath(ctx, filepath.Dir(ip))) + return errtypes.NotFound(ref.Path) } // FIXME we also need already exists error, webdav expects 405 MethodNotAllowed - return errors.Wrap(err, "ocfs: error creating dir "+ip) + return errors.Wrap(err, "ocfs: error creating dir "+ref.Path) } return fs.propagate(ctx, ip) } diff --git a/pkg/storage/fs/owncloudsql/owncloudsql.go b/pkg/storage/fs/owncloudsql/owncloudsql.go index 860ebff74b..e2a8539c63 100644 --- a/pkg/storage/fs/owncloudsql/owncloudsql.go +++ b/pkg/storage/fs/owncloudsql/owncloudsql.go @@ -582,10 +582,6 @@ func (fs *owncloudsqlfs) resolve(ctx context.Context, ref *provider.Reference) ( return fs.toInternalPath(ctx, ref.GetPath()), nil } - if ref.GetPath() != "" { - return fs.toInternalPath(ctx, ref.GetPath()), nil - } - // reference is invalid return "", fmt.Errorf("invalid reference %+v", ref) } @@ -705,8 +701,12 @@ func (fs *owncloudsqlfs) GetHome(ctx context.Context) (string, error) { return "", nil } -func (fs *owncloudsqlfs) CreateDir(ctx context.Context, ref *provider.Reference, sp string) (err error) { - ip := fs.toInternalPath(ctx, sp) +func (fs *owncloudsqlfs) CreateDir(ctx context.Context, ref *provider.Reference) (err error) { + + ip, err := fs.resolve(ctx, ref) + if err != nil { + return err + } // check permissions of parent dir if perm, err := fs.readPermissions(ctx, filepath.Dir(ip)); err == nil { @@ -715,17 +715,17 @@ func (fs *owncloudsqlfs) CreateDir(ctx context.Context, ref *provider.Reference, } } else { if isNotFound(err) { - return errtypes.NotFound(fs.toStoragePath(ctx, filepath.Dir(ip))) + return errtypes.NotFound(ref.Path) } return errors.Wrap(err, "owncloudsql: error reading permissions") } if err = os.Mkdir(ip, 0700); err != nil { if os.IsNotExist(err) { - return errtypes.NotFound(sp) + return errtypes.NotFound(ref.Path) } // FIXME we also need already exists error, webdav expects 405 MethodNotAllowed - return errors.Wrap(err, "owncloudsql: error creating dir "+ip) + return errors.Wrap(err, "owncloudsql: error creating dir "+fs.toStoragePath(ctx, filepath.Dir(ip))) } fi, err := os.Stat(ip) diff --git a/pkg/storage/fs/s3/s3.go b/pkg/storage/fs/s3/s3.go index 85f643a6c1..eaf717562a 100644 --- a/pkg/storage/fs/s3/s3.go +++ b/pkg/storage/fs/s3/s3.go @@ -293,14 +293,13 @@ func (fs *s3FS) CreateHome(ctx context.Context) error { return errtypes.NotSupported("s3fs: not supported") } -func (fs *s3FS) CreateDir(ctx context.Context, ref *provider.Reference, name string) error { +func (fs *s3FS) CreateDir(ctx context.Context, ref *provider.Reference) error { log := appctx.GetLogger(ctx) - dir, err := fs.resolve(ctx, ref) + fn, err := fs.resolve(ctx, ref) if err != nil { return nil } - fn := path.Join(dir, name) fn = fs.addRoot(fn) + "/" // append / to indicate folder // TODO only if fn does not end in / @@ -316,11 +315,11 @@ func (fs *s3FS) CreateDir(ctx context.Context, ref *provider.Reference, name str log.Error().Err(err) if aerr, ok := err.(awserr.Error); ok { if aerr.Code() == s3.ErrCodeNoSuchBucket { - return errtypes.NotFound(fn) + return errtypes.NotFound(ref.Path) } } // FIXME we also need already exists error, webdav expects 405 MethodNotAllowed - return errors.Wrap(err, "s3fs: error creating dir "+fn) + return errors.Wrap(err, "s3fs: error creating dir "+ref.Path) } log.Debug().Interface("result", result) // todo cache etag? diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index 4ce00a4e2f..1b65c30fdd 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -31,7 +31,7 @@ import ( type FS interface { GetHome(ctx context.Context) (string, error) CreateHome(ctx context.Context) error - CreateDir(ctx context.Context, ref *provider.Reference, name string) error + CreateDir(ctx context.Context, ref *provider.Reference) error Delete(ctx context.Context, ref *provider.Reference) error Move(ctx context.Context, oldRef, newRef *provider.Reference) error GetMD(ctx context.Context, ref *provider.Reference, mdKeys []string) (*provider.ResourceInfo, error) diff --git a/pkg/storage/utils/decomposedfs/decomposedfs.go b/pkg/storage/utils/decomposedfs/decomposedfs.go index 8f783e2c41..d851323f85 100644 --- a/pkg/storage/utils/decomposedfs/decomposedfs.go +++ b/pkg/storage/utils/decomposedfs/decomposedfs.go @@ -27,6 +27,7 @@ import ( "math" "net/url" "os" + "path" "path/filepath" "strconv" "strings" @@ -278,7 +279,12 @@ func (fs *Decomposedfs) GetPathByID(ctx context.Context, id *provider.ResourceId } // CreateDir creates the specified directory -func (fs *Decomposedfs) CreateDir(ctx context.Context, ref *provider.Reference, name string) (err error) { +func (fs *Decomposedfs) CreateDir(ctx context.Context, ref *provider.Reference) (err error) { + name := path.Base(ref.Path) + if name == "" || name == "." || name == "/" { + return errtypes.BadRequest("Invalid path") + } + ref.Path = path.Dir(ref.Path) var n *node.Node if n, err = fs.lu.NodeFromResource(ctx, ref); err != nil { return @@ -288,7 +294,7 @@ func (fs *Decomposedfs) CreateDir(ctx context.Context, ref *provider.Reference, } if n.Exists { - return errtypes.AlreadyExists(name) + return errtypes.AlreadyExists(ref.Path) } pn, err := n.Parent() if err != nil { diff --git a/pkg/storage/utils/decomposedfs/decomposedfs_concurrency_test.go b/pkg/storage/utils/decomposedfs/decomposedfs_concurrency_test.go index ec09d955eb..00d4de1e79 100644 --- a/pkg/storage/utils/decomposedfs/decomposedfs_concurrency_test.go +++ b/pkg/storage/utils/decomposedfs/decomposedfs_concurrency_test.go @@ -139,9 +139,9 @@ var _ = Describe("Decomposed", func() { for i := 0; i < 10; i++ { go func() { defer GinkgoRecover() - err := fs.CreateDir(ctx, &provider.Reference{Path: "."}, "fightforit") + err := fs.CreateDir(ctx, &provider.Reference{Path: "/fightforit"}) if err != nil { - rinfo, err := fs.GetMD(ctx, &provider.Reference{Path: "fightforit"}, nil) + rinfo, err := fs.GetMD(ctx, &provider.Reference{Path: "/fightforit"}, nil) Expect(err).ToNot(HaveOccurred()) Expect(rinfo).ToNot(BeNil()) } diff --git a/pkg/storage/utils/decomposedfs/lookup.go b/pkg/storage/utils/decomposedfs/lookup.go index f9ff44f66b..bcd9ca72e7 100644 --- a/pkg/storage/utils/decomposedfs/lookup.go +++ b/pkg/storage/utils/decomposedfs/lookup.go @@ -40,7 +40,6 @@ type Lookup struct { // NodeFromResource takes in a request path or request id and converts it to a Node func (lu *Lookup) NodeFromResource(ctx context.Context, ref *provider.Reference) (*node.Node, error) { - if ref.ResourceId != nil { // check if a storage space reference is used // currently, the decomposed fs uses the root node id as the space id diff --git a/pkg/storage/utils/decomposedfs/testhelpers/helpers.go b/pkg/storage/utils/decomposedfs/testhelpers/helpers.go index 3f785020fe..3d189f1177 100644 --- a/pkg/storage/utils/decomposedfs/testhelpers/helpers.go +++ b/pkg/storage/utils/decomposedfs/testhelpers/helpers.go @@ -21,7 +21,6 @@ package helpers import ( "context" "os" - "path" "path/filepath" "github.com/google/uuid" @@ -114,7 +113,7 @@ func NewTestEnv() (*TestEnv, error) { } // Create dir1 - dir1, err := env.CreateTestDir("dir1") + dir1, err := env.CreateTestDir("/dir1") if err != nil { return nil, err } @@ -126,13 +125,13 @@ func NewTestEnv() (*TestEnv, error) { } // Create subdir1 in dir1 - err = fs.CreateDir(ctx, &providerv1beta1.Reference{Path: "dir1"}, "subdir1") + err = fs.CreateDir(ctx, &providerv1beta1.Reference{Path: "/dir1/subdir1"}) if err != nil { return nil, err } // Create emptydir - err = fs.CreateDir(ctx, &providerv1beta1.Reference{Path: "."}, "emptydir") + err = fs.CreateDir(ctx, &providerv1beta1.Reference{Path: "/emptydir"}) if err != nil { return nil, err } @@ -147,7 +146,7 @@ func (t *TestEnv) Cleanup() { // CreateTestDir create a directory and returns a corresponding Node func (t *TestEnv) CreateTestDir(name string) (*node.Node, error) { - err := t.Fs.CreateDir(t.Ctx, &providerv1beta1.Reference{Path: path.Dir(name)}, path.Base(name)) + err := t.Fs.CreateDir(t.Ctx, &providerv1beta1.Reference{Path: name}) if err != nil { return nil, err } diff --git a/pkg/storage/utils/eosfs/eosfs.go b/pkg/storage/utils/eosfs/eosfs.go index 7c181ba834..73b66c9b71 100644 --- a/pkg/storage/utils/eosfs/eosfs.go +++ b/pkg/storage/utils/eosfs/eosfs.go @@ -1158,18 +1158,17 @@ func (fs *eosfs) createUserDir(ctx context.Context, u *userpb.User, path string, return nil } -func (fs *eosfs) CreateDir(ctx context.Context, ref *provider.Reference, name string) error { +func (fs *eosfs) CreateDir(ctx context.Context, ref *provider.Reference) error { log := appctx.GetLogger(ctx) u, err := getUser(ctx) if err != nil { return errors.Wrap(err, "eosfs: no user in ctx") } - dir, err := fs.resolve(ctx, ref) + p, err := fs.resolve(ctx, ref) if err != nil { return nil } - p := path.Join(dir, name) auth, err := fs.getUserAuth(ctx, u, p) if err != nil { return err diff --git a/pkg/storage/utils/localfs/localfs.go b/pkg/storage/utils/localfs/localfs.go index eca07839c8..2af306dde0 100644 --- a/pkg/storage/utils/localfs/localfs.go +++ b/pkg/storage/utils/localfs/localfs.go @@ -751,13 +751,12 @@ func (fs *localfs) createHomeInternal(ctx context.Context, fn string) error { return nil } -func (fs *localfs) CreateDir(ctx context.Context, ref *provider.Reference, name string) error { +func (fs *localfs) CreateDir(ctx context.Context, ref *provider.Reference) error { - dir, err := fs.resolve(ctx, ref) + fn, err := fs.resolve(ctx, ref) if err != nil { return nil } - fn := path.Join(dir, name) if fs.isShareFolder(ctx, fn) { return errtypes.PermissionDenied("localfs: cannot create folder under the share folder")