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
security: add dry run mode for external cluster script #9189
Conversation
This pull request has merge conflicts that must be resolved before it can be merged. @subhamkrai please rebase it. https://rook.io/docs/rook/latest/development-flow.html#updating-your-fork |
c74f0f6
to
b141fdf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current PR is simple and to the point. But I could see some short coming with this approach,
- As the
read-only
mode isTrue
by default, we will always end-up in an exception. Why because the functions we modified are called always by default. - Even if we could overcome the above^ issue (by adding conditions around the function calls), we have to make sure the lesser output (that is the subset of original output we get in a read-only mode) is good enough to connect to an external cluster.
I will suggest a bit more sophisticated/complex approach, where in place of get-or-create
ceph api commands, we use a variable. This variable will be (by default) set to get
and if read-only
mode is False
it will be set to get-or-create
. Use this variable in places where we use cmd_json
prefix get-or-create
part.
if self._arg_parser.read_only: | ||
raise ExecutionFailureException( | ||
"Read-only mode is enabled. Pass '--read-only' or '-r' as False") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a common piece of code which is used. Can we convert it to a function, something like,
def raise_exception_if_read_only(self):
if self._arg_parser.read_only:
raise ExecutionFailureException(
"Read-only mode is enabled. Pass '--read-only' or '-r' as False")
and call it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leseb ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on previous discussions, we need to implement:
- a dry-run functionality, so let's not call this read-only
- make sure the script is idempotent and can run multiple times in a row
This pull request has merge conflicts that must be resolved before it can be merged. @subhamkrai please rebase it. https://rook.io/docs/rook/latest/development-flow.html#updating-your-fork |
f440671
to
ce97c27
Compare
for dry-run, currently, I'm just printing a message so that we don't get different outputs which were the case in read-only mode.
|
ce97c27
to
084f27b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A dry-run prints the command that would be executed if the dry-run was not set. You can print with a message too, just like you do now. If it's a ceph command, just print the command that would be executed too. Thanks
bdf0003
to
12ae8dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to print the full command, without duplicating entries in the code. Also, the CI should test the dry-run too.
12ae8dc
to
0419bf9
Compare
1c12905
to
570055c
Compare
570055c
to
e44dcd4
Compare
This pull request has merge conflicts that must be resolved before it can be merged. @subhamkrai please rebase it. https://rook.io/docs/rook/latest/development-flow.html#updating-your-fork |
eae639a
to
bc413ab
Compare
bc413ab
to
e2170de
Compare
e2170de
to
8459712
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can add a simple unit test for it,
Rest looks good to me.
@subhamkrai CI is failing https://github.com/rook/rook/runs/4520799129?check_suite_focus=true PTAL |
the error says
this also says
|
I was not able to check op logs due to timeout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not able to check op logs due to timeout
you can run external script unit test locally python3 -m unittest --verbose <script_name_without_dot_py>
.
PS: ohh the actual run is failing
locally it is running |
This pull request has merge conflicts that must be resolved before it can be merged. @subhamkrai please rebase it. https://rook.io/docs/rook/latest/development-flow.html#updating-your-fork |
Adding dry run mode for external cluster script. This will add cli argument `--dry-run`. By default `dry-run` option will be `False` which means it will only print something like below. ``` Execute: 'ceph fs ls' Execute: 'ceph fsid' Execute: 'ceph quorum_status' Execute: 'ceph auth get-or-create client.healthchecker mon allow r, allow command quorum_status, allow command version mgr allow command config osd allow rwx pool=default.rgw.meta, allow r pool= .rgw.root, allow rw pool=default.rgw.control, allow rx pool=default.rgw.log, allow x pool=default.rgw.buckets.index' Execute: 'ceph mgr services' Execute: 'ceph auth get-or-create client.csi-rbd-node mon profile rbd osd profile rbd' Execute: 'ceph auth get-or-create client.csi-rbd-provisioner mon profile rbd mgr allow rw osd profile rbd' Execute: 'ceph status' ``` Signed-off-by: subhamkrai <srai@redhat.com>
8459712
to
c891ddb
Compare
Since we remove the external ec test from the canary test. We should be good now and later we can discuss how we can add the removed test. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The output is missing the cephfs keyring, only rbd is present:
Execute: 'ceph auth get-or-create client.csi-rbd-node mon profile rbd osd profile rbd'
Execute: 'ceph auth get-or-create client.csi-rbd-provisioner mon profile rbd mgr allow rw osd profile rbd'
I think it depends on what argument we are passing with |
security: add dry run mode for external cluster script (backport #9189)
Adding dry run mode for external cluster script.
This will add cli argument
--dry-run
. By defaultdry-run
option will beFalse
which means it will only print something like below.
Signed-off-by: subhamkrai srai@redhat.com
Description of your changes:
Which issue is resolved by this Pull Request:
Resolves #
Checklist:
make codegen
) has been run to update object specifications, if necessary.