Skip to content

Commit 9dd1d3c

Browse files
AshleyDumaineAshleyDumaine
committed
add validation admission webhook checks for resource name length
1 parent 9f4d0a7 commit 9dd1d3c

16 files changed

+301
-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: 24 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
),

‎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: 55 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -17,34 +17,65 @@ 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+
2739
var (
28-
obj *infrav1alpha2.LinodeFirewall
29-
oldObj *infrav1alpha2.LinodeFirewall
30-
validator LinodeFirewallCustomValidator
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{}
3155
)
3256

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-
})
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+
}

‎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
}

‎internal/webhook/v1alpha2/linodemachine_webhook_test.go‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,16 @@ func TestValidateCreateLinodeMachine(t *testing.T) {
237237
Type: "example",
238238
},
239239
}
240+
machineLongName = infrav1alpha2.LinodeMachine{
241+
ObjectMeta: metav1.ObjectMeta{
242+
Name: longName,
243+
Namespace: "example",
244+
},
245+
Spec: infrav1alpha2.LinodeMachineSpec{
246+
Region: "example",
247+
Type: "example",
248+
},
249+
}
240250
credentialsRefMachine = infrav1alpha2.LinodeMachine{
241251
ObjectMeta: metav1.ObjectMeta{
242252
Name: "example",
@@ -250,8 +260,7 @@ func TestValidateCreateLinodeMachine(t *testing.T) {
250260
Type: "example",
251261
},
252262
}
253-
expectedErrorSubString = "\"example\" is invalid: [spec.region: Not found: \"example\", spec.type: Not found: \"example\"]"
254-
validator = &linodeMachineValidator{}
263+
validator = &linodeMachineValidator{}
255264
)
256265

257266
NewSuite(t, mock.MockLinodeClient{}).Run(
@@ -261,7 +270,15 @@ func TestValidateCreateLinodeMachine(t *testing.T) {
261270
}),
262271
Result("error", func(ctx context.Context, mck Mock) {
263272
_, err := validator.ValidateCreate(ctx, &machine)
264-
assert.ErrorContains(t, err, expectedErrorSubString)
273+
assert.ErrorContains(t, err, "\"example\" is invalid: [spec.region: Not found: \"example\", spec.type: Not found: \"example\"]")
274+
}),
275+
),
276+
Path(
277+
Call("name too long", func(ctx context.Context, mck Mock) {
278+
}),
279+
Result("error", func(ctx context.Context, mck Mock) {
280+
_, err := validator.ValidateCreate(ctx, &machineLongName)
281+
assert.ErrorContains(t, err, labelLengthDetail)
265282
}),
266283
),
267284
),

‎internal/webhook/v1alpha2/linodeobjectstoragebucket_webhook.go‎

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,21 @@ func (v *LinodeObjectStorageBucketCustomValidator) ValidateCreate(ctx context.Co
6464
linodeobjectstoragebucketlog.Info("validate create", "name", bucket.Name)
6565
skipAPIValidation, linodeClient := setupClientWithCredentials(ctx, v.Client, bucket.Spec.CredentialsRef,
6666
bucket.Name, bucket.GetNamespace(), linodemachinelog)
67-
return nil, v.validateLinodeObjectStorageBucket(ctx, bucket, linodeClient, skipAPIValidation)
67+
68+
var errs field.ErrorList
69+
if err := validateLabelLength(bucket.GetName(), field.NewPath("metadata").Child("name")); err != nil {
70+
errs = append(errs, err)
71+
}
72+
if err := v.validateLinodeObjectStorageBucketSpec(ctx, bucket, linodeClient, skipAPIValidation); err != nil {
73+
errs = slices.Concat(errs, err)
74+
}
75+
76+
if len(errs) == 0 {
77+
return nil, nil
78+
}
79+
return nil, apierrors.NewInvalid(
80+
schema.GroupKind{Group: "infrastructure.cluster.x-k8s.io", Kind: "LinodeObjectStorageBucket"},
81+
bucket.Name, errs)
6882
}
6983

7084
// ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type LinodeObjectStorageBucket.
@@ -90,21 +104,6 @@ func (v *LinodeObjectStorageBucketCustomValidator) ValidateDelete(ctx context.Co
90104
return nil, nil
91105
}
92106

93-
func (v *LinodeObjectStorageBucketCustomValidator) validateLinodeObjectStorageBucket(ctx context.Context, bucket *infrav1alpha2.LinodeObjectStorageBucket, linodeClient clients.LinodeClient, skipAPIValidation bool) error {
94-
var errs field.ErrorList
95-
96-
if err := v.validateLinodeObjectStorageBucketSpec(ctx, bucket, linodeClient, skipAPIValidation); err != nil {
97-
errs = slices.Concat(errs, err)
98-
}
99-
100-
if len(errs) == 0 {
101-
return nil
102-
}
103-
return apierrors.NewInvalid(
104-
schema.GroupKind{Group: "infrastructure.cluster.x-k8s.io", Kind: "LinodeObjectStorageBucket"},
105-
bucket.Name, errs)
106-
}
107-
108107
func (v *LinodeObjectStorageBucketCustomValidator) validateLinodeObjectStorageBucketSpec(ctx context.Context, bucket *infrav1alpha2.LinodeObjectStorageBucket, linodeClient clients.LinodeClient, skipAPIValidation bool) field.ErrorList {
109108
var errs field.ErrorList
110109
if skipAPIValidation {

‎internal/webhook/v1alpha2/linodeobjectstoragebucket_webhook_test.go‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ import (
3333
. "github.com/linode/cluster-api-provider-linode/mock/mocktest"
3434
)
3535

36-
func TestValidateLinodeObjectStorageBucket(t *testing.T) {
36+
func TestValidateLinodeObjectStorageBucketSpec(t *testing.T) {
3737
t.Parallel()
3838

3939
var (
@@ -63,7 +63,7 @@ func TestValidateLinodeObjectStorageBucket(t *testing.T) {
6363
Result("success", func(ctx context.Context, mck Mock) {
6464
bucket := bucket
6565
bucket.Spec.Region = "iad"
66-
assert.NoError(t, objvalidator.validateLinodeObjectStorageBucket(ctx, &bucket, mck.LinodeClient, true))
66+
assert.Nil(t, objvalidator.validateLinodeObjectStorageBucketSpec(ctx, &bucket, mck.LinodeClient, true))
6767
}),
6868
),
6969
Path(
@@ -75,7 +75,7 @@ func TestValidateLinodeObjectStorageBucket(t *testing.T) {
7575
Result("success", func(ctx context.Context, mck Mock) {
7676
bucket := bucket
7777
bucket.Spec.Region = "us-iad"
78-
assert.NoError(t, objvalidator.validateLinodeObjectStorageBucket(ctx, &bucket, mck.LinodeClient, true))
78+
assert.Nil(t, objvalidator.validateLinodeObjectStorageBucketSpec(ctx, &bucket, mck.LinodeClient, true))
7979
}),
8080
),
8181
Path(
@@ -87,7 +87,7 @@ func TestValidateLinodeObjectStorageBucket(t *testing.T) {
8787
Result("success", func(ctx context.Context, mck Mock) {
8888
bucket := bucket
8989
bucket.Spec.Region = "us-iad-1"
90-
assert.NoError(t, objvalidator.validateLinodeObjectStorageBucket(ctx, &bucket, mck.LinodeClient, true))
90+
assert.Nil(t, objvalidator.validateLinodeObjectStorageBucketSpec(ctx, &bucket, mck.LinodeClient, true))
9191
}),
9292
),
9393
),
@@ -98,7 +98,7 @@ func TestValidateLinodeObjectStorageBucket(t *testing.T) {
9898
Result("error", func(ctx context.Context, mck Mock) {
9999
bucket := bucket
100100
bucket.Spec.Region = "123invalid"
101-
assert.Error(t, objvalidator.validateLinodeObjectStorageBucket(ctx, &bucket, mck.LinodeClient, false))
101+
assert.NotNil(t, objvalidator.validateLinodeObjectStorageBucketSpec(ctx, &bucket, mck.LinodeClient, false))
102102
}),
103103
),
104104
Path(
@@ -107,7 +107,7 @@ func TestValidateLinodeObjectStorageBucket(t *testing.T) {
107107
Result("error", func(ctx context.Context, mck Mock) {
108108
bucket := bucket
109109
bucket.Spec.Region = "invalid-2-2"
110-
assert.Error(t, objvalidator.validateLinodeObjectStorageBucket(ctx, &bucket, mck.LinodeClient, false))
110+
assert.NotNil(t, objvalidator.validateLinodeObjectStorageBucketSpec(ctx, &bucket, mck.LinodeClient, false))
111111
}),
112112
),
113113
Path(
@@ -117,7 +117,7 @@ func TestValidateLinodeObjectStorageBucket(t *testing.T) {
117117
Result("error", func(ctx context.Context, mck Mock) {
118118
bucket := bucket
119119
bucket.Spec.Region = "us-1"
120-
assert.Error(t, objvalidator.validateLinodeObjectStorageBucket(ctx, &bucket, mck.LinodeClient, false))
120+
assert.NotNil(t, objvalidator.validateLinodeObjectStorageBucketSpec(ctx, &bucket, mck.LinodeClient, false))
121121
}),
122122
),
123123
Path(
@@ -127,7 +127,7 @@ func TestValidateLinodeObjectStorageBucket(t *testing.T) {
127127
mck.LinodeClient.EXPECT().GetRegion(gomock.Any(), gomock.Any()).Return(&region, nil).AnyTimes()
128128
}),
129129
Result("error", func(ctx context.Context, mck Mock) {
130-
assert.Error(t, objvalidator.validateLinodeObjectStorageBucket(ctx, &bucket, mck.LinodeClient, false))
130+
assert.NotNil(t, objvalidator.validateLinodeObjectStorageBucketSpec(ctx, &bucket, mck.LinodeClient, false))
131131
}),
132132
),
133133
),

0 commit comments

Comments
 (0)