Skip to content

Commit

Permalink
Grant locking (#4117)
Browse files Browse the repository at this point in the history
* Improve tracing

* Lock the node before changing the grants

* log cache diff on sync

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

* WIP debugging

* Validae if-unmodified-since before finishing the upload

* WIP

* etag based cache invalidation

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

* providercache seems solid now

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

* log diffs as before ind after

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

* Remove dead code

* Fix concurrent map writes

* Return the new etag after upload

* Lock on the provider level. fixes concurrent map accesses

* Retry more often

* Update in-memory etag with the one from the Upload response

* Improve logging

* ensure etag did not change during reading blob metadata and download

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

* move mtime to metadata

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

* openlock before reading metadata

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

* fix etag calculation

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

* drop diff, remove unused variable

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

* revert info level log from gateway storageprovider

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

* reduce logging in providercache

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

* crank up retries when adding shares

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

* drop unused mtime in jsoncs3

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

* bring back providercache persist debug log

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

* update providercache tests

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

* update sharecache tests

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

* make providercache logging more resilient

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

* update jsoncs3 tests

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

* update mock

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

* add changelog

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

* AddGrant must not fall back to UpdateGrant when a grant exists to prevent deadlock

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

* always return file handle

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

* use locks when copying revision metadata

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

* Fix header name

* Apply providercache fixes to sharecache as well

* fix restoring revision mtime

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

* make RemoveGrant possible with existing lock

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

* List created shares concurrently

* Use a sync.Map for the provider map and use space level locking again

* Also use a sync map for the spaces map

* Cleanup

* Fix unit tests

* Fix unit tests

* Fix linter issues

* update space root grant instead of always adding

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

* fix unreachable file handle

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

* fix space grant updates

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

* remove expired shares without checking permissions

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

* ignore expired grants when assembling permissions

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

* note idea how to aggregate persist requests in jsoncs3

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

* no need to propagate share expiry as an etag change

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

* Update changelog/unreleased/grant-locking.md

Co-authored-by: Michael Barz <michael.barz@zeitgestalten.eu>

* retry providercache removes more often

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

* only retry on recoverable errors

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

* unify body close

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

* dedupliacate metadata client download code

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

* do not magically retry downloads

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

---------

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Co-authored-by: André Duffeck <andre.duffeck@firondu.de>
Co-authored-by: Michael Barz <michael.barz@zeitgestalten.eu>
  • Loading branch information
3 people authored Aug 21, 2023
1 parent e3a2be9 commit 010c817
Show file tree
Hide file tree
Showing 45 changed files with 1,471 additions and 696 deletions.
7 changes: 7 additions & 0 deletions changelog/unreleased/grant-locking.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
Bugfix: fix jsoncs3 atomic persistence

The jsoncs3 share manager now uses etags instead of mtimes to determine when metadata needs to be updated.
As a precondtition we had to change decomposedfs as well: to consistently calculate the etag for the file content
we now store the mtime in the metadata and use the metadata lock for atomicity.

https://github.com/cs3org/reva/pull/4117
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ require (
go.opentelemetry.io/otel/sdk v1.16.0
go.opentelemetry.io/otel/trace v1.16.0
golang.org/x/crypto v0.11.0
golang.org/x/exp v0.0.0-20221026004748-78e5e7837ae6
golang.org/x/oauth2 v0.10.0
golang.org/x/sync v0.2.0
golang.org/x/sys v0.10.0
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -1305,6 +1305,8 @@ golang.org/x/exp v0.0.0-20191227195350-da58074b4299/go.mod h1:2RIsYlXP63K8oxa1u0
golang.org/x/exp v0.0.0-20200119233911-0405dc783f0a/go.mod h1:2RIsYlXP63K8oxa1u096TMicItID8zy7Y6sNkU49FU4=
golang.org/x/exp v0.0.0-20200207192155-f17229e696bd/go.mod h1:J/WKrq2StrnmMY6+EHIKF9dgMWnmCNThgcyBT1FY9mM=
golang.org/x/exp v0.0.0-20200224162631-6cc2880d07d6/go.mod h1:3jZMyOhIsHpP37uCMkUooju7aAi5cS1Q23tOzKc+0MU=
golang.org/x/exp v0.0.0-20221026004748-78e5e7837ae6 h1:mC6uOkPi9SUk8A59jZvw7//rlyc+MlELtQUCyOUSKZQ=
golang.org/x/exp v0.0.0-20221026004748-78e5e7837ae6/go.mod h1:cyybsKvd6eL0RnXn6p/Grxp8F5bW7iYuBgsNCOHpMYE=
golang.org/x/image v0.0.0-20190227222117-0694c2d4d067/go.mod h1:kZ7UVZpmo3dzQBMxlp+ypCbDeSB+sBbTgSJuh5dn5js=
golang.org/x/image v0.0.0-20190802002840-cff245a6509b/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0=
golang.org/x/lint v0.0.0-20181026193005-c67002cb31c3/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE=
Expand Down
77 changes: 65 additions & 12 deletions internal/grpc/services/gateway/usershareprovider.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,13 @@ func (s *svc) RemoveShare(ctx context.Context, req *collaboration.RemoveShareReq
return s.removeShare(ctx, req)
}

func (s *svc) UpdateShare(ctx context.Context, req *collaboration.UpdateShareRequest) (*collaboration.UpdateShareResponse, error) {
if !s.c.UseCommonSpaceRootShareLogic && refIsSpaceRoot(req.GetShare().GetResourceId()) {
return s.updateSpaceShare(ctx, req)
}
return s.updateShare(ctx, req)
}

// TODO(labkode): we need to validate share state vs storage grant and storage ref
// If there are any inconsistencies, the share needs to be flag as invalid and a background process
// or active fix needs to be performed.
Expand Down Expand Up @@ -98,7 +105,7 @@ func (s *svc) ListShares(ctx context.Context, req *collaboration.ListSharesReque
return res, nil
}

func (s *svc) UpdateShare(ctx context.Context, req *collaboration.UpdateShareRequest) (*collaboration.UpdateShareResponse, error) {
func (s *svc) updateShare(ctx context.Context, req *collaboration.UpdateShareRequest) (*collaboration.UpdateShareResponse, error) {
c, err := pool.GetUserShareProviderClient(s.c.UserShareProviderEndpoint)
if err != nil {
appctx.GetLogger(ctx).
Expand All @@ -114,9 +121,14 @@ func (s *svc) UpdateShare(ctx context.Context, req *collaboration.UpdateShareReq
}

if s.c.CommitShareToStorageGrant {
updateGrantStatus, err := s.updateGrant(ctx, res.GetShare().GetResourceId(),
res.GetShare().GetGrantee(),
res.GetShare().GetPermissions().GetPermissions())
creator := ctxpkg.ContextMustGetUser(ctx)
grant := &provider.Grant{
Grantee: req.GetShare().GetGrantee(),
Permissions: req.GetShare().GetPermissions().GetPermissions(),
Expiration: req.GetShare().GetExpiration(),
Creator: creator.GetId(),
}
updateGrantStatus, err := s.updateGrant(ctx, res.GetShare().GetResourceId(), grant, nil)

if err != nil {
return nil, errors.Wrap(err, "gateway: error calling updateGrant")
Expand All @@ -134,6 +146,51 @@ func (s *svc) UpdateShare(ctx context.Context, req *collaboration.UpdateShareReq
return res, nil
}

func (s *svc) updateSpaceShare(ctx context.Context, req *collaboration.UpdateShareRequest) (*collaboration.UpdateShareResponse, error) {
// If the share is a denial we call denyGrant instead.
var st *rpc.Status
var err error
// TODO: change CS3 APIs
opaque := &typesv1beta1.Opaque{
Map: map[string]*typesv1beta1.OpaqueEntry{
"spacegrant": {},
},
}
utils.AppendPlainToOpaque(opaque, "spacetype", utils.ReadPlainFromOpaque(req.Opaque, "spacetype"))

creator := ctxpkg.ContextMustGetUser(ctx)
grant := &provider.Grant{
Grantee: req.GetShare().GetGrantee(),
Permissions: req.GetShare().GetPermissions().GetPermissions(),
Expiration: req.GetShare().GetExpiration(),
Creator: creator.GetId(),
}
if grants.PermissionsEqual(req.Share.GetPermissions().GetPermissions(), &provider.ResourcePermissions{}) {
st, err = s.denyGrant(ctx, req.GetShare().GetResourceId(), req.GetShare().GetGrantee(), opaque)
if err != nil {
return nil, errors.Wrap(err, "gateway: error denying grant in storage")
}
} else {
st, err = s.updateGrant(ctx, req.GetShare().GetResourceId(), grant, opaque)
if err != nil {
return nil, errors.Wrap(err, "gateway: error adding grant to storage")
}
}

res := &collaboration.UpdateShareResponse{
Status: st,
Share: req.Share,
}

if st.Code != rpc.Code_CODE_OK {
return res, nil
}

s.statCache.RemoveStatContext(ctx, ctxpkg.ContextMustGetUser(ctx).GetId(), req.GetShare().GetResourceId())
s.providerCache.RemoveListStorageProviders(req.GetShare().GetResourceId())
return res, nil
}

// TODO(labkode): listing received shares just goes to the user share manager and gets the list of
// received shares. The display name of the shares should be the a friendly name, like the basename
// of the original file.
Expand Down Expand Up @@ -333,19 +390,15 @@ func (s *svc) addGrant(ctx context.Context, id *provider.ResourceId, g *provider
return grantRes.Status, nil
}

func (s *svc) updateGrant(ctx context.Context, id *provider.ResourceId, g *provider.Grantee, p *provider.ResourcePermissions) (*rpc.Status, error) {
func (s *svc) updateGrant(ctx context.Context, id *provider.ResourceId, grant *provider.Grant, opaque *typesv1beta1.Opaque) (*rpc.Status, error) {
ref := &provider.Reference{
ResourceId: id,
}

creator := ctxpkg.ContextMustGetUser(ctx)
grantReq := &provider.UpdateGrantRequest{
Ref: ref,
Grant: &provider.Grant{
Grantee: g,
Permissions: p,
Creator: creator.GetId(),
},
Opaque: opaque,
Ref: ref,
Grant: grant,
}

c, _, err := s.find(ctx, ref)
Expand Down
33 changes: 28 additions & 5 deletions internal/grpc/services/storageprovider/storageprovider.go
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,13 @@ func (s *service) InitiateFileUpload(ctx context.Context, req *provider.Initiate
return nil, err
}
switch sRes.Status.Code {
case rpc.Code_CODE_OK, rpc.Code_CODE_NOT_FOUND:
case rpc.Code_CODE_OK:
if req.GetIfNotExist() {
return &provider.InitiateFileUploadResponse{
Status: status.NewAlreadyExists(ctx, errors.New("already exists"), "already exists"),
}, nil
}
case rpc.Code_CODE_NOT_FOUND:
// Just continue with a normal upload
default:
return &provider.InitiateFileUploadResponse{
Expand All @@ -342,10 +348,14 @@ func (s *service) InitiateFileUpload(ctx context.Context, req *provider.Initiate
}
metadata["if-match"] = ifMatch
}
if !validateIfUnmodifiedSince(req.GetIfUnmodifiedSince(), sRes.GetInfo()) {
return &provider.InitiateFileUploadResponse{
Status: status.NewFailedPrecondition(ctx, errors.New("resource has been modified"), "resource has been modified"),
}, nil
ifUnmodifiedSince := req.GetIfUnmodifiedSince()
if ifUnmodifiedSince != nil {
metadata["if-unmodified-since"] = utils.TSToTime(ifUnmodifiedSince).Format(time.RFC3339Nano)
if !validateIfUnmodifiedSince(ifUnmodifiedSince, sRes.GetInfo()) {
return &provider.InitiateFileUploadResponse{
Status: status.NewFailedPrecondition(ctx, errors.New("resource has been modified"), "resource has been modified"),
}, nil
}
}

ctx = ctxpkg.ContextSetLockID(ctx, req.LockId)
Expand Down Expand Up @@ -1080,6 +1090,19 @@ func (s *service) UpdateGrant(ctx context.Context, req *provider.UpdateGrantRequ
}
}

// TODO: update CS3 APIs
// FIXME these should be part of the AddGrantRequest object
// https://github.com/owncloud/ocis/issues/4312
if utils.ExistsInOpaque(req.Opaque, "spacegrant") {
ctx = context.WithValue(
ctx,
utils.SpaceGrant,
struct{ SpaceType string }{
SpaceType: utils.ReadPlainFromOpaque(req.Opaque, "spacetype"),
},
)
}

// check grantee type is valid
if req.Grant.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_INVALID {
return &provider.UpdateGrantResponse{
Expand Down
1 change: 1 addition & 0 deletions internal/http/services/owncloud/ocdav/net/headers.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ const (
HeaderLocation = "Location"
HeaderRange = "Range"
HeaderIfMatch = "If-Match"
HeaderIfNoneMatch = "If-None-Match"
HeaderPrefer = "Prefer"
HeaderPreferenceApplied = "Preference-Applied"
HeaderVary = "Vary"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import (
"github.com/cs3org/reva/v2/pkg/rgrpc/todo/pool"
sdk "github.com/cs3org/reva/v2/pkg/sdk/common"
"github.com/cs3org/reva/v2/pkg/storagespace"
"github.com/cs3org/reva/v2/pkg/utils"
"github.com/pkg/errors"
)

Expand Down Expand Up @@ -126,45 +127,70 @@ func (h *Handler) addSpaceMember(w http.ResponseWriter, r *http.Request, info *p
}
}

if role.Name != conversions.RoleManager {
ref := provider.Reference{ResourceId: info.GetId()}
p, err := h.findProvider(ctx, &ref)
if err != nil {
response.WriteOCSError(w, r, response.MetaNotFound.StatusCode, "error getting storage provider", err)
return
}
ref := provider.Reference{ResourceId: info.GetId()}
p, err := h.findProvider(ctx, &ref)
if err != nil {
response.WriteOCSError(w, r, response.MetaNotFound.StatusCode, "error getting storage provider", err)
return
}

providerClient, err := h.getStorageProviderClient(p)
if err != nil {
response.WriteOCSError(w, r, response.MetaNotFound.StatusCode, "error getting storage provider client", err)
return
}
providerClient, err := h.getStorageProviderClient(p)
if err != nil {
response.WriteOCSError(w, r, response.MetaNotFound.StatusCode, "error getting storage provider client", err)
return
}

lgRes, err := providerClient.ListGrants(ctx, &provider.ListGrantsRequest{Ref: &ref})
if err != nil || lgRes.Status.Code != rpc.Code_CODE_OK {
response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error listing space grants", err)
return
}
lgRes, err := providerClient.ListGrants(ctx, &provider.ListGrantsRequest{Ref: &ref})
if err != nil || lgRes.Status.Code != rpc.Code_CODE_OK {
response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error listing space grants", err)
return
}

if !isSpaceManagerRemaining(lgRes.Grants, grantee) {
response.WriteOCSError(w, r, http.StatusForbidden, "the space must have at least one manager", nil)
return
}
if !isSpaceManagerRemaining(lgRes.Grants, grantee) {
response.WriteOCSError(w, r, http.StatusForbidden, "the space must have at least one manager", nil)
return
}

createShareRes, err := client.CreateShare(ctx, &collaborationv1beta1.CreateShareRequest{
ResourceInfo: info,
Grant: &collaborationv1beta1.ShareGrant{
Permissions: &collaborationv1beta1.SharePermissions{
Permissions: permissions,
// we have to send the update request to the gateway to give it a chance to invalidate its cache
// TODO the gateway no longer should cache stuff because invalidation is to expensive. The decomposedfs already has a better cache.
if granteeExists(lgRes.Grants, grantee) {
updateShareReq := &collaborationv1beta1.UpdateShareRequest{
// TODO: change CS3 APIs
Opaque: &types.Opaque{
Map: map[string]*types.OpaqueEntry{
"spacegrant": {},
},
},
Grantee: &grantee,
Expiration: expirationTs,
},
})
if err != nil || createShareRes.Status.Code != rpc.Code_CODE_OK {
response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "could not add space member", err)
return
Share: &collaborationv1beta1.Share{
ResourceId: ref.GetResourceId(),
Permissions: &collaborationv1beta1.SharePermissions{
Permissions: permissions,
},
Grantee: &grantee,
Expiration: expirationTs,
},
}
updateShareReq.Opaque = utils.AppendPlainToOpaque(updateShareReq.Opaque, "spacetype", info.GetSpace().GetSpaceType())
updateShareRes, err := client.UpdateShare(ctx, updateShareReq)
if err != nil || updateShareRes.Status.Code != rpc.Code_CODE_OK {
response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "could not update space member grant", err)
return
}
} else {
createShareRes, err := client.CreateShare(ctx, &collaborationv1beta1.CreateShareRequest{
ResourceInfo: info,
Grant: &collaborationv1beta1.ShareGrant{
Permissions: &collaborationv1beta1.SharePermissions{
Permissions: permissions,
},
Grantee: &grantee,
Expiration: expirationTs,
},
})
if err != nil || createShareRes.Status.Code != rpc.Code_CODE_OK {
response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "could not add space member grant", err)
return
}
}

response.WriteOCSSuccess(w, r, nil)
Expand Down Expand Up @@ -325,6 +351,15 @@ func isSpaceManagerRemaining(grants []*provider.Grant, grantee provider.Grantee)
return false
}

func granteeExists(grants []*provider.Grant, grantee provider.Grantee) bool {
for _, g := range grants {
if isEqualGrantee(*g.Grantee, grantee) {
return true
}
}
return false
}

func isEqualGrantee(a, b provider.Grantee) bool {
// Ideally we would want to use utils.GranteeEqual()
// but the grants stored in the decomposedfs aren't complete (missing usertype and idp)
Expand Down
48 changes: 48 additions & 0 deletions pkg/errtypes/errtypes.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
package errtypes

import (
"net/http"
"strings"

rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1"
Expand Down Expand Up @@ -172,6 +173,19 @@ func (e InsufficientStorage) StatusCode() int {
return StatusInsufficientStorage
}

// NotModified is the error to use when a resource was not modified, e.g. the requested etag did not change.
type NotModified string

func (e NotModified) Error() string { return "error: not modified: " + string(e) }

// NotModified implements the IsNotModified interface.
func (e NotModified) IsNotModified() {}

// StatusCode returns StatusInsufficientStorage, this implementation is needed to allow TUS to cast the correct http errors.
func (e NotModified) StatusCode() int {
return http.StatusNotModified
}

// Body returns the error body. This implementation is needed to allow TUS to cast the correct http errors
func (e InsufficientStorage) Body() []byte {
return []byte(e.Error())
Expand Down Expand Up @@ -302,3 +316,37 @@ func NewErrtypeFromStatus(status *rpc.Status) error {
return InternalError(status.Message)
}
}

// NewErrtypeFromHTTPStatusCode maps an http status to an errtype
func NewErrtypeFromHTTPStatusCode(code int, message string) error {
switch code {
case http.StatusOK:
return nil
case http.StatusNotFound:
return NotFound(message)
case http.StatusConflict:
return AlreadyExists(message)
case http.StatusNotImplemented:
return NotSupported(message)
case http.StatusNotModified:
return NotModified(message)
case http.StatusForbidden:
return PermissionDenied(message)
case http.StatusLocked:
return Locked(message)
case http.StatusPreconditionFailed:
return Aborted(message)
case http.StatusMethodNotAllowed:
return PreconditionFailed(message)
case http.StatusInsufficientStorage:
return InsufficientStorage(message)
case http.StatusBadRequest:
return BadRequest(message)
case http.StatusPartialContent:
return PartialContent(message)
case StatusChecksumMismatch:
return ChecksumMismatch(message)
default:
return InternalError(message)
}
}
Loading

0 comments on commit 010c817

Please sign in to comment.