Skip to content

Commit d1dd6f4

Browse files
cpanatocpanato
committed
improve cosigned validation error messages
Signed-off-by: cpanato <[email protected]>
1 parent 36d7646 commit d1dd6f4

File tree

2 files changed

+90
-67
lines changed

2 files changed

+90
-67
lines changed

‎pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation.go

+4-3
Original file line numberDiff line numberDiff line change
@@ -29,17 +29,18 @@ func (policy *ClusterImagePolicy) Validate(ctx context.Context) *apis.FieldError
2929

3030
func (spec *ClusterImagePolicySpec) Validate(ctx context.Context) (errors *apis.FieldError) {
3131
if len(spec.Images) == 0 {
32-
errors = errors.Also(apis.ErrGeneric("At least one image should be defined").ViaField("images"))
32+
errors = errors.Also(apis.ErrMissingField("images"))
3333
}
3434
for i, image := range spec.Images {
3535
errors = errors.Also(image.Validate(ctx).ViaFieldIndex("images", i))
3636
}
3737
if len(spec.Authorities) == 0 {
38-
errors = errors.Also(apis.ErrGeneric("At least one authority should be defined").ViaField("authorities"))
38+
errors = errors.Also(apis.ErrMissingField("authorities"))
3939
}
4040
for i, authority := range spec.Authorities {
4141
errors = errors.Also(authority.Validate(ctx).ViaFieldIndex("authorities", i))
4242
}
43+
4344
return
4445
}
4546

@@ -121,7 +122,7 @@ func (keyless *KeylessRef) Validate(ctx context.Context) *apis.FieldError {
121122
}
122123

123124
if keyless.Identities != nil && len(keyless.Identities) == 0 {
124-
errs = errs.Also(apis.ErrGeneric("At least one identity must be provided"))
125+
errs = errs.Also(apis.ErrMissingField("identities"))
125126
}
126127

127128
for i, identity := range keyless.Identities {

‎pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation_test.go

+86-64
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"strings"
2020
"testing"
2121

22+
"github.com/stretchr/testify/require"
2223
"knative.dev/pkg/apis"
2324
)
2425

@@ -30,9 +31,9 @@ func TestImagePatternValidation(t *testing.T) {
3031
policy ClusterImagePolicy
3132
}{
3233
{
33-
name: "Should fail when both regex and glob are present: %v",
34+
name: "Should fail when both regex and glob are present",
3435
expectErr: true,
35-
errorString: "expected exactly one, got both: spec.images[0].glob, spec.images[0].regex",
36+
errorString: "expected exactly one, got both: spec.images[0].glob, spec.images[0].regex\ninvalid value: **: spec.images[0].glob\nglob match supports only a single * as a trailing character\nmissing field(s): spec.authorities\nmust not set the field(s): spec.images[0].regex",
3637
policy: ClusterImagePolicy{
3738
Spec: ClusterImagePolicySpec{
3839
Images: []ImagePattern{
@@ -45,9 +46,9 @@ func TestImagePatternValidation(t *testing.T) {
4546
},
4647
},
4748
{
48-
name: "Should fail when neither regex nor glob are present: %v",
49+
name: "Should fail when neither regex nor glob are present",
4950
expectErr: true,
50-
errorString: "expected exactly one, got neither: spec.images[0].glob, spec.images[0].regex",
51+
errorString: "expected exactly one, got neither: spec.images[0].glob, spec.images[0].regex\nmissing field(s): spec.authorities",
5152
policy: ClusterImagePolicy{
5253
Spec: ClusterImagePolicySpec{
5354
Images: []ImagePattern{
@@ -57,9 +58,9 @@ func TestImagePatternValidation(t *testing.T) {
5758
},
5859
},
5960
{
60-
name: "Glob should fail with multiple *: %v",
61+
name: "Glob should fail with multiple *",
6162
expectErr: true,
62-
errorString: "glob match supports only a single * as a trailing character",
63+
errorString: "invalid value: **: spec.images[0].glob\nglob match supports only a single * as a trailing character\nmissing field(s): spec.authorities",
6364
policy: ClusterImagePolicy{
6465
Spec: ClusterImagePolicySpec{
6566
Images: []ImagePattern{
@@ -71,9 +72,9 @@ func TestImagePatternValidation(t *testing.T) {
7172
},
7273
},
7374
{
74-
name: "Glob should fail with non-trailing *: %v",
75+
name: "Glob should fail with non-trailing *",
7576
expectErr: true,
76-
errorString: "glob match supports only * as a trailing character",
77+
errorString: "invalid value: foo*bar: spec.images[0].glob\nglob match supports only * as a trailing character\nmissing field(s): spec.authorities",
7778
policy: ClusterImagePolicy{
7879
Spec: ClusterImagePolicySpec{
7980
Images: []ImagePattern{
@@ -84,13 +85,26 @@ func TestImagePatternValidation(t *testing.T) {
8485
},
8586
},
8687
},
88+
{
89+
name: "missing image and authorities in the spec",
90+
expectErr: true,
91+
errorString: "missing field(s): spec.authorities, spec.images",
92+
policy: ClusterImagePolicy{
93+
Spec: ClusterImagePolicySpec{},
94+
},
95+
},
8796
}
8897

8998
for _, test := range tests {
90-
err := test.policy.Validate(context.TODO())
91-
if test.expectErr && !strings.Contains(err.Error(), test.errorString) {
92-
t.Errorf(test.name, err)
93-
}
99+
t.Run(test.name, func(t *testing.T) {
100+
err := test.policy.Validate(context.TODO())
101+
if test.expectErr {
102+
require.NotNil(t, err)
103+
require.EqualError(t, err, test.errorString)
104+
} else {
105+
require.Nil(t, err)
106+
}
107+
})
94108
}
95109
}
96110

@@ -102,9 +116,9 @@ func TestKeyValidation(t *testing.T) {
102116
policy ClusterImagePolicy
103117
}{
104118
{
105-
name: "Should fail when key has multiple properties: %v",
119+
name: "Should fail when key has multiple properties",
106120
expectErr: true,
107-
errorString: "expected exactly one, got both",
121+
errorString: "expected exactly one, got both: spec.authorities[0].key.data, spec.authorities[0].key.kms, spec.authorities[0].key.secretref",
108122
policy: ClusterImagePolicy{
109123
Spec: ClusterImagePolicySpec{
110124
Images: []ImagePattern{
@@ -124,9 +138,9 @@ func TestKeyValidation(t *testing.T) {
124138
},
125139
},
126140
{
127-
name: "Should fail when key has mixed valid and invalid data: %v",
141+
name: "Should fail when key has mixed valid and invalid data",
128142
expectErr: true,
129-
errorString: "invalid value: -----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEaEOVJCFtduYr3xqTxeRWSW32CY/s\nTBNZj4oIUPl8JvhVPJ1TKDPlNcuT4YphSt6t3yOmMvkdQbCj8broX6vijw==\n-----END PUBLIC KEY-----\n---somedata---",
143+
errorString: "invalid value: -----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEaEOVJCFtduYr3xqTxeRWSW32CY/s\nTBNZj4oIUPl8JvhVPJ1TKDPlNcuT4YphSt6t3yOmMvkdQbCj8broX6vijw==\n-----END PUBLIC KEY-----\n---somedata---: spec.authorities[0].key.data",
130144
policy: ClusterImagePolicy{
131145
Spec: ClusterImagePolicySpec{
132146
Images: []ImagePattern{
@@ -145,9 +159,9 @@ func TestKeyValidation(t *testing.T) {
145159
},
146160
},
147161
{
148-
name: "Should fail when key has malformed pubkey data: %v",
162+
name: "Should fail when key has malformed pubkey data",
149163
expectErr: true,
150-
errorString: "invalid value: ---some key data----",
164+
errorString: "invalid value: ---some key data----: spec.authorities[0].key.data",
151165
policy: ClusterImagePolicy{
152166
Spec: ClusterImagePolicySpec{
153167
Images: []ImagePattern{
@@ -166,9 +180,9 @@ func TestKeyValidation(t *testing.T) {
166180
},
167181
},
168182
{
169-
name: "Should fail when key is empty: %v",
183+
name: "Should fail when key is empty",
170184
expectErr: true,
171-
errorString: "expected exactly one, got neither",
185+
errorString: "expected exactly one, got neither: spec.authorities[0].key.data, spec.authorities[0].key.kms, spec.authorities[0].key.secretref",
172186
policy: ClusterImagePolicy{
173187
Spec: ClusterImagePolicySpec{
174188
Images: []ImagePattern{
@@ -185,7 +199,7 @@ func TestKeyValidation(t *testing.T) {
185199
},
186200
},
187201
{
188-
name: "Should fail when regex is given: %v",
202+
name: "Should fail when regex is given",
189203
expectErr: true,
190204
errorString: "must not set the field(s): spec.images[0].regex",
191205
policy: ClusterImagePolicy{
@@ -206,8 +220,8 @@ func TestKeyValidation(t *testing.T) {
206220
},
207221
},
208222
{
209-
name: "Should pass when key has only one property: %v",
210-
errorString: "",
223+
name: "Should pass when key has only one property",
224+
expectErr: false,
211225
policy: ClusterImagePolicy{
212226
Spec: ClusterImagePolicySpec{
213227
Images: []ImagePattern{
@@ -228,13 +242,15 @@ func TestKeyValidation(t *testing.T) {
228242
}
229243

230244
for _, test := range tests {
231-
err := test.policy.Validate(context.TODO())
232-
if test.expectErr && !strings.Contains(err.Error(), test.errorString) {
233-
t.Errorf(test.name, err)
234-
}
235-
if !test.expectErr && err != nil {
236-
t.Errorf(test.name, err)
237-
}
245+
t.Run(test.name, func(t *testing.T) {
246+
err := test.policy.Validate(context.TODO())
247+
if test.expectErr {
248+
require.NotNil(t, err)
249+
require.EqualError(t, err, test.errorString)
250+
} else {
251+
require.Nil(t, err)
252+
}
253+
})
238254
}
239255
}
240256

@@ -246,7 +262,7 @@ func TestKeylessValidation(t *testing.T) {
246262
policy ClusterImagePolicy
247263
}{
248264
{
249-
name: "Should fail when keyless is empty: %v",
265+
name: "Should fail when keyless is empty",
250266
expectErr: true,
251267
errorString: "expected exactly one, got neither",
252268
policy: ClusterImagePolicy{
@@ -265,7 +281,7 @@ func TestKeylessValidation(t *testing.T) {
265281
},
266282
},
267283
{
268-
name: "Should fail when keyless has multiple properties: %v",
284+
name: "Should fail when keyless has multiple properties",
269285
expectErr: true,
270286
errorString: "expected exactly one, got both",
271287
policy: ClusterImagePolicy{
@@ -291,8 +307,8 @@ func TestKeylessValidation(t *testing.T) {
291307
},
292308
},
293309
{
294-
name: "Should pass when a valid keyless ref is specified: %v",
295-
errorString: "",
310+
name: "Should pass when a valid keyless ref is specified",
311+
expectErr: false,
296312
policy: ClusterImagePolicy{
297313
Spec: ClusterImagePolicySpec{
298314
Images: []ImagePattern{
@@ -313,14 +329,17 @@ func TestKeylessValidation(t *testing.T) {
313329
},
314330
},
315331
}
332+
316333
for _, test := range tests {
317-
err := test.policy.Validate(context.TODO())
318-
if test.expectErr && !strings.Contains(err.Error(), test.errorString) {
319-
t.Errorf(test.name, err)
320-
}
321-
if !test.expectErr && err != nil {
322-
t.Errorf(test.name, err)
323-
}
334+
t.Run(test.name, func(t *testing.T) {
335+
err := test.policy.Validate(context.TODO())
336+
if test.expectErr {
337+
require.NotNil(t, err)
338+
require.True(t, strings.Contains(err.Error(), test.errorString))
339+
} else {
340+
require.Nil(t, err)
341+
}
342+
})
324343
}
325344
}
326345

@@ -332,9 +351,9 @@ func TestAuthoritiesValidation(t *testing.T) {
332351
policy ClusterImagePolicy
333352
}{
334353
{
335-
name: "Should fail when keyless is empty: %v",
354+
name: "Should fail when keyless is empty",
336355
expectErr: true,
337-
errorString: "expected exactly one, got both",
356+
errorString: "expected exactly one, got both: spec.authorities[0].key, spec.authorities[0].keyless\nexpected exactly one, got neither: spec.authorities[0].key.data, spec.authorities[0].key.kms, spec.authorities[0].key.secretref, spec.authorities[0].keyless.ca-key, spec.authorities[0].keyless.identities, spec.authorities[0].keyless.url",
338357
policy: ClusterImagePolicy{
339358
Spec: ClusterImagePolicySpec{
340359
Images: []ImagePattern{
@@ -351,11 +370,10 @@ func TestAuthoritiesValidation(t *testing.T) {
351370
},
352371
},
353372
},
354-
355373
{
356-
name: "Should fail when keyless is empty: %v",
374+
name: "Should fail when keyless is empty",
357375
expectErr: true,
358-
errorString: "At least one authority should be defined",
376+
errorString: "missing field(s): spec.authorities",
359377
policy: ClusterImagePolicy{
360378
Spec: ClusterImagePolicySpec{
361379
Images: []ImagePattern{
@@ -369,13 +387,15 @@ func TestAuthoritiesValidation(t *testing.T) {
369387
},
370388
}
371389
for _, test := range tests {
372-
err := test.policy.Validate(context.TODO())
373-
if test.expectErr && !strings.Contains(err.Error(), test.errorString) {
374-
t.Errorf(test.name, err)
375-
}
376-
if !test.expectErr && err != nil {
377-
t.Errorf(test.name, err)
378-
}
390+
t.Run(test.name, func(t *testing.T) {
391+
err := test.policy.Validate(context.TODO())
392+
if test.expectErr {
393+
require.NotNil(t, err)
394+
require.EqualError(t, err, test.errorString)
395+
} else {
396+
require.Nil(t, err)
397+
}
398+
})
379399
}
380400
}
381401

@@ -387,9 +407,9 @@ func TestIdentitiesValidation(t *testing.T) {
387407
policy ClusterImagePolicy
388408
}{
389409
{
390-
name: "Should fail when identities is empty: %v",
410+
name: "Should fail when identities is empty",
391411
expectErr: true,
392-
errorString: "At least one identity must be provided",
412+
errorString: "missing field(s): spec.authorities[0].keyless.identities",
393413
policy: ClusterImagePolicy{
394414
Spec: ClusterImagePolicySpec{
395415
Images: []ImagePattern{
@@ -408,8 +428,8 @@ func TestIdentitiesValidation(t *testing.T) {
408428
},
409429
},
410430
{
411-
name: "Should pass when identities is valid: %v",
412-
errorString: "",
431+
name: "Should pass when identities is valid",
432+
expectErr: false,
413433
policy: ClusterImagePolicy{
414434
Spec: ClusterImagePolicySpec{
415435
Images: []ImagePattern{
@@ -433,12 +453,14 @@ func TestIdentitiesValidation(t *testing.T) {
433453
},
434454
}
435455
for _, test := range tests {
436-
err := test.policy.Validate(context.TODO())
437-
if test.expectErr && !strings.Contains(err.Error(), test.errorString) {
438-
t.Errorf(test.name, err)
439-
}
440-
if !test.expectErr && err != nil {
441-
t.Errorf(test.name, err)
442-
}
456+
t.Run(test.name, func(t *testing.T) {
457+
err := test.policy.Validate(context.TODO())
458+
if test.expectErr {
459+
require.NotNil(t, err)
460+
require.EqualError(t, err, test.errorString)
461+
} else {
462+
require.Nil(t, err)
463+
}
464+
})
443465
}
444466
}

0 commit comments

Comments
 (0)