Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Leverage shares space storageid and type when listing shares #2975

Merged
merged 9 commits into from
Jun 16, 2022
Merged

Leverage shares space storageid and type when listing shares #2975

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
41f7ec9
use shares providerid to ease routing
butonic Jun 15, 2022
2049489
introduce ShareStorageSpaceID
butonic Jun 15, 2022
e4abdec
make sharesstorageprovider return storageid
butonic Jun 15, 2022
018c6cf
allow empty providerid when checking share jail root
butonic Jun 15, 2022
347f9f4
add todo for cache
butonic Jun 15, 2022
89e70a5
skip storageproviders that are not configured for the requested space…
butonic Jun 15, 2022
d03fce0
add changelog
butonic Jun 15, 2022
063d4da
use ShareStorageSpaceID
butonic Jun 16, 2022
11a4c96
make codacy happy
butonic Jun 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions changelog/unreleased/leverage-sharesstorage-storageid.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Enhancement: Leverage shares space storageid and type when listing shares

The list shares call now also fills the storageid to allow the space registry to directly route requests to the correct storageprovider. The spaces registry will now also skip storageproviders that are not configured for a requested type, causing type 'personal' requests to skip the sharestorageprovider.

https://github.com/cs3org/reva/pull/2975
2 changes: 1 addition & 1 deletion internal/grpc/services/gateway/storageprovidercache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ func (c *cachedRegistryClient) ListStorageProviders(ctx context.Context, in *reg
return resp, nil
case storageID == "":
return resp, nil
case storageID == utils.ShareStorageProviderID:
case storageID == utils.ShareStorageProviderID: // TODO do we need to compare providerid and spaceid separately?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not make the debugger halt here, but AFACT we need to skip the cache when the spaceid or the providerid match the sharesstorageproviderid. In and case the CI was green without this, so ... can be deferred to a subsequent PR.

return resp, nil
default:
return resp, pushToCache(cache, key, resp)
Expand Down
14 changes: 9 additions & 5 deletions internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
switch k {
case "virtual":
virtualRootID := &provider.ResourceId{
StorageId: utils.ShareStorageProviderID,
StorageId: storagespace.FormatStorageID(utils.ShareStorageProviderID, utils.ShareStorageSpaceID),
OpaqueId: utils.ShareStorageProviderID,
}
if spaceID == nil || isShareJailRoot(spaceID) {
Expand All @@ -387,7 +387,7 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
space := &provider.StorageSpace{
Opaque: opaque,
Id: &provider.StorageSpaceId{
OpaqueId: virtualRootID.StorageId + "!" + virtualRootID.OpaqueId,
OpaqueId: storagespace.FormatResourceID(*virtualRootID),
},
SpaceType: "virtual",
//Owner: &userv1beta1.User{Id: receivedShare.Share.Owner}, // FIXME actually, the mount point belongs to the recipient
Expand Down Expand Up @@ -415,7 +415,7 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
space := &provider.StorageSpace{
Opaque: opaque,
Id: &provider.StorageSpaceId{
OpaqueId: root.StorageId + "!" + root.OpaqueId,
OpaqueId: storagespace.FormatResourceID(*root),
},
SpaceType: "grant",
Owner: &userv1beta1.User{Id: receivedShare.Share.Owner},
Expand All @@ -433,7 +433,6 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
root := &provider.ResourceId{
StorageId: utils.ShareStorageProviderID,
OpaqueId: receivedShare.Share.Id.OpaqueId,
//OpaqueId: utils.ShareStorageProviderID,
}
// do we filter by id
if spaceID != nil {
Expand Down Expand Up @@ -461,10 +460,15 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
opaque = utils.AppendPlainToOpaque(opaque, "grantOpaqueID", receivedShare.Share.ResourceId.OpaqueId)
}

// prefix storageid if we are responsible
if root.StorageId == utils.ShareStorageSpaceID {
root.StorageId = storagespace.FormatStorageID(utils.ShareStorageProviderID, root.StorageId)
}

space := &provider.StorageSpace{
Opaque: opaque,
Id: &provider.StorageSpaceId{
OpaqueId: root.StorageId + "!" + root.OpaqueId,
OpaqueId: storagespace.FormatResourceID(*root),
},
SpaceType: "mountpoint",
Owner: &userv1beta1.User{Id: receivedShare.Share.Owner}, // FIXME actually, the mount point belongs to the recipient
Expand Down
13 changes: 9 additions & 4 deletions internal/http/services/owncloud/ocdav/propfind/propfind.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1354,9 +1354,14 @@ func (pn *Props) UnmarshalXML(d *xml.Decoder, start xml.StartElement) error {
}
}

// isVirtualRootResourceID returns true if the id points to the share jail root. The providerid is optional for legacy ids
func isVirtualRootResourceID(id *provider.ResourceId) bool {
return utils.ResourceIDEqual(id, &provider.ResourceId{
StorageId: utils.ShareStorageProviderID,
OpaqueId: utils.ShareStorageProviderID,
})
switch {
case id == nil:
return false
case id.OpaqueId != utils.ShareStorageProviderID:
return false
}
providerID, spaceID := storagespace.SplitStorageID(id.StorageId)
return spaceID == utils.ShareStorageProviderID && (providerID == "" || providerID == utils.ShareStorageProviderID)
}
2 changes: 1 addition & 1 deletion internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -864,7 +864,7 @@ func (h *Handler) listSharesWithMe(w http.ResponseWriter, r *http.Request) {
// first stat mount point, but the shares storage provider only handles accepted shares so we send the try to make the requests for only those
if rs.State == collaboration.ShareState_SHARE_STATE_ACCEPTED {
mountID := &provider.ResourceId{
StorageId: utils.ShareStorageProviderID,
StorageId: storagespace.FormatStorageID(utils.ShareStorageProviderID, utils.ShareStorageSpaceID),
OpaqueId: rs.Share.Id.OpaqueId,
}
info, status, err = h.getResourceInfoByID(ctx, client, mountID)
Expand Down
19 changes: 19 additions & 0 deletions pkg/storage/registry/spaces/spaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,9 +321,28 @@ func (r *registry) buildFilters(filterMap map[string]string) []*providerpb.ListS

func (r *registry) findProvidersForFilter(ctx context.Context, filters []*providerpb.ListStorageSpacesRequest_Filter, unrestricted bool, _ string) []*registrypb.ProviderInfo {

var requestedSpaceType string
for _, f := range filters {
if f.Type == providerpb.ListStorageSpacesRequest_Filter_TYPE_SPACE_TYPE {
requestedSpaceType = f.GetSpaceType()
}
}

currentUser := ctxpkg.ContextMustGetUser(ctx)
providerInfos := []*registrypb.ProviderInfo{}
for address, provider := range r.c.Providers {
if requestedSpaceType != "" {
// check if we can skip this provider altogether
found := false
for spaceType := range provider.Spaces {
if spaceType == requestedSpaceType {
found = true
}
}
if !found {
continue
}
}
p := &registrypb.ProviderInfo{
Address: address,
}
Expand Down
4 changes: 3 additions & 1 deletion pkg/utils/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,10 @@ var (
// globals is not encouraged, and this is a workaround until the PR is out of a draft state.
GlobalRegistry registry.Registry = memory.New(map[string]interface{}{})

// ShareStorageProviderID is the id used by the sharestorageprovider
// ShareStorageProviderID is the provider id used by the sharestorageprovider
ShareStorageProviderID = "a0ca6a90-a365-4782-871e-d44447bbc668"
// ShareStorageSpaceID is the space id used by the sharestorageprovider share jail space
ShareStorageSpaceID = "a0ca6a90-a365-4782-871e-d44447bbc668"

// PublicStorageProviderID is the id used by the sharestorageprovider
PublicStorageProviderID = "7993447f-687f-490d-875c-ac95e89a62a4"
Expand Down