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.

Sign up for GitHub

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

Updates with new discussed changes #1

Merged
merged 2 commits into from
Aug 23, 2018
+34 −55
Merged

Updates with new discussed changes #1

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4791685
Updates with new discussed changes
vladimirvivien Aug 22, 2018
d6e26d2
Update with CSIDriver fields and other corrections
vladimirvivien Aug 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 35 additions & 54 deletions ...ributors/design-proposals/storage/container-storage-interface-inline-volumes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,6 @@ type CSIVolumeSource struct {
// Required
VolumeHandle string

// VolumeHandlePrefix is type of prefix added to VolumeHandle before using
// it as CSI volume ID. It ensures that volumes with the same VolumeHandle
// in different pods or namespaces get unique CSI volume ID.
// Required.
VolumeHandlePrefix CSIVolumeHandlePrefix

// Optional: The value to pass to ControllerPublishVolumeRequest.
// Defaults to false (read/write).
// +optional
Expand Down Expand Up @@ -82,34 +76,40 @@ type CSIVolumeSource struct {
// +optional
NodePublishSecretRef *LocalObjectReference
}

type CSIVolumeHandlePrefix string
const (
// VolumeHandle is prefixed by Pod UID.
CSIVolumeHandlePrefixPod CSIVolumeHandlePrefix = "Pod"
// VolumeHandle is prefixed by UID of the namespace where the pod is located.
CSIVolumeHandlePrefixNamespace CSIVolumeHandlePrefix = "Namespace"
// VolumeHandle is not modified.
CSIVolumeHandlePrefixNone CSIVolumeHandlePrefix = "None"
)
```

The difference between `CSIVolumeSource` (in-lined in a pod) and `CSIPersistentVolumeSource` (in PV) are:
CSI volume sources, that is `CSIVolumeSource` embedded in a pod specs, will be work differently than existing `CSIPersistentVolumeSource` specified in PVs:

* All secret references in in-line volumes can refer only to secrets in the same namespace where the corresponding pod is running. This is common in all other volume sources that refer to secrets, incl. Flex.
* VolumeHandle in in-line volumes can have a prefix. This prefix (Pod UID, Namespace UID or nothing) is added to the VolumeHandle before each CSI call. It makes sure that each pod uses a different volume ID for its ephemeral volumes. The prefix must be explicitly set by pod author, there is no default.
* Users don't need to think about VolumeHandles used in other pods in their namespace, as each pod will get an unique prefix when `CSIVolumeHandlePrefixPod` is used. CSI volume ID with this prefix cannot accidentally conflict by another volume ID in another pod.
* Each pod created by ReplicaSet, StatefulSet or DaemonSet will get the same copy of a pod template. `CSIVolumeHandlePrefixPod` makes sure that each pod gets its own unique volume ID and thus can get its own volume instance.
* Without the prefix, user could guess volume ID of a secret-like CSI volume of another user and craft a pod with in-line volume referencing it. CSI driver, obeying idempotency, must then give the same volume to this pod. If users can use only`CSIVolumeHandlePrefixNamespace` or `CSIVolumeHandlePrefixPod`in their in-line volumes, we can make sure that they can't steal secrets of each other.
* `PodSecurityPolicy` will be extended to allow / deny users using in-line volumes with no prefix.
* Finally, `CSIVolumeHandlePrefixNone` allows selected users (based on PSP) to use persistent storage volumes in-line in pods.

### Volume handles
The VolumeHandle, in in-line volumes, will be optional for certain drivers (i.e. secrets, configMaps):

* When mounting a volume and the volumeHandle is not specified (think Secrets):
* Internally, CSI will check the driver's configuration object, `CSIInfo`, for the driver type.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSIInfo -> CSIDriver, see kubernetes#2514

"CSI will check the driver's" - it's not CSI, it's Kubernetes / kubelet.

And it would be helpful to include here exactly what change in CSIDriver is needed. You refer to "driver type" as "ephemeral" and "remote/persistent" in the text below, but the enum values you suggest are "PodVolumeHandleGenerated" and "NoVolumeHandleGenerated". I'd suggest to use "PodVolumeHandleGenerated" in the text, with a note that it's intended to be used by ephemeral volumes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok i will update.

* If the driver is ephemeral and the `volumeHandle is not specified`, the handle will be completely auto-generated and passed to the external CSI driver.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generated from what? Should "volume name" apply here too?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSI kubelet plugin has existing logic to generate naming for attachment, for instance, maybe similar logic could be used here to completely auto-generate the volumeHandle. Will clarify.

* If the driver is ephemeral and `volumeHandle is provided`, the handle will be prefixed to an auto-generated value that will be the name of the volume.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is "volume name" here? Pod.Spec.Volumes[x].Name? That allows guessing, attacker in a different namespace can craft a pod with the same volume name as mine and steal my data. We must prefix with something outside of attacker's control, e.g. pod UID.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, good catch. Yes, what I meant to say is that the handle will be prefix with an auto-generated value (podUID) that will be prefixed to the volume ID as you pointed out.

* If the driver is configured as remote/persistent and volumeHandle is not specified, the operation will fail.

The mechanism used to generate the volumeHandle will be configuered using field `CSIInfo.VolumeHandleGeneratedMode` with supported values:
* `PodVolumeHandleGenerated`
* `NoVolumeHandleGenerated`

See CSIInfo proposal []() for type detail.

This approach provides several advantages:
* It makes sure that each pod uses a different volume ID for its ephemeral volumes.
* Users don't need to think about VolumeHandles used in other pods in their namespace, as each pod will get an uniquely generated handle, preventing accidental naming conflicts in pods.
* Each pod created by ReplicaSet, StatefulSet or DaemonSet will get the same copy of a pod template. This makes sure that each pod gets its own unique volume ID and thus can get its own volume instance.
* Without an auto-generated naming strategy, user could guess volume ID of a secret-like CSI volume of another user and craft a pod with in-line volume referencing it. CSI driver, obeying idempotency, must then give the same volume to this pod.


## Implementation
#### Provisioning/Deletion
N/A, it works only with PVs and not with in-line volumes.

### Attach/Detach
Current `storage.VolumeAttachment` object contains only reference to PV that's being attached. It must be extended with VolumeSource for in-line volumes in pods.
Current `storage.VolumeAttachment` object contains only reference to PV that's being attached. It must be extended with `VolumeSource` for in-line volumes in pods.

```go
// VolumeAttachmentSpec is the specification of a VolumeAttachment request.
Expand Down Expand Up @@ -139,7 +139,7 @@ type InlineVolumeSource struct {
// information to detach a volume when the pod is deleted before detaching.
// Only CSIVolumeSource can be set.
// Required.
VolumeSource v1.VolumeSource
CSIVolumeSource v1.VolumeSource

// Namespace of the pod with in-line volume. It is used to resolve
// references to Secrets in VolumeSource.
Expand All @@ -149,21 +149,21 @@ type InlineVolumeSource struct {
```

* A/D controller **copies whole `VolumeSource`** from `Pod` into `VolumeAttachment`. This allows external CSI attacher to detach volumes for deleted pods without keeping any internal database of attached VolumeSources.
* Using whole `VolumeSource` allows us to re-use `VolumeAttachment` for any other in-line volume in the future. We provide validation that this `VolumeSource` contains only `CSIVolumeSource` to clearly state that only CSI is supported now.
* Using whole `VolumeSource` makes it easier to re-use type `VolumeAttachment` for any other in-line volume in the future. We provide validation that this `VolumeSource` contains only `CSIVolumeSource` to clearly state that only CSI is supported now.
* External CSI attacher must be extended to process either `PersistentVolumeName` or `VolumeSource`.
* Since in-line volume in a pod can refer to a secret in the same namespace as the pod, **external attacher may need permissions to read any Secrets in any namespace**.
* CSI `ControllerUnpublishVolume` call (~ volume detach) requires the Secrets to be available at detach time. Current CSI attacher implementation simply expects that the Secrets are available at detach time.
* Secrets for PVs are "global", out of user's namespace, so this assumption is probably OK.
* Secrets for in-line volumes must be in the same namespace as the pod that contains the volume. Users can delete them before the volume is detached. We deliberately choose to let the external attacher to fail when such Secret cannot be found on detach time and keep the volume attached, reporting errors about missing Secrets to user.
* Secrets for in-line volumes must be in the same namespace as the pod that contains the volume. Users can delete them before the volume is detached. We deliberately choose to let the external attacher fail when such Secret cannot be found at detach time and keep the volume attached, reporting errors about missing Secrets to user.
* Since access to in-line volumes can be configured by `PodSecurityPolicy` (see below), we expect that cluster admin gives access to CSI drivers that require secrets at detach time only to educated users that know they should not delete Secrets used in volumes.
* Number of CSI drivers that require Secrets on detach is probably very limited. No in-tree Kubernetes volume plugin requires them on detach.
* We will provide clear documentation that using in-line volumes with drivers that require credentials on detach may leave orphaned attached volumes that Kubernetes is not able to detach. It's up to the cluster admin to decide if using such CSI driver is worth it.
* We will provide clear documentation that using in-line volumes drivers that require credentials on detach may leave orphaned attached volumes that Kubernetes is not able to detach. It's up to the cluster admin to decide if using such CSI driver is worth it.

### Kubelet (MountDevice/SetUp/TearDown/UnmountDevice)
In-tree CSI volume plugin calls in kubelet get universal `volume.Spec`, which contains either `v1.VolumeSource` from Pod (for in-line volumes) or `v1.PersistentVolume`. We need to modify CSI volume plugin to check for presence of `VolumeSource` or `PersistentVolume` and read NodeStage/NodePublish secrets from appropriate source. Kubelet does not need any new permissions, it already can read secrets for pods that it handles. These secrets are needed only for `MountDevice/SetUp` calls and don't need to be cached until `TearDown`/`UnmountDevice`.
In-tree CSI volume plugin calls in kubelet, get universal `volume.Spec`, which contains either `v1.VolumeSource` from Pod (for in-line volumes) or `v1.PersistentVolume`. We need to modify CSI volume plugin to check for presence of `VolumeSource` or `PersistentVolume` and read NodeStage/NodePublish secrets from appropriate source. Kubelet does not need any new permissions, it already can read secrets for pods that it handles. These secrets are needed only for `MountDevice/SetUp` calls and don't need to be cached until `TearDown`/`UnmountDevice`.

### `PodSecurityPolicy`
* `PodSecurityPolicy` must be enhanced to limit pods in using in-line CSI volumes. It will be modeled following existing Flex volume policy. There is no default, users can't use in-line CSI volumes unless some CSI drivers are explicitly allowed.
* `PodSecurityPolicy` must be enhanced to limit pods in using in-line CSI volumes. It will be modeled following existing Flex volume policy. There is no default, users can't use any in-line CSI volumes unless at least one CSI drivers is explicitly allowed.
```go
type PodSecurityPolicySpec struct {
// <snip>
Expand All @@ -174,38 +174,19 @@ In-tree CSI volume plugin calls in kubelet get universal `volume.Spec`, which co
// +optional
AllowedFlexVolumes []AllowedFlexVolume

// AllowedCSIVolumes is a whitelist of allowed CSI volumes. Empty or nil indicates that all
// CSI volumes may be used. This parameter is effective only when the usage of the CSI volumes
// AllowedCSIDrivers is a whitelist of allowed CSI drivers. Empty or nil indicates that all
// CSI drivers may be used. This parameter is effective only when the usage of the CSI plugin
// is allowed in the "Volumes" field.
// +optional
AllowedCSIVolumes []AllowedCSIVolume
AllowedCSIDrivers []AllowedCSIDriver
}

// AllowedCSIVolume represents a single CSI volume that is allowed to be used.
type AllowedCSIVolume struct {
// AllowedCSIDriver represents a single CSI driver that is allowed to be used.
type AllowedCSIDriver struct {
// Driver is the name of the CSI volume driver.
Driver string
}
```
* `PodSecurityPolicy` must be extended to allow users to use in-line volumes with no prefixes. This prevents users from stealing data from Secrets-like ephemeral volumes inlined in pods by guessing volume ID of someone else. There is no default, users can't use in-line CSI volumes unless some prefixes are explicitly allowed.
```go
type PodSecurityPolicySpec struct {
// <snip>
// AllowedCSIVolumeHandlePrefixes is a whitelist of volume prefixes
// allowed to be used in CSI volumes in-lined in pods.
AllowedCSIVolumeHandlePrefixes []core.CSIVolumeHandlePrefix
}
```

* `PodSecurityPolicy` must be extended to allow users to use attachable in-line CSI volumes. This prevents users from leaving orphaned attached volumes when they delete Secrets required to detach volumes. **Kubernetes currently does not know which CSI volumes are attachable or not. There are several options considered and it will be handled in a separate proposal.**
```go
type PodSecurityPolicySpec struct {
// <snip>
// AllowAttachableCSIVolumes allows users to use attachable CSI volumes
// in-line in pod definitions.
AllowAttachableCSIVolumes bool
}
```

### Security considerations
As written above, external attacher may requrie permissions to read Secrets in any namespace. It is up to CSI driver author to document if the driver needs such permission (i.e. access to Secrets at attach/detach time) and up to cluster admin to deploy the driver with these permissions or restrict external attacher to access secrets only in some namespaces.