Remove the CEL validation on VSphereMachineTemplate.spec.network

When the Cluster API topology controller does a dry-run patch on
the VSphereMachineTemplate to figure out if there are changes to
apply, it fails with below error:

server side apply dry-run failed for modified object:
VSphereMachineTemplate.vmware.infrastructure.cluster.x-k8s.io
"guestcluster1-rhhvs" is invalid: spec.template.spec: Invalid value:
"object": field ''network'' cannot be added or removed after
creation

Move diff validation on VSphereMachineTemplate spec to Webhook and
Skip validation if it 's triggerd by server side dry-run.

Author: DanielXiao

apis/vmware/v1beta1/vspheremachine_types.go

@@ -38,7 +38,6 @@ type VSphereMachineVolume struct {
 }
 
 // VSphereMachineSpec defines the desired state of VSphereMachine.
-// +kubebuilder:validation:XValidation:rule="has(self.network) == has(oldSelf.network)",message="field 'network' cannot be added or removed after creation"
 type VSphereMachineSpec struct {
 	// ProviderID is the virtual machine's BIOS UUID formatted as
 	// vsphere://12345678-1234-1234-1234-123456789abc.
@@ -102,7 +101,6 @@ type VSphereMachineSpec struct {
 }
 
 // VSphereMachineNetworkSpec defines the network configuration of a VSphereMachine.
-// +kubebuilder:validation:XValidation:rule="has(self.interfaces) == has(oldSelf.interfaces)",message="field 'interfaces' cannot be added or removed after creation"
 // +kubebuilder:validation:MinProperties=1
 type VSphereMachineNetworkSpec struct {
 	// interfaces is the list of network interfaces attached to this VSphereMachine.
@@ -117,7 +115,6 @@ func (r *VSphereMachineNetworkSpec) IsDefined() bool {
 }
 
 // InterfacesSpec defines all the network interfaces of a VSphereMachine from Kubernetes perspective.
-// +kubebuilder:validation:XValidation:rule="has(self.primary) == has(oldSelf.primary)",message="field 'primary' cannot be added or removed after creation"
 // +kubebuilder:validation:MinProperties=1
 type InterfacesSpec struct {
 	// primary is the primary network interface.

config/supervisor/crd/bases/vmware.infrastructure.cluster.x-k8s.io_vspheremachines.yaml

@@ -298,13 +298,7 @@ spec:
                         type: array
                         x-kubernetes-list-type: atomic
                     type: object
-                    x-kubernetes-validations:
-                    - message: field 'primary' cannot be added or removed after creation
-                      rule: has(self.primary) == has(oldSelf.primary)
                 type: object
-                x-kubernetes-validations:
-                - message: field 'interfaces' cannot be added or removed after creation
-                  rule: has(self.interfaces) == has(oldSelf.interfaces)
               powerOffMode:
                 default: hard
                 description: |-
@@ -366,9 +360,6 @@ spec:
             - className
             - imageName
             type: object
-            x-kubernetes-validations:
-            - message: field 'network' cannot be added or removed after creation
-              rule: has(self.network) == has(oldSelf.network)
           status:
             description: VSphereMachineStatus defines the observed state of VSphereMachine.
             properties:

config/supervisor/crd/bases/vmware.infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml

@@ -295,15 +295,7 @@ spec:
                                 type: array
                                 x-kubernetes-list-type: atomic
                             type: object
-                            x-kubernetes-validations:
-                            - message: field 'primary' cannot be added or removed
-                                after creation
-                              rule: has(self.primary) == has(oldSelf.primary)
                         type: object
-                        x-kubernetes-validations:
-                        - message: field 'interfaces' cannot be added or removed after
-                            creation
-                          rule: has(self.interfaces) == has(oldSelf.interfaces)
                       powerOffMode:
                         default: hard
                         description: |-
@@ -365,9 +357,6 @@ spec:
                     - className
                     - imageName
                     type: object
-                    x-kubernetes-validations:
-                    - message: field 'network' cannot be added or removed after creation
-                      rule: has(self.network) == has(oldSelf.network)
                 required:
                 - spec
                 type: object

go.mod

@@ -105,7 +105,7 @@ require (
 	github.com/gobuffalo/flect v1.0.3 // indirect
 	github.com/gogo/protobuf v1.3.2 // indirect
 	github.com/google/cel-go v0.23.2 // indirect
-	github.com/google/go-cmp v0.7.0 // indirect
+	github.com/google/go-cmp v0.7.0
 	github.com/inconshreveable/mousetrap v1.1.0 // indirect
 	github.com/josharian/intern v1.0.0 // indirect
 	github.com/json-iterator/go v1.1.12 // indirect

internal/webhooks/vmware/vspheremachinetemplate.go

@@ -25,12 +25,14 @@ import (
 	"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"
 
 	vmwarev1 "sigs.k8s.io/cluster-api-provider-vsphere/apis/vmware/v1beta1"
 	"sigs.k8s.io/cluster-api-provider-vsphere/pkg/services/vmoperator"
+	"sigs.k8s.io/cluster-api-provider-vsphere/pkg/util"
 )
 
 // +kubebuilder:webhook:verbs=create;update,path=/validate-vmware-infrastructure-cluster-x-k8s-io-v1beta1-vspheremachinetemplate,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=vmware.infrastructure.cluster.x-k8s.io,resources=vspheremachinetemplates,versions=v1beta1,name=validation.vspheremachinetemplate.vmware.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1
@@ -60,12 +62,31 @@ func (webhook *VSphereMachineTemplate) ValidateCreate(ctx context.Context, obj r
 }
 
 // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type.
-func (webhook *VSphereMachineTemplate) ValidateUpdate(ctx context.Context, _, newRaw runtime.Object) (admission.Warnings, error) {
-	vSphereMachineTemplate, ok := newRaw.(*vmwarev1.VSphereMachineTemplate)
+func (webhook *VSphereMachineTemplate) ValidateUpdate(ctx context.Context, oldRaw runtime.Object, newRaw runtime.Object) (admission.Warnings, error) {
+	newObj, ok := newRaw.(*vmwarev1.VSphereMachineTemplate)
 	if !ok {
 		return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a VSphereMachineTemplate but got a %T", newRaw))
 	}
-	return webhook.validate(ctx, nil, vSphereMachineTemplate)
+	oldObj, ok := oldRaw.(*vmwarev1.VSphereMachineTemplate)
+	if !ok {
+		return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a VSphereMachineTemplate but got a %T", oldRaw))
+	}
+
+	req, err := admission.RequestFromContext(ctx)
+	if err != nil {
+		return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a admission.Request inside context: %v", err))
+	}
+	if !topology.IsDryRunRequest(req, newObj) {
+		equal, diff, err := util.Diff(oldObj.Spec.Template.Spec, newObj.Spec.Template.Spec)
+		if err != nil {
+			return nil, apierrors.NewBadRequest(fmt.Sprintf("failed to compare old and new VSphereMachineTemplate: %v", err))
+		}
+		if !equal {
+			return nil, field.Invalid(field.NewPath("spec", "template", "spec"), newObj, fmt.Sprintf("VSphereMachineTemplate spec.template.spec field is immutable. Please create a new resource instead. Diff: %s", diff))
+		}
+	}
+
+	return webhook.validate(ctx, nil, newObj)
 }
 
 func (webhook *VSphereMachineTemplate) validate(_ context.Context, _, newVSphereMachineTemplate *vmwarev1.VSphereMachineTemplate) (admission.Warnings, error) {

internal/webhooks/vmware/vspheremachinetemplate_test.go

@@ -21,8 +21,12 @@ import (
 	"testing"
 
 	. "github.com/onsi/gomega"
+	admissionv1 "k8s.io/api/admission/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	featuregatetesting "k8s.io/component-base/featuregate/testing"
 	"k8s.io/utils/ptr"
+	clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	vmwarev1 "sigs.k8s.io/cluster-api-provider-vsphere/apis/vmware/v1beta1"
 	"sigs.k8s.io/cluster-api-provider-vsphere/feature"
@@ -303,3 +307,96 @@ func TestVSphereMachineTemplate_ValidateInterfaces(t *testing.T) {
 		})
 	}
 }
+
+func TestVSphereMachineTemplate_ValidateUpdate_Immutability(t *testing.T) {
+	oldTemplate := vmwarev1.VSphereMachineTemplate{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-template",
+			Namespace: "default",
+		},
+		Spec: vmwarev1.VSphereMachineTemplateSpec{
+			Template: vmwarev1.VSphereMachineTemplateResource{
+				Spec: vmwarev1.VSphereMachineSpec{
+					ImageName:    "ubuntu-20.04",
+					ClassName:    "best-effort-small",
+					StorageClass: "fast-storage",
+				},
+			},
+		},
+	}
+
+	newTemplate := oldTemplate.DeepCopy()
+	newTemplate.Spec.Template.Spec.ImageName = "ubuntu-22.04"
+
+	newTemplateSkipImmutabilityAnnotationSet := newTemplate.DeepCopy()
+	newTemplateSkipImmutabilityAnnotationSet.SetAnnotations(map[string]string{clusterv1.TopologyDryRunAnnotation: ""})
+
+	tests := []struct {
+		name        string
+		newTemplate *vmwarev1.VSphereMachineTemplate
+		oldTemplate *vmwarev1.VSphereMachineTemplate
+		req         *admission.Request
+		wantError   bool
+		wantErrMsg  string
+	}{
+		{
+			name:        "return no error if no modification",
+			newTemplate: &oldTemplate,
+			oldTemplate: &oldTemplate,
+			req:         &admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{DryRun: ptr.To(false)}},
+			wantError:   false,
+		},
+		{
+			name:        "don't allow modification of spec.template.spec",
+			newTemplate: newTemplate,
+			oldTemplate: &oldTemplate,
+			req:         &admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{DryRun: ptr.To(false)}},
+			wantError:   true,
+			wantErrMsg:  "VSphereMachineTemplate spec.template.spec field is immutable",
+		},
+		{
+			name:        "don't allow modification even with skip immutability annotation when not dry run",
+			newTemplate: newTemplateSkipImmutabilityAnnotationSet,
+			oldTemplate: &oldTemplate,
+			req:         &admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{DryRun: ptr.To(false)}},
+			wantError:   true,
+			wantErrMsg:  "VSphereMachineTemplate spec.template.spec field is immutable",
+		},
+		{
+			name:        "don't allow modification when dry run but no skip immutability annotation",
+			newTemplate: newTemplate,
+			oldTemplate: &oldTemplate,
+			req:         &admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{DryRun: ptr.To(true)}},
+			wantError:   true,
+			wantErrMsg:  "VSphereMachineTemplate spec.template.spec field is immutable",
+		},
+		{
+			name:        "skip immutability check when dry run and skip immutability annotation set",
+			newTemplate: newTemplateSkipImmutabilityAnnotationSet,
+			oldTemplate: &oldTemplate,
+			req:         &admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{DryRun: ptr.To(true)}},
+			wantError:   false,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewWithT(t)
+			webhook := &VSphereMachineTemplate{}
+			ctx := context.Background()
+			if tc.req != nil {
+				ctx = admission.NewContextWithRequest(ctx, *tc.req)
+			}
+			warnings, err := webhook.ValidateUpdate(ctx, tc.oldTemplate, tc.newTemplate)
+			if tc.wantError {
+				g.Expect(err).To(HaveOccurred())
+				if tc.wantErrMsg != "" {
+					g.Expect(err.Error()).To(ContainSubstring(tc.wantErrMsg))
+				}
+			} else {
+				g.Expect(err).NotTo(HaveOccurred())
+			}
+			g.Expect(warnings).To(BeEmpty())
+		})
+	}
+}

pkg/util/equal.go

@@ -0,0 +1,59 @@
+/*
+Copyright 2024 The Kubernetes 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 util contains utils to compare objects.
+package util
+
+import (
+	"strings"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/pkg/errors"
+)
+
+// Diff uses cmp.Diff to compare x and y.
+// If cmp.Diff panics, Equal returns an error.
+func Diff(x any, y any) (equal bool, diff string, matchErr error) {
+	defer func() {
+		if r := recover(); r != nil {
+			equal = false
+			if err, ok := r.(error); ok {
+				matchErr = errors.Wrapf(err, "error diffing objects")
+				return
+			}
+
+			if errMsg, ok := r.(string); ok {
+				matchErr = errors.Errorf("error diffing objects: %s", errMsg)
+				return
+			}
+
+			matchErr = errors.Errorf("error diffing objects: panic of unknown type %T", r)
+		}
+	}()
+
+	diff = cmp.Diff(x, y)
+
+	if diff != "" {
+		// Replace non-breaking space (NBSP) through a regular space.
+		// This prevents output like this: "\u00a0\u00a0int(\n-\u00a0\t1,\n+\u00a0\t2,\n\u00a0\u00a0)\n"
+		diff = strings.ReplaceAll(diff, "\u00a0", " ")
+		// Replace \t through "  " because it's easier to read in log output
+		diff = strings.ReplaceAll(diff, "\t", "  ")
+		diff = strings.TrimSpace(diff)
+	}
+
+	return diff == "", diff, nil
+}

pkg/util/equal_test.go

@@ -0,0 +1,94 @@
+/*
+Copyright 2024 The Kubernetes 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 util
+
+import (
+	"testing"
+
+	. "github.com/onsi/gomega"
+	clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2"
+)
+
+func TestDiff(t *testing.T) {
+	type test struct {
+		name      string
+		x, y      interface{}
+		wantEqual bool
+		wantDiff  string
+		wantError string
+	}
+
+	tests := []test{
+		{
+			name:      "Equal integers, no diff",
+			x:         1,
+			y:         1,
+			wantEqual: true,
+		},
+		{
+			name:      "Different integers, diff",
+			x:         1,
+			y:         2,
+			wantEqual: false,
+			wantDiff: `int(
+-   1,
++   2,
+  )`,
+		},
+		{
+			name: "Different labels, diff",
+			x: map[string]string{
+				clusterv1.ClusterNameLabel:          "cluster-1",
+				clusterv1.ClusterTopologyOwnedLabel: "",
+			},
+			y: map[string]string{
+				clusterv1.ClusterNameLabel: "cluster-2",
+			},
+			wantEqual: false,
+			wantDiff: `map[string]string{
+-   "cluster.x-k8s.io/cluster-name":   "cluster-1",
++   "cluster.x-k8s.io/cluster-name":   "cluster-2",
+-   "topology.cluster.x-k8s.io/owned": "",
+  }`,
+		},
+		{
+			name: "Diff unexported fields, error",
+			x:    struct{ a, b, c int }{1, 2, 3},
+			y:    struct{ a, b, c int }{1, 2, 4},
+			wantError: `error diffing objects: cannot handle unexported field at root.a:
+	"sigs.k8s.io/cluster-api-provider-vsphere/pkg/util".(struct { a int; b int; c int })
+consider using a custom Comparer; if you control the implementation of type, you can also consider using an Exporter, AllowUnexported, or cmpopts.IgnoreUnexported`,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			gotEqual, gotDiff, err := Diff(tt.x, tt.y)
+
+			if tt.wantError != "" {
+				g.Expect(err.Error()).To(BeComparableTo(tt.wantError))
+			} else {
+				g.Expect(err).ToNot(HaveOccurred())
+
+				g.Expect(gotEqual).To(Equal(tt.wantEqual))
+				g.Expect(gotDiff).To(BeComparableTo(tt.wantDiff))
+			}
+		})
+	}
+}