MDSAuthCaps: print better error message for perm flag in MDS caps by rishabh-d-dave · Pull Request #52042 · ceph/ceph

rishabh-d-dave · 2023-06-13T19:30:15Z

Permissions mentioned in MDS caps flags can either begin with "r" or
"rw", but it can't start with "w" or be just "w". This is confusing for
some CephFS users since MON caps can be just "w".

Command "ceph fs authorize" informs users that permission MDS caps have
to start with "r" or "rw" but other commands like "auth add", "auth
caps", "auth get-or-create" don't. Make these commands to print a
helpful message, the way "ceph fs authorize" command does.

https://tracker.ceph.com/issues/61666

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

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)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

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

rishabh-d-dave · 2023-06-15T10:07:52Z

jenkins test make check

rishabh-d-dave · 2023-06-18T13:20:19Z

jenkins test api

src/mon/AuthMonitor.cc

src/mds/MDSAuthCaps.cc

rishabh-d-dave

CephFS integration tests passed - https://tracker.ceph.com/projects/cephfs/wiki/Main#6-Sep-2023-Run-2

rishabh-d-dave · 2023-09-06T15:29:15Z

jenkins test make check

rishabh-d-dave · 2023-09-06T15:29:59Z

jenkins test make check

After make check passes this PR can be merged.

qa/tasks/cephfs/cephfs_test_case.py

rishabh-d-dave · 2023-09-07T09:35:46Z

jenkins test make check

rishabh-d-dave · 2023-09-07T12:48:05Z

The make check failure is strange -

flake8: commands[0] /home/jenkins-build/build/workspace/ceph-pull-requests/qa> flake8 --select=F,E9 --exclude=venv,.tox
./tasks/cephfs/cephfs_test_case.py:458:28: F821 undefined name 'StringIO'
flake8: exit 1 (5.34 seconds) /home/jenkins-build/build/workspace/ceph-pull-requests/qa> flake8 --select=F,E9 --exclude=venv,.tox pid=2139318
flake8: FAIL ✖ in 9.49 seconds

But there is not "name" StringIO in these lines -

        if exp_retval is None and exp_errmsgs is None:
            raise RuntimeError('Method didn\'t get enough parameters. Pass '
                               'return value or error message expected from '
                               'the command/process.')

https://jenkins.ceph.com/job/ceph-pull-requests/121500/consoleFull#5759820286733401c-e9d0-4737-9832-6594c5da0afa

rishabh-d-dave · 2023-09-07T12:53:28Z

jenkins test make check

rishabh-d-dave · 2023-09-07T13:42:40Z

Same issue again -

flake8: commands[0] /home/jenkins-build/build/workspace/ceph-pull-requests/qa> flake8 --select=F,E9 --exclude=venv,.tox
./tasks/cephfs/cephfs_test_case.py:458:28: F821 undefined name 'StringIO'
flake8: exit 1 (3.77 seconds) /home/jenkins-build/build/workspace/ceph-pull-requests/qa> flake8 --select=F,E9 --exclude=venv,.tox pid=2482217
flake8: FAIL ✖ in 9.34 seconds

https://jenkins.ceph.com/job/ceph-pull-requests/121516/consoleFull#-1695845042e840cee4-f4a4-4183-81dd-42855615f2c1

rishabh-d-dave · 2023-09-07T13:43:51Z

Same command locally doesn't detect any issue -

$ git branch
* better-errmsg-for-perm
  main
$
$ flake8 --select=F,E9 --exclude=venv,.tox qa/tasks/cephfs/
$

vshankar · 2023-09-07T15:56:42Z

Same issue again -

Which means something is boken - checking.

vshankar · 2023-09-07T16:05:10Z

@rishabh-d-dave https://ceph-storage.slack.com/archives/C04LVQMHM9B/p1694096206947159

idryomov · 2023-09-07T19:24:12Z

qa/tasks/cephfs/cephfs_test_case.py

+        # conducting negative testing
+        kwargs['check_status'] = False
+        # stderr is needed to check for expected error messages.
+        kwargs['stderr'] = StringIO()


flake8: commands[0] /home/jenkins-build/build/workspace/ceph-pull-requests/qa> flake8 --select=F,E9 --exclude=venv,.tox
./tasks/cephfs/cephfs_test_case.py:458:28: F821 undefined name 'StringIO'
flake8: exit 1 (5.34 seconds) /home/jenkins-build/build/workspace/ceph-pull-requests/qa> flake8 --select=F,E9 --exclude=venv,.tox pid=2139318
flake8: FAIL ✖ in 9.49 seconds

@rishabh-d-dave It's complaining about this line. make check merges the branch into main before building and running tests.

Thanks for the reply. StringIO is imported at the top of this file, so it can't be undefined. See - https://github.com/rishabh-d-dave/ceph/blob/better-errmsg-for-perm/qa/tasks/cephfs/cephfs_test_case.py#L5.

That import was removed in 4b369cf by you yourself ;)

True, thanks! Following Laura's steps I found out same.

Thank you so much for help!

Also, add comments to explain the users the arguments are accepted by run_ceph_cmd(), get_ceph_cmd_result(), get_ceph_cmd_stdout() and negtest_ceph_cmd() methods of class RunCephCmd. Signed-off-by: Rishabh Dave <ridave@redhat.com>

Permissions mentioned in MDS caps flags can either begin with "r" or "rw", or can be "*" and "all". But it can't start with or be just "w" or something else. This is confusing for some CephFS users since MON caps can be just "w". Command "ceph fs authorize" complains about this to the user. But other commands (specifically, "ceph auth add", "ceph auth caps", "ceph auth get-or-create" and "ceph auth get-or-create-key") don't. Make these commands too print a helpful message, the way "ceph fs authorize" command does. Fixes: https://tracker.ceph.com/issues/61666 Signed-off-by: Rishabh Dave <ridave@redhat.com>

For "fs authorize" command, AuthMonitor.cc checks if permissions is "r" or begins with "rw". This check is redundant now. AuthMonitor::valid_caps() runs MDSAuthCaps.parse() which now runs same check for the MDS caps, regardless of the command. Signed-off-by: Rishabh Dave <ridave@redhat.com>

rishabh-d-dave

https://tracker.ceph.com/projects/cephfs/wiki/Main#22-Sep-2023

rishabh-d-dave · 2023-09-22T14:33:12Z

@vshankar @gregsfortytwo @batrick Should this PR be backported? Leaving the ticket unupdated until then...

vshankar · 2023-09-22T14:54:08Z

@vshankar @gregsfortytwo @batrick Should this PR be backported? Leaving the ticket unupdated until then...

yes, I think so.

rishabh-d-dave requested a review from a team as a code owner June 13, 2023 19:30

rishabh-d-dave added cephfs Ceph File System needs-review labels Jun 13, 2023

rishabh-d-dave requested a review from a team June 13, 2023 19:30

github-actions bot added core mon tests labels Jun 13, 2023

rishabh-d-dave requested a review from rzarzynski June 13, 2023 19:31

rishabh-d-dave force-pushed the better-errmsg-for-perm branch 2 times, most recently from 3a8120a to 09f016f Compare June 13, 2023 19:53

rishabh-d-dave changed the title ~~src/mon/AuthMonitor: add a check for perm flag in MDS caps~~ AuthMonitor: print better error message for perm flag in MDS caps Jun 13, 2023

rishabh-d-dave force-pushed the better-errmsg-for-perm branch from 09f016f to dcf08d1 Compare June 13, 2023 20:02

rishabh-d-dave requested a review from vshankar June 14, 2023 13:36

rishabh-d-dave force-pushed the better-errmsg-for-perm branch from dcf08d1 to 1e9db39 Compare June 16, 2023 20:49

dparmar18 reviewed Jun 18, 2023

View reviewed changes

src/mon/AuthMonitor.cc Outdated Show resolved Hide resolved

vshankar reviewed Jun 21, 2023

View reviewed changes

src/mon/AuthMonitor.cc Outdated Show resolved Hide resolved

neesingh-rh reviewed Jun 21, 2023

View reviewed changes

src/mon/AuthMonitor.cc Outdated Show resolved Hide resolved

rishabh-d-dave force-pushed the better-errmsg-for-perm branch from 1e9db39 to 42ec3b7 Compare June 22, 2023 08:05

rishabh-d-dave requested review from dparmar18, neesingh-rh and vshankar June 22, 2023 08:08

rishabh-d-dave changed the title ~~AuthMonitor: print better error message for perm flag in MDS caps~~ MDSAuthCaps: print better error message for perm flag in MDS caps Jun 22, 2023

vshankar reviewed Jun 23, 2023

View reviewed changes

src/mds/MDSAuthCaps.cc Outdated Show resolved Hide resolved

rishabh-d-dave force-pushed the better-errmsg-for-perm branch 3 times, most recently from 8e33571 to fe3e22b Compare June 28, 2023 15:08

rishabh-d-dave force-pushed the better-errmsg-for-perm branch from fe3e22b to 361eb7b Compare July 11, 2023 15:35

github-actions bot added the needs-rebase label Jul 13, 2023

rishabh-d-dave removed the wip-rishabh-testing Rishabh's testing label label Aug 24, 2023

rishabh-d-dave force-pushed the better-errmsg-for-perm branch from a2c5d02 to f200f5c Compare August 24, 2023 19:26

rishabh-d-dave added the wip-rishabh-testing Rishabh's testing label label Aug 24, 2023

rishabh-d-dave commented Sep 6, 2023

View reviewed changes

rishabh-d-dave removed the needs-review label Sep 6, 2023

vshankar reviewed Sep 7, 2023

View reviewed changes

qa/tasks/cephfs/cephfs_test_case.py Outdated Show resolved Hide resolved

idryomov reviewed Sep 7, 2023

View reviewed changes

rishabh-d-dave force-pushed the better-errmsg-for-perm branch from f200f5c to 8b9e3f9 Compare September 8, 2023 14:43

rishabh-d-dave added 3 commits September 8, 2023 20:17

rishabh-d-dave force-pushed the better-errmsg-for-perm branch from 8b9e3f9 to 8e1e322 Compare September 8, 2023 14:52

rishabh-d-dave commented Sep 22, 2023

View reviewed changes

rishabh-d-dave merged commit c4f968b into ceph:main Sep 22, 2023

rishabh-d-dave deleted the better-errmsg-for-perm branch September 22, 2023 15:01

Conversation

rishabh-d-dave commented Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

rishabh-d-dave commented Jun 15, 2023

Uh oh!

rishabh-d-dave commented Jun 18, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rishabh-d-dave left a comment

Choose a reason for hiding this comment

Uh oh!

rishabh-d-dave commented Sep 6, 2023

Uh oh!

rishabh-d-dave commented Sep 6, 2023

Uh oh!

Uh oh!

rishabh-d-dave commented Sep 7, 2023

Uh oh!

rishabh-d-dave commented Sep 7, 2023

Uh oh!

rishabh-d-dave commented Sep 7, 2023

Uh oh!

rishabh-d-dave commented Sep 7, 2023

Uh oh!

rishabh-d-dave commented Sep 7, 2023

Uh oh!

vshankar commented Sep 7, 2023

Uh oh!

vshankar commented Sep 7, 2023

Uh oh!

idryomov Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

rishabh-d-dave Sep 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

idryomov Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

rishabh-d-dave Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

rishabh-d-dave left a comment

Choose a reason for hiding this comment

Uh oh!

rishabh-d-dave commented Sep 22, 2023

Uh oh!

vshankar commented Sep 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rishabh-d-dave commented Jun 13, 2023 •

edited

Loading

rishabh-d-dave Sep 7, 2023 •

edited

Loading