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

osd: Reverse order of op_has_sufficient_caps and do_pg_op #15354

Merged
merged 1 commit into from Jun 22, 2017

Conversation

Projects
None yet
3 participants
@badone
Contributor

badone commented May 29, 2017

Fixes: http://tracker.ceph.com/issues/19790

Signed-off-by: Brad Hubbard bhubbard@redhat.com

@liewegas liewegas added needs-qa and removed needs-review labels May 30, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented May 30, 2017

retest this please

@gregsfortytwo

This comment has been minimized.

Member

gregsfortytwo commented May 30, 2017

@badone, how did you validate this works? When I skimmed through it I wasn't sure it would behave well since PG ops aren't targeted at a particular object.
Or maybe that all just behaves correctly and means listing is only allowed by users with full read permissions on the pool?

@badone

This comment has been minimized.

Contributor

badone commented May 30, 2017

@gregsfortytwo in hammer op_has_sufficient_caps was called at the top of ReplicatedPG::do_request. In 818d790 it was moved to ReplicatedPG::do_op but placed after do_pg_op for reasons I can't fathom. I validated the fix against the original reported tracker issue and also ran testing (unit and CI). I will mark this PR DNM and look into adding additional testing to check for this regression and correct auth caps validation.

@badone badone changed the title from osd: Reverse order of op_has_sufficient_caps and do_pg_op to [DNM] osd: Reverse order of op_has_sufficient_caps and do_pg_op May 30, 2017

@gregsfortytwo

This comment has been minimized.

Member

gregsfortytwo commented May 31, 2017

That sounds to me like enough to get on with for now, then! :)

@badone badone removed the needs-qa label Jun 2, 2017

@gregsfortytwo

This comment has been minimized.

Member

gregsfortytwo commented Jun 17, 2017

@badone I'm satisfied with this given the testing you say you've already done. Did this need to wait on anything else?

@badone

This comment has been minimized.

Contributor

badone commented Jun 17, 2017

@gregsfortytwo I was going to add a test specifically for this case but got sidetracked. Could you give me another workday or two?

osd: Reverse order of op_has_sufficient_caps and do_pg_op
Fixes: http://tracker.ceph.com/issues/19790

Signed-off-by: Brad Hubbard <bhubbard@redhat.com>

@badone badone changed the title from [DNM] osd: Reverse order of op_has_sufficient_caps and do_pg_op to osd: Reverse order of op_has_sufficient_caps and do_pg_op Jun 19, 2017

@badone

This comment has been minimized.

Contributor

badone commented Jun 19, 2017

@gregsfortytwo mind taking a look now?

@badone badone added the needs-qa label Jun 19, 2017

@badone

This comment has been minimized.

Contributor

badone commented Jun 20, 2017

@liewegas added a test so I hope the approval stands?

@liewegas

This comment has been minimized.

Member

liewegas commented Jun 20, 2017

yep lgtm!

@gregsfortytwo

This comment has been minimized.

Member

gregsfortytwo commented Jun 20, 2017

Reviewed-by: Greg Farnum gfarnum@redhat.com

@liewegas liewegas merged commit 288f623 into ceph:master Jun 22, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
arm64 make check arm64 make check succeeded
Details
make check make check succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment