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

Remove duplicate validation logic from webhook #1091

Merged
merged 1 commit into from
May 20, 2024
Merged

Remove duplicate validation logic from webhook #1091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions deploy/kubernetes/webhook-example/admission-configuration-template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ webhooks:
- apiGroups: ["snapshot.storage.k8s.io"]
apiVersions: ["v1"]
operations: ["CREATE", "UPDATE"]
resources: ["volumesnapshots", "volumesnapshotcontents", "volumesnapshotclasses"]
resources: ["volumesnapshotclasses"]
scope: "*"
clientConfig:
service:
Expand All @@ -31,7 +31,7 @@ webhooks:
- apiGroups: ["groupsnapshot.storage.k8s.io"]
apiVersions: ["v1alpha1"]
operations: ["CREATE", "UPDATE"]
resources: ["volumegroupsnapshots", "volumegroupsnapshotcontents", "volumegroupsnapshotclasses"]
resources: ["volumegroupsnapshotclasses"]
scope: "*"
clientConfig:
service:
Expand Down
114 changes: 1 addition & 113 deletions pkg/common-controller/snapshot_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

// ==================================================================
Expand Down Expand Up @@ -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) ||
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down
134 changes: 2 additions & 132 deletions pkg/validation-webhook/groupsnapshot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"}
)
Expand All @@ -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,
Expand All @@ -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 {
Expand All @@ -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,
Expand Down Expand Up @@ -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
}
Loading