Skip to content

Commit 459d722

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #1091 from leonardoce/remove-webhook
Remove duplicate validation logic from webhook
2 parents 52b72d3 + 9461f70 commit 459d722

File tree

8 files changed

+9
-1173
lines changed

8 files changed

+9
-1173
lines changed

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

+2-2
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

+1-113
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

+2-132
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)