Skip to content

mds: allow all types of mds caps#51317

Merged
rishabh-d-dave merged 3 commits intoceph:mainfrom
rishabh-d-dave:mdscaps-combinations
Jul 15, 2023
Merged

mds: allow all types of mds caps#51317
rishabh-d-dave merged 3 commits intoceph:mainfrom
rishabh-d-dave:mdscaps-combinations

Conversation

@rishabh-d-dave
Copy link
Contributor

@rishabh-d-dave rishabh-d-dave commented May 2, 2023

Fixes: https://tracker.ceph.com/issues/59388

Contribution Guidelines

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • 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 cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows

@github-actions github-actions bot added the cephfs Ceph File System label May 2, 2023
gregsfortytwo
gregsfortytwo previously requested changes May 2, 2023
View reviewed changes
Copy link
Member

@gregsfortytwo gregsfortytwo left a comment

Choose a reason for hiding this comment

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

This was a good discovery (of something that scares me) and a nice start. Still need some improvements to testing to make sure the fix is complete. :)

@vshankar vshankar requested a review from a team May 2, 2023 13:52
vshankar
vshankar reviewed May 3, 2023
View reviewed changes
@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented May 9, 2023

jenkins test make check

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented May 9, 2023

https://jenkins.ceph.com/job/ceph-pull-requests/114916/

@github-actions github-actions bot added the tests label May 9, 2023
@rishabh-d-dave rishabh-d-dave force-pushed the mdscaps-combinations branch 3 times, most recently from 174b7db to 087f92b Compare May 9, 2023 12:39
@gregsfortytwo gregsfortytwo dismissed their stale review May 9, 2023 14:12

test updates look good

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented May 11, 2023

https://jenkins.ceph.com/job/ceph-pull-requests/114928/consoleFull#7654512942a811ea2-3e7b-466b-84b4-d13df7e35809

@rishabh-d-dave rishabh-d-dave force-pushed the mdscaps-combinations branch 9 times, most recently from 0b313d5 to 2a4afce Compare May 16, 2023 11:07
@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented May 17, 2023

jenkins test api

@rishabh-d-dave rishabh-d-dave force-pushed the mdscaps-combinations branch 2 times, most recently from 23dd800 to f5f58cf Compare June 9, 2023 15:53
@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Jun 12, 2023

Strange the ceph API tests failed because it looks like test runner exited with success -

2023-06-09 17:09:13,467.467 INFO:__main__:----------------------------------------------------------------------
2023-06-09 17:09:13,467.467 INFO:__main__:Ran 308 tests in 3871.918s
2023-06-09 17:09:13,467.467 INFO:__main__:
2023-06-09 17:09:13,467.467 INFO:__main__:

https://jenkins.ceph.com/job/ceph-api/56590/

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Jun 12, 2023 •
edited
Loading

@gregsfortytwo @vshankar
GIDs in MDS caps work if and only if they are mentioned in the cap along with UID. Following 2 cases demonstrate this. The former only has GIDs it MDS cap and latter has both UID and GID. Former mounts successfully and latter fails to mount.

$ cat ceph.client.x1.keyring 
[client.x1] 
        key = AQDy+oZkoWJvLxAAeRTzmjAqZ3FD04DHGKLf1Q== 
        caps mds = "allow rw gids=1001" 
        caps mon = "allow r fsname=a" 
        caps osd = "allow rw tag cephfs data=a" 

$ cat ceph.client.x2.keyring 
[client.x2] 
        key = AQDM+4ZkdoUTLxAAyYZq+AAy2LerkSKtE0NjeQ== 
        caps mds = "allow rw uid=1001 gids=1001" 
        caps mon = "allow r fsname=a" 
        caps osd = "allow rw tag cephfs data=a" 

$ ./bin/ceph-fuse cephfs1 --client_fs a --id x1 -k ceph.client.x1.keyring
2023-06-12T16:36:17.449+0530 7faab6c39500 -1 init, newargv = 0x7faaa401c290 newargc=15
ceph-fuse[27741]: starting ceph client
ceph-fuse[27741]: starting fuse

$ ./bin/ceph-fuse cephfs2 --client_fs a --id x2 -k ceph.client.x2.keyring
2023-06-12T16:36:22.063+0530 7f7938560500 -1 init, newargv = 0x7f792401c1d0 newargc=15
ceph-fuse[27799]: starting ceph client
ceph-fuse[27799]: ceph mount failed with (13) Permission denied

Should an MDS caps with GID should be allowed? IMO it should be. If a CephFS user wants to allow 5 Linux users to access a certain CephFS, he/she can simply add all of the users to a group and then create an MDS cap mentioning the GID. The cap would look like this allow rw fsname =cephfs1 gids=1001. Currently, the user would have to do multiple cap for each user: allow rw fsname=cephfs1 uid=1001, allow rw fsname=cephfs1 uid=1002, allow rw fsname=cephfs1 uid=1003, allow rw fsname=cephfs1 uid=1004, allow rw fsname=cephfs1 uid=1005.

Obviously, former style is easier on eyes and is also better scalable and manageable.

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Jun 16, 2023

jenkins test api

@github-actions
Copy link

github-actions bot commented Jun 16, 2023

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Jun 16, 2023

Rebased and resolved the merge conflicts.

@vshankar
Copy link
Contributor

vshankar commented Jun 23, 2023

@rishabh-d-dave please run this through QA and post the result.

@kotreshhr
Copy link
Contributor

kotreshhr commented Jun 23, 2023 •
edited
Loading

GIDs in MDS caps work if and only if they are mentioned in the cap along with UID.

@rishabh-d-dave In your results, the cap with only gids mentioned could successfully mount. Am I missing something?

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Jun 27, 2023 •
edited
Loading

@kotreshhr

GIDs in MDS caps work if and only if they are mentioned in the cap along with UID.

@rishabh-d-dave In your results, the cap with only gids mentioned could successfully mount. Am I missing something?

I am confused by what you mean. We expect only GIDs 1001,1002,1003 should be able to mount when MDS cap is allow rw gids=1001,1002,1003. But that is not the case, other GIDs too can mount CephFS.

@rishabh-d-dave rishabh-d-dave added the wip-rishabh-testing Rishabh's testing label label Jul 5, 2023
rishabh-d-dave
rishabh-d-dave commented Jul 13, 2023
View reviewed changes
Copy link
Contributor Author

@rishabh-d-dave rishabh-d-dave left a comment

Choose a reason for hiding this comment

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

Tests were successful - https://tracker.ceph.com/projects/cephfs/wiki/Main#13-Jul-2023

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Jul 13, 2023

Tests were successful - https://tracker.ceph.com/projects/cephfs/wiki/Main#13-Jul-2023

@vshankar This PR is ready for merge but an approval is needed.

vshankar
vshankar reviewed Jul 13, 2023
View reviewed changes
@github-actions
Copy link

github-actions bot commented Jul 13, 2023

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

vshankar
vshankar approved these changes Jul 14, 2023
View reviewed changes
Copy link
Contributor

@vshankar vshankar left a comment

Choose a reason for hiding this comment

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

LGTM

@rishabh-d-dave
mds: allow all types of MDS caps
0c36929 
MDS caps can contain 5 components: name of a CephFS, a path inside
CephFS, a flag for enabling root squashing mechanism, a UID and list of
GIDs. These 5 components result in 31 combinations, so there can be 31
types of MDS caps. Out of these, the current main branch only allows 11
combinations. This restriction is strange and inappropriate. Ideally,
all combinations should be allowed.

This strange restriction must've been created unintentionally by
previous developers while adding FS name and root squash to MDS caps. A
TODO for a allowing a subset of these combination was also left in
codebase:
https://github.com/ceph/ceph/blob/reef/src/mds/MDSAuthCaps.cc#L69

Fixes: https://tracker.ceph.com/issues/59388
Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave
src/tests/mds: add tests for all combinations of MDS caps
248e9da 
Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave
src/tests/mds: improve test coverage in TestMDSAuthCaps
bfacee1 
Currently we check if the right caps parse successfully and the wrong
caps fail to parse. Increase the test coverage by adding one more
tests -- dump the right caps in to a string after the parsing is
successfully and then re-parse the caps and check if re-parsed caps
dumps to the exact same string.

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

rishabh-d-dave commented Jul 14, 2023 •
edited
Loading

Rebased PR branch to resolve the conflict. I'll merge as soon as the CI tests finish running successfully.

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Jul 14, 2023

jenkins test make check

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Jul 14, 2023

https://jenkins.ceph.com/job/ceph-pull-requests/118293/

@rishabh-d-dave rishabh-d-dave merged commit 5f1dece into ceph:main Jul 15, 2023
@rishabh-d-dave rishabh-d-dave deleted the mdscaps-combinations branch July 15, 2023 02:38
This was referenced Jul 21, 2023
Merged
Closed
Closed
@vshankar vshankar mentioned this pull request Apr 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lxbsz lxbsz lxbsz left review comments

@vshankar vshankar vshankar approved these changes

@gregsfortytwo gregsfortytwo gregsfortytwo left review comments

Assignees

No one assigned

Labels

cephfs Ceph File System tests wip-rishabh-testing Rishabh's testing label

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@rishabh-d-dave @vshankar @kotreshhr @gregsfortytwo @lxbsz