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

doc: Update capabilities readme to solve to open permissions in it #4469

Merged
merged 1 commit into from
Mar 13, 2024

Conversation

dragoangel
Copy link
Contributor

@dragoangel dragoangel commented Mar 3, 2024

Describe what this PR does

Update documentation about required capabilities by Ceph CSI plugin and improve examples:

  1. limiting RBD to only one pool which only necessary to work with CSI
  2. limit CephFS to only one fs, only allowing read access to volumes with write and snapshot capabilities to only used by CSI subvolume
  3. removes unneeded complexity of splitting provisioner with controller from node capabilities, as they doesn't provide any security benefits at all, just confuse people.

Is there anything that requires special attention

N/A

Related issues

#1818
Fixes: #2687

Future concerns

N/A

Checklist:

  • Commit Message Formatting: Commit titles and messages follow
    guidelines in the developer
    guide
    .
  • Reviewed the developer guide on Submitting a Pull
    Request
  • Pending release
    notes

    updated with breaking and/or notable changes for the next major release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

Show available bot commands

These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:

  • /retest ci/centos/<job-name>: retest the <job-name> after unrelated
    failure (please report the failure too!)

@dragoangel dragoangel changed the title Update capabilities readme to solve misleading information in it Update capabilities readme to solve to open permissions in it Mar 3, 2024
@nixpanic
Copy link
Member

nixpanic commented Mar 4, 2024

Thanks for the PR!

Please update the commit subject with doc: as prefix, and add your Signed-off-by: line. There should be a single commit (please squash them together) as well.

@nixpanic nixpanic changed the title Update capabilities readme to solve to open permissions in it doc: Update capabilities readme to solve to open permissions in it Mar 4, 2024
@mergify mergify bot added ci/skip/e2e skip running e2e CI jobs ci/skip/multi-arch-build skip building on multiple architectures component/docs Issues and PRs related to documentation labels Mar 4, 2024
@dragoangel
Copy link
Contributor Author

@nixpanic now it's good?

nixpanic
nixpanic previously approved these changes Mar 4, 2024
@nixpanic nixpanic requested a review from a team March 4, 2024 12:21
@nixpanic
Copy link
Member

nixpanic commented Mar 4, 2024

@nixpanic now it's good?

Thanks! Yes, I think it looks good. 2 reviews are needed to get it merged, maybe someone else has other comments.

Copy link
Member

@nixpanic nixpanic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, tests were not passing, you will need to update the PR once again 😞

```

To get more insights on capabilities of CephFS you can refer
[this document](https://ceph.readthedocs.io/en/latest/cephfs/client-auth/)

## Command to a create user with required capabilities

`kubernetes` in the below commands represents an user which is subjected
to change as per your requirement.
`USER`, `POOL` and `FS_NAME` with `SUB_VOL` variables bellow is subject to change, please adjust them to your needs.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: bellow -> below

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a linebreak before 80 chars.

./docs/capabilities.md:40: MD013 Line length

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, okay 👍

Copy link
Contributor Author

@dragoangel dragoangel Mar 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nixpanic can you please advice what about code blocks? Can they be longer? Like:
mds "allow r fsname=cephfs path=/volumes, allow rws fsname=cephfs path=/volumes/csi"

I fixed everything else, except that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think code blocks should be fine. CI runs have been approved again, so we'll know in a few minutes :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything passed, looks good 👍

docs/capabilities.md Outdated Show resolved Hide resolved
nixpanic
nixpanic previously approved these changes Mar 8, 2024
@nixpanic nixpanic requested a review from a team March 8, 2024 14:27
@nixpanic nixpanic added the ok-to-test Label to trigger E2E tests label Mar 8, 2024
@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.27

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Mar 8, 2024
mds 'allow rw'
USER=csi-cephfs
FS_NAME=cephfs
SUB_VOL=csi
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will always be csi we dont allow changing it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this not true, it possible to change it to another subvolume, and I used it this way already to host multiply clusters on same cehpfs but on different subvolumes.

mon 'profile rbd' \
osd 'profile rbd' \
mgr 'allow rw'
USER=csi
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

USER=csi to USER=csi-rbd lets keep the names constant for both cephfs and rbd

Copy link
Contributor Author

@dragoangel dragoangel Mar 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Madhu-1 the point that ceph-csi-rbd and ceph-csi-cephfs is different deployments that can (and should) have different users that each has own permissions, I don't understand why they should be one mixed super overpowered thing. The point of this PR is to provide example of real permissions needed and not overgive permissions where it doesn't needed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont see any point in calling rbd user name as just csi user and cephfs as csi-cephfs, ideally both should be similar in the documented examples. but as its configurable users can configure it as per the requirement.

Copy link
Contributor Author

@dragoangel dragoangel Mar 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can change it to csi-rbd to align with csi-cephfs, so they will be definitely not same, but will look better, is this okay?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO yes this looks ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed, please review 😊

@nixpanic
Copy link
Member

@Mergifyio queue

Copy link
Contributor

mergify bot commented Mar 13, 2024

queue

🛑 The pull request has been removed from the queue default

The pull request can't be updated.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@nixpanic
Copy link
Member

@Mergifyio rebase

Signed-off-by: Dmytro Alieksieiev <1865999+dragoangel@users.noreply.github.com>
Copy link
Contributor

mergify bot commented Mar 13, 2024

rebase

✅ Branch has been successfully rebased

@nixpanic
Copy link
Member

@Mergifyio queue

Copy link
Contributor

mergify bot commented Mar 13, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 6c43789

@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Mar 13, 2024
@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.28

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Mar 13, 2024
@mergify mergify bot merged commit 6c43789 into ceph:devel Mar 13, 2024
32 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/skip/e2e skip running e2e CI jobs ci/skip/multi-arch-build skip building on multiple architectures component/docs Issues and PRs related to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to restrict cephfs-csi access to single filesystem
4 participants