NodeUnpublishVolumeRequest should have Secrets

[ggriffiths]

NodeUnpublishVolumeRequest currently does not have a Secrets map: https://github.com/container-storage-interface/spec/blob/release-1.0/spec.md#nodeunpublishvolume

NodePublishVolumeRequest currently supports secrets: https://github.com/container-storage-interface/spec/blob/release-1.0/spec.md#NodePublishVolume

Happy to submit a PR if this proposal is accepted.

[jdef]

Is there an actual use case for this? Or is this proposal about achieving some sort of symmetry between the publish/unpublish RPCs?

On Tue, Mar 5, 2019 at 5:02 PM Grant Griffiths notifications@github.com wrote:

NodeUnpublishVolumeRequest currently does not have a Secrets map:

https://github.com/container-storage-interface/spec/blob/release-1.0/spec.md#nodeunpublishvolume

NodePublishVolumeRequest currently supports secrets:

https://github.com/container-storage-interface/spec/blob/release-1.0/spec.md#NodePublishVolume

Happy to submit a PR if this proposal is accepted.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/container-storage-interface/spec/issues/354, or mute the thread https://github.com/notifications/unsubscribe-auth/ACPVLKWNR0aSuG4FhiFm8Dil9-2YicDIks5vTulWgaJpZM4bfl8H .

[ggriffiths]

Our publishing/unpublishing use case is similar to Figure 8 in the spec docs volume-lifecycle section [1]. We're not supporting ControllerUnpublish and ControllerPublish, only NodePublish and NodeUnpublish.

The use case for wanting Secrets in NodeUnpublish is that we want to authenticate every request that comes into our plugin.

       +-+  +-+
       |X|  | |
       +++  +^+
        |    |
   Node |    | Node
Publish |    | Unpublish
 Volume |    | Volume
    +---v----+---+
    | PUBLISHED  |
    +------------+

Figure 8: Plugins MAY forego other lifecycle steps by contraindicating
them via the capabilities API. Interactions with the volumes of such
plugins is reduced to `NodePublishVolume` and `NodeUnpublishVolume`
calls.

[1] https://github.com/container-storage-interface/spec/blob/master/spec.md#volume-lifecycle

[jdef]

At one point there was concern about a CO going insane and forgetting secrets that were needed in order to do implement a proper detach/cleanup operation. The argument was something along the lines of: if a CO loses credentials, and a detach/cleanup RPC needs them, then we're stuck with a vol that can't be unpublished.

The context of that argument was the ControllerUnpublishVolume RPC and the OP wanted to remove the secrets field from that call. We didn't do that, and kept secrets in place for the RPC. I still think that was the right decision. That said, NodeUnpublish is a bit of a different beast, and I'm not convinced that it should actually have a secrets field. If a CO does go crazy, it should be able to invoke the plugin to handle per-workload cleanup. We don't want per-workload containers/VMs/sandboxes/bananas piling up because some NodeUnpublish RPC can't be invoked due to secrets that went missing.

Authentication for every request issued by the CO to a plugin is a more general use case that we punted on for CSI v1.0 (see the Non-Goals: https://github.com/container-storage-interface/spec/blob/master/spec.md#non-goals-in-mvp). This is a broader use case that probably does merit some discussion now that we've reached 1.0, but I don't think the solution is going to be slapping a secrets field into every RPC that doesn't already have one.

[lpabon]

This is a broader use case that probably does merit some discussion now that we've reached 1.0, but I don't think the solution is going to be slapping a secrets field into every RPC that doesn't already have one.

I absolutely agree. Instead we should be using gRPC context.metadata["authorization"] = "secret..." or context.metadata[k]=v for each pair in what would be in the secrets section. This way the client only sets this once in their interceptor, and the server only has to check this once in the gRPC interceptor.

In other words, the payload does not need to have authentication information, just as it is done in HTTP/1.x.

[jdef]

@lpabon are you referring to some k8s implementation, or are you suggesting a CSI spec change?

[lpabon]

CSI spec. Instead of passing the map[string]string as Secrets in the payload, we can pass it in the metadata (as long as the metadata supports it). In case any weird characters are in the secret, we can base64 encode them (as done in k8s secret) before storing in the grpc metadata.

Here is an example of a gRPC request using metadata to pass authentication information (no base64 encoding here): https://github.com/libopenstorage/libopenstorage.github.io/blob/master/examples/golang/main.go#L33-L80

[ggriffiths]

After discussing this issue in the community meeting, we've decided to leave Secrets out from NodeUnpublishVolumeRequest in order to allow COs to still cleanup volumes in the event that a secret is lost. We also discussed that this issue is mitigated by using a unix domain socket and that we should trust COs to handle authentication.