mon,osd,mds: allow fs authorize to update caps#41779
mon,osd,mds: allow fs authorize to update caps#41779rishabh-d-dave merged 6 commits intoceph:mainfrom
Conversation
|
@rishabh-d-dave I'll hold off on reviewing this because I suspect you want to do more work after the email discussion. Unless there's a particular change you want me to look at? |
|
If I could get a confirmation now that I am moving in the right direction, it would save me the effort of rewriting the code (specifically C++ code) later.
I am thinking to have a different ticket and PR for work related to the email discussion to keep the current patch set simple. However, this PR is not complete yet, I need to add few improvements to test code, docs and a note to PendingReleaseNotes. |
batrick
left a comment
There was a problem hiding this comment.
Otherwise it looks like you're heading in the right direction.
|
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
f46a01e to
b01065b
Compare
b01065b to
3eae558
Compare
7590583 to
2001bf2
Compare
2001bf2 to
d90a73c
Compare
|
@vshankar PTAL. Tests needs some cleanup, I'll ping you again when tests will be fully ready for review in next couple of days. Please let me know about the AuthMonitor part, so that I can start rewriting code to use parsers instead. |
d90a73c to
4096b2e
Compare
|
jenkins test make check arm64 |
When "fs authorize" subcommand is executed for the second time with different caps, the subcommand exits with error. Modify the behaviour so that the caps passed every subsequent time is incorporated in to the caps that are already present in the entity's keyring. Behaviour before this commit - $ ./bin/ceph fs authorize a client.x1 / rw [client.x1] key = AQBirqxg5KHeFxAAgOm6lHMYych6OTI+y1HJKw== $ ./bin/ceph fs authorize b client.x1 / rw Error EINVAL: client.x1 already has fs capabilities that differ from those supplied. To generate a new auth key for client.x1, first remove client.x1 from configuration files, execute 'ceph auth rm client.x1', then execute this command again. $ ./bin/ceph auth get client.x1 [client.x1] key = AQBirqxg5KHeFxAAgOm6lHMYych6OTI+y1HJKw== caps mds = "allow rw fsname=a" caps mon = "allow r fsname=a" caps osd = "allow rw tag cephfs data=a" exported keyring for client.x1 After this commit - $ ./bin/ceph fs authorize a client.x1 / rw [client.x1] key = AQDvrqxgU3I3FBAAJWwF6ZtcOVeHH8TA8CwWmQ== $ ./bin/ceph fs authorize b client.x1 / rw updated caps for client.x1 $ ./bin/ceph auth get client.x1 [client.x1] key = AQDvrqxgU3I3FBAAJWwF6ZtcOVeHH8TA8CwWmQ== caps mds = "allow rw fsname=a, allow rw fsname=b" caps mon = "allow r fsname=a, allow r fsname=b" caps osd = "allow rw tag cephfs data=a, allow rw tag cephfs data=b" exported keyring for client.x1 Fixes: https://tracker.ceph.com/issues/47264 Signed-off-by: Rishabh Dave <ridave@redhat.com> MDSAuthCaps: bug fixes Signed-off-by: Rishabh Dave <ridave@redhat.com>
If "fs authorize" subcommand is executed for a client that already has
a keyring but with no caps present in it, the command should update the
keyring with the caps supplied instead of quitting with an error message.
Example -
$ ./bin/ceph auth add client.x
added key for client.x
$ ./bin/ceph auth get client.x
[client.x]
key = AQCqOrJgtsJDHBAARGzbd1sj+ycRtWHOAcWz1w==
exported keyring for client.x
Before this commit -
$ ./bin/ceph fs authorize a client.x / rw
Error EINVAL: client.x already has fs capabilities that differ from those supplied. To generate a new auth key for client.x, first remove client.x from configuration files, execute 'ceph auth rm client.x', then execute this command again.
After this commit -
$ ./bin/ceph fs authorize a client.x1 / rw
updated caps for client.x1
Signed-off-by: Rishabh Dave <ridave@redhat.com>
Add tests for the case where "fs authorize" subcommand upgrades the caps for the client. Signed-off-by: Rishabh Dave <ridave@redhat.com>
|
jenkins test make check |
|
jenkins test api |
|
jenkins test docs |
|
Just for record: bugs introduced into this PR and on main branch were was introduced on PR #50569. |
|
@rishabh-d-dave do you want this in one of Yuri's batches? I see you added your own testing label, so wanted to confirm. |
Do you mean testing this PR with RADOS integration tests? |
|
@rishabh-d-dave yes, do you need us to put it through a rados batch? Either way, it should go through the rados suite since it has a "core" label. It's just a question whether you are already doing it, or if you want us to do it through Yuri's pipeline. |
|
jenkins retest this please |
|
@rishabh-d-dave I went ahead and put it in one of Yuri's batches. Feel free to continue with your own testing though if preferred. |
Thanks. Sorry for no reply from my end. I was super-busy last week due to some non-office work. I think this PR had gone through RADOS testing before, but I am not fully sure. Please let me know when the run is complete and results are out. |
|
Hi @rishabh-d-dave, tests have completed, so a review will be ready soon. |
|
jenkins retest this please |
|
@rishabh-d-dave you can monitor progress here: https://trello.com/c/Fllj7bVM/1833-wip-yuri8-testing-2023-08-28-1340 |
|
jenkins test docs |
|
This failure message looks legit. Once this review of integration test run is posted, I'll fix it. https://github.com/ceph/ceph/pull/41779/checks?check_run_id=16524795501 |
rishabh-d-dave
left a comment
There was a problem hiding this comment.
CephFS integration tests ran fine - https://tracker.ceph.com/projects/cephfs/wiki/Main#6-Sep-2023.
|
Feel free to merge when the docs issue is fixed. |
|
Fixed a minor doc issue. Waiting for CI to be green, after this PR can be merged since both CephFS and RADOS tests ran successfully. |
Signed-off-by: Rishabh Dave <ridave@redhat.com>
Replace call to run_ceph_cmd() by call to get_ceph_cmd_stdout() in method qa.tasks.cephfs.cephfs_test_case.CephFSTestCase.create_client(). run_ceph_cmd() will not return keyring which is wrong. get_ceph_cmd_stdout() will return the stdout of "ceph auth add" command, which is the keyring that is expected to be returned by CephFSTestCase.create_client(). Fixes: https://tracker.ceph.com/issues/62246 Signed-off-by: Rishabh Dave <ridave@redhat.com>
|
Minor doc update. Waiting for CI so that this PR can be merged. |
|
jenkins test make check |
|
jenkins test api |
Related PRs: #42335, #46991, #46993, #46994, #47013, #50312, #50497, #50664, #50665, #50874, #50876, #50882, #51004, #51127 and #52008.
Modifies the code for
fs authorizeto update auth caps instead of exiting with an error when entity already has caps (whether or not for the FS). Very related case where this modification has an impact is that, unlike before, auth caps are updated even when the entity has no capsFixes: https://tracker.ceph.com/issues/47264
Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume tox