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.

Sign up for GitHub

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 VM naming strategy for VMs in govmomi mode #3286

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+402 −46
Open

✨ add VM naming strategy for VMs in govmomi mode #3286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4fc91b1
:sparkles: add VM naming strategy for VMs in govami mode
viveksyngh Dec 8, 2024
2a8faf0
rename the type to VSphereVMNamingStrategy
viveksyngh Dec 11, 2024
a2e314d
add support for a random function
viveksyngh Dec 11, 2024
29f7975
remove random function support
viveksyngh Dec 14, 2024
9c3a28a
add webhook validation and tests
viveksyngh Dec 16, 2024
eb7233a
fix lint issues
viveksyngh Dec 25, 2024
07f2025
add sprig package as depedency
viveksyngh Jan 13, 2025
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
1 change: 1 addition & 0 deletions apis/v1alpha3/vspheremachine_conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ func (src *VSphereMachine) ConvertTo(dstRaw conversion.Hub) error {
dst.Spec.TagIDs = restored.Spec.TagIDs
dst.Spec.PowerOffMode = restored.Spec.PowerOffMode
dst.Spec.GuestSoftPowerOffTimeout = restored.Spec.GuestSoftPowerOffTimeout
dst.Spec.NamingStrategy = restored.Spec.NamingStrategy
for i := range dst.Spec.Network.Devices {
dst.Spec.Network.Devices[i].AddressesFromPools = restored.Spec.Network.Devices[i].AddressesFromPools
dst.Spec.Network.Devices[i].DHCP4Overrides = restored.Spec.Network.Devices[i].DHCP4Overrides
Expand Down
1 change: 1 addition & 0 deletions apis/v1alpha3/vspheremachinetemplate_conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ func (src *VSphereMachineTemplate) ConvertTo(dstRaw conversion.Hub) error {
dst.Spec.Template.Spec.AdditionalDisksGiB = restored.Spec.Template.Spec.AdditionalDisksGiB
dst.Spec.Template.Spec.PowerOffMode = restored.Spec.Template.Spec.PowerOffMode
dst.Spec.Template.Spec.GuestSoftPowerOffTimeout = restored.Spec.Template.Spec.GuestSoftPowerOffTimeout
dst.Spec.Template.Spec.NamingStrategy = restored.Spec.Template.Spec.NamingStrategy
for i := range dst.Spec.Template.Spec.Network.Devices {
dst.Spec.Template.Spec.Network.Devices[i].AddressesFromPools = restored.Spec.Template.Spec.Network.Devices[i].AddressesFromPools
dst.Spec.Template.Spec.Network.Devices[i].DHCP4Overrides = restored.Spec.Template.Spec.Network.Devices[i].DHCP4Overrides
Expand Down
1 change: 1 addition & 0 deletions apis/v1alpha3/zz_generated.conversion.go
View file
Open in desktop

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

1 change: 1 addition & 0 deletions apis/v1alpha4/vspheremachine_conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ func (src *VSphereMachine) ConvertTo(dstRaw conversion.Hub) error {
dst.Spec.TagIDs = restored.Spec.TagIDs
dst.Spec.PowerOffMode = restored.Spec.PowerOffMode
dst.Spec.GuestSoftPowerOffTimeout = restored.Spec.GuestSoftPowerOffTimeout
dst.Spec.NamingStrategy = restored.Spec.NamingStrategy
for i := range dst.Spec.Network.Devices {
dst.Spec.Network.Devices[i].AddressesFromPools = restored.Spec.Network.Devices[i].AddressesFromPools
dst.Spec.Network.Devices[i].DHCP4Overrides = restored.Spec.Network.Devices[i].DHCP4Overrides
Expand Down
1 change: 1 addition & 0 deletions apis/v1alpha4/vspheremachinetemplate_conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ func (src *VSphereMachineTemplate) ConvertTo(dstRaw conversion.Hub) error {
dst.Spec.Template.Spec.AdditionalDisksGiB = restored.Spec.Template.Spec.AdditionalDisksGiB
dst.Spec.Template.Spec.PowerOffMode = restored.Spec.Template.Spec.PowerOffMode
dst.Spec.Template.Spec.GuestSoftPowerOffTimeout = restored.Spec.Template.Spec.GuestSoftPowerOffTimeout
dst.Spec.Template.Spec.NamingStrategy = restored.Spec.Template.Spec.NamingStrategy
for i := range dst.Spec.Template.Spec.Network.Devices {
dst.Spec.Template.Spec.Network.Devices[i].AddressesFromPools = restored.Spec.Template.Spec.Network.Devices[i].AddressesFromPools
dst.Spec.Template.Spec.Network.Devices[i].DHCP4Overrides = restored.Spec.Template.Spec.Network.Devices[i].DHCP4Overrides
Expand Down
1 change: 1 addition & 0 deletions apis/v1alpha4/zz_generated.conversion.go
View file
Open in desktop

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

24 changes: 24 additions & 0 deletions apis/v1beta1/vspheremachine_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,30 @@ type VSphereMachineSpec struct {
//
// +optional
GuestSoftPowerOffTimeout *metav1.Duration `json:"guestSoftPowerOffTimeout,omitempty"`

// NamingStrategy allows configuring the naming strategy used when calculating the name of the VSphereVM.
viveksyngh marked this conversation as resolved.
Show resolved Hide resolved
// +optional
NamingStrategy *VSphereVMNamingStrategy `json:"namingStrategy,omitempty"`
}

// VSphereVMNamingStrategy defines the naming strategy for the VSphereVMs.
type VSphereVMNamingStrategy struct {
Comment on lines +77 to +78
Copy link
Member

Choose a reason for hiding this comment

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

should this be named
VSphereMachineNamingStrategy

Copy link
Member

Choose a reason for hiding this comment

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

+1, as this struct is used at the VSphereMachine

Copy link
Author

Choose a reason for hiding this comment

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

I can update that but I did this to avoid confusion when someone looking at the code later .. this is used to decide the strategy for vspherevms objects.

// Template defines the template to use for generating the name of the VSphereVM object.
// If not defined, it will fall back to `{{ .machine.name }}`.
// The templating has the following data available:
// * `.machine.name`: The name of the Machine object.
// The templating also has the following funcs available:
// * `trimSuffix`: same as strings.TrimSuffix
// * `trunc`: truncates a string, e.g. `trunc 2 "hello"` or `trunc -2 "hello"`
// Notes:
// * While the template offers some flexibility, we would like the name to link to the Machine name
// to ensure better user experience when troubleshooting
viveksyngh marked this conversation as resolved.
Show resolved Hide resolved
// * Generated names must be valid Kubernetes names as they are used to create a VSphereVM object
// and usually also as the name of the Node object.
// * Names are automatically truncated at 63 characters. Please note that this can lead to name conflicts,
// so we highly recommend to use a template which leads to a name shorter than 63 characters.
// +optional
Template *string `json:"template,omitempty"`
viveksyngh marked this conversation as resolved.
Show resolved Hide resolved
}

// VSphereMachineStatus defines the observed state of VSphereMachine.
Expand Down
25 changes: 25 additions & 0 deletions apis/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

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

22 changes: 22 additions & 0 deletions config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachines.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1051,6 +1051,28 @@ spec:
virtual machine is cloned.
format: int64
type: integer
namingStrategy:
description: NamingStrategy allows configuring the naming strategy
used when calculating the name of the VSphereVM.
properties:
template:
description: |-
Template defines the template to use for generating the name of the VSphereVM object.
If not defined, it will fall back to `{{ .machine.name }}`.
The templating has the following data available:
* `.machine.name`: The name of the Machine object.
The templating also has the following funcs available:
* `trimSuffix`: same as strings.TrimSuffix
* `trunc`: truncates a string, e.g. `trunc 2 "hello"` or `trunc -2 "hello"`
Notes:
* While the template offers some flexibility, we would like the name to link to the Machine name
to ensure better user experience when troubleshooting
* Generated names must be valid Kubernetes names as they are used to create a VSphereVM object
and usually also as the name of the Node object.
* Names are automatically truncated at 63 characters. Please note that this can lead to name conflicts,
so we highly recommend to use a template which leads to a name shorter than 63 characters.
type: string
type: object
network:
description: Network is the network configuration for this machine's
VM.
Expand Down
22 changes: 22 additions & 0 deletions config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -921,6 +921,28 @@ spec:
virtual machine is cloned.
format: int64
type: integer
namingStrategy:
description: NamingStrategy allows configuring the naming
strategy used when calculating the name of the VSphereVM.
properties:
template:
description: |-
Template defines the template to use for generating the name of the VSphereVM object.
If not defined, it will fall back to `{{ .machine.name }}`.
The templating has the following data available:
* `.machine.name`: The name of the Machine object.
The templating also has the following funcs available:
* `trimSuffix`: same as strings.TrimSuffix
* `trunc`: truncates a string, e.g. `trunc 2 "hello"` or `trunc -2 "hello"`
Notes:
* While the template offers some flexibility, we would like the name to link to the Machine name
to ensure better user experience when troubleshooting
* Generated names must be valid Kubernetes names as they are used to create a VSphereVM object
and usually also as the name of the Node object.
* Names are automatically truncated at 63 characters. Please note that this can lead to name conflicts,
so we highly recommend to use a template which leads to a name shorter than 63 characters.
type: string
type: object
network:
description: Network is the network configuration for this
machine's VM.
Expand Down
10 changes: 10 additions & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ require (
)

require (
github.com/Masterminds/sprig/v3 v3.3.0
github.com/blang/semver/v4 v4.0.0
github.com/go-logr/logr v1.4.2
github.com/google/gofuzz v1.2.0
Expand Down Expand Up @@ -45,6 +46,9 @@ require (
)

require (
dario.cat/mergo v1.0.1 // indirect
github.com/Masterminds/goutils v1.1.1 // indirect
github.com/Masterminds/semver/v3 v3.3.0 // indirect
Comment on lines +49 to +51
Copy link
Member

Choose a reason for hiding this comment

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

These dependencies are something we might want to prevent having.

Copy link
Author

@viveksyngh viveksyngh Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think these came up from sprig library.

Copy link
Member

Choose a reason for hiding this comment

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

Just noted: This now adds a new dependency to github.com/Masterminds/sprig/v3, while we currently only have a go dependency to github.com/go-task/slim-sprig/v3.

github.com/NYTimes/gziphandler v1.1.1 // indirect
github.com/antlr4-go/antlr/v4 v4.13.0 // indirect
github.com/asaskevich/govalidator v0.0.0-20190424111038-f61b66f89f4a // indirect
Expand All @@ -58,8 +62,13 @@ require (
github.com/google/gnostic-models v0.6.8 // indirect
github.com/google/pprof v0.0.0-20241210010833-40e02aabc2ad // indirect
github.com/grpc-ecosystem/grpc-gateway/v2 v2.20.0 // indirect
github.com/huandu/xstrings v1.5.0 // indirect
github.com/mitchellh/copystructure v1.2.0 // indirect
github.com/mitchellh/reflectwalk v1.0.2 // indirect
Comment on lines +66 to +67
Copy link
Member

Choose a reason for hiding this comment

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

These dependencies are even archived and not maintained anymore.

Copy link
Member

Choose a reason for hiding this comment

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

cc @sbueringer , not sure if we should instead do the third_party approach with "minimal" change 🤔

Copy link
Member

@chrischdi chrischdi Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

xref #3286 (comment)

We want to import a different sprig so this may be gone then

github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect
github.com/shopspring/decimal v1.4.0 // indirect
github.com/spf13/cast v1.7.0 // indirect
github.com/stretchr/objx v0.5.2 // indirect
github.com/x448/float16 v0.8.4 // indirect
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.53.0 // indirect
Expand All @@ -72,6 +81,7 @@ require (
go.opentelemetry.io/proto/otlp v1.3.1 // indirect
go.uber.org/multierr v1.11.0 // indirect
go.uber.org/zap v1.27.0 // indirect
golang.org/x/crypto v0.32.0 // indirect
golang.org/x/net v0.34.0 // indirect
golang.org/x/oauth2 v0.24.0 // indirect
golang.org/x/sync v0.10.0 // indirect
Expand Down
2 changes: 2 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ github.com/evanphx/json-patch/v5 v5.9.0 h1:kcBlZQbplgElYIlo/n1hJbls2z/1awpXxpRi0
github.com/evanphx/json-patch/v5 v5.9.0/go.mod h1:VNkHZ/282BpEyt/tObQO8s5CMPmYYq14uClGH4abBuQ=
github.com/felixge/httpsnoop v1.0.4 h1:NFTV2Zj1bL4mc9sqWACXbQFVBBg2W3GPvqp8/ESS2Wg=
github.com/felixge/httpsnoop v1.0.4/go.mod h1:m8KPJKqk1gH5J9DgRY2ASl2lWCfGKXixSwevea8zH2U=
github.com/frankban/quicktest v1.14.6 h1:7Xjx+VpznH+oBnejlPUj8oUpdxnVs4f8XU8WnHkI4W8=
github.com/frankban/quicktest v1.14.6/go.mod h1:4ptaffx2x8+WTWXmUCuVU6aPUX1/Mz7zb5vbUoiM6w0=
github.com/fsnotify/fsnotify v1.4.7/go.mod h1:jwhsz4b93w/PPRr/qN1Yymfu8t87LnFCMoQvtojpjFo=
github.com/fsnotify/fsnotify v1.7.0 h1:8JEhPFa5W2WU7YfeZzPNqzMP6Lwt7L2715Ggo0nosvA=
github.com/fsnotify/fsnotify v1.7.0/go.mod h1:40Bi/Hjc2AVfZrqy+aj+yEI+/bRxZnMJyTJwOpGvigM=
Expand Down
43 changes: 42 additions & 1 deletion internal/webhooks/vspheremachinetemplate.go
View file
Open in desktop
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 {
Copy link
Member

Choose a reason for hiding this comment

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

is this following some code that core CAPI already has or is this done from scratch as a use case for CAPV?
if the former, could you link to that code for reviewer's reference?

Copy link
Author

Choose a reason for hiding this comment

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

This is based on this MR which was added to support this in supervisor mode #3099

In supervisor mode, the object is called VirtualMachine which drives the creation of vSphere VMs while in govami mode this object is called VSphereVM.

Copy link
Member

Choose a reason for hiding this comment

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

looks like a lot comes from that PR.
why not also name it VirtualMachineNamingStrategy here? that seems more consistent despite the diff backend objects.

Copy link
Author

Choose a reason for hiding this comment

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

I can do that if we prefer that. I just to wanted to call it what it is being used for.

Copy link
Member

Choose a reason for hiding this comment

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

that would be my preference. not sure what other reviewers will comment on this.

Copy link
Member

Choose a reason for hiding this comment

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

👍 to keep it as VSphereVM, because that's the object which gets created from a VSphereMachine.

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
}
Loading
Loading