diff --git a/apis/vmware/v1beta1/vspheremachine_types.go b/apis/vmware/v1beta1/vspheremachine_types.go index df16ff3db0..6014d7f115 100644 --- a/apis/vmware/v1beta1/vspheremachine_types.go +++ b/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. diff --git a/config/supervisor/crd/bases/vmware.infrastructure.cluster.x-k8s.io_vspheremachines.yaml b/config/supervisor/crd/bases/vmware.infrastructure.cluster.x-k8s.io_vspheremachines.yaml index 5cf223a242..4501f00265 100644 --- a/config/supervisor/crd/bases/vmware.infrastructure.cluster.x-k8s.io_vspheremachines.yaml +++ b/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: diff --git a/config/supervisor/crd/bases/vmware.infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml b/config/supervisor/crd/bases/vmware.infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml index cb3cf05d48..4c25222194 100644 --- a/config/supervisor/crd/bases/vmware.infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml +++ b/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 diff --git a/go.mod b/go.mod index ed0066cc84..80297d4259 100644 --- a/go.mod +++ b/go.mod @@ -106,7 +106,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 diff --git a/internal/webhooks/vmware/vspheremachinetemplate.go b/internal/webhooks/vmware/vspheremachinetemplate.go index b004eaf1c3..10677bff4e 100644 --- a/internal/webhooks/vmware/vspheremachinetemplate.go +++ b/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,33 @@ 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, apierrors.NewInvalid(vmwarev1.GroupVersion.WithKind("VSphereMachineTemplate").GroupKind(), newObj.Name, field.ErrorList{ + 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) { diff --git a/internal/webhooks/vmware/vspheremachinetemplate_test.go b/internal/webhooks/vmware/vspheremachinetemplate_test.go index 52104f42a5..adf8ea923b 100644 --- a/internal/webhooks/vmware/vspheremachinetemplate_test.go +++ b/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()) + }) + } +} diff --git a/pkg/util/equal.go b/pkg/util/equal.go new file mode 100644 index 0000000000..ac6fadea2a --- /dev/null +++ b/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 +} diff --git a/pkg/util/equal_test.go b/pkg/util/equal_test.go new file mode 100644 index 0000000000..4ebcbcbd3c --- /dev/null +++ b/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)) + } + }) + } +}