From 11c7feae2dd17ac4cadf680250135433d653897f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 8 Dec 2021 21:33:49 +0000 Subject: [PATCH 1/3] allow listing all storage spaces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../spaces-registry-allow-list-all.md | 5 ++ pkg/storage/registry/spaces/spaces.go | 53 ++++++++++--------- 2 files changed, 32 insertions(+), 26 deletions(-) create mode 100644 changelog/unreleased/spaces-registry-allow-list-all.md diff --git a/changelog/unreleased/spaces-registry-allow-list-all.md b/changelog/unreleased/spaces-registry-allow-list-all.md new file mode 100644 index 0000000000..096e4deeeb --- /dev/null +++ b/changelog/unreleased/spaces-registry-allow-list-all.md @@ -0,0 +1,5 @@ +Enhancement: allow listing all storage spaces + +To implement the drives api we allow listing all spaces by sending a spaceid `*!*`. This is a workaround until a proper filter to list all storage spaces a user has access to / can manage is implemented, or we make that the implicit default filter. + +https://github.com/cs3org/reva/pull/2344 diff --git a/pkg/storage/registry/spaces/spaces.go b/pkg/storage/registry/spaces/spaces.go index 079eeb4974..8890415597 100644 --- a/pkg/storage/registry/spaces/spaces.go +++ b/pkg/storage/registry/spaces/spaces.go @@ -275,6 +275,7 @@ func (r *registry) ListProviders(ctx context.Context, filters map[string]string) return r.findProvidersForResource(ctx, filters["storage_id"]+"!"+filters["opaque_id"]), nil case filters["path"] != "": return r.findProvidersForAbsolutePathReference(ctx, filters["path"]), nil + // TODO add filter for all spaces the user can manage? } return []*registrypb.ProviderInfo{}, nil } @@ -284,6 +285,7 @@ func (r *registry) ListProviders(ctx context.Context, filters map[string]string) // for share spaces the res.StorageId tells the registry the spaceid and res.OpaqueId is a node in that space func (r *registry) findProvidersForResource(ctx context.Context, id string) []*registrypb.ProviderInfo { currentUser := ctxpkg.ContextMustGetUser(ctx) + providerInfos := []*registrypb.ProviderInfo{} for address, provider := range r.c.Providers { p := ®istrypb.ProviderInfo{ Address: address, @@ -312,36 +314,35 @@ func (r *registry) findProvidersForResource(ctx context.Context, id string) []*r continue } - switch len(spaces) { - case 0: - // nothing to do, will continue with next provider - case 1: - space := spaces[0] - for spaceType, sc := range provider.Spaces { - if spaceType == space.SpaceType { - providerPath, err := sc.SpacePath(currentUser, space) - if err != nil { - appctx.GetLogger(ctx).Error().Err(err).Interface("provider", provider).Interface("space", space).Msg("failed to execute template, continuing") - continue - } - spacePaths := map[string]string{ - space.Id.OpaqueId: providerPath, - } - p.Opaque, err = spacePathsToOpaque(spacePaths) - if err != nil { - appctx.GetLogger(ctx).Debug().Err(err).Msg("marshaling space paths map failed, continuing") - continue - } - return []*registrypb.ProviderInfo{p} - } + // build the correct path for every space + spacePaths := map[string]string{} + for _, space := range spaces { + var sc *spaceConfig + var ok bool + if sc, ok = provider.Spaces[space.SpaceType]; !ok { + continue + } + spacePaths[space.Id.OpaqueId], err = sc.SpacePath(currentUser, space) + if err != nil { + appctx.GetLogger(ctx).Error().Err(err).Interface("provider", provider).Interface("space", space).Msg("failed to execute template, continuing") + continue } - default: - // there should not be multiple spaces with the same id per provider - appctx.GetLogger(ctx).Error().Err(err).Interface("provider", provider).Interface("spaces", spaces).Msg("multiple spaces returned, ignoring") } + if len(spacePaths) == 0 { + // do not add provider to result + continue + } + + // add spaces to providerinfo + p.Opaque, err = spacePathsToOpaque(spacePaths) + if err != nil { + appctx.GetLogger(ctx).Debug().Err(err).Msg("marshaling space paths map failed, continuing") + continue + } + providerInfos = append(providerInfos, p) } - return []*registrypb.ProviderInfo{} + return providerInfos } // findProvidersForAbsolutePathReference takes a path and ruturns the storage provider with the longest matching path prefix From a2d55c717ebedb8cad61ba8f424248a1924ebc0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Thu, 9 Dec 2021 13:52:54 +0000 Subject: [PATCH 2/3] introduce dumb findAllProviders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- pkg/storage/registry/spaces/spaces.go | 72 ++++++++++++++++----------- 1 file changed, 44 insertions(+), 28 deletions(-) diff --git a/pkg/storage/registry/spaces/spaces.go b/pkg/storage/registry/spaces/spaces.go index 8890415597..e9801086d8 100644 --- a/pkg/storage/registry/spaces/spaces.go +++ b/pkg/storage/registry/spaces/spaces.go @@ -276,6 +276,9 @@ func (r *registry) ListProviders(ctx context.Context, filters map[string]string) case filters["path"] != "": return r.findProvidersForAbsolutePathReference(ctx, filters["path"]), nil // TODO add filter for all spaces the user can manage? + case len(filters) == 0: + // return all providers + return r.findAllProviders(ctx), nil } return []*registrypb.ProviderInfo{}, nil } @@ -285,7 +288,6 @@ func (r *registry) ListProviders(ctx context.Context, filters map[string]string) // for share spaces the res.StorageId tells the registry the spaceid and res.OpaqueId is a node in that space func (r *registry) findProvidersForResource(ctx context.Context, id string) []*registrypb.ProviderInfo { currentUser := ctxpkg.ContextMustGetUser(ctx) - providerInfos := []*registrypb.ProviderInfo{} for address, provider := range r.c.Providers { p := ®istrypb.ProviderInfo{ Address: address, @@ -314,38 +316,40 @@ func (r *registry) findProvidersForResource(ctx context.Context, id string) []*r continue } - // build the correct path for every space - spacePaths := map[string]string{} - for _, space := range spaces { - var sc *spaceConfig - var ok bool - if sc, ok = provider.Spaces[space.SpaceType]; !ok { - continue - } - spacePaths[space.Id.OpaqueId], err = sc.SpacePath(currentUser, space) - if err != nil { - appctx.GetLogger(ctx).Error().Err(err).Interface("provider", provider).Interface("space", space).Msg("failed to execute template, continuing") - continue + switch len(spaces) { + case 0: + // nothing to do, will continue with next provider + case 1: + space := spaces[0] + for spaceType, sc := range provider.Spaces { + if spaceType == space.SpaceType { + providerPath, err := sc.SpacePath(currentUser, space) + if err != nil { + appctx.GetLogger(ctx).Error().Err(err).Interface("provider", provider).Interface("space", space).Msg("failed to execute template, continuing") + continue + } + spacePaths := map[string]string{ + space.Id.OpaqueId: providerPath, + } + p.Opaque, err = spacePathsToOpaque(spacePaths) + if err != nil { + appctx.GetLogger(ctx).Debug().Err(err).Msg("marshaling space paths map failed, continuing") + continue + } + // we can stop after we found the first space + // TODO to improve lookup time the registry could cache which provider last was responsible for a space? could be invalidated by simple ttl? would that work for shares? + return []*registrypb.ProviderInfo{p} + } } + default: + // there should not be multiple spaces with the same id per provider + appctx.GetLogger(ctx).Error().Err(err).Interface("provider", provider).Interface("spaces", spaces).Msg("multiple spaces returned, ignoring") } - - if len(spacePaths) == 0 { - // do not add provider to result - continue - } - - // add spaces to providerinfo - p.Opaque, err = spacePathsToOpaque(spacePaths) - if err != nil { - appctx.GetLogger(ctx).Debug().Err(err).Msg("marshaling space paths map failed, continuing") - continue - } - providerInfos = append(providerInfos, p) } - return providerInfos + return []*registrypb.ProviderInfo{} } -// findProvidersForAbsolutePathReference takes a path and ruturns the storage provider with the longest matching path prefix +// findProvidersForAbsolutePathReference takes a path and returns the storage provider with the longest matching path prefix // FIXME use regex to return the correct provider when multiple are configured func (r *registry) findProvidersForAbsolutePathReference(ctx context.Context, path string) []*registrypb.ProviderInfo { currentUser := ctxpkg.ContextMustGetUser(ctx) @@ -453,6 +457,18 @@ func (r *registry) findProvidersForAbsolutePathReference(ctx context.Context, pa return pis } +// findAllProviders returns a list of all storage providers +// This is a dumb call that does not call ListStorageSpaces() on the providers: ListStorageSpaces() in the gateway can cache that better. +func (r *registry) findAllProviders(ctx context.Context) []*registrypb.ProviderInfo { + pis := make([]*registrypb.ProviderInfo, 0, len(r.c.Providers)) + for address := range r.c.Providers { + pis = append(pis, ®istrypb.ProviderInfo{ + Address: address, + }) + } + return pis +} + func spacePathsToOpaque(spacePaths map[string]string) (*typesv1beta1.Opaque, error) { spacePathsJSON, err := json.Marshal(spacePaths) if err != nil { From f30ae65a9198f1f80154d28befe45320aa03064a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Thu, 9 Dec 2021 14:03:53 +0000 Subject: [PATCH 3/3] update spaces registry test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- changelog/unreleased/spaces-registry-allow-list-all.md | 2 +- pkg/storage/registry/spaces/spaces_test.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/changelog/unreleased/spaces-registry-allow-list-all.md b/changelog/unreleased/spaces-registry-allow-list-all.md index 096e4deeeb..c826d7baff 100644 --- a/changelog/unreleased/spaces-registry-allow-list-all.md +++ b/changelog/unreleased/spaces-registry-allow-list-all.md @@ -1,5 +1,5 @@ Enhancement: allow listing all storage spaces -To implement the drives api we allow listing all spaces by sending a spaceid `*!*`. This is a workaround until a proper filter to list all storage spaces a user has access to / can manage is implemented, or we make that the implicit default filter. +To implement the drives api we now list all spaces when no filter is given. The Provider info will not contain any spaces as the client is responsible for looking up the spaces. https://github.com/cs3org/reva/pull/2344 diff --git a/pkg/storage/registry/spaces/spaces_test.go b/pkg/storage/registry/spaces/spaces_test.go index 0736687dd0..182e3a9b67 100644 --- a/pkg/storage/registry/spaces/spaces_test.go +++ b/pkg/storage/registry/spaces/spaces_test.go @@ -37,7 +37,7 @@ import ( . "github.com/onsi/gomega" ) -var _ = Describe("Static", func() { +var _ = Describe("Spaces", func() { var ( handler storage.Registry ctxAlice context.Context @@ -278,11 +278,11 @@ var _ = Describe("Static", func() { }) Describe("ListProviders", func() { - It("returns an empty list when no filters are set", func() { + It("returns all providers when no filter is set", func() { filters := map[string]string{} providers, err := handler.ListProviders(ctxAlice, filters) Expect(err).ToNot(HaveOccurred()) - Expect(len(providers)).To(Equal(0)) + Expect(len(providers)).To(Equal(3)) }) It("filters by path", func() {