Add producer API to write specs

[elezar]

This change proposes a "producer" API for generating and writing specs.

The more general cdi package imports this package when writing specs.

[klihub]

nit: Shouldn't we export this interface, too (even if it is so trivial) ?

The package-provided Default and Disabled implementations are exported (and are supposed to be referenced directly as such, and not for instance indirectly by a name, etc.). I am guessing that we want to allow folks to plug in their own validation logic. Since (assuming those folks use the stock c-d-i package also for eventual interpretation/injection) custom validators should always be stricter than the default one (to ensure injection by the same package wouldn't fail). That suggests that most custom validators will be a composition of the default validator + some extra custom restrictions.

Now, it is already possible to do composition using the default validator, but if my aforementioned assumptions are true, it feels strange to not expose the interface itself.

[elezar]

My thinking was that one could plug in the schema validators here as well. I had some local changes that would expose those as interfaces instead of as raw function pointers.

I'm happy to expose the interface as public though to make the intent clearer and allow users to compose these as required.

[klihub]

Having the ability to plug in additional schema-based validation here feels like a good idea.

[elezar]

@klihub I simplified this PR a bit to start with. If you're ok with the general direction, we can get this in and then move some of the other functionalty (annotations, device name generation) to this package as well.

I would also like to add functionality to allow the minimum verison to be determined at save, but would rather do that as a follow up.

[bart0sh]

Would it make sense to move this to the specs-go package? Validation is generally usable on both producer and consumer side, I guess.

[elezar]

I don't want to add unnecessary things to the specs-go package. I think we made the call that code to determine the minimum version was close enough to the spec to implement there, but I think an operation on the spec such as validation is not.

Note that due to the nature of go interfaces, this type need not be exported at all -- although it may be useful if it is.

[bart0sh]

Sorry for not being clear with this. I didn't mean moving only SpecValidator interface. I was talking about moving the whole validator package there. The idea was that it doesn't logically belong to producer. Having it here looks a bit confusing to me.

There is at least one reason not to move it to the spec module though. If it brings a lot of additional external dependencies to the spec package, we should probably avoid doing this.

@klihub @elezar WDYT?

[elezar]

I would rather suggest that we move the version specifics out of specs-go implement them as part of the "producer" package. Ideally we would implement this as a submodule as well so that we can properly separate the dependencies.

In the end we will have specs-go defining the actual data structures and helpers around this -- including validation. Implemented in other packages / submodules.

[klihub]

Sorry for not being clear with this. I didn't mean moving only SpecValidator interface. I was talking about moving the whole validator package there. The idea was that it doesn't logically belong to producer. Having it here looks a bit confusing to me.

There is at least one reason not to move it to the spec module though. If it brings a lot of additional external dependencies to the spec package, we should probably avoid doing this.

@klihub @elezar WDYT?

Yes, I had the same gut feeling. As long as the producer and the consumer were under a single package, having the validator as a subpackage thereof felt like a logical choice. Now if we split them, it does not feel so natural to have it under either... so maybe a pkg/validator. This can be adjusted with subsequent PRs, too.

[elezar]

I was actually wondering whether we shouldn't make this split at a module level so as to better manage dependencies. Meaning that we'd have:

tags.cncf.io/container-device-interface/api/v1/consumer
tags.cncf.io/container-device-interface/api/v1/producer

as top-level go submodules.

For the producer specifically, we would start out with:

tags.cncf.io/container-device-interface/api/v1/producer/cdi

with the types in this package.

We could keep the existing pkg/cdi package to provide backward compatibility.

A client that produces specs would then be something like:

package main
import (
  "tags.cncf.io/container-device-interface/api/v1/producer/cdi"
)

func main() {
    raw := generateRawCDISpec()
    saver, _ := cdi.New(raw, cdi.WithPermissions(0644))
    err := saver.Save("/var/run/cdi/example.json)
}

[klihub]

A client that produces specs would then be something like:

package main
import (
  "tags.cncf.io/container-device-interface/api/v1/producer/cdi"
)

func main() {
    raw := generateRawCDISpec()
    saver, _ := cdi.New(raw, cdi.WithPermissions(0644))
    err := saver.Save("/var/run/cdi/example.json)
}

As abstractions go, it feels a bit artificial to have a 1-to-1 association between a producer and a Spec, IOW to essentially make SpecProducer a simple Spec wrapper. I would expect a producer to produce one or more Spec files, typically with the same file format and permissions (at least by default). So maybe I'd allow passing those as options to the constructor, and rather pass the raw spec to be written to a Save or some similar function of the producer, but not to its constructor itself.

[elezar]

That's a fair point. One question in this regard, let's say that the producer is configured to set the minimum version of the spec on save is it OK to modify the spec in-place?

[bart0sh]

For the producer specifically, we would start out with: tags.cncf.io/container-device-interface/api/v1/producer/cdi

It'd look a bit inconsistent that producer and consumer APIs have a bit different paths. I'd propose to use
tags.cncf.io/container-device-interface/api/v1/producer and tags.cncf.io/container-device-interface/api/v1/consumer as you've proposed above.

[klihub]

nit: again, this it not a biggie, but it feels a bit weird, having to 'clone' the producer just to change the format... and clone is only used for this.

[elezar]

An initial implementation that I had was to have a separate type to do the writing / saving and to construct this when Save is called. Let me look at it a bit again and see if I can come up with a better abstraction.

[klihub]

nit: Perhaps call it WithDefaultSpecFormat()... now it is possible to configure a writer supposedly for a format, which is then overridden by the filename extension if it does not match... so semantically this is a default format.

[elezar]

Note that the type also exposes a WriteTo(io.Writer) function which is where this spec format is used. As you note, the Save(string) function does override the format to be consistent with the filename.

Happy to change the method name though.

[bart0sh]

@elezar @klihub I thought that we agreed to introduce validator package as a separate PR, no?

[klihub]

@elezar @klihub I thought that we agreed to introduce validator package as a separate PR, no?

That was also my understanding.

[elezar]

This could strictly speaking be removed.

[bart0sh]

Or moved to the #247

[elezar]

This was removed since the same check is already done in newSpec.

[elezar]

This change can also be removed. The intent is to remove the requirement that device.spec.vendor and device.spec.class are set before constructing a device. GetQualifiedName is called during the device validation.

[elezar]

@bart0sh @klihub sorry for the delay here. I have pulled the validation specifics into #247.

[elezar]

As an example, I have created NVIDIA/nvidia-container-toolkit#872 to provide an example of the usage of the new API.

[klihub]

nit: old typo extneral retained.

[elezar]

Updated.

[bart0sh]

Is this an external API?

[elezar]

It's not required to be public here. In the NVIDIA Container Toolkit implementation, we have the following interface:

type Transformer interface {
    Transform(*cdi.Spec) error
}

which allows for composition.

I reused some of the implementation details, and didn't change the function visibility.

Note that since the type itself is private, defining the function as public should not affect the API at all.

[bart0sh]

Yes, but it creates confusion. At least I was confused. Can we make it non-public?

[elezar]

I will update it.

[bart0sh]

As we're adding new API package we should consider adding a documentation for it, similar to https://github.com/cncf-tags/container-device-interface/blob/main/pkg/cdi/doc.go

[bart0sh]

I remember asking about this, but forgot the answer. Is there any reason to not use yaml.v3 for us?

[elezar]

I don't think so. I suppose we didn't want to introduce a dependency into our downstream clients that was not needed.

[bart0sh]

Is go-spew brought by testify?

[elezar]

Yes, I believe so.

[bart0sh]

I know it's not related to this PR, but both hidden validation and new* function returning an error look unusual to me. Usually creation of object and its validation are often separated. This is more traditional way of doing this, I think:

spec = newSpec(raw, path, prio)
if err := spec.Validate(); err != nil {
    return err
}