From 397025aa90bb36ccc8b49ad0218d431b1ed863aa Mon Sep 17 00:00:00 2001 From: Axel Christ Date: Thu, 29 Sep 2022 16:30:24 +0200 Subject: [PATCH] Allow specifying volume device --- .../plugin/machinevolumedevices/admission.go | 118 +++++++++++ .../machinevolumedevices/admission_test.go | 119 +++++++++++ .../machinevolumedevices/device/device.go | 188 ++++++++++++++++++ .../device/device_suite_test.go | 27 +++ .../device/device_test.go | 131 ++++++++++++ .../machinevolumedevices_suite_test.go | 27 +++ apis/compute/machine_types.go | 3 + apis/compute/register.go | 4 + apis/compute/v1alpha1/machine_types.go | 3 + .../v1alpha1/zz_generated.conversion.go | 2 + apis/compute/validation/machine.go | 44 +++- apis/compute/validation/machine_test.go | 55 +++++ apis/storage/register.go | 9 +- app/apiserver/apiserver.go | 5 + docs/api-reference/common.md | 8 +- docs/api-reference/compute.md | 12 ++ generated/openapi/zz_generated.openapi.go | 7 + go.mod | 1 + go.sum | 2 + 19 files changed, 751 insertions(+), 14 deletions(-) create mode 100644 admission/plugin/machinevolumedevices/admission.go create mode 100644 admission/plugin/machinevolumedevices/admission_test.go create mode 100644 admission/plugin/machinevolumedevices/device/device.go create mode 100644 admission/plugin/machinevolumedevices/device/device_suite_test.go create mode 100644 admission/plugin/machinevolumedevices/device/device_test.go create mode 100644 admission/plugin/machinevolumedevices/machinevolumedevices_suite_test.go diff --git a/admission/plugin/machinevolumedevices/admission.go b/admission/plugin/machinevolumedevices/admission.go new file mode 100644 index 000000000..e21d8d542 --- /dev/null +++ b/admission/plugin/machinevolumedevices/admission.go @@ -0,0 +1,118 @@ +// Copyright 2022 OnMetal 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 machinevolumedevices + +import ( + "context" + "fmt" + "io" + + "github.com/onmetal/onmetal-api/admission/plugin/machinevolumedevices/device" + "github.com/onmetal/onmetal-api/apis/compute" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apiserver/pkg/admission" +) + +// PluginName indicates name of admission plugin. +const PluginName = "MachineVolumeDevices" + +// Register registers a plugin +func Register(plugins *admission.Plugins) { + plugins.Register(PluginName, func(config io.Reader) (admission.Interface, error) { + return NewMachineVolumeDevices(), nil + }) +} + +type MachineVolumeDevices struct { + *admission.Handler +} + +func NewMachineVolumeDevices() *MachineVolumeDevices { + return &MachineVolumeDevices{ + Handler: admission.NewHandler(admission.Create, admission.Update), + } +} + +func (d *MachineVolumeDevices) Admit(ctx context.Context, a admission.Attributes, o admission.ObjectInterfaces) error { + if shouldIgnore(a) { + return nil + } + + machine, ok := a.GetObject().(*compute.Machine) + if !ok { + return apierrors.NewBadRequest("Resource was marked with kind Machine but was unable to be converted") + } + + namer, err := deviceNamerFromMachineVolumes(machine) + if err != nil { + return apierrors.NewBadRequest("Machine has conflicting volume device names") + } + + for i := range machine.Spec.Volumes { + volume := &machine.Spec.Volumes[i] + if volume.Device != "" { + continue + } + + dev, err := namer.Generate(device.VirtioPrefix) // TODO: We should have a better way for a device prefix. + if err != nil { + return apierrors.NewBadRequest("No device names left for machine") + } + + volume.Device = dev + } + + return nil +} + +func shouldIgnore(a admission.Attributes) bool { + if a.GetKind().GroupKind() != compute.Kind("Machine") { + return true + } + + machine, ok := a.GetObject().(*compute.Machine) + if !ok { + return true + } + + return !machineHasAnyVolumeWithoutDevice(machine) +} + +func machineHasAnyVolumeWithoutDevice(machine *compute.Machine) bool { + for _, volume := range machine.Spec.Volumes { + if volume.Device == "" { + return true + } + } + return false +} + +func deviceNamerFromMachineVolumes(machine *compute.Machine) (*device.Namer, error) { + namer := device.NewNamer() + + // Observe reserved names. + if err := namer.Observe(device.Name(device.VirtioPrefix, 0)); err != nil { + return nil, err + } + + for _, volume := range machine.Spec.Volumes { + if dev := volume.Device; dev != "" { + if err := namer.Observe(dev); err != nil { + return nil, fmt.Errorf("error observing device %s: %w", dev, err) + } + } + } + return namer, nil +} diff --git a/admission/plugin/machinevolumedevices/admission_test.go b/admission/plugin/machinevolumedevices/admission_test.go new file mode 100644 index 000000000..60a01b7c1 --- /dev/null +++ b/admission/plugin/machinevolumedevices/admission_test.go @@ -0,0 +1,119 @@ +// Copyright 2022 OnMetal 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 machinevolumedevices_test + +import ( + "context" + + . "github.com/onmetal/onmetal-api/admission/plugin/machinevolumedevices" + "github.com/onmetal/onmetal-api/apis/compute" + "github.com/onmetal/onmetal-api/apis/storage" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apiserver/pkg/admission" +) + +var _ = Describe("Admission", func() { + var ( + plugin *MachineVolumeDevices + ) + BeforeEach(func() { + plugin = NewMachineVolumeDevices() + }) + + It("should ignore non-machine objects", func() { + volume := &storage.Volume{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "foo", + Name: "bar", + }, + } + origVolume := volume.DeepCopy() + Expect(plugin.Admit( + context.TODO(), + admission.NewAttributesRecord( + volume, + nil, + storage.Kind("Volume").WithVersion("version"), + volume.Namespace, + volume.Name, + storage.Resource("volumes").WithVersion("version"), + "", + admission.Create, + &metav1.CreateOptions{}, + false, + nil, + ), + nil, + )).NotTo(HaveOccurred()) + Expect(volume).To(Equal(origVolume)) + }) + + It("should add volume device names when unset", func() { + machine := &compute.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "foo", + Name: "bar", + }, + Spec: compute.MachineSpec{ + Volumes: []compute.Volume{ + { + Name: "foo", + Device: "vdb", + }, + { + Name: "bar", + }, + { + Name: "baz", + }, + }, + }, + } + Expect(plugin.Admit( + context.TODO(), + admission.NewAttributesRecord( + machine, + nil, + compute.Kind("Machine").WithVersion("version"), + machine.Namespace, + machine.Name, + compute.Resource("machines").WithVersion("version"), + "", + admission.Create, + &metav1.CreateOptions{}, + false, + nil, + ), + nil, + )).NotTo(HaveOccurred()) + + Expect(machine.Spec.Volumes).To(Equal([]compute.Volume{ + { + Name: "foo", + Device: "vdb", + }, + { + Name: "bar", + Device: "vdc", + }, + { + Name: "baz", + Device: "vdd", + }, + })) + }) +}) diff --git a/admission/plugin/machinevolumedevices/device/device.go b/admission/plugin/machinevolumedevices/device/device.go new file mode 100644 index 000000000..64e908aaa --- /dev/null +++ b/admission/plugin/machinevolumedevices/device/device.go @@ -0,0 +1,188 @@ +// Copyright 2022 OnMetal 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 device + +import ( + "fmt" + "regexp" + + "github.com/bits-and-blooms/bitset" +) + +const ( + letters = "abcdefghijklmnopqrstuvwxyz" + numLetters = len(letters) + + // MaxIndex is the maximum index usable for Name / returned by ParseName. + MaxIndex = numLetters*numLetters + numLetters - 1 + + // VirtioPrefix is the device prefix used by virtio devices. + VirtioPrefix = "vd" +) + +var ( + prefixRegex = regexp.MustCompile("^[a-z]{2}$") + nameRegex = regexp.MustCompile("^(?P[a-z]{2})(?P[a-z][a-z]?)$") +) + +// Name creates a device name with the given prefix and index. +// If idx is greater than MaxIndex or if the prefix is not a valid prefix (two lowercase letters) it panics. +func Name(prefix string, idx int) string { + if !prefixRegex.MatchString(prefix) { + panic(fmt.Sprintf("invalid device prefix %s - must match regex %s", prefix, prefixRegex)) + } + + if idx > MaxIndex { + panic(fmt.Sprintf("device index too large %d", idx)) + } + if idx < 0 { + panic(fmt.Sprintf("negative device index %d", idx)) + } + + if idx < numLetters { + return prefix + string(letters[idx]) + } + + rmd := idx % numLetters + idx = (idx / numLetters) - 1 + return prefix + string(letters[idx]) + string(letters[rmd]) +} + +// ParseName parses the name into its prefix and index. An error is returned if the name is not a valid device name. +func ParseName(name string) (string, int, error) { + match := nameRegex.FindStringSubmatch(name) + if match == nil { + return "", 0, fmt.Errorf("%s does not match device name regex %s", name, nameRegex) + } + + prefix := match[1] + + idxStr := match[2] + if len(idxStr) == 1 { + idx := int(idxStr[0] - 'a') + return prefix, idx, nil + } + + r1, r2 := int(idxStr[0]-'a'), int(idxStr[1]-'a') + idx := (r1+1)*numLetters + r2 + + return prefix, idx, nil +} + +type taken struct { + count uint + set *bitset.BitSet +} + +func newTaken(bits uint) *taken { + return &taken{set: bitset.New(bits)} +} + +func (t *taken) claim(i uint) bool { + if t.set.Test(i) { + return false + } + + t.count++ + t.set.Set(i) + return true +} + +func (t *taken) release(i uint) bool { + if !t.set.Test(i) { + return false + } + + t.count-- + t.set.Clear(i) + return true +} + +func (t *taken) pop() (uint, bool) { + if t.count == t.set.Len() { + return 0, false + } + + // We can safely assume that there is a next clear. + idx, _ := t.set.NextClear(0) + + t.count++ + t.set.Set(idx) + return idx, true +} + +// Namer allows managing multiple device names. It remembers reserved ones and allows claiming / releasing new ones. +type Namer struct { + takenByPrefix map[string]*taken +} + +// NewNamer creates a new Namer that allows managing multiple device names. +func NewNamer() *Namer { + return &Namer{takenByPrefix: make(map[string]*taken)} +} + +// Observe marks the given name as already claimed. If it already has been claimed, an error is returned. +func (n *Namer) Observe(name string) error { + prefix, idx, err := ParseName(name) + if err != nil { + return err + } + + t := n.takenByPrefix[prefix] + if t == nil { + t = newTaken(uint(MaxIndex)) + n.takenByPrefix[prefix] = t + } + + if !t.claim(uint(idx)) { + return fmt.Errorf("index %d is already occupied", idx) + } + return nil +} + +// Generate generates and claims a new free device name. An error is returned if no free device names are available. +func (n *Namer) Generate(prefix string) (string, error) { + t := n.takenByPrefix[prefix] + if t == nil { + t = newTaken(uint(MaxIndex)) + n.takenByPrefix[prefix] = t + } + + idx, ok := t.pop() + if !ok { + return "", fmt.Errorf("no free device names available") + } + + return Name(prefix, int(idx)), nil +} + +// Free releases the given name. If it has not been claimed before, an error is returned. +func (n *Namer) Free(name string) error { + prefix, idx, err := ParseName(name) + if err != nil { + return err + } + + t := n.takenByPrefix[prefix] + if t == nil { + t = newTaken(uint(MaxIndex)) + n.takenByPrefix[prefix] = t + } + + if !t.release(uint(idx)) { + return fmt.Errorf("index %d is not claimed", idx) + } + return nil +} diff --git a/admission/plugin/machinevolumedevices/device/device_suite_test.go b/admission/plugin/machinevolumedevices/device/device_suite_test.go new file mode 100644 index 000000000..660d89b0b --- /dev/null +++ b/admission/plugin/machinevolumedevices/device/device_suite_test.go @@ -0,0 +1,27 @@ +// Copyright 2022 OnMetal 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 device_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestDevice(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Device Suite") +} diff --git a/admission/plugin/machinevolumedevices/device/device_test.go b/admission/plugin/machinevolumedevices/device/device_test.go new file mode 100644 index 000000000..fb3654463 --- /dev/null +++ b/admission/plugin/machinevolumedevices/device/device_test.go @@ -0,0 +1,131 @@ +// Copyright 2022 OnMetal 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 device_test + +import ( + "fmt" + + . "github.com/onmetal/onmetal-api/admission/plugin/machinevolumedevices/device" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/onsi/gomega/types" +) + +var _ = Describe("Device", func() { + DescribeTable("ParseName", + func(name string, matchers ...types.GomegaMatcher) { + prefix, idx, err := ParseName(name) + switch len(matchers) { + case 1: + Expect(err).To(matchers[0]) + case 2: + Expect(err).NotTo(HaveOccurred()) + Expect(prefix).To(matchers[0]) + Expect(idx).To(matchers[1]) + default: + Fail(fmt.Sprintf("invalid number of matchers: %d, expected 1 (error case) / 2 (success case)", len(matchers))) + } + }, + Entry("minimum index", "sda", Equal("sd"), Equal(0)), + Entry("other prefix", "fda", Equal("fd"), Equal(0)), + Entry("simple valid name", "sdb", Equal("sd"), Equal(1)), + Entry("two-letter name", "sdbb", Equal("sd"), Equal(53)), + Entry("maximum index", "sdzz", Equal("sd"), Equal(MaxIndex)), + Entry("more than two letters", "sdaaa", HaveOccurred()), + Entry("invalid prefix", "f_aa", HaveOccurred()), + ) + + DescribeTable("Name", + func(prefix string, index int, matchers ...types.GomegaMatcher) { + if len(matchers) == 2 { + Expect(func() { Name(prefix, index) }).To(matchers[1]) + } else { + Expect(Name(prefix, index)).To(matchers[0]) + } + }, + Entry("minimum index", "sd", 0, Equal("sda")), + Entry("other index", "sd", 22, Equal("sdw")), + Entry("maximum index", "sd", MaxIndex, Equal("sdzz")), + Entry("negative index", "sd", -1, nil, Panic()), + Entry("too large index", "sd", MaxIndex+1, nil, Panic()), + Entry("invalid prefix", "f_a", 1, nil, Panic()), + ) + + Context("Namer", func() { + var namer *Namer + BeforeEach(func() { + namer = NewNamer() + }) + + Describe("Observe", func() { + It("should allow claiming names", func() { + By("claiming some names") + Expect(namer.Observe("foo")).To(Succeed()) + Expect(namer.Observe("bar")).To(Succeed()) + Expect(namer.Observe("baz")).To(Succeed()) + + By("claiming a name that already has been claimed") + Expect(namer.Observe("foo")).To(HaveOccurred()) + }) + + It("should error on invalid names", func() { + Expect(namer.Observe("invalid")).To(HaveOccurred()) + }) + }) + + Describe("Free", func() { + It("should allow free names, making them reclaimable", func() { + By("claiming some names") + Expect(namer.Observe("foo")).To(Succeed()) + Expect(namer.Observe("bar")).To(Succeed()) + + By("freeing the names") + Expect(namer.Free("foo")).To(Succeed()) + Expect(namer.Free("bar")).To(Succeed()) + + By("claiming the names again") + Expect(namer.Observe("foo")).To(Succeed()) + Expect(namer.Observe("bar")).To(Succeed()) + + By("freeing a name that has not been claimed") + Expect(namer.Free("qux")).To(HaveOccurred()) + }) + + It("should error on invalid names", func() { + Expect(namer.Free("invalid")).To(HaveOccurred()) + }) + }) + + Describe("Generate", func() { + It("should generate names with the desired prefix", func() { + Expect(namer.Generate("sd")).To(Equal("sda")) + Expect(namer.Generate("sd")).To(Equal("sdb")) + Expect(namer.Generate("vd")).To(Equal("vda")) + }) + + It("should error if all names have been claimed", func() { + By("claiming all available names") + for i := 0; i < MaxIndex; i++ { + _, err := namer.Generate("sd") + Expect(err).NotTo(HaveOccurred()) + } + + By("trying to generate again") + _, err := namer.Generate("sd") + Expect(err).To(HaveOccurred()) + }) + }) + }) +}) diff --git a/admission/plugin/machinevolumedevices/machinevolumedevices_suite_test.go b/admission/plugin/machinevolumedevices/machinevolumedevices_suite_test.go new file mode 100644 index 000000000..4e19212c0 --- /dev/null +++ b/admission/plugin/machinevolumedevices/machinevolumedevices_suite_test.go @@ -0,0 +1,27 @@ +// Copyright 2022 OnMetal 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 machinevolumedevices_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestMachinevolumedevices(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Machinevolumedevices Suite") +} diff --git a/apis/compute/machine_types.go b/apis/compute/machine_types.go index 910442f04..b43c930ed 100644 --- a/apis/compute/machine_types.go +++ b/apis/compute/machine_types.go @@ -84,6 +84,9 @@ type NetworkInterfaceSource struct { type Volume struct { // Name is the name of the Volume Name string + // Device is the device name where the volume should be attached. If empty, + // an unused device name will be determined if possible. + Device string // VolumeSource is the source where the storage for the Volume resides at. VolumeSource } diff --git a/apis/compute/register.go b/apis/compute/register.go index 0392fbabd..80d82830b 100644 --- a/apis/compute/register.go +++ b/apis/compute/register.go @@ -39,6 +39,10 @@ func Resource(resource string) schema.GroupResource { return SchemeGroupVersion.WithResource(resource).GroupResource() } +func Kind(kind string) schema.GroupKind { + return SchemeGroupVersion.WithKind(kind).GroupKind() +} + func addKnownTypes(scheme *runtime.Scheme) error { scheme.AddKnownTypes(SchemeGroupVersion, &Machine{}, diff --git a/apis/compute/v1alpha1/machine_types.go b/apis/compute/v1alpha1/machine_types.go index 9847eb80c..e086d9d9e 100644 --- a/apis/compute/v1alpha1/machine_types.go +++ b/apis/compute/v1alpha1/machine_types.go @@ -84,6 +84,9 @@ type NetworkInterfaceSource struct { type Volume struct { // Name is the name of the Volume Name string `json:"name"` + // Device is the device name where the volume should be attached. If empty, + // an unused device name will be determined if possible. + Device string `json:"device,omitempty"` // VolumeSource is the source where the storage for the Volume resides at. VolumeSource `json:",inline"` } diff --git a/apis/compute/v1alpha1/zz_generated.conversion.go b/apis/compute/v1alpha1/zz_generated.conversion.go index 9def02f69..404482c5f 100644 --- a/apis/compute/v1alpha1/zz_generated.conversion.go +++ b/apis/compute/v1alpha1/zz_generated.conversion.go @@ -908,6 +908,7 @@ func Convert_compute_NetworkInterfaceStatus_To_v1alpha1_NetworkInterfaceStatus(i func autoConvert_v1alpha1_Volume_To_compute_Volume(in *Volume, out *compute.Volume, s conversion.Scope) error { out.Name = in.Name + out.Device = in.Device if err := Convert_v1alpha1_VolumeSource_To_compute_VolumeSource(&in.VolumeSource, &out.VolumeSource, s); err != nil { return err } @@ -921,6 +922,7 @@ func Convert_v1alpha1_Volume_To_compute_Volume(in *Volume, out *compute.Volume, func autoConvert_compute_Volume_To_v1alpha1_Volume(in *compute.Volume, out *Volume, s conversion.Scope) error { out.Name = in.Name + out.Device = in.Device if err := Convert_compute_VolumeSource_To_v1alpha1_VolumeSource(&in.VolumeSource, &out.VolumeSource, s); err != nil { return err } diff --git a/apis/compute/validation/machine.go b/apis/compute/validation/machine.go index 770f10711..c1534a35f 100644 --- a/apis/compute/validation/machine.go +++ b/apis/compute/validation/machine.go @@ -17,6 +17,9 @@ package validation import ( + "fmt" + + "github.com/onmetal/onmetal-api/admission/plugin/machinevolumedevices/device" onmetalapivalidation "github.com/onmetal/onmetal-api/api/validation" "github.com/onmetal/onmetal-api/apis/compute" "github.com/onmetal/onmetal-api/apis/storage" @@ -80,16 +83,19 @@ func validateMachineSpec(machineSpec *compute.MachineSpec, fldPath *field.Path) } seenNames := sets.NewString() + seenDevices := sets.NewString() for i, vol := range machineSpec.Volumes { if seenNames.Has(vol.Name) { allErrs = append(allErrs, field.Duplicate(fldPath.Child("volume").Index(i).Child("name"), vol.Name)) } else { seenNames.Insert(vol.Name) - for _, msg := range apivalidation.NameIsDNSLabel(vol.Name, false) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("volume").Index(i).Child("name"), vol.Name, msg)) - } } - allErrs = append(allErrs, validateVolumeSource(&vol.VolumeSource, fldPath.Child("volume").Index(i))...) + if seenDevices.Has(vol.Device) { + allErrs = append(allErrs, field.Duplicate(fldPath.Child("volume").Index(i).Child("device"), vol.Device)) + } else { + seenDevices.Insert(vol.Device) + } + allErrs = append(allErrs, validateVolume(&vol, fldPath.Child("volume").Index(i))...) } allErrs = append(allErrs, metav1validation.ValidateLabels(machineSpec.MachinePoolSelector, fldPath.Child("machinePoolSelector"))...) @@ -97,6 +103,36 @@ func validateMachineSpec(machineSpec *compute.MachineSpec, fldPath *field.Path) return allErrs } +func isReservedDeviceName(prefix string, idx int) bool { + return idx == 0 +} + +func validateVolume(volume *compute.Volume, fldPath *field.Path) field.ErrorList { + var allErrs field.ErrorList + + for _, msg := range apivalidation.NameIsDNSLabel(volume.Name, false) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("name"), volume.Name, msg)) + } + + if volume.Device == "" { + allErrs = append(allErrs, field.Required(fldPath.Child("device"), "must specify device")) + } else { + // TODO: Improve validation on prefix. + prefix, idx, err := device.ParseName(volume.Device) + if err != nil { + allErrs = append(allErrs, field.Invalid(fldPath.Child("device"), volume.Device, fmt.Sprintf("invalid device name: %v", err))) + } else { + if isReservedDeviceName(prefix, idx) { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("device"), fmt.Sprintf("device name %s is reserved", volume.Device))) + } + } + } + + allErrs = append(allErrs, validateVolumeSource(&volume.VolumeSource, fldPath)...) + + return allErrs +} + func validateVolumeSource(source *compute.VolumeSource, fldPath *field.Path) field.ErrorList { var allErrs field.ErrorList diff --git a/apis/compute/validation/machine_test.go b/apis/compute/validation/machine_test.go index e70b47493..4f72b46d6 100644 --- a/apis/compute/validation/machine_test.go +++ b/apis/compute/validation/machine_test.go @@ -121,6 +121,61 @@ var _ = Describe("Machine", func() { }, ContainElement(InvalidField("spec.volume[0].emptyDisk.sizeLimit")), ), + Entry("duplicate machine volume device", + &compute.Machine{ + Spec: compute.MachineSpec{ + Volumes: []compute.Volume{ + { + Name: "foo", + Device: "vda", + }, + { + Name: "bar", + Device: "vda", + }, + }, + }, + }, + ContainElement(DuplicateField("spec.volume[1].device")), + ), + Entry("empty machine volume device", + &compute.Machine{ + Spec: compute.MachineSpec{ + Volumes: []compute.Volume{ + { + Name: "foo", + }, + }, + }, + }, + ContainElement(RequiredField("spec.volume[0].device")), + ), + Entry("invalid volume device", + &compute.Machine{ + Spec: compute.MachineSpec{ + Volumes: []compute.Volume{ + { + Name: "foo", + Device: "foobar", + }, + }, + }, + }, + ContainElement(InvalidField("spec.volume[0].device")), + ), + Entry("reserved volume device", + &compute.Machine{ + Spec: compute.MachineSpec{ + Volumes: []compute.Volume{ + { + Name: "foo", + Device: "vda", + }, + }, + }, + }, + ContainElement(ForbiddenField("spec.volume[0].device")), + ), Entry("invalid ignition ref name", &compute.Machine{ Spec: compute.MachineSpec{ diff --git a/apis/storage/register.go b/apis/storage/register.go index 88c920899..8da2b612a 100644 --- a/apis/storage/register.go +++ b/apis/storage/register.go @@ -36,10 +36,11 @@ var ( ) func Resource(name string) schema.GroupResource { - return schema.GroupResource{ - Group: SchemeGroupVersion.Group, - Resource: name, - } + return SchemeGroupVersion.WithResource(name).GroupResource() +} + +func Kind(name string) schema.GroupKind { + return SchemeGroupVersion.WithKind(name).GroupKind() } func addKnownTypes(scheme *runtime.Scheme) error { diff --git a/app/apiserver/apiserver.go b/app/apiserver/apiserver.go index aec44c5b1..d3bd060c2 100644 --- a/app/apiserver/apiserver.go +++ b/app/apiserver/apiserver.go @@ -20,6 +20,7 @@ import ( "net" "time" + "github.com/onmetal/onmetal-api/admission/plugin/machinevolumedevices" "github.com/onmetal/onmetal-api/api" "github.com/onmetal/onmetal-api/apis/compute" computev1alpha1 "github.com/onmetal/onmetal-api/apis/compute/v1alpha1" @@ -156,6 +157,10 @@ func (o *OnmetalAPIServerOptions) Validate(args []string) error { } func (o *OnmetalAPIServerOptions) Complete() error { + machinevolumedevices.Register(o.RecommendedOptions.Admission.Plugins) + + o.RecommendedOptions.Admission.RecommendedPluginOrder = append(o.RecommendedOptions.Admission.RecommendedPluginOrder, machinevolumedevices.PluginName) + return nil } diff --git a/docs/api-reference/common.md b/docs/api-reference/common.md index f1e199614..a7c3a9d9e 100644 --- a/docs/api-reference/common.md +++ b/docs/api-reference/common.md @@ -76,9 +76,7 @@ required.

-
- -inet.af/netaddr.IP - +net/netip.Addr @@ -103,9 +101,7 @@ inet.af/netaddr.IP -
- -inet.af/netaddr.IPPrefix - +net/netip.Prefix diff --git a/docs/api-reference/compute.md b/docs/api-reference/compute.md index 5e4e227c3..995d53534 100644 --- a/docs/api-reference/compute.md +++ b/docs/api-reference/compute.md @@ -1516,6 +1516,18 @@ string +device
+ +string + + + +

Device is the device name where the volume should be attached. If empty, +an unused device name will be determined if possible.

+ + + + VolumeSource
diff --git a/generated/openapi/zz_generated.openapi.go b/generated/openapi/zz_generated.openapi.go index 7c51f8cf4..115bdf8bb 100644 --- a/generated/openapi/zz_generated.openapi.go +++ b/generated/openapi/zz_generated.openapi.go @@ -1633,6 +1633,13 @@ func schema_onmetal_api_apis_compute_v1alpha1_Volume(ref common.ReferenceCallbac Format: "", }, }, + "device": { + SchemaProps: spec.SchemaProps{ + Description: "Device is the device name where the volume should be attached. If empty, an unused device name will be determined if possible.", + Type: []string{"string"}, + Format: "", + }, + }, "volumeRef": { SchemaProps: spec.SchemaProps{ Description: "VolumeRef instructs to use the specified Volume as source for the attachment.", diff --git a/go.mod b/go.mod index 6ceb80506..d1c72ff10 100644 --- a/go.mod +++ b/go.mod @@ -46,6 +46,7 @@ require ( github.com/PuerkitoBio/purell v1.1.1 // indirect github.com/PuerkitoBio/urlesc v0.0.0-20170810143723-de5bf2ad4578 // indirect github.com/beorn7/perks v1.0.1 // indirect + github.com/bits-and-blooms/bitset v1.3.3 // indirect github.com/blang/semver/v4 v4.0.0 // indirect github.com/bmatcuk/doublestar/v4 v4.0.2 // indirect github.com/cespare/xxhash/v2 v2.1.2 // indirect diff --git a/go.sum b/go.sum index c87c2b457..9691e936a 100644 --- a/go.sum +++ b/go.sum @@ -88,6 +88,8 @@ github.com/beorn7/perks v1.0.0/go.mod h1:KWe93zE9D1o94FZ5RNwFwVgaQK1VOXiVxmqh+Ce github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw= github.com/bgentry/speakeasy v0.1.0/go.mod h1:+zsyZBPWlz7T6j88CTgSN5bM796AkVf0kBD4zp0CCIs= +github.com/bits-and-blooms/bitset v1.3.3 h1:R1XWiopGiXf66xygsiLpzLo67xEYvMkHw3w+rCOSAwg= +github.com/bits-and-blooms/bitset v1.3.3/go.mod h1:gIdJ4wp64HaoK2YrL1Q5/N7Y16edYb8uY+O0FJTyyDA= github.com/bketelsen/crypt v0.0.3-0.20200106085610-5cbc8cc4026c/go.mod h1:MKsuJmJgSg28kpZDP6UIiPt0e0Oz0kqKNGyRaWEPv84= github.com/blang/semver/v4 v4.0.0 h1:1PFHFE6yCCTv8C1TeyNNarDzntLi7wMI5i/pzqYIsAM= github.com/blang/semver/v4 v4.0.0/go.mod h1:IbckMUScFkM3pff0VJDNKRiT6TG/YpiHIM2yvyW5YoQ=