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.

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

Enforce space quota on uploads #2164

Closed
wants to merge 4 commits into from

Conversation

refs
Copy link
Member

@refs refs commented Oct 13, 2021

Acceptance Criteria

  • Space members can not add files to a Space if they would exceed the quota limit
  • If users try to do it and hit the limit, they'll see a meaningful error message (API-level)
  • acceptance tests cover the implemented functionality

TODO

  • Check with nested folders within a storage space

Regression found

The create storage space response returns an invalid webdav url. Needs to be fixed.

Questions

What is the behavior if:

  • the space quota is 99% used and it is reduced to 50%? Can we still delete? (i.e move items to the trash bin)
  • can a space have a larger quota than the root of the FS?
    i.e: space quota: 50 bytes, root FS quota: 20 bytes
  • Should the metadata be taken into consideration? Are we allowed to go over the total quota?

Technical Debt

  • Should we guard against setting the space quota to a number lower than the number of user bytes?
  • Size calculations are done ignoring the .space folder. This folder needs to only host the description and image of the storage, to prevent any leaks.

Testing

  1. create a storage space with quota set to 20 bytes

curl -k -X POST 'https://localhost:9200/graph/v1.0/drives' -u admin:admin -d '{"name":"marketing", "quota": {"total": 20}}' -v

The resulting node will use up 39 bytes (because of the metadata)

  1. attempt to upload a file with 10 bytes. It will fail, because the first upload ate up all the available space:

curl -k -X PUT https://localhost:9200/dav/spaces/1284d238-aa92-42ce-bdc4-0b0000009157\!d36bfa36-9441-4ec4-9d32-f2e8880bd301/test.txt -d "1234567890" -u admin:admin -v

@refs refs requested a review from labkode as a code owner October 13, 2021 11:21
@refs refs requested a review from C0rby October 13, 2021 11:21
// containedWithinSpace is a working name. It checks whether a reference is contained within a Storage Space or not. If
// it is, it will return the root node of the storage space.
func (fs *Decomposedfs) containedWithinSpace(ctx context.Context, n *node.Node) (*node.Node, bool) {
// get home or root node
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tested it but I think this method will not work correctly when you start at a sub-sub-folder.
/<space-root>/f1/f2 f2.Parent() = f1 and f1 has a name so that will return f1, if I'm not mistaken.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if err := n.SetMetadata(xattrs.SpaceNameAttr, req.Name); err != nil {
return nil, err
}

AFAIK we only set the name attribute at the root of the space 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If what you meant to say is that if a file does not contain a name the function will early return, then you're right :p. This is the tests I'm running:

Test with a nested folder:

  1. create test folders under space's root: <root>/f1/f2

curl -k -X MKCOL https://localhost:9200/dav/spaces/1284d238-aa92-42ce-bdc4-0b0000009157\!492dc039-70c3-44e2-8a71-bdcf690a9dbe/f1 -u admin:admin

curl -k -X MKCOL https://localhost:9200/dav/spaces/1284d238-aa92-42ce-bdc4-0b0000009157\!492dc039-70c3-44e2-8a71-bdcf690a9dbe/f1 -u admin:admin

  1. upload in f2

curl -k -X PUT https://localhost:9200/dav/spaces/1284d238-aa92-42ce-bdc4-0b0000009157\!492dc039-70c3-44e2-8a71-bdcf690a9dbe/f1/f2/test.txt -d "12345678901" -u admin:admin -v

  1. update the space quota

xattr -w user.ocis.quota 10 /var/tmp/ocis/storage/users/spaces/project/492dc039-70c3-44e2-8a71-bdcf690a9dbe

  1. upload again

curl -k -X PUT https://localhost:9200/dav/spaces/1284d238-aa92-42ce-bdc4-0b0000009157\!492dc039-70c3-44e2-8a71-bdcf690a9dbe/f1/f2/test3.txt -d "12345678901" -u admin:admin -v

Should and must fail with insufficient storage.

I will push the new patch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah now I found my mistake. I was thinking of the attribute name not spaceName. Every node has a name but not every node has a spaceName. So this is fine. 👍

@C0rby C0rby self-requested a review October 13, 2021 13:15
@refs
Copy link
Member Author

refs commented Oct 13, 2021

DO NOT MERGE just yet. This reports the wrong real disk usage of the blobs, as filepath.Walk does not follow symbolic links.

@refs refs marked this pull request as draft October 14, 2021 11:33
@refs refs closed this Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants