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

mgr: fix crash on set_config from python module with insufficient caps #14706

Merged
merged 1 commit into from Apr 28, 2017

Conversation

Projects
None yet
3 participants
@tserong
Contributor

tserong commented Apr 21, 2017

If config-key put fails, we now log the failure rather than asserting that
it must always succeed.

Fixes: http://tracker.ceph.com/issues/19629
Signed-off-by: Tim Serong tserong@suse.com

@tchaikov tchaikov requested review from tchaikov and jcsp Apr 21, 2017

// config-key put will fail if mgr's auth key has insufficient
// permission to set config keys
// FIXME: should this somehow raise an exception back into Python land?
dout(0) << "`config-key put " << global_key << " " << val << "` failed: "

This comment has been minimized.

@tchaikov

tchaikov Apr 21, 2017

Contributor

if set_cmd.r is 0, shall we continue happily without printing this error message?

This comment has been minimized.

@tserong

tserong Apr 21, 2017

Contributor

...and this is why code review is good :-)

// permission to set config keys
// FIXME: should this somehow raise an exception back into Python land?
dout(0) << "`config-key put " << global_key << " " << val << "` failed: "
<< cpp_strerror(set_cmd.r) << dendl;

This comment has been minimized.

@tchaikov

tchaikov Apr 21, 2017

Contributor

maybe we can print out the set_cmd.outs also to help debug the failure.

This comment has been minimized.

@tserong

tserong Apr 21, 2017

Contributor

I just tried adding dout(0) << set_cmd.outs << dendl; but it doesn't seem to add much useful. The existing code prints:

2017-04-21 22:06:30.807527 7f1fdfd60700  0 mgr set_config `config-key put mgr.rest.enable_auth false` failed: (13) Permission denied

If I add set_cmd.outs we also get:

2017-04-21 22:06:30.807538 7f1fdfd60700  0 mgr set_config access denied

This comment has been minimized.

@tchaikov

tchaikov Apr 24, 2017

Contributor

@tserong i don't think we can assume that monitor does not provide any information more helpful than "access denied". so i think it would helpful if we can have all available error message in mgr's log file even it sounds redundant: error 13 versus `error 13 + "access denied". BTW, i suggested we print the errorno along with the error message. not just the error message from mon.

This comment has been minimized.

@tserong

tserong Apr 24, 2017

Contributor

OK, seems reasonable, updated.

@tchaikov

other than the nit, lgtm.

mgr: fix crash on set_config from python module with insufficient caps
If config-key put fails, we now log the failure rather than asserting that
it must always succeed.

Fixes: http://tracker.ceph.com/issues/19629
Signed-off-by: Tim Serong <tserong@suse.com>

@liewegas liewegas merged commit 22ef06d into ceph:master Apr 28, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment