Skip to content

Commit d7f9e06

Browse files
Rajneesh180Rajneesh180
committed
fix(webhook): address review feedback — license headers, decoder-first validation, table-driven tests
- Add Apache 2.0 license headers to all files - Add kubebuilder:webhook marker for config generation - Remove incorrect mounts+runtimes required check (Mounts is optional) - Remove raw JSON fallback path in favor of decoder-only flow - Handle DELETE requests with empty Object.Raw gracefully - Validate individual mount entries and runtime entries - Rewrite tests as table-driven with proper TypeMeta/ObjectMeta Signed-off-by: Rajneesh180 <rajneeshrehsaan48@gmail.com>
1 parent 1daac4e commit d7f9e06

File tree

3 files changed

+158
-79
lines changed

3 files changed

+158
-79
lines changed

‎pkg/webhook/handler/validating/validating_handler.go‎

Lines changed: 43 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,23 @@
1+
/*
2+
Copyright 2021 The Fluid Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
117
package validating
218

319
import (
420
"context"
5-
"encoding/json"
6-
"net/http"
721

822
v1alpha1 "github.com/fluid-cloudnative/fluid/api/v1alpha1"
923
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
@@ -19,50 +33,45 @@ func NewValidatingHandler() *ValidatingHandler {
1933
}
2034

2135
func (h *ValidatingHandler) Handle(ctx context.Context, req admission.Request) admission.Response {
22-
// Try to decode into a known type (Dataset) when possible
36+
// DELETE requests may have an empty Object.Raw; allow them through.
37+
if len(req.Object.Raw) == 0 {
38+
return admission.Allowed("no object to validate")
39+
}
40+
41+
// Decode into a Dataset when possible.
2342
var ds v1alpha1.Dataset
2443
if h.decoder != nil {
2544
if err := h.decoder.Decode(req, &ds); err == nil {
26-
// Perform Dataset-specific validations
27-
// Require either Mounts or Runtimes to be present
28-
if len(ds.Spec.Mounts) == 0 && len(ds.Spec.Runtimes) == 0 {
29-
return admission.Denied("dataset.spec must contain at least one mount or runtime")
30-
}
31-
32-
// Validate mounts
33-
for _, m := range ds.Spec.Mounts {
34-
if m.MountPoint == "" {
35-
return admission.Denied("mount.mountPoint must not be empty")
36-
}
37-
}
38-
39-
// Validate runtimes
40-
for _, r := range ds.Spec.Runtimes {
41-
if r.Name == "" || r.Namespace == "" {
42-
return admission.Denied("runtime entries must include name and namespace")
43-
}
44-
}
45-
46-
return admission.Allowed("dataset validation passed")
45+
return h.validateDataset(&ds)
4746
}
4847
}
4948

50-
// Fallback generic validation: ensure object contains metadata.name
51-
var obj map[string]interface{}
52-
if err := json.Unmarshal(req.Object.Raw, &obj); err != nil {
53-
return admission.Errored(http.StatusBadRequest, err)
49+
// For unknown types, allow through (skeleton — extend as needed).
50+
return admission.Allowed("validation passed")
51+
}
52+
53+
func (h *ValidatingHandler) validateDataset(ds *v1alpha1.Dataset) admission.Response {
54+
// Mounts is optional (e.g. Vineyard runtime), so we only validate
55+
// individual entries when present.
56+
for _, m := range ds.Spec.Mounts {
57+
if m.MountPoint == "" {
58+
return admission.Denied("mount.mountPoint must not be empty")
59+
}
5460
}
5561

56-
metadata, ok := obj["metadata"].(map[string]interface{})
57-
if !ok {
58-
return admission.Denied("metadata.name is required")
62+
// Validate runtime entries when present.
63+
for _, r := range ds.Spec.Runtimes {
64+
if r.Name == "" || r.Namespace == "" {
65+
return admission.Denied("runtime entries must include name and namespace")
66+
}
5967
}
60-
name, ok := metadata["name"].(string)
61-
if !ok || name == "" {
68+
69+
// Require metadata.name (also covers generateName-only submissions).
70+
if ds.Name == "" {
6271
return admission.Denied("metadata.name is required")
6372
}
6473

65-
return admission.Allowed("validation passed")
74+
return admission.Allowed("dataset validation passed")
6675
}
6776

6877
func (h *ValidatingHandler) InjectDecoder(d *admission.Decoder) error {

‎pkg/webhook/handler/validating/validating_handler_test.go‎

Lines changed: 97 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -1,74 +1,126 @@
1+
/*
2+
Copyright 2021 The Fluid Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
117
package validating
218

319
import (
420
"context"
5-
"testing"
6-
721
"encoding/json"
22+
"testing"
823

924
"github.com/stretchr/testify/assert"
1025
admissionv1 "k8s.io/api/admission/v1"
26+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1127
"k8s.io/apimachinery/pkg/runtime"
1228
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
29+
1330
v1alpha1 "github.com/fluid-cloudnative/fluid/api/v1alpha1"
1431
)
1532

16-
func TestValidatingHandler_Basic(t *testing.T) {
17-
h := NewValidatingHandler()
33+
func TestValidatingHandler_EmptyRaw(t *testing.T) {
34+
h := newHandler(t)
1835

19-
// Valid object
20-
obj := []byte(`{"metadata":{"name":"test-dataset"}}`)
36+
// DELETE requests may have empty Object.Raw — should be allowed.
2137
req := admission.Request{
2238
AdmissionRequest: admissionv1.AdmissionRequest{
23-
Object: runtime.RawExtension{Raw: obj},
39+
Object: runtime.RawExtension{Raw: nil},
2440
},
2541
}
2642
resp := h.Handle(context.Background(), req)
2743
assert.True(t, resp.Allowed)
28-
29-
// Missing metadata.name
30-
obj = []byte(`{"metadata":{}}`)
31-
req.AdmissionRequest.Object.Raw = obj
32-
resp = h.Handle(context.Background(), req)
33-
assert.False(t, resp.Allowed)
34-
assert.Contains(t, resp.Result.Message, "metadata.name is required")
3544
}
3645

3746
func TestValidatingHandler_Dataset(t *testing.T) {
38-
h := NewValidatingHandler()
39-
// prepare decoder with scheme so that typed decoding works
40-
scheme := runtime.NewScheme()
41-
err := v1alpha1.AddToScheme(scheme)
42-
if err != nil {
43-
t.Fatalf("failed to add v1alpha1 to scheme: %v", err)
44-
}
45-
dec := admission.NewDecoder(scheme)
46-
h.InjectDecoder(dec)
47+
h := newHandler(t)
4748

48-
// Dataset missing mounts and runtimes -> denied
49-
ds := &v1alpha1.Dataset{}
50-
raw, _ := json.Marshal(ds)
51-
req := admission.Request{
52-
AdmissionRequest: admissionv1.AdmissionRequest{
53-
Object: runtime.RawExtension{Raw: raw},
49+
tests := []struct {
50+
name string
51+
ds *v1alpha1.Dataset
52+
allowed bool
53+
message string
54+
}{
55+
{
56+
name: "empty mounts and runtimes is allowed (mounts are optional)",
57+
ds: &v1alpha1.Dataset{ObjectMeta: metav1.ObjectMeta{Name: "ds"}},
58+
allowed: true,
59+
},
60+
{
61+
name: "valid mount is allowed",
62+
ds: &v1alpha1.Dataset{
63+
ObjectMeta: metav1.ObjectMeta{Name: "ds"},
64+
Spec: v1alpha1.DatasetSpec{Mounts: []v1alpha1.Mount{{MountPoint: "/data"}}},
65+
},
66+
allowed: true,
67+
},
68+
{
69+
name: "empty mountPoint is denied",
70+
ds: &v1alpha1.Dataset{
71+
ObjectMeta: metav1.ObjectMeta{Name: "ds"},
72+
Spec: v1alpha1.DatasetSpec{Mounts: []v1alpha1.Mount{{MountPoint: ""}}},
73+
},
74+
allowed: false,
75+
message: "mount.mountPoint must not be empty",
76+
},
77+
{
78+
name: "runtime missing namespace is denied",
79+
ds: &v1alpha1.Dataset{
80+
ObjectMeta: metav1.ObjectMeta{Name: "ds"},
81+
Spec: v1alpha1.DatasetSpec{Runtimes: []v1alpha1.Runtime{{Name: "alluxio"}}},
82+
},
83+
allowed: false,
84+
message: "runtime entries must include name and namespace",
85+
},
86+
{
87+
name: "missing metadata.name is denied",
88+
ds: &v1alpha1.Dataset{},
89+
allowed: false,
90+
message: "metadata.name is required",
5491
},
5592
}
56-
resp := h.Handle(context.Background(), req)
57-
assert.False(t, resp.Allowed)
5893

59-
// Dataset with a valid mount -> allowed
60-
ds = &v1alpha1.Dataset{}
61-
ds.Spec.Mounts = []v1alpha1.Mount{{MountPoint: "/data"}}
62-
raw, _ = json.Marshal(ds)
63-
req.AdmissionRequest.Object.Raw = raw
64-
resp = h.Handle(context.Background(), req)
65-
assert.True(t, resp.Allowed)
94+
for _, tc := range tests {
95+
t.Run(tc.name, func(t *testing.T) {
96+
// Set TypeMeta so the decoder can recognize the GVK.
97+
tc.ds.TypeMeta = metav1.TypeMeta{
98+
APIVersion: "data.fluid.io/v1alpha1",
99+
Kind: "Dataset",
100+
}
101+
raw, err := json.Marshal(tc.ds)
102+
assert.NoError(t, err)
103+
104+
req := admission.Request{
105+
AdmissionRequest: admissionv1.AdmissionRequest{
106+
Object: runtime.RawExtension{Raw: raw},
107+
},
108+
}
109+
resp := h.Handle(context.Background(), req)
110+
assert.Equal(t, tc.allowed, resp.Allowed, "test %q", tc.name)
111+
if tc.message != "" {
112+
assert.Contains(t, resp.Result.Message, tc.message)
113+
}
114+
})
115+
}
116+
}
66117

67-
// Dataset with invalid mount (too short) -> denied
68-
ds = &v1alpha1.Dataset{}
69-
ds.Spec.Mounts = []v1alpha1.Mount{{MountPoint: "abc"}}
70-
raw, _ = json.Marshal(ds)
71-
req.AdmissionRequest.Object.Raw = raw
72-
resp = h.Handle(context.Background(), req)
73-
assert.False(t, resp.Allowed)
118+
// newHandler creates a ValidatingHandler wired with a decoder for v1alpha1.
119+
func newHandler(t *testing.T) *ValidatingHandler {
120+
t.Helper()
121+
scheme := runtime.NewScheme()
122+
assert.NoError(t, v1alpha1.AddToScheme(scheme))
123+
h := NewValidatingHandler()
124+
h.InjectDecoder(admission.NewDecoder(scheme))
125+
return h
74126
}

‎pkg/webhook/handler/validating/webhook.go‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,19 @@
1+
/*
2+
Copyright 2021 The Fluid Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
117
package validating
218

319
import (
@@ -6,6 +22,8 @@ import (
622
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
723
)
824

25+
// +kubebuilder:webhook:path=/validate,mutating=false,failurePolicy=fail,sideEffects=None,admissionReviewVersions=v1;v1beta1,groups=data.fluid.io,resources=datasets,verbs=create;update,versions=v1alpha1,name=validate.fluid.io
26+
927
var HandlerMap = map[string]common.AdmissionHandler{
1028
"/validate": &Handler{},
1129
}

0 commit comments

Comments
 (0)