error codes: clarify gRPC error code usage vs. CSI error code usage

[jdef]

gRPC has an error code mechanism that's independent of the CSI error code mechanism. Provide clarifying docs about error reporting expectations. Namely that CSI errors are returned with a successful/normal gRPC status: we explicitly do NOT mix gRPC errors and CSI errors.

[jdef]

#23 (comment)

This is up for debate, @saad-ali is looking into it

[jdef]

#63

[julian-hj]

Note that as the spec is written today, if we use the CSI error and not the gRPC error, then CreateVolume cannot be idempotent as claimed.

The reason is that in the response, either a result or an error can be returned, but not both. So if CreateVolume gets called twice, there is no way for the plugin to return VOLUME_ALREADY_EXISTS and also return volume_info. As a result, the CO will know that the call succeeded, but lack the required information to do anything with that volume (including deleting it.)

I think we should either make use of gRPC errors, or allow for returning status codes and response information in the same reply.

message CreateVolumeResponse {
  message Result {
    // Contains all attributes of the newly created volume that are
    // relevant to the CO along with information required by the Plugin
    // to uniquely identifying the volume. This field is REQUIRED.
    VolumeInfo volume_info = 1;
  }

  // One of the following fields MUST be specified.
  oneof reply {
    Result result = 1;
    Error error = 2;
  }
}

[jdef]

Although we've decided to use gRPC for CSI I think we should carefully consider the impact to "protocol adapters" each time we consider more tightly coupling the CSI spec to gRPC. For example, there's a standard, default status/error translator for the gRPC gateway service (that adapts REST-like RPCs to/from gRPC). That translator doesn't seem to include gRPC status Details in the generated response entity. https://github.com/grpc-ecosystem/grpc-gateway/blob/7195ea445241f9773e71fc7b94c306d3ed6b35b6/runtime/errors.go#L92

While it's possible to build and plug-in a custom error translator for the gRPC gateway case (https://github.com/grpc-ecosystem/grpc-gateway/blob/7195ea445241f9773e71fc7b94c306d3ed6b35b6/runtime/errors.go#L60) I think this serves as a good example of how further coupling CSI to gRPC could impact downstream consumers that may adapt CSI to other protocols. I can pretty clearly see a case for a vendor needing/wanting to adapt gRPC calls to another protocol in order to reach out to an external, backend storage system.

I don't think this is a huge red flag, just something I'd like for us to keep in mind. Ultimately it's the job of the CSI consumer to handle the potential pitfalls of adapting gRPC to some other protocol. It might be nice of us to consider such use cases when advancing the state of the spec.

[jdef]

resolved by #115