Skip to content

mon, cephfs: Add auth caps for CephFS fsids#32581

Merged
ajarr merged 15 commits intoceph:masterfrom
rishabh-d-dave:wip-djf-15070
Sep 11, 2020
Merged

mon, cephfs: Add auth caps for CephFS fsids#32581
ajarr merged 15 commits intoceph:masterfrom
rishabh-d-dave:wip-djf-15070

Conversation

@rishabh-d-dave
Copy link
Contributor

@rishabh-d-dave rishabh-d-dave commented Jan 9, 2020

Fixes: https://tracker.ceph.com/issues/15070
First 3 commits are rebased and modified version of PR #26855.

  • Add new MON and MDS auth caps to restrict access based on fsnames.
  • Allow passing fsnames and paths in same cap.
  • Make fs authorize subcommand assign MON cap specific to that FS
  • Update doc client-auth for the changes above and improve it.
  • Make changes to qa/ code to support multi-FS tests -
    • Create new CephFSMount attributes for client's keyring, CephFS name and mountpoints on host and Ceph FS
    • Update all mount creation calls to use keyword arguments.
    • Allow reusing mount objects
    • Add remount method to remount Ceph FS in a single call
    • Allow not aborting when mount command fails (useful for negative tests)
    • Improve filesystem.py
      • Add a method to destroy an FS
      • Modify Filesytem.recreate() and mds_cluster.delete_all_filesystems()
    • Add helper methods to read/write files from CephFS mounts
    • Improve setup/teardown for CephFS tests in general
  • Add tests for multifs_auth and for fs authorize subcommand.

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug

Show available Jenkins commands
  • jenkins retest this please
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard backend
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@rishabh-d-dave
Copy link
Contributor Author

@batrick and @fullerdj if the PR looks fine, what next? Anything besides adding tests? Some of the changes requested on #26855 are outstanding, I'll go ahead with right now.

@fullerdj
Copy link
Contributor

fullerdj commented Jan 9, 2020

Hi Rishabh,

This will need a test associated with it.

Copy link
Member

@batrick batrick left a comment

Choose a reason for hiding this comment

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

Also need to address especially: #26855 (comment)

@gregsfortytwo @liewegas do you have a suggestion on how that would look? mon allow r fscid=<fscid>?

@gregsfortytwo
Copy link
Member

Also need to address especially: #26855 (comment)

@gregsfortytwo @liewegas do you have a suggestion on how that would look? mon allow r fscid=<fscid>?

Yes, that looks right to me if you're trying to restrict a user to reading only from fscid 1. As you note in the previous comment, an "allow rw" would supersede any separate "allow fscid=N" clause, so they'd need to be stuck together in any case.
Was that the whole question?

@batrick
Copy link
Member

batrick commented Jan 21, 2020

Also need to address especially: #26855 (comment)
@gregsfortytwo @liewegas do you have a suggestion on how that would look? mon allow r fscid=<fscid>?

Yes, that looks right to me if you're trying to restrict a user to reading only from fscid 1. As you note in the previous comment, an "allow rw" would supersede any separate "allow fscid=N" clause, so they'd need to be stuck together in any case.
Was that the whole question?

Yes, I think so. Thanks!

@batrick batrick added this to the octopus milestone Jan 24, 2020
@batrick
Copy link
Member

batrick commented Jan 30, 2020

Needs rebase

@rishabh-d-dave rishabh-d-dave force-pushed the wip-djf-15070 branch 3 times, most recently from 78c6eb8 to 5a6df79 Compare February 11, 2020 17:38
@rishabh-d-dave rishabh-d-dave added the wip-rishabh-testing Rishabh's testing label label Feb 12, 2020
@rishabh-d-dave
Copy link
Contributor Author

Testing bits of my patch in that applies in qa that can't be tested with vstart_runner.py

@rishabh-d-dave rishabh-d-dave removed the wip-rishabh-testing Rishabh's testing label label Feb 12, 2020
@rishabh-d-dave rishabh-d-dave force-pushed the wip-djf-15070 branch 5 times, most recently from a3d13e4 to 1b02da6 Compare February 17, 2020 15:07
@batrick batrick removed this from the octopus milestone Feb 19, 2020
@rishabh-d-dave
Copy link
Contributor Author

@batrick

Teuthology testing fails with both kernel as well as FUSE client. Here's the links to the tests -
http://pulpito.ceph.com/rishabh-2020-02-14_12:07:11-fs-wip-rishabh-wip-djf-15070-distro-basic-smithi/
http://pulpito.ceph.com/rishabh-2020-02-18_05:31:37-kcephfs-wip-rishabh-wip-djf-15070-distro-basic-smithi/

Here's the failure of kernel client -

======
2020-02-18T13:23:43.531 INFO:tasks.cephfs_test_runner:FAIL: test_write (tasks.cephfs.test_multifs.TestClientsWithOutAuth)
2020-02-18T13:23:43.531 INFO:tasks.cephfs_test_runner:----------------------------------------------------------------------
2020-02-18T13:23:43.531 INFO:tasks.cephfs_test_runner:Only have 1 clients, require 2
2020-02-18T13:23:43.531 INFO:tasks.cephfs_test_runner:----------------------------------------------------------------------
2020-02-18T13:23:43.531 INFO:tasks.cephfs_test_runner:Ran 5 tests in 5.179s
2020-02-18T13:23:43.532 INFO:tasks.cephfs_test_runner:

Looks like I need to modify some file qa/suites to provision 2 clients instead of 1.

And, here's the failure for FUSE client -

Traceback (most recent call last):
  File "/home/teuthworker/src/git.ceph.com_git_teuthology_master/teuthology/run_tasks.py", line 89, in run_tasks
    manager.__enter__()
  File "/usr/lib/python2.7/contextlib.py", line 17, in __enter__
    return self.gen.next()
  File "/home/teuthworker/src/git.ceph.com_ceph-c_wip-rishabh-wip-djf-15070/qa/tasks/ceph_fuse.py", line 137, in task
    mount.mount()
  File "/home/teuthworker/src/git.ceph.com_ceph-c_wip-rishabh-wip-djf-15070/qa/tasks/cephfs/fuse_mount.py", line 39, in mount
    return self._mount(mount_path, mount_fs_name)
  File "/home/teuthworker/src/git.ceph.com_ceph-c_wip-rishabh-wip-djf-15070/qa/tasks/cephfs/fuse_mount.py", line 144, in _mount
    waited
RuntimeError: Fuse mount failed to populate /sys/ after 31 seconds

Looks like the command to mount was executed successfully but the mount itself failed for some reason. I've got not idea what's the reason behind this. The remounting is different from any remounting in the sense that this time mounting happens with a different client ID and keyring,

FYI: These tests ran successfully when I tested with vstart_runner.py.

This commit introduces following two set of changes -

First, make client keyring path, mountpoint on host FS and CephFS and
CephFS's name attributes of the object representing the mount
and update all the mount object creation calls accordingly. Also,
rewrite all the mount object creation to use keyword arguments instead
of positional arguments to avoid mistakes, especially since a new
argument was added in this commit.

Second, add remount method to mount.py so that it's possible to unmount
safely, modify the attributes of the object representing the mount and
mount again based on new state of the object *in a single call*. The
method is placed in mount.py to avoid duplication.

This change has two leads to two more changes: upgrading interface of
mount() and mount_wait() and upgrading testsuites to adapt to these
change.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
This commit adds a new argument check_status to mount methods of
KernelMount, FuseMount, LocalKernelMount and LocalFuseMount. When value
of this argument is False, these methods would catch the
CommandFailedError exception and would return a tuple consisting of the
exception itself, and stdout and stderr of the mount command. This
allows reusing these mount methods while running negative tests for
commands.

The name "check_status" is selected so since teuthology's run() and
vstart_runner's run() use a variable with same name for the very same
purpose.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
And reset_obj_attrs parameter to it so that the caller of the method can
choose to destroy the Ceph FS represented by the object without
disturbing the object attributes.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
Modify cephfs.filesystem.Filesystem.recreate() method to delete only the
FS represented by the object instead of deleting the every FS on the
Ceph cluster.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave
Copy link
Contributor Author

Added DNM to test changes the fix for ceph API tests.

Modify filesystem.Filesystem.delete_all_filesystems() method to make it
more succinct, move it to class MDSCluster instead and update every call
to it accordingly.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave
Copy link
Contributor Author

ceph API tests passed - https://jenkins.ceph.com/job/ceph-api/2802/

@rishabh-d-dave
Copy link
Contributor Author

jenkins test make check

@rishabh-d-dave
Copy link
Contributor Author

jenkins render docs

@ceph-jenkins
Copy link
Collaborator

Doc render available at http://docs.ceph.com/ceph-prs/32581/

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Sep 11, 2020

Add testsuite for testing authorization on Ceph cluster with multiple
file systems and enable it to be executable with Teuthology framework.

Also add helper methods required to setup the test environment for
multi-FS tests.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
Right now, only client IDs are stashed and restored but with the recent
changes (addition of more attributes to mount objects, specifically),
this is not enough. Saving and restoring these details before and after
tests respectively ensures that mount commands rus smoothly. Not doing
this typically leads to mount command failure for the second test in the
testsuite under execution since the client IDs are saved and restored in
CephFSTestCase.setUp and CephFSTestCase.tearDown respectively but the
rest of the details are not.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
Make caps FS-specific affects "fs authorize" subcommand. Let's add few
tests to verify its behaviour.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave
Copy link
Contributor Author

@rishabh-d-dave
Copy link
Contributor Author

jenkins test make check

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Sep 11, 2020

@ajarr
Copy link
Contributor

ajarr commented Sep 11, 2020

Thanks, Rishabh!

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Sep 11, 2020

@ajarr @batrick Thanks for all the review and help. :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants