security: add dry run mode for external cluster script

[subhamkrai]

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'
None

Signed-off-by: subhamkrai srai@redhat.com

Description of your changes:

Which issue is resolved by this Pull Request:
Resolves #

Checklist:

• Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
• Skip Tests for Docs: Add the flag for skipping the build if this is only a documentation change. See here for the flag.
• Skip Unrelated Tests: Add a flag to run tests for a specific storage provider. See test options.
• Reviewed the developer guide on Submitting a Pull Request
• Documentation has been updated, if necessary.
• Unit tests have been added, if necessary.
• Integration tests have been added, if necessary.
• Pending release notes updated with breaking and/or notable changes, if necessary.
• Upgrade from previous release is tested and upgrade user guide is updated, if necessary.
• Code generation (make codegen) has been run to update object specifications, if necessary.

[mergify]

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

[aruniiird]

Current PR is simple and to the point. But I could see some short coming with this approach,

• As the read-only mode is True 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.

[aruniiird]

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.

[subhamkrai]

@leseb ^^

[leseb]

👍🏻

[leseb]

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

[mergify]

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

[subhamkrai]

Based on previous discussions, we need to implement:

• a dry-run functionality, so let's not call this read-only

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.

• make sure the script is idempotent and can run multiple times in a row

[leseb]

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

[leseb]

We want to print the full command, without duplicating entries in the code. Also, the CI should test the dry-run too.

[mergify]

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

[parth-gr]

we can add a simple unit test for it,
Rest looks good to me.

[leseb]

@subhamkrai CI is failing https://github.com/rook/rook/runs/4520799129?check_suite_focus=true PTAL

[subhamkrai]

@subhamkrai CI is failing https://github.com/rook/rook/runs/4520799129?check_suite_focus=true PTAL

the error says Execution Failed: The provided pool, 'replicapool', does not exist and

1 device_health_metrics
2 ec-pool
3 .nfs
4 my-store.rgw.buckets.index
5 my-store.rgw.buckets.non-ec
6 my-store.rgw.control
7 my-store.rgw.meta
8 my-store.rgw.log
9 .rgw.root
10 myfs-metadata
11 my-store.rgw.buckets.data
12 myfs-data0

this also says replicapool doesn't exist but we are creating that

cephblockpool.ceph.rook.io/replicapool created
+ kubectl create -f pool-ec.yaml
cephblockpool.ceph.rook.io/ec-pool created```

I also noticed that once earlier but the error went away after I pushed latest changes 

[subhamkrai]

I was not able to check op logs due to timeout

[parth-gr]

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

[subhamkrai]

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

[mergify]

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

[subhamkrai]

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

[leseb]

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'

[subhamkrai]

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 --dry-run and currently in this CI I'm passing only --rbd-data-pool-name that's why only rbd commands only.

comments addressed