From cf5a889ff1f2980d1ece501415bde0b4ee1fcda0 Mon Sep 17 00:00:00 2001 From: Melvin Hillsman Date: Sat, 13 Jun 2026 10:34:06 -0500 Subject: [PATCH] fix: remove dead inline userdata branch in BuildVMSpec The orchestrator always creates a cloud-init Secret and sets CloudInitSecretName before calling BuildVMSpec, so the else branch that inlined UserData was dead code. Remove it along with the CloudInitUserdata field from VMSpecOpts, and validate that CloudInitSecretName is always provided. Resolves #183 Signed-off-by: Melvin Hillsman --- internal/cleanup/cleanup_test.go | 52 +++++------ internal/orchestrator/orchestrator.go | 2 - internal/testutil/testutil.go | 12 +-- internal/testutil/testutil_test.go | 4 +- internal/vm/vm.go | 41 ++++----- internal/vm/vm_test.go | 125 ++++++++++++-------------- 6 files changed, 108 insertions(+), 128 deletions(-) diff --git a/internal/cleanup/cleanup_test.go b/internal/cleanup/cleanup_test.go index db9a04f..e51e669 100644 --- a/internal/cleanup/cleanup_test.go +++ b/internal/cleanup/cleanup_test.go @@ -49,13 +49,13 @@ var _ = Describe("PreviewCleanup", func() { maps.Copy(l, el) } v, err := vm.BuildVMSpec(vm.VMSpecOpts{ - Name: name, - Namespace: namespace, - ContainerDiskImage: "test-image", - CloudInitUserdata: "#cloud-config\n", - CPUCores: 1, - Memory: "1Gi", - Labels: l, + Name: name, + Namespace: namespace, + ContainerDiskImage: "test-image", + CloudInitSecretName: "test-cloudinit", + CPUCores: 1, + Memory: "1Gi", + Labels: l, }) Expect(err).NotTo(HaveOccurred()) return v @@ -168,12 +168,12 @@ var _ = Describe("PreviewCleanup", func() { It("should not count unmanaged resources", func() { managedVM := newManagedVM("managed-vm") unmanagedVM, err := vm.BuildVMSpec(vm.VMSpecOpts{ - Name: "unmanaged-vm", - Namespace: namespace, - ContainerDiskImage: "test-image", - CloudInitUserdata: "#cloud-config\n", - CPUCores: 1, - Memory: "1Gi", + Name: "unmanaged-vm", + Namespace: namespace, + ContainerDiskImage: "test-image", + CloudInitSecretName: "test-cloudinit", + CPUCores: 1, + Memory: "1Gi", Labels: map[string]string{ constants.LabelManagedBy: "other-tool", }, @@ -329,13 +329,13 @@ var _ = Describe("CleanupAll", func() { newManagedVM := func(name string) *kubevirtv1.VirtualMachine { v, err := vm.BuildVMSpec(vm.VMSpecOpts{ - Name: name, - Namespace: namespace, - ContainerDiskImage: "test-image", - CloudInitUserdata: "#cloud-config\n", - CPUCores: 1, - Memory: "1Gi", - Labels: labels, + Name: name, + Namespace: namespace, + ContainerDiskImage: "test-image", + CloudInitSecretName: "test-cloudinit", + CPUCores: 1, + Memory: "1Gi", + Labels: labels, }) Expect(err).NotTo(HaveOccurred()) return v @@ -745,12 +745,12 @@ var _ = Describe("CleanupAll", func() { // Create a managed VM and an unmanaged VM managedVM := newManagedVM("managed-vm") unmanagedVM, err := vm.BuildVMSpec(vm.VMSpecOpts{ - Name: "unmanaged-vm", - Namespace: namespace, - ContainerDiskImage: "test-image", - CloudInitUserdata: "#cloud-config\n", - CPUCores: 1, - Memory: "1Gi", + Name: "unmanaged-vm", + Namespace: namespace, + ContainerDiskImage: "test-image", + CloudInitSecretName: "test-cloudinit", + CPUCores: 1, + Memory: "1Gi", Labels: map[string]string{ constants.LabelManagedBy: "other-tool", }, diff --git a/internal/orchestrator/orchestrator.go b/internal/orchestrator/orchestrator.go index 9e51e71..8db791a 100644 --- a/internal/orchestrator/orchestrator.go +++ b/internal/orchestrator/orchestrator.go @@ -526,7 +526,6 @@ func (ro *RunOrchestrator) createVMs( Name: p.VMSpec.Name, Namespace: p.VMSpec.Namespace, ContainerDiskImage: p.VMSpec.ContainerDiskImage, - CloudInitUserdata: p.VMSpec.CloudInitUserdata, CloudInitSecretName: p.VMSpec.CloudInitSecretName, CPUCores: p.VMSpec.CPUCores, Memory: p.VMSpec.Memory, @@ -764,7 +763,6 @@ func (ro *RunOrchestrator) printDryRun( Name: p.VMSpec.Name, Namespace: p.VMSpec.Namespace, ContainerDiskImage: p.VMSpec.ContainerDiskImage, - CloudInitUserdata: p.VMSpec.CloudInitUserdata, CloudInitSecretName: p.VMSpec.CloudInitSecretName, CPUCores: p.VMSpec.CPUCores, Memory: p.VMSpec.Memory, diff --git a/internal/testutil/testutil.go b/internal/testutil/testutil.go index 4cde149..3c6a1f2 100644 --- a/internal/testutil/testutil.go +++ b/internal/testutil/testutil.go @@ -119,12 +119,12 @@ func CleanupNamespace(ctx context.Context, c client.Client, namespace string) { // vm.CreateVM(). Modify the returned options as needed for specific tests. func DefaultVMOpts(name, namespace string) vm.VMSpecOpts { return vm.VMSpecOpts{ - Name: name, - Namespace: namespace, - ContainerDiskImage: constants.DefaultContainerDiskImage, - CloudInitUserdata: "#cloud-config\n", - CPUCores: 1, - Memory: "512Mi", + Name: name, + Namespace: namespace, + ContainerDiskImage: constants.DefaultContainerDiskImage, + CloudInitSecretName: "test-cloudinit", + CPUCores: 1, + Memory: "512Mi", Labels: map[string]string{ constants.LabelManagedBy: constants.ManagedByValue, constants.LabelAppName: "virtwork", diff --git a/internal/testutil/testutil_test.go b/internal/testutil/testutil_test.go index 5ee8791..e8ec2c0 100644 --- a/internal/testutil/testutil_test.go +++ b/internal/testutil/testutil_test.go @@ -114,9 +114,9 @@ var _ = Describe("DefaultVMOpts", func() { Expect(opts.Labels).To(HaveKeyWithValue(constants.LabelComponent, "test")) }) - It("should have a simple cloud-init userdata", func() { + It("should have a cloud-init secret name", func() { opts := testutil.DefaultVMOpts(vmName, vmNamespace) - Expect(opts.CloudInitUserdata).To(Equal("#cloud-config\n")) + Expect(opts.CloudInitSecretName).To(Equal("test-cloudinit")) }) }) }) diff --git a/internal/vm/vm.go b/internal/vm/vm.go index 159c375..7c1c1ac 100644 --- a/internal/vm/vm.go +++ b/internal/vm/vm.go @@ -25,10 +25,11 @@ const defaultMaxRetries = 5 var baseRetryBackoff = time.Second var ( - ErrInvalidName = errors.New("invalid name: must not be empty") - ErrInvalidNamespace = errors.New("invalid namespace: must not be empty") - ErrInvalidCPUCores = errors.New("invalid cpuCores: must be a positive integer") - ErrInvalidContainerDiskImage = errors.New("invalid containerDiskImage: must not be empty") + ErrInvalidName = errors.New("invalid name: must not be empty") + ErrInvalidNamespace = errors.New("invalid namespace: must not be empty") + ErrInvalidCPUCores = errors.New("invalid cpuCores: must be a positive integer") + ErrInvalidContainerDiskImage = errors.New("invalid containerDiskImage: must not be empty") + ErrInvalidCloudInitSecretName = errors.New("invalid cloudInitSecretName: must not be empty") ) // VMSpecOpts contains all parameters needed to construct a VirtualMachine spec. @@ -36,8 +37,7 @@ type VMSpecOpts struct { Name string Namespace string ContainerDiskImage string - CloudInitUserdata string - CloudInitSecretName string // When set, use UserDataSecretRef instead of inline + CloudInitSecretName string CPUCores int Memory string Labels map[string]string @@ -62,6 +62,9 @@ func BuildVMSpec(opts VMSpecOpts) (*kubevirtv1.VirtualMachine, error) { if opts.ContainerDiskImage == "" { return nil, ErrInvalidContainerDiskImage } + if opts.CloudInitSecretName == "" { + return nil, ErrInvalidCloudInitSecretName + } runStrategy := kubevirtv1.RunStrategyAlways @@ -85,27 +88,15 @@ func BuildVMSpec(opts VMSpecOpts) (*kubevirtv1.VirtualMachine, error) { } disks = append(disks, opts.ExtraDisks...) - var cloudInitVolume kubevirtv1.Volume - if opts.CloudInitSecretName != "" { - cloudInitVolume = kubevirtv1.Volume{ - Name: constants.DiskNameCloudInit, - VolumeSource: kubevirtv1.VolumeSource{ - CloudInitNoCloud: &kubevirtv1.CloudInitNoCloudSource{ - UserDataSecretRef: &corev1.LocalObjectReference{ - Name: opts.CloudInitSecretName, - }, + cloudInitVolume := kubevirtv1.Volume{ + Name: constants.DiskNameCloudInit, + VolumeSource: kubevirtv1.VolumeSource{ + CloudInitNoCloud: &kubevirtv1.CloudInitNoCloudSource{ + UserDataSecretRef: &corev1.LocalObjectReference{ + Name: opts.CloudInitSecretName, }, }, - } - } else { - cloudInitVolume = kubevirtv1.Volume{ - Name: constants.DiskNameCloudInit, - VolumeSource: kubevirtv1.VolumeSource{ - CloudInitNoCloud: &kubevirtv1.CloudInitNoCloudSource{ - UserData: opts.CloudInitUserdata, - }, - }, - } + }, } volumes := []kubevirtv1.Volume{ diff --git a/internal/vm/vm_test.go b/internal/vm/vm_test.go index 0b4f673..88fc077 100644 --- a/internal/vm/vm_test.go +++ b/internal/vm/vm_test.go @@ -31,12 +31,12 @@ var _ = Describe("BuildVMSpec", func() { BeforeEach(func() { opts = vm.VMSpecOpts{ - Name: "test-vm", - Namespace: "test-ns", - ContainerDiskImage: "quay.io/containerdisks/fedora:41", - CloudInitUserdata: "#cloud-config\n", - CPUCores: 2, - Memory: "2Gi", + Name: "test-vm", + Namespace: "test-ns", + ContainerDiskImage: "quay.io/containerdisks/fedora:41", + CloudInitSecretName: "test-vm-cloudinit", + CPUCores: 2, + Memory: "2Gi", Labels: map[string]string{ "app.kubernetes.io/managed-by": "virtwork", "app.kubernetes.io/name": "cpu", @@ -82,7 +82,8 @@ var _ = Describe("BuildVMSpec", func() { } Expect(cloudInit).NotTo(BeNil()) Expect(cloudInit.CloudInitNoCloud).NotTo(BeNil()) - Expect(cloudInit.CloudInitNoCloud.UserData).To(Equal("#cloud-config\n")) + Expect(cloudInit.CloudInitNoCloud.UserDataSecretRef).NotTo(BeNil()) + Expect(cloudInit.CloudInitNoCloud.UserDataSecretRef.Name).To(Equal("test-vm-cloudinit")) }) It("should set labels", func() { @@ -120,8 +121,7 @@ var _ = Describe("BuildVMSpec", func() { Expect(networks[0].Pod).NotTo(BeNil()) }) - It("should use UserDataSecretRef when CloudInitSecretName is set", func() { - opts.CloudInitSecretName = "my-vm-cloudinit" + It("should use UserDataSecretRef from CloudInitSecretName", func() { var err error result, err = vm.BuildVMSpec(opts) Expect(err).NotTo(HaveOccurred()) @@ -137,28 +137,14 @@ var _ = Describe("BuildVMSpec", func() { Expect(cloudInit).NotTo(BeNil()) Expect(cloudInit.CloudInitNoCloud).NotTo(BeNil()) Expect(cloudInit.CloudInitNoCloud.UserDataSecretRef).NotTo(BeNil()) - Expect(cloudInit.CloudInitNoCloud.UserDataSecretRef.Name).To(Equal("my-vm-cloudinit")) + Expect(cloudInit.CloudInitNoCloud.UserDataSecretRef.Name).To(Equal("test-vm-cloudinit")) Expect(cloudInit.CloudInitNoCloud.UserData).To(BeEmpty()) }) - It("should use inline UserData when CloudInitSecretName is empty", func() { + It("should return error when CloudInitSecretName is empty", func() { opts.CloudInitSecretName = "" - var err error - result, err = vm.BuildVMSpec(opts) - Expect(err).NotTo(HaveOccurred()) - - volumes := result.Spec.Template.Spec.Volumes - var cloudInit *kubevirtv1.Volume - for i := range volumes { - if volumes[i].Name == "cloudinitdisk" { - cloudInit = &volumes[i] - break - } - } - Expect(cloudInit).NotTo(BeNil()) - Expect(cloudInit.CloudInitNoCloud).NotTo(BeNil()) - Expect(cloudInit.CloudInitNoCloud.UserData).To(Equal("#cloud-config\n")) - Expect(cloudInit.CloudInitNoCloud.UserDataSecretRef).To(BeNil()) + _, err := vm.BuildVMSpec(opts) + Expect(err).To(MatchError(vm.ErrInvalidCloudInitSecretName)) }) It("should include extra disks when provided", func() { @@ -209,13 +195,13 @@ var _ = Describe("BuildVMSpec", func() { "should reject invalid inputs", func(mutate func(*vm.VMSpecOpts), substr string) { good := vm.VMSpecOpts{ - Name: "test-vm", - Namespace: "test-ns", - ContainerDiskImage: "quay.io/containerdisks/fedora:41", - CloudInitUserdata: "#cloud-config\n", - CPUCores: 2, - Memory: "2Gi", - Labels: map[string]string{"test": "true"}, + Name: "test-vm", + Namespace: "test-ns", + ContainerDiskImage: "quay.io/containerdisks/fedora:41", + CloudInitSecretName: "test-cloudinit", + CPUCores: 2, + Memory: "2Gi", + Labels: map[string]string{"test": "true"}, } mutate(&good) _, err := vm.BuildVMSpec(good) @@ -231,6 +217,11 @@ var _ = Describe("BuildVMSpec", func() { func(o *vm.VMSpecOpts) { o.ContainerDiskImage = "" }, "containerDiskImage", ), + Entry( + "empty cloud init secret name", + func(o *vm.VMSpecOpts) { o.CloudInitSecretName = "" }, + "cloudInitSecretName", + ), ) }) }) @@ -279,13 +270,13 @@ var _ = Describe("CreateVM", func() { newTestVM := func(name string) *kubevirtv1.VirtualMachine { v, err := vm.BuildVMSpec(vm.VMSpecOpts{ - Name: name, - Namespace: "default", - ContainerDiskImage: "test-image", - CloudInitUserdata: "#cloud-config\n", - CPUCores: 1, - Memory: "1Gi", - Labels: map[string]string{"test": "true"}, + Name: name, + Namespace: "default", + ContainerDiskImage: "test-image", + CloudInitSecretName: "test-cloudinit", + CPUCores: 1, + Memory: "1Gi", + Labels: map[string]string{"test": "true"}, }) Expect(err).NotTo(HaveOccurred()) return v @@ -293,12 +284,12 @@ var _ = Describe("CreateVM", func() { It("should return error for invalid memory", func() { _, err := vm.BuildVMSpec(vm.VMSpecOpts{ - Name: "bad-mem", - Namespace: "default", - ContainerDiskImage: "test-image", - CloudInitUserdata: "#cloud-config\n", - CPUCores: 1, - Memory: "not-a-quantity", + Name: "bad-mem", + Namespace: "default", + ContainerDiskImage: "test-image", + CloudInitSecretName: "test-cloudinit", + CPUCores: 1, + Memory: "not-a-quantity", }) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("invalid memory")) @@ -398,12 +389,12 @@ var _ = Describe("DeleteVM", func() { It("should delete VM successfully", func() { testVM, err := vm.BuildVMSpec(vm.VMSpecOpts{ - Name: "delete-me", - Namespace: "default", - ContainerDiskImage: "test-image", - CloudInitUserdata: "#cloud-config\n", - CPUCores: 1, - Memory: "1Gi", + Name: "delete-me", + Namespace: "default", + ContainerDiskImage: "test-image", + CloudInitSecretName: "test-cloudinit", + CPUCores: 1, + Memory: "1Gi", }) Expect(err).NotTo(HaveOccurred()) c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(testVM).Build() @@ -436,23 +427,23 @@ var _ = Describe("ListVMs", func() { It("should list VMs by labels", func() { vm1, err := vm.BuildVMSpec(vm.VMSpecOpts{ - Name: "vm-1", - Namespace: "default", - ContainerDiskImage: "test-image", - CloudInitUserdata: "#cloud-config\n", - CPUCores: 1, - Memory: "1Gi", - Labels: map[string]string{"app.kubernetes.io/managed-by": "virtwork"}, + Name: "vm-1", + Namespace: "default", + ContainerDiskImage: "test-image", + CloudInitSecretName: "test-cloudinit", + CPUCores: 1, + Memory: "1Gi", + Labels: map[string]string{"app.kubernetes.io/managed-by": "virtwork"}, }) Expect(err).NotTo(HaveOccurred()) vm2, err := vm.BuildVMSpec(vm.VMSpecOpts{ - Name: "vm-2", - Namespace: "default", - ContainerDiskImage: "test-image", - CloudInitUserdata: "#cloud-config\n", - CPUCores: 1, - Memory: "1Gi", - Labels: map[string]string{"app.kubernetes.io/managed-by": "other"}, + Name: "vm-2", + Namespace: "default", + ContainerDiskImage: "test-image", + CloudInitSecretName: "test-cloudinit", + CPUCores: 1, + Memory: "1Gi", + Labels: map[string]string{"app.kubernetes.io/managed-by": "other"}, }) Expect(err).NotTo(HaveOccurred()) c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(vm1, vm2).Build()