Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add custom tags to machines #158

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions api/v1alpha1/proxmoxmachine_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,14 @@ type ProxmoxMachineSpec struct {
// MetadataSettings defines the metadata settings for this machine's VM.
// +optional
MetadataSettings *MetadataSettings `json:"metadataSettings,omitempty"`

// Tags is a list of tags to be applied to the virtual machine.
// +optional
// +immutable
// +listType=set
// +kubebuilder:validation:MinItems=1
// +kubebuilder:validation:items:Pattern=`^[a-zA-Z0-9-_+]+$`
Copy link
Contributor

@pborn-ionos pborn-ionos Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might need some more refinement.

https://github.com/proxmox/pve-common/blob/master/src/PVE/JSONSchema.pm#L706

our $PVE_TAG_RE = qr/[a-z0-9_][a-z0-9_\-\+\.]*/i;

i.e.

// +kubebuilder:validation:items:Pattern=`^(?i)[a-z0-9_][a-z0-9_\-\+\.]*$`

to be true to upstream :)

Tags []string `json:"tags,omitempty"`
}

// Storage is the physical storage on the node.
Expand Down
28 changes: 28 additions & 0 deletions api/v1alpha1/proxmoxmachine_types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -352,4 +352,32 @@ var _ = Describe("ProxmoxMachine Test", func() {
Expect(k8sClient.Create(context.Background(), dm)).Should(MatchError(ContainSubstring("spec.vmIDRange.start in body should be greater than or equal to 100")))
})
})

Context("Tags", func() {
It("should disallow invalid tags", func() {
dm := defaultMachine()
dm.Spec.Tags = []string{"foo=bar"}
Expect(k8sClient.Create(context.Background(), dm)).Should(MatchError(ContainSubstring("Invalid value")))

dm.Spec.Tags = []string{"foo$bar"}
Expect(k8sClient.Create(context.Background(), dm)).Should(MatchError(ContainSubstring("Invalid value")))

dm.Spec.Tags = []string{"foo^bar"}
Expect(k8sClient.Create(context.Background(), dm)).Should(MatchError(ContainSubstring("Invalid value")))

dm.Spec.Tags = []string{"foo bar"}
Expect(k8sClient.Create(context.Background(), dm)).Should(MatchError(ContainSubstring("Invalid value")))

dm.Spec.Tags = []string{"foo "}
Expect(k8sClient.Create(context.Background(), dm)).Should(MatchError(ContainSubstring("Invalid value")))
})

It("Should not allow duplicated tags", func() {
dm := defaultMachine()
dm.Spec.Tags = []string{"foo", "bar", "foo"}
Expect(k8sClient.Create(context.Background(), dm)).Should(MatchError(ContainSubstring("Duplicate value")))
dm.Spec.Tags = []string{"foo", "bar"}
Expect(k8sClient.Create(context.Background(), dm)).To(Succeed())
})
})
})
5 changes: 5 additions & 0 deletions api/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,15 @@ spec:
storage:
description: Storage for full clone.
type: string
tags:
description: Tags is a list of tags to be applied to the
virtual machine.
items:
pattern: ^[a-zA-Z0-9-_+]+$
type: string
minItems: 1
type: array
x-kubernetes-list-type: set
target:
description: Target node. Only allowed if the original VM
is on shared storage.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,15 @@ spec:
storage:
description: Storage for full clone.
type: string
tags:
description: Tags is a list of tags to be applied
to the virtual machine.
items:
pattern: ^[a-zA-Z0-9-_+]+$
type: string
minItems: 1
type: array
x-kubernetes-list-type: set
target:
description: Target node. Only allowed if the original
VM is on shared storage.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -518,6 +518,14 @@ spec:
storage:
description: Storage for full clone.
type: string
tags:
description: Tags is a list of tags to be applied to the virtual machine.
items:
pattern: ^[a-zA-Z0-9-_+]+$
type: string
minItems: 1
type: array
x-kubernetes-list-type: set
target:
description: Target node. Only allowed if the original VM is on shared
storage.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,15 @@ spec:
storage:
description: Storage for full clone.
type: string
tags:
description: Tags is a list of tags to be applied to the virtual
machine.
items:
pattern: ^[a-zA-Z0-9-_+]+$
type: string
minItems: 1
type: array
x-kubernetes-list-type: set
target:
description: Target node. Only allowed if the original VM
is on shared storage.
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ replace github.com/google/cel-go => github.com/google/cel-go v0.17.8
require (
github.com/flatcar/ignition v0.36.2
github.com/go-logr/logr v1.4.2
github.com/google/go-cmp v0.6.0
github.com/google/uuid v1.6.0
github.com/jarcoal/httpmock v1.3.1
github.com/luthermonson/go-proxmox v0.2.1
Expand Down Expand Up @@ -75,7 +76,6 @@ require (
github.com/golang/protobuf v1.5.4 // indirect
github.com/google/cel-go v0.18.2 // indirect
github.com/google/gnostic-models v0.6.8 // indirect
github.com/google/go-cmp v0.6.0 // indirect
github.com/google/go-github/v53 v53.2.0 // indirect
github.com/google/go-querystring v1.1.0 // indirect
github.com/google/gofuzz v1.2.0 // indirect
Expand Down
34 changes: 31 additions & 3 deletions internal/service/vmservice/vm.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ package vmservice
import (
"context"
"slices"
"strings"

"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
Expand All @@ -42,9 +43,11 @@ const (
// See the following link for a list of available config options:
// https://pve.proxmox.com/pve-docs/api-viewer/index.html#/nodes/{node}/qemu/{vmid}/config

optionSockets = "sockets"
optionCores = "cores"
optionMemory = "memory"
optionSockets = "sockets"
optionCores = "cores"
optionMemory = "memory"
optionTags = "tags"
optionDescription = "description"
)

// ErrNoVMIDInRangeFree is returned if no free VMID is found in the specified vmIDRange.
Expand Down Expand Up @@ -240,6 +243,17 @@ func reconcileVirtualMachineConfig(ctx context.Context, machineScope *scope.Mach
vmOptions = append(vmOptions, proxmox.VirtualMachineOption{Name: optionMemory, Value: value})
}

// Description
if machineScope.ProxmoxMachine.Spec.Description != nil {
if machineScope.VirtualMachine.VirtualMachineConfig.Description != *machineScope.ProxmoxMachine.Spec.Description {
vmOptions = append(vmOptions, proxmox.VirtualMachineOption{Name: optionDescription, Value: machineScope.ProxmoxMachine.Spec.Description})
}
} else {
if machineScope.VirtualMachine.VirtualMachineConfig.Description != machineScope.ProxmoxMachine.GetName() {
vmOptions = append(vmOptions, proxmox.VirtualMachineOption{Name: optionDescription, Value: machineScope.ProxmoxMachine.GetName()})
}
}

// Network vmbrs.
if machineScope.ProxmoxMachine.Spec.Network != nil && shouldUpdateNetworkDevices(machineScope) {
// adding the default network device.
Expand All @@ -263,6 +277,20 @@ func reconcileVirtualMachineConfig(ctx context.Context, machineScope *scope.Mach
}
}

// custom tags
if machineScope.ProxmoxMachine.Spec.Tags != nil {
machineScope.VirtualMachine.SplitTags()
length := len(machineScope.VirtualMachine.VirtualMachineConfig.TagsSlice)
for _, tag := range machineScope.ProxmoxMachine.Spec.Tags {
if !machineScope.VirtualMachine.HasTag(tag) {
machineScope.VirtualMachine.VirtualMachineConfig.TagsSlice = append(machineScope.VirtualMachine.VirtualMachineConfig.TagsSlice, tag)
}
}
if len(machineScope.VirtualMachine.VirtualMachineConfig.TagsSlice) > length {
vmOptions = append(vmOptions, proxmox.VirtualMachineOption{Name: optionTags, Value: strings.Join(machineScope.VirtualMachine.VirtualMachineConfig.TagsSlice, ";")})
}
}

if len(vmOptions) == 0 {
return false, nil
}
Expand Down
25 changes: 25 additions & 0 deletions internal/service/vmservice/vm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,7 @@ func TestEnsureVirtualMachine_UpdateVMLocation_Error(t *testing.T) {
func TestReconcileVirtualMachineConfig_NoConfig(t *testing.T) {
machineScope, _, _ := setupReconcilerTest(t)
vm := newStoppedVM()
vm.VirtualMachineConfig.Description = machineScope.ProxmoxMachine.GetName()
machineScope.SetVirtualMachine(vm)

requeue, err := reconcileVirtualMachineConfig(context.Background(), machineScope)
Expand All @@ -395,6 +396,7 @@ func TestReconcileVirtualMachineConfig_NoConfig(t *testing.T) {

func TestReconcileVirtualMachineConfig_ApplyConfig(t *testing.T) {
machineScope, proxmoxClient, _ := setupReconcilerTest(t)
machineScope.ProxmoxMachine.Spec.Description = ptr.To("test vm")
machineScope.ProxmoxMachine.Spec.NumSockets = 4
machineScope.ProxmoxMachine.Spec.NumCores = 4
machineScope.ProxmoxMachine.Spec.MemoryMiB = 16 * 1024
Expand All @@ -415,6 +417,7 @@ func TestReconcileVirtualMachineConfig_ApplyConfig(t *testing.T) {
proxmox.VirtualMachineOption{Name: optionSockets, Value: machineScope.ProxmoxMachine.Spec.NumSockets},
proxmox.VirtualMachineOption{Name: optionCores, Value: machineScope.ProxmoxMachine.Spec.NumCores},
proxmox.VirtualMachineOption{Name: optionMemory, Value: machineScope.ProxmoxMachine.Spec.MemoryMiB},
proxmox.VirtualMachineOption{Name: optionDescription, Value: machineScope.ProxmoxMachine.Spec.Description},
proxmox.VirtualMachineOption{Name: "net0", Value: formatNetworkDevice("virtio", "vmbr0", ptr.To(uint16(1500)), nil)},
proxmox.VirtualMachineOption{Name: "net1", Value: formatNetworkDevice("virtio", "vmbr1", ptr.To(uint16(1500)), nil)},
}
Expand All @@ -427,6 +430,27 @@ func TestReconcileVirtualMachineConfig_ApplyConfig(t *testing.T) {
require.EqualValues(t, task.UPID, *machineScope.ProxmoxMachine.Status.TaskRef)
}

func TestReconcileVirtualMachineConfigTags(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test case for multiple tags and for zero/empty tags?

machineScope, proxmoxClient, _ := setupReconcilerTest(t)
machineScope.ProxmoxMachine.Spec.Tags = []string{"tag1", "tag2"}

vm := newStoppedVM()
vm.VirtualMachineConfig.Tags = "tag0"
task := newTask()
machineScope.SetVirtualMachine(vm)
expectedOptions := []interface{}{
proxmox.VirtualMachineOption{Name: optionDescription, Value: machineScope.ProxmoxMachine.GetName()},
proxmox.VirtualMachineOption{Name: optionTags, Value: "tag0;tag1;tag2"},
}

proxmoxClient.EXPECT().ConfigureVM(context.Background(), vm, expectedOptions...).Return(task, nil).Once()

requeue, err := reconcileVirtualMachineConfig(context.Background(), machineScope)
require.NoError(t, err)
require.True(t, requeue)
require.EqualValues(t, task.UPID, *machineScope.ProxmoxMachine.Status.TaskRef)
}

func TestReconcileDisks_RunningVM(t *testing.T) {
machineScope, _, _ := setupReconcilerTest(t)
machineScope.ProxmoxMachine.Spec.Disks = &infrav1alpha1.Storage{
Expand Down Expand Up @@ -525,6 +549,7 @@ func TestReconcileVirtualMachineConfigVLAN(t *testing.T) {
proxmox.VirtualMachineOption{Name: optionSockets, Value: machineScope.ProxmoxMachine.Spec.NumSockets},
proxmox.VirtualMachineOption{Name: optionCores, Value: machineScope.ProxmoxMachine.Spec.NumCores},
proxmox.VirtualMachineOption{Name: optionMemory, Value: machineScope.ProxmoxMachine.Spec.MemoryMiB},
proxmox.VirtualMachineOption{Name: optionDescription, Value: machineScope.ProxmoxMachine.GetName()},
proxmox.VirtualMachineOption{Name: "net0", Value: formatNetworkDevice("virtio", "vmbr0", nil, ptr.To(uint16(100)))},
proxmox.VirtualMachineOption{Name: "net1", Value: formatNetworkDevice("virtio", "vmbr1", nil, ptr.To(uint16(100)))},
}
Expand Down
13 changes: 11 additions & 2 deletions internal/webhook/proxmoxmachine_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@ import (
"context"
"fmt"

"github.com/google/go-cmp/cmp"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/validation/field"

ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"

Expand Down Expand Up @@ -63,12 +63,21 @@ func (p *ProxmoxMachine) ValidateCreate(_ context.Context, obj runtime.Object) (
}

// ValidateUpdate implements the update validation function.
func (p *ProxmoxMachine) ValidateUpdate(_ context.Context, _, newObj runtime.Object) (warnings admission.Warnings, err error) {
func (p *ProxmoxMachine) ValidateUpdate(_ context.Context, old, newObj runtime.Object) (warnings admission.Warnings, err error) {
newMachine, ok := newObj.(*infrav1.ProxmoxMachine)
if !ok {
return warnings, apierrors.NewBadRequest(fmt.Sprintf("expected a ProxmoxMachine but got %T", newObj))
}

oldMachine, ok := old.(*infrav1.ProxmoxMachine)
if !ok {
return warnings, apierrors.NewBadRequest(fmt.Sprintf("expected a ProxmoxMachine but got %T", old))
}
// tags are immutable
if !cmp.Equal(newMachine.Spec.Tags, oldMachine.Spec.Tags) {
return warnings, apierrors.NewBadRequest("tags are immutable")
}

err = validateNetworks(newMachine)
if err != nil {
warnings = append(warnings, fmt.Sprintf("cannot update proxmox machine %s", newMachine.GetName()))
Expand Down
9 changes: 9 additions & 0 deletions internal/webhook/proxmoxmachine_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,15 @@ var _ = Describe("Controller Test", func() {
WithPolling(time.Second).
Should(Succeed())
})

It("should not allow updates on tags", func() {
machine := validProxmoxMachine("test-machine-tags")
machine.Spec.Tags = []string{"foo_bar"}
g.Expect(k8sClient.Create(testEnv.GetContext(), &machine)).To(Succeed())

machine.Spec.Tags = []string{"foobar", "barfoo"}
g.Expect(k8sClient.Update(testEnv.GetContext(), &machine)).To(MatchError(ContainSubstring("tags are immutable")))
})
})
})

Expand Down
Loading