feat: Use custom SCC for topolvm-node by jmolmo · Pull Request #35 · openshift/lvm-operator

jmolmo · 2021-12-20T19:30:37Z

Replaced the SCC with the custom topolvm-node SCC

Signed-off-by: Juan Miguel Olmo Martínez jolmomar@redhat.com

sp98 · 2021-12-21T02:52:22Z

config/rbac/topolvm_node_scc.yaml

+  - 'configMap'
+  - 'emptyDir'
+  - 'secret'
+  - 'hostPath'


(trying to understand how SCC works) How is this SCC associated with the topolvm-node Service account? There is a user filed that contains the list of serviceAccounts. For example- rook

thanks for finding this, I saw users field somewhere and forgot where that was during our meetings

in current scenario we are using ClusterRole & ClusterRoleBinding to use this SCC

in merged commit it was using privileged directly and this changes that to use topolvm-node named SCC

@sp98: I have include the different settings reading what they do .
As @leelavg commented we are using a cluster role binding to to associate the sa with the scc. Include the user in the scc seems to link the scc to a single namespace (because the way you need to specify the user), which i think is very limited for our needs.

The operator only needs the permissions for the sa in the operator namespace as that is where it will create the containers. Can we try this with a Role?

@nbalacha : We cannot use Role instead of Cluster Role, because we need permissions for cluster-scoped resources (like nodes).

leelavg

/lgtm

leelavg · 2021-12-21T03:03:49Z

config/rbac/topolvm_node_scc.yaml

+allowPrivilegeEscalation: true
+allowPrivilegedContainer: true


not immediate but our final goal should be to use specific capabilities that the container needs and curtail everything else.

openshift-ci · 2021-12-21T03:04:06Z

@leelavg: changing LGTM is restricted to collaborators

Details

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

leseb

Generally, it's good to comment on why we require permission when we hope to remove it (if ever possible).

leseb · 2021-12-21T08:36:44Z

config/rbac/topolvm_node_scc.yaml

+allowHostDirVolumePlugin: true
+allowHostIPC: false
+allowHostNetwork: false
+allowHostPID: true


https://github.com/red-hat-storage/lvm-operator/blob/848792e3cda401d0567e0140d006b068e3c54adf/controllers/topolvm_node.go#L182

That does not tell why we need HostPID.

@leseb generally nodeplugin doesn't need it however lvmd in the daemonset needs this.

It uses nsenter to get into host pid nasemspace and run lvm ops

I don't think you need HostPID for nsenter to run any commands on the host. As far as I can tell, Rook uses nsenter already to exec command on the host and we don't use HostPID.

However, if you really feel like we do then let's comment on why. Thanks

Extract from Openshift documentation:

allowHostPID | boolean | AllowHostPID determines if the policy allows host pid in the containers. -- | -- | --

If you are asking why do we need hostpid: true in the pod, this is because lvmd needs to see the pid:1 on the host. (here you can follow the complete discussion that drove to this point)

But, for me is new this world of SCCs.. how do you suggest to add this requirement (see pid 1 in the host) in the SCC?

I don't mean to rehearse or argue on the past decision that led to enabling HostPID. I just want to make sure everybody knows and understands why it is needed. That's all :) So in the config/rbac/topolvm_node_scc.yaml I'm just suggesting adding more comments to explain why each option we enable is needed.

config/rbac/topolvm_node_scc.yaml

nbalacha · 2021-12-21T09:08:01Z

config/rbac/topolvm_node_scc.yaml

+  - 'configMap'
+  - 'emptyDir'
+  - 'secret'
+  - 'hostPath'


The operator only needs the permissions for the sa in the operator namespace as that is where it will create the containers. Can we try this with a Role?

config/rbac/topolvm_node_scc.yaml

Replaced the <privileged> SCC with the custom topolvm-node SCC Signed-off-by: Juan Miguel Olmo Martínez <jolmomar@redhat.com>

jmolmo · 2021-12-21T12:07:17Z

I have extracted the topolvm-node scc from the cluster role, and created a specific cluster role and binding for the scc. In this way deployment can be more flexible. I guess that we also will need in a near future to add PSPs, :-)

openshift-ci · 2021-12-21T15:23:28Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jmolmo, leelavg, nbalacha, sp98

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jmolmo force-pushed the topolvm_node_scc branch from cb309ad to 59ceb45 Compare December 20, 2021 19:55

sp98 reviewed Dec 21, 2021

View reviewed changes

leelavg approved these changes Dec 21, 2021

View reviewed changes

leseb reviewed Dec 21, 2021

View reviewed changes

nbalacha suggested changes Dec 21, 2021

View reviewed changes

openshift-ci bot assigned nbalacha Dec 21, 2021

jmolmo force-pushed the topolvm_node_scc branch from 59ceb45 to f879995 Compare December 21, 2021 11:58

feat: use custom scc for topolvm-node

f7350ca

Replaced the <privileged> SCC with the custom topolvm-node SCC Signed-off-by: Juan Miguel Olmo Martínez <jolmomar@redhat.com>

jmolmo force-pushed the topolvm_node_scc branch from f879995 to f7350ca Compare December 21, 2021 12:00

jmolmo requested review from leelavg, leseb, nbalacha and sp98 December 21, 2021 12:07

nbalacha approved these changes Dec 21, 2021

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 21, 2021

sp98 approved these changes Dec 21, 2021

View reviewed changes

openshift-ci bot assigned sp98 Dec 21, 2021

nbalacha merged commit 7b5eb34 into openshift:main Dec 21, 2021

		allowPrivilegeEscalation: true
		allowPrivilegedContainer: true

Conversation

jmolmo commented Dec 20, 2021

Uh oh!

sp98 Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leelavg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Dec 21, 2021

Uh oh!

leseb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmolmo commented Dec 21, 2021

Uh oh!

openshift-ci bot commented Dec 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sp98 Dec 21, 2021 •

edited

Loading

leseb left a comment •

edited

Loading