lang: Official CSI bindings should live in this repo

[akutz]

This repo currently has the following directory structure:

<root>
|
|--lib
   |
   |--cxx
   |
   |--go

The lib/* directories exist in the official repo as the result of a Slack conversation between myself, @jdef, @jieyu, and @saad-ali. We all agreed that the official repo should have some automated means via CI for validating changes to spec.md. The most obvious solution was generating a csi.proto protobuf file from spec.md and then generating language bindings from csi.proto. However, @saad-ali reminding us that there was consensus that this repo should not be host to source code, only spec.md -- the single truth when it comes to the CSI specification.

We agreed upon a compromise -- there would be language bindings generated in the spec repo, but only for the purpose of validating changes to spec.md. The reason the bindings are committed instead of occurring only as part of CI is so there can also be a committed version of csi.proto that is known to be valid and from which bindings can be generated.

Everything works.

Except...it kind of doesn't :(

I work on a project called GoCSI -- a CSI library, client, and other helpful utilities created with Go. That project's Makefile is able to pull spec.md from this repo, generate csi.proto and then generate Go language bindings. The Go language bindings for GoCSI live in the project's root package csi.

People that use Go and wish to use GoCSI for working with CSI in Go are free to do so. People that use Go and wish to not use GoCSI are free to do that as well.

However, here is what people cannot do:

People that use Go and wish to use GoCSI and some other package or project that imports different CSI bindings.

Protobuf generated types are all registered in a global, static map via RegisterType(msg, name string). If RegisterType is invoked more than once for the same type, the Go program will panic and exit with a stack trace.

Thus it is not possible for GoCSI to generate its own CSI bindings and for a program to both use GoCSI and some other library that also generates its own CSI bindings.

You may be thinking, "Okay, so I'll only use GoCSI, or I'll not use it and generate my own bindings."

That's fair, but the way protobuf works makes that a cumbersome proposition. Protobuf generates Go sources in such a way that it is not possible to use Go interfaces to abstract away the language bindings being used. That means that if someone wants to use the type csi.VolumeInfo (generated from the CSI spec) in their code there has to be a single package that defines that type. That is true for a Go program and any of the packages imported directly and both transitively.

There are two ways around this:

1. A library can use its own, custom CSI language bindings if the library also vendors github.com/golang/protobuf/proto and google.golang.org/grpc.
2. A single package containing CSI language bindings that all Go programs and other libraries reference as the source of truth for CSI types.

The first option is not precluded by the second, but the second is necessary if Go developers are to easily consume and develop for CSI.

I propose that either the lib/go/csi package in this repo become the official source for CSI language bindings and types or we deem some other package as such.

Thoughts?

cc @saad-ali @jdef @jieyu

[akutz]

Hi All,

It also occurs to me that this proposal also would restrict which versions of gRPC will be required by programs importing the proposed, official language bindings. I really dislike this side-effect, but I'm not sure I see a good way around it.

Although to be fair, I think it's not a terrible idea if the spec recommends required min/max versions of proto/gRPC.

[cpuguy83]

I like keeping the bindings with the protos as it also makes it dead simple to get the bindings related to a specific change in the spec... of course there are other ways to deal with this as well.

Specifically the case of go type handling, looking at the gocsi packages it doesn't seem like there's a lot there that's tied to the types outside of the very generic controller functions.

[akutz]

Hi @cpuguy83,

There will be far fewer things in GoCSI tied to CSI types once the error handling PR is merged. However, all the interceptors in GoCSI rely heavily on the CSI types. So I would say plenty in GoCSI still relies on those language bindings and the types contained within.

[cpuguy83]

Oh you're right on the interceptors, I looked at the two interceptor files that don't and didn't look at the rest 👼

[saad-ali]

I am ok with lib/go/csi package in this repo become the official source for CSI language bindings

[akutz]

Are there any objections? This is of great importance to me, and the sooner people approve this the sooner I'll make some substantial changes to GoCSI.

[jdef]

golang/protobuf#178

[jdef]

I'm actually wondering if there's a way to enforce that the generated bindings in this repo are the canonical ones.

There's some syntax that the golang compiler interprets, but I don't see in the language spec:

package foo // import "github.com/canonical/package/for/foo"

.. then if someone tries to import that package foo from a different import path, the go compiler complains. what does this mean for CSI? well, we could add a file lib/go/csi/package.go to the project with the following contents (apparently only 1 file in a package needs this syntax):

package csi // import "github.com/container-storage-interface/spec/lib/go/csi"

Upon doing so, people using that csi package are required to use the given import path. I'm pretty sure that I'm missing some related consequences of doing this. A design doc or golang proposal for this "commented import" feature would be a cool thing to read. It probably exists somewhere and I just can't find it.

Anyway, you can find evidence of this kind of comment in the golang source tree, and in other projects as well, for example: https://github.com/grpc/grpc-go/blob/409fd8e23b1b48140bc8dc73950f762063baa859/codes/codes.go#L21

[cpuguy83]

@jdef https://golang.org/doc/go1.4#canonicalimports

[edisonxiang]

Hi @jdef , actually it is a very good prompt for developers who refer to csi.

[akutz]

Hi All,

Please note I opened and closed #156 re: canonical import paths. They do not appear compatible with vendoring. Which makes sense as the former was introduced in Go 1.4, and vendoring support became experimental in 1.5. Essentially if I'm using vendoring, my CSI import path will never be github.com/container-storage-interface/spec/lib/go/csi.

[cpuguy83]

@akutz Can you explain the issue you saw? I don't see why canonical import paths would break vendoring.

[akutz]

Hi @cpuguy83,

I think you're correct after all. I must have had a prolonged brain-fart and misunderstood the purpose of the canonical import.

However, I wonder if they will add any value regardless? We're not discussing the ability to access a package from multiple URIs that ultimately has the same import path. We're talking about entirely different packages as far as Go is concerned.

In fact, that's why I made my original comment regarding vendoring. We all know these are two entirely different packages as far as Go is concerned:

• github.com/csi
• github.com/gocsi/vendor/github.com/csi

So let's say I import the following code because I generate it locally in my project:

• github.com/gocsi/csi

Hmm. I think yes, there is still value in having a canonical import in csi.pb.go, but only IFF the process that is used to generate the sources also places the canonical import in its generated source.

For example, if I generate csi.pb.go locally, I have the option not to place the canonical import restriction next to package csi at the top of the file.

I suppose at best we add the canonical import and hope that if others generate the code locally they copy our process, which will also add the canonical import, and then they'll receive an error when they import that package from the incorrect path.

[saad-ali]

As we approach 1.0, this doesn't seem like a large enough issue to block 1.0. And post-1.0 this will not be viable.