Skip to content

Commit 30dce62

Browse files
AshleyDumaineAshleyDumaine
authored
[improvement] add validation admission webhook checks for resource name length (#851)
* add validation admission webhook checks for resource name length * increase coverage for no-op webhook functions to appease codecov
1 parent 4a56243 commit 30dce62

16 files changed

+857
-86
lines changed

‎internal/webhook/v1alpha2/linodecluster_webhook.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,9 @@ func (r *linodeClusterValidator) ValidateCreate(ctx context.Context, obj runtime
6363

6464
// TODO: instrument with tracing, might need refactor to preserve readability
6565
var errs field.ErrorList
66-
66+
if err := validateLabelLength(cluster.GetName(), field.NewPath("metadata").Child("name")); err != nil {
67+
errs = append(errs, err)
68+
}
6769
if err := r.validateLinodeClusterSpec(ctx, linodeClient, spec, skipAPIValidation); err != nil {
6870
errs = slices.Concat(errs, err)
6971
}

‎internal/webhook/v1alpha2/linodecluster_webhook_test.go‎

Lines changed: 114 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ func TestValidateLinodeCluster(t *testing.T) {
8989
)
9090
}
9191

92-
func TestValidateCreate(t *testing.T) {
92+
func TestValidateLinodeClusterCreate(t *testing.T) {
9393
t.Parallel()
9494
ctrl := gomock.NewController(t)
9595
defer ctrl.Finish()
@@ -109,8 +109,19 @@ func TestValidateCreate(t *testing.T) {
109109
},
110110
},
111111
}
112-
expectedErrorSubString = "\"example\" is invalid: spec.region: Not found:"
113-
credentialsRefCluster = infrav1alpha2.LinodeCluster{
112+
clusterLongName = infrav1alpha2.LinodeCluster{
113+
ObjectMeta: metav1.ObjectMeta{
114+
Name: longName,
115+
Namespace: "example",
116+
},
117+
Spec: infrav1alpha2.LinodeClusterSpec{
118+
Region: "example",
119+
Network: infrav1alpha2.NetworkSpec{
120+
LoadBalancerType: "NodeBalancer",
121+
},
122+
},
123+
}
124+
credentialsRefCluster = infrav1alpha2.LinodeCluster{
114125
ObjectMeta: metav1.ObjectMeta{
115126
Name: "example",
116127
Namespace: "example",
@@ -136,7 +147,16 @@ func TestValidateCreate(t *testing.T) {
136147
}),
137148
Result("error", func(ctx context.Context, mck Mock) {
138149
_, err := validator.ValidateCreate(ctx, &cluster)
139-
assert.ErrorContains(t, err, expectedErrorSubString)
150+
assert.ErrorContains(t, err, "\"example\" is invalid: spec.region: Not found:")
151+
}),
152+
),
153+
Path(
154+
Call("name too long", func(ctx context.Context, mck Mock) {
155+
156+
}),
157+
Result("error", func(ctx context.Context, mck Mock) {
158+
_, err := validator.ValidateCreate(ctx, &clusterLongName)
159+
assert.ErrorContains(t, err, labelLengthDetail)
140160
}),
141161
),
142162
),
@@ -169,6 +189,96 @@ func TestValidateCreate(t *testing.T) {
169189
)
170190
}
171191

192+
func TestValidateLinodeClusterUpdate(t *testing.T) {
193+
t.Parallel()
194+
ctrl := gomock.NewController(t)
195+
defer ctrl.Finish()
196+
197+
mockK8sClient := mock.NewMockK8sClient(ctrl)
198+
199+
var (
200+
oldCluster = infrav1alpha2.LinodeCluster{
201+
ObjectMeta: metav1.ObjectMeta{
202+
Name: "example",
203+
Namespace: "example",
204+
},
205+
Spec: infrav1alpha2.LinodeClusterSpec{
206+
Region: "example",
207+
Network: infrav1alpha2.NetworkSpec{
208+
LoadBalancerType: "NodeBalancer",
209+
},
210+
},
211+
}
212+
newCluster = infrav1alpha2.LinodeCluster{
213+
ObjectMeta: metav1.ObjectMeta{
214+
Name: "example",
215+
Namespace: "example",
216+
},
217+
Spec: infrav1alpha2.LinodeClusterSpec{
218+
Region: "example",
219+
Network: infrav1alpha2.NetworkSpec{
220+
LoadBalancerType: "dns",
221+
},
222+
},
223+
}
224+
225+
validator = &linodeClusterValidator{Client: mockK8sClient}
226+
)
227+
228+
NewSuite(t, mock.MockLinodeClient{}).Run(
229+
OneOf(
230+
Path(
231+
Call("update", func(ctx context.Context, mck Mock) {
232+
233+
}),
234+
Result("success", func(ctx context.Context, mck Mock) {
235+
_, err := validator.ValidateUpdate(ctx, &oldCluster, &newCluster)
236+
assert.NoError(t, err)
237+
}),
238+
),
239+
),
240+
)
241+
}
242+
243+
func TestValidateLinodeClusterDelete(t *testing.T) {
244+
t.Parallel()
245+
ctrl := gomock.NewController(t)
246+
defer ctrl.Finish()
247+
248+
mockK8sClient := mock.NewMockK8sClient(ctrl)
249+
250+
var (
251+
cluster = infrav1alpha2.LinodeCluster{
252+
ObjectMeta: metav1.ObjectMeta{
253+
Name: "example",
254+
Namespace: "example",
255+
},
256+
Spec: infrav1alpha2.LinodeClusterSpec{
257+
Region: "example",
258+
Network: infrav1alpha2.NetworkSpec{
259+
LoadBalancerType: "NodeBalancer",
260+
},
261+
},
262+
}
263+
264+
validator = &linodeClusterValidator{Client: mockK8sClient}
265+
)
266+
267+
NewSuite(t, mock.MockLinodeClient{}).Run(
268+
OneOf(
269+
Path(
270+
Call("delete", func(ctx context.Context, mck Mock) {
271+
272+
}),
273+
Result("success", func(ctx context.Context, mck Mock) {
274+
_, err := validator.ValidateDelete(ctx, &cluster)
275+
assert.NoError(t, err)
276+
}),
277+
),
278+
),
279+
)
280+
}
281+
172282
func TestValidateDNSLinodeCluster(t *testing.T) {
173283
t.Parallel()
174284

‎internal/webhook/v1alpha2/linodefirewall_webhook.go‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,10 @@ import (
2020
"context"
2121
"fmt"
2222

23+
apierrors "k8s.io/apimachinery/pkg/api/errors"
2324
"k8s.io/apimachinery/pkg/runtime"
25+
"k8s.io/apimachinery/pkg/runtime/schema"
26+
"k8s.io/apimachinery/pkg/util/validation/field"
2427
ctrl "sigs.k8s.io/controller-runtime"
2528
logf "sigs.k8s.io/controller-runtime/pkg/log"
2629
"sigs.k8s.io/controller-runtime/pkg/webhook"
@@ -38,9 +41,6 @@ func SetupLinodeFirewallWebhookWithManager(mgr ctrl.Manager) error {
3841
Complete()
3942
}
4043

41-
// TODO(user): EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN!
42-
43-
// TODO(user): change verbs to "verbs=create;update;delete" if you want to enable deletion validation.
4444
// NOTE: The 'path' attribute must follow a specific pattern and should not be modified directly here.
4545
// Modifying the path for an invalid path can cause API server errors; failing to locate the webhook.
4646
// +kubebuilder:webhook:path=/validate-infrastructure-cluster-x-k8s-io-v1alpha2-linodefirewall,mutating=false,failurePolicy=fail,sideEffects=None,groups=infrastructure.cluster.x-k8s.io,resources=linodefirewalls,verbs=create;update,versions=v1alpha2,name=validation.linodefirewall.infrastructure.cluster.x-k8s.io,admissionReviewVersions=v1
@@ -64,8 +64,17 @@ func (v *LinodeFirewallCustomValidator) ValidateCreate(ctx context.Context, obj
6464
}
6565
linodefirewalllog.Info("Validation for LinodeFirewall upon creation", "name", linodefirewall.GetName())
6666

67-
// TODO(user): fill in your validation logic upon object creation.
68-
return nil, nil
67+
var errs field.ErrorList
68+
if err := validateLabelLength(linodefirewall.GetName(), field.NewPath("metadata").Child("name")); err != nil {
69+
errs = append(errs, err)
70+
}
71+
72+
if len(errs) == 0 {
73+
return nil, nil
74+
}
75+
return nil, apierrors.NewInvalid(
76+
schema.GroupKind{Group: "infrastructure.cluster.x-k8s.io", Kind: "LinodeFirewall"},
77+
linodefirewall.Name, errs)
6978
}
7079

7180
// ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type LinodeFirewall.

‎internal/webhook/v1alpha2/linodefirewall_webhook_test.go‎

Lines changed: 126 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -17,34 +17,136 @@ limitations under the License.
1717
package v1alpha2
1818

1919
import (
20+
"context"
21+
"testing"
22+
23+
"github.com/stretchr/testify/assert"
24+
"github.com/stretchr/testify/require"
25+
"go.uber.org/mock/gomock"
26+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
27+
2028
infrav1alpha2 "github.com/linode/cluster-api-provider-linode/api/v1alpha2"
29+
"github.com/linode/cluster-api-provider-linode/mock"
2130

22-
. "github.com/onsi/ginkgo/v2"
23-
. "github.com/onsi/gomega"
31+
. "github.com/linode/cluster-api-provider-linode/mock/mocktest"
2432
)
2533

26-
var _ = Describe("LinodeFirewall Webhook", func() {
34+
func TestValidateLinodeFirewallCreate(t *testing.T) {
35+
t.Parallel()
36+
ctrl := gomock.NewController(t)
37+
defer ctrl.Finish()
38+
39+
var (
40+
lfw = infrav1alpha2.LinodeFirewall{
41+
ObjectMeta: metav1.ObjectMeta{
42+
Name: "example",
43+
Namespace: "example",
44+
},
45+
Spec: infrav1alpha2.LinodeFirewallSpec{},
46+
}
47+
lfwLongName = infrav1alpha2.LinodeFirewall{
48+
ObjectMeta: metav1.ObjectMeta{
49+
Name: longName,
50+
Namespace: "example",
51+
},
52+
Spec: infrav1alpha2.LinodeFirewallSpec{},
53+
}
54+
validator = &LinodeFirewallCustomValidator{}
55+
)
56+
57+
NewSuite(t, mock.MockLinodeClient{}).Run(
58+
OneOf(
59+
Path(
60+
Call("name too long", func(ctx context.Context, mck Mock) {
61+
62+
}),
63+
Result("error", func(ctx context.Context, mck Mock) {
64+
_, err := validator.ValidateCreate(ctx, &lfwLongName)
65+
assert.ErrorContains(t, err, labelLengthDetail)
66+
}),
67+
),
68+
),
69+
OneOf(
70+
Path(
71+
Call("valid", func(ctx context.Context, mck Mock) {
72+
73+
}),
74+
Result("success", func(ctx context.Context, mck Mock) {
75+
_, err := validator.ValidateCreate(ctx, &lfw)
76+
require.NoError(t, err)
77+
}),
78+
),
79+
),
80+
)
81+
}
82+
83+
func TestValidateLinodeFirewallUpdate(t *testing.T) {
84+
t.Parallel()
85+
ctrl := gomock.NewController(t)
86+
defer ctrl.Finish()
87+
2788
var (
28-
obj *infrav1alpha2.LinodeFirewall
29-
oldObj *infrav1alpha2.LinodeFirewall
30-
validator LinodeFirewallCustomValidator
89+
oldFW = infrav1alpha2.LinodeFirewall{
90+
ObjectMeta: metav1.ObjectMeta{
91+
Name: "example",
92+
Namespace: "example",
93+
},
94+
Spec: infrav1alpha2.LinodeFirewallSpec{},
95+
}
96+
newFW = infrav1alpha2.LinodeFirewall{
97+
ObjectMeta: metav1.ObjectMeta{
98+
Name: "example",
99+
Namespace: "example",
100+
},
101+
Spec: infrav1alpha2.LinodeFirewallSpec{},
102+
}
103+
104+
validator = &LinodeFirewallCustomValidator{}
105+
)
106+
107+
NewSuite(t, mock.MockLinodeClient{}).Run(
108+
OneOf(
109+
Path(
110+
Call("update", func(ctx context.Context, mck Mock) {
111+
112+
}),
113+
Result("success", func(ctx context.Context, mck Mock) {
114+
_, err := validator.ValidateUpdate(ctx, &oldFW, &newFW)
115+
assert.NoError(t, err)
116+
}),
117+
),
118+
),
31119
)
120+
}
121+
122+
func TestValidateLinodeFirewallDelete(t *testing.T) {
123+
t.Parallel()
124+
ctrl := gomock.NewController(t)
125+
defer ctrl.Finish()
32126

33-
BeforeEach(func() {
34-
obj = &infrav1alpha2.LinodeFirewall{}
35-
oldObj = &infrav1alpha2.LinodeFirewall{}
36-
validator = LinodeFirewallCustomValidator{}
37-
Expect(validator).NotTo(BeNil(), "Expected validator to be initialized")
38-
Expect(oldObj).NotTo(BeNil(), "Expected oldObj to be initialized")
39-
Expect(obj).NotTo(BeNil(), "Expected obj to be initialized")
40-
// TODO (user): Add any setup logic common to all tests
41-
})
42-
43-
AfterEach(func() {
44-
// TODO (user): Add any teardown logic common to all tests
45-
})
46-
47-
Context("When creating or updating LinodeFirewall under Validating Webhook", func() {
48-
// TODO (user): Add logic for validating webhooks
49-
})
50-
})
127+
var (
128+
lfw = infrav1alpha2.LinodeFirewall{
129+
ObjectMeta: metav1.ObjectMeta{
130+
Name: "example",
131+
Namespace: "example",
132+
},
133+
Spec: infrav1alpha2.LinodeFirewallSpec{},
134+
}
135+
136+
validator = &LinodeFirewallCustomValidator{}
137+
)
138+
139+
NewSuite(t, mock.MockLinodeClient{}).Run(
140+
OneOf(
141+
Path(
142+
Call("delete", func(ctx context.Context, mck Mock) {
143+
144+
}),
145+
Result("success", func(ctx context.Context, mck Mock) {
146+
_, err := validator.ValidateDelete(ctx, &lfw)
147+
assert.NoError(t, err)
148+
}),
149+
),
150+
),
151+
)
152+
}

‎internal/webhook/v1alpha2/linodemachine_webhook.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,8 +69,6 @@ func SetupLinodeMachineWebhookWithManager(mgr ctrl.Manager) error {
6969

7070
// ValidateCreate implements webhook.Validator so a webhook will be registered for the type
7171
func (r *linodeMachineValidator) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
72-
var errs field.ErrorList
73-
7472
machine, ok := obj.(*infrav1alpha2.LinodeMachine)
7573
if !ok {
7674
return nil, apierrors.NewBadRequest("expected a LinodeMachine Resource")
@@ -81,6 +79,10 @@ func (r *linodeMachineValidator) ValidateCreate(ctx context.Context, obj runtime
8179
skipAPIValidation, linodeClient := setupClientWithCredentials(ctx, r.Client, spec.CredentialsRef,
8280
machine.Name, machine.GetNamespace(), linodemachinelog)
8381

82+
var errs field.ErrorList
83+
if err := validateLabelLength(machine.GetName(), field.NewPath("metadata").Child("name")); err != nil {
84+
errs = append(errs, err)
85+
}
8486
if err := r.validateLinodeMachineSpec(ctx, linodeClient, spec, skipAPIValidation); err != nil {
8587
errs = slices.Concat(errs, err)
8688
}

0 commit comments

Comments
 (0)