Skip to content

Commit 9461f70

Browse files
leonardoceleonardoce
committed
Remove duplicate validation logic from webhook
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.
1 parent 52b72d3 commit 9461f70

File tree

8 files changed

+9
-1173
lines changed

8 files changed

+9
-1173
lines changed

‎deploy/kubernetes/webhook-example/admission-configuration-template

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ webhooks:
88
- apiGroups: ["snapshot.storage.k8s.io"]
99
apiVersions: ["v1"]
1010
operations: ["CREATE", "UPDATE"]
11-
resources: ["volumesnapshots", "volumesnapshotcontents", "volumesnapshotclasses"]
11+
resources: ["volumesnapshotclasses"]
1212
scope: "*"
1313
clientConfig:
1414
service:
@@ -31,7 +31,7 @@ webhooks:
3131
- apiGroups: ["groupsnapshot.storage.k8s.io"]
3232
apiVersions: ["v1alpha1"]
3333
operations: ["CREATE", "UPDATE"]
34-
resources: ["volumegroupsnapshots", "volumegroupsnapshotcontents", "volumegroupsnapshotclasses"]
34+
resources: ["volumegroupsnapshotclasses"]
3535
scope: "*"
3636
clientConfig:
3737
service:

‎pkg/common-controller/snapshot_controller.go

Lines changed: 1 addition & 113 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@ import (
3636
crdv1 "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumesnapshot/v1"
3737
"github.com/kubernetes-csi/external-snapshotter/v7/pkg/metrics"
3838
"github.com/kubernetes-csi/external-snapshotter/v7/pkg/utils"
39-
webhook "github.com/kubernetes-csi/external-snapshotter/v7/pkg/validation-webhook"
4039
)
4140

4241
// ==================================================================
@@ -91,14 +90,6 @@ func (ctrl *csiSnapshotCommonController) syncContent(content *crdv1.VolumeSnapsh
9190
klog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: content is bound to snapshot %s", content.Name, snapshotName)
9291

9392
klog.V(5).Infof("syncContent[%s]: check if we should add invalid label on content", content.Name)
94-
// Perform additional validation. Label objects which fail.
95-
// Part of a plan to tighten validation, this label will enable users to
96-
// query for invalid content objects. See issue #363
97-
content, err := ctrl.checkAndSetInvalidContentLabel(content)
98-
if err != nil {
99-
klog.Errorf("syncContent[%s]: check and add invalid content label failed, %s", content.Name, err.Error())
100-
return err
101-
}
10293

10394
// Keep this check in the controller since the validation webhook may not have been deployed.
10495
if (content.Spec.Source.VolumeHandle == nil && content.Spec.Source.SnapshotHandle == nil) ||
@@ -128,7 +119,7 @@ func (ctrl *csiSnapshotCommonController) syncContent(content *crdv1.VolumeSnapsh
128119
// and it may have already been deleted, and it will fall into the
129120
// snapshot == nil case below
130121
var snapshot *crdv1.VolumeSnapshot
131-
snapshot, err = ctrl.getSnapshotFromStore(snapshotName)
122+
snapshot, err := ctrl.getSnapshotFromStore(snapshotName)
132123
if err != nil {
133124
return err
134125
}
@@ -195,14 +186,6 @@ func (ctrl *csiSnapshotCommonController) syncSnapshot(snapshot *crdv1.VolumeSnap
195186
}
196187

197188
klog.V(5).Infof("syncSnapshot[%s]: check if we should add invalid label on snapshot", utils.SnapshotKey(snapshot))
198-
// Perform additional validation. Label objects which fail.
199-
// Part of a plan to tighten validation, this label will enable users to
200-
// query for invalid snapshot objects. See issue #363
201-
snapshot, err := ctrl.checkAndSetInvalidSnapshotLabel(snapshot)
202-
if err != nil {
203-
klog.Errorf("syncSnapshot[%s]: check and add invalid snapshot label failed, %s", utils.SnapshotKey(snapshot), err.Error())
204-
return err
205-
}
206189

207190
// Proceed with snapshot deletion and remove finalizers when needed
208191
if snapshot.ObjectMeta.DeletionTimestamp != nil {
@@ -1654,101 +1637,6 @@ func (ctrl *csiSnapshotCommonController) setAnnVolumeSnapshotBeingDeleted(conten
16541637
return content, nil
16551638
}
16561639

1657-
// checkAndSetInvalidContentLabel adds a label to unlabeled invalid content objects and removes the label from valid ones.
1658-
func (ctrl *csiSnapshotCommonController) checkAndSetInvalidContentLabel(content *crdv1.VolumeSnapshotContent) (*crdv1.VolumeSnapshotContent, error) {
1659-
hasLabel := utils.MapContainsKey(content.ObjectMeta.Labels, utils.VolumeSnapshotContentInvalidLabel)
1660-
err := webhook.ValidateV1SnapshotContent(content)
1661-
if err != nil {
1662-
klog.Errorf("syncContent[%s]: Invalid content detected, %s", content.Name, err.Error())
1663-
}
1664-
// If the snapshot content correctly has the label, or correctly does not have the label, take no action.
1665-
if hasLabel && err != nil || !hasLabel && err == nil {
1666-
return content, nil
1667-
}
1668-
1669-
var patches []utils.PatchOp
1670-
contentClone := content.DeepCopy()
1671-
if hasLabel {
1672-
// Need to remove the label
1673-
patches = append(patches, utils.PatchOp{
1674-
Op: "remove",
1675-
Path: "/metadata/labels/" + utils.VolumeSnapshotContentInvalidLabel,
1676-
})
1677-
1678-
} else {
1679-
// Snapshot content is invalid and does not have the label. Need to add the label
1680-
patches = append(patches, utils.PatchOp{
1681-
Op: "add",
1682-
Path: "/metadata/labels/" + utils.VolumeSnapshotContentInvalidLabel,
1683-
Value: "",
1684-
})
1685-
}
1686-
updatedContent, err := utils.PatchVolumeSnapshotContent(contentClone, patches, ctrl.clientset)
1687-
if err != nil {
1688-
return content, newControllerUpdateError(content.Name, err.Error())
1689-
}
1690-
1691-
_, err = ctrl.storeContentUpdate(updatedContent)
1692-
if err != nil {
1693-
klog.Errorf("failed to update content store %v", err)
1694-
}
1695-
1696-
if hasLabel {
1697-
klog.V(5).Infof("Removed invalid content label from volume snapshot content %s", content.Name)
1698-
} else {
1699-
klog.V(5).Infof("Added invalid content label to volume snapshot content %s", content.Name)
1700-
}
1701-
return updatedContent, nil
1702-
}
1703-
1704-
// checkAndSetInvalidSnapshotLabel adds a label to unlabeled invalid snapshot objects and removes the label from valid ones.
1705-
func (ctrl *csiSnapshotCommonController) checkAndSetInvalidSnapshotLabel(snapshot *crdv1.VolumeSnapshot) (*crdv1.VolumeSnapshot, error) {
1706-
hasLabel := utils.MapContainsKey(snapshot.ObjectMeta.Labels, utils.VolumeSnapshotInvalidLabel)
1707-
err := webhook.ValidateV1Snapshot(snapshot)
1708-
if err != nil {
1709-
klog.Errorf("syncSnapshot[%s]: Invalid snapshot detected, %s", utils.SnapshotKey(snapshot), err.Error())
1710-
}
1711-
// If the snapshot correctly has the label, or correctly does not have the label, take no action.
1712-
if hasLabel && err != nil || !hasLabel && err == nil {
1713-
return snapshot, nil
1714-
}
1715-
1716-
var patches []utils.PatchOp
1717-
snapshotClone := snapshot.DeepCopy()
1718-
if hasLabel {
1719-
// Need to remove the label
1720-
patches = append(patches, utils.PatchOp{
1721-
Op: "remove",
1722-
Path: "/metadata/labels/" + utils.VolumeSnapshotInvalidLabel,
1723-
})
1724-
} else {
1725-
// Snapshot is invalid and does not have the label. Need to add the label
1726-
patches = append(patches, utils.PatchOp{
1727-
Op: "add",
1728-
Path: "/metadata/labels/" + utils.VolumeSnapshotInvalidLabel,
1729-
Value: "",
1730-
})
1731-
}
1732-
1733-
updatedSnapshot, err := utils.PatchVolumeSnapshot(snapshotClone, patches, ctrl.clientset)
1734-
if err != nil {
1735-
return snapshot, newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error())
1736-
}
1737-
1738-
_, err = ctrl.storeSnapshotUpdate(updatedSnapshot)
1739-
if err != nil {
1740-
klog.Errorf("failed to update snapshot store %v", err)
1741-
}
1742-
1743-
if hasLabel {
1744-
klog.V(5).Infof("Removed invalid snapshot label from volume snapshot %s", utils.SnapshotKey(snapshot))
1745-
} else {
1746-
klog.V(5).Infof("Added invalid snapshot label to volume snapshot %s", utils.SnapshotKey(snapshot))
1747-
}
1748-
1749-
return updatedSnapshot, nil
1750-
}
1751-
17521640
func (ctrl *csiSnapshotCommonController) getManagedByNode(pv *v1.PersistentVolume) (string, error) {
17531641
if pv.Spec.NodeAffinity == nil {
17541642
klog.V(5).Infof("NodeAffinity not set for pv %s", pv.Name)

‎pkg/validation-webhook/groupsnapshot.go

Lines changed: 2 additions & 132 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ package webhook
1818

1919
import (
2020
"fmt"
21-
"reflect"
2221

2322
volumegroupsnapshotv1alpha1 "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumegroupsnapshot/v1alpha1"
2423
groupsnapshotlisters "github.com/kubernetes-csi/external-snapshotter/client/v7/listers/volumegroupsnapshot/v1alpha1"
@@ -30,10 +29,6 @@ import (
3029
)
3130

3231
var (
33-
// GroupSnapshotV1Alpha1GVR is GroupVersionResource for v1alpha1 VolumeGroupSnapshots
34-
GroupSnapshotV1Alpha1GVR = metav1.GroupVersionResource{Group: volumegroupsnapshotv1alpha1.GroupName, Version: "v1alpha1", Resource: "volumegroupsnapshots"}
35-
// GroupSnapshotContentV1Apha1GVR is GroupVersionResource for v1alpha1 VolumeGroupSnapshotContents
36-
GroupSnapshotContentV1Apha1GVR = metav1.GroupVersionResource{Group: volumegroupsnapshotv1alpha1.GroupName, Version: "v1alpha1", Resource: "volumegroupsnapshotcontents"}
3732
// GroupSnapshotClassV1Apha1GVR is GroupVersionResource for v1alpha1 VolumeGroupSnapshotClasses
3833
GroupSnapshotClassV1Apha1GVR = metav1.GroupVersionResource{Group: volumegroupsnapshotv1alpha1.GroupName, Version: "v1alpha1", Resource: "volumegroupsnapshotclasses"}
3934
)
@@ -54,8 +49,7 @@ func NewGroupSnapshotAdmitter(lister groupsnapshotlisters.VolumeGroupSnapshotCla
5449

5550
// Add a label {"added-label": "yes"} to the object
5651
func (a groupSnapshotAdmitter) Admit(ar v1.AdmissionReview) *v1.AdmissionResponse {
57-
klog.V(2).Info("admitting volumegroupsnapshots volumegroupsnapshotcontents " +
58-
"or volumegroupsnapshotclasses")
52+
klog.V(2).Info("admitting volumegroupsnapshotclasses")
5953

6054
reviewResponse := &v1.AdmissionResponse{
6155
Allowed: true,
@@ -66,37 +60,12 @@ func (a groupSnapshotAdmitter) Admit(ar v1.AdmissionReview) *v1.AdmissionRespons
6660
if !(ar.Request.Operation == v1.Update || ar.Request.Operation == v1.Create) {
6761
return reviewResponse
6862
}
69-
isUpdate := ar.Request.Operation == v1.Update
7063

7164
raw := ar.Request.Object.Raw
7265
oldRaw := ar.Request.OldObject.Raw
7366

7467
deserializer := codecs.UniversalDeserializer()
7568
switch ar.Request.Resource {
76-
case GroupSnapshotV1Alpha1GVR:
77-
groupSnapshot := &volumegroupsnapshotv1alpha1.VolumeGroupSnapshot{}
78-
if _, _, err := deserializer.Decode(raw, nil, groupSnapshot); err != nil {
79-
klog.Error(err)
80-
return toV1AdmissionResponse(err)
81-
}
82-
oldGroupSnapshot := &volumegroupsnapshotv1alpha1.VolumeGroupSnapshot{}
83-
if _, _, err := deserializer.Decode(oldRaw, nil, oldGroupSnapshot); err != nil {
84-
klog.Error(err)
85-
return toV1AdmissionResponse(err)
86-
}
87-
return decideGroupSnapshotV1Alpha1(groupSnapshot, oldGroupSnapshot, isUpdate)
88-
case GroupSnapshotContentV1Apha1GVR:
89-
groupSnapContent := &volumegroupsnapshotv1alpha1.VolumeGroupSnapshotContent{}
90-
if _, _, err := deserializer.Decode(raw, nil, groupSnapContent); err != nil {
91-
klog.Error(err)
92-
return toV1AdmissionResponse(err)
93-
}
94-
oldGroupSnapContent := &volumegroupsnapshotv1alpha1.VolumeGroupSnapshotContent{}
95-
if _, _, err := deserializer.Decode(oldRaw, nil, oldGroupSnapContent); err != nil {
96-
klog.Error(err)
97-
return toV1AdmissionResponse(err)
98-
}
99-
return decideGroupSnapshotContentV1Alpha1(groupSnapContent, oldGroupSnapContent, isUpdate)
10069
case GroupSnapshotClassV1Apha1GVR:
10170
groupSnapClass := &volumegroupsnapshotv1alpha1.VolumeGroupSnapshotClass{}
10271
if _, _, err := deserializer.Decode(raw, nil, groupSnapClass); err != nil {
@@ -110,60 +79,13 @@ func (a groupSnapshotAdmitter) Admit(ar v1.AdmissionReview) *v1.AdmissionRespons
11079
}
11180
return decideGroupSnapshotClassV1Alpha1(groupSnapClass, oldGroupSnapClass, a.lister)
11281
default:
113-
err := fmt.Errorf("expect resource to be %s, %s, or %s, but found %v",
114-
GroupSnapshotV1Alpha1GVR, GroupSnapshotContentV1Apha1GVR,
82+
err := fmt.Errorf("expect resource to be %s, but found %v",
11583
GroupSnapshotClassV1Apha1GVR, ar.Request.Resource)
11684
klog.Error(err)
11785
return toV1AdmissionResponse(err)
11886
}
11987
}
12088

121-
func decideGroupSnapshotV1Alpha1(groupSnapshot, oldGroupSnapshot *volumegroupsnapshotv1alpha1.VolumeGroupSnapshot, isUpdate bool) *v1.AdmissionResponse {
122-
reviewResponse := &v1.AdmissionResponse{
123-
Allowed: true,
124-
Result: &metav1.Status{},
125-
}
126-
127-
if isUpdate {
128-
// if it is an UPDATE and oldGroupSnapshot is valid, check immutable fields
129-
if err := checkGroupSnapshotImmutableFieldsV1Alpha1(groupSnapshot, oldGroupSnapshot); err != nil {
130-
reviewResponse.Allowed = false
131-
reviewResponse.Result.Message = err.Error()
132-
return reviewResponse
133-
}
134-
}
135-
// Enforce strict validation for CREATE requests. Immutable checks don't apply for CREATE requests.
136-
// Enforce strict validation for UPDATE requests where old is valid and passes immutability check.
137-
if err := ValidateV1Alpha1GroupSnapshot(groupSnapshot); err != nil {
138-
reviewResponse.Allowed = false
139-
reviewResponse.Result.Message = err.Error()
140-
}
141-
return reviewResponse
142-
}
143-
144-
func decideGroupSnapshotContentV1Alpha1(groupSnapcontent, oldGroupSnapcontent *volumegroupsnapshotv1alpha1.VolumeGroupSnapshotContent, isUpdate bool) *v1.AdmissionResponse {
145-
reviewResponse := &v1.AdmissionResponse{
146-
Allowed: true,
147-
Result: &metav1.Status{},
148-
}
149-
150-
if isUpdate {
151-
// if it is an UPDATE and oldGroupSnapcontent is valid, check immutable fields
152-
if err := checkGroupSnapshotContentImmutableFieldsV1Alpha1(groupSnapcontent, oldGroupSnapcontent); err != nil {
153-
reviewResponse.Allowed = false
154-
reviewResponse.Result.Message = err.Error()
155-
return reviewResponse
156-
}
157-
}
158-
// Enforce strict validation for all CREATE requests. Immutable checks don't apply for CREATE requests.
159-
// Enforce strict validation for UPDATE requests where old is valid and passes immutability check.
160-
if err := ValidateV1Alpha1GroupSnapshotContent(groupSnapcontent); err != nil {
161-
reviewResponse.Allowed = false
162-
reviewResponse.Result.Message = err.Error()
163-
}
164-
return reviewResponse
165-
}
166-
16789
func decideGroupSnapshotClassV1Alpha1(groupSnapClass, oldGroupSnapClass *volumegroupsnapshotv1alpha1.VolumeGroupSnapshotClass, lister groupsnapshotlisters.VolumeGroupSnapshotClassLister) *v1.AdmissionResponse {
16890
reviewResponse := &v1.AdmissionResponse{
16991
Allowed: true,
@@ -200,55 +122,3 @@ func decideGroupSnapshotClassV1Alpha1(groupSnapClass, oldGroupSnapClass *volumeg
200122

201123
return reviewResponse
202124
}
203-
204-
func checkGroupSnapshotImmutableFieldsV1Alpha1(groupSnapshot, oldGroupSnapshot *volumegroupsnapshotv1alpha1.VolumeGroupSnapshot) error {
205-
if groupSnapshot == nil {
206-
return fmt.Errorf("VolumeGroupSnapshot is nil")
207-
}
208-
if oldGroupSnapshot == nil {
209-
return fmt.Errorf("old VolumeGroupSnapshot is nil")
210-
}
211-
212-
source := groupSnapshot.Spec.Source
213-
oldSource := oldGroupSnapshot.Spec.Source
214-
215-
if !reflect.DeepEqual(source.Selector, oldSource.Selector) {
216-
return fmt.Errorf("Spec.Source.Selector is immutable but was changed from %s to %s", oldSource.Selector, source.Selector)
217-
}
218-
if !reflect.DeepEqual(source.VolumeGroupSnapshotContentName, oldSource.VolumeGroupSnapshotContentName) {
219-
return fmt.Errorf("Spec.Source.VolumeGroupSnapshotContentName is immutable but was changed from %s to %s", strPtrDereference(oldSource.VolumeGroupSnapshotContentName), strPtrDereference(source.VolumeGroupSnapshotContentName))
220-
}
221-
222-
return nil
223-
}
224-
225-
func checkGroupSnapshotContentImmutableFieldsV1Alpha1(groupSnapcontent, oldGroupSnapcontent *volumegroupsnapshotv1alpha1.VolumeGroupSnapshotContent) error {
226-
if groupSnapcontent == nil {
227-
return fmt.Errorf("VolumeGroupSnapshotContent is nil")
228-
}
229-
if oldGroupSnapcontent == nil {
230-
return fmt.Errorf("old VolumeGroupSnapshotContent is nil")
231-
}
232-
233-
source := groupSnapcontent.Spec.Source
234-
oldSource := oldGroupSnapcontent.Spec.Source
235-
236-
if !reflect.DeepEqual(source.GroupSnapshotHandles, oldSource.GroupSnapshotHandles) {
237-
return fmt.Errorf("Spec.Source.GroupSnapshotHandles is immutable but was changed from %s to %s", oldSource.GroupSnapshotHandles, source.GroupSnapshotHandles)
238-
}
239-
if !reflect.DeepEqual(source.VolumeHandles, oldSource.VolumeHandles) {
240-
return fmt.Errorf("Spec.Source.VolumeHandles is immutable but was changed from %v to %v", oldSource.VolumeHandles, source.VolumeHandles)
241-
}
242-
243-
ref := groupSnapcontent.Spec.VolumeGroupSnapshotRef
244-
oldRef := oldGroupSnapcontent.Spec.VolumeGroupSnapshotRef
245-
246-
if ref.Name != oldRef.Name {
247-
return fmt.Errorf("Spec.VolumeGroupSnapshotRef.Name is immutable but was changed from %s to %s", oldRef.Name, ref.Name)
248-
}
249-
if ref.Namespace != oldRef.Namespace {
250-
return fmt.Errorf("Spec.VolumeGroupSnapshotRef.Namespace is immutable but was changed from %s to %s", oldRef.Namespace, ref.Namespace)
251-
}
252-
253-
return nil
254-
}

0 commit comments

Comments
 (0)