Spaces registry #2234

butonic · 2021-11-04T09:16:28Z

We are prototyping a spaces registry that uses spaceids instead of storageids to route requests. This should clarify the concepts we are pursuing with #2023 and #2016

cc @dragotin @kobergj

temporary change to check CI

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

lgtm-com · 2021-11-05T16:35:04Z

This pull request introduces 3 alerts when merging 965bf36 into c35c530 - view on LGTM.com

new alerts:

3 for Unreachable statement

…provider

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

lgtm-com · 2021-11-08T11:00:43Z

This pull request introduces 4 alerts when merging 4704592 into bb40f54 - view on LGTM.com

new alerts:

3 for Missing error check
1 for Uncontrolled data used in path expression

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

lgtm-com · 2021-11-08T14:53:30Z

This pull request introduces 1 alert when merging 70e7123 into bb40f54 - view on LGTM.com

new alerts:

1 for Uncontrolled data used in path expression

lgtm-com · 2021-12-06T11:39:23Z

This pull request introduces 2 alerts when merging cbe1a2c into 147c0c2 - view on LGTM.com

new alerts:

2 for Uncontrolled data used in path expression

lgtm-com · 2021-12-06T12:22:40Z

This pull request introduces 2 alerts when merging a023668 into 147c0c2 - view on LGTM.com

new alerts:

2 for Uncontrolled data used in path expression

…torageprovider_static_test.go Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

lgtm-com · 2021-12-06T13:35:04Z

This pull request introduces 2 alerts when merging b48b2f3 into 147c0c2 - view on LGTM.com

new alerts:

2 for Uncontrolled data used in path expression

lgtm-com · 2021-12-06T14:54:40Z

This pull request introduces 2 alerts when merging b48b2f3 into 255730c - view on LGTM.com

new alerts:

2 for Uncontrolled data used in path expression

refs

👍

refs · 2021-12-06T14:55:57Z

internal/grpc/interceptors/auth/scope.go

+	// resourcePath := statResponse.Info.Path
+
+	// if strings.HasPrefix(ref.GetPath(), resourcePath) {
+	// 	// The path corresponds to the resource to which the token has access.
+	// 	// We allow access to it.
+	// 	return true, nil
+	// }
+
+	// // If we arrived here that could mean that ref.GetPath is not prefixed with the storage mount path but resourcePath is
+	// // because it was returned by the gateway which will prefix it. To fix that we remove the mount path from the resourcePath.
+	// // resourcePath = "/users/<name>/some/path"
+	// // After the split we have [" ", "users", "<name>/some/path"].
+	// trimmedPath := "/" + strings.SplitN(resourcePath, "/", 3)[2]
+	// if strings.HasPrefix(ref.GetPath(), trimmedPath) {
+	// 	// The path corresponds to the resource to which the token has access.
+	// 	// We allow access to it.
+	// 	return true, nil
+	// }
+
+	// return false, nil


I'd remove these lines instead of commenting them. It is clutter.

Fixed in #2383

refs · 2021-12-09T10:55:19Z

internal/grpc/services/gateway/appprovider.go


-	statRes, err := s.stat(ctx, &storageprovider.StatRequest{
-		Ref: &storageprovider.Reference{Path: resName},
+	resChild := ""


resChild is unset. Was your intention to document the parameter here? https://github.com/butonic/reva/blob/b48b2f37569170dfff52ea2ed644859b2e84304d/internal/grpc/services/gateway/appprovider.go#L75

Nope. Removed in #2383

refs · 2021-12-09T11:16:45Z

internal/grpc/services/gateway/storageprovider.go

+		spaceID := ""
+		mountPath := providerInfos[i].ProviderPath
+
+		spacePaths := decodeSpacePaths(providerInfos[i].Opaque)


We're doing the same time and again. I think we could benefit with an abstraction for this section. Perhaps a function should be enough. Also, how volatile is providerInfos, err := s.findProviders(ctx, req.Ref) I would imagine looking for providers depending on a reference a costly unnecessary operation, since resources don't tend to change storages often (or at all...) so caching it would improve quite a bit. I'm unaware of any other caching efforts on this area as for now, but IIRC @kobergj enabled the cached and had it working. Is it because of this perhaps?

Caching is enabled in #2372 Provider caching is working there.

I moved logic to a dedicated function in #2383

refs · 2021-12-09T11:22:59Z

internal/grpc/services/gateway/gateway.go

+	_ = statCache.SetTTL(time.Duration(c.StatCacheTTL) * time.Second)
+	statCache.SkipTTLExtensionOnHit(true)
+
+	// mountCache := ttlcache.NewCache()


could all lines related to the mountCache be thrown away? it clutters a bit

Fixed together with caching in #2372

refs · 2021-12-09T11:32:58Z

internal/grpc/services/gateway/storageprovider.go

+		return spacePaths
+	}
+	if entry, ok := o.Map["space_paths"]; ok {
+		_ = json.Unmarshal(entry.Value, &spacePaths)


I think this method is wrong, and assuming too much. CS3 OpaqueEntries do have decoders, assuming the map is encoded in json, and using the json decoder might break things.

Added more decoders in #2383

refs · 2021-12-13T11:36:17Z

pkg/storage/utils/decomposedfs/spaces.go

 		SpaceType: spaceType,
 		// Mtime is set either as node.tmtime or as fi.mtime below
 	}

-	user := ctxpkg.ContextMustGetUser(ctx)
-
 	// filter out spaces user cannot access (currently based on stat permission)


clutter. remove these comments

Done in #2383

refs · 2021-12-13T11:38:53Z

pkg/storage/utils/decomposedfs/upload.go

@@ -185,21 +186,24 @@ func (fs *Decomposedfs) NewUpload(ctx context.Context, info tusd.FileInfo) (uplo
 	log := appctx.GetLogger(ctx)
 	log.Debug().Interface("info", info).Msg("Decomposedfs: NewUpload")

-	fn := info.MetaData["filename"]
-	if fn == "" {
+	// sanity checks


spureous comment

Removed in #2383

refs · 2021-12-13T11:40:02Z

pkg/storage/utils/decomposedfs/tree/tree.go

 		return errtypes.AlreadyExists(n.ID) // path?
 	}

+	// Allow passing in the node id


Removed in #2383

refs · 2021-12-13T11:40:48Z

pkg/storage/utils/decomposedfs/tree/tree.go

+			return
+		}
+
+		// try to remove the node


unnecessary comment, line below is expressive enough

Removed in #2383

refs · 2021-12-13T11:41:23Z

pkg/storage/utils/decomposedfs/tree/tree.go

+		e := t.Delete(ctx, n)
+		switch {
+		case e != nil:
+			appctx.GetLogger(ctx).Debug().Err(e).Msg("cannot move to trashcan")


rename: trashcan -> trashbin

Changed in #2383

aduffeck and others added 14 commits October 26, 2021 16:25

Do not allow cross-storage restores

5f8edba

Fix expected failures for s3ng

958f9b2

Be more robust when handling error cases

028b279

WIP: Fix restoring recycle items

4f0949d

Fix unit tests

a5ca0f3

Fix license headers

df6021e

Fix linter issues

a00d562

move unwrapping and wrapping of paths to the gateway

7f40804

temporary change to check CI

minimal space provider and registry prototypes

434fb0e

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

Merge branch 'master' into sharestorageprovider-oc10-sm

606bfc4

add missing go.sum entry

38d8d72

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

fix import

e5bcf1e

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

add embedded mounts when listing /home

5b5f8ed

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

remove unexpected passes

4e257e8

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

butonic force-pushed the spaces-registry-and-provider branch from a7d370e to 6380dc1 Compare November 4, 2021 16:25

butonic and others added 5 commits November 4, 2021 16:33

use space registry to manage aliases

6380dc1

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

add changelog

e735fed

add old logic as comment as we want to bring parts of it back for OCM

d615470

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

Merge branch 'master' into spaces-registry-and-provider

1df7cc2

further work

965bf36

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

butonic added 6 commits November 5, 2021 16:46

Merge branch 'sharestorageprovider-oc10-sm' into spaces-registry-and-…

03b1f29

…provider

fix access vi space & path

2f157f9

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

Merge branch 'wrap-unwrap-gateway2' into spaces-registry-and-provider

ab89f60

register /home as path alies for the users personal space

4fb0937

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

update toml files

e8b8376

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

Merge branch 'master' into spaces-registry-and-provider

4704592

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

rewrite gateway Stat()

70e7123

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

make codacy happy (part III)

a023668

virtual views pipeline is covered by tests/integration/grpc/gateway_s…

b48b2f3

…torageprovider_static_test.go Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

butonic force-pushed the spaces-registry-and-provider branch from 352694d to b48b2f3 Compare December 6, 2021 13:14

butonic changed the base branch from master to edge December 6, 2021 14:37

kobergj approved these changes Dec 6, 2021

View reviewed changes

butonic marked this pull request as ready for review December 6, 2021 14:41

butonic requested review from glpatcern, ishank011, labkode and a team as code owners December 6, 2021 14:41

kobergj approved these changes Dec 6, 2021

View reviewed changes

butonic merged commit b2a9a7e into cs3org:edge Dec 6, 2021

butonic deleted the spaces-registry-and-provider branch December 6, 2021 14:59

This was referenced Dec 7, 2021

Sharestorageprovider oc10 sm #2023

Closed

move unwrapping and wrapping of paths to the gateway #2016

Closed

C0rby mentioned this pull request Dec 8, 2021

ocis changes for the reva wrap-unwrap refactoring owncloud/ocis#2460

Closed

refs reviewed Dec 13, 2021

View reviewed changes

kobergj mentioned this pull request Dec 17, 2021

[tests-only] Spaces registry PR comments #2383

Merged

micbar mentioned this pull request Feb 16, 2022

Release 1.17.0 Tech Preview owncloud/ocis#2998

Closed

22 tasks

micbar mentioned this pull request Mar 3, 2022

Release 1.18.0 Tech Preview owncloud/ocis#3244

Closed

26 tasks

kobergj mentioned this pull request Oct 14, 2022

[tests-only] FixRescanning #3355

Merged

kiranparajuli589 mentioned this pull request Oct 20, 2022

[tests-only][full-ci] bump owncloud core commit for the tests (edge) #3380

Merged

kiranparajuli589 mentioned this pull request Oct 28, 2022

[tests-only][full-ci] Bump the oC/core commit ID for the tests" #3411

Merged

kobergj mentioned this pull request Dec 19, 2022

Fix Tags pkg #3564

Merged

kobergj mentioned this pull request May 22, 2023

Use defer to invalidate ID Cache #3910

Closed

kobergj mentioned this pull request Mar 1, 2024

Extend ServiceAccount Permissions #4544

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spaces registry #2234

Spaces registry #2234

butonic commented Nov 4, 2021

lgtm-com bot commented Nov 5, 2021

lgtm-com bot commented Nov 8, 2021

lgtm-com bot commented Nov 8, 2021

lgtm-com bot commented Dec 6, 2021

lgtm-com bot commented Dec 6, 2021

lgtm-com bot commented Dec 6, 2021

lgtm-com bot commented Dec 6, 2021

refs left a comment

refs Dec 6, 2021

kobergj Dec 17, 2021

refs Dec 9, 2021

kobergj Dec 17, 2021

refs Dec 9, 2021

kobergj Dec 17, 2021

refs Dec 9, 2021

kobergj Dec 17, 2021

refs Dec 9, 2021

kobergj Dec 17, 2021

refs Dec 13, 2021

kobergj Dec 17, 2021

refs Dec 13, 2021

kobergj Dec 17, 2021

refs Dec 13, 2021

kobergj Dec 17, 2021

refs Dec 13, 2021

kobergj Dec 17, 2021

refs Dec 13, 2021

kobergj Dec 17, 2021

Spaces registry #2234

Spaces registry #2234

Conversation

butonic commented Nov 4, 2021

lgtm-com bot commented Nov 5, 2021

lgtm-com bot commented Nov 8, 2021

lgtm-com bot commented Nov 8, 2021

lgtm-com bot commented Dec 6, 2021

lgtm-com bot commented Dec 6, 2021

lgtm-com bot commented Dec 6, 2021

lgtm-com bot commented Dec 6, 2021

refs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment