Skip to content

Commit 04877f4

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

File tree

2 files changed

+92
-70
lines changed

2 files changed

+92
-70
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

+88-67
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,9 @@ package v1alpha1
1616

1717
import (
1818
"context"
19-
"strings"
2019
"testing"
2120

21+
"github.com/stretchr/testify/require"
2222
"knative.dev/pkg/apis"
2323
)
2424

@@ -30,9 +30,9 @@ func TestImagePatternValidation(t *testing.T) {
3030
policy ClusterImagePolicy
3131
}{
3232
{
33-
name: "Should fail when both regex and glob are present: %v",
33+
name: "Should fail when both regex and glob are present",
3434
expectErr: true,
35-
errorString: "expected exactly one, got both: spec.images[0].glob, spec.images[0].regex",
35+
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",
3636
policy: ClusterImagePolicy{
3737
Spec: ClusterImagePolicySpec{
3838
Images: []ImagePattern{
@@ -45,9 +45,9 @@ func TestImagePatternValidation(t *testing.T) {
4545
},
4646
},
4747
{
48-
name: "Should fail when neither regex nor glob are present: %v",
48+
name: "Should fail when neither regex nor glob are present",
4949
expectErr: true,
50-
errorString: "expected exactly one, got neither: spec.images[0].glob, spec.images[0].regex",
50+
errorString: "expected exactly one, got neither: spec.images[0].glob, spec.images[0].regex\nmissing field(s): spec.authorities",
5151
policy: ClusterImagePolicy{
5252
Spec: ClusterImagePolicySpec{
5353
Images: []ImagePattern{
@@ -57,9 +57,9 @@ func TestImagePatternValidation(t *testing.T) {
5757
},
5858
},
5959
{
60-
name: "Glob should fail with multiple *: %v",
60+
name: "Glob should fail with multiple *",
6161
expectErr: true,
62-
errorString: "glob match supports only a single * as a trailing character",
62+
errorString: "invalid value: **: spec.images[0].glob\nglob match supports only a single * as a trailing character\nmissing field(s): spec.authorities",
6363
policy: ClusterImagePolicy{
6464
Spec: ClusterImagePolicySpec{
6565
Images: []ImagePattern{
@@ -71,9 +71,9 @@ func TestImagePatternValidation(t *testing.T) {
7171
},
7272
},
7373
{
74-
name: "Glob should fail with non-trailing *: %v",
74+
name: "Glob should fail with non-trailing *",
7575
expectErr: true,
76-
errorString: "glob match supports only * as a trailing character",
76+
errorString: "invalid value: foo*bar: spec.images[0].glob\nglob match supports only * as a trailing character\nmissing field(s): spec.authorities",
7777
policy: ClusterImagePolicy{
7878
Spec: ClusterImagePolicySpec{
7979
Images: []ImagePattern{
@@ -84,13 +84,26 @@ func TestImagePatternValidation(t *testing.T) {
8484
},
8585
},
8686
},
87+
{
88+
name: "missing image and authorities in the spec",
89+
expectErr: true,
90+
errorString: "missing field(s): spec.authorities, spec.images",
91+
policy: ClusterImagePolicy{
92+
Spec: ClusterImagePolicySpec{},
93+
},
94+
},
8795
}
8896

8997
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-
}
98+
t.Run(test.name, func(t *testing.T) {
99+
err := test.policy.Validate(context.TODO())
100+
if test.expectErr {
101+
require.NotNil(t, err)
102+
require.EqualError(t, err, test.errorString)
103+
} else {
104+
require.Nil(t, err)
105+
}
106+
})
94107
}
95108
}
96109

@@ -102,9 +115,9 @@ func TestKeyValidation(t *testing.T) {
102115
policy ClusterImagePolicy
103116
}{
104117
{
105-
name: "Should fail when key has multiple properties: %v",
118+
name: "Should fail when key has multiple properties",
106119
expectErr: true,
107-
errorString: "expected exactly one, got both",
120+
errorString: "expected exactly one, got both: spec.authorities[0].key.data, spec.authorities[0].key.kms, spec.authorities[0].key.secretref",
108121
policy: ClusterImagePolicy{
109122
Spec: ClusterImagePolicySpec{
110123
Images: []ImagePattern{
@@ -124,9 +137,9 @@ func TestKeyValidation(t *testing.T) {
124137
},
125138
},
126139
{
127-
name: "Should fail when key has mixed valid and invalid data: %v",
140+
name: "Should fail when key has mixed valid and invalid data",
128141
expectErr: true,
129-
errorString: "invalid value: -----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEaEOVJCFtduYr3xqTxeRWSW32CY/s\nTBNZj4oIUPl8JvhVPJ1TKDPlNcuT4YphSt6t3yOmMvkdQbCj8broX6vijw==\n-----END PUBLIC KEY-----\n---somedata---",
142+
errorString: "invalid value: -----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEaEOVJCFtduYr3xqTxeRWSW32CY/s\nTBNZj4oIUPl8JvhVPJ1TKDPlNcuT4YphSt6t3yOmMvkdQbCj8broX6vijw==\n-----END PUBLIC KEY-----\n---somedata---: spec.authorities[0].key.data",
130143
policy: ClusterImagePolicy{
131144
Spec: ClusterImagePolicySpec{
132145
Images: []ImagePattern{
@@ -145,9 +158,9 @@ func TestKeyValidation(t *testing.T) {
145158
},
146159
},
147160
{
148-
name: "Should fail when key has malformed pubkey data: %v",
161+
name: "Should fail when key has malformed pubkey data",
149162
expectErr: true,
150-
errorString: "invalid value: ---some key data----",
163+
errorString: "invalid value: ---some key data----: spec.authorities[0].key.data",
151164
policy: ClusterImagePolicy{
152165
Spec: ClusterImagePolicySpec{
153166
Images: []ImagePattern{
@@ -166,9 +179,9 @@ func TestKeyValidation(t *testing.T) {
166179
},
167180
},
168181
{
169-
name: "Should fail when key is empty: %v",
182+
name: "Should fail when key is empty",
170183
expectErr: true,
171-
errorString: "expected exactly one, got neither",
184+
errorString: "expected exactly one, got neither: spec.authorities[0].key.data, spec.authorities[0].key.kms, spec.authorities[0].key.secretref",
172185
policy: ClusterImagePolicy{
173186
Spec: ClusterImagePolicySpec{
174187
Images: []ImagePattern{
@@ -185,7 +198,7 @@ func TestKeyValidation(t *testing.T) {
185198
},
186199
},
187200
{
188-
name: "Should fail when regex is given: %v",
201+
name: "Should fail when regex is given",
189202
expectErr: true,
190203
errorString: "must not set the field(s): spec.images[0].regex",
191204
policy: ClusterImagePolicy{
@@ -206,8 +219,8 @@ func TestKeyValidation(t *testing.T) {
206219
},
207220
},
208221
{
209-
name: "Should pass when key has only one property: %v",
210-
errorString: "",
222+
name: "Should pass when key has only one property",
223+
expectErr: false,
211224
policy: ClusterImagePolicy{
212225
Spec: ClusterImagePolicySpec{
213226
Images: []ImagePattern{
@@ -228,13 +241,15 @@ func TestKeyValidation(t *testing.T) {
228241
}
229242

230243
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-
}
244+
t.Run(test.name, func(t *testing.T) {
245+
err := test.policy.Validate(context.TODO())
246+
if test.expectErr {
247+
require.NotNil(t, err)
248+
require.EqualError(t, err, test.errorString)
249+
} else {
250+
require.Nil(t, err)
251+
}
252+
})
238253
}
239254
}
240255

@@ -246,9 +261,9 @@ func TestKeylessValidation(t *testing.T) {
246261
policy ClusterImagePolicy
247262
}{
248263
{
249-
name: "Should fail when keyless is empty: %v",
264+
name: "Should fail when keyless is empty",
250265
expectErr: true,
251-
errorString: "expected exactly one, got neither",
266+
errorString: "expected exactly one, got neither: spec.authorities[0].keyless.ca-cert, spec.authorities[0].keyless.identities, spec.authorities[0].keyless.url",
252267
policy: ClusterImagePolicy{
253268
Spec: ClusterImagePolicySpec{
254269
Images: []ImagePattern{
@@ -265,9 +280,9 @@ func TestKeylessValidation(t *testing.T) {
265280
},
266281
},
267282
{
268-
name: "Should fail when keyless has multiple properties: %v",
283+
name: "Should fail when keyless has multiple properties",
269284
expectErr: true,
270-
errorString: "expected exactly one, got both",
285+
errorString: "expected exactly one, got both: spec.authorities[0].keyless.ca-cert, spec.authorities[0].keyless.identities, spec.authorities[0].keyless.url",
271286
policy: ClusterImagePolicy{
272287
Spec: ClusterImagePolicySpec{
273288
Images: []ImagePattern{
@@ -291,8 +306,8 @@ func TestKeylessValidation(t *testing.T) {
291306
},
292307
},
293308
{
294-
name: "Should pass when a valid keyless ref is specified: %v",
295-
errorString: "",
309+
name: "Should pass when a valid keyless ref is specified",
310+
expectErr: false,
296311
policy: ClusterImagePolicy{
297312
Spec: ClusterImagePolicySpec{
298313
Images: []ImagePattern{
@@ -313,14 +328,17 @@ func TestKeylessValidation(t *testing.T) {
313328
},
314329
},
315330
}
331+
316332
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-
}
333+
t.Run(test.name, func(t *testing.T) {
334+
err := test.policy.Validate(context.TODO())
335+
if test.expectErr {
336+
require.NotNil(t, err)
337+
require.EqualError(t, err, test.errorString)
338+
} else {
339+
require.Nil(t, err)
340+
}
341+
})
324342
}
325343
}
326344

@@ -332,9 +350,9 @@ func TestAuthoritiesValidation(t *testing.T) {
332350
policy ClusterImagePolicy
333351
}{
334352
{
335-
name: "Should fail when keyless is empty: %v",
353+
name: "Should fail when keyless is empty",
336354
expectErr: true,
337-
errorString: "expected exactly one, got both",
355+
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-cert, spec.authorities[0].keyless.identities, spec.authorities[0].keyless.url",
338356
policy: ClusterImagePolicy{
339357
Spec: ClusterImagePolicySpec{
340358
Images: []ImagePattern{
@@ -351,11 +369,10 @@ func TestAuthoritiesValidation(t *testing.T) {
351369
},
352370
},
353371
},
354-
355372
{
356-
name: "Should fail when keyless is empty: %v",
373+
name: "Should fail when keyless is empty",
357374
expectErr: true,
358-
errorString: "At least one authority should be defined",
375+
errorString: "missing field(s): spec.authorities",
359376
policy: ClusterImagePolicy{
360377
Spec: ClusterImagePolicySpec{
361378
Images: []ImagePattern{
@@ -369,13 +386,15 @@ func TestAuthoritiesValidation(t *testing.T) {
369386
},
370387
}
371388
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-
}
389+
t.Run(test.name, func(t *testing.T) {
390+
err := test.policy.Validate(context.TODO())
391+
if test.expectErr {
392+
require.NotNil(t, err)
393+
require.EqualError(t, err, test.errorString)
394+
} else {
395+
require.Nil(t, err)
396+
}
397+
})
379398
}
380399
}
381400

@@ -387,9 +406,9 @@ func TestIdentitiesValidation(t *testing.T) {
387406
policy ClusterImagePolicy
388407
}{
389408
{
390-
name: "Should fail when identities is empty: %v",
409+
name: "Should fail when identities is empty",
391410
expectErr: true,
392-
errorString: "At least one identity must be provided",
411+
errorString: "missing field(s): spec.authorities[0].keyless.identities",
393412
policy: ClusterImagePolicy{
394413
Spec: ClusterImagePolicySpec{
395414
Images: []ImagePattern{
@@ -408,8 +427,8 @@ func TestIdentitiesValidation(t *testing.T) {
408427
},
409428
},
410429
{
411-
name: "Should pass when identities is valid: %v",
412-
errorString: "",
430+
name: "Should pass when identities is valid",
431+
expectErr: false,
413432
policy: ClusterImagePolicy{
414433
Spec: ClusterImagePolicySpec{
415434
Images: []ImagePattern{
@@ -433,12 +452,14 @@ func TestIdentitiesValidation(t *testing.T) {
433452
},
434453
}
435454
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-
}
455+
t.Run(test.name, func(t *testing.T) {
456+
err := test.policy.Validate(context.TODO())
457+
if test.expectErr {
458+
require.NotNil(t, err)
459+
require.EqualError(t, err, test.errorString)
460+
} else {
461+
require.Nil(t, err)
462+
}
463+
})
443464
}
444465
}

0 commit comments

Comments
 (0)