diff --git a/deploy/kubernetes/webhook-example/admission-configuration-template b/deploy/kubernetes/webhook-example/admission-configuration-template index 5825adc64..3d8ccc63c 100644 --- a/deploy/kubernetes/webhook-example/admission-configuration-template +++ b/deploy/kubernetes/webhook-example/admission-configuration-template @@ -8,7 +8,7 @@ webhooks: - apiGroups: ["snapshot.storage.k8s.io"] apiVersions: ["v1"] operations: ["CREATE", "UPDATE"] - resources: ["volumesnapshots", "volumesnapshotcontents", "volumesnapshotclasses"] + resources: ["volumesnapshotclasses"] scope: "*" clientConfig: service: @@ -31,7 +31,7 @@ webhooks: - apiGroups: ["groupsnapshot.storage.k8s.io"] apiVersions: ["v1alpha1"] operations: ["CREATE", "UPDATE"] - resources: ["volumegroupsnapshots", "volumegroupsnapshotcontents", "volumegroupsnapshotclasses"] + resources: ["volumegroupsnapshotclasses"] scope: "*" clientConfig: service: diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index b5419e21a..f3cb8c884 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -36,7 +36,6 @@ import ( crdv1 "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumesnapshot/v1" "github.com/kubernetes-csi/external-snapshotter/v7/pkg/metrics" "github.com/kubernetes-csi/external-snapshotter/v7/pkg/utils" - webhook "github.com/kubernetes-csi/external-snapshotter/v7/pkg/validation-webhook" ) // ================================================================== @@ -91,14 +90,6 @@ func (ctrl *csiSnapshotCommonController) syncContent(content *crdv1.VolumeSnapsh klog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: content is bound to snapshot %s", content.Name, snapshotName) klog.V(5).Infof("syncContent[%s]: check if we should add invalid label on content", content.Name) - // Perform additional validation. Label objects which fail. - // Part of a plan to tighten validation, this label will enable users to - // query for invalid content objects. See issue #363 - content, err := ctrl.checkAndSetInvalidContentLabel(content) - if err != nil { - klog.Errorf("syncContent[%s]: check and add invalid content label failed, %s", content.Name, err.Error()) - return err - } // Keep this check in the controller since the validation webhook may not have been deployed. if (content.Spec.Source.VolumeHandle == nil && content.Spec.Source.SnapshotHandle == nil) || @@ -128,7 +119,7 @@ func (ctrl *csiSnapshotCommonController) syncContent(content *crdv1.VolumeSnapsh // and it may have already been deleted, and it will fall into the // snapshot == nil case below var snapshot *crdv1.VolumeSnapshot - snapshot, err = ctrl.getSnapshotFromStore(snapshotName) + snapshot, err := ctrl.getSnapshotFromStore(snapshotName) if err != nil { return err } @@ -195,14 +186,6 @@ func (ctrl *csiSnapshotCommonController) syncSnapshot(snapshot *crdv1.VolumeSnap } klog.V(5).Infof("syncSnapshot[%s]: check if we should add invalid label on snapshot", utils.SnapshotKey(snapshot)) - // Perform additional validation. Label objects which fail. - // Part of a plan to tighten validation, this label will enable users to - // query for invalid snapshot objects. See issue #363 - snapshot, err := ctrl.checkAndSetInvalidSnapshotLabel(snapshot) - if err != nil { - klog.Errorf("syncSnapshot[%s]: check and add invalid snapshot label failed, %s", utils.SnapshotKey(snapshot), err.Error()) - return err - } // Proceed with snapshot deletion and remove finalizers when needed if snapshot.ObjectMeta.DeletionTimestamp != nil { @@ -1654,101 +1637,6 @@ func (ctrl *csiSnapshotCommonController) setAnnVolumeSnapshotBeingDeleted(conten return content, nil } -// checkAndSetInvalidContentLabel adds a label to unlabeled invalid content objects and removes the label from valid ones. -func (ctrl *csiSnapshotCommonController) checkAndSetInvalidContentLabel(content *crdv1.VolumeSnapshotContent) (*crdv1.VolumeSnapshotContent, error) { - hasLabel := utils.MapContainsKey(content.ObjectMeta.Labels, utils.VolumeSnapshotContentInvalidLabel) - err := webhook.ValidateV1SnapshotContent(content) - if err != nil { - klog.Errorf("syncContent[%s]: Invalid content detected, %s", content.Name, err.Error()) - } - // If the snapshot content correctly has the label, or correctly does not have the label, take no action. - if hasLabel && err != nil || !hasLabel && err == nil { - return content, nil - } - - var patches []utils.PatchOp - contentClone := content.DeepCopy() - if hasLabel { - // Need to remove the label - patches = append(patches, utils.PatchOp{ - Op: "remove", - Path: "/metadata/labels/" + utils.VolumeSnapshotContentInvalidLabel, - }) - - } else { - // Snapshot content is invalid and does not have the label. Need to add the label - patches = append(patches, utils.PatchOp{ - Op: "add", - Path: "/metadata/labels/" + utils.VolumeSnapshotContentInvalidLabel, - Value: "", - }) - } - updatedContent, err := utils.PatchVolumeSnapshotContent(contentClone, patches, ctrl.clientset) - if err != nil { - return content, newControllerUpdateError(content.Name, err.Error()) - } - - _, err = ctrl.storeContentUpdate(updatedContent) - if err != nil { - klog.Errorf("failed to update content store %v", err) - } - - if hasLabel { - klog.V(5).Infof("Removed invalid content label from volume snapshot content %s", content.Name) - } else { - klog.V(5).Infof("Added invalid content label to volume snapshot content %s", content.Name) - } - return updatedContent, nil -} - -// checkAndSetInvalidSnapshotLabel adds a label to unlabeled invalid snapshot objects and removes the label from valid ones. -func (ctrl *csiSnapshotCommonController) checkAndSetInvalidSnapshotLabel(snapshot *crdv1.VolumeSnapshot) (*crdv1.VolumeSnapshot, error) { - hasLabel := utils.MapContainsKey(snapshot.ObjectMeta.Labels, utils.VolumeSnapshotInvalidLabel) - err := webhook.ValidateV1Snapshot(snapshot) - if err != nil { - klog.Errorf("syncSnapshot[%s]: Invalid snapshot detected, %s", utils.SnapshotKey(snapshot), err.Error()) - } - // If the snapshot correctly has the label, or correctly does not have the label, take no action. - if hasLabel && err != nil || !hasLabel && err == nil { - return snapshot, nil - } - - var patches []utils.PatchOp - snapshotClone := snapshot.DeepCopy() - if hasLabel { - // Need to remove the label - patches = append(patches, utils.PatchOp{ - Op: "remove", - Path: "/metadata/labels/" + utils.VolumeSnapshotInvalidLabel, - }) - } else { - // Snapshot is invalid and does not have the label. Need to add the label - patches = append(patches, utils.PatchOp{ - Op: "add", - Path: "/metadata/labels/" + utils.VolumeSnapshotInvalidLabel, - Value: "", - }) - } - - updatedSnapshot, err := utils.PatchVolumeSnapshot(snapshotClone, patches, ctrl.clientset) - if err != nil { - return snapshot, newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error()) - } - - _, err = ctrl.storeSnapshotUpdate(updatedSnapshot) - if err != nil { - klog.Errorf("failed to update snapshot store %v", err) - } - - if hasLabel { - klog.V(5).Infof("Removed invalid snapshot label from volume snapshot %s", utils.SnapshotKey(snapshot)) - } else { - klog.V(5).Infof("Added invalid snapshot label to volume snapshot %s", utils.SnapshotKey(snapshot)) - } - - return updatedSnapshot, nil -} - func (ctrl *csiSnapshotCommonController) getManagedByNode(pv *v1.PersistentVolume) (string, error) { if pv.Spec.NodeAffinity == nil { klog.V(5).Infof("NodeAffinity not set for pv %s", pv.Name) diff --git a/pkg/validation-webhook/groupsnapshot.go b/pkg/validation-webhook/groupsnapshot.go index dc7375d3a..246551083 100644 --- a/pkg/validation-webhook/groupsnapshot.go +++ b/pkg/validation-webhook/groupsnapshot.go @@ -18,7 +18,6 @@ package webhook import ( "fmt" - "reflect" volumegroupsnapshotv1alpha1 "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumegroupsnapshot/v1alpha1" groupsnapshotlisters "github.com/kubernetes-csi/external-snapshotter/client/v7/listers/volumegroupsnapshot/v1alpha1" @@ -30,10 +29,6 @@ import ( ) var ( - // GroupSnapshotV1Alpha1GVR is GroupVersionResource for v1alpha1 VolumeGroupSnapshots - GroupSnapshotV1Alpha1GVR = metav1.GroupVersionResource{Group: volumegroupsnapshotv1alpha1.GroupName, Version: "v1alpha1", Resource: "volumegroupsnapshots"} - // GroupSnapshotContentV1Apha1GVR is GroupVersionResource for v1alpha1 VolumeGroupSnapshotContents - GroupSnapshotContentV1Apha1GVR = metav1.GroupVersionResource{Group: volumegroupsnapshotv1alpha1.GroupName, Version: "v1alpha1", Resource: "volumegroupsnapshotcontents"} // GroupSnapshotClassV1Apha1GVR is GroupVersionResource for v1alpha1 VolumeGroupSnapshotClasses GroupSnapshotClassV1Apha1GVR = metav1.GroupVersionResource{Group: volumegroupsnapshotv1alpha1.GroupName, Version: "v1alpha1", Resource: "volumegroupsnapshotclasses"} ) @@ -54,8 +49,7 @@ func NewGroupSnapshotAdmitter(lister groupsnapshotlisters.VolumeGroupSnapshotCla // Add a label {"added-label": "yes"} to the object func (a groupSnapshotAdmitter) Admit(ar v1.AdmissionReview) *v1.AdmissionResponse { - klog.V(2).Info("admitting volumegroupsnapshots volumegroupsnapshotcontents " + - "or volumegroupsnapshotclasses") + klog.V(2).Info("admitting volumegroupsnapshotclasses") reviewResponse := &v1.AdmissionResponse{ Allowed: true, @@ -66,37 +60,12 @@ func (a groupSnapshotAdmitter) Admit(ar v1.AdmissionReview) *v1.AdmissionRespons if !(ar.Request.Operation == v1.Update || ar.Request.Operation == v1.Create) { return reviewResponse } - isUpdate := ar.Request.Operation == v1.Update raw := ar.Request.Object.Raw oldRaw := ar.Request.OldObject.Raw deserializer := codecs.UniversalDeserializer() switch ar.Request.Resource { - case GroupSnapshotV1Alpha1GVR: - groupSnapshot := &volumegroupsnapshotv1alpha1.VolumeGroupSnapshot{} - if _, _, err := deserializer.Decode(raw, nil, groupSnapshot); err != nil { - klog.Error(err) - return toV1AdmissionResponse(err) - } - oldGroupSnapshot := &volumegroupsnapshotv1alpha1.VolumeGroupSnapshot{} - if _, _, err := deserializer.Decode(oldRaw, nil, oldGroupSnapshot); err != nil { - klog.Error(err) - return toV1AdmissionResponse(err) - } - return decideGroupSnapshotV1Alpha1(groupSnapshot, oldGroupSnapshot, isUpdate) - case GroupSnapshotContentV1Apha1GVR: - groupSnapContent := &volumegroupsnapshotv1alpha1.VolumeGroupSnapshotContent{} - if _, _, err := deserializer.Decode(raw, nil, groupSnapContent); err != nil { - klog.Error(err) - return toV1AdmissionResponse(err) - } - oldGroupSnapContent := &volumegroupsnapshotv1alpha1.VolumeGroupSnapshotContent{} - if _, _, err := deserializer.Decode(oldRaw, nil, oldGroupSnapContent); err != nil { - klog.Error(err) - return toV1AdmissionResponse(err) - } - return decideGroupSnapshotContentV1Alpha1(groupSnapContent, oldGroupSnapContent, isUpdate) case GroupSnapshotClassV1Apha1GVR: groupSnapClass := &volumegroupsnapshotv1alpha1.VolumeGroupSnapshotClass{} if _, _, err := deserializer.Decode(raw, nil, groupSnapClass); err != nil { @@ -110,60 +79,13 @@ func (a groupSnapshotAdmitter) Admit(ar v1.AdmissionReview) *v1.AdmissionRespons } return decideGroupSnapshotClassV1Alpha1(groupSnapClass, oldGroupSnapClass, a.lister) default: - err := fmt.Errorf("expect resource to be %s, %s, or %s, but found %v", - GroupSnapshotV1Alpha1GVR, GroupSnapshotContentV1Apha1GVR, + err := fmt.Errorf("expect resource to be %s, but found %v", GroupSnapshotClassV1Apha1GVR, ar.Request.Resource) klog.Error(err) return toV1AdmissionResponse(err) } } -func decideGroupSnapshotV1Alpha1(groupSnapshot, oldGroupSnapshot *volumegroupsnapshotv1alpha1.VolumeGroupSnapshot, isUpdate bool) *v1.AdmissionResponse { - reviewResponse := &v1.AdmissionResponse{ - Allowed: true, - Result: &metav1.Status{}, - } - - if isUpdate { - // if it is an UPDATE and oldGroupSnapshot is valid, check immutable fields - if err := checkGroupSnapshotImmutableFieldsV1Alpha1(groupSnapshot, oldGroupSnapshot); err != nil { - reviewResponse.Allowed = false - reviewResponse.Result.Message = err.Error() - return reviewResponse - } - } - // Enforce strict validation for CREATE requests. Immutable checks don't apply for CREATE requests. - // Enforce strict validation for UPDATE requests where old is valid and passes immutability check. - if err := ValidateV1Alpha1GroupSnapshot(groupSnapshot); err != nil { - reviewResponse.Allowed = false - reviewResponse.Result.Message = err.Error() - } - return reviewResponse -} - -func decideGroupSnapshotContentV1Alpha1(groupSnapcontent, oldGroupSnapcontent *volumegroupsnapshotv1alpha1.VolumeGroupSnapshotContent, isUpdate bool) *v1.AdmissionResponse { - reviewResponse := &v1.AdmissionResponse{ - Allowed: true, - Result: &metav1.Status{}, - } - - if isUpdate { - // if it is an UPDATE and oldGroupSnapcontent is valid, check immutable fields - if err := checkGroupSnapshotContentImmutableFieldsV1Alpha1(groupSnapcontent, oldGroupSnapcontent); err != nil { - reviewResponse.Allowed = false - reviewResponse.Result.Message = err.Error() - return reviewResponse - } - } - // Enforce strict validation for all CREATE requests. Immutable checks don't apply for CREATE requests. - // Enforce strict validation for UPDATE requests where old is valid and passes immutability check. - if err := ValidateV1Alpha1GroupSnapshotContent(groupSnapcontent); err != nil { - reviewResponse.Allowed = false - reviewResponse.Result.Message = err.Error() - } - return reviewResponse -} - func decideGroupSnapshotClassV1Alpha1(groupSnapClass, oldGroupSnapClass *volumegroupsnapshotv1alpha1.VolumeGroupSnapshotClass, lister groupsnapshotlisters.VolumeGroupSnapshotClassLister) *v1.AdmissionResponse { reviewResponse := &v1.AdmissionResponse{ Allowed: true, @@ -200,55 +122,3 @@ func decideGroupSnapshotClassV1Alpha1(groupSnapClass, oldGroupSnapClass *volumeg return reviewResponse } - -func checkGroupSnapshotImmutableFieldsV1Alpha1(groupSnapshot, oldGroupSnapshot *volumegroupsnapshotv1alpha1.VolumeGroupSnapshot) error { - if groupSnapshot == nil { - return fmt.Errorf("VolumeGroupSnapshot is nil") - } - if oldGroupSnapshot == nil { - return fmt.Errorf("old VolumeGroupSnapshot is nil") - } - - source := groupSnapshot.Spec.Source - oldSource := oldGroupSnapshot.Spec.Source - - if !reflect.DeepEqual(source.Selector, oldSource.Selector) { - return fmt.Errorf("Spec.Source.Selector is immutable but was changed from %s to %s", oldSource.Selector, source.Selector) - } - if !reflect.DeepEqual(source.VolumeGroupSnapshotContentName, oldSource.VolumeGroupSnapshotContentName) { - return fmt.Errorf("Spec.Source.VolumeGroupSnapshotContentName is immutable but was changed from %s to %s", strPtrDereference(oldSource.VolumeGroupSnapshotContentName), strPtrDereference(source.VolumeGroupSnapshotContentName)) - } - - return nil -} - -func checkGroupSnapshotContentImmutableFieldsV1Alpha1(groupSnapcontent, oldGroupSnapcontent *volumegroupsnapshotv1alpha1.VolumeGroupSnapshotContent) error { - if groupSnapcontent == nil { - return fmt.Errorf("VolumeGroupSnapshotContent is nil") - } - if oldGroupSnapcontent == nil { - return fmt.Errorf("old VolumeGroupSnapshotContent is nil") - } - - source := groupSnapcontent.Spec.Source - oldSource := oldGroupSnapcontent.Spec.Source - - if !reflect.DeepEqual(source.GroupSnapshotHandles, oldSource.GroupSnapshotHandles) { - return fmt.Errorf("Spec.Source.GroupSnapshotHandles is immutable but was changed from %s to %s", oldSource.GroupSnapshotHandles, source.GroupSnapshotHandles) - } - if !reflect.DeepEqual(source.VolumeHandles, oldSource.VolumeHandles) { - return fmt.Errorf("Spec.Source.VolumeHandles is immutable but was changed from %v to %v", oldSource.VolumeHandles, source.VolumeHandles) - } - - ref := groupSnapcontent.Spec.VolumeGroupSnapshotRef - oldRef := oldGroupSnapcontent.Spec.VolumeGroupSnapshotRef - - if ref.Name != oldRef.Name { - return fmt.Errorf("Spec.VolumeGroupSnapshotRef.Name is immutable but was changed from %s to %s", oldRef.Name, ref.Name) - } - if ref.Namespace != oldRef.Namespace { - return fmt.Errorf("Spec.VolumeGroupSnapshotRef.Namespace is immutable but was changed from %s to %s", oldRef.Namespace, ref.Namespace) - } - - return nil -} diff --git a/pkg/validation-webhook/groupsnapshot_test.go b/pkg/validation-webhook/groupsnapshot_test.go index da4e1c7b1..1caf9a13c 100644 --- a/pkg/validation-webhook/groupsnapshot_test.go +++ b/pkg/validation-webhook/groupsnapshot_test.go @@ -18,14 +18,12 @@ package webhook import ( "encoding/json" - "fmt" "testing" volumegroupsnapshotv1alpha1 "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumegroupsnapshot/v1alpha1" groupsnapshotlisters "github.com/kubernetes-csi/external-snapshotter/client/v7/listers/volumegroupsnapshot/v1alpha1" "github.com/kubernetes-csi/external-snapshotter/v7/pkg/utils" v1 "k8s.io/api/admission/v1" - core_v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" @@ -48,379 +46,6 @@ func (f *fakeGroupSnapshotLister) Get(name string) (*volumegroupsnapshotv1alpha1 return nil, nil } -func TestAdmitVolumeGroupSnapshotV1Alpha1(t *testing.T) { - selector := metav1.LabelSelector{MatchLabels: map[string]string{ - "group": "A", - }} - mutatedField := "changed-immutable-field" - contentname := "groupsnapcontent1" - emptyVolumeGroupSnapshotClassName := "" - - testCases := []struct { - name string - volumeGroupSnapshot *volumegroupsnapshotv1alpha1.VolumeGroupSnapshot - oldVolumeGroupSnapshot *volumegroupsnapshotv1alpha1.VolumeGroupSnapshot - shouldAdmit bool - msg string - operation v1.Operation - }{ - { - name: "Delete: new and old are nil. Should admit", - volumeGroupSnapshot: nil, - oldVolumeGroupSnapshot: nil, - shouldAdmit: true, - operation: v1.Delete, - }, - { - name: "Create: old is nil and new is valid, with contentname", - volumeGroupSnapshot: &volumegroupsnapshotv1alpha1.VolumeGroupSnapshot{ - Spec: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSpec{ - Source: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSource{ - VolumeGroupSnapshotContentName: &contentname, - }, - }, - }, - oldVolumeGroupSnapshot: nil, - shouldAdmit: true, - msg: "", - operation: v1.Create, - }, - { - name: "Create: old is nil and new is valid, with selector", - volumeGroupSnapshot: &volumegroupsnapshotv1alpha1.VolumeGroupSnapshot{ - Spec: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSpec{ - Source: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSource{ - Selector: &selector, - }, - }, - }, - oldVolumeGroupSnapshot: nil, - shouldAdmit: true, - msg: "", - operation: v1.Create, - }, - { - name: "Update: old is valid and new is invalid", - volumeGroupSnapshot: &volumegroupsnapshotv1alpha1.VolumeGroupSnapshot{ - Spec: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSpec{ - Source: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSource{ - VolumeGroupSnapshotContentName: &contentname, - }, - VolumeGroupSnapshotClassName: &emptyVolumeGroupSnapshotClassName, - }, - }, - oldVolumeGroupSnapshot: &volumegroupsnapshotv1alpha1.VolumeGroupSnapshot{ - Spec: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSpec{ - Source: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSource{ - VolumeGroupSnapshotContentName: &contentname, - }, - }, - }, - shouldAdmit: false, - operation: v1.Update, - msg: "Spec.VolumeGroupSnapshotClassName must not be the empty string", - }, - { - name: "Update: old is valid and new is valid", - volumeGroupSnapshot: &volumegroupsnapshotv1alpha1.VolumeGroupSnapshot{ - Spec: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSpec{ - Source: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSource{ - VolumeGroupSnapshotContentName: &contentname, - }, - }, - }, - oldVolumeGroupSnapshot: &volumegroupsnapshotv1alpha1.VolumeGroupSnapshot{ - Spec: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSpec{ - Source: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSource{ - VolumeGroupSnapshotContentName: &contentname, - }, - }, - }, - shouldAdmit: true, - operation: v1.Update, - }, - { - name: "Update: old is valid and new is valid but changes immutable field spec.source", - volumeGroupSnapshot: &volumegroupsnapshotv1alpha1.VolumeGroupSnapshot{ - Spec: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSpec{ - Source: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSource{ - VolumeGroupSnapshotContentName: &mutatedField, - }, - }, - }, - oldVolumeGroupSnapshot: &volumegroupsnapshotv1alpha1.VolumeGroupSnapshot{ - Spec: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSpec{ - Source: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSource{ - VolumeGroupSnapshotContentName: &contentname, - }, - }, - }, - shouldAdmit: false, - operation: v1.Update, - msg: fmt.Sprintf("Spec.Source.VolumeGroupSnapshotContentName is immutable but was changed from %s to %s", contentname, mutatedField), - }, - { - name: "Update: old is invalid and new is valid", - volumeGroupSnapshot: &volumegroupsnapshotv1alpha1.VolumeGroupSnapshot{ - Spec: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSpec{ - Source: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSource{ - VolumeGroupSnapshotContentName: &contentname, - }, - }, - }, - oldVolumeGroupSnapshot: &volumegroupsnapshotv1alpha1.VolumeGroupSnapshot{ - Spec: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSpec{ - Source: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSource{ - VolumeGroupSnapshotContentName: &contentname, - Selector: &selector, - }, - }, - }, - shouldAdmit: false, - operation: v1.Update, - msg: fmt.Sprintf("Spec.Source.Selector is immutable but was changed from %v to %v", &selector, "nil"), - }, - { - // will be handled by schema validation - name: "Update: old is invalid and new is invalid", - volumeGroupSnapshot: &volumegroupsnapshotv1alpha1.VolumeGroupSnapshot{ - Spec: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSpec{ - Source: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSource{ - VolumeGroupSnapshotContentName: &contentname, - Selector: &selector, - }, - }, - }, - oldVolumeGroupSnapshot: &volumegroupsnapshotv1alpha1.VolumeGroupSnapshot{ - Spec: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSpec{ - Source: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSource{ - VolumeGroupSnapshotContentName: &contentname, - Selector: &selector, - }, - }, - }, - shouldAdmit: true, - operation: v1.Update, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - groupSnapshot := tc.volumeGroupSnapshot - raw, err := json.Marshal(groupSnapshot) - if err != nil { - t.Fatal(err) - } - oldGroupSnapshot := tc.oldVolumeGroupSnapshot - oldRaw, err := json.Marshal(oldGroupSnapshot) - if err != nil { - t.Fatal(err) - } - review := v1.AdmissionReview{ - Request: &v1.AdmissionRequest{ - Object: runtime.RawExtension{ - Raw: raw, - }, - OldObject: runtime.RawExtension{ - Raw: oldRaw, - }, - Resource: GroupSnapshotV1Alpha1GVR, - Operation: tc.operation, - }, - } - sa := NewGroupSnapshotAdmitter(nil) - response := sa.Admit(review) - shouldAdmit := response.Allowed - msg := response.Result.Message - - expectedResponse := tc.shouldAdmit - expectedMsg := tc.msg - - if shouldAdmit != expectedResponse { - t.Errorf("expected \"%v\" to equal \"%v\": %v", shouldAdmit, expectedResponse, msg) - } - if msg != expectedMsg { - t.Errorf("expected \"%v\" to equal \"%v\"", msg, expectedMsg) - } - }) - } -} - -func TestAdmitVolumeGroupSnapshotContentV1Alpha1(t *testing.T) { - volumeHandle := "volumeHandle1" - modifiedRefName := "modified-ref-name" - groupSnapshotHandle := "groupsnapshotHandle1" - volumeSnapshotHandles := []string{"volumeSnapshotHandle1"} - groupSnapshotHandles := &volumegroupsnapshotv1alpha1.GroupSnapshotHandles{ - VolumeGroupSnapshotHandle: groupSnapshotHandle, - VolumeSnapshotHandles: volumeSnapshotHandles, - } - modifiedGroupSnapshotHandles := &volumegroupsnapshotv1alpha1.GroupSnapshotHandles{ - VolumeGroupSnapshotHandle: groupSnapshotHandle, - VolumeSnapshotHandles: append(volumeSnapshotHandles, "volumeSnapshotHandle2"), - } - volumeGroupSnapshotClassName := "volume-snapshot-class-1" - validContent := &volumegroupsnapshotv1alpha1.VolumeGroupSnapshotContent{ - Spec: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotContentSpec{ - Source: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotContentSource{ - GroupSnapshotHandles: groupSnapshotHandles, - }, - VolumeGroupSnapshotRef: core_v1.ObjectReference{ - Name: "group-snapshot-ref", - Namespace: "default-ns", - }, - VolumeGroupSnapshotClassName: &volumeGroupSnapshotClassName, - }, - } - invalidContent := &volumegroupsnapshotv1alpha1.VolumeGroupSnapshotContent{ - Spec: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotContentSpec{ - Source: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotContentSource{ - GroupSnapshotHandles: groupSnapshotHandles, - VolumeHandles: []string{volumeHandle}, - }, - VolumeGroupSnapshotRef: core_v1.ObjectReference{ - Name: "", - Namespace: "default-ns", - }}, - } - - testCases := []struct { - name string - groupSnapContent *volumegroupsnapshotv1alpha1.VolumeGroupSnapshotContent - oldGroupSnapContent *volumegroupsnapshotv1alpha1.VolumeGroupSnapshotContent - shouldAdmit bool - msg string - operation v1.Operation - }{ - { - name: "Delete: both new and old are nil", - groupSnapContent: nil, - oldGroupSnapContent: nil, - shouldAdmit: true, - operation: v1.Delete, - }, - { - name: "Create: old is nil and new is valid", - groupSnapContent: validContent, - oldGroupSnapContent: nil, - shouldAdmit: true, - operation: v1.Create, - }, - { - name: "Update: old is valid and new is invalid", - groupSnapContent: invalidContent, - oldGroupSnapContent: validContent, - shouldAdmit: false, - operation: v1.Update, - msg: fmt.Sprintf("Spec.Source.VolumeHandles is immutable but was changed from %s to %s", []string{}, []string{volumeHandle}), - }, - { - name: "Update: old is valid and new is valid", - groupSnapContent: validContent, - oldGroupSnapContent: validContent, - shouldAdmit: true, - operation: v1.Update, - }, - { - name: "Update: old is valid and new is valid but modifies immutable field", - groupSnapContent: &volumegroupsnapshotv1alpha1.VolumeGroupSnapshotContent{ - Spec: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotContentSpec{ - Source: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotContentSource{ - GroupSnapshotHandles: modifiedGroupSnapshotHandles, - }, - VolumeGroupSnapshotRef: core_v1.ObjectReference{ - Name: "snapshot-ref", - Namespace: "default-ns", - }, - }, - }, - oldGroupSnapContent: validContent, - shouldAdmit: false, - operation: v1.Update, - msg: fmt.Sprintf("Spec.Source.GroupSnapshotHandles is immutable but was changed from %s to %s", groupSnapshotHandles, modifiedGroupSnapshotHandles), - }, - { - name: "Update: old is valid and new is valid but modifies immutable ref", - groupSnapContent: &volumegroupsnapshotv1alpha1.VolumeGroupSnapshotContent{ - Spec: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotContentSpec{ - Source: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotContentSource{ - GroupSnapshotHandles: &volumegroupsnapshotv1alpha1.GroupSnapshotHandles{ - VolumeGroupSnapshotHandle: groupSnapshotHandle, - VolumeSnapshotHandles: volumeSnapshotHandles, - }, - }, - VolumeGroupSnapshotRef: core_v1.ObjectReference{ - Name: modifiedRefName, - Namespace: "default-ns", - }, - }, - }, - oldGroupSnapContent: validContent, - shouldAdmit: false, - operation: v1.Update, - msg: fmt.Sprintf("Spec.VolumeGroupSnapshotRef.Name is immutable but was changed from %s to %s", - validContent.Spec.VolumeGroupSnapshotRef.Name, modifiedRefName), - }, - { - name: "Update: old is invalid and new is valid", - groupSnapContent: validContent, - oldGroupSnapContent: invalidContent, - shouldAdmit: false, - operation: v1.Update, - msg: fmt.Sprintf("Spec.Source.VolumeHandles is immutable but was changed from %s to %s", []string{volumeHandle}, []string{}), - }, - { - name: "Update: old is invalid and new is invalid", - groupSnapContent: invalidContent, - oldGroupSnapContent: invalidContent, - shouldAdmit: false, - operation: v1.Update, - msg: "both Spec.VolumeGroupSnapshotRef.Name = and Spec.VolumeGroupSnapshotRef.Namespace = default-ns must be set", - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - groupSnapContent := tc.groupSnapContent - raw, err := json.Marshal(groupSnapContent) - if err != nil { - t.Fatal(err) - } - oldGroupSnapContent := tc.oldGroupSnapContent - oldRaw, err := json.Marshal(oldGroupSnapContent) - if err != nil { - t.Fatal(err) - } - review := v1.AdmissionReview{ - Request: &v1.AdmissionRequest{ - Object: runtime.RawExtension{ - Raw: raw, - }, - OldObject: runtime.RawExtension{ - Raw: oldRaw, - }, - Resource: GroupSnapshotContentV1Apha1GVR, - Operation: tc.operation, - }, - } - sa := NewGroupSnapshotAdmitter(nil) - response := sa.Admit(review) - shouldAdmit := response.Allowed - msg := response.Result.Message - - expectedResponse := tc.shouldAdmit - expectedMsg := tc.msg - - if shouldAdmit != expectedResponse { - t.Errorf("expected \"%v\" to equal \"%v\"", shouldAdmit, expectedResponse) - } - if msg != expectedMsg { - t.Errorf("expected \"%v\" to equal \"%v\"", msg, expectedMsg) - } - }) - } -} - func TestAdmitVolumeGroupSnapshotClassV1Alpha1(t *testing.T) { testCases := []struct { name string diff --git a/pkg/validation-webhook/snapshot.go b/pkg/validation-webhook/snapshot.go index d0fb2a8f5..62f34787f 100644 --- a/pkg/validation-webhook/snapshot.go +++ b/pkg/validation-webhook/snapshot.go @@ -18,7 +18,6 @@ package webhook import ( "fmt" - "reflect" volumesnapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumesnapshot/v1" storagelisters "github.com/kubernetes-csi/external-snapshotter/client/v7/listers/volumesnapshot/v1" @@ -30,10 +29,6 @@ import ( ) var ( - // SnapshotV1GVR is GroupVersionResource for v1 VolumeSnapshots - SnapshotV1GVR = metav1.GroupVersionResource{Group: volumesnapshotv1.GroupName, Version: "v1", Resource: "volumesnapshots"} - // SnapshotContentV1GVR is GroupVersionResource for v1 VolumeSnapshotContents - SnapshotContentV1GVR = metav1.GroupVersionResource{Group: volumesnapshotv1.GroupName, Version: "v1", Resource: "volumesnapshotcontents"} // SnapshotContentV1GVR is GroupVersionResource for v1 VolumeSnapshotContents SnapshotClassV1GVR = metav1.GroupVersionResource{Group: volumesnapshotv1.GroupName, Version: "v1", Resource: "volumesnapshotclasses"} ) @@ -54,7 +49,7 @@ func NewSnapshotAdmitter(lister storagelisters.VolumeSnapshotClassLister) Snapsh // Add a label {"added-label": "yes"} to the object func (a admitter) Admit(ar v1.AdmissionReview) *v1.AdmissionResponse { - klog.V(2).Info("admitting volumesnapshots or volumesnapshotcontents") + klog.V(2).Info("admitting volumesnapshotclasses") reviewResponse := &v1.AdmissionResponse{ Allowed: true, @@ -65,37 +60,12 @@ func (a admitter) Admit(ar v1.AdmissionReview) *v1.AdmissionResponse { if !(ar.Request.Operation == v1.Update || ar.Request.Operation == v1.Create) { return reviewResponse } - isUpdate := ar.Request.Operation == v1.Update raw := ar.Request.Object.Raw oldRaw := ar.Request.OldObject.Raw deserializer := codecs.UniversalDeserializer() switch ar.Request.Resource { - case SnapshotV1GVR: - snapshot := &volumesnapshotv1.VolumeSnapshot{} - if _, _, err := deserializer.Decode(raw, nil, snapshot); err != nil { - klog.Error(err) - return toV1AdmissionResponse(err) - } - oldSnapshot := &volumesnapshotv1.VolumeSnapshot{} - if _, _, err := deserializer.Decode(oldRaw, nil, oldSnapshot); err != nil { - klog.Error(err) - return toV1AdmissionResponse(err) - } - return decideSnapshotV1(snapshot, oldSnapshot, isUpdate) - case SnapshotContentV1GVR: - snapcontent := &volumesnapshotv1.VolumeSnapshotContent{} - if _, _, err := deserializer.Decode(raw, nil, snapcontent); err != nil { - klog.Error(err) - return toV1AdmissionResponse(err) - } - oldSnapcontent := &volumesnapshotv1.VolumeSnapshotContent{} - if _, _, err := deserializer.Decode(oldRaw, nil, oldSnapcontent); err != nil { - klog.Error(err) - return toV1AdmissionResponse(err) - } - return decideSnapshotContentV1(snapcontent, oldSnapcontent, isUpdate) case SnapshotClassV1GVR: snapClass := &volumesnapshotv1.VolumeSnapshotClass{} if _, _, err := deserializer.Decode(raw, nil, snapClass); err != nil { @@ -109,59 +79,13 @@ func (a admitter) Admit(ar v1.AdmissionReview) *v1.AdmissionResponse { } return decideSnapshotClassV1(snapClass, oldSnapClass, a.lister) default: - err := fmt.Errorf("expect resource to be %s, %s, or %s, but found %v", - SnapshotV1GVR, SnapshotContentV1GVR, SnapshotClassV1GVR, ar.Request.Resource) + err := fmt.Errorf("expect resource to be %s, but found %v", + SnapshotClassV1GVR, ar.Request.Resource) klog.Error(err) return toV1AdmissionResponse(err) } } -func decideSnapshotV1(snapshot, oldSnapshot *volumesnapshotv1.VolumeSnapshot, isUpdate bool) *v1.AdmissionResponse { - reviewResponse := &v1.AdmissionResponse{ - Allowed: true, - Result: &metav1.Status{}, - } - - if isUpdate { - // if it is an UPDATE and oldSnapshot is valid, check immutable fields - if err := checkSnapshotImmutableFieldsV1(snapshot, oldSnapshot); err != nil { - reviewResponse.Allowed = false - reviewResponse.Result.Message = err.Error() - return reviewResponse - } - } - // Enforce strict validation for CREATE requests. Immutable checks don't apply for CREATE requests. - // Enforce strict validation for UPDATE requests where old is valid and passes immutability check. - if err := ValidateV1Snapshot(snapshot); err != nil { - reviewResponse.Allowed = false - reviewResponse.Result.Message = err.Error() - } - return reviewResponse -} - -func decideSnapshotContentV1(snapcontent, oldSnapcontent *volumesnapshotv1.VolumeSnapshotContent, isUpdate bool) *v1.AdmissionResponse { - reviewResponse := &v1.AdmissionResponse{ - Allowed: true, - Result: &metav1.Status{}, - } - - if isUpdate { - // if it is an UPDATE and oldSnapcontent is valid, check immutable fields - if err := checkSnapshotContentImmutableFieldsV1(snapcontent, oldSnapcontent); err != nil { - reviewResponse.Allowed = false - reviewResponse.Result.Message = err.Error() - return reviewResponse - } - } - // Enforce strict validation for all CREATE requests. Immutable checks don't apply for CREATE requests. - // Enforce strict validation for UPDATE requests where old is valid and passes immutability check. - if err := ValidateV1SnapshotContent(snapcontent); err != nil { - reviewResponse.Allowed = false - reviewResponse.Result.Message = err.Error() - } - return reviewResponse -} - func decideSnapshotClassV1(snapClass, oldSnapClass *volumesnapshotv1.VolumeSnapshotClass, lister storagelisters.VolumeSnapshotClassLister) *v1.AdmissionResponse { reviewResponse := &v1.AdmissionResponse{ Allowed: true, @@ -205,55 +129,3 @@ func strPtrDereference(s *string) string { } return *s } - -func checkSnapshotImmutableFieldsV1(snapshot, oldSnapshot *volumesnapshotv1.VolumeSnapshot) error { - if snapshot == nil { - return fmt.Errorf("VolumeSnapshot is nil") - } - if oldSnapshot == nil { - return fmt.Errorf("old VolumeSnapshot is nil") - } - - source := snapshot.Spec.Source - oldSource := oldSnapshot.Spec.Source - - if !reflect.DeepEqual(source.PersistentVolumeClaimName, oldSource.PersistentVolumeClaimName) { - return fmt.Errorf("Spec.Source.PersistentVolumeClaimName is immutable but was changed from %s to %s", strPtrDereference(oldSource.PersistentVolumeClaimName), strPtrDereference(source.PersistentVolumeClaimName)) - } - if !reflect.DeepEqual(source.VolumeSnapshotContentName, oldSource.VolumeSnapshotContentName) { - return fmt.Errorf("Spec.Source.VolumeSnapshotContentName is immutable but was changed from %s to %s", strPtrDereference(oldSource.VolumeSnapshotContentName), strPtrDereference(source.VolumeSnapshotContentName)) - } - - return nil -} - -func checkSnapshotContentImmutableFieldsV1(snapcontent, oldSnapcontent *volumesnapshotv1.VolumeSnapshotContent) error { - if snapcontent == nil { - return fmt.Errorf("VolumeSnapshotContent is nil") - } - if oldSnapcontent == nil { - return fmt.Errorf("old VolumeSnapshotContent is nil") - } - - source := snapcontent.Spec.Source - oldSource := oldSnapcontent.Spec.Source - - if !reflect.DeepEqual(source.VolumeHandle, oldSource.VolumeHandle) { - return fmt.Errorf("Spec.Source.VolumeHandle is immutable but was changed from %s to %s", strPtrDereference(oldSource.VolumeHandle), strPtrDereference(source.VolumeHandle)) - } - if !reflect.DeepEqual(source.SnapshotHandle, oldSource.SnapshotHandle) { - return fmt.Errorf("Spec.Source.SnapshotHandle is immutable but was changed from %s to %s", strPtrDereference(oldSource.SnapshotHandle), strPtrDereference(source.SnapshotHandle)) - } - - if preventVolumeModeConversion { - if oldSnapcontent.Spec.SourceVolumeMode != nil && snapcontent.Spec.SourceVolumeMode != nil && !reflect.DeepEqual(snapcontent.Spec.SourceVolumeMode, oldSnapcontent.Spec.SourceVolumeMode) { - return fmt.Errorf("Spec.SourceVolumeMode is immutable but was changed from %v to %v", *oldSnapcontent.Spec.SourceVolumeMode, *snapcontent.Spec.SourceVolumeMode) - } else if oldSnapcontent.Spec.SourceVolumeMode == nil && snapcontent.Spec.SourceVolumeMode != nil { - return fmt.Errorf("Spec.SourceVolumeMode is immutable but was changed from nil to %v", *snapcontent.Spec.SourceVolumeMode) - } else if oldSnapcontent.Spec.SourceVolumeMode != nil && snapcontent.Spec.SourceVolumeMode == nil { - return fmt.Errorf("Spec.SourceVolumeMode is immutable but was changed from %v to nil", *oldSnapcontent.Spec.SourceVolumeMode) - } - } - - return nil -} diff --git a/pkg/validation-webhook/snapshot_test.go b/pkg/validation-webhook/snapshot_test.go index 36ecdeb2e..77449900f 100644 --- a/pkg/validation-webhook/snapshot_test.go +++ b/pkg/validation-webhook/snapshot_test.go @@ -18,348 +18,17 @@ package webhook import ( "encoding/json" - "fmt" "testing" volumesnapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumesnapshot/v1" storagelisters "github.com/kubernetes-csi/external-snapshotter/client/v7/listers/volumesnapshot/v1" "github.com/kubernetes-csi/external-snapshotter/v7/pkg/utils" v1 "k8s.io/api/admission/v1" - core_v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" ) -func TestAdmitVolumeSnapshotV1(t *testing.T) { - pvcname := "pvcname1" - mutatedField := "changed-immutable-field" - contentname := "snapcontent1" - volumeSnapshotClassName := "volume-snapshot-class-1" - emptyVolumeSnapshotClassName := "" - - testCases := []struct { - name string - volumeSnapshot *volumesnapshotv1.VolumeSnapshot - oldVolumeSnapshot *volumesnapshotv1.VolumeSnapshot - shouldAdmit bool - msg string - operation v1.Operation - }{ - { - name: "Delete: new and old are nil. Should admit", - volumeSnapshot: nil, - oldVolumeSnapshot: nil, - shouldAdmit: true, - operation: v1.Delete, - }, - { - name: "Create: old is nil and new is valid", - volumeSnapshot: &volumesnapshotv1.VolumeSnapshot{ - Spec: volumesnapshotv1.VolumeSnapshotSpec{ - Source: volumesnapshotv1.VolumeSnapshotSource{ - VolumeSnapshotContentName: &contentname, - }, - }, - }, - oldVolumeSnapshot: nil, - shouldAdmit: true, - operation: v1.Create, - }, - { - name: "Update: old is valid and new is invalid", - volumeSnapshot: &volumesnapshotv1.VolumeSnapshot{ - Spec: volumesnapshotv1.VolumeSnapshotSpec{ - Source: volumesnapshotv1.VolumeSnapshotSource{ - VolumeSnapshotContentName: &contentname, - }, - VolumeSnapshotClassName: &emptyVolumeSnapshotClassName, - }, - }, - oldVolumeSnapshot: &volumesnapshotv1.VolumeSnapshot{ - Spec: volumesnapshotv1.VolumeSnapshotSpec{ - Source: volumesnapshotv1.VolumeSnapshotSource{ - VolumeSnapshotContentName: &contentname, - }, - }, - }, - shouldAdmit: false, - operation: v1.Update, - msg: "Spec.VolumeSnapshotClassName must not be the empty string", - }, - { - name: "Update: old is valid and new is valid", - volumeSnapshot: &volumesnapshotv1.VolumeSnapshot{ - Spec: volumesnapshotv1.VolumeSnapshotSpec{ - Source: volumesnapshotv1.VolumeSnapshotSource{ - VolumeSnapshotContentName: &contentname, - }, - VolumeSnapshotClassName: &volumeSnapshotClassName, - }, - }, - oldVolumeSnapshot: &volumesnapshotv1.VolumeSnapshot{ - Spec: volumesnapshotv1.VolumeSnapshotSpec{ - Source: volumesnapshotv1.VolumeSnapshotSource{ - VolumeSnapshotContentName: &contentname, - }, - }, - }, - shouldAdmit: true, - operation: v1.Update, - }, - { - name: "Update: old is valid and new is valid but changes immutable field spec.source", - volumeSnapshot: &volumesnapshotv1.VolumeSnapshot{ - Spec: volumesnapshotv1.VolumeSnapshotSpec{ - Source: volumesnapshotv1.VolumeSnapshotSource{ - VolumeSnapshotContentName: &mutatedField, - }, - VolumeSnapshotClassName: &volumeSnapshotClassName, - }, - }, - oldVolumeSnapshot: &volumesnapshotv1.VolumeSnapshot{ - Spec: volumesnapshotv1.VolumeSnapshotSpec{ - Source: volumesnapshotv1.VolumeSnapshotSource{ - VolumeSnapshotContentName: &contentname, - }, - }, - }, - shouldAdmit: false, - operation: v1.Update, - msg: fmt.Sprintf("Spec.Source.VolumeSnapshotContentName is immutable but was changed from %s to %s", contentname, mutatedField), - }, - { - name: "Update: old is invalid and new is valid", - volumeSnapshot: &volumesnapshotv1.VolumeSnapshot{ - Spec: volumesnapshotv1.VolumeSnapshotSpec{ - Source: volumesnapshotv1.VolumeSnapshotSource{ - VolumeSnapshotContentName: &contentname, - }, - }, - }, - oldVolumeSnapshot: &volumesnapshotv1.VolumeSnapshot{ - Spec: volumesnapshotv1.VolumeSnapshotSpec{ - Source: volumesnapshotv1.VolumeSnapshotSource{ - PersistentVolumeClaimName: &pvcname, - VolumeSnapshotContentName: &contentname, - }, - }, - }, - shouldAdmit: false, - operation: v1.Update, - msg: fmt.Sprintf("Spec.Source.PersistentVolumeClaimName is immutable but was changed from %s to ", pvcname), - }, - { - // will be handled by schema validation - name: "Update: old is invalid and new is invalid", - volumeSnapshot: &volumesnapshotv1.VolumeSnapshot{ - Spec: volumesnapshotv1.VolumeSnapshotSpec{ - Source: volumesnapshotv1.VolumeSnapshotSource{ - VolumeSnapshotContentName: &contentname, - PersistentVolumeClaimName: &pvcname, - }, - }, - }, - oldVolumeSnapshot: &volumesnapshotv1.VolumeSnapshot{ - Spec: volumesnapshotv1.VolumeSnapshotSpec{ - Source: volumesnapshotv1.VolumeSnapshotSource{ - PersistentVolumeClaimName: &pvcname, - VolumeSnapshotContentName: &contentname, - }, - }, - }, - shouldAdmit: true, - operation: v1.Update, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - snapshot := tc.volumeSnapshot - raw, err := json.Marshal(snapshot) - if err != nil { - t.Fatal(err) - } - oldSnapshot := tc.oldVolumeSnapshot - oldRaw, err := json.Marshal(oldSnapshot) - if err != nil { - t.Fatal(err) - } - review := v1.AdmissionReview{ - Request: &v1.AdmissionRequest{ - Object: runtime.RawExtension{ - Raw: raw, - }, - OldObject: runtime.RawExtension{ - Raw: oldRaw, - }, - Resource: SnapshotV1GVR, - Operation: tc.operation, - }, - } - sa := NewSnapshotAdmitter(nil) - response := sa.Admit(review) - shouldAdmit := response.Allowed - msg := response.Result.Message - - expectedResponse := tc.shouldAdmit - expectedMsg := tc.msg - - if shouldAdmit != expectedResponse { - t.Errorf("expected \"%v\" to equal \"%v\"", shouldAdmit, expectedResponse) - } - if msg != expectedMsg { - t.Errorf("expected \"%v\" to equal \"%v\"", msg, expectedMsg) - } - }) - } -} - -func TestAdmitVolumeSnapshotContentV1(t *testing.T) { - volumeHandle := "volumeHandle1" - modifiedField := "modified-field" - snapshotHandle := "snapshotHandle1" - volumeSnapshotClassName := "volume-snapshot-class-1" - validContent := &volumesnapshotv1.VolumeSnapshotContent{ - Spec: volumesnapshotv1.VolumeSnapshotContentSpec{ - Source: volumesnapshotv1.VolumeSnapshotContentSource{ - SnapshotHandle: &snapshotHandle, - }, - VolumeSnapshotRef: core_v1.ObjectReference{ - Name: "snapshot-ref", - Namespace: "default-ns", - }, - VolumeSnapshotClassName: &volumeSnapshotClassName, - }, - } - invalidContent := &volumesnapshotv1.VolumeSnapshotContent{ - Spec: volumesnapshotv1.VolumeSnapshotContentSpec{ - Source: volumesnapshotv1.VolumeSnapshotContentSource{ - SnapshotHandle: &snapshotHandle, - VolumeHandle: &volumeHandle, - }, - VolumeSnapshotRef: core_v1.ObjectReference{ - Name: "", - Namespace: "default-ns", - }, - }, - } - - testCases := []struct { - name string - volumeSnapshotContent *volumesnapshotv1.VolumeSnapshotContent - oldVolumeSnapshotContent *volumesnapshotv1.VolumeSnapshotContent - shouldAdmit bool - msg string - operation v1.Operation - }{ - { - name: "Delete: both new and old are nil", - volumeSnapshotContent: nil, - oldVolumeSnapshotContent: nil, - shouldAdmit: true, - operation: v1.Delete, - }, - { - name: "Create: old is nil and new is valid", - volumeSnapshotContent: validContent, - oldVolumeSnapshotContent: nil, - shouldAdmit: true, - operation: v1.Create, - }, - { - name: "Update: old is valid and new is invalid", - volumeSnapshotContent: invalidContent, - oldVolumeSnapshotContent: validContent, - shouldAdmit: false, - operation: v1.Update, - msg: fmt.Sprintf("Spec.Source.VolumeHandle is immutable but was changed from %s to %s", strPtrDereference(nil), volumeHandle), - }, - { - name: "Update: old is valid and new is valid", - volumeSnapshotContent: validContent, - oldVolumeSnapshotContent: validContent, - shouldAdmit: true, - operation: v1.Update, - }, - { - name: "Update: old is valid and new is valid but modifies immutable field", - volumeSnapshotContent: &volumesnapshotv1.VolumeSnapshotContent{ - Spec: volumesnapshotv1.VolumeSnapshotContentSpec{ - Source: volumesnapshotv1.VolumeSnapshotContentSource{ - SnapshotHandle: &modifiedField, - }, - VolumeSnapshotRef: core_v1.ObjectReference{ - Name: "snapshot-ref", - Namespace: "default-ns", - }, - }, - }, - oldVolumeSnapshotContent: validContent, - shouldAdmit: false, - operation: v1.Update, - msg: fmt.Sprintf("Spec.Source.SnapshotHandle is immutable but was changed from %s to %s", snapshotHandle, modifiedField), - }, - { - name: "Update: old is invalid and new is valid", - volumeSnapshotContent: validContent, - oldVolumeSnapshotContent: invalidContent, - shouldAdmit: false, - operation: v1.Update, - msg: fmt.Sprintf("Spec.Source.VolumeHandle is immutable but was changed from %s to ", volumeHandle), - }, - { - name: "Update: old is invalid and new is invalid", - volumeSnapshotContent: invalidContent, - oldVolumeSnapshotContent: invalidContent, - shouldAdmit: false, - operation: v1.Update, - msg: "both Spec.VolumeSnapshotRef.Name = and Spec.VolumeSnapshotRef.Namespace = default-ns must be set", - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - snapshotContent := tc.volumeSnapshotContent - raw, err := json.Marshal(snapshotContent) - if err != nil { - t.Fatal(err) - } - oldSnapshotContent := tc.oldVolumeSnapshotContent - oldRaw, err := json.Marshal(oldSnapshotContent) - if err != nil { - t.Fatal(err) - } - review := v1.AdmissionReview{ - Request: &v1.AdmissionRequest{ - Object: runtime.RawExtension{ - Raw: raw, - }, - OldObject: runtime.RawExtension{ - Raw: oldRaw, - }, - Resource: SnapshotContentV1GVR, - Operation: tc.operation, - }, - } - sa := NewSnapshotAdmitter(nil) - response := sa.Admit(review) - shouldAdmit := response.Allowed - msg := response.Result.Message - - expectedResponse := tc.shouldAdmit - expectedMsg := tc.msg - - if shouldAdmit != expectedResponse { - t.Errorf("expected \"%v\" to equal \"%v\"", shouldAdmit, expectedResponse) - } - if msg != expectedMsg { - t.Errorf("expected \"%v\" to equal \"%v\"", msg, expectedMsg) - } - }) - } -} - type fakeSnapshotLister struct { values []*volumesnapshotv1.VolumeSnapshotClass } diff --git a/pkg/validation-webhook/validation.go b/pkg/validation-webhook/validation.go deleted file mode 100644 index 2f245015c..000000000 --- a/pkg/validation-webhook/validation.go +++ /dev/null @@ -1,88 +0,0 @@ -/* -Copyright 2021 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 webhook - -import ( - "fmt" - - groupsnapshotcrdv1alpha1 "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumegroupsnapshot/v1alpha1" - crdv1 "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumesnapshot/v1" -) - -// ValidateV1Snapshot performs additional strict validation. -// Do NOT rely on this function to fully validate snapshot objects. -// This function will only check the additional rules provided by the webhook. -func ValidateV1Snapshot(snapshot *crdv1.VolumeSnapshot) error { - if snapshot == nil { - return fmt.Errorf("VolumeSnapshot is nil") - } - - vscname := snapshot.Spec.VolumeSnapshotClassName - if vscname != nil && *vscname == "" { - return fmt.Errorf("Spec.VolumeSnapshotClassName must not be the empty string") - } - return nil -} - -// ValidateV1SnapshotContent performs additional strict validation. -// Do NOT rely on this function to fully validate snapshot content objects. -// This function will only check the additional rules provided by the webhook. -func ValidateV1SnapshotContent(snapcontent *crdv1.VolumeSnapshotContent) error { - if snapcontent == nil { - return fmt.Errorf("VolumeSnapshotContent is nil") - } - - vsref := snapcontent.Spec.VolumeSnapshotRef - - if vsref.Name == "" || vsref.Namespace == "" { - return fmt.Errorf("both Spec.VolumeSnapshotRef.Name = %s and Spec.VolumeSnapshotRef.Namespace = %s must be set", vsref.Name, vsref.Namespace) - } - - return nil -} - -// ValidateV1Alpha1GroupSnapshotContent performs additional strict validation. -// Do NOT rely on this function to fully validate group snapshot content objects. -// This function will only check the additional rules provided by the webhook. -func ValidateV1Alpha1GroupSnapshotContent(groupSnapcontent *groupsnapshotcrdv1alpha1.VolumeGroupSnapshotContent) error { - if groupSnapcontent == nil { - return fmt.Errorf("VolumeGroupSnapshotContent is nil") - } - - vgsref := groupSnapcontent.Spec.VolumeGroupSnapshotRef - - if vgsref.Name == "" || vgsref.Namespace == "" { - return fmt.Errorf("both Spec.VolumeGroupSnapshotRef.Name = %s and Spec.VolumeGroupSnapshotRef.Namespace = %s must be set", vgsref.Name, vgsref.Namespace) - } - - return nil -} - -// ValidateV1Alpha1GroupSnapshot performs additional strict validation. -// Do NOT rely on this function to fully validate group snapshot objects. -// This function will only check the additional rules provided by the webhook. -func ValidateV1Alpha1GroupSnapshot(snapshot *groupsnapshotcrdv1alpha1.VolumeGroupSnapshot) error { - if snapshot == nil { - return fmt.Errorf("VolumeGroupSnapshot is nil") - } - - vgscname := snapshot.Spec.VolumeGroupSnapshotClassName - if vgscname != nil && *vgscname == "" { - return fmt.Errorf("Spec.VolumeGroupSnapshotClassName must not be the empty string") - } - return nil -} diff --git a/pkg/validation-webhook/webhook.go b/pkg/validation-webhook/webhook.go index d27155016..0a56c7fcb 100644 --- a/pkg/validation-webhook/webhook.go +++ b/pkg/validation-webhook/webhook.go @@ -73,7 +73,7 @@ func init() { CmdWebhook.Flags().BoolVar(&preventVolumeModeConversion, "prevent-volume-mode-conversion", true, "Prevents an unauthorised user from modifying the volume mode when creating a PVC from an existing VolumeSnapshot.") CmdWebhook.Flags().BoolVar(&enableVolumeGroupSnapshotWebhook, "enable-volume-group-snapshot-webhook", - false, "Enables webhook for VolumeGroupSnapshot, VolumeGroupSnapshotContent and VolumeGroupSnapshotClass.") + false, "Enables webhook for VolumeGroupSnapshotClass.") } // admitv1beta1Func handles a v1beta1 admission