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

rbd: Add cephArgs config option #1035

Merged
merged 2 commits into from Oct 11, 2017

Conversation

codenrhoden
Copy link
Member

@codenrhoden codenrhoden commented Sep 18, 2017

This patch adds an rbd.cephArgs config option. It takes free form text,
and is used to set the CEPH_ARGS env var. This env var is mostly used to
set "--id" and "--cluster" CLI args, the former being the name client
user to use (instead of admin) and the latter for using a non-default
cluster name (default is Ceph).

Using this field, a non-admin client and keyring can be used. Using that
client successfully means that the keyring has to be present in
/etc/ceph, and that the client's auth caps are set correctly. It is easy
to get in a situation where those are wrong, and calls to "rbd" or
"rados" hang instead of issuing "operation not permitted". To help
combat this, the driver no longer supports multiple pools when cephArgs
is set. Only the single pool defined as the defaultPool is supported.

I would have removed multi-pool support entirely, but that would break
backwards compatibility, and I've seen users use this functionality. But
setting CephArgs limits you to one pool only.

Replaces #934
Fixes #924
Fixes #955
xref #957 (addresses parts of it)

This patch also has an unrelated, 2nd commit with a fix I found necessary while working on the new feature.

Calling cmd.Help() prints the help output, but it does not exit the
program. There were places in the code were would detect a critical
error, emit it stderr, then emit the help output, but not exit the
program. This caused the program to continue execution leading to
potential faults from nil dereferences depending on what went wrong. We
really just want to emit the help then shutdown.

@codecov-io
Copy link

codecov-io commented Sep 18, 2017

Codecov Report

Merging #1035 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1035   +/-   ##
=======================================
  Coverage   34.16%   34.16%           
=======================================
  Files          36       36           
  Lines        2901     2901           
=======================================
  Hits          991      991           
  Misses       1807     1807           
  Partials      103      103

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17bd4ea...fd30f64. Read the comment docs.

@codenrhoden codenrhoden added this to the 2017.10-1 milestone Sep 26, 2017
@akutz akutz changed the title Add cephArgs config option to RBD driver rbd: Add cephArgs config option Sep 27, 2017
@clintkitson
Copy link
Member

@codenrhoden Are we exposing this to the docker managed plugin as well?

@codenrhoden
Copy link
Member Author

@clintkitson It's not there yet, but it absolutely should be.

@akutz
Copy link
Member

akutz commented Oct 5, 2017

Hi @codenrhoden,

Should we merge this PR or let you first expose the above env vars to the Docker plug-in?

@codenrhoden
Copy link
Member Author

@akutz no merge yet. When I asked @mvollman to review this, the comments ended up on his PR, #934. I have some feedback in there to address still.

@codenrhoden codenrhoden force-pushed the enhancement/rbd-ceph-args branch 2 times, most recently from 8f3633d to d6d301c Compare October 5, 2017 20:14
@codenrhoden
Copy link
Member Author

@mvollman okay, I've relaxed the previous restriction a bit, such that it still allows multi pool if you are only setting something like --cluster in CEPH_ARGS. Basically, since we default to the admin user, if you don't do something to change it to something else, we know it's still using the admin user. But if you use any of the flags to set the user, we don't know what is going to happen, so we take the safest route of only allowing you the pool you set.

A future enhancement could indeed be to allow a list of pools. I'd be happy if you could take a look.

@akutz akutz self-requested a review October 11, 2017 18:14
@akutz
Copy link
Member

akutz commented Oct 11, 2017

HI @codenrhoden,

Please rebase this since I merged the doc change.

Calling cmd.Help() prints the help output, but it does not exit the
program. There were places in the code were would detect a critical
error, emit it stderr, then emit the help output, but *not* exit the
program. This caused the program to continue execution leading to
potential faults from nil dereferences depending on what went wrong. We
really just want to emit the help then shutdown.
@codenrhoden codenrhoden force-pushed the enhancement/rbd-ceph-args branch 2 times, most recently from 870d148 to 1bbaf8f Compare October 11, 2017 19:18
This patch adds an rbd.cephArgs config option. It takes free form text,
and is used to set the CEPH_ARGS env var. This env var is mostly used to
set "--id" and "--cluster" CLI args, the former being the name client
user to use (instead of admin) and the latter for using a non-default
cluster name (default is Ceph).

Using this field, a non-admin client and keyring can be used. Using that
client successfully means that the keyring has to be present in
/etc/ceph, and that the client's auth caps are set correctly. It is easy
to get in a situation where those are wrong, and calls to "rbd" or
"rados" hang instead of issuing "operation not permitted". To help
combat this, the driver no longer supports multiple pools when cephArgs
contains flags that set the Ceph client id (e.g. "--id" or "--name").
In that scenario, only the single pool defined as the defaultPool is
supported.

I would have removed multi-pool support entirely, but that would break
backwards compatibility, and I've seen users use this functionality. But
setting CephArgs limits you to one pool only.
@akutz akutz merged commit 944bc49 into rexray:master Oct 11, 2017
@codenrhoden codenrhoden deleted the enhancement/rbd-ceph-args branch October 11, 2017 20:18
@mvollman
Copy link
Contributor

@codenrhoden I was waiting to review this once it was finished and then I just saw that it was merged. I see that you have added cephArgs, but I do not see that you are using them. I built and tested the code and I am not able to use a non-admin user. Your rbd, rados and ceph commands need to be passed the cephArgs.

@codenrhoden
Copy link
Member Author

@mvollman The CEPH_ARGS env var gets set here: https://github.com/codedellemc/rexray/blob/a35e8bba97a94f427dd9da965332e220aee22d4c/libstorage/drivers/storage/rbd/utils/utils.go#L378-L380

in my tests, I was able to use both a non-admin user, and use a custom cluster name.

@codenrhoden
Copy link
Member Author

@mvollman FWIW, there is an RC being created right now, and once that is up I'd be thrilled if you can install it and give it a try. That way we are sure of the version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rbd: Allow non-admin user rbd: Configurable cephx keyring
5 participants