diff --git a/pkg/cdi/container-edits_test.go b/pkg/cdi/container-edits_test.go index 5c20dbee..41c9077a 100644 --- a/pkg/cdi/container-edits_test.go +++ b/pkg/cdi/container-edits_test.go @@ -721,6 +721,24 @@ func TestApplyContainerEdits(t *testing.T) { }, }, }, + { + name: "empty spec, legacy rdt monitoring", + spec: &oci.Spec{}, + edits: &cdi.ContainerEdits{ + IntelRdt: &cdi.IntelRdt{ + ClosID: "clos-1", + EnableCMT: true, + }, + }, + result: &oci.Spec{ + Linux: &oci.Linux{ + IntelRdt: &oci.LinuxIntelRdt{ + ClosID: "clos-1", + EnableMonitoring: true, + }, + }, + }, + }, { name: "non-empty spec, overriding rdt", spec: &oci.Spec{ diff --git a/pkg/cdi/oci.go b/pkg/cdi/oci.go index 68ffafe9..cbcffdad 100644 --- a/pkg/cdi/oci.go +++ b/pkg/cdi/oci.go @@ -92,7 +92,7 @@ func (i *IntelRdt) toOCI() *spec.LinuxIntelRdt { L3CacheSchema: i.L3CacheSchema, MemBwSchema: i.MemBwSchema, Schemata: i.Schemata, - EnableMonitoring: i.EnableMonitoring, + EnableMonitoring: i.EnableMonitoring || i.EnableCMT || i.EnableMBM, } } diff --git a/pkg/cdi/spec.go b/pkg/cdi/spec.go index fdaa2684..1e3d1970 100644 --- a/pkg/cdi/spec.go +++ b/pkg/cdi/spec.go @@ -250,6 +250,14 @@ func (s *Spec) validate() (map[string]*Device, error) { // ParseSpec parses CDI Spec data into a raw CDI Spec. func ParseSpec(data []byte) (*cdi.Spec, error) { + var contents map[string]interface{} + if err := yaml.UnmarshalStrict(data, &contents); err != nil { + return nil, fmt.Errorf("failed to unmarshal CDI Spec: %w", err) + } + if err := cdi.ValidateVersionContents(contents); err != nil { + return nil, fmt.Errorf("failed to validate CDI Spec version: %w", err) + } + var raw *cdi.Spec err := yaml.UnmarshalStrict(data, &raw) if err != nil { diff --git a/pkg/cdi/spec_test.go b/pkg/cdi/spec_test.go index a1a05ed8..993b9e5a 100644 --- a/pkg/cdi/spec_test.go +++ b/pkg/cdi/spec_test.go @@ -256,6 +256,79 @@ devices: } } +func TestParseSpecIntelRdtVersionCompatibility(t *testing.T) { + baseSpec := ` +cdiVersion: "%s" +kind: vendor.com/device +containerEdits: + intelRdt: +%s +devices: + - name: "dev1" + containerEdits: + deviceNodes: + - path: "/dev/null" +` + + for _, tc := range []struct { + name string + version string + intelRdt string + shouldParse bool + shouldValidate bool + enableCMT bool + }{ + { + name: "legacy CMT is accepted before v1.1.0", + version: "1.0.0", + intelRdt: " enableCMT: true", + shouldParse: true, + shouldValidate: true, + enableCMT: true, + }, + { + name: "legacy CMT is rejected for v1.1.0", + version: "1.1.0", + intelRdt: " enableCMT: true", + shouldParse: false, + shouldValidate: false, + }, + { + name: "new monitoring field is rejected before v1.1.0", + version: "1.0.0", + intelRdt: " enableMonitoring: false", + shouldParse: false, + shouldValidate: false, + }, + { + name: "unknown RDT fields are still rejected", + version: "1.0.0", + intelRdt: " unknown: true", + shouldParse: false, + shouldValidate: false, + }, + } { + t.Run(tc.name, func(t *testing.T) { + raw, err := ParseSpec([]byte(fmt.Sprintf(baseSpec, tc.version, tc.intelRdt))) + if !tc.shouldParse { + require.Error(t, err) + return + } + require.NoError(t, err) + require.Equal(t, tc.enableCMT, raw.ContainerEdits.IntelRdt.EnableCMT) + + spec, err := newSpec(raw, tc.name, 0) + if !tc.shouldValidate { + require.Error(t, err) + require.Nil(t, spec) + return + } + require.NoError(t, err) + require.NotNil(t, spec) + }) + } +} + func TestWriteSpec(t *testing.T) { type testCase struct { name string diff --git a/schema/schema.go b/schema/schema.go index a6b00305..6dd6f6d9 100644 --- a/schema/schema.go +++ b/schema/schema.go @@ -167,12 +167,11 @@ func Load(source string) (*Schema, error) { // ReadAndValidate all data from the given reader, using the schema for validation. func (s *Schema) ReadAndValidate(r io.Reader) ([]byte, error) { - loader, reader := schema.NewReaderLoader(r) - data, err := io.ReadAll(reader) + data, err := io.ReadAll(r) if err != nil { return nil, fmt.Errorf("failed to read data for validation: %w", err) } - return data, s.validate(loader) + return data, s.ValidateData(data) } // ValidateReader validates the data read from an io.Reader against the schema. @@ -197,6 +196,11 @@ func (s *Schema) ValidateData(data []byte) error { if err != nil { return fmt.Errorf("failed to JSON remarshal data for validation: %w", err) } + } else { + err = json.Unmarshal(data, &any) + if err != nil { + return fmt.Errorf("failed to JSON unmarshal data for validation: %w", err) + } } if err := s.validate(schema.NewBytesLoader(data)); err != nil { @@ -208,10 +212,6 @@ func (s *Schema) ValidateData(data []byte) error { // ValidateFile validates the given JSON file against the schema. func (s *Schema) ValidateFile(path string) error { - if filepath.Ext(path) == ".json" { - return s.validate(schema.NewReferenceLoader("file://" + path)) - } - data, err := os.ReadFile(path) if err != nil { return err @@ -308,12 +308,16 @@ func (c schemaContents) getDevices() ([]schemaContents, error) { // validateContents performs additional validation against the schema contents. func (s *Schema) validateContents(any map[string]interface{}) error { - if any == nil || s == nil { + if any == nil || s == nil || s.schema == nil { return nil } contents := schemaContents(any) + if err := cdi.ValidateVersionContents(any); err != nil { + return err + } + if specAnnotations, ok := contents.getAnnotations(); ok { if err := validation.ValidateSpecAnnotations("", specAnnotations); err != nil { return err diff --git a/schema/schema_test.go b/schema/schema_test.go index 6198a79d..f4557a08 100644 --- a/schema/schema_test.go +++ b/schema/schema_test.go @@ -131,6 +131,88 @@ func TestValidateData(t *testing.T) { } } +func TestValidateDataIntelRdtVersionCompatibility(t *testing.T) { + scm := loadSchema(t, schema.BuiltinSchemaName) + + for _, tc := range []struct { + name string + data string + wantErr bool + }{ + { + name: "legacy field is valid before v1.1.0", + data: ` +cdiVersion: "1.0.0" +kind: "vendor.com/device" +containerEdits: + intelRdt: + enableCMT: true +devices: + - name: "gpu0" + containerEdits: + deviceNodes: + - path: "/dev/null" +`, + }, + { + name: "legacy field is invalid for v1.1.0 even when false", + data: ` +cdiVersion: "1.1.0" +kind: "vendor.com/device" +containerEdits: + intelRdt: + enableMBM: false +devices: + - name: "gpu0" + containerEdits: + deviceNodes: + - path: "/dev/null" +`, + wantErr: true, + }, + { + name: "new monitoring field is invalid before v1.1.0 even when false", + data: ` +cdiVersion: "1.0.0" +kind: "vendor.com/device" +containerEdits: + intelRdt: + enableMonitoring: false +devices: + - name: "gpu0" + containerEdits: + deviceNodes: + - path: "/dev/null" +`, + wantErr: true, + }, + { + name: "device scoped new RDT field is invalid before v1.1.0", + data: ` +cdiVersion: "1.0.0" +kind: "vendor.com/device" +devices: + - name: "gpu0" + containerEdits: + intelRdt: + schemata: [] + deviceNodes: + - path: "/dev/null" +`, + wantErr: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + err := scm.ValidateData([]byte(tc.data)) + if tc.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + }) + } +} + func TestValidateReader(t *testing.T) { type testCase struct { testName string diff --git a/specs-go/config.go b/specs-go/config.go index ccda9d86..100edcb7 100644 --- a/specs-go/config.go +++ b/specs-go/config.go @@ -68,6 +68,8 @@ type IntelRdt struct { ClosID string `json:"closID,omitempty" yaml:"closID,omitempty"` L3CacheSchema string `json:"l3CacheSchema,omitempty" yaml:"l3CacheSchema,omitempty"` MemBwSchema string `json:"memBwSchema,omitempty" yaml:"memBwSchema,omitempty"` + EnableCMT bool `json:"enableCMT,omitempty" yaml:"enableCMT,omitempty"` // Added in v0.7.0. Removed in v1.1.0. + EnableMBM bool `json:"enableMBM,omitempty" yaml:"enableMBM,omitempty"` // Added in v0.7.0. Removed in v1.1.0. Schemata []string `json:"schemata,omitempty" yaml:"schemata,omitempty"` // Added in v1.1.0. EnableMonitoring bool `json:"enableMonitoring,omitempty" yaml:"enableMonitoring,omitempty"` // Added in v1.1.0. } diff --git a/specs-go/version.go b/specs-go/version.go index c3f5a8d1..1ff05508 100644 --- a/specs-go/version.go +++ b/specs-go/version.go @@ -70,6 +70,9 @@ func ValidateVersion(spec *Spec) error { if !validSpecVersions.isValidVersion(spec.Version) { return fmt.Errorf("invalid version %q", spec.Version) } + if err := validateVersionedSpec(spec); err != nil { + return err + } minVersion, err := MinimumRequiredVersion(spec) if err != nil { return fmt.Errorf("could not determine minimum required version: %w", err) @@ -80,6 +83,36 @@ func ValidateVersion(spec *Spec) error { return nil } +// ValidateVersionContents checks versioned fields in raw CDI Spec contents. +// This complements ValidateVersion for fields whose mere presence matters but +// whose zero value is indistinguishable from omission after unmarshalling. +// Invalid or missing cdiVersion values are left for ValidateVersion to report. +func ValidateVersionContents(contents map[string]interface{}) error { + specVersion, ok := getString(contents, "cdiVersion") + if !ok { + return nil + } + if !validSpecVersions.isValidVersion(specVersion) { + return nil + } + + version := newVersion(specVersion) + if err := validateVersionedContainerEditsContents(version, "containerEdits", getMap(contents, "containerEdits")); err != nil { + return err + } + + devices, _ := contents["devices"].([]interface{}) + for idx, device := range devices { + deviceContents, _ := device.(map[string]interface{}) + path := fmt.Sprintf("devices[%d].containerEdits", idx) + if err := validateVersionedContainerEditsContents(version, path, getMap(deviceContents, "containerEdits")); err != nil { + return err + } + } + + return nil +} + // MinimumRequiredVersion determines the minimum spec version for the input spec. func MinimumRequiredVersion(spec *Spec) (string, error) { minVersion := validSpecVersions.requiredVersion(spec) @@ -110,6 +143,93 @@ func (v version) isLatest() bool { return v == vCurrent } +func getString(contents map[string]interface{}, field string) (string, bool) { + if contents == nil { + return "", false + } + value, ok := contents[field] + if !ok { + return "", false + } + valueString, ok := value.(string) + return valueString, ok +} + +func getMap(contents map[string]interface{}, field string) map[string]interface{} { + if contents == nil { + return nil + } + value, ok := contents[field] + if !ok { + return nil + } + valueMap, _ := value.(map[string]interface{}) + return valueMap +} + +func validateVersionedContainerEditsContents(specVersion version, path string, edits map[string]interface{}) error { + if edits != nil { + if _, ok := edits["netDevices"]; ok && v110.isGreaterThan(specVersion) { + return fmt.Errorf("%s.netDevices requires CDI spec version at least %s", path, v110) + } + if err := validateVersionedIntelRdtContents(specVersion, path+".intelRdt", getMap(edits, "intelRdt")); err != nil { + return err + } + } + + return nil +} + +func validateVersionedIntelRdtContents(specVersion version, path string, intelRdt map[string]interface{}) error { + if intelRdt == nil { + return nil + } + + for _, field := range []string{"enableCMT", "enableMBM"} { + if _, ok := intelRdt[field]; ok && !v110.isGreaterThan(specVersion) { + return fmt.Errorf("%s.%s is not valid for CDI spec version %s", path, field, specVersion) + } + } + for _, field := range []string{"schemata", "enableMonitoring"} { + if _, ok := intelRdt[field]; ok && v110.isGreaterThan(specVersion) { + return fmt.Errorf("%s.%s requires CDI spec version at least %s", path, field, v110) + } + } + + return nil +} + +func validateVersionedSpec(spec *Spec) error { + specVersion := newVersion(spec.Version) + if err := validateVersionedIntelRdt(specVersion, "containerEdits.intelRdt", spec.ContainerEdits.IntelRdt); err != nil { + return err + } + + for idx, dev := range spec.Devices { + path := fmt.Sprintf("devices[%d].containerEdits.intelRdt", idx) + if err := validateVersionedIntelRdt(specVersion, path, dev.ContainerEdits.IntelRdt); err != nil { + return err + } + } + + return nil +} + +func validateVersionedIntelRdt(specVersion version, path string, intelRdt *IntelRdt) error { + if intelRdt == nil || v110.isGreaterThan(specVersion) { + return nil + } + + switch { + case intelRdt.EnableCMT: + return fmt.Errorf("%s.enableCMT is not valid for CDI spec version %s", path, specVersion) + case intelRdt.EnableMBM: + return fmt.Errorf("%s.enableMBM is not valid for CDI spec version %s", path, specVersion) + } + + return nil +} + type requiredFunc func(*Spec) bool type requiredVersionMap map[version]requiredFunc