Skip to content

Commit

Permalink
Fix static registry, increase test coverage (#2339)
Browse files Browse the repository at this point in the history 
* Return the provider info from findAndUnwrap

* Compare the providers to prevent cross-storage moves

* Test move and delete with sharded directories/static registry

* Add test for id based requests

* Fix id based requests when using the static registry

* Make hound happy

* Add changelog

* Fix returning the storage id as the provider id
  • Loading branch information
@aduffeck
aduffeck committed Dec 8, 2021
1 parent 7b49a17 commit 2dbf826
Show file tree
Hide file tree
Showing 8 changed files with 169 additions and 44 deletions.
5 changes: 5 additions & 0 deletions changelog/unreleased/static-registry.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Change: Fix static registry regressions

We fixed some smaller issues with using the static registry which were introduced with the spaces registry changes.

https://github.com/cs3org/reva/pull/2339
36 changes: 20 additions & 16 deletions internal/grpc/services/gateway/storageprovider.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,7 @@ func (s *svc) InitiateFileDownload(ctx context.Context, req *provider.InitiateFi
// TODO(ishank011): enable downloading references spread across storage providers, eg. /eos
var c provider.ProviderAPIClient
var err error
c, req.Ref, err = s.findAndUnwrap(ctx, req.Ref)
c, _, req.Ref, err = s.findAndUnwrap(ctx, req.Ref)
if err != nil {
return &gateway.InitiateFileDownloadResponse{
Status: status.NewStatusFromErrType(ctx, "error initiating download ref="+req.Ref.String(), err),
Expand Down Expand Up @@ -486,7 +486,7 @@ func (s *svc) InitiateFileDownload(ctx context.Context, req *provider.InitiateFi
func (s *svc) InitiateFileUpload(ctx context.Context, req *provider.InitiateFileUploadRequest) (*gateway.InitiateFileUploadResponse, error) {
var c provider.ProviderAPIClient
var err error
c, req.Ref, err = s.findAndUnwrap(ctx, req.Ref)
c, _, req.Ref, err = s.findAndUnwrap(ctx, req.Ref)
if err != nil {
return &gateway.InitiateFileUploadResponse{
Status: status.NewStatusFromErrType(ctx, "initiateFileUpload ref="+req.Ref.String(), err),
Expand Down Expand Up @@ -570,7 +570,7 @@ func (s *svc) GetPath(ctx context.Context, req *provider.GetPathRequest) (*provi
func (s *svc) CreateContainer(ctx context.Context, req *provider.CreateContainerRequest) (*provider.CreateContainerResponse, error) {
var c provider.ProviderAPIClient
var err error
c, req.Ref, err = s.findAndUnwrap(ctx, req.Ref)
c, _, req.Ref, err = s.findAndUnwrap(ctx, req.Ref)
if err != nil {
return &provider.CreateContainerResponse{
Status: status.NewStatusFromErrType(ctx, "createContainer ref="+req.Ref.String(), err),
Expand All @@ -590,7 +590,7 @@ func (s *svc) Delete(ctx context.Context, req *provider.DeleteRequest) (*provide
// TODO(ishank011): enable deleting references spread across storage providers, eg. /eos
var c provider.ProviderAPIClient
var err error
c, req.Ref, err = s.findAndUnwrap(ctx, req.Ref)
c, _, req.Ref, err = s.findAndUnwrap(ctx, req.Ref)
if err != nil {
return &provider.DeleteResponse{
Status: status.NewStatusFromErrType(ctx, "delete ref="+req.Ref.String(), err),
Expand All @@ -613,13 +613,14 @@ func (s *svc) Delete(ctx context.Context, req *provider.DeleteRequest) (*provide

func (s *svc) Move(ctx context.Context, req *provider.MoveRequest) (*provider.MoveResponse, error) {
var c provider.ProviderAPIClient
var sourceProviderInfo, destinationProviderInfo *registry.ProviderInfo
var err error

rename := utils.IsAbsolutePathReference(req.Source) &&
utils.IsAbsolutePathReference(req.Destination) &&
filepath.Dir(req.Source.Path) == filepath.Dir(req.Destination.Path)

c, req.Source, err = s.findAndUnwrap(ctx, req.Source)
c, sourceProviderInfo, req.Source, err = s.findAndUnwrap(ctx, req.Source)
if err != nil {
return &provider.MoveResponse{
Status: status.NewStatusFromErrType(ctx, "Move ref="+req.Source.String(), err),
Expand All @@ -632,15 +633,15 @@ func (s *svc) Move(ctx context.Context, req *provider.MoveRequest) (*provider.Mo
req.Destination.ResourceId = req.Source.ResourceId
req.Destination.Path = utils.MakeRelativePath(filepath.Base(req.Destination.Path))
} else {
_, req.Destination, err = s.findAndUnwrap(ctx, req.Destination)
_, destinationProviderInfo, req.Destination, err = s.findAndUnwrap(ctx, req.Destination)
if err != nil {
return &provider.MoveResponse{
Status: status.NewStatusFromErrType(ctx, "Move ref="+req.Destination.String(), err),
}, nil
}

// if the storage id is the same the storage provider decides if the move is allowedy or not
if req.Source.ResourceId.StorageId != req.Destination.ResourceId.StorageId {
if sourceProviderInfo.Address != destinationProviderInfo.Address {
res := &provider.MoveResponse{
Status: status.NewUnimplemented(ctx, nil, "gateway: cross storage move not supported, use copy and delete"),
}
Expand All @@ -657,7 +658,7 @@ func (s *svc) SetArbitraryMetadata(ctx context.Context, req *provider.SetArbitra
// TODO(ishank011): enable for references spread across storage providers, eg. /eos
var c provider.ProviderAPIClient
var err error
c, req.Ref, err = s.findAndUnwrap(ctx, req.Ref)
c, _, req.Ref, err = s.findAndUnwrap(ctx, req.Ref)
if err != nil {
return &provider.SetArbitraryMetadataResponse{
Status: status.NewStatusFromErrType(ctx, "SetArbitraryMetadata ref="+req.Ref.String(), err),
Expand All @@ -680,7 +681,7 @@ func (s *svc) UnsetArbitraryMetadata(ctx context.Context, req *provider.UnsetArb
// TODO(ishank011): enable for references spread across storage providers, eg. /eos
var c provider.ProviderAPIClient
var err error
c, req.Ref, err = s.findAndUnwrap(ctx, req.Ref)
c, _, req.Ref, err = s.findAndUnwrap(ctx, req.Ref)
if err != nil {
return &provider.UnsetArbitraryMetadataResponse{
Status: status.NewStatusFromErrType(ctx, "UnsetArbitraryMetadata ref="+req.Ref.String(), err),
Expand Down Expand Up @@ -806,6 +807,9 @@ func (s *svc) Stat(ctx context.Context, req *provider.StatRequest) (*provider.St
statResp.Info.Path = path.Join(requestPath, statResp.Info.Path)
}
}
if statResp.Info.Id.StorageId == "" {
statResp.Info.Id.StorageId = providerInfos[i].ProviderId
}
currentInfo = statResp.Info

if info == nil {
Expand Down Expand Up @@ -1042,7 +1046,7 @@ func (s *svc) CreateSymlink(ctx context.Context, req *provider.CreateSymlinkRequ
func (s *svc) ListFileVersions(ctx context.Context, req *provider.ListFileVersionsRequest) (*provider.ListFileVersionsResponse, error) {
var c provider.ProviderAPIClient
var err error
c, req.Ref, err = s.findAndUnwrap(ctx, req.Ref)
c, _, req.Ref, err = s.findAndUnwrap(ctx, req.Ref)
if err != nil {
return &provider.ListFileVersionsResponse{
Status: status.NewStatusFromErrType(ctx, "ListFileVersions ref="+req.Ref.String(), err),
Expand All @@ -1060,7 +1064,7 @@ func (s *svc) ListFileVersions(ctx context.Context, req *provider.ListFileVersio
func (s *svc) RestoreFileVersion(ctx context.Context, req *provider.RestoreFileVersionRequest) (*provider.RestoreFileVersionResponse, error) {
var c provider.ProviderAPIClient
var err error
c, req.Ref, err = s.findAndUnwrap(ctx, req.Ref)
c, _, req.Ref, err = s.findAndUnwrap(ctx, req.Ref)
if err != nil {
return &provider.RestoreFileVersionResponse{
Status: status.NewStatusFromErrType(ctx, "RestoreFileVersion ref="+req.Ref.String(), err),
Expand Down Expand Up @@ -1325,7 +1329,7 @@ func (s *svc) RestoreRecycleItem(ctx context.Context, req *provider.RestoreRecyc
}

func (s *svc) PurgeRecycle(ctx context.Context, req *provider.PurgeRecycleRequest) (*provider.PurgeRecycleResponse, error) {
c, relativeReference, err := s.findAndUnwrap(ctx, req.Ref)
c, _, relativeReference, err := s.findAndUnwrap(ctx, req.Ref)
if err != nil {
return &provider.PurgeRecycleResponse{
Status: status.NewStatusFromErrType(ctx, "PurgeRecycle ref="+req.Ref.String(), err),
Expand All @@ -1346,7 +1350,7 @@ func (s *svc) PurgeRecycle(ctx context.Context, req *provider.PurgeRecycleReques
}

func (s *svc) GetQuota(ctx context.Context, req *gateway.GetQuotaRequest) (*provider.GetQuotaResponse, error) {
c, relativeReference, err := s.findAndUnwrap(ctx, req.Ref)
c, _, relativeReference, err := s.findAndUnwrap(ctx, req.Ref)
if err != nil {
return &provider.GetQuotaResponse{
Status: status.NewStatusFromErrType(ctx, "GetQuota ref="+req.Ref.String(), err),
Expand Down Expand Up @@ -1384,10 +1388,10 @@ func (s *svc) find(ctx context.Context, ref *provider.Reference) (provider.Provi

// FIXME findAndUnwrap currently just returns the first provider ... which may not be what is needed.
// for the ListRecycle call we need an exact match, for Stat and List we need to query all related providers
func (s *svc) findAndUnwrap(ctx context.Context, ref *provider.Reference) (provider.ProviderAPIClient, *provider.Reference, error) {
func (s *svc) findAndUnwrap(ctx context.Context, ref *provider.Reference) (provider.ProviderAPIClient, *registry.ProviderInfo, *provider.Reference, error) {
c, p, err := s.find(ctx, ref)
if err != nil {
return nil, nil, err
return nil, nil, nil, err
}

mountPath := p.ProviderPath
Expand All @@ -1406,7 +1410,7 @@ func (s *svc) findAndUnwrap(ctx context.Context, ref *provider.Reference) (provi
}
relativeReference := unwrap(ref, mountPath, root)

return c, relativeReference, nil
return c, p, relativeReference, nil
}

func (s *svc) getStorageProviderClient(_ context.Context, p *registry.ProviderInfo) (provider.ProviderAPIClient, error) {
Expand Down
37 changes: 20 additions & 17 deletions pkg/storage/registry/static/static.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,17 @@ func init() {

var bracketRegex = regexp.MustCompile(`\[(.*?)\]`)

type alias struct {
Address string `mapstructure:"address"`
ID string `mapstructure:"provider_id"`
}
type rule struct {
Mapping string `mapstructure:"mapping"`
Address string `mapstructure:"address"`
ProviderID string `mapstructure:"provider_id"`
ProviderPath string `mapstructure:"provider_path"`
Aliases map[string]string `mapstructure:"aliases"`
AllowedUserAgents []string `mapstructure:"allowed_user_agents"`
Mapping string `mapstructure:"mapping"`
Address string `mapstructure:"address"`
ProviderID string `mapstructure:"provider_id"`
ProviderPath string `mapstructure:"provider_path"`
Aliases map[string]alias `mapstructure:"aliases"`
AllowedUserAgents []string `mapstructure:"allowed_user_agents"`
}

type config struct {
Expand Down Expand Up @@ -97,28 +101,29 @@ type reg struct {
c *config
}

func getProviderAddr(ctx context.Context, r rule) string {
func getProviderAddr(ctx context.Context, r rule) (string, string) {
addr := r.Address
if addr == "" {
if u, ok := ctxpkg.ContextGetUser(ctx); ok {
layout := templates.WithUser(u, r.Mapping)
for k, v := range r.Aliases {
if match, _ := regexp.MatchString("^"+k, layout); match {
addr = v
return v.Address, v.ID
}
}
}
}
return addr
return addr, r.ProviderID
}

func (b *reg) GetProvider(ctx context.Context, space *provider.StorageSpace) (*registrypb.ProviderInfo, error) {
// Assume that HomeProvider is not a regexp
if space.SpaceType == "personal" {
if r, ok := b.c.Rules[b.c.HomeProvider]; ok {
if addr := getProviderAddr(ctx, r); addr != "" {
if addr, id := getProviderAddr(ctx, r); addr != "" {
return &registrypb.ProviderInfo{
ProviderPath: b.c.HomeProvider,
ProviderId: id,
Address: addr,
}, nil
}
Expand All @@ -143,22 +148,21 @@ func (b *reg) GetProvider(ctx context.Context, space *provider.StorageSpace) (*r
}

func (b *reg) ListProviders(ctx context.Context, filters map[string]string) ([]*registrypb.ProviderInfo, error) {

// find longest match
var match *registrypb.ProviderInfo
var shardedMatches []*registrypb.ProviderInfo
// If the reference has a resource id set, use it to route
if filters["storage_id"] != "" {
for prefix, rule := range b.c.Rules {
addr := getProviderAddr(ctx, rule)
addr, _ := getProviderAddr(ctx, rule)
r, err := regexp.Compile("^" + prefix + "$")
if err != nil {
continue
}
// TODO(labkode): fill path info based on provider id, if path and storage id points to same id, take that.
if m := r.FindString(filters["storage_id"]); m != "" {
return []*registrypb.ProviderInfo{{
ProviderId: filters["storage_id"],
ProviderId: prefix,
Address: addr,
ProviderPath: rule.ProviderPath,
}}, nil
Expand All @@ -176,8 +180,7 @@ func (b *reg) ListProviders(ctx context.Context, filters map[string]string) ([]*
fn := path.Clean(filters["path"])
if fn != "" {
for prefix, rule := range b.c.Rules {

addr := getProviderAddr(ctx, rule)
addr, id := getProviderAddr(ctx, rule)
r, err := regexp.Compile("^" + prefix)
if err != nil {
continue
Expand All @@ -188,7 +191,7 @@ func (b *reg) ListProviders(ctx context.Context, filters map[string]string) ([]*
continue
}
match = &registrypb.ProviderInfo{
ProviderId: rule.ProviderID,
ProviderId: id,
ProviderPath: m,
Address: addr,
}
Expand All @@ -198,7 +201,7 @@ func (b *reg) ListProviders(ctx context.Context, filters map[string]string) ([]*
combs := generateRegexCombinations(prefix)
for _, c := range combs {
shardedMatches = append(shardedMatches, &registrypb.ProviderInfo{
ProviderId: rule.ProviderID,
ProviderId: id,
ProviderPath: c,
Address: addr,
})
Expand Down
28 changes: 20 additions & 8 deletions pkg/storage/registry/static/static_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,18 +40,30 @@ var _ = Describe("Static", func() {
"rules": map[string]interface{}{
"/home": map[string]interface{}{
"mapping": "/home-{{substr 0 1 .Id.OpaqueId}}",
"aliases": map[string]string{
"/home-[a-fg-o]": "home-00-home",
"/home-[pqrstu]": "home-01-home",
"/home-[v-z]": "home-02-home",
"aliases": map[string]interface{}{
"/home-[a-fg-o]": map[string]string{
"address": "home-00-home",
},
"/home-[pqrstu]": map[string]string{
"address": "home-01-home",
},
"/home-[v-z]": map[string]string{
"address": "home-02-home",
},
},
},
"/MyShares": map[string]interface{}{
"mapping": "/MyShares-{{substr 0 1 .Id.OpaqueId}}",
"aliases": map[string]string{
"/MyShares-[a-fg-o]": "home-00-shares",
"/MyShares-[pqrstu]": "home-01-shares",
"/MyShares-[v-z]": "home-02-shares",
"aliases": map[string]interface{}{
"/MyShares-[a-fg-o]": map[string]string{
"address": "home-00-shares",
},
"/MyShares-[pqrstu]": map[string]string{
"address": "home-01-shares",
},
"/MyShares-[v-z]": map[string]string{
"address": "home-02-shares",
},
},
},
"/eos/user/[a-fg-o]": map[string]interface{}{
Expand Down
8 changes: 5 additions & 3 deletions tests/integration/grpc/fixtures/gateway-static.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,11 @@ driver = "static"
home_provider = "/home"

[grpc.services.storageregistry.drivers.static.rules]
"/home" = {"mapping" = "/home-{{substr 0 1 .Id.OpaqueId}}", "aliases" = {"/home-[0-9a-e]" = "{{storage_address}}", "/home-[f-z]" = "{{storage2_address}}"}}
"/users/[0-9a-e]" = {address = "{{storage_address}}"}
"/users/[f-z]" = {address = "{{storage2_address}}"}
"/home" = {"mapping" = "/home-{{substr 0 1 .Id.OpaqueId}}", "aliases" = {"/home-[0-9a-e]" = {"address" = "{{storage_address}}", "provider_id" = "{{storage_id}}"}, "/home-[f-z]" = {"address" = "{{storage2_address}}", "provider_id" = "{{storage2_id}}"}}}
"{{storage_id}}" = {address = "{{storage_address}}", "provider_id" = "{{storage_id}}"}
"{{storage2_id}}" = {address = "{{storage2_address}}", "provider_id" = "{{storage2_id}}"}
"/users/[0-9a-e]" = {address = "{{storage_address}}", "provider_id" = "{{storage_id}}"}
"/users/[f-z]" = {address = "{{storage2_address}}", "provider_id" = "{{storage2_id}}"}

[http]
address = "{{grpc_address+1}}"
Expand Down
1 change: 1 addition & 0 deletions tests/integration/grpc/fixtures/storageprovider-owncloud.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,5 @@ driver = "owncloud"
enable_home = {{enable_home}}
datadirectory = "{{root}}/storage"
userprovidersvc = "{{users_address}}"
mount_id = "{{id}}"
redis = "{{redis_address}}"
Loading

0 comments on commit 2dbf826

Please sign in to comment.