From a43235d017a4bcba457972a22e881e89fd38e101 Mon Sep 17 00:00:00 2001 From: Philip Guyton Date: Fri, 26 Jul 2024 19:14:52 +0100 Subject: [PATCH] Block creation of system reserved Share names #2881 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. --- src/rockstor/fs/btrfs.py | 8 +- .../storageadmin/tests/test_shares.py | 20 ++++- src/rockstor/storageadmin/views/share.py | 77 +++++++++---------- 3 files changed, 60 insertions(+), 45 deletions(-) diff --git a/src/rockstor/fs/btrfs.py b/src/rockstor/fs/btrfs.py index 96fc93bb2..134efb8af 100644 --- a/src/rockstor/fs/btrfs.py +++ b/src/rockstor/fs/btrfs.py @@ -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"] diff --git a/src/rockstor/storageadmin/tests/test_shares.py b/src/rockstor/storageadmin/tests/test_shares.py index 9fbfcf32c..cf6b33d6a 100644 --- a/src/rockstor/storageadmin/tests/test_shares.py +++ b/src/rockstor/storageadmin/tests/test_shares.py @@ -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" @@ -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 @@ -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 @@ -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 @@ -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 diff --git a/src/rockstor/storageadmin/views/share.py b/src/rockstor/storageadmin/views/share.py index e1c72b084..8d67c943e 100644 --- a/src/rockstor/storageadmin/views/share.py +++ b/src/rockstor/storageadmin/views/share.py @@ -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 @@ -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 @@ -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 @@ -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) @@ -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. @@ -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) @@ -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) @@ -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: @@ -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 @@ -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) @@ -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()