BUILD-381: require CSI Volumes using the Shared Resources driver to be specified as readOnly == true #84

gabemontero · 2022-01-14T17:03:22Z

Also fixed a doc update that was missed when I changed the controller watch to listen to the namespaces that are referenced by shares instead of listening to all namespaces but filter out some openshift system namespaces.

/assign @coreydaley
a lot of deleted code
I would also use ?w=1 at the end of the git URL used during code review in your browser to filter down white space differences

for PR review

@akram FYI - yeah a fair amount of e2e reduction was needed since we can no longer support shares mounted off other shares since we require read only to be true

gabemontero · 2022-01-14T17:04:14Z

by code review URL I mean https://github.com/openshift/csi-driver-shared-resource/pull/84/files?w=1 @coreydaley

gabemontero · 2022-01-14T17:05:15Z

/assign @rolfedh

for docs approve label
I previously updated https://docs.google.com/document/d/1F3VFkiLUJbsUkFJcjnA2oyROYTI5RRlTJYl4ZWQoNls/edit#heading=h.zdumv4gow9cw
to account for these changes

gabemontero · 2022-01-14T17:05:37Z

/assign @prietyc123

for qe approved label

gabemontero · 2022-01-14T17:05:46Z

/label px-approved

gabemontero · 2022-01-14T17:06:33Z

by code review URL I mean https://github.com/openshift/csi-driver-shared-resource/pull/84/files?w=1 @coreydaley

or something similar for each commit if you want to break it up that way @coreydaley

openshift-ci · 2022-01-14T17:08:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gabemontero

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [gabemontero]

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

gabemontero · 2022-01-14T18:27:36Z

aws /install fail on https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_csi-driver-shared-resource/84/pull-ci-openshift-csi-driver-shared-resource-master-e2e-aws-csi-driver-slow/1482035631273021440

fyi it passed for me locally

/retest

rolfedh · 2022-01-14T19:03:16Z

/label docs-approved

gabemontero · 2022-01-14T19:05:15Z

more aws pain

/retest

prietyc123 · 2022-01-15T06:17:30Z

/label qe-approved

akram · 2022-01-17T07:11:52Z

docs/content-update-details.md

+in the system will have their content corresponding to those `Secrets` and `ConfigMaps`.
+
+So while the driver requires all volume references in the various `Pods` are read only, so that


Can we rephrase this one to be more understable?

yep part of next push

akram · 2022-01-17T07:15:27Z

docs/csi.md

- Also, mounting of one `SharedConfigMap` OR `SharedSecret` off of a subdirectory of another `SharedConfigMap` OR `SharedSecret` is only supported with read-write `Volumes`.  
+- the `ReadOnly` field is required to be set to 'true'.  This follows conventions introduced in upstream Kubernetes CSI Drivers to facilitate proper SELinux labelling.  What occurs is that
+this driver will return a read-write linux file system to the kubelet, so that CRI-O can apply the correct SELinux labels on the file system (CRI-O would not be able to update the SELinux labels on a read only file system
+after it is created), but the kubelet still makes sure that the file system later exposed to the consuming (which sits on top of the file system returned by this repository's driver) is read only.


s/consuming /consuming pod/

akram · 2022-01-17T07:33:38Z

pkg/hostpath/hostpath_test.go

@@ -95,318 +95,282 @@ func TestCreateHostPathVolumeBadAccessType(t *testing.T) {
 }

 func TestCreateDeleteConfigMap(t *testing.T) {
-	readOnly := []bool{true, false}


in my attempt to change these tests, I was wondering if it wouldn't be interresting to keep this for loop with 2 attempts; so we are sure that a creation then a deletion then a re-creation works well.
That would change the for loop with an i++ counter instead, but that would keep the test valid and valuable; wdyt ?

Sorry no, I don't think such a permutation has value for any of these tests. The volume won't even be provisioned when the kubelet asks for it if readOnly is not set to true with these changes. That is the requirement we have had to establish, the pattern from upstream we are following, in order to get the SELinux labels correct.

I have not just the one unit test which makes sure we fail the provision if readOnly is not set to true.

akram · 2022-01-17T07:40:26Z

pkg/hostpath/hostpath.go

@@ -50,6 +50,8 @@ type hostPath struct {
 	ephemeral         bool
 	maxVolumesPerNode int64

+	testSoNoMounter bool


Shouldn't we mock/stub this interface instead of adding this parameter especially to a public method?
Also, by reading the tests comments, it seems that we will be needing to have sudo privilege to run the tests; which is a bit complex.

Yes I took a short cut of not creating a mock mounter for unit tests.

I'll switch to creating a mock mounter and remove this.

fyi k8s over at https://github.com/kubernetes/utils/tree/master/mount does not have a "fake" mounter

and yes with all this we are "avoiding" the need for sudo priv's that are required to call the "real" mounter

I am glad to see that now we have something more elegant than k8s folks 😄

akram · 2022-01-17T07:57:53Z

pkg/hostpath/hostpath.go

-			klog.V(2).Infof("pruner: issue unmounting for volume %s mount id %s: %s", hpv.GetVolID(), hpv.GetVolPathAnchorDir(), err.Error())
-		} else {
-			klog.V(2).Infof("pruner: successfully unmounted volume %s mount id %s", hpv.GetVolID(), hpv.GetVolPathAnchorDir())
+		if mounter != nil {


so this is only needed when we are in unit tests?
The mounter here is only instantiated if we are in test mode. And so is the clean-up. I would suggest that we add this logic only in unit tests methods only.

err = mounter.Unmount(hpv.GetVolPathAnchorDir())

we only need a reference to the hpv and we could create a mounter in the unit test.
wdyt ?

see my comment above ... I'll create a mock/test version of the mount interface

gabemontero · 2022-01-17T16:47:06Z

ok thanks for the review @akram I've either responded in comments or made change in a separate commit for this review from you.... PTAL ... I'll squash commits when we reach a steady state with your review

akram · 2022-01-18T09:19:11Z

That looks good now @gabemontero ! You can squash then, so I can lgtm that.

gabemontero · 2022-01-18T12:45:12Z

That looks good now @gabemontero ! You can squash then, so I can lgtm that.

thanks @akram

the akram-1 commit has been squashed into the code/doc/unit/e2e commits appropriately

gabemontero · 2022-01-18T12:47:37Z

bump @prietyc123

for qe approved label

the scenario here is to confirm that it the pod trying to mount the shared resource volume does not set the volumeAttribute readOnly to true, it gets a failed to mount error now, as we require that attribute to be set

akram · 2022-01-18T13:10:07Z

/lgtm

coreydaley · 2022-01-18T13:16:30Z

docs/content-update-details.md

+`SharedConfigMaps`.  Then, when the actual `Secrets` or `ConfigMaps` referenced by those
+`SharedSecrets` and `SharedConfigMaps` change, each volume across all the active `Pods`
+in the system will have their content corresponding to those `Secrets` and `ConfigMaps`.


Suggested change

in the system will have their content corresponding to those `Secrets` and `ConfigMaps`.

in the system will have their content corresponding to those `Secrets` and `ConfigMaps` updated.

coreydaley · 2022-01-18T13:17:15Z

docs/content-update-details.md

+with the belief that if they want resource refreshing disabled, it should be disabled for everyone.
+
+For disabling at the volume specific level, the `volumeAttributes` field in should have an entry with the 


Suggested change

For disabling at the volume specific level, the `volumeAttributes` field in should have an entry with the

For disabling at the volume specific level, the `volumeAttributes` field should have an entry with the

coreydaley · 2022-01-18T13:19:46Z

docs/csi.md

+this driver will return a read-write linux file system to the kubelet, so that CRI-O can apply the correct SELinux labels on the file system (CRI-O would not be able to update the SELinux labels on a read only file system
+after it is created), but the kubelet still makes sure that the file system later exposed to the consuming pod (which sits on top of the file system returned by this repository's driver) is read only.
+If this driver allowed both read-only and read-write, there is in fact no way to provide differing support that still allows for correct SELinux labelling for each).
+- Also, mounting of one `SharedConfigMap` OR `SharedSecret` off of a subdirectory of another `SharedConfigMap` OR `SharedSecret` is *NOT* supported the driver only supports read-only `Volumes`.  


Suggested change

- Also, mounting of one `SharedConfigMap` OR `SharedSecret` off of a subdirectory of another `SharedConfigMap` OR `SharedSecret` is *NOT* supported the driver only supports read-only `Volumes`.

- Also, mounting of one `SharedConfigMap` OR `SharedSecret` off of a subdirectory of another `SharedConfigMap` OR `SharedSecret` is *NOT* supported. The driver only supports read-only `Volumes`.

coreydaley · 2022-01-18T13:21:40Z

pkg/hostpath/hostpath.go

+		if hp.mounter != nil {
+			err = hp.mounter.Unmount(hpv.GetVolPathAnchorDir())
+			if err != nil {
+				klog.V(2).Infof("pruner: issue unmounting for volume %s mount id %s: %s", hpv.GetVolID(), hpv.GetVolPathAnchorDir(), err.Error())


I think that this should be a warning, not an info.

akram · 2022-01-18T13:27:35Z

/hold

gabemontero · 2022-01-18T15:23:39Z

updates made @coreydaley - went ahead and squashed to existing code/doc commits given degree of change

/hold cancel

coreydaley · 2022-01-18T16:22:29Z

/lgtm

gabemontero · 2022-01-18T16:26:41Z

install pain no refresh e2e

/retest

openshift-ci · 2022-01-18T17:15:57Z

@gabemontero: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

openshift-ci bot assigned coreydaley Jan 14, 2022

openshift-ci bot assigned rolfedh Jan 14, 2022

openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Jan 14, 2022

openshift-ci bot assigned prietyc123 Jan 14, 2022

openshift-ci bot requested review from coreydaley and otaviof January 14, 2022 17:08

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 14, 2022

openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Jan 14, 2022

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Jan 15, 2022

akram reviewed Jan 17, 2022

View reviewed changes

gabemontero force-pushed the require-read-only branch from 56dcbef to 6b51a3e Compare January 18, 2022 12:43

openshift-ci bot assigned akram Jan 18, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2022

coreydaley requested changes Jan 18, 2022

View reviewed changes

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2022

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 18, 2022

gabemontero added 5 commits January 18, 2022 10:22

doc updates for requiring read only volumes

3812615

unit test updates for requiring read only volumes

a4b986e

e2e test updates for requiring read only volumes

c5f3e4d

example updates for requiring read only volumes

78f0622

code updates for requiring read only volumes

2120e56

gabemontero force-pushed the require-read-only branch from 6b51a3e to 2120e56 Compare January 18, 2022 15:22

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 18, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2022

openshift-merge-robot merged commit a19732f into openshift:master Jan 18, 2022

gabemontero deleted the require-read-only branch January 18, 2022 17:54

		in the system will have their content corresponding to those `Secrets` and `ConfigMaps`.

		So while the driver requires all volume references in the various `Pods` are read only, so that

		with the belief that if they want resource refreshing disabled, it should be disabled for everyone.

		For disabling at the volume specific level, the `volumeAttributes` field in should have an entry with the

	- Also, mounting of one `SharedConfigMap` OR `SharedSecret` off of a subdirectory of another `SharedConfigMap` OR `SharedSecret` is NOT supported the driver only supports read-only `Volumes`.
	- Also, mounting of one `SharedConfigMap` OR `SharedSecret` off of a subdirectory of another `SharedConfigMap` OR `SharedSecret` is NOT supported. The driver only supports read-only `Volumes`.

BUILD-381: require CSI Volumes using the Shared Resources driver to be specified as readOnly == true #84

BUILD-381: require CSI Volumes using the Shared Resources driver to be specified as readOnly == true #84

Conversation

gabemontero commented Jan 14, 2022

gabemontero commented Jan 14, 2022

gabemontero commented Jan 14, 2022

gabemontero commented Jan 14, 2022

gabemontero commented Jan 14, 2022

gabemontero commented Jan 14, 2022

openshift-ci bot commented Jan 14, 2022

gabemontero commented Jan 14, 2022

rolfedh commented Jan 14, 2022

gabemontero commented Jan 14, 2022

prietyc123 commented Jan 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akram Jan 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabemontero commented Jan 17, 2022

akram commented Jan 18, 2022

gabemontero commented Jan 18, 2022

gabemontero commented Jan 18, 2022

akram commented Jan 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akram commented Jan 18, 2022

gabemontero commented Jan 18, 2022

coreydaley commented Jan 18, 2022

gabemontero commented Jan 18, 2022

openshift-ci bot commented Jan 18, 2022

akram Jan 18, 2022 •

edited