Skip to content

Commit

Permalink
Block creation of system reserved Share names rockstor#2881
Browse files Browse the repository at this point in the history 
Use/extend existing knowledge of system associated
subvolumes to avoid breaking (optional) ROOT pool
import, and non-optional system unique share names.

Includes:
- Addition of CREATE_SUBVOL_EXCLUDE list.
- Block creation of all currently EXCLUDED subvol names,
previously used only to avoid Web-UI surfacing.
- Incidental string.format to fstrings in share.py.
- Additional unit test to exercise added Share creation
filter.
  • Loading branch information
@phillxnet
phillxnet committed Jul 26, 2024
1 parent 2ecd4df commit a43235d
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 45 deletions.
8 changes: 7 additions & 1 deletion src/rockstor/fs/btrfs.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,14 @@
"@/.snapshots",
".snapshots",
]
# Note in the above we have a non symmetrical exclusions entry of '@/.snapshots
# Note in the above we have a non-symmetrical exclusions entry of '@/.snapshots
# this is to help distinguish our .snapshots from snapper's rollback subvol.

# Create subvolume blacklist to avoid name clash with default ROOT pool.
# Used in addition to ROOT_SUBVOL_EXCLUDE. From 5.0.9-0 we no longer auto-import
# the system (ROOT) pool.
CREATE_SUBVOL_EXCLUDE = ["home", "@/home"]

# System-wide subvolume exclude list.
SUBVOL_EXCLUDE = [".beeshome", "@/.beeshome"]

Expand Down
20 changes: 16 additions & 4 deletions src/rockstor/storageadmin/tests/test_shares.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,14 @@
--natural-foreign --indent 4 > \
src/rockstor/storageadmin/fixtures/test_shares-services.json
./bin/test -v 2 -p test_shares.py
To run the tests:
cd /opt/rockstor/src/rockstor
poetry run django-admin test -v 2 -p test_shares.py
"""


class ShareTests(APITestMixin):
databases = '__all__'
databases = "__all__"
fixtures = ["test_api.json", "test_shares.json", "test_shares-services.json"]
BASE_URL = "/api/shares"

Expand Down Expand Up @@ -242,6 +244,7 @@ def test_create(self):
- Create share with valid replica
- Create share with invalid replica
- Create share with share size > pool size
- Create share with system reserved name
"""

# create a share on non-existent pool
Expand Down Expand Up @@ -354,6 +357,17 @@ def test_create(self):
pool = Pool.objects.get(name=data6["pool"])
self.assertEqual(response10.data["size"], pool.size)

# Create share with system reserved name
data7 = {"sname": "var", "pool": "rock-pool", "size": 1048576}
e_msg8 = f"Share name ({data7['sname']}) reserved for system. Choose a different name."
response11 = self.client.post(self.BASE_URL, data=data7)
self.assertEqual(
response11.status_code,
status.HTTP_500_INTERNAL_SERVER_ERROR,
msg=response11.data,
)
self.assertEqual(response11.data[0], e_msg8)

def test_resize(self):
"""
Test PUT request to update size of share
Expand Down Expand Up @@ -627,7 +641,6 @@ def test_delete_exported_replicated(self):
self.assertEqual(response9.data[0], e_msg)

def test_delete_with_regular_snapshot(self):

# Delete share with regular snapshot and nothing else.

share = Share.objects.get(name="share-with-snap") # share - regular snap only
Expand Down Expand Up @@ -680,7 +693,6 @@ def test_delete_rock_ons_root(self):
self.assertEqual(response8.data[0], e_msg)

def test_delete_os_exception(self):

# Delete share mocking OS exception
share = Share.objects.get(name="root-share") # no associated exports/services
sId = share.id # from fixture - rock-ons-root
Expand Down
77 changes: 37 additions & 40 deletions src/rockstor/storageadmin/views/share.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@
qgroup_id,
qgroup_create,
share_pqgroup_assign,
ROOT_SUBVOL_EXCLUDE,
CREATE_SUBVOL_EXCLUDE,
SUBVOL_EXCLUDE,
)
from system.services import systemctl
from storageadmin.serializers import ShareSerializer, SharePoolSerializer
Expand Down Expand Up @@ -67,9 +70,7 @@ def _validate_share_size(request, pool):
except:
handle_exception(Exception("Share size must be an integer."), request)
if size < settings.MIN_SHARE_SIZE:
e_msg = (
"Share size should be at least {} KB. Given size is {} KB."
).format(settings.MIN_SHARE_SIZE, size)
e_msg = f"Share size should be at least {settings.MIN_SHARE_SIZE} KB. Given size is {size} KB."
handle_exception(Exception(e_msg), request)
if size > pool.size:
return pool.size
Expand All @@ -81,9 +82,7 @@ def _validate_compression(request):
if compression is None:
compression = "no"
if compression not in settings.COMPRESSION_TYPES:
e_msg = ("Unsupported compression algorithm ({}). Use one of {}.").format(
compression, settings.COMPRESSION_TYPES
)
e_msg = f"Unsupported compression algorithm ({compression}). Use one of {settings.COMPRESSION_TYPES}."
handle_exception(Exception(e_msg), request)
return compression

Expand All @@ -92,14 +91,11 @@ def _validate_share(request, sid):
try:
share = Share.objects.get(id=sid)
if share.name == "home" or share.name == "root":
e_msg = (
"Operation not permitted on this share ({}) because "
"it is a special system share."
).format(share.name)
e_msg = f"Operation not permitted on this share ({share.name}) because it is a special system share."
handle_exception(Exception(e_msg), request)
return share
except Share.DoesNotExist:
e_msg = "Share id ({}) does not exist.".format(sid)
e_msg = f"Share id ({sid}) does not exist."
handle_exception(Exception(e_msg), request)


Expand Down Expand Up @@ -152,7 +148,7 @@ def post(self, request):
# We will set the qgroup limit on our qgroup and it will enforce the
# quota on every subvolume(i.e., Share and Snapshot) in that qgroup.

# When a Share is deleted, we need to destroy two qgroups. One is it's
# When a Share is deleted, we need to destroy two qgroups. One is its
# auto 0/x qgroup and the other is our explicitly-created 2015/y
# qgroup.

Expand All @@ -161,7 +157,7 @@ def post(self, request):
try:
pool = Pool.objects.get(name=pool_name)
except:
e_msg = "Pool ({}) does not exist.".format(pool_name)
e_msg = f"Pool ({pool_name}) does not exist."
handle_exception(Exception(e_msg), request)
compression = self._validate_compression(request)
size = self._validate_share_size(request, pool)
Expand All @@ -179,28 +175,31 @@ def post(self, request):
e_msg = "Share name length cannot exceed 254 characters."
handle_exception(Exception(e_msg), request)

if sname in ROOT_SUBVOL_EXCLUDE + CREATE_SUBVOL_EXCLUDE + SUBVOL_EXCLUDE:
e_msg = (
f"Share name ({sname}) reserved for system. "
f"Choose a different name."
)
handle_exception(Exception(e_msg), request)

if Share.objects.filter(name=sname).exists():
# Note e_msg is consumed by replication/util.py create_share()
e_msg = ("Share ({}) already exists. Choose a different name.").format(
sname
)
e_msg = f"Share ({sname}) already exists. Choose a different name."
handle_exception(Exception(e_msg), request)

if Pool.objects.filter(name=sname).exists():
e_msg = (
"A pool with this name ({}) exists. Share "
f"A pool with this name ({sname}) exists. Share "
"and pool names must be distinct. Choose "
"a different name."
).format(sname)
)
handle_exception(Exception(e_msg), request)
replica = False
if "replica" in request.data:
replica = request.data["replica"]
if type(replica) != bool:
# TODO: confirm this 'type' call works as format parameter.
e_msg = ("Replica must be a boolean, not ({}).").format(
type(replica)
)
e_msg = f"Replica must be a boolean, not ({type(replica)})."
handle_exception(Exception(e_msg), request)
pqid = qgroup_create(pool)
add_share(pool, sname, pqid)
Expand Down Expand Up @@ -259,9 +258,9 @@ def put(self, request, sid):
if new_size < cur_rusage:
e_msg = (
"Unable to resize because requested new "
"size {} KB is less than current usage {} KB "
f"size {new_size} KB is less than current usage {cur_rusage} KB "
"of the share."
).format(new_size, cur_rusage)
)
handle_exception(Exception(e_msg), request)
# quota maintenance
if share.pool.quotas_enabled:
Expand Down Expand Up @@ -314,10 +313,10 @@ def _rockon_check(request, sname, force):
RockOn.objects.all().delete()
return
e_msg = (
"Share ({}) cannot be deleted because it is in use "
f"Share ({sname}) cannot be deleted because it is in use "
"by the Rock-on service. To override this block select "
"the force checkbox and try again."
).format(sname)
)
handle_exception(Exception(e_msg), request)

@transaction.atomic
Expand All @@ -331,48 +330,48 @@ def delete(self, request, sid, command=""):
share = self._validate_share(request, sid)
if Snapshot.objects.filter(share=share, snap_type="replication").exists():
e_msg = (
"Share ({}) cannot be deleted as it has replication "
f"Share ({share.name}) cannot be deleted as it has replication "
"related snapshots."
).format(share.name)
)
handle_exception(Exception(e_msg), request)

if NFSExport.objects.filter(share=share).exists():
e_msg = (
"Share ({}) cannot be deleted as it is exported via "
f"Share ({share.name}) cannot be deleted as it is exported via "
"NFS. Delete NFS exports and "
"try again."
).format(share.name)
)
handle_exception(Exception(e_msg), request)

if SambaShare.objects.filter(share=share).exists():
e_msg = (
"Share ({}) cannot be deleted as it is shared via "
f"Share ({share.name}) cannot be deleted as it is shared via "
"Samba. Unshare and try again."
).format(share.name)
)
handle_exception(Exception(e_msg), request)

if Snapshot.objects.filter(share=share).exists():
e_msg = (
"Share ({}) cannot be deleted as it has "
f"Share ({share.name}) cannot be deleted as it has "
"snapshots. Delete snapshots and "
"try again."
).format(share.name)
)
handle_exception(Exception(e_msg), request)

if SFTP.objects.filter(share=share).exists():
e_msg = (
"Share ({}) cannot be deleted as it is exported via "
f"Share ({share.name}) cannot be deleted as it is exported via "
"SFTP. Delete SFTP export and "
"try again."
).format(share.name)
)
handle_exception(Exception(e_msg), request)

if Replica.objects.filter(share=share.name).exists():
e_msg = (
"Share ({}) is configured for replication. If you "
f"Share ({share.name}) is configured for replication. If you "
"are sure, delete the replication task and "
"try again."
).format(share.name)
)
handle_exception(Exception(e_msg), request)

self._rockon_check(request, share.name, force=force)
Expand All @@ -381,9 +380,7 @@ def delete(self, request, sid, command=""):
remove_share(share.pool, share.subvol_name, share.pqgroup, force=force)
except Exception as e:
logger.exception(e)
e_msg = (
"Failed to delete the share ({}). Error from the OS: {}"
).format(share.name, e.__str__())
e_msg = f"Failed to delete the share ({share.name}). Error from the OS: {e.__str__()}"
handle_exception(Exception(e_msg), request)
share.delete()
return Response()

0 comments on commit a43235d

Please sign in to comment.