added permission to get nodes for rbd

[nemcikjan]

Describe what this PR does

Based on the implementation in rbd driver the nodeplugin should have by default permission to get nodes in order to retrieve the node labels

Is there anything that requires special attention

• Related rbd driver implementation can be found here

Future concerns

List items that are not part of the PR and do not impact it's
functionality, but are work items that can be taken up subsequently.

Checklist:

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

---

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!)

[Rakshith-R]

Thanks for the pr !

@iPraveenParihar after the new changes to obtain node labels if the pods are running in k8s cluster, we'll need rbac to get nodes regardless of topology or read affinity right ?

[Rakshith-R]

@nemcikjan Can you remove the surrounding conditions here too ?

ceph-csi/charts/ceph-csi-cephfs/templates/nodeplugin-clusterrole.yaml

Line 14 in ec05ae6

{{- if and .Values.readAffinity .Values.readAffinity.enabled }}

[nemcikjan]

@nemcikjan Can you remove the surrounding conditions here too ?

ceph-csi/charts/ceph-csi-cephfs/templates/nodeplugin-clusterrole.yaml

Line 14 in ec05ae6

{{- if and .Values.readAffinity .Values.readAffinity.enabled }}

@Rakshith-R I can but I looked also into the cephfs driver implementation and got the impression that it's not necessary for the cephfs rbac but it's possible that I missed something

[Rakshith-R]

@nemcikjan Can you remove the surrounding conditions here too ?

ceph-csi/charts/ceph-csi-cephfs/templates/nodeplugin-clusterrole.yaml

Line 14 in ec05ae6

{{- if and .Values.readAffinity .Values.readAffinity.enabled }}

@Rakshith-R I can but I looked also into the cephfs driver implementation and got the impression that it's not necessary for the cephfs rbac but it's possible that I missed something

Yes, we're only checking if cephcsi is running inside k8s before trying to fetch the node labels now.
Therefore, That rbac is needed regardless of the read affinity condition

[iPraveenParihar]

Thanks for the pr !

@iPraveenParihar after the new changes to obtain node labels if the pods are running in k8s cluster, we'll need rbac to get nodes regardless of topology or read affinity right ?

yes, we can remove the read affinity condition.

[XtremeOwnageDotCom]

Just a note, I also had to update the provisioner role, in my cluster. It apparently also needs the ability to get nodes.

https://github.com/ceph/ceph-csi/blob/devel/charts/ceph-csi-rbd/templates/provisioner-clusterrole.yaml

The exact same resolution, would apply to it.

[nemcikjan]

@XtremeOwnageDotCom I reviewed the implementation and it doesn't seem that the rbd provisioner needs that permission at all based on the condition here. Maybe @Rakshith-R or @iPraveenParihar could confirm.

[iPraveenParihar]

Just a note, I also had to update the provisioner role, in my cluster. It apparently also needs the ability to get nodes.

https://github.com/ceph/ceph-csi/blob/devel/charts/ceph-csi-rbd/templates/provisioner-clusterrole.yaml

The exact same resolution, would apply to it.

In RBD, we need to add the NodeServer condition as done in CephFS

ceph-csi/internal/rbd/driver/driver.go

Lines 128 to 133 in 2309168

	if k8s.RunsOnKubernetes() {
	nodeLabels, err = k8s.GetNodeLabels(conf.NodeID)
	if err != nil {
	log.FatalLogMsg(err.Error())
	}
	}

CephFS,

ceph-csi/internal/cephfs/driver.go

Lines 112 to 118 in 2309168

	if conf.IsNodeServer && k8s.RunsOnKubernetes() {
	nodeLabels, err = k8s.GetNodeLabels(conf.NodeID)
	if err != nil {
	log.FatalLogMsg(err.Error())
	}
	}

[Rakshith-R]

Looks good to me,
Can you please squash both the commits into one ?

[Rakshith-R]

Looks good to me, Can you please squash both the commits into one ?

The last commit came after I pressed enter for this review.
just one more change on the latest commit.

Thanks

[nemcikjan]

@Rakshith-R should I then squash all the commits?

[Rakshith-R]

@Rakshith-R should I then squash all the commits?

Just the first two since they are so similar
and modify the third commit according to #4302 (comment)

[Rakshith-R]

Thanks

[Rakshith-R]

ci failure https://github.com/ceph/ceph-csi/actions/runs/7139935245/job/19446163983?pr=4302

Pull request has been modified.

[Rakshith-R]

Thanks !

[Rakshith-R]

@Mergifyio rebase
@Mergifyio queue

[mergify]

rebase

✅ Branch has been successfully rebased

[Rakshith-R]

@Mergifyio queue

[mergify]

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 3443546

[ceph-csi-bot]

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

[ceph-csi-bot]

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

[ceph-csi-bot]

/test ci/centos/upgrade-tests-cephfs

[ceph-csi-bot]

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

[ceph-csi-bot]

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

[ceph-csi-bot]

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

[ceph-csi-bot]

/test ci/centos/upgrade-tests-rbd

[ceph-csi-bot]

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

[ceph-csi-bot]

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

[ceph-csi-bot]

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

[ceph-csi-bot]

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