cert-manager / csi-lib

A library for building CSI drivers that request certificates from cert-manager
Apache License 2.0
14 stars 13 forks source link

Allow storage implementations to optionally specifiy an fsGroup GID when writing files. #12

Closed JoshVanL closed 3 years ago

JoshVanL commented 3 years ago

This PR changes the DataWriter interface to now accept metadata, rather than just the volume ID. The filesystem implementation now has the optional fields:

    // FixedFSGroup is an optional field which will set the gid ownership of all
    // volume's data directories to this value.
    // If this value is set, FSGroupVolumeAttributeKey has no effect.
    FixedFSGroup *int64

    // FSGroupVolumeAttributeKey is an optional well-known key in the volume
    // attributes. If this attribute is present in the context when writing
    // files, gid ownership of the volume's data directory will be changed to
    // the value. Attribute value must be a valid int64 value.
    // If FixedFSGroup is defined, this field has no effect.
    FSGroupVolumeAttributeKey string

Filesystem will use these fields to optionally change the GID of the volume's data directory owner.

This is a required addition since the Kubelet will not chown CSI volumes that have the readOnly flag set, regardless of the CSI driver settings. The cert-manager CSI driver requires the readOnly flag is set to true for all mounted volumes.

We can also not discover the user and group of the container without making an API call to get the Pod spec. We do not want to require CSI drivers to read Pods from the API. There is an accepted KEP, which once implemented, will do away with the CSI driver needing this value passed and instead can rely on the volume context.


The intention for this value is that CSI driver implementations will either expose an option to users or hard code a GID value when calling WriteFiles. An istio CSR driver for example could hard code this value to 1337.

The example implementation has been updated to expose a csi.cert-manager.io/fs-group volume attribute to set this value. Users will want to set this value to the same group context of the mounting container.


I'm not sure how we can reliability test this feature without e2e tests, as it requires chowning.

jetstack-bot commented 3 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoshVanL

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](https://github.com/cert-manager/csi-lib/blob/main/OWNERS)~~ [JoshVanL] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
munnerz commented 3 years ago

The KEP seems to discuss fsGroup rather than fsUser - this PR seems to handle both. What's the reasoning here/should we be exposing controls around the owning user?

munnerz commented 3 years ago

Upon closer inspection of the KEP, I'm inclined to say we should align (for now) with upstream and only exposing controlling the group (given that files are mounted in such a way that the group permissions will permit reading the files anyway)

JoshVanL commented 3 years ago

The KEP seems to discuss fsGroup rather than fsUser - this PR seems to handle both. What's the reasoning here/should we be exposing controls around the owning user?

This PR actually is only concerned with fsUser. The reason was that this is the only option for the atomic_writer.

We could change this over to fsGroup instead, however the container will only be able to execute on the data directory, and not be able to read the data dir files. Will check this now.

JoshVanL commented 3 years ago

As discussed, there is nothing stopping us adding fsGroup support to the atomic writer. This simplifies things a lot and keep us aligned.

munnerz commented 3 years ago

/lgtm