Skip to content

Commit

Permalink
Remove duplicate validation logic from webhook
Browse files Browse the repository at this point in the history 
This patch removes the logic to admit VolumeSnapshots,
VolumeSnapshotContents, VolumeGroupSnapshots and
VolumeGroupSnapshotContents in the validation webhook.

That logic is already implemented via CEL expressions (see kubernetes-csi#1073)

The logic to admit VolumeSnapshotClasses and VolumeGroupSnapshotClasses
is still implemented in the webhook and avoids having multiple default
classes for the same CSI Driver.
  • Loading branch information
@leonardoce
leonardoce committed May 20, 2024
1 parent 52b72d3 commit 9461f70
Show file tree
Hide file tree
Showing 8 changed files with 9 additions and 1,173 deletions.
4 changes: 2 additions & 2 deletions deploy/kubernetes/webhook-example/admission-configuration-template
View file
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
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
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

0 comments on commit 9461f70

Please sign in to comment.