Skip to content

Commit

Permalink
add webhook validation and tests
Browse files Browse the repository at this point in the history
  • Loading branch information
@viveksyngh
viveksyngh committed Jan 13, 2025
1 parent 29f7975 commit 9c3a28a
Show file tree
Hide file tree
Showing 5 changed files with 194 additions and 28 deletions.
43 changes: 42 additions & 1 deletion internal/webhooks/vspheremachinetemplate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,15 @@ import (

apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/validation"
"k8s.io/apimachinery/pkg/util/validation/field"
"sigs.k8s.io/cluster-api/util/topology"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/webhook"
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"

infrav1 "sigs.k8s.io/cluster-api-provider-vsphere/apis/v1beta1"
"sigs.k8s.io/cluster-api-provider-vsphere/pkg/services"
)

const machineTemplateImmutableMsg = "VSphereMachineTemplate spec.template.spec field is immutable. Please create a new resource instead."
Expand All @@ -50,7 +52,7 @@ func (webhook *VSphereMachineTemplateWebhook) SetupWebhookWithManager(mgr ctrl.M
}

// ValidateCreate implements webhook.Validator so a webhook will be registered for the type.
func (webhook *VSphereMachineTemplateWebhook) ValidateCreate(_ context.Context, raw runtime.Object) (admission.Warnings, error) {
func (webhook *VSphereMachineTemplateWebhook) ValidateCreate(ctx context.Context, raw runtime.Object) (admission.Warnings, error) {
obj, ok := raw.(*infrav1.VSphereMachineTemplate)
if !ok {
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a VSphereMachineTemplate but got a %T", raw))
Expand Down Expand Up @@ -87,6 +89,10 @@ func (webhook *VSphereMachineTemplateWebhook) ValidateCreate(_ context.Context,
pciErrs := validatePCIDevices(spec.PciDevices)
allErrs = append(allErrs, pciErrs...)

templateErrs := validateVSphereVMNamingTemplate(ctx, obj)
if len(templateErrs) > 0 {
allErrs = append(allErrs, templateErrs...)
}
return nil, AggregateObjErrors(obj.GroupVersionKind().GroupKind(), obj.Name, allErrs)
}

Expand All @@ -111,10 +117,45 @@ func (webhook *VSphereMachineTemplateWebhook) ValidateUpdate(ctx context.Context
!reflect.DeepEqual(newTyped.Spec.Template.Spec, oldTyped.Spec.Template.Spec) {
allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "template", "spec"), newTyped, machineTemplateImmutableMsg))
}

templateErrs := validateVSphereVMNamingTemplate(ctx, newTyped)
if len(templateErrs) > 0 {
allErrs = append(allErrs, templateErrs...)
}
return nil, AggregateObjErrors(newTyped.GroupVersionKind().GroupKind(), newTyped.Name, allErrs)
}

// ValidateDelete implements webhook.Validator so a webhook will be registered for the type.
func (webhook *VSphereMachineTemplateWebhook) ValidateDelete(_ context.Context, _ runtime.Object) (admission.Warnings, error) {
return nil, nil
}

func validateVSphereVMNamingTemplate(_ context.Context, vsphereMachineTemplate *infrav1.VSphereMachineTemplate) field.ErrorList {
var allErrs field.ErrorList
namingStrategy := vsphereMachineTemplate.Spec.Template.Spec.NamingStrategy
if namingStrategy != nil && namingStrategy.Template != nil {
name, err := services.GenerateVSphereVMName("machine", namingStrategy)
templateFldPath := field.NewPath("spec", "template", "spec", "namingStrategy", "template")
if err != nil {
allErrs = append(allErrs,
field.Invalid(
templateFldPath,
*namingStrategy.Template,
fmt.Sprintf("invalid VSphereVM name template: %v", err),
),
)
} else {
// Note: This validates that the resulting name is a valid Kubernetes object name.
for _, err := range validation.IsDNS1123Subdomain(name) {
allErrs = append(allErrs,
field.Invalid(
templateFldPath,
*namingStrategy.Template,
fmt.Sprintf("invalid VSphereVM name template, generated name is not a valid Kubernetes object name: %v", err),
),
)
}
}
}
return allErrs
}
88 changes: 61 additions & 27 deletions internal/webhooks/vspheremachinetemplate_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,70 +37,96 @@ func TestVSphereMachineTemplate_ValidateCreate(t *testing.T) {
}{
{
name: "preferredAPIServerCIDR set on creation ",
vsphereMachine: createVSphereMachineTemplate("foo.com", "", nil, "192.168.0.1/32", []string{}, nil),
vsphereMachine: createVSphereMachineTemplate("foo.com", "", nil, "192.168.0.1/32", []string{}, nil, nil),
wantErr: true,
},
{
name: "ProviderID set on creation",
vsphereMachine: createVSphereMachineTemplate("foo.com", "", &someProviderID, "", []string{}, nil),
vsphereMachine: createVSphereMachineTemplate("foo.com", "", &someProviderID, "", []string{}, nil, nil),
wantErr: true,
},
{
name: "IPs are not in CIDR format",
vsphereMachine: createVSphereMachineTemplate("foo.com", "", nil, "", []string{"192.168.0.1/32", "192.168.0.3"}, nil),
vsphereMachine: createVSphereMachineTemplate("foo.com", "", nil, "", []string{"192.168.0.1/32", "192.168.0.3"}, nil, nil),
wantErr: true,
},
{
name: "successful VSphereMachine creation",
vsphereMachine: createVSphereMachineTemplate("foo.com", "", nil, "", []string{"192.168.0.1/32", "192.168.0.3/32"}, nil),
vsphereMachine: createVSphereMachineTemplate("foo.com", "", nil, "", []string{"192.168.0.1/32", "192.168.0.3/32"}, nil, nil),
wantErr: true,
},
{
name: "incomplete hardware version",
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-", nil, "", []string{"192.168.0.1/32", "192.168.0.3/32"}, nil),
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-", nil, "", []string{"192.168.0.1/32", "192.168.0.3/32"}, nil, nil),
wantErr: true,
},
{
name: "incorrect hardware version",
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-0", nil, "", []string{"192.168.0.1/32", "192.168.0.3/32"}, nil),
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-0", nil, "", []string{"192.168.0.1/32", "192.168.0.3/32"}, nil, nil),
wantErr: true,
},
{
name: "empty pciDevice",
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-17", nil, "", []string{}, []infrav1.PCIDeviceSpec{{VGPUProfile: ""}}),
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-17", nil, "", []string{}, []infrav1.PCIDeviceSpec{{VGPUProfile: ""}}, nil),
wantErr: true,
},
{
name: "incorrect pciDevice",
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-17", nil, "", []string{}, []infrav1.PCIDeviceSpec{{VGPUProfile: "vgpu", DeviceID: ptr.To[int32](1)}}),
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-17", nil, "", []string{}, []infrav1.PCIDeviceSpec{{VGPUProfile: "vgpu", DeviceID: ptr.To[int32](1)}}, nil),
wantErr: true,
},
{
name: "incorrect pciDevice",
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-17", nil, "", []string{}, []infrav1.PCIDeviceSpec{{VGPUProfile: "vgpu", DeviceID: ptr.To[int32](1), VendorID: ptr.To[int32](1)}}),
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-17", nil, "", []string{}, []infrav1.PCIDeviceSpec{{VGPUProfile: "vgpu", DeviceID: ptr.To[int32](1), VendorID: ptr.To[int32](1)}}, nil),
wantErr: true,
},
{
name: "incomplete pciDevice",
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-17", nil, "", []string{}, []infrav1.PCIDeviceSpec{{DeviceID: ptr.To[int32](1)}}),
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-17", nil, "", []string{}, []infrav1.PCIDeviceSpec{{DeviceID: ptr.To[int32](1)}}, nil),
wantErr: true,
},
{
name: "incomplete pciDevice",
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-17", nil, "", []string{}, []infrav1.PCIDeviceSpec{{VendorID: ptr.To[int32](1)}}),
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-17", nil, "", []string{}, []infrav1.PCIDeviceSpec{{VendorID: ptr.To[int32](1)}}, nil),
wantErr: true,
},
{
name: "successful VSphereMachine creation with PCI device",
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-17", nil, "", []string{}, []infrav1.PCIDeviceSpec{{DeviceID: ptr.To[int32](1), VendorID: ptr.To[int32](1)}}),
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-17", nil, "", []string{}, []infrav1.PCIDeviceSpec{{DeviceID: ptr.To[int32](1), VendorID: ptr.To[int32](1)}}, nil),
},
{
name: "successful VSphereMachine creation with vgpu",
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-17", nil, "", []string{}, []infrav1.PCIDeviceSpec{{VGPUProfile: "vgpu"}}),
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-17", nil, "", []string{}, []infrav1.PCIDeviceSpec{{VGPUProfile: "vgpu"}}, nil),
},
{
name: "successful VSphereMachine creation with hardware version set",
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-17", nil, "", []string{}, nil),
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-17", nil, "", []string{}, nil, nil),
},
{
name: "successful VSphereMachineTemplate creation with namingStrategy not set",
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-17", nil, "", []string{}, nil, nil),
},
{
name: "successful VSphereMachineTemplate creation with namingStrategy.Template not set",
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-17", nil, "", []string{}, nil, &infrav1.VSphereVMNamingStrategy{Template: nil}),
},
{
name: "successful VSphereMachineTemplate creation with namingStrategy.template is set to the fallback value",
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-17", nil, "", []string{}, nil, &infrav1.VSphereVMNamingStrategy{Template: ptr.To[string]("{{ .machine.name }}")}),
},
{
name: "successful VSphereMachineTemplate creation with namingStrategy.template is set the Windows example",
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-17", nil, "", []string{}, nil, &infrav1.VSphereVMNamingStrategy{Template: ptr.To[string]("{{ if le (len .machine.name) 20 }}{{ .machine.name }}{{else}}{{ trimSuffix \"-\" (trunc 14 .machine.name) }}-{{ trunc -5 .machine.name }}{{end}}")}),
},
{
name: "failed VSphereMachineTemplate creation with namingStrategy.template is set to an invalid template",
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-17", nil, "", []string{}, nil, &infrav1.VSphereVMNamingStrategy{Template: ptr.To[string]("{{ invalid")}),
wantErr: true,
},
{
name: "failed VSphereMachineTemplate creation with namingStrategy.template is set to a valid template that renders an invalid name",
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-17", nil, "", []string{}, nil, &infrav1.VSphereVMNamingStrategy{Template: ptr.To[string]("-{{ .machine.name }}")}),
wantErr: true,
},
}
for _, tc := range tests {
Expand All @@ -127,46 +153,53 @@ func TestVSphereMachineTemplate_ValidateUpdate(t *testing.T) {
}{
{
name: "ProviderID cannot be updated",
oldVSphereMachine: createVSphereMachineTemplate("foo.com", "", nil, "", []string{"192.168.0.1/32"}, nil),
vsphereMachine: createVSphereMachineTemplate("foo.com", "", &someProviderID, "", []string{"192.168.0.1/32"}, nil),
oldVSphereMachine: createVSphereMachineTemplate("foo.com", "", nil, "", []string{"192.168.0.1/32"}, nil, nil),
vsphereMachine: createVSphereMachineTemplate("foo.com", "", &someProviderID, "", []string{"192.168.0.1/32"}, nil, nil),
req: &admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{DryRun: ptr.To(false)}},
wantErr: true,
},
{
name: "ip addresses cannot be updated",
oldVSphereMachine: createVSphereMachineTemplate("foo.com", "", nil, "", []string{"192.168.0.1/32"}, nil),
vsphereMachine: createVSphereMachineTemplate("foo.com", "", &someProviderID, "", []string{"192.168.0.1/32", "192.168.0.10/32"}, nil),
oldVSphereMachine: createVSphereMachineTemplate("foo.com", "", nil, "", []string{"192.168.0.1/32"}, nil, nil),
vsphereMachine: createVSphereMachineTemplate("foo.com", "", &someProviderID, "", []string{"192.168.0.1/32", "192.168.0.10/32"}, nil, nil),
req: &admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{DryRun: ptr.To(false)}},
wantErr: true,
},
{
name: "server cannot be updated",
oldVSphereMachine: createVSphereMachineTemplate("foo.com", "", nil, "", []string{"192.168.0.1/32"}, nil),
vsphereMachine: createVSphereMachineTemplate("baz.com", "", &someProviderID, "", []string{"192.168.0.1/32", "192.168.0.10/32"}, nil),
oldVSphereMachine: createVSphereMachineTemplate("foo.com", "", nil, "", []string{"192.168.0.1/32"}, nil, nil),
vsphereMachine: createVSphereMachineTemplate("baz.com", "", &someProviderID, "", []string{"192.168.0.1/32", "192.168.0.10/32"}, nil, nil),
req: &admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{DryRun: ptr.To(false)}},
wantErr: true,
},
{
name: "hardware version cannot be updated",
oldVSphereMachine: createVSphereMachineTemplate("foo.com", "vmx-16", nil, "", []string{"192.168.0.1/32"}, nil),
vsphereMachine: createVSphereMachineTemplate("baz.com", "vmx-17", nil, "", []string{"192.168.0.1/32"}, nil),
oldVSphereMachine: createVSphereMachineTemplate("foo.com", "vmx-16", nil, "", []string{"192.168.0.1/32"}, nil, nil),
vsphereMachine: createVSphereMachineTemplate("baz.com", "vmx-17", nil, "", []string{"192.168.0.1/32"}, nil, nil),
req: &admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{DryRun: ptr.To(false)}},
wantErr: true,
},
{
name: "pci devices cannot be updated",
oldVSphereMachine: createVSphereMachineTemplate("foo.com", "vmx-16", nil, "", []string{"192.168.0.1/32"}, []infrav1.PCIDeviceSpec{{VGPUProfile: "vgpu"}}),
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-16", nil, "", []string{"192.168.0.1/32"}, []infrav1.PCIDeviceSpec{{VGPUProfile: "new-vgpu"}}),
oldVSphereMachine: createVSphereMachineTemplate("foo.com", "vmx-16", nil, "", []string{"192.168.0.1/32"}, []infrav1.PCIDeviceSpec{{VGPUProfile: "vgpu"}}, nil),
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-16", nil, "", []string{"192.168.0.1/32"}, []infrav1.PCIDeviceSpec{{VGPUProfile: "new-vgpu"}}, nil),
req: &admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{DryRun: ptr.To(false)}},
wantErr: true,
},
{
name: "with hardware version set and not updated",
oldVSphereMachine: createVSphereMachineTemplate("foo.com", "vmx-16", nil, "", []string{"192.168.0.1/32"}, nil),
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-16", nil, "", []string{"192.168.0.1/32"}, nil),
oldVSphereMachine: createVSphereMachineTemplate("foo.com", "vmx-16", nil, "", []string{"192.168.0.1/32"}, nil, nil),
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-16", nil, "", []string{"192.168.0.1/32"}, nil, nil),
req: &admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{DryRun: ptr.To(false)}},
wantErr: false, // explicitly calling out that this is a valid scenario.
},
{
name: "naming strategy can not be updated",
oldVSphereMachine: createVSphereMachineTemplate("foo.com", "vmx-16", nil, "", []string{"192.168.0.1/32"}, nil, nil),
vsphereMachine: createVSphereMachineTemplate("foo.com", "vmx-16", nil, "", []string{}, nil, &infrav1.VSphereVMNamingStrategy{Template: ptr.To[string]("{{ .machine.name }}")}),
req: &admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{DryRun: ptr.To(false)}},
wantErr: true,
},
}
for _, tc := range tests {
t.Run(tc.name, func(*testing.T) {
Expand All @@ -185,7 +218,7 @@ func TestVSphereMachineTemplate_ValidateUpdate(t *testing.T) {
}
}

func createVSphereMachineTemplate(server, hwVersion string, providerID *string, preferredAPIServerCIDR string, ips []string, pciDevices []infrav1.PCIDeviceSpec) *infrav1.VSphereMachineTemplate {
func createVSphereMachineTemplate(server, hwVersion string, providerID *string, preferredAPIServerCIDR string, ips []string, pciDevices []infrav1.PCIDeviceSpec, vmNamingStrategy *infrav1.VSphereVMNamingStrategy) *infrav1.VSphereMachineTemplate {
vsphereMachineTemplate := &infrav1.VSphereMachineTemplate{
Spec: infrav1.VSphereMachineTemplateSpec{
Template: infrav1.VSphereMachineTemplateResource{
Expand All @@ -200,6 +233,7 @@ func createVSphereMachineTemplate(server, hwVersion string, providerID *string,
HardwareVersion: hwVersion,
PciDevices: pciDevices,
},
NamingStrategy: vmNamingStrategy,
},
},
},
Expand Down
Loading

0 comments on commit 9c3a28a

Please sign in to comment.