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

BUILD-277: Fix fsGroup handling #19

Merged
merged 2 commits into from Nov 4, 2021

Conversation

jsafrane
Copy link
Contributor

@jsafrane jsafrane commented Nov 4, 2021

By default, CSIDriver.fsGroupPolicy is ReadWriteOnceWithFSType, which is a legacy heuristics used to detect shared filesystems, where we don't want fsGroup to be applied.. This heuristics does not work for ephemeral CSI volumes, therefore turn it off and always apply pod.spec.securityContext.fsGroup to all files provided by the CSI driver.

As result, files are readable (and writable) by the pod processes.

From:

-rw-r--r--. 1 root root 4 Nov  4 12:31 foo

To:

-rw-rw-r--. 1 root 1000590000 4 Nov  4 12:43 foo

(Where pod.spec.securityContext.fsGroup = 1000590000)

Also add attachRequired: false to make it explicit that this CSI driver does not implement ControllerPublish call. It is not used in ephemeral volumes, still, it was quite surprising when I saw attachRequired: true there, as defaulted by the API server.

@gabemontero
Copy link
Contributor

gabemontero commented Nov 4, 2021

/retitle BUILD-277: Fix fsGroup handling

@gabemontero
Copy link
Contributor

looks like a we need an update bindata @jsafrane

the unit test is a known port conflict / concurrent test thing with metrics

/test test

@openshift-ci openshift-ci bot changed the title Fix fsGroup handling BUILD-277: Fix fsGroup handling Nov 4, 2021
By default, CSIDriver.fsGroupPolicy is ReadWriteOnceWithFSType, which is a
legacy heuristics used to detect shared filesystems. This heuristics does
not work for ephemeral CSI volumes, therefore turn it off and always apply
pod.spec.securityContext.fsGroup to all files provided by the CSI driver.

As result, files are readable (and writable) by the pod processes.
This CSI driver does not implement ControllerPublish CSI call.
The call is not used in Ephemeral volumes, still, explicit is better than
implicit.
@jsafrane
Copy link
Contributor Author

jsafrane commented Nov 4, 2021

Fixed.

In the meantime we migrated the other operators to go embed and we don't need to regenerate anything. See openshift/aws-ebs-csi-driver-operator#131. You're welcome to follow suit ;-)

@gabemontero
Copy link
Contributor

Fixed.

In the meantime we migrated the other operators to go embed and we don't need to regenerate anything. See openshift/aws-ebs-csi-driver-operator#131. You're welcome to follow suit ;-)

thanks for the pointer .... I'll open an issue to track .... @coreydaley FYI

@gabemontero
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 4, 2021
@gabemontero
Copy link
Contributor

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 4, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gabemontero, jsafrane

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2021
@openshift-merge-robot openshift-merge-robot merged commit 665e497 into openshift:master Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants