Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mon: don't require OSD W for MRemoveSnaps #6601

Merged
merged 1 commit into from Dec 7, 2015

Conversation

jcsp
Copy link
Contributor

@jcsp jcsp commented Nov 16, 2015

Use ability to execute "osd pool rmsnap" command
as a signal that the client should be permitted
to send MRemoveSnaps too.

Note that we don't also require the W ability,
unlike Monitor::_allowed_command -- this is slightly
more permissive handling, but anyone crafting caps
that explicitly permit "osd pool rmsnap" needs to
know what they are doing.

Signed-off-by: John Spray john.spray@redhat.com

@jcsp
Copy link
Contributor Author

jcsp commented Nov 16, 2015

Related: http://tracker.ceph.com/issues/13777

@jcsp jcsp added bug-fix cephfs Ceph File System labels Nov 16, 2015
@@ -2271,7 +2271,8 @@ bool OSDMonitor::preprocess_remove_snaps(MonOpRequestRef op)
MonSession *session = m->get_session();
if (!session)
goto ignore;
if (!session->is_capable("osd", MON_CAP_R | MON_CAP_W)) {
if (!session->caps.is_capable(g_ceph_context, session->entity_name,
"osd", "osd pool rmsnap", {}, true, false, false)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

MonCapGrant("mon", MON_CAP_R) is enough to make is_capable(..., true, false, false) return true. I think we should set parameter op_may_write or parameter op_may_exec to true.

Copy link
Contributor

Choose a reason for hiding this comment

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

see the 'return MON_CAP_ALL' line in MonCapGrant::get_allowed()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, you're right. I've updated it

@gregsfortytwo gregsfortytwo assigned jecluis and unassigned ukernel Nov 23, 2015
@gregsfortytwo
Copy link
Member

Are we missing anything about this change, @jecluis?

@ukernel
Copy link
Contributor

ukernel commented Nov 24, 2015

looks good

@tchaikov
Copy link
Contributor

lgtm, might want to have "Fixes: #13777" in commit message as #6553 does.

probably eventually, we will have "osd allow rw" when more of this changes show up.

Use ability to execute "osd pool rmsnap" command
as a signal that the client should be permitted
to send MRemoveSnaps too.

Note that we don't also require the W ability,
unlike Monitor::_allowed_command -- this is slightly
more permissive handling, but anyone crafting caps
that explicitly permit "osd pool rmsnap" needs to
know what they are doing.

Fixes: ceph#13777
Signed-off-by: John Spray <john.spray@redhat.com>
@jcsp
Copy link
Contributor Author

jcsp commented Dec 1, 2015

See the new TestStrays.test_snapshot_remove passing here http://pulpito.ceph.com/jspray-2015-11-30_02:06:10-fs-wip-testing-jcsp---basic-multi/1163869 (merge ceph/ceph-qa-suite#728 at the same time as this)

@jcsp jcsp assigned gregsfortytwo and unassigned jecluis Dec 1, 2015
@jcsp
Copy link
Contributor Author

jcsp commented Dec 1, 2015

@gregsfortytwo were you waiting for @jecluis on this?

@gregsfortytwo
Copy link
Member

@jcsp as discussed in standup (yesterday? today?), I was hoping to hear from him since he did the auth stuff here, but @liewegas gave it a thumbs-up so I'm good.

gregsfortytwo added a commit that referenced this pull request Dec 7, 2015
mon: don't require OSD W for MRemoveSnaps

Reviewed-by: Kefu Chai <kchai@redhat.com>
Reviewed-by: Yan, Zheng <zyan@redhat.com>
Reviewed-by: Greg Farnum <gfarnum@redhat.com>
Reviewed-by: Sage Weil <sage@redhat.com>
@gregsfortytwo gregsfortytwo merged commit b1d5c48 into ceph:jewel Dec 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix cephfs Ceph File System
Projects
None yet
6 participants