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

Fix static registry, increase test coverage #2339

Merged
merged 8 commits into from
Dec 8, 2021
Merged

Fix static registry, increase test coverage #2339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4ccd56a
Return the provider info from findAndUnwrap
aduffeck Dec 6, 2021
b7f0aba
Compare the providers to prevent cross-storage moves
aduffeck Dec 6, 2021
4fa2375
Test move and delete with sharded directories/static registry
aduffeck Dec 6, 2021
628fdd8
Add test for id based requests
aduffeck Dec 7, 2021
d904dea
Fix id based requests when using the static registry
aduffeck Dec 7, 2021
6b65f56
Make hound happy
aduffeck Dec 8, 2021
16139d2
Add changelog
aduffeck Dec 8, 2021
7ca26aa
Fix returning the storage id as the provider id
aduffeck Dec 8, 2021
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/static-registry.md
View file
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,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 @@ -476,7 +476,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 @@ -559,7 +559,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 @@ -578,7 +578,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 @@ -600,13 +600,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 @@ -619,15 +620,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 @@ -642,7 +643,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 @@ -664,7 +665,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 @@ -789,6 +790,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 @@ -1025,7 +1029,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 @@ -1043,7 +1047,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 @@ -1301,7 +1305,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 @@ -1320,7 +1324,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 @@ -1358,10 +1362,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 @@ -1380,7 +1384,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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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