From 0256ad419b02bfa4e6f13f22f80409e7a2d28b17 Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Tue, 15 Oct 2024 21:05:41 +0200 Subject: [PATCH 01/10] Add producer API to write specs This change adds a SpecProducer that can be used by clients that are only concerned with outputing specs. A spec producer is configured on construction to allow for the default output format, and file permissions to be specified. Signed-off-by: Evan Lezar --- api/producer/api.go | 34 ++++ api/producer/go.mod | 20 ++ api/producer/go.sum | 21 ++ api/producer/options.go | 61 ++++++ .../producer/renamein_linux.go | 2 +- .../producer/renamein_other.go | 2 +- api/producer/spec-format.go | 95 +++++++++ api/producer/writer.go | 103 ++++++++++ api/producer/writer_test.go | 187 ++++++++++++++++++ cmd/cdi/go.mod | 2 + cmd/validate/go.mod | 1 + go.mod | 8 +- pkg/cdi/cache.go | 22 ++- pkg/cdi/device.go | 2 +- pkg/cdi/spec.go | 53 ----- pkg/cdi/spec_test.go | 13 +- schema/go.mod | 2 + 17 files changed, 561 insertions(+), 67 deletions(-) create mode 100644 api/producer/api.go create mode 100644 api/producer/go.mod create mode 100644 api/producer/go.sum create mode 100644 api/producer/options.go rename pkg/cdi/spec_linux.go => api/producer/renamein_linux.go (98%) rename pkg/cdi/spec_other.go => api/producer/renamein_other.go (98%) create mode 100644 api/producer/spec-format.go create mode 100644 api/producer/writer.go create mode 100644 api/producer/writer_test.go diff --git a/api/producer/api.go b/api/producer/api.go new file mode 100644 index 00000000..7845b2e9 --- /dev/null +++ b/api/producer/api.go @@ -0,0 +1,34 @@ +/* + Copyright © 2024 The CDI Authors + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package producer + +import cdi "tags.cncf.io/container-device-interface/specs-go" + +const ( + // DefaultSpecFormat defines the default encoding used to write CDI specs. + DefaultSpecFormat = SpecFormatYAML + + // SpecFormatJSON defines a CDI spec formatted as JSON. + SpecFormatJSON = SpecFormat(".json") + // SpecFormatYAML defines a CDI spec formatted as YAML. + SpecFormatYAML = SpecFormat(".yaml") +) + +// A SpecValidator is used to validate a CDI spec. +type SpecValidator interface { + Validate(*cdi.Spec) error +} diff --git a/api/producer/go.mod b/api/producer/go.mod new file mode 100644 index 00000000..a4fa3c38 --- /dev/null +++ b/api/producer/go.mod @@ -0,0 +1,20 @@ +module tags.cncf.io/container-device-interface/api/producer + +go 1.20 + +require ( + github.com/stretchr/testify v1.7.0 + golang.org/x/sys v0.1.0 + sigs.k8s.io/yaml v1.3.0 + tags.cncf.io/container-device-interface/specs-go v0.8.0 +) + +require ( + github.com/davecgh/go-spew v1.1.1 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect + golang.org/x/mod v0.19.0 // indirect + gopkg.in/yaml.v2 v2.4.0 // indirect + gopkg.in/yaml.v3 v3.0.1 // indirect +) + +replace tags.cncf.io/container-device-interface/specs-go => ../../specs-go diff --git a/api/producer/go.sum b/api/producer/go.sum new file mode 100644 index 00000000..9df47ee0 --- /dev/null +++ b/api/producer/go.sum @@ -0,0 +1,21 @@ +github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= +github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5CcY= +github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +golang.org/x/mod v0.19.0 h1:fEdghXQSo20giMthA7cd28ZC+jts4amQ3YMXiP5oMQ8= +golang.org/x/mod v0.19.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= +golang.org/x/sys v0.1.0 h1:kunALQeHf1/185U1i0GOB/fy1IPRDDpuoOOqRReG57U= +golang.org/x/sys v0.1.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/yaml.v2 v2.4.0 h1:D8xgwECY7CYvx+Y2n4sBz93Jn9JRvxdiyyo8CTfuKaY= +gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ= +gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= +sigs.k8s.io/yaml v1.3.0 h1:a2VclLzOGrwOHDiV8EfBGhvjHvP46CtW5j6POvhYGGo= +sigs.k8s.io/yaml v1.3.0/go.mod h1:GeOyir5tyXNByN85N/dRIT9es5UQNerPYEKK56eTBm8= diff --git a/api/producer/options.go b/api/producer/options.go new file mode 100644 index 00000000..8c247866 --- /dev/null +++ b/api/producer/options.go @@ -0,0 +1,61 @@ +/* + Copyright © 2024 The CDI Authors + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package producer + +import ( + "fmt" + "io/fs" +) + +// An Option defines a functional option for constructing a producer. +type Option func(*options) error + +type options struct { + specFormat SpecFormat + overwrite bool + permissions fs.FileMode +} + +// WithSpecFormat sets the output format of a CDI specification. +func WithSpecFormat(format SpecFormat) Option { + return func(o *options) error { + switch format { + case SpecFormatJSON, SpecFormatYAML: + o.specFormat = format + default: + return fmt.Errorf("invalid CDI spec format %v", format) + } + return nil + } +} + +// WithOverwrite specifies whether a producer should overwrite a CDI spec when +// saving to file. +func WithOverwrite(overwrite bool) Option { + return func(o *options) error { + o.overwrite = overwrite + return nil + } +} + +// WithPermissions sets the file mode to be used for a saved CDI spec. +func WithPermissions(permissions fs.FileMode) Option { + return func(o *options) error { + o.permissions = permissions + return nil + } +} diff --git a/pkg/cdi/spec_linux.go b/api/producer/renamein_linux.go similarity index 98% rename from pkg/cdi/spec_linux.go rename to api/producer/renamein_linux.go index 9ad27392..7d17b2f3 100644 --- a/pkg/cdi/spec_linux.go +++ b/api/producer/renamein_linux.go @@ -14,7 +14,7 @@ limitations under the License. */ -package cdi +package producer import ( "fmt" diff --git a/pkg/cdi/spec_other.go b/api/producer/renamein_other.go similarity index 98% rename from pkg/cdi/spec_other.go rename to api/producer/renamein_other.go index 285e04e2..96ba268a 100644 --- a/pkg/cdi/spec_other.go +++ b/api/producer/renamein_other.go @@ -17,7 +17,7 @@ limitations under the License. */ -package cdi +package producer import ( "os" diff --git a/api/producer/spec-format.go b/api/producer/spec-format.go new file mode 100644 index 00000000..6b68656a --- /dev/null +++ b/api/producer/spec-format.go @@ -0,0 +1,95 @@ +/* + Copyright © 2024 The CDI Authors + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package producer + +import ( + "encoding/json" + "fmt" + "io" + "path/filepath" + + "sigs.k8s.io/yaml" + + cdi "tags.cncf.io/container-device-interface/specs-go" +) + +// A SpecFormat defines the encoding to use when reading or writing a CDI specification. +type SpecFormat string + +// A specFormatter wraps a raw CDI specification and allows it to be formatted +// using the specified options. +type specFormatter struct { + *cdi.Spec + options +} + +// WriteTo writes the spec to the specified writer. +func (p *specFormatter) WriteTo(w io.Writer) (int64, error) { + data, err := p.contents() + if err != nil { + return 0, fmt.Errorf("failed to marshal Spec file: %w", err) + } + + n, err := w.Write(data) + return int64(n), err +} + +// marshal returns the raw contents of a CDI specification. +// No validation is performed. +func (p SpecFormat) marshal(spec *cdi.Spec) ([]byte, error) { + switch p { + case SpecFormatYAML: + data, err := yaml.Marshal(spec) + if err != nil { + return nil, err + } + data = append([]byte("---\n"), data...) + return data, nil + case SpecFormatJSON: + return json.Marshal(spec) + default: + return nil, fmt.Errorf("undefined CDI spec format %v", p) + } +} + +// normalizeFilename ensures that the specified filename ends in a supported extension. +func (p SpecFormat) normalizeFilename(filename string) (string, SpecFormat) { + switch filepath.Ext(filename) { + case ".json": + return filename, SpecFormatJSON + case ".yaml": + return filename, SpecFormatYAML + default: + return filename + string(p), p + } +} + +// validate performs an explicit validation of the spec. +// This is currently a placeholder for validation that should be performed when +// saving a spec. +func (p *specFormatter) validate() error { + return nil +} + +// contents returns the raw contents of a CDI specification. +// Validation is performed before marshalling the contentent based on the spec format. +func (p *specFormatter) contents() ([]byte, error) { + if err := p.validate(); err != nil { + return nil, fmt.Errorf("spec validation failed: %w", err) + } + return p.specFormat.marshal(p.Spec) +} diff --git a/api/producer/writer.go b/api/producer/writer.go new file mode 100644 index 00000000..f25c5c35 --- /dev/null +++ b/api/producer/writer.go @@ -0,0 +1,103 @@ +/* + Copyright © 2024 The CDI Authors + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package producer + +import ( + "fmt" + "io" + "os" + "path/filepath" + + cdi "tags.cncf.io/container-device-interface/specs-go" +) + +// A SpecWriter defines a structure for outputting CDI specifications. +type SpecWriter struct { + options +} + +// NewSpecWriter creates a spec writer with the supplied options. +func NewSpecWriter(opts ...Option) (*SpecWriter, error) { + sw := &SpecWriter{ + options: options{ + overwrite: true, + // TODO: This could be updated to 0644 to be world-readable. + permissions: 0600, + specFormat: DefaultSpecFormat, + }, + } + for _, opt := range opts { + err := opt(&sw.options) + if err != nil { + return nil, err + } + } + return sw, nil +} + +// Save writes a CDI spec to a file with the specified name. +// If the filename ends in a supported extension, the format implied by the +// extension takes precedence over the format with which the SpecWriter was +// configured. +func (p *SpecWriter) Save(spec *cdi.Spec, filename string) (string, error) { + filename, outputFormat := p.specFormat.normalizeFilename(filename) + + options := p.options + options.specFormat = outputFormat + specFormatter := specFormatter{ + Spec: spec, + options: options, + } + + dir := filepath.Dir(filename) + if dir != "" { + if err := os.MkdirAll(dir, 0o755); err != nil { + return "", fmt.Errorf("failed to create Spec dir: %w", err) + } + } + + tmp, err := os.CreateTemp(dir, "spec.*.tmp") + if err != nil { + return "", fmt.Errorf("failed to create Spec file: %w", err) + } + if err := tmp.Chmod(p.permissions); err != nil { + return "", fmt.Errorf("failed to set permissions on spec file: %w", err) + } + + _, err = specFormatter.WriteTo(tmp) + tmp.Close() + if err != nil { + return "", fmt.Errorf("failed to write Spec file: %w", err) + } + + err = renameIn(dir, filepath.Base(tmp.Name()), filepath.Base(filename), p.overwrite) + if err != nil { + _ = os.Remove(tmp.Name()) + return "", fmt.Errorf("failed to write Spec file: %w", err) + } + return filename, nil +} + +// WriteSpecTo writes the specified spec to the specified writer. +func (p *SpecWriter) WriteSpecTo(spec *cdi.Spec, w io.Writer) (int64, error) { + specFormatter := specFormatter{ + Spec: spec, + options: p.options, + } + + return specFormatter.WriteTo(w) +} diff --git a/api/producer/writer_test.go b/api/producer/writer_test.go new file mode 100644 index 00000000..96a533b6 --- /dev/null +++ b/api/producer/writer_test.go @@ -0,0 +1,187 @@ +/* + Copyright © 2024 The CDI Authors + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package producer + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + cdi "tags.cncf.io/container-device-interface/specs-go" +) + +func TestSave(t *testing.T) { + testCases := []struct { + description string + spec cdi.Spec + options []Option + filename string + expectedError error + expectedFilename string + expectedPermissions os.FileMode + expectedOutput string + }{ + { + description: "output as json", + spec: cdi.Spec{ + Version: "0.3.0", + Kind: "example.com/class", + Devices: []cdi.Device{ + { + Name: "dev1", + ContainerEdits: cdi.ContainerEdits{ + DeviceNodes: []*cdi.DeviceNode{ + { + Path: "/dev/foo", + }, + }, + }, + }, + }, + }, + options: []Option{}, + filename: "foo.json", + expectedFilename: "foo.json", + expectedPermissions: 0600, + expectedOutput: `{"cdiVersion":"0.3.0","kind":"example.com/class","devices":[{"name":"dev1","containerEdits":{"deviceNodes":[{"path":"/dev/foo"}]}}],"containerEdits":{}}`, + }, + { + description: "output with permissions", + spec: cdi.Spec{ + Version: "0.3.0", + Kind: "example.com/class", + Devices: []cdi.Device{ + { + Name: "dev1", + ContainerEdits: cdi.ContainerEdits{ + DeviceNodes: []*cdi.DeviceNode{ + { + Path: "/dev/foo", + }, + }, + }, + }, + }, + }, + options: []Option{WithPermissions(0644)}, + filename: "foo.json", + expectedFilename: "foo.json", + expectedPermissions: 0644, + expectedOutput: `{"cdiVersion":"0.3.0","kind":"example.com/class","devices":[{"name":"dev1","containerEdits":{"deviceNodes":[{"path":"/dev/foo"}]}}],"containerEdits":{}}`, + }, + { + description: "filename overwrites format", + spec: cdi.Spec{ + Version: "0.3.0", + Kind: "example.com/class", + Devices: []cdi.Device{ + { + Name: "dev1", + ContainerEdits: cdi.ContainerEdits{ + DeviceNodes: []*cdi.DeviceNode{ + { + Path: "/dev/foo", + }, + }, + }, + }, + }, + }, + options: []Option{WithSpecFormat(SpecFormatJSON)}, + filename: "foo.yaml", + expectedFilename: "foo.yaml", + expectedPermissions: 0600, + expectedOutput: `--- +cdiVersion: 0.3.0 +containerEdits: {} +devices: +- containerEdits: + deviceNodes: + - path: /dev/foo + name: dev1 +kind: example.com/class +`, + }, + { + description: "filename is inferred from format", + spec: cdi.Spec{ + Version: "0.3.0", + Kind: "example.com/class", + Devices: []cdi.Device{ + { + Name: "dev1", + ContainerEdits: cdi.ContainerEdits{ + DeviceNodes: []*cdi.DeviceNode{ + { + Path: "/dev/foo", + }, + }, + }, + }, + }, + }, + options: []Option{WithSpecFormat(SpecFormatYAML)}, + filename: "foo", + expectedFilename: "foo.yaml", + expectedPermissions: 0600, + expectedOutput: `--- +cdiVersion: 0.3.0 +containerEdits: {} +devices: +- containerEdits: + deviceNodes: + - path: /dev/foo + name: dev1 +kind: example.com/class +`, + }, + } + + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + + outputDir := t.TempDir() + + p, err := NewSpecWriter(tc.options...) + require.NoError(t, err) + + f, err := p.Save(&tc.spec, filepath.Join(outputDir, tc.filename)) + require.ErrorIs(t, err, tc.expectedError) + if tc.expectedError != nil { + return + } + + require.Equal(t, filepath.Join(outputDir, tc.expectedFilename), f) + info, err := os.Stat(f) + require.NoError(t, err) + + require.Equal(t, tc.expectedPermissions, info.Mode()) + + contents, _ := os.ReadFile(f) + require.Equal(t, tc.expectedOutput, string(contents)) + }) + } +} + +type validatorWithError struct { + err error +} + +func (v *validatorWithError) Validate(*cdi.Spec) error { + return error(v.err) +} diff --git a/cmd/cdi/go.mod b/cmd/cdi/go.mod index e7a254a5..223d4aaa 100644 --- a/cmd/cdi/go.mod +++ b/cmd/cdi/go.mod @@ -22,11 +22,13 @@ require ( github.com/xeipuuv/gojsonschema v1.2.0 // indirect golang.org/x/mod v0.19.0 // indirect golang.org/x/sys v0.19.0 // indirect + tags.cncf.io/container-device-interface/api/producer v0.8.0 // indirect tags.cncf.io/container-device-interface/specs-go v0.8.0 // indirect ) replace ( tags.cncf.io/container-device-interface => ../.. + tags.cncf.io/container-device-interface/api/producer => ../../api/producer tags.cncf.io/container-device-interface/schema => ../../schema tags.cncf.io/container-device-interface/specs-go => ../../specs-go ) diff --git a/cmd/validate/go.mod b/cmd/validate/go.mod index 14ce556d..4a47d4f0 100644 --- a/cmd/validate/go.mod +++ b/cmd/validate/go.mod @@ -17,6 +17,7 @@ require ( replace ( tags.cncf.io/container-device-interface => ../.. + tags.cncf.io/container-device-interface/api/producer => ../../api/producer tags.cncf.io/container-device-interface/schema => ../../schema tags.cncf.io/container-device-interface/specs-go => ../../specs-go ) diff --git a/go.mod b/go.mod index c91e8215..e1a75e28 100644 --- a/go.mod +++ b/go.mod @@ -8,8 +8,8 @@ require ( github.com/opencontainers/runtime-tools v0.9.1-0.20221107090550-2e043c6bd626 github.com/stretchr/testify v1.7.0 golang.org/x/sys v0.19.0 - gopkg.in/yaml.v2 v2.4.0 sigs.k8s.io/yaml v1.3.0 + tags.cncf.io/container-device-interface/api/producer v0.8.0 tags.cncf.io/container-device-interface/specs-go v0.8.0 ) @@ -19,7 +19,11 @@ require ( github.com/pmezard/go-difflib v1.0.0 // indirect github.com/syndtr/gocapability v0.0.0-20200815063812-42c35b437635 // indirect golang.org/x/mod v0.19.0 // indirect + gopkg.in/yaml.v2 v2.4.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect ) -replace tags.cncf.io/container-device-interface/specs-go => ./specs-go +replace ( + tags.cncf.io/container-device-interface/api/producer => ./api/producer + tags.cncf.io/container-device-interface/specs-go => ./specs-go +) diff --git a/pkg/cdi/cache.go b/pkg/cdi/cache.go index d399c793..fe2e3576 100644 --- a/pkg/cdi/cache.go +++ b/pkg/cdi/cache.go @@ -29,6 +29,7 @@ import ( "github.com/fsnotify/fsnotify" oci "github.com/opencontainers/runtime-spec/specs-go" + "tags.cncf.io/container-device-interface/api/producer" cdi "tags.cncf.io/container-device-interface/specs-go" ) @@ -284,28 +285,35 @@ func (c *Cache) highestPrioritySpecDir() (string, int) { func (c *Cache) WriteSpec(raw *cdi.Spec, name string) error { var ( specDir string - path string prio int spec *Spec err error ) - specDir, prio = c.highestPrioritySpecDir() if specDir == "" { return errors.New("no Spec directories to write to") } - path = filepath.Join(specDir, name) - if ext := filepath.Ext(path); ext != ".json" && ext != ".yaml" { - path += defaultSpecExt - } + path := filepath.Join(specDir, name) + // We call newSpec to perform the required validation. + // This also sets the extension of the path. spec, err = newSpec(raw, path, prio) if err != nil { return err } - return spec.write(true) + p, err := producer.NewSpecWriter( + producer.WithSpecFormat(producer.SpecFormatYAML), + producer.WithOverwrite(true), + ) + if err != nil { + return err + } + if _, err := p.Save(spec.Spec, spec.path); err != nil { + return err + } + return nil } // RemoveSpec removes a Spec with the given name from the highest diff --git a/pkg/cdi/device.go b/pkg/cdi/device.go index 2e5fa57f..80629eff 100644 --- a/pkg/cdi/device.go +++ b/pkg/cdi/device.go @@ -52,7 +52,7 @@ func (d *Device) GetSpec() *Spec { // GetQualifiedName returns the qualified name for this device. func (d *Device) GetQualifiedName() string { - return parser.QualifiedName(d.spec.GetVendor(), d.spec.GetClass(), d.Name) + return d.spec.Kind + "=" + d.Name } // ApplyEdits applies the device-speific container edits to an OCI Spec. diff --git a/pkg/cdi/spec.go b/pkg/cdi/spec.go index 1ddc244f..b826aa2e 100644 --- a/pkg/cdi/spec.go +++ b/pkg/cdi/spec.go @@ -17,7 +17,6 @@ package cdi import ( - "encoding/json" "fmt" "os" "path/filepath" @@ -25,7 +24,6 @@ import ( "sync" oci "github.com/opencontainers/runtime-spec/specs-go" - orderedyaml "gopkg.in/yaml.v2" "sigs.k8s.io/yaml" "tags.cncf.io/container-device-interface/internal/validation" @@ -120,57 +118,6 @@ func newSpec(raw *cdi.Spec, path string, priority int) (*Spec, error) { return spec, nil } -// Write the CDI Spec to the file associated with it during instantiation -// by newSpec() or ReadSpec(). -func (s *Spec) write(overwrite bool) error { - var ( - data []byte - dir string - tmp *os.File - err error - ) - - err = validateSpec(s.Spec) - if err != nil { - return err - } - - if filepath.Ext(s.path) == ".yaml" { - data, err = orderedyaml.Marshal(s.Spec) - data = append([]byte("---\n"), data...) - } else { - data, err = json.Marshal(s.Spec) - } - if err != nil { - return fmt.Errorf("failed to marshal Spec file: %w", err) - } - - dir = filepath.Dir(s.path) - err = os.MkdirAll(dir, 0o755) - if err != nil { - return fmt.Errorf("failed to create Spec dir: %w", err) - } - - tmp, err = os.CreateTemp(dir, "spec.*.tmp") - if err != nil { - return fmt.Errorf("failed to create Spec file: %w", err) - } - _, err = tmp.Write(data) - tmp.Close() - if err != nil { - return fmt.Errorf("failed to write Spec file: %w", err) - } - - err = renameIn(dir, filepath.Base(tmp.Name()), filepath.Base(s.path), overwrite) - - if err != nil { - os.Remove(tmp.Name()) - err = fmt.Errorf("failed to write Spec file: %w", err) - } - - return err -} - // GetVendor returns the vendor of this Spec. func (s *Spec) GetVendor() string { return s.vendor diff --git a/pkg/cdi/spec_test.go b/pkg/cdi/spec_test.go index 89028ac2..c3cae5d6 100644 --- a/pkg/cdi/spec_test.go +++ b/pkg/cdi/spec_test.go @@ -26,6 +26,7 @@ import ( "sigs.k8s.io/yaml" "github.com/stretchr/testify/require" + "tags.cncf.io/container-device-interface/api/producer" "tags.cncf.io/container-device-interface/pkg/parser" cdi "tags.cncf.io/container-device-interface/specs-go" ) @@ -372,12 +373,20 @@ devices: require.NoError(t, err) require.NotNil(t, spec) - err = spec.write(true) + overwrite, err := producer.NewSpecWriter( + producer.WithOverwrite(true), + ) + require.NoError(t, err) + _, err = overwrite.Save(spec.Spec, spec.path) require.NoError(t, err) _, err = os.Stat(spec.GetPath()) require.NoError(t, err, "spec.Write destination file") - err = spec.write(false) + nonOverwrite, err := producer.NewSpecWriter( + producer.WithOverwrite(false), + ) + require.NoError(t, err) + _, err = nonOverwrite.Save(spec.Spec, spec.path) require.Error(t, err) chk, err = ReadSpec(spec.GetPath(), spec.GetPriority()) diff --git a/schema/go.mod b/schema/go.mod index 379f1ec4..c469c3f9 100644 --- a/schema/go.mod +++ b/schema/go.mod @@ -23,9 +23,11 @@ require ( golang.org/x/sys v0.19.0 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect + tags.cncf.io/container-device-interface/api/producer v0.8.0 // indirect ) replace ( tags.cncf.io/container-device-interface => ../ + tags.cncf.io/container-device-interface/api/producer => ../api/producer tags.cncf.io/container-device-interface/specs-go => ../specs-go ) From 53358fd28fc6efcc61e60572e6902593929eb0b7 Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Mon, 20 Jan 2025 13:47:37 +0100 Subject: [PATCH 02/10] Add logic to detect minimum spec version on save Signed-off-by: Evan Lezar --- api/producer/options.go | 15 +++++++++--- api/producer/set-minimum-version.go | 36 +++++++++++++++++++++++++++++ api/producer/spec-format.go | 13 +++++++++++ api/producer/writer.go | 5 ++-- api/producer/writer_test.go | 33 ++++++++++++++++++++++++++ 5 files changed, 97 insertions(+), 5 deletions(-) create mode 100644 api/producer/set-minimum-version.go diff --git a/api/producer/options.go b/api/producer/options.go index 8c247866..c8f0b1ef 100644 --- a/api/producer/options.go +++ b/api/producer/options.go @@ -25,9 +25,18 @@ import ( type Option func(*options) error type options struct { - specFormat SpecFormat - overwrite bool - permissions fs.FileMode + specFormat SpecFormat + overwrite bool + permissions fs.FileMode + detectMinimumVersion bool +} + +// WithDetectMinimumVersion toggles whether a minimum version should be detected for a CDI specification. +func WithDetectMinimumVersion(detectMinimumVersion bool) Option { + return func(o *options) error { + o.detectMinimumVersion = detectMinimumVersion + return nil + } } // WithSpecFormat sets the output format of a CDI specification. diff --git a/api/producer/set-minimum-version.go b/api/producer/set-minimum-version.go new file mode 100644 index 00000000..0b502761 --- /dev/null +++ b/api/producer/set-minimum-version.go @@ -0,0 +1,36 @@ +/* + Copyright © 2025 The CDI Authors + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package producer + +import ( + "fmt" + + cdi "tags.cncf.io/container-device-interface/specs-go" +) + +type setMinimumRequiredVersion struct{} + +// transform detects the minimum required version required for the specified +// spec and sets the version field accordingly. +func (d setMinimumRequiredVersion) transform(spec *cdi.Spec) error { + minVersion, err := cdi.MinimumRequiredVersion(spec) + if err != nil { + return fmt.Errorf("failed to get minimum required CDI spec version: %w", err) + } + spec.Version = minVersion + return nil +} diff --git a/api/producer/spec-format.go b/api/producer/spec-format.go index 6b68656a..489bdc9b 100644 --- a/api/producer/spec-format.go +++ b/api/producer/spec-format.go @@ -85,9 +85,22 @@ func (p *specFormatter) validate() error { return nil } +// transform applies a transform to the spec associated with the CDI spec formatter. +// This is currently limited to detecting (and updating) the spec so that the minimum +// CDI spec version is used, but could be extended to apply other transformations. +func (p *specFormatter) transform() error { + if !p.detectMinimumVersion { + return nil + } + return (&setMinimumRequiredVersion{}).transform(p.Spec) +} + // contents returns the raw contents of a CDI specification. // Validation is performed before marshalling the contentent based on the spec format. func (p *specFormatter) contents() ([]byte, error) { + if err := p.transform(); err != nil { + return nil, fmt.Errorf("spec transform failed: %w", err) + } if err := p.validate(); err != nil { return nil, fmt.Errorf("spec validation failed: %w", err) } diff --git a/api/producer/writer.go b/api/producer/writer.go index f25c5c35..8b288b2b 100644 --- a/api/producer/writer.go +++ b/api/producer/writer.go @@ -36,8 +36,9 @@ func NewSpecWriter(opts ...Option) (*SpecWriter, error) { options: options{ overwrite: true, // TODO: This could be updated to 0644 to be world-readable. - permissions: 0600, - specFormat: DefaultSpecFormat, + permissions: 0600, + specFormat: DefaultSpecFormat, + detectMinimumVersion: false, }, } for _, opt := range opts { diff --git a/api/producer/writer_test.go b/api/producer/writer_test.go index 96a533b6..59ad88c6 100644 --- a/api/producer/writer_test.go +++ b/api/producer/writer_test.go @@ -148,6 +148,39 @@ devices: - path: /dev/foo name: dev1 kind: example.com/class +`, + }, + { + description: "minimum version is detected", + spec: cdi.Spec{ + Version: cdi.CurrentVersion, + Kind: "example.com/class", + Devices: []cdi.Device{ + { + Name: "dev1", + ContainerEdits: cdi.ContainerEdits{ + DeviceNodes: []*cdi.DeviceNode{ + { + Path: "/dev/foo", + }, + }, + }, + }, + }, + }, + options: []Option{WithDetectMinimumVersion(true)}, + filename: "foo", + expectedFilename: "foo.yaml", + expectedPermissions: 0600, + expectedOutput: `--- +cdiVersion: 0.3.0 +containerEdits: {} +devices: +- containerEdits: + deviceNodes: + - path: /dev/foo + name: dev1 +kind: example.com/class `, }, } From 537b98471a24a94cc31561e36a072171361722c2 Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Tue, 4 Feb 2025 14:06:23 +0100 Subject: [PATCH 03/10] Add validation by default to producer Signed-off-by: Evan Lezar --- api/producer/annotations.go | 56 ++++ api/producer/identifiers.go | 139 ++++++++++ api/producer/k8s/objectmeta.go | 56 ++++ api/producer/k8s/validation.go | 217 +++++++++++++++ api/producer/options.go | 9 + api/producer/spec-format.go | 5 +- api/producer/validator-default.go | 245 +++++++++++++++++ api/producer/validator-default_test.go | 365 +++++++++++++++++++++++++ api/producer/writer.go | 1 + api/producer/writer_test.go | 53 ++++ 10 files changed, 1145 insertions(+), 1 deletion(-) create mode 100644 api/producer/annotations.go create mode 100644 api/producer/identifiers.go create mode 100644 api/producer/k8s/objectmeta.go create mode 100644 api/producer/k8s/validation.go create mode 100644 api/producer/validator-default.go create mode 100644 api/producer/validator-default_test.go diff --git a/api/producer/annotations.go b/api/producer/annotations.go new file mode 100644 index 00000000..d451ad9f --- /dev/null +++ b/api/producer/annotations.go @@ -0,0 +1,56 @@ +/* + Copyright © The CDI Authors + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package producer + +import ( + "fmt" + "strings" + + "tags.cncf.io/container-device-interface/api/producer/k8s" +) + +// ValidateSpecAnnotations checks whether spec annotations are valid. +func ValidateSpecAnnotations(name string, any interface{}) error { + if any == nil { + return nil + } + + switch v := any.(type) { + case map[string]interface{}: + annotations := make(map[string]string) + for k, v := range v { + if s, ok := v.(string); ok { + annotations[k] = s + } else { + return fmt.Errorf("invalid annotation %v.%v; %v is not a string", name, k, any) + } + } + return validateSpecAnnotations(name, annotations) + } + + return nil +} + +// validateSpecAnnotations checks whether spec annotations are valid. +func validateSpecAnnotations(name string, annotations map[string]string) error { + path := "annotations" + if name != "" { + path = strings.Join([]string{name, path}, ".") + } + + return k8s.ValidateAnnotations(annotations, path) +} diff --git a/api/producer/identifiers.go b/api/producer/identifiers.go new file mode 100644 index 00000000..8aeb2216 --- /dev/null +++ b/api/producer/identifiers.go @@ -0,0 +1,139 @@ +/* + Copyright © 2024 The CDI Authors + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package producer + +import ( + "fmt" + "strings" +) + +// ValidateKind checks the validity of a CDI kind. +// The syntax for a device kind“ is +// +// "/" +func ValidateKind(kind string) error { + parts := strings.SplitN(kind, "/", 2) + if len(parts) != 2 || parts[0] == "" || parts[1] == "" { + return fmt.Errorf("kind %s does not contain a / %w", kind, errInvalid) + } + if err := ValidateVendorName(parts[0]); err != nil { + return err + } + if err := ValidateClassName(parts[1]); err != nil { + return err + } + return nil +} + +// ValidateVendorName checks the validity of a vendor name. +// A vendor name may contain the following ASCII characters: +// - upper- and lowercase letters ('A'-'Z', 'a'-'z') +// - digits ('0'-'9') +// - underscore, dash, and dot ('_', '-', and '.') +func ValidateVendorName(vendor string) error { + err := validateVendorOrClassName(vendor) + if err != nil { + err = fmt.Errorf("invalid vendor. %w", err) + } + return err +} + +// ValidateClassName checks the validity of class name. +// A class name may contain the following ASCII characters: +// - upper- and lowercase letters ('A'-'Z', 'a'-'z') +// - digits ('0'-'9') +// - underscore, dash, and dot ('_', '-', and '.') +func ValidateClassName(class string) error { + err := validateVendorOrClassName(class) + if err != nil { + err = fmt.Errorf("invalid class. %w", err) + } + return err +} + +// validateVendorOrClassName checks the validity of vendor or class name. +// A name may contain the following ASCII characters: +// - upper- and lowercase letters ('A'-'Z', 'a'-'z') +// - digits ('0'-'9') +// - underscore, dash, and dot ('_', '-', and '.') +func validateVendorOrClassName(name string) error { + if name == "" { + return fmt.Errorf("empty name") + } + if !IsLetter(rune(name[0])) { + return fmt.Errorf("%q, should start with letter", name) + } + for _, c := range string(name[1 : len(name)-1]) { + switch { + case IsAlphaNumeric(c): + case c == '_' || c == '-' || c == '.': + default: + return fmt.Errorf("invalid character '%c' in name %q", + c, name) + } + } + if !IsAlphaNumeric(rune(name[len(name)-1])) { + return fmt.Errorf("%q, should end with a letter or digit", name) + } + + return nil +} + +// ValidateDeviceName checks the validity of a device name. +// A device name may contain the following ASCII characters: +// - upper- and lowercase letters ('A'-'Z', 'a'-'z') +// - digits ('0'-'9') +// - underscore, dash, dot, colon ('_', '-', '.', ':') +func ValidateDeviceName(name string) error { + if name == "" { + return fmt.Errorf("invalid (empty) device name") + } + if !IsAlphaNumeric(rune(name[0])) { + return fmt.Errorf("invalid class %q, should start with a letter or digit", name) + } + if len(name) == 1 { + return nil + } + for _, c := range string(name[1 : len(name)-1]) { + switch { + case IsAlphaNumeric(c): + case c == '_' || c == '-' || c == '.' || c == ':': + default: + return fmt.Errorf("invalid character '%c' in device name %q", + c, name) + } + } + if !IsAlphaNumeric(rune(name[len(name)-1])) { + return fmt.Errorf("invalid name %q, should end with a letter or digit", name) + } + return nil +} + +// IsLetter reports whether the rune is a letter. +func IsLetter(c rune) bool { + return ('A' <= c && c <= 'Z') || ('a' <= c && c <= 'z') +} + +// IsDigit reports whether the rune is a digit. +func IsDigit(c rune) bool { + return '0' <= c && c <= '9' +} + +// IsAlphaNumeric reports whether the rune is a letter or digit. +func IsAlphaNumeric(c rune) bool { + return IsLetter(c) || IsDigit(c) +} diff --git a/api/producer/k8s/objectmeta.go b/api/producer/k8s/objectmeta.go new file mode 100644 index 00000000..fb86c67a --- /dev/null +++ b/api/producer/k8s/objectmeta.go @@ -0,0 +1,56 @@ +/* +Copyright 2014 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Adapted from k8s.io/apimachinery/pkg/api/validation: +// https://github.com/kubernetes/apimachinery/blob/7687996c715ee7d5c8cf1e3215e607eb065a4221/pkg/api/validation/objectmeta.go + +package k8s + +import ( + "errors" + "fmt" + "strings" +) + +// TotalAnnotationSizeLimitB defines the maximum size of all annotations in characters. +const TotalAnnotationSizeLimitB int = 256 * (1 << 10) // 256 kB + +// ValidateAnnotations validates that a set of annotations are correctly defined. +func ValidateAnnotations(annotations map[string]string, path string) error { + errs := []error{} + for k := range annotations { + // The rule is QualifiedName except that case doesn't matter, so convert to lowercase before checking. + for _, msg := range IsQualifiedName(strings.ToLower(k)) { + errs = append(errs, fmt.Errorf("%v.%v is invalid: %v", path, k, msg)) + } + } + if err := ValidateAnnotationsSize(annotations); err != nil { + errs = append(errs, fmt.Errorf("%v is too long: %v", path, err)) + } + return errors.Join(errs...) +} + +// ValidateAnnotationsSize validates that a set of annotations is not too large. +func ValidateAnnotationsSize(annotations map[string]string) error { + var totalSize int64 + for k, v := range annotations { + totalSize += (int64)(len(k)) + (int64)(len(v)) + } + if totalSize > (int64)(TotalAnnotationSizeLimitB) { + return fmt.Errorf("annotations size %d is larger than limit %d", totalSize, TotalAnnotationSizeLimitB) + } + return nil +} diff --git a/api/producer/k8s/validation.go b/api/producer/k8s/validation.go new file mode 100644 index 00000000..5ad6ce27 --- /dev/null +++ b/api/producer/k8s/validation.go @@ -0,0 +1,217 @@ +/* +Copyright 2014 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Adapted from k8s.io/apimachinery/pkg/util/validation: +// https://github.com/kubernetes/apimachinery/blob/7687996c715ee7d5c8cf1e3215e607eb065a4221/pkg/util/validation/validation.go + +package k8s + +import ( + "fmt" + "regexp" + "strings" +) + +const qnameCharFmt string = "[A-Za-z0-9]" +const qnameExtCharFmt string = "[-A-Za-z0-9_.]" +const qualifiedNameFmt string = "(" + qnameCharFmt + qnameExtCharFmt + "*)?" + qnameCharFmt +const qualifiedNameErrMsg string = "must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character" +const qualifiedNameMaxLength int = 63 + +var qualifiedNameRegexp = regexp.MustCompile("^" + qualifiedNameFmt + "$") + +// IsQualifiedName tests whether the value passed is what Kubernetes calls a +// "qualified name". This is a format used in various places throughout the +// system. If the value is not valid, a list of error strings is returned. +// Otherwise an empty list (or nil) is returned. +func IsQualifiedName(value string) []string { + var errs []string + parts := strings.Split(value, "/") + var name string + switch len(parts) { + case 1: + name = parts[0] + case 2: + var prefix string + prefix, name = parts[0], parts[1] + if len(prefix) == 0 { + errs = append(errs, "prefix part "+EmptyError()) + } else if msgs := IsDNS1123Subdomain(prefix); len(msgs) != 0 { + errs = append(errs, prefixEach(msgs, "prefix part ")...) + } + default: + return append(errs, "a qualified name "+RegexError(qualifiedNameErrMsg, qualifiedNameFmt, "MyName", "my.name", "123-abc")+ + " with an optional DNS subdomain prefix and '/' (e.g. 'example.com/MyName')") + } + + if len(name) == 0 { + errs = append(errs, "name part "+EmptyError()) + } else if len(name) > qualifiedNameMaxLength { + errs = append(errs, "name part "+MaxLenError(qualifiedNameMaxLength)) + } + if !qualifiedNameRegexp.MatchString(name) { + errs = append(errs, "name part "+RegexError(qualifiedNameErrMsg, qualifiedNameFmt, "MyName", "my.name", "123-abc")) + } + return errs +} + +const labelValueFmt string = "(" + qualifiedNameFmt + ")?" +const labelValueErrMsg string = "a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character" + +// LabelValueMaxLength is a label's max length +const LabelValueMaxLength int = 63 + +var labelValueRegexp = regexp.MustCompile("^" + labelValueFmt + "$") + +// IsValidLabelValue tests whether the value passed is a valid label value. If +// the value is not valid, a list of error strings is returned. Otherwise an +// empty list (or nil) is returned. +func IsValidLabelValue(value string) []string { + var errs []string + if len(value) > LabelValueMaxLength { + errs = append(errs, MaxLenError(LabelValueMaxLength)) + } + if !labelValueRegexp.MatchString(value) { + errs = append(errs, RegexError(labelValueErrMsg, labelValueFmt, "MyValue", "my_value", "12345")) + } + return errs +} + +const dns1123LabelFmt string = "[a-z0-9]([-a-z0-9]*[a-z0-9])?" +const dns1123LabelErrMsg string = "a lowercase RFC 1123 label must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character" + +// DNS1123LabelMaxLength is a label's max length in DNS (RFC 1123) +const DNS1123LabelMaxLength int = 63 + +var dns1123LabelRegexp = regexp.MustCompile("^" + dns1123LabelFmt + "$") + +// IsDNS1123Label tests for a string that conforms to the definition of a label in +// DNS (RFC 1123). +func IsDNS1123Label(value string) []string { + var errs []string + if len(value) > DNS1123LabelMaxLength { + errs = append(errs, MaxLenError(DNS1123LabelMaxLength)) + } + if !dns1123LabelRegexp.MatchString(value) { + errs = append(errs, RegexError(dns1123LabelErrMsg, dns1123LabelFmt, "my-name", "123-abc")) + } + return errs +} + +const dns1123SubdomainFmt string = dns1123LabelFmt + "(\\." + dns1123LabelFmt + ")*" +const dns1123SubdomainErrorMsg string = "a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character" + +// DNS1123SubdomainMaxLength is a subdomain's max length in DNS (RFC 1123) +const DNS1123SubdomainMaxLength int = 253 + +var dns1123SubdomainRegexp = regexp.MustCompile("^" + dns1123SubdomainFmt + "$") + +// IsDNS1123Subdomain tests for a string that conforms to the definition of a +// subdomain in DNS (RFC 1123). +func IsDNS1123Subdomain(value string) []string { + var errs []string + if len(value) > DNS1123SubdomainMaxLength { + errs = append(errs, MaxLenError(DNS1123SubdomainMaxLength)) + } + if !dns1123SubdomainRegexp.MatchString(value) { + errs = append(errs, RegexError(dns1123SubdomainErrorMsg, dns1123SubdomainFmt, "example.com")) + } + return errs +} + +const dns1035LabelFmt string = "[a-z]([-a-z0-9]*[a-z0-9])?" +const dns1035LabelErrMsg string = "a DNS-1035 label must consist of lower case alphanumeric characters or '-', start with an alphabetic character, and end with an alphanumeric character" + +// DNS1035LabelMaxLength is a label's max length in DNS (RFC 1035) +const DNS1035LabelMaxLength int = 63 + +var dns1035LabelRegexp = regexp.MustCompile("^" + dns1035LabelFmt + "$") + +// IsDNS1035Label tests for a string that conforms to the definition of a label in +// DNS (RFC 1035). +func IsDNS1035Label(value string) []string { + var errs []string + if len(value) > DNS1035LabelMaxLength { + errs = append(errs, MaxLenError(DNS1035LabelMaxLength)) + } + if !dns1035LabelRegexp.MatchString(value) { + errs = append(errs, RegexError(dns1035LabelErrMsg, dns1035LabelFmt, "my-name", "abc-123")) + } + return errs +} + +// wildcard definition - RFC 1034 section 4.3.3. +// examples: +// - valid: *.bar.com, *.foo.bar.com +// - invalid: *.*.bar.com, *.foo.*.com, *bar.com, f*.bar.com, * +const wildcardDNS1123SubdomainFmt = "\\*\\." + dns1123SubdomainFmt +const wildcardDNS1123SubdomainErrMsg = "a wildcard DNS-1123 subdomain must start with '*.', followed by a valid DNS subdomain, which must consist of lower case alphanumeric characters, '-' or '.' and end with an alphanumeric character" + +// IsWildcardDNS1123Subdomain tests for a string that conforms to the definition of a +// wildcard subdomain in DNS (RFC 1034 section 4.3.3). +func IsWildcardDNS1123Subdomain(value string) []string { + wildcardDNS1123SubdomainRegexp := regexp.MustCompile("^" + wildcardDNS1123SubdomainFmt + "$") + + var errs []string + if len(value) > DNS1123SubdomainMaxLength { + errs = append(errs, MaxLenError(DNS1123SubdomainMaxLength)) + } + if !wildcardDNS1123SubdomainRegexp.MatchString(value) { + errs = append(errs, RegexError(wildcardDNS1123SubdomainErrMsg, wildcardDNS1123SubdomainFmt, "*.example.com")) + } + return errs +} + +// MaxLenError returns a string explanation of a "string too long" validation +// failure. +func MaxLenError(length int) string { + return fmt.Sprintf("must be no more than %d characters", length) +} + +// RegexError returns a string explanation of a regex validation failure. +func RegexError(msg string, fmt string, examples ...string) string { + if len(examples) == 0 { + return msg + " (regex used for validation is '" + fmt + "')" + } + msg += " (e.g. " + for i := range examples { + if i > 0 { + msg += " or " + } + msg += "'" + examples[i] + "', " + } + msg += "regex used for validation is '" + fmt + "')" + return msg +} + +// EmptyError returns a string explanation of a "must not be empty" validation +// failure. +func EmptyError() string { + return "must be non-empty" +} + +func prefixEach(msgs []string, prefix string) []string { + for i := range msgs { + msgs[i] = prefix + msgs[i] + } + return msgs +} + +// InclusiveRangeError returns a string explanation of a numeric "must be +// between" validation failure. +func InclusiveRangeError(lo, hi int) string { + return fmt.Sprintf(`must be between %d and %d, inclusive`, lo, hi) +} diff --git a/api/producer/options.go b/api/producer/options.go index c8f0b1ef..84ade763 100644 --- a/api/producer/options.go +++ b/api/producer/options.go @@ -29,6 +29,7 @@ type options struct { overwrite bool permissions fs.FileMode detectMinimumVersion bool + validator SpecValidator } // WithDetectMinimumVersion toggles whether a minimum version should be detected for a CDI specification. @@ -68,3 +69,11 @@ func WithPermissions(permissions fs.FileMode) Option { return nil } } + +// WithSpecValidator sets a validator for the CDO spec. +func WithSpecValidator(validator SpecValidator) Option { + return func(o *options) error { + o.validator = validator + return nil + } +} diff --git a/api/producer/spec-format.go b/api/producer/spec-format.go index 489bdc9b..48f3e75e 100644 --- a/api/producer/spec-format.go +++ b/api/producer/spec-format.go @@ -82,7 +82,10 @@ func (p SpecFormat) normalizeFilename(filename string) (string, SpecFormat) { // This is currently a placeholder for validation that should be performed when // saving a spec. func (p *specFormatter) validate() error { - return nil + if p.validator == nil { + return nil + } + return p.validator.Validate(p.Spec) } // transform applies a transform to the spec associated with the CDI spec formatter. diff --git a/api/producer/validator-default.go b/api/producer/validator-default.go new file mode 100644 index 00000000..1908fa21 --- /dev/null +++ b/api/producer/validator-default.go @@ -0,0 +1,245 @@ +/* + Copyright © 2024 The CDI Authors + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package producer + +import ( + "errors" + "fmt" + "strings" + + cdi "tags.cncf.io/container-device-interface/specs-go" +) + +const ( + // DefaultValidator implements the basic checks for a valid CDI spec in code. + DefaultValidator = defaultValidator("default") +) + +type defaultValidator string + +var _ SpecValidator = (*defaultValidator)(nil) + +// errInvalid can be returned if CDI spec validation fails. +var errInvalid = errors.New("invalid") + +// Validate performs a default validation on a CDI spec. +func (v defaultValidator) Validate(s *cdi.Spec) (rerr error) { + defer func() { + if rerr != nil { + rerr = errors.Join(rerr, errInvalid) + } + }() + + if s == nil { + return fmt.Errorf("spec is nil") + } + if err := cdi.ValidateVersion(s); err != nil { + return err + } + if err := ValidateKind(s.Kind); err != nil { + return err + } + if err := ValidateSpecAnnotations(s.Kind, s.Annotations); err != nil { + return err + } + if err := v.validateEdits(&s.ContainerEdits); err != nil { + return err + } + + seen := make(map[string]bool) + for _, d := range s.Devices { + if seen[d.Name] { + return fmt.Errorf("invalid spec, multiple device %q", d.Name) + } + seen[d.Name] = true + if err := v.ValidateDevice(&d, s.Kind); err != nil { + return fmt.Errorf("invalid device %q: %w", d.Name, err) + } + } + if len(seen) == 0 { + return fmt.Errorf("invalid spec, no devices") + } + + return nil +} + +// ValidatedDevice validates a CDI device. +// The kind is optional and is used to provide additional context when validating annotations. +func (v defaultValidator) ValidateDevice(d *cdi.Device, kind string) error { + if err := ValidateDeviceName(d.Name); err != nil { + return err + } + + name := d.Name + if kind != "" { + name = kind + "=" + name + } + if err := ValidateSpecAnnotations(name, d.Annotations); err != nil { + return err + } + + if err := v.assertNonEmptyEdits(&d.ContainerEdits); err != nil { + return err + } + if err := v.validateEdits(&d.ContainerEdits); err != nil { + return err + } + return nil +} + +func (v defaultValidator) assertNonEmptyEdits(e *cdi.ContainerEdits) error { + if e == nil { + return nil + } + if len(e.Env) > 0 { + return nil + } + if len(e.DeviceNodes) > 0 { + return nil + } + if len(e.Hooks) > 0 { + return nil + } + if len(e.Mounts) > 0 { + return nil + } + if len(e.AdditionalGIDs) > 0 { + return nil + } + if e.IntelRdt != nil { + return nil + } + return errors.New("empty container edits") +} + +func (v defaultValidator) validateEdits(e *cdi.ContainerEdits) error { + if e == nil { + return nil + } + if err := v.validateEnv(e.Env); err != nil { + return fmt.Errorf("invalid container edits: %w", err) + } + for _, d := range e.DeviceNodes { + if err := v.validateDeviceNode(d); err != nil { + return err + } + } + for _, h := range e.Hooks { + if err := v.validateHook(h); err != nil { + return err + } + } + for _, m := range e.Mounts { + if err := v.validateMount(m); err != nil { + return err + } + } + if err := v.validateIntelRdt(e.IntelRdt); err != nil { + return err + } + return nil +} + +func (v defaultValidator) validateEnv(env []string) error { + for _, v := range env { + if strings.IndexByte(v, byte('=')) <= 0 { + return fmt.Errorf("invalid environment variable %q", v) + } + } + return nil +} + +func (v defaultValidator) validateDeviceNode(d *cdi.DeviceNode) error { + validTypes := map[string]struct{}{ + "": {}, + "b": {}, + "c": {}, + "u": {}, + "p": {}, + } + + if d.Path == "" { + return errors.New("invalid (empty) device path") + } + if _, ok := validTypes[d.Type]; !ok { + return fmt.Errorf("device %q: invalid type %q", d.Path, d.Type) + } + for _, bit := range d.Permissions { + if bit != 'r' && bit != 'w' && bit != 'm' { + return fmt.Errorf("device %q: invalid permissions %q", + d.Path, d.Permissions) + } + } + return nil +} + +func (v defaultValidator) validateHook(h *cdi.Hook) error { + const ( + // PrestartHook is the name of the OCI "prestart" hook. + PrestartHook = "prestart" + // CreateRuntimeHook is the name of the OCI "createRuntime" hook. + CreateRuntimeHook = "createRuntime" + // CreateContainerHook is the name of the OCI "createContainer" hook. + CreateContainerHook = "createContainer" + // StartContainerHook is the name of the OCI "startContainer" hook. + StartContainerHook = "startContainer" + // PoststartHook is the name of the OCI "poststart" hook. + PoststartHook = "poststart" + // PoststopHook is the name of the OCI "poststop" hook. + PoststopHook = "poststop" + ) + validHookNames := map[string]struct{}{ + PrestartHook: {}, + CreateRuntimeHook: {}, + CreateContainerHook: {}, + StartContainerHook: {}, + PoststartHook: {}, + PoststopHook: {}, + } + + if _, ok := validHookNames[h.HookName]; !ok { + return fmt.Errorf("invalid hook name %q", h.HookName) + } + if h.Path == "" { + return fmt.Errorf("invalid hook %q with empty path", h.HookName) + } + if err := v.validateEnv(h.Env); err != nil { + return fmt.Errorf("invalid hook %q: %w", h.HookName, err) + } + return nil +} + +func (v defaultValidator) validateMount(m *cdi.Mount) error { + if m.HostPath == "" { + return errors.New("invalid mount, empty host path") + } + if m.ContainerPath == "" { + return errors.New("invalid mount, empty container path") + } + return nil +} + +func (v defaultValidator) validateIntelRdt(i *cdi.IntelRdt) error { + if i == nil { + return nil + } + // ClosID must be a valid Linux filename + if len(i.ClosID) >= 4096 || i.ClosID == "." || i.ClosID == ".." || strings.ContainsAny(i.ClosID, "/\n") { + return errors.New("invalid ClosID") + } + return nil +} diff --git a/api/producer/validator-default_test.go b/api/producer/validator-default_test.go new file mode 100644 index 00000000..d733096c --- /dev/null +++ b/api/producer/validator-default_test.go @@ -0,0 +1,365 @@ +/* + Copyright © 2024 The CDI Authors + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package producer + +import ( + "testing" + + "github.com/stretchr/testify/require" + "sigs.k8s.io/yaml" + + cdi "tags.cncf.io/container-device-interface/specs-go" +) + +func TestValidateSpec(t *testing.T) { + testCases := []struct { + name string + spec string + invalid bool + }{ + { + name: "invalid kind", + spec: ` +cdiVersion: "0.3.0" +kind: "vendor1.comdevice" +devices: + - name: "dev1" + containerEdits: + deviceNodes: + - path: "/dev/vendor1-dev1" + type: b + major: 10 + minor: 1 + - name: "dev2" + containerEdits: + deviceNodes: + - path: "/dev/vendor1-dev2" + type: b + major: 10 + minor: 2 +`, + invalid: true, + }, + { + name: "no device name", + spec: ` +cdiVersion: "0.3.0" +kind: "vendor3.com/device" +containerEdits: + deviceNodes: + - path: "/dev/vendor3-dev1" + type: b + major: 10 + minor: 1 +`, + invalid: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + spec := cdi.Spec{} + err := yaml.Unmarshal([]byte(tc.spec), &spec) + require.NoError(t, err) + require.NotNil(t, spec) + + err = DefaultValidator.Validate(&spec) + if tc.invalid { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} + +func TestValidateContainerEdits(t *testing.T) { + type testCase struct { + name string + edits *cdi.ContainerEdits + invalid bool + } + for _, tc := range []*testCase{ + { + name: "valid, empty edits", + edits: &cdi.ContainerEdits{}, + }, + { + name: "valid, env var", + edits: &cdi.ContainerEdits{ + Env: []string{"BAR=BARVALUE1"}, + }, + }, + { + name: "invalid env, empty var", + edits: &cdi.ContainerEdits{ + Env: []string{""}, + }, + invalid: true, + }, + { + name: "invalid env, no var name", + edits: &cdi.ContainerEdits{ + Env: []string{"=foo"}, + }, + invalid: true, + }, + { + name: "invalid env, no assignment", + edits: &cdi.ContainerEdits{ + Env: []string{"FOOBAR"}, + }, + invalid: true, + }, + { + name: "valid device, path only", + edits: &cdi.ContainerEdits{ + DeviceNodes: []*cdi.DeviceNode{ + { + Path: "/dev/null", + }, + }, + }, + }, + { + name: "valid device, path+type", + edits: &cdi.ContainerEdits{ + DeviceNodes: []*cdi.DeviceNode{ + { + Path: "/dev/null", + Type: "c", + }, + }, + }, + }, + { + name: "valid device, path+type+permissions", + edits: &cdi.ContainerEdits{ + DeviceNodes: []*cdi.DeviceNode{ + { + Path: "/dev/null", + Type: "b", + Permissions: "rwm", + }, + }, + }, + }, + { + name: "invalid device, empty path", + edits: &cdi.ContainerEdits{ + DeviceNodes: []*cdi.DeviceNode{ + { + Path: "", + }, + }, + }, + invalid: true, + }, + { + name: "invalid device, wrong type", + edits: &cdi.ContainerEdits{ + DeviceNodes: []*cdi.DeviceNode{ + { + Path: "/dev/vendorctl", + Type: "f", + }, + }, + }, + invalid: true, + }, + { + name: "invalid device, wrong permissions", + edits: &cdi.ContainerEdits{ + DeviceNodes: []*cdi.DeviceNode{ + { + Path: "/dev/vendorctl", + Type: "b", + Permissions: "to land", + }, + }, + }, + invalid: true, + }, + { + name: "valid mount", + edits: &cdi.ContainerEdits{ + Mounts: []*cdi.Mount{ + { + HostPath: "/dev/vendorctl", + ContainerPath: "/dev/vendorctl", + }, + }, + }, + }, + { + name: "invalid mount, empty host path", + edits: &cdi.ContainerEdits{ + Mounts: []*cdi.Mount{ + { + HostPath: "", + ContainerPath: "/dev/vendorctl", + }, + }, + }, + invalid: true, + }, + { + name: "invalid mount, empty container path", + edits: &cdi.ContainerEdits{ + Mounts: []*cdi.Mount{ + { + HostPath: "/dev/vendorctl", + ContainerPath: "", + }, + }, + }, + invalid: true, + }, + { + name: "valid hooks", + edits: &cdi.ContainerEdits{ + Hooks: []*cdi.Hook{ + { + HookName: "prestart", + Path: "/usr/local/bin/prestart-vendor-hook", + Args: []string{"--verbose"}, + Env: []string{"VENDOR_ENV1=value1"}, + }, + { + HookName: "createRuntime", + Path: "/usr/local/bin/cr-vendor-hook", + Args: []string{"--debug"}, + Env: []string{"VENDOR_ENV2=value2"}, + }, + { + HookName: "createContainer", + Path: "/usr/local/bin/cc-vendor-hook", + Args: []string{"--create"}, + Env: []string{"VENDOR_ENV3=value3"}, + }, + { + HookName: "startContainer", + Path: "/usr/local/bin/sc-vendor-hook", + Args: []string{"--start"}, + Env: []string{"VENDOR_ENV4=value4"}, + }, + { + HookName: "poststart", + Path: "/usr/local/bin/poststart-vendor-hook", + Env: []string{"VENDOR_ENV5=value5"}, + }, + { + HookName: "poststop", + Path: "/usr/local/bin/poststop-vendor-hook", + }, + }, + }, + }, + { + name: "invalid hook, empty path", + edits: &cdi.ContainerEdits{ + Hooks: []*cdi.Hook{ + { + HookName: "prestart", + }, + }, + }, + invalid: true, + }, + { + name: "invalid hook, wrong hook name", + edits: &cdi.ContainerEdits{ + Hooks: []*cdi.Hook{ + { + HookName: "misCreateRuntime", + Path: "/usr/local/bin/cr-vendor-hook", + Args: []string{"--debug"}, + Env: []string{"VENDOR_ENV2=value2"}, + }, + }, + }, + invalid: true, + }, + { + name: "invalid hook, wrong env", + edits: &cdi.ContainerEdits{ + Hooks: []*cdi.Hook{ + { + HookName: "poststart", + Path: "/usr/local/bin/cr-vendor-hook", + Args: []string{"--debug"}, + Env: []string{"=value2"}, + }, + }, + }, + invalid: true, + }, + { + name: "valid rdt config", + edits: &cdi.ContainerEdits{ + IntelRdt: &cdi.IntelRdt{ + ClosID: "foo.bar", + }, + }, + }, + { + name: "invalid rdt config, invalid closID (slash)", + edits: &cdi.ContainerEdits{ + IntelRdt: &cdi.IntelRdt{ + ClosID: "foo/bar", + }, + }, + invalid: true, + }, + { + name: "invalid rdt config, invalid closID (dot)", + edits: &cdi.ContainerEdits{ + IntelRdt: &cdi.IntelRdt{ + ClosID: ".", + }, + }, + invalid: true, + }, + { + name: "invalid rdt config, invalid closID (double dot)", + edits: &cdi.ContainerEdits{ + IntelRdt: &cdi.IntelRdt{ + ClosID: "..", + }, + }, + invalid: true, + }, + { + name: "invalid rdt config, invalid closID (newline)", + edits: &cdi.ContainerEdits{ + IntelRdt: &cdi.IntelRdt{ + ClosID: "foo\nbar", + }, + }, + invalid: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + err := defaultValidator("default").validateEdits(tc.edits) + if tc.invalid { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/api/producer/writer.go b/api/producer/writer.go index 8b288b2b..72575732 100644 --- a/api/producer/writer.go +++ b/api/producer/writer.go @@ -39,6 +39,7 @@ func NewSpecWriter(opts ...Option) (*SpecWriter, error) { permissions: 0600, specFormat: DefaultSpecFormat, detectMinimumVersion: false, + validator: DefaultValidator, }, } for _, opt := range opts { diff --git a/api/producer/writer_test.go b/api/producer/writer_test.go index 59ad88c6..30fde675 100644 --- a/api/producer/writer_test.go +++ b/api/producer/writer_test.go @@ -17,6 +17,7 @@ package producer import ( + "errors" "os" "path/filepath" "testing" @@ -26,6 +27,9 @@ import ( ) func TestSave(t *testing.T) { + + errValidation := errors.New("test error") + testCases := []struct { description string spec cdi.Spec @@ -183,6 +187,54 @@ devices: kind: example.com/class `, }, + { + description: "validation can be disabled", + spec: cdi.Spec{ + Version: "INVALID", + Kind: "example.com/class", + ContainerEdits: cdi.ContainerEdits{ + DeviceNodes: []*cdi.DeviceNode{ + { + Path: "/dev/foo", + }, + }, + }, + }, + options: []Option{WithSpecValidator(nil)}, + filename: "foo", + expectedFilename: "foo.yaml", + expectedPermissions: 0600, + expectedOutput: `--- +cdiVersion: INVALID +containerEdits: + deviceNodes: + - path: /dev/foo +devices: null +kind: example.com/class +`, + }, + { + description: "validation error is returned", + spec: cdi.Spec{ + Version: cdi.CurrentVersion, + Kind: "example.com/class", + Devices: []cdi.Device{ + { + Name: "dev1", + ContainerEdits: cdi.ContainerEdits{ + DeviceNodes: []*cdi.DeviceNode{ + { + Path: "/dev/foo", + }, + }, + }, + }, + }, + }, + options: []Option{WithSpecValidator(&validatorWithError{errValidation})}, + filename: "foo", + expectedError: errValidation, + }, } for _, tc := range testCases { @@ -211,6 +263,7 @@ kind: example.com/class } } +// A validatorWithError always returns the specified error on validation. type validatorWithError struct { err error } From 08fb223b568770d7654349501e81fbab32d01a60 Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Tue, 4 Feb 2025 15:44:41 +0100 Subject: [PATCH 04/10] Use producer validation in cache Signed-off-by: Evan Lezar --- pkg/cdi/cache.go | 18 +++++++----------- pkg/cdi/device.go | 27 +++++++-------------------- pkg/cdi/spec.go | 17 +++-------------- 3 files changed, 17 insertions(+), 45 deletions(-) diff --git a/pkg/cdi/cache.go b/pkg/cdi/cache.go index fe2e3576..fc51e667 100644 --- a/pkg/cdi/cache.go +++ b/pkg/cdi/cache.go @@ -282,35 +282,31 @@ func (c *Cache) highestPrioritySpecDir() (string, int) { // WriteSpec writes a Spec file with the given content into the highest // priority Spec directory. If name has a "json" or "yaml" extension it // choses the encoding. Otherwise the default YAML encoding is used. +// +// Deprecated: use producer.NewSpecWriter instead. func (c *Cache) WriteSpec(raw *cdi.Spec, name string) error { var ( specDir string - prio int - spec *Spec err error ) - specDir, prio = c.highestPrioritySpecDir() + specDir, _ = c.highestPrioritySpecDir() if specDir == "" { return errors.New("no Spec directories to write to") } path := filepath.Join(specDir, name) - - // We call newSpec to perform the required validation. - // This also sets the extension of the path. - spec, err = newSpec(raw, path, prio) - if err != nil { - return err + if ext := filepath.Ext(path); ext != ".yaml" && ext != ".json" { + path += defaultSpecExt } + path = filepath.Clean(path) p, err := producer.NewSpecWriter( - producer.WithSpecFormat(producer.SpecFormatYAML), producer.WithOverwrite(true), ) if err != nil { return err } - if _, err := p.Save(spec.Spec, spec.path); err != nil { + if _, err := p.Save(raw, path); err != nil { return err } return nil diff --git a/pkg/cdi/device.go b/pkg/cdi/device.go index 80629eff..2117cdee 100644 --- a/pkg/cdi/device.go +++ b/pkg/cdi/device.go @@ -17,11 +17,8 @@ package cdi import ( - "fmt" - oci "github.com/opencontainers/runtime-spec/specs-go" - "tags.cncf.io/container-device-interface/internal/validation" - "tags.cncf.io/container-device-interface/pkg/parser" + "tags.cncf.io/container-device-interface/api/producer" cdi "tags.cncf.io/container-device-interface/specs-go" ) @@ -52,6 +49,9 @@ func (d *Device) GetSpec() *Spec { // GetQualifiedName returns the qualified name for this device. func (d *Device) GetQualifiedName() string { + if d.spec == nil { + return d.Name + } return d.spec.Kind + "=" + d.Name } @@ -67,22 +67,9 @@ func (d *Device) edits() *ContainerEdits { // Validate the device. func (d *Device) validate() error { - if err := parser.ValidateDeviceName(d.Name); err != nil { - return err - } - name := d.Name + var kind string if d.spec != nil { - name = d.GetQualifiedName() - } - if err := validation.ValidateSpecAnnotations(name, d.Annotations); err != nil { - return err - } - edits := d.edits() - if edits.isEmpty() { - return fmt.Errorf("invalid device, empty device edits") - } - if err := edits.Validate(); err != nil { - return fmt.Errorf("invalid device %q: %w", d.Name, err) + kind = d.spec.Kind } - return nil + return producer.DefaultValidator.ValidateDevice(d.Device, kind) } diff --git a/pkg/cdi/spec.go b/pkg/cdi/spec.go index b826aa2e..5ef8f5b1 100644 --- a/pkg/cdi/spec.go +++ b/pkg/cdi/spec.go @@ -26,7 +26,7 @@ import ( oci "github.com/opencontainers/runtime-spec/specs-go" "sigs.k8s.io/yaml" - "tags.cncf.io/container-device-interface/internal/validation" + "tags.cncf.io/container-device-interface/api/producer" "tags.cncf.io/container-device-interface/pkg/parser" cdi "tags.cncf.io/container-device-interface/specs-go" ) @@ -161,22 +161,11 @@ func MinimumRequiredVersion(spec *cdi.Spec) (string, error) { // Validate the Spec. func (s *Spec) validate() (map[string]*Device, error) { - if err := cdi.ValidateVersion(s.Spec); err != nil { - return nil, err - } - if err := parser.ValidateVendorName(s.vendor); err != nil { - return nil, err - } - if err := parser.ValidateClassName(s.class); err != nil { - return nil, err - } - if err := validation.ValidateSpecAnnotations(s.Kind, s.Annotations); err != nil { - return nil, err - } - if err := s.edits().Validate(); err != nil { + if err := producer.DefaultValidator.Validate(s.Spec); err != nil { return nil, err } + // We construct a list of devices associated with the spec. devices := make(map[string]*Device) for _, d := range s.Devices { dev, err := newDevice(s, d) From d2dd125123b8bed6b0a5cc47c213e94fba72befb Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Tue, 4 Feb 2025 20:41:03 +0100 Subject: [PATCH 05/10] Remove unused isEmpty function Signed-off-by: Evan Lezar --- pkg/cdi/container-edits.go | 27 --------------------------- 1 file changed, 27 deletions(-) diff --git a/pkg/cdi/container-edits.go b/pkg/cdi/container-edits.go index 4744eff8..ac8543d8 100644 --- a/pkg/cdi/container-edits.go +++ b/pkg/cdi/container-edits.go @@ -220,33 +220,6 @@ func (e *ContainerEdits) Append(o *ContainerEdits) *ContainerEdits { return e } -// isEmpty returns true if these edits are empty. This is valid in a -// global Spec context but invalid in a Device context. -func (e *ContainerEdits) isEmpty() bool { - if e == nil { - return false - } - if len(e.Env) > 0 { - return false - } - if len(e.DeviceNodes) > 0 { - return false - } - if len(e.Hooks) > 0 { - return false - } - if len(e.Mounts) > 0 { - return false - } - if len(e.AdditionalGIDs) > 0 { - return false - } - if e.IntelRdt != nil { - return false - } - return true -} - // ValidateEnv validates the given environment variables. func ValidateEnv(env []string) error { for _, v := range env { From c4e27054e5390d8c23aa0f7fbc4c1b64841bfc89 Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Tue, 4 Feb 2025 22:45:51 +0100 Subject: [PATCH 06/10] Use producer.ValidateAnnotations Signed-off-by: Evan Lezar --- cmd/validate/go.mod | 3 +- cmd/validate/go.sum | 1 + internal/validation/k8s/objectmeta.go | 56 ------- internal/validation/k8s/validation.go | 217 -------------------------- internal/validation/validate.go | 56 ------- schema/go.mod | 2 +- schema/schema.go | 6 +- 7 files changed, 7 insertions(+), 334 deletions(-) delete mode 100644 internal/validation/k8s/objectmeta.go delete mode 100644 internal/validation/k8s/validation.go delete mode 100644 internal/validation/validate.go diff --git a/cmd/validate/go.mod b/cmd/validate/go.mod index 4a47d4f0..11c1bce5 100644 --- a/cmd/validate/go.mod +++ b/cmd/validate/go.mod @@ -9,9 +9,10 @@ require ( github.com/xeipuuv/gojsonreference v0.0.0-20180127040603-bd5ef7bd5415 // indirect github.com/xeipuuv/gojsonschema v1.2.0 // indirect golang.org/x/mod v0.19.0 // indirect + golang.org/x/sys v0.19.0 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect sigs.k8s.io/yaml v1.3.0 // indirect - tags.cncf.io/container-device-interface v0.0.0 // indirect + tags.cncf.io/container-device-interface/api/producer v0.8.0 // indirect tags.cncf.io/container-device-interface/specs-go v0.8.0 // indirect ) diff --git a/cmd/validate/go.sum b/cmd/validate/go.sum index 9f83b553..0f223b72 100644 --- a/cmd/validate/go.sum +++ b/cmd/validate/go.sum @@ -19,6 +19,7 @@ github.com/xeipuuv/gojsonschema v1.2.0/go.mod h1:anYRn/JVcOK2ZgGU+IjEV4nwlhoK5sQ golang.org/x/mod v0.19.0 h1:fEdghXQSo20giMthA7cd28ZC+jts4amQ3YMXiP5oMQ8= golang.org/x/mod v0.19.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= golang.org/x/sys v0.19.0 h1:q5f1RH2jigJ1MoAWp2KTp3gm5zAGFUTarQZ5U386+4o= +golang.org/x/sys v0.19.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v2 v2.4.0 h1:D8xgwECY7CYvx+Y2n4sBz93Jn9JRvxdiyyo8CTfuKaY= diff --git a/internal/validation/k8s/objectmeta.go b/internal/validation/k8s/objectmeta.go deleted file mode 100644 index fb86c67a..00000000 --- a/internal/validation/k8s/objectmeta.go +++ /dev/null @@ -1,56 +0,0 @@ -/* -Copyright 2014 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -// Adapted from k8s.io/apimachinery/pkg/api/validation: -// https://github.com/kubernetes/apimachinery/blob/7687996c715ee7d5c8cf1e3215e607eb065a4221/pkg/api/validation/objectmeta.go - -package k8s - -import ( - "errors" - "fmt" - "strings" -) - -// TotalAnnotationSizeLimitB defines the maximum size of all annotations in characters. -const TotalAnnotationSizeLimitB int = 256 * (1 << 10) // 256 kB - -// ValidateAnnotations validates that a set of annotations are correctly defined. -func ValidateAnnotations(annotations map[string]string, path string) error { - errs := []error{} - for k := range annotations { - // The rule is QualifiedName except that case doesn't matter, so convert to lowercase before checking. - for _, msg := range IsQualifiedName(strings.ToLower(k)) { - errs = append(errs, fmt.Errorf("%v.%v is invalid: %v", path, k, msg)) - } - } - if err := ValidateAnnotationsSize(annotations); err != nil { - errs = append(errs, fmt.Errorf("%v is too long: %v", path, err)) - } - return errors.Join(errs...) -} - -// ValidateAnnotationsSize validates that a set of annotations is not too large. -func ValidateAnnotationsSize(annotations map[string]string) error { - var totalSize int64 - for k, v := range annotations { - totalSize += (int64)(len(k)) + (int64)(len(v)) - } - if totalSize > (int64)(TotalAnnotationSizeLimitB) { - return fmt.Errorf("annotations size %d is larger than limit %d", totalSize, TotalAnnotationSizeLimitB) - } - return nil -} diff --git a/internal/validation/k8s/validation.go b/internal/validation/k8s/validation.go deleted file mode 100644 index 5ad6ce27..00000000 --- a/internal/validation/k8s/validation.go +++ /dev/null @@ -1,217 +0,0 @@ -/* -Copyright 2014 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -// Adapted from k8s.io/apimachinery/pkg/util/validation: -// https://github.com/kubernetes/apimachinery/blob/7687996c715ee7d5c8cf1e3215e607eb065a4221/pkg/util/validation/validation.go - -package k8s - -import ( - "fmt" - "regexp" - "strings" -) - -const qnameCharFmt string = "[A-Za-z0-9]" -const qnameExtCharFmt string = "[-A-Za-z0-9_.]" -const qualifiedNameFmt string = "(" + qnameCharFmt + qnameExtCharFmt + "*)?" + qnameCharFmt -const qualifiedNameErrMsg string = "must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character" -const qualifiedNameMaxLength int = 63 - -var qualifiedNameRegexp = regexp.MustCompile("^" + qualifiedNameFmt + "$") - -// IsQualifiedName tests whether the value passed is what Kubernetes calls a -// "qualified name". This is a format used in various places throughout the -// system. If the value is not valid, a list of error strings is returned. -// Otherwise an empty list (or nil) is returned. -func IsQualifiedName(value string) []string { - var errs []string - parts := strings.Split(value, "/") - var name string - switch len(parts) { - case 1: - name = parts[0] - case 2: - var prefix string - prefix, name = parts[0], parts[1] - if len(prefix) == 0 { - errs = append(errs, "prefix part "+EmptyError()) - } else if msgs := IsDNS1123Subdomain(prefix); len(msgs) != 0 { - errs = append(errs, prefixEach(msgs, "prefix part ")...) - } - default: - return append(errs, "a qualified name "+RegexError(qualifiedNameErrMsg, qualifiedNameFmt, "MyName", "my.name", "123-abc")+ - " with an optional DNS subdomain prefix and '/' (e.g. 'example.com/MyName')") - } - - if len(name) == 0 { - errs = append(errs, "name part "+EmptyError()) - } else if len(name) > qualifiedNameMaxLength { - errs = append(errs, "name part "+MaxLenError(qualifiedNameMaxLength)) - } - if !qualifiedNameRegexp.MatchString(name) { - errs = append(errs, "name part "+RegexError(qualifiedNameErrMsg, qualifiedNameFmt, "MyName", "my.name", "123-abc")) - } - return errs -} - -const labelValueFmt string = "(" + qualifiedNameFmt + ")?" -const labelValueErrMsg string = "a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character" - -// LabelValueMaxLength is a label's max length -const LabelValueMaxLength int = 63 - -var labelValueRegexp = regexp.MustCompile("^" + labelValueFmt + "$") - -// IsValidLabelValue tests whether the value passed is a valid label value. If -// the value is not valid, a list of error strings is returned. Otherwise an -// empty list (or nil) is returned. -func IsValidLabelValue(value string) []string { - var errs []string - if len(value) > LabelValueMaxLength { - errs = append(errs, MaxLenError(LabelValueMaxLength)) - } - if !labelValueRegexp.MatchString(value) { - errs = append(errs, RegexError(labelValueErrMsg, labelValueFmt, "MyValue", "my_value", "12345")) - } - return errs -} - -const dns1123LabelFmt string = "[a-z0-9]([-a-z0-9]*[a-z0-9])?" -const dns1123LabelErrMsg string = "a lowercase RFC 1123 label must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character" - -// DNS1123LabelMaxLength is a label's max length in DNS (RFC 1123) -const DNS1123LabelMaxLength int = 63 - -var dns1123LabelRegexp = regexp.MustCompile("^" + dns1123LabelFmt + "$") - -// IsDNS1123Label tests for a string that conforms to the definition of a label in -// DNS (RFC 1123). -func IsDNS1123Label(value string) []string { - var errs []string - if len(value) > DNS1123LabelMaxLength { - errs = append(errs, MaxLenError(DNS1123LabelMaxLength)) - } - if !dns1123LabelRegexp.MatchString(value) { - errs = append(errs, RegexError(dns1123LabelErrMsg, dns1123LabelFmt, "my-name", "123-abc")) - } - return errs -} - -const dns1123SubdomainFmt string = dns1123LabelFmt + "(\\." + dns1123LabelFmt + ")*" -const dns1123SubdomainErrorMsg string = "a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character" - -// DNS1123SubdomainMaxLength is a subdomain's max length in DNS (RFC 1123) -const DNS1123SubdomainMaxLength int = 253 - -var dns1123SubdomainRegexp = regexp.MustCompile("^" + dns1123SubdomainFmt + "$") - -// IsDNS1123Subdomain tests for a string that conforms to the definition of a -// subdomain in DNS (RFC 1123). -func IsDNS1123Subdomain(value string) []string { - var errs []string - if len(value) > DNS1123SubdomainMaxLength { - errs = append(errs, MaxLenError(DNS1123SubdomainMaxLength)) - } - if !dns1123SubdomainRegexp.MatchString(value) { - errs = append(errs, RegexError(dns1123SubdomainErrorMsg, dns1123SubdomainFmt, "example.com")) - } - return errs -} - -const dns1035LabelFmt string = "[a-z]([-a-z0-9]*[a-z0-9])?" -const dns1035LabelErrMsg string = "a DNS-1035 label must consist of lower case alphanumeric characters or '-', start with an alphabetic character, and end with an alphanumeric character" - -// DNS1035LabelMaxLength is a label's max length in DNS (RFC 1035) -const DNS1035LabelMaxLength int = 63 - -var dns1035LabelRegexp = regexp.MustCompile("^" + dns1035LabelFmt + "$") - -// IsDNS1035Label tests for a string that conforms to the definition of a label in -// DNS (RFC 1035). -func IsDNS1035Label(value string) []string { - var errs []string - if len(value) > DNS1035LabelMaxLength { - errs = append(errs, MaxLenError(DNS1035LabelMaxLength)) - } - if !dns1035LabelRegexp.MatchString(value) { - errs = append(errs, RegexError(dns1035LabelErrMsg, dns1035LabelFmt, "my-name", "abc-123")) - } - return errs -} - -// wildcard definition - RFC 1034 section 4.3.3. -// examples: -// - valid: *.bar.com, *.foo.bar.com -// - invalid: *.*.bar.com, *.foo.*.com, *bar.com, f*.bar.com, * -const wildcardDNS1123SubdomainFmt = "\\*\\." + dns1123SubdomainFmt -const wildcardDNS1123SubdomainErrMsg = "a wildcard DNS-1123 subdomain must start with '*.', followed by a valid DNS subdomain, which must consist of lower case alphanumeric characters, '-' or '.' and end with an alphanumeric character" - -// IsWildcardDNS1123Subdomain tests for a string that conforms to the definition of a -// wildcard subdomain in DNS (RFC 1034 section 4.3.3). -func IsWildcardDNS1123Subdomain(value string) []string { - wildcardDNS1123SubdomainRegexp := regexp.MustCompile("^" + wildcardDNS1123SubdomainFmt + "$") - - var errs []string - if len(value) > DNS1123SubdomainMaxLength { - errs = append(errs, MaxLenError(DNS1123SubdomainMaxLength)) - } - if !wildcardDNS1123SubdomainRegexp.MatchString(value) { - errs = append(errs, RegexError(wildcardDNS1123SubdomainErrMsg, wildcardDNS1123SubdomainFmt, "*.example.com")) - } - return errs -} - -// MaxLenError returns a string explanation of a "string too long" validation -// failure. -func MaxLenError(length int) string { - return fmt.Sprintf("must be no more than %d characters", length) -} - -// RegexError returns a string explanation of a regex validation failure. -func RegexError(msg string, fmt string, examples ...string) string { - if len(examples) == 0 { - return msg + " (regex used for validation is '" + fmt + "')" - } - msg += " (e.g. " - for i := range examples { - if i > 0 { - msg += " or " - } - msg += "'" + examples[i] + "', " - } - msg += "regex used for validation is '" + fmt + "')" - return msg -} - -// EmptyError returns a string explanation of a "must not be empty" validation -// failure. -func EmptyError() string { - return "must be non-empty" -} - -func prefixEach(msgs []string, prefix string) []string { - for i := range msgs { - msgs[i] = prefix + msgs[i] - } - return msgs -} - -// InclusiveRangeError returns a string explanation of a numeric "must be -// between" validation failure. -func InclusiveRangeError(lo, hi int) string { - return fmt.Sprintf(`must be between %d and %d, inclusive`, lo, hi) -} diff --git a/internal/validation/validate.go b/internal/validation/validate.go deleted file mode 100644 index 5d9b55ff..00000000 --- a/internal/validation/validate.go +++ /dev/null @@ -1,56 +0,0 @@ -/* - Copyright © The CDI Authors - - Licensed under the Apache License, Version 2.0 (the "License"); - you may not use this file except in compliance with the License. - You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - - Unless required by applicable law or agreed to in writing, software - distributed under the License is distributed on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - See the License for the specific language governing permissions and - limitations under the License. -*/ - -package validation - -import ( - "fmt" - "strings" - - "tags.cncf.io/container-device-interface/internal/validation/k8s" -) - -// ValidateSpecAnnotations checks whether spec annotations are valid. -func ValidateSpecAnnotations(name string, any interface{}) error { - if any == nil { - return nil - } - - switch v := any.(type) { - case map[string]interface{}: - annotations := make(map[string]string) - for k, v := range v { - if s, ok := v.(string); ok { - annotations[k] = s - } else { - return fmt.Errorf("invalid annotation %v.%v; %v is not a string", name, k, any) - } - } - return validateSpecAnnotations(name, annotations) - } - - return nil -} - -// validateSpecAnnotations checks whether spec annotations are valid. -func validateSpecAnnotations(name string, annotations map[string]string) error { - path := "annotations" - if name != "" { - path = strings.Join([]string{name, path}, ".") - } - - return k8s.ValidateAnnotations(annotations, path) -} diff --git a/schema/go.mod b/schema/go.mod index c469c3f9..2d59f5aa 100644 --- a/schema/go.mod +++ b/schema/go.mod @@ -7,6 +7,7 @@ require ( github.com/xeipuuv/gojsonschema v1.2.0 sigs.k8s.io/yaml v1.3.0 tags.cncf.io/container-device-interface v0.0.0 + tags.cncf.io/container-device-interface/api/producer v0.8.0 tags.cncf.io/container-device-interface/specs-go v0.8.0 ) @@ -23,7 +24,6 @@ require ( golang.org/x/sys v0.19.0 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect - tags.cncf.io/container-device-interface/api/producer v0.8.0 // indirect ) replace ( diff --git a/schema/schema.go b/schema/schema.go index a6b00305..9fa13ee8 100644 --- a/schema/schema.go +++ b/schema/schema.go @@ -31,7 +31,7 @@ import ( "sigs.k8s.io/yaml" schema "github.com/xeipuuv/gojsonschema" - "tags.cncf.io/container-device-interface/internal/validation" + "tags.cncf.io/container-device-interface/api/producer" cdi "tags.cncf.io/container-device-interface/specs-go" ) @@ -315,7 +315,7 @@ func (s *Schema) validateContents(any map[string]interface{}) error { contents := schemaContents(any) if specAnnotations, ok := contents.getAnnotations(); ok { - if err := validation.ValidateSpecAnnotations("", specAnnotations); err != nil { + if err := producer.ValidateSpecAnnotations("", specAnnotations); err != nil { return err } } @@ -328,7 +328,7 @@ func (s *Schema) validateContents(any map[string]interface{}) error { for _, device := range devices { name, _ := device.getFieldAsString("name") if annotations, ok := device.getAnnotations(); ok { - if err := validation.ValidateSpecAnnotations(name, annotations); err != nil { + if err := producer.ValidateSpecAnnotations(name, annotations); err != nil { return err } } From df6e7f69309522cd8727af281e1f8ad30bed90ed Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Tue, 4 Feb 2025 23:18:43 +0100 Subject: [PATCH 07/10] Use producer validation from parser package Signed-off-by: Evan Lezar --- api/producer/identifiers.go | 99 ++++++++++++++++++++++++++++++++----- pkg/parser/parser.go | 94 ++++------------------------------- 2 files changed, 96 insertions(+), 97 deletions(-) diff --git a/api/producer/identifiers.go b/api/producer/identifiers.go index 8aeb2216..3eba1c79 100644 --- a/api/producer/identifiers.go +++ b/api/producer/identifiers.go @@ -21,6 +21,12 @@ import ( "strings" ) +// ValidateQualifiedName checks the validity of a fully-qualified device name. +func ValidateQualifiedName(name string) error { + _, _, _, err := ParseFullyQualifiedName(name) + return err +} + // ValidateKind checks the validity of a CDI kind. // The syntax for a device kind“ is // @@ -74,19 +80,19 @@ func validateVendorOrClassName(name string) error { if name == "" { return fmt.Errorf("empty name") } - if !IsLetter(rune(name[0])) { + if !isLetter(rune(name[0])) { return fmt.Errorf("%q, should start with letter", name) } for _, c := range string(name[1 : len(name)-1]) { switch { - case IsAlphaNumeric(c): + case isAlphaNumeric(c): case c == '_' || c == '-' || c == '.': default: return fmt.Errorf("invalid character '%c' in name %q", c, name) } } - if !IsAlphaNumeric(rune(name[len(name)-1])) { + if !isAlphaNumeric(rune(name[len(name)-1])) { return fmt.Errorf("%q, should end with a letter or digit", name) } @@ -102,7 +108,7 @@ func ValidateDeviceName(name string) error { if name == "" { return fmt.Errorf("invalid (empty) device name") } - if !IsAlphaNumeric(rune(name[0])) { + if !isAlphaNumeric(rune(name[0])) { return fmt.Errorf("invalid class %q, should start with a letter or digit", name) } if len(name) == 1 { @@ -110,30 +116,99 @@ func ValidateDeviceName(name string) error { } for _, c := range string(name[1 : len(name)-1]) { switch { - case IsAlphaNumeric(c): + case isAlphaNumeric(c): case c == '_' || c == '-' || c == '.' || c == ':': default: return fmt.Errorf("invalid character '%c' in device name %q", c, name) } } - if !IsAlphaNumeric(rune(name[len(name)-1])) { + if !isAlphaNumeric(rune(name[len(name)-1])) { return fmt.Errorf("invalid name %q, should end with a letter or digit", name) } return nil } -// IsLetter reports whether the rune is a letter. -func IsLetter(c rune) bool { +// ParseFullyQualifiedName splits a fully-qualified name into device vendor, class, +// and name. If the device fails to parse as a qualified name, or if any +// of the split components fail to pass syntax validation, vendor and +// class are returned as empty, together with the verbatim input as the +// name and an error describing the reason for failure. +func ParseFullyQualifiedName(device string) (string, string, string, error) { + vendor, class, name := parseDevice(device) + + if vendor == "" { + return "", "", device, fmt.Errorf("unqualified device %q, missing vendor", device) + } + if class == "" { + return "", "", device, fmt.Errorf("unqualified device %q, missing class", device) + } + if name == "" { + return "", "", device, fmt.Errorf("unqualified device %q, missing device name", device) + } + + if err := ValidateVendorName(vendor); err != nil { + return "", "", device, fmt.Errorf("invalid device %q: %w", device, err) + } + if err := ValidateClassName(class); err != nil { + return "", "", device, fmt.Errorf("invalid device %q: %w", device, err) + } + if err := ValidateDeviceName(name); err != nil { + return "", "", device, fmt.Errorf("invalid device %q: %w", device, err) + } + + return vendor, class, name, nil +} + +// parseDevice tries to split a device name into vendor, class, and name. +// If this fails, for instance in the case of unqualified device names, +// parseDevice returns an empty vendor and class together with name set +// to the verbatim input. +func parseDevice(device string) (string, string, string) { + if device == "" || device[0] == '/' { + return "", "", device + } + + parts := strings.SplitN(device, "=", 2) + if len(parts) != 2 || parts[0] == "" || parts[1] == "" { + return "", "", device + } + + name := parts[1] + vendor, class := ParseKind(parts[0]) + if vendor == "" { + return "", "", device + } + + return vendor, class, name +} + +// ParseKind splits a device qualifier into vendor and class. +// The syntax for a device qualifier is +// +// "/" +// +// If parsing fails, an empty vendor and the class set to the +// verbatim input is returned. +func ParseKind(kind string) (string, string) { + parts := strings.SplitN(kind, "/", 2) + if len(parts) != 2 || parts[0] == "" || parts[1] == "" { + return "", kind + } + return parts[0], parts[1] +} + +// isLetter reports whether the rune is a letter. +func isLetter(c rune) bool { return ('A' <= c && c <= 'Z') || ('a' <= c && c <= 'z') } // IsDigit reports whether the rune is a digit. -func IsDigit(c rune) bool { +func isDigit(c rune) bool { return '0' <= c && c <= '9' } -// IsAlphaNumeric reports whether the rune is a letter or digit. -func IsAlphaNumeric(c rune) bool { - return IsLetter(c) || IsDigit(c) +// isAlphaNumeric reports whether the rune is a letter or digit. +func isAlphaNumeric(c rune) bool { + return isLetter(c) || isDigit(c) } diff --git a/pkg/parser/parser.go b/pkg/parser/parser.go index 53259895..14ae4560 100644 --- a/pkg/parser/parser.go +++ b/pkg/parser/parser.go @@ -17,8 +17,9 @@ package parser import ( - "fmt" "strings" + + "tags.cncf.io/container-device-interface/api/producer" ) // QualifiedName returns the qualified name for a device. @@ -39,7 +40,7 @@ func QualifiedName(vendor, class, name string) string { // IsQualifiedName tests if a device name is qualified. func IsQualifiedName(device string) bool { - _, _, _, err := ParseQualifiedName(device) + err := producer.ValidateQualifiedName(device) return err == nil } @@ -49,35 +50,15 @@ func IsQualifiedName(device string) bool { // class are returned as empty, together with the verbatim input as the // name and an error describing the reason for failure. func ParseQualifiedName(device string) (string, string, string, error) { - vendor, class, name := ParseDevice(device) - - if vendor == "" { - return "", "", device, fmt.Errorf("unqualified device %q, missing vendor", device) - } - if class == "" { - return "", "", device, fmt.Errorf("unqualified device %q, missing class", device) - } - if name == "" { - return "", "", device, fmt.Errorf("unqualified device %q, missing device name", device) - } - - if err := ValidateVendorName(vendor); err != nil { - return "", "", device, fmt.Errorf("invalid device %q: %w", device, err) - } - if err := ValidateClassName(class); err != nil { - return "", "", device, fmt.Errorf("invalid device %q: %w", device, err) - } - if err := ValidateDeviceName(name); err != nil { - return "", "", device, fmt.Errorf("invalid device %q: %w", device, err) - } - - return vendor, class, name, nil + return producer.ParseFullyQualifiedName(device) } // ParseDevice tries to split a device name into vendor, class, and name. // If this fails, for instance in the case of unqualified device names, // ParseDevice returns an empty vendor and class together with name set // to the verbatim input. +// +// Deprecated: This function will be removed. Use producer.ParseQualifiedName instead. func ParseDevice(device string) (string, string, string) { if device == "" || device[0] == '/' { return "", "", device @@ -118,11 +99,7 @@ func ParseQualifier(kind string) (string, string) { // - digits ('0'-'9') // - underscore, dash, and dot ('_', '-', and '.') func ValidateVendorName(vendor string) error { - err := validateVendorOrClassName(vendor) - if err != nil { - err = fmt.Errorf("invalid vendor. %w", err) - } - return err + return producer.ValidateVendorName(vendor) } // ValidateClassName checks the validity of class name. @@ -131,39 +108,7 @@ func ValidateVendorName(vendor string) error { // - digits ('0'-'9') // - underscore, dash, and dot ('_', '-', and '.') func ValidateClassName(class string) error { - err := validateVendorOrClassName(class) - if err != nil { - err = fmt.Errorf("invalid class. %w", err) - } - return err -} - -// validateVendorOrClassName checks the validity of vendor or class name. -// A name may contain the following ASCII characters: -// - upper- and lowercase letters ('A'-'Z', 'a'-'z') -// - digits ('0'-'9') -// - underscore, dash, and dot ('_', '-', and '.') -func validateVendorOrClassName(name string) error { - if name == "" { - return fmt.Errorf("empty name") - } - if !IsLetter(rune(name[0])) { - return fmt.Errorf("%q, should start with letter", name) - } - for _, c := range string(name[1 : len(name)-1]) { - switch { - case IsAlphaNumeric(c): - case c == '_' || c == '-' || c == '.': - default: - return fmt.Errorf("invalid character '%c' in name %q", - c, name) - } - } - if !IsAlphaNumeric(rune(name[len(name)-1])) { - return fmt.Errorf("%q, should end with a letter or digit", name) - } - - return nil + return producer.ValidateClassName(class) } // ValidateDeviceName checks the validity of a device name. @@ -172,28 +117,7 @@ func validateVendorOrClassName(name string) error { // - digits ('0'-'9') // - underscore, dash, dot, colon ('_', '-', '.', ':') func ValidateDeviceName(name string) error { - if name == "" { - return fmt.Errorf("invalid (empty) device name") - } - if !IsAlphaNumeric(rune(name[0])) { - return fmt.Errorf("invalid class %q, should start with a letter or digit", name) - } - if len(name) == 1 { - return nil - } - for _, c := range string(name[1 : len(name)-1]) { - switch { - case IsAlphaNumeric(c): - case c == '_' || c == '-' || c == '.' || c == ':': - default: - return fmt.Errorf("invalid character '%c' in device name %q", - c, name) - } - } - if !IsAlphaNumeric(rune(name[len(name)-1])) { - return fmt.Errorf("invalid name %q, should end with a letter or digit", name) - } - return nil + return producer.ValidateDeviceName(name) } // IsLetter reports whether the rune is a letter. From 5fec3a5e0baa330a1d5b209f96e00f6a318abe38 Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Tue, 4 Feb 2025 23:19:08 +0100 Subject: [PATCH 08/10] Mark IsLetter, IsDigit, IsAlphanumeric as deprecated Signed-off-by: Evan Lezar --- pkg/parser/parser.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/parser/parser.go b/pkg/parser/parser.go index 14ae4560..45d035bd 100644 --- a/pkg/parser/parser.go +++ b/pkg/parser/parser.go @@ -121,16 +121,22 @@ func ValidateDeviceName(name string) error { } // IsLetter reports whether the rune is a letter. +// +// Deprecated: This check is internal and should not be part of the public API. func IsLetter(c rune) bool { return ('A' <= c && c <= 'Z') || ('a' <= c && c <= 'z') } // IsDigit reports whether the rune is a digit. +// +// Deprecated: This check is internal and should not be part of the public API. func IsDigit(c rune) bool { return '0' <= c && c <= '9' } // IsAlphaNumeric reports whether the rune is a letter or digit. +// +// Deprecated: This check is internal and should not be part of the public API. func IsAlphaNumeric(c rune) bool { return IsLetter(c) || IsDigit(c) } From a708dec307cd2007f1725757cbb153bd5dc40279 Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Tue, 4 Feb 2025 23:19:59 +0100 Subject: [PATCH 09/10] Move annotation generation to producer package Signed-off-by: Evan Lezar --- api/producer/annotations.go | 89 ++++++++ api/producer/annotations_test.go | 353 +++++++++++++++++++++++++++++++ pkg/cdi/annotations.go | 77 +------ 3 files changed, 453 insertions(+), 66 deletions(-) create mode 100644 api/producer/annotations_test.go diff --git a/api/producer/annotations.go b/api/producer/annotations.go index d451ad9f..fbe1dc18 100644 --- a/api/producer/annotations.go +++ b/api/producer/annotations.go @@ -17,12 +17,101 @@ package producer import ( + "errors" "fmt" "strings" "tags.cncf.io/container-device-interface/api/producer/k8s" ) +const ( + // AnnotationPrefix is the prefix for CDI container annotation keys. + AnnotationPrefix = "cdi.k8s.io/" +) + +// UpdateAnnotations updates annotations with a plugin-specific CDI device +// injection request for the given devices. Upon any error a non-nil error +// is returned and annotations are left intact. By convention plugin should +// be in the format of "vendor.device-type". +func UpdateAnnotations(annotations map[string]string, plugin string, deviceID string, devices []string) (map[string]string, error) { + key, err := AnnotationKey(plugin, deviceID) + if err != nil { + return annotations, fmt.Errorf("CDI annotation failed: %w", err) + } + if _, ok := annotations[key]; ok { + return annotations, fmt.Errorf("CDI annotation failed, key %q used", key) + } + value, err := AnnotationValue(devices) + if err != nil { + return annotations, fmt.Errorf("CDI annotation failed: %w", err) + } + + if annotations == nil { + annotations = make(map[string]string) + } + annotations[key] = value + + return annotations, nil +} + +// AnnotationKey returns a unique annotation key for an device allocation +// by a K8s device plugin. pluginName should be in the format of +// "vendor.device-type". deviceID is the ID of the device the plugin is +// allocating. It is used to make sure that the generated key is unique +// even if multiple allocations by a single plugin needs to be annotated. +func AnnotationKey(pluginName, deviceID string) (string, error) { + const maxNameLen = 63 + + if pluginName == "" { + return "", errors.New("invalid plugin name, empty") + } + if deviceID == "" { + return "", errors.New("invalid deviceID, empty") + } + + name := pluginName + "_" + strings.ReplaceAll(deviceID, "/", "_") + + if len(name) > maxNameLen { + return "", fmt.Errorf("invalid plugin+deviceID %q, too long", name) + } + + if c := rune(name[0]); !isAlphaNumeric(c) { + return "", fmt.Errorf("invalid name %q, first '%c' should be alphanumeric", + name, c) + } + if len(name) > 2 { + for _, c := range name[1 : len(name)-1] { + switch { + case isAlphaNumeric(c): + case c == '_' || c == '-' || c == '.': + default: + return "", fmt.Errorf("invalid name %q, invalid character '%c'", + name, c) + } + } + } + if c := rune(name[len(name)-1]); !isAlphaNumeric(c) { + return "", fmt.Errorf("invalid name %q, last '%c' should be alphanumeric", + name, c) + } + + return AnnotationPrefix + name, nil +} + +// AnnotationValue returns an annotation value for the given devices. +func AnnotationValue(devices []string) (string, error) { + value, sep := "", "" + for _, d := range devices { + if err := ValidateQualifiedName(d); err != nil { + return "", err + } + value += sep + d + sep = "," + } + + return value, nil +} + // ValidateSpecAnnotations checks whether spec annotations are valid. func ValidateSpecAnnotations(name string, any interface{}) error { if any == nil { diff --git a/api/producer/annotations_test.go b/api/producer/annotations_test.go new file mode 100644 index 00000000..522a7e82 --- /dev/null +++ b/api/producer/annotations_test.go @@ -0,0 +1,353 @@ +/* + Copyright © 2022 The CDI Authors + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package producer + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestAnnotationKey(t *testing.T) { + type testCase = struct { + name string + plugin string + devID string + key string + invalid bool + } + + for _, tc := range []*testCase{ + { + name: "invalid, empty plugin", + plugin: "", + invalid: true, + }, + { + name: "invalid, empty device ID", + plugin: "plugin", + devID: "", + invalid: true, + }, + { + name: "invalid, non-alphanumeric first character", + plugin: "_vendor.class", + devID: "device", + invalid: true, + }, + { + name: "invalid, non-alphanumeric last character", + plugin: "vendor.class", + devID: "device_", + invalid: true, + }, + { + name: "invalid, plugin contains invalid characters", + plugin: "ven.dor-cl+ass", + devID: "device", + invalid: true, + }, + { + name: "invalid, devID contains invalid characters", + plugin: "vendor.class", + devID: "dev+ice", + invalid: true, + }, + { + name: "invalid, too plugin long", + plugin: "123456789012345678901234567890123456789012345678901234567", + devID: "device", + invalid: true, + }, + { + name: "valid, simple", + plugin: "vendor.class", + devID: "device", + key: AnnotationPrefix + "vendor.class" + "_" + "device", + }, + { + name: "valid, with special characters", + plugin: "v-e.n_d.or.cl-as_s", + devID: "d_e-v-i-c_e", + key: AnnotationPrefix + "v-e.n_d.or.cl-as_s" + "_" + "d_e-v-i-c_e", + }, + { + name: "valid, with /'s replaced in devID", + plugin: "v-e.n_d.or.cl-as_s", + devID: "d-e/v/i/c-e", + key: AnnotationPrefix + "v-e.n_d.or.cl-as_s" + "_" + "d-e_v_i_c-e", + }, + } { + t.Run(tc.name, func(t *testing.T) { + key, err := AnnotationKey(tc.plugin, tc.devID) + if !tc.invalid { + require.NoError(t, err, "annotation key") + require.Equal(t, tc.key, key, "annotation key") + } else { + require.Error(t, err) + } + }) + } +} + +func TestUpdateAnnotations(t *testing.T) { + type inject = struct { + plugin string + devID string + devices []string + } + type testCase = struct { + name string + existing map[string]string + injections []*inject + annotations map[string]string + parsed []string + invalid bool + } + + for _, tc := range []*testCase{ + { + name: "one plugin, one device", + injections: []*inject{ + { + plugin: "vendor.class", + devID: "device", + devices: []string{ + "vendor.com/class=device", + }, + }, + }, + annotations: map[string]string{ + AnnotationPrefix + "vendor.class_device": "vendor.com/class=device", + }, + parsed: []string{ + "vendor.com/class=device", + }, + }, + { + name: "one plugin, multiple devices", + injections: []*inject{ + { + plugin: "vendor.class", + devID: "device", + devices: []string{ + "vendor.com/class=device1", + "vendor.com/class=device2", + "vendor.com/class=device3", + }, + }, + }, + annotations: map[string]string{ + AnnotationPrefix + "vendor.class_device": "vendor.com/class=device1,vendor.com/class=device2,vendor.com/class=device3", + }, + parsed: []string{ + "vendor.com/class=device1", + "vendor.com/class=device2", + "vendor.com/class=device3", + }, + }, + { + name: "multiple plugins, multiple devices", + injections: []*inject{ + { + plugin: "vendor1.class", + devID: "device1", + devices: []string{ + "vendor1.com/class=device1", + }, + }, + { + plugin: "vendor1.class", + devID: "device2", + devices: []string{ + "vendor2.com/class=device1", + "vendor2.com/class=device2", + }, + }, + { + plugin: "vendor3.class2", + devID: "device", + devices: []string{ + "vendor3.com/class2=device1", + "vendor3.com/class2=device2", + "vendor3.com/class2=device3", + }, + }, + }, + annotations: map[string]string{ + AnnotationPrefix + "vendor1.class_device1": "vendor1.com/class=device1", + AnnotationPrefix + "vendor1.class_device2": "vendor2.com/class=device1,vendor2.com/class=device2", + AnnotationPrefix + "vendor3.class2_device": "vendor3.com/class2=device1,vendor3.com/class2=device2,vendor3.com/class2=device3", + }, + parsed: []string{ + "vendor1.com/class=device1", + "vendor2.com/class=device1", + "vendor2.com/class=device2", + "vendor3.com/class2=device1", + "vendor3.com/class2=device2", + "vendor3.com/class2=device3", + }, + }, + { + name: "invalid, empty plugin", + injections: []*inject{ + { + plugin: "vendor1.class", + devID: "device", + devices: []string{ + "vendor1.com/class=device1", + }, + }, + { + plugin: "vendor2.class", + devID: "device", + devices: []string{ + "vendor2.com/class=device1", + "vendor2.com/class=device2", + }, + }, + { + plugin: "", + devID: "device", + devices: []string{ + "vendor3.com/class2=device1", + "vendor3.com/class2=device2", + "vendor3.com/class2=device3", + }, + }, + }, + invalid: true, + }, + { + name: "invalid, malformed device reference", + injections: []*inject{ + { + plugin: "vendor1.class", + devID: "device", + devices: []string{ + "vendor1.com/class=device1", + }, + }, + { + plugin: "vendor2.class", + devID: "device", + devices: []string{ + "vendor2.com/class=device1", + "vendor2.com/device2", + }, + }, + { + plugin: "vendor3.class2", + devID: "device", + devices: []string{ + "vendor3.com/class2=device1", + "vendor3.com/class2=device2", + "vendor3.com/class2=device3", + }, + }, + }, + invalid: true, + }, + { + name: "invalid, pre-resolved device", + injections: []*inject{ + { + plugin: "vendor1.class", + devID: "device", + devices: []string{ + "vendor1.com/class=device1", + }, + }, + { + plugin: "vendor2.class", + devID: "device", + devices: []string{ + "vendor2.com/class=device1", + "vendor2.com/class=device2", + }, + }, + { + plugin: "vendor3.class2", + devID: "device", + devices: []string{ + "vendor3.com/class2=device1", + "vendor3.com/class2=device2", + "/dev/null", + }, + }, + }, + invalid: true, + }, + { + name: "invalid, conflicting keys", + existing: map[string]string{ + AnnotationPrefix + "vendor3.class2_device": "vendor3.com/class2=device0", + }, + injections: []*inject{ + { + plugin: "vendor1.class", + devID: "device", + devices: []string{ + "vendor1.com/class=device1", + }, + }, + { + plugin: "vendor2.class", + devID: "device", + devices: []string{ + "vendor2.com/class=device1", + "vendor2.com/class=device2", + }, + }, + { + plugin: "vendor3.class2", + devID: "device", + devices: []string{ + "vendor3.com/class2=device1", + "vendor3.com/class2=device2", + }, + }, + }, + invalid: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + var ( + annotations map[string]string + err error + ) + for _, i := range tc.injections { + if tc.existing != nil { + annotations = tc.existing + } + annotations, err = UpdateAnnotations(annotations, i.plugin, i.devID, i.devices) + if !tc.invalid { + require.NoError(t, err, "CDI device injection annotation") + } else { + if err != nil { + break + } + } + } + if tc.invalid { + require.Error(t, err, "invalid injection") + } else { + require.Equal(t, tc.annotations, annotations) + } + }) + } +} diff --git a/pkg/cdi/annotations.go b/pkg/cdi/annotations.go index a596c610..29386ed1 100644 --- a/pkg/cdi/annotations.go +++ b/pkg/cdi/annotations.go @@ -17,41 +17,26 @@ package cdi import ( - "errors" "fmt" "strings" + "tags.cncf.io/container-device-interface/api/producer" "tags.cncf.io/container-device-interface/pkg/parser" ) const ( // AnnotationPrefix is the prefix for CDI container annotation keys. - AnnotationPrefix = "cdi.k8s.io/" + AnnotationPrefix = producer.AnnotationPrefix ) // UpdateAnnotations updates annotations with a plugin-specific CDI device // injection request for the given devices. Upon any error a non-nil error // is returned and annotations are left intact. By convention plugin should // be in the format of "vendor.device-type". +// +// Deprecated: use producer.UpdateAnnotations directly. func UpdateAnnotations(annotations map[string]string, plugin string, deviceID string, devices []string) (map[string]string, error) { - key, err := AnnotationKey(plugin, deviceID) - if err != nil { - return annotations, fmt.Errorf("CDI annotation failed: %w", err) - } - if _, ok := annotations[key]; ok { - return annotations, fmt.Errorf("CDI annotation failed, key %q used", key) - } - value, err := AnnotationValue(devices) - if err != nil { - return annotations, fmt.Errorf("CDI annotation failed: %w", err) - } - - if annotations == nil { - annotations = make(map[string]string) - } - annotations[key] = value - - return annotations, nil + return producer.UpdateAnnotations(annotations, plugin, deviceID, devices) } // ParseAnnotations parses annotations for CDI device injection requests. @@ -87,55 +72,15 @@ func ParseAnnotations(annotations map[string]string) ([]string, []string, error) // "vendor.device-type". deviceID is the ID of the device the plugin is // allocating. It is used to make sure that the generated key is unique // even if multiple allocations by a single plugin needs to be annotated. +// +// Deprecated: use producer.AnnotationKey directly. func AnnotationKey(pluginName, deviceID string) (string, error) { - const maxNameLen = 63 - - if pluginName == "" { - return "", errors.New("invalid plugin name, empty") - } - if deviceID == "" { - return "", errors.New("invalid deviceID, empty") - } - - name := pluginName + "_" + strings.ReplaceAll(deviceID, "/", "_") - - if len(name) > maxNameLen { - return "", fmt.Errorf("invalid plugin+deviceID %q, too long", name) - } - - if c := rune(name[0]); !parser.IsAlphaNumeric(c) { - return "", fmt.Errorf("invalid name %q, first '%c' should be alphanumeric", - name, c) - } - if len(name) > 2 { - for _, c := range name[1 : len(name)-1] { - switch { - case parser.IsAlphaNumeric(c): - case c == '_' || c == '-' || c == '.': - default: - return "", fmt.Errorf("invalid name %q, invalid character '%c'", - name, c) - } - } - } - if c := rune(name[len(name)-1]); !parser.IsAlphaNumeric(c) { - return "", fmt.Errorf("invalid name %q, last '%c' should be alphanumeric", - name, c) - } - - return AnnotationPrefix + name, nil + return producer.AnnotationKey(pluginName, deviceID) } // AnnotationValue returns an annotation value for the given devices. +// +// Deprecated: use producer.AnnotationValue directly. func AnnotationValue(devices []string) (string, error) { - value, sep := "", "" - for _, d := range devices { - if _, _, _, err := parser.ParseQualifiedName(d); err != nil { - return "", err - } - value += sep + d - sep = "," - } - - return value, nil + return producer.AnnotationValue(devices) } From 09b62552598fdc3e74eabec3c00a32c57bfb37dc Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Tue, 4 Feb 2025 23:32:08 +0100 Subject: [PATCH 10/10] Move spec name generation to producer Signed-off-by: Evan Lezar --- api/producer/spec-names.go | 72 ++++++++++++++++++++++++++++++++++++++ pkg/cdi/spec.go | 28 +++++++-------- 2 files changed, 84 insertions(+), 16 deletions(-) create mode 100644 api/producer/spec-names.go diff --git a/api/producer/spec-names.go b/api/producer/spec-names.go new file mode 100644 index 00000000..9190d97a --- /dev/null +++ b/api/producer/spec-names.go @@ -0,0 +1,72 @@ +package producer + +import ( + "fmt" + "strings" + + cdi "tags.cncf.io/container-device-interface/specs-go" +) + +// GenerateSpecName generates a vendor+class scoped Spec file name. The +// name can be passed to WriteSpec() to write a Spec file to the file +// system. +// +// vendor and class should match the vendor and class of the CDI Spec. +// The file name is generated without a ".json" or ".yaml" extension. +// The caller can append the desired extension to choose a particular +// encoding. Otherwise WriteSpec() will use its default encoding. +// +// This function always returns the same name for the same vendor/class +// combination. Therefore it cannot be used as such to generate multiple +// Spec file names for a single vendor and class. +func GenerateSpecName(vendor, class string) string { + return vendor + "-" + class +} + +// GenerateTransientSpecName generates a vendor+class scoped transient +// Spec file name. The name can be passed to WriteSpec() to write a Spec +// file to the file system. +// +// Transient Specs are those whose lifecycle is tied to that of some +// external entity, for instance a container. vendor and class should +// match the vendor and class of the CDI Spec. transientID should be +// unique among all CDI users on the same host that might generate +// transient Spec files using the same vendor/class combination. If +// the external entity to which the lifecycle of the transient Spec +// is tied to has a unique ID of its own, then this is usually a +// good choice for transientID. +// +// The file name is generated without a ".json" or ".yaml" extension. +// The caller can append the desired extension to choose a particular +// encoding. Otherwise WriteSpec() will use its default encoding. +func GenerateTransientSpecName(vendor, class, transientID string) string { + transientID = strings.TrimSpace(strings.ReplaceAll(transientID, "/", "_")) + base := GenerateSpecName(vendor, class) + if transientID == "" { + return base + } + return base + "_" + transientID +} + +// GenerateNameForSpec generates a name for the given Spec using +// GenerateSpecName with the vendor and class taken from the Spec. +// On success it returns the generated name and a nil error. If +// the Spec does not contain a valid vendor or class, it returns +// an empty name and a non-nil error. +func GenerateNameForSpec(raw *cdi.Spec) (string, error) { + return GenerateNameForTransientSpec(raw, "") +} + +// GenerateNameForTransientSpec generates a name for the given transient +// Spec using GenerateTransientSpecName with the vendor and class taken +// from the Spec. On success it returns the generated name and a nil error. +// If the Spec does not contain a valid vendor or class, it returns an +// an empty name and a non-nil error. +func GenerateNameForTransientSpec(raw *cdi.Spec, transientID string) (string, error) { + vendor, class := ParseKind(raw.Kind) + if vendor == "" { + return "", fmt.Errorf("invalid vendor/class %q in Spec", raw.Kind) + } + + return GenerateTransientSpecName(vendor, class, transientID), nil +} diff --git a/pkg/cdi/spec.go b/pkg/cdi/spec.go index 5ef8f5b1..f0668134 100644 --- a/pkg/cdi/spec.go +++ b/pkg/cdi/spec.go @@ -20,7 +20,6 @@ import ( "fmt" "os" "path/filepath" - "strings" "sync" oci "github.com/opencontainers/runtime-spec/specs-go" @@ -230,8 +229,10 @@ func validateSpec(raw *cdi.Spec) error { // This function always returns the same name for the same vendor/class // combination. Therefore it cannot be used as such to generate multiple // Spec file names for a single vendor and class. +// +// Deprecated: Use producer.GenerateSpecName instead func GenerateSpecName(vendor, class string) string { - return vendor + "-" + class + return producer.GenerateSpecName(vendor, class) } // GenerateTransientSpecName generates a vendor+class scoped transient @@ -250,9 +251,10 @@ func GenerateSpecName(vendor, class string) string { // The file name is generated without a ".json" or ".yaml" extension. // The caller can append the desired extension to choose a particular // encoding. Otherwise WriteSpec() will use its default encoding. +// +// Deprecated: Use producer.GenerateTransientSpecName instead func GenerateTransientSpecName(vendor, class, transientID string) string { - transientID = strings.ReplaceAll(transientID, "/", "_") - return GenerateSpecName(vendor, class) + "_" + transientID + return producer.GenerateTransientSpecName(vendor, class, transientID) } // GenerateNameForSpec generates a name for the given Spec using @@ -260,13 +262,10 @@ func GenerateTransientSpecName(vendor, class, transientID string) string { // On success it returns the generated name and a nil error. If // the Spec does not contain a valid vendor or class, it returns // an empty name and a non-nil error. +// +// Deprecated: Use producer.GenerateNameForSpec instead func GenerateNameForSpec(raw *cdi.Spec) (string, error) { - vendor, class := parser.ParseQualifier(raw.Kind) - if vendor == "" { - return "", fmt.Errorf("invalid vendor/class %q in Spec", raw.Kind) - } - - return GenerateSpecName(vendor, class), nil + return producer.GenerateNameForSpec(raw) } // GenerateNameForTransientSpec generates a name for the given transient @@ -274,11 +273,8 @@ func GenerateNameForSpec(raw *cdi.Spec) (string, error) { // from the Spec. On success it returns the generated name and a nil error. // If the Spec does not contain a valid vendor or class, it returns an // an empty name and a non-nil error. +// +// Deprecated: Use producer.GenerateNameForTransientSpec instead func GenerateNameForTransientSpec(raw *cdi.Spec, transientID string) (string, error) { - vendor, class := parser.ParseQualifier(raw.Kind) - if vendor == "" { - return "", fmt.Errorf("invalid vendor/class %q in Spec", raw.Kind) - } - - return GenerateTransientSpecName(vendor, class, transientID), nil + return producer.GenerateNameForTransientSpec(raw, transientID) }