Merge pull request ceph#54277 from rhcs-dashboard/nfs-export-apply-fix

mgr/nfs: generate user_id & access_key for apply_export(CephFS)

Reviewed-by: Adam King <[email protected]>
Reviewed-by: Dhairya Parmar <[email protected]>
Reviewed-by: John Mulligan <[email protected]>

Author: adk3798

PendingReleaseNotes

@@ -279,6 +279,10 @@ CephFS: Disallow delegating preallocated inode ranges to clients. Config
 * RGW: in bucket notifications, the `principalId` inside `ownerIdentity` now contains
   complete user id, prefixed with tenant id
 
+* NFS: The export create/apply of CephFS based exports will now have a additional parameter `cmount_path` under the FSAL block,
+  which specifies the path within the CephFS to mount this export on. If this and the other
+  `EXPORT { FSAL {} }` options are the same between multiple exports, those exports will share a single CephFS client. If not specified, the default is `/`.
+
 >=18.0.0
 
 * The RGW policy parser now rejects unknown principals by default. If you are

doc/mgr/nfs.rst

@@ -283,7 +283,7 @@ Create CephFS Export
 
 .. code:: bash
 
-    $ ceph nfs export create cephfs --cluster-id <cluster_id> --pseudo-path <pseudo_path> --fsname <fsname> [--readonly] [--path=/path/in/cephfs] [--client_addr <value>...] [--squash <value>] [--sectype <value>...]
+    $ ceph nfs export create cephfs --cluster-id <cluster_id> --pseudo-path <pseudo_path> --fsname <fsname> [--readonly] [--path=/path/in/cephfs] [--client_addr <value>...] [--squash <value>] [--sectype <value>...] [--cmount_path <value>]
 
 This creates export RADOS objects containing the export block, where
 
@@ -318,6 +318,12 @@ values may be separated by a comma (example: ``--sectype krb5p,krb5i``). The
 server will negotatiate a supported security type with the client preferring
 the supplied methods left-to-right.
 
+``<cmount_path>`` specifies the path within the CephFS to mount this export on. It is
+allowed to be any complete path hierarchy between ``/`` and the ``EXPORT {path}``. (i.e. if ``EXPORT { Path }`` parameter is ``/foo/bar`` then cmount_path could be ``/``, ``/foo`` or ``/foo/bar``).
+
+.. note:: If this and the other ``EXPORT { FSAL {} }`` options are the same between multiple exports, those exports will share a single CephFS client.
+          If not specified, the default is ``/``.
+
 .. note:: Specifying values for sectype that require Kerberos will only function on servers
           that are configured to support Kerberos. Setting up NFS-Ganesha to support Kerberos
           is outside the scope of this document.
@@ -477,9 +483,9 @@ For example,::
      ],
      "fsal": {
        "name": "CEPH",
-       "user_id": "nfs.mynfs.1",
        "fs_name": "a",
-       "sec_label_xattr": ""
+       "sec_label_xattr": "",
+       "cmount_path": "/"
      },
      "clients": []
    }
@@ -494,6 +500,9 @@ as when creating a new export), with the exception of the
 authentication credentials, which will be carried over from the
 previous state of the export where possible.
 
+!! NOTE: The ``user_id`` in the ``fsal`` block should not be modified or mentioned in the JSON file as it is auto-generated for CephFS exports.
+It's auto-generated in the format ``nfs.<cluster_id>.<fs_name>.<hash_id>``.
+
 ::
 
    $ ceph nfs export apply mynfs -i update_cephfs_export.json
@@ -514,9 +523,9 @@ previous state of the export where possible.
      ],
      "fsal": {
        "name": "CEPH",
-       "user_id": "nfs.mynfs.1",
        "fs_name": "a",
-       "sec_label_xattr": ""
+       "sec_label_xattr": "",
+       "cmount_path": "/"
      },
      "clients": []
    }

qa/tasks/cephfs/test_nfs.py

@@ -14,6 +14,8 @@
 NFS_POOL_NAME = '.nfs'  # should match mgr_module.py
 
 # TODO Add test for cluster update when ganesha can be deployed on multiple ports.
+
+
 class TestNFS(MgrTestCase):
     def _cmd(self, *args):
         return self.get_ceph_cmd_stdout(args)
@@ -59,8 +61,9 @@ def setUp(self):
          ],
          "fsal": {
            "name": "CEPH",
-           "user_id": "nfs.test.1",
+           "user_id": "nfs.test.nfs-cephfs.3746f603",
            "fs_name": self.fs_name,
+           "cmount_path": "/",
          },
          "clients": []
         }
@@ -118,18 +121,20 @@ def _check_nfs_cluster_status(self, expected_status, fail_msg):
                     return
         self.fail(fail_msg)
 
-    def _check_auth_ls(self, export_id=1, check_in=False):
+    def _check_auth_ls(self, fs_name, check_in=False, user_id=None):
         '''
         Tests export user id creation or deletion.
         :param export_id: Denotes export number
         :param check_in: Check specified export id
         '''
         output = self._cmd('auth', 'ls')
         client_id = f'client.nfs.{self.cluster_id}'
+        search_id = f'client.{user_id}' if user_id else f'{client_id}.{fs_name}'
+
         if check_in:
-            self.assertIn(f'{client_id}.{export_id}', output)
+            self.assertIn(search_id, output)
         else:
-            self.assertNotIn(f'{client_id}.{export_id}', output)
+            self.assertNotIn(search_id, output)
 
     def _test_idempotency(self, cmd_func, cmd_args):
         '''
@@ -216,7 +221,7 @@ def _create_export(self, export_id, create_fs=False, extra_cmd=None):
         # Runs the nfs export create command
         self._cmd(*export_cmd)
         # Check if user id for export is created
-        self._check_auth_ls(export_id, check_in=True)
+        self._check_auth_ls(self.fs_name, check_in=True)
         res = self._sys_cmd(['rados', '-p', NFS_POOL_NAME, '-N', self.cluster_id, 'get',
                              f'export-{export_id}', '-'])
         # Check if export object is created
@@ -230,12 +235,12 @@ def _create_default_export(self):
         self._test_create_cluster()
         self._create_export(export_id='1', create_fs=True)
 
-    def _delete_export(self):
+    def _delete_export(self, pseduo_path=None, check_in=False, user_id=None):
         '''
         Delete an export.
         '''
-        self._nfs_cmd('export', 'rm', self.cluster_id, self.pseudo_path)
-        self._check_auth_ls()
+        self._nfs_cmd('export', 'rm', self.cluster_id, pseduo_path if pseduo_path else self.pseudo_path)
+        self._check_auth_ls(self.fs_name, check_in, user_id)
 
     def _test_list_export(self):
         '''
@@ -256,26 +261,27 @@ def _test_list_detailed(self, sub_vol_path):
         self.sample_export['export_id'] = 2
         self.sample_export['pseudo'] = self.pseudo_path + '1'
         self.sample_export['access_type'] = 'RO'
-        self.sample_export['fsal']['user_id'] = f'{self.expected_name}.2'
+        self.sample_export['fsal']['user_id'] = f'{self.expected_name}.{self.fs_name}.3746f603'
         self.assertDictEqual(self.sample_export, nfs_output[1])
         # Export-3 for subvolume with r only
         self.sample_export['export_id'] = 3
         self.sample_export['path'] = sub_vol_path
         self.sample_export['pseudo'] = self.pseudo_path + '2'
-        self.sample_export['fsal']['user_id'] = f'{self.expected_name}.3'
+        self.sample_export['fsal']['user_id'] = f'{self.expected_name}.{self.fs_name}.3746f603'
         self.assertDictEqual(self.sample_export, nfs_output[2])
         # Export-4 for subvolume
         self.sample_export['export_id'] = 4
         self.sample_export['pseudo'] = self.pseudo_path + '3'
         self.sample_export['access_type'] = 'RW'
-        self.sample_export['fsal']['user_id'] = f'{self.expected_name}.4'
+        self.sample_export['fsal']['user_id'] = f'{self.expected_name}.{self.fs_name}.3746f603'
         self.assertDictEqual(self.sample_export, nfs_output[3])
 
-    def _get_export(self):
+    def _get_export(self, pseudo_path=None):
         '''
         Returns export block in json format
         '''
-        return json.loads(self._nfs_cmd('export', 'info', self.cluster_id, self.pseudo_path))
+        return json.loads(self._nfs_cmd('export', 'info', self.cluster_id,
+                                        pseudo_path if pseudo_path else self.pseudo_path))
 
     def _test_get_export(self):
         '''
@@ -506,7 +512,7 @@ def test_create_multiple_exports(self):
         self._test_delete_cluster()
         # Check if rados ganesha conf object is deleted
         self._check_export_obj_deleted(conf_obj=True)
-        self._check_auth_ls()
+        self._check_auth_ls(self.fs_name)
 
     def test_exports_on_mgr_restart(self):
         '''
@@ -935,7 +941,7 @@ def test_nfs_export_apply_multiple_exports(self):
                     "protocols": [4],
                     "fsal": {
                         "name": "CEPH",
-                        "user_id": "nfs.test.1",
+                        "user_id": "nfs.test.nfs-cephfs.3746f603",
                         "fs_name": self.fs_name
                     }
                 },
@@ -948,7 +954,7 @@ def test_nfs_export_apply_multiple_exports(self):
                     "protocols": [4],
                     "fsal": {
                         "name": "CEPH",
-                        "user_id": "nfs.test.2",
+                        "user_id": "nfs.test.nfs-cephfs.3746f603",
                         "fs_name": "invalid_fs_name"  # invalid fs
                     }
                 },
@@ -961,7 +967,7 @@ def test_nfs_export_apply_multiple_exports(self):
                     "protocols": [4],
                     "fsal": {
                         "name": "CEPH",
-                        "user_id": "nfs.test.3",
+                        "user_id": "nfs.test.nfs-cephfs.3746f603",
                         "fs_name": self.fs_name
                     }
                 }
@@ -1008,7 +1014,7 @@ def test_nfs_export_apply_single_export(self):
                 "protocols": [4],
                 "fsal": {
                     "name": "CEPH",
-                    "user_id": "nfs.test.1",
+                    "user_id": "nfs.test.nfs-cephfs.3746f603",
                     "fs_name": "invalid_fs_name"  # invalid fs
                 }
             }
@@ -1048,7 +1054,7 @@ def test_nfs_export_apply_json_output_states(self):
                     "protocols": [4],
                     "fsal": {
                         "name": "CEPH",
-                        "user_id": "nfs.test.1",
+                        "user_id": "nfs.test.nfs-cephfs.3746f603",
                         "fs_name": self.fs_name
                     }
                 },
@@ -1061,7 +1067,7 @@ def test_nfs_export_apply_json_output_states(self):
                     "protocols": [4],
                     "fsal": {
                         "name": "CEPH",
-                        "user_id": "nfs.test.2",
+                        "user_id": "nfs.test.nfs-cephfs.3746f603",
                         "fs_name": self.fs_name
                     }
                 },
@@ -1075,7 +1081,7 @@ def test_nfs_export_apply_json_output_states(self):
                     "protocols": [4],
                     "fsal": {
                         "name": "CEPH",
-                        "user_id": "nfs.test.3",
+                        "user_id": "nfs.test.nfs-cephfs.3746f603",
                         "fs_name": "invalid_fs_name"
                     }
                 }
@@ -1211,3 +1217,65 @@ def test_cephfs_export_update_at_non_dir_path(self):
         finally:
             self.ctx.cluster.run(args=['rm', '-rf', f'{mnt_pt}/*'])
             self._delete_cluster_with_fs(self.fs_name, mnt_pt, preserve_mode)
+
+    def test_nfs_export_creation_without_cmount_path(self):
+        """
+        Test that ensure cmount_path is present in FSAL block
+        """
+        self._create_cluster_with_fs(self.fs_name)
+
+        pseudo_path = '/test_without_cmount'
+        self._create_export(export_id='1',
+                            extra_cmd=['--pseudo-path', pseudo_path])
+        nfs_output = self._get_export(pseudo_path)
+        self.assertIn('cmount_path', nfs_output['fsal'])
+
+        self._delete_export(pseudo_path)
+
+    def test_nfs_exports_with_same_and_diff_user_id(self):
+        """
+        Test that exports with same FSAL share same user_id
+        """
+        self._create_cluster_with_fs(self.fs_name)
+
+        pseudo_path_1 = '/test1'
+        pseudo_path_2 = '/test2'
+        pseudo_path_3 = '/test3'
+
+        # Create subvolumes
+        self._cmd('fs', 'subvolume', 'create', self.fs_name, 'sub_vol_1')
+        self._cmd('fs', 'subvolume', 'create', self.fs_name, 'sub_vol_2')
+
+        fs_path_1 = self._cmd('fs', 'subvolume', 'getpath', self.fs_name, 'sub_vol_1').strip()
+        fs_path_2 = self._cmd('fs', 'subvolume', 'getpath', self.fs_name, 'sub_vol_2').strip()
+        # Both exports should have same user_id(since cmount_path=/ & fs_name is same)
+        self._create_export(export_id='1',
+                            extra_cmd=['--pseudo-path', pseudo_path_1,
+                                       '--path', fs_path_1])
+        self._create_export(export_id='2',
+                            extra_cmd=['--pseudo-path', pseudo_path_2,
+                                       '--path', fs_path_2])
+
+        nfs_output_1 = self._get_export(pseudo_path_1)
+        nfs_output_2 = self._get_export(pseudo_path_2)
+        # Check if both exports have same user_id
+        self.assertEqual(nfs_output_2['fsal']['user_id'], nfs_output_1['fsal']['user_id'])
+        self.assertEqual(nfs_output_1['fsal']['user_id'], 'nfs.test.nfs-cephfs.3746f603')
+
+        cmount_path = '/volumes'
+        self._create_export(export_id='3',
+                            extra_cmd=['--pseudo-path', pseudo_path_3,
+                                       '--path', fs_path_1,
+                                       '--cmount-path', cmount_path])
+
+        nfs_output_3 = self._get_export(pseudo_path_3)
+        self.assertNotEqual(nfs_output_3['fsal']['user_id'], nfs_output_1['fsal']['user_id'])
+        self.assertEqual(nfs_output_3['fsal']['user_id'], 'nfs.test.nfs-cephfs.32cd8545')
+
+        # Deleting export with same user_id should not delete the user_id
+        self._delete_export(pseudo_path_1, True, nfs_output_1['fsal']['user_id'])
+        # Deleting export 22 should delete the user_id since it's only export left with that user_id
+        self._delete_export(pseudo_path_2, False, nfs_output_2['fsal']['user_id'])
+
+        # Deleting export 23 should delete the user_id since it's only export with that user_id
+        self._delete_export(pseudo_path_3, False, nfs_output_3['fsal']['user_id'])