Feat/share consistency#5562
Conversation
jessegeens
force-pushed
the
feat/share-consistency
branch
7 times, most recently
from
6abc06c to
521f438
Compare
jessegeens
force-pushed
the
feat/share-consistency
branch
4 times, most recently
from
e417154 to
e87365d
Compare
glpatcern
left a comment
glpatcern
left a comment
There was a problem hiding this comment.
So far so good, I didn't go to the very end though
jessegeens
force-pushed
the
feat/share-consistency
branch
6 times, most recently
from
bdfd14f to
129c8f3
Compare
| req.ResourceInfo.Id.SpaceId = spaceId | ||
| log.Debug().Str("keyword", "sharehierarchy").Str("spaceId", spaceId).Msg("sharehierarchy: populated missing spaceId via stat") | ||
| } else { | ||
| log.Warn().Str("keyword", "sharehierarchy").Msg("sharehierarchy: spaceId missing and stat could not resolve it") |
There was a problem hiding this comment.
Is it ok to continue? If so, explain why with a comment.
I see that existing shares are empty in this case, so we skip the check (fine, I guess we don't want to do a check of all shares). But at the same time, it looks like we allow creating a share without a space (don't see where we use it, because we haven't changed the sharing column name to reflect this, right?). It's just that it looks a bit weird that we allow creating a share without a space.
There was a problem hiding this comment.
maybe indeed best to exit with an error there
| for _, child := range result.ToDelete { | ||
| if st, err := s.removeGrant(ctx, child.ResourceId, child.Grantee, child.Permissions.Permissions); err != nil || st.Code != rpc.Code_CODE_OK { | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "gateway: error removing child grant") |
There was a problem hiding this comment.
Shall we return or skip to try to remove the most that we can (and reach the re-apply section as well)?
There was a problem hiding this comment.
Also, all of this iteration logic (to delete and to reapply) is repeated in all methods. can we extract this to aux functions? it would make the flow of this also nicer if abstracted.
| // Then we commit to the db | ||
| // ------------------------ | ||
| // Finally, we write to the db | ||
| res, err := shareClient.CreateShare(ctx, req) |
There was a problem hiding this comment.
Would it be more consistent if we brought this closer to the call to apply the new acl to storage? We do everything we need for the current share, then we do the cleanups and consistencies?
Although, keeping it here would allow the user to re-issue the same share request again...
There was a problem hiding this comment.
And we should think very well what happens if any action fails. Our consistency check should include this algorithm verification to cleanup unnecessary shares.
I'm just wondering, if we move the db creation upwards, and this call fails, how bad would it be? Shall we exit? In the meantime the ACLs in EOS are all broken, so we should still try to do the consistency cleanups...
* Use a (configurable) dedicated service account for accesses made by external accounts, instead of impersonating the owner or using a token * Renamed the different types of auth to be more clear (e.g. cboxAuth became systemAuth) * Added a `InvalidAuthorization` to be returned instead of an empty auth; because empty auth maps to the system user (which is a sudo'er)
jessegeens
force-pushed
the
feat/share-consistency
branch
3 times, most recently
from
33163d5 to
04f96ce
Compare
jessegeens
force-pushed
the
feat/share-consistency
branch
from
04f96ce to
b870097
Compare
3 participants
No description provided.