Skip to content

Commit

Permalink
improve cosigned validation error messages
Browse files Browse the repository at this point in the history 
Signed-off-by: cpanato <[email protected]>
  • Loading branch information
@cpanato
cpanato committed Mar 16, 2022
1 parent 36d7646 commit d1dd6f4
Show file tree
Hide file tree
Showing 2 changed files with 90 additions and 67 deletions.
7 changes: 4 additions & 3 deletions pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,18 @@ func (policy *ClusterImagePolicy) Validate(ctx context.Context) *apis.FieldError

func (spec *ClusterImagePolicySpec) Validate(ctx context.Context) (errors *apis.FieldError) {
if len(spec.Images) == 0 {
errors = errors.Also(apis.ErrGeneric("At least one image should be defined").ViaField("images"))
errors = errors.Also(apis.ErrMissingField("images"))
}
for i, image := range spec.Images {
errors = errors.Also(image.Validate(ctx).ViaFieldIndex("images", i))
}
if len(spec.Authorities) == 0 {
errors = errors.Also(apis.ErrGeneric("At least one authority should be defined").ViaField("authorities"))
errors = errors.Also(apis.ErrMissingField("authorities"))
}
for i, authority := range spec.Authorities {
errors = errors.Also(authority.Validate(ctx).ViaFieldIndex("authorities", i))
}

return
}

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

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

for i, identity := range keyless.Identities {
Expand Down
150 changes: 86 additions & 64 deletions pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"strings"
"testing"

"github.com/stretchr/testify/require"
"knative.dev/pkg/apis"
)

Expand All @@ -30,9 +31,9 @@ func TestImagePatternValidation(t *testing.T) {
policy ClusterImagePolicy
}{
{
name: "Should fail when both regex and glob are present: %v",
name: "Should fail when both regex and glob are present",
expectErr: true,
errorString: "expected exactly one, got both: spec.images[0].glob, spec.images[0].regex",
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",
policy: ClusterImagePolicy{
Spec: ClusterImagePolicySpec{
Images: []ImagePattern{
Expand All @@ -45,9 +46,9 @@ func TestImagePatternValidation(t *testing.T) {
},
},
{
name: "Should fail when neither regex nor glob are present: %v",
name: "Should fail when neither regex nor glob are present",
expectErr: true,
errorString: "expected exactly one, got neither: spec.images[0].glob, spec.images[0].regex",
errorString: "expected exactly one, got neither: spec.images[0].glob, spec.images[0].regex\nmissing field(s): spec.authorities",
policy: ClusterImagePolicy{
Spec: ClusterImagePolicySpec{
Images: []ImagePattern{
Expand All @@ -57,9 +58,9 @@ func TestImagePatternValidation(t *testing.T) {
},
},
{
name: "Glob should fail with multiple *: %v",
name: "Glob should fail with multiple *",
expectErr: true,
errorString: "glob match supports only a single * as a trailing character",
errorString: "invalid value: **: spec.images[0].glob\nglob match supports only a single * as a trailing character\nmissing field(s): spec.authorities",
policy: ClusterImagePolicy{
Spec: ClusterImagePolicySpec{
Images: []ImagePattern{
Expand All @@ -71,9 +72,9 @@ func TestImagePatternValidation(t *testing.T) {
},
},
{
name: "Glob should fail with non-trailing *: %v",
name: "Glob should fail with non-trailing *",
expectErr: true,
errorString: "glob match supports only * as a trailing character",
errorString: "invalid value: foo*bar: spec.images[0].glob\nglob match supports only * as a trailing character\nmissing field(s): spec.authorities",
policy: ClusterImagePolicy{
Spec: ClusterImagePolicySpec{
Images: []ImagePattern{
Expand All @@ -84,13 +85,26 @@ func TestImagePatternValidation(t *testing.T) {
},
},
},
{
name: "missing image and authorities in the spec",
expectErr: true,
errorString: "missing field(s): spec.authorities, spec.images",
policy: ClusterImagePolicy{
Spec: ClusterImagePolicySpec{},
},
},
}

for _, test := range tests {
err := test.policy.Validate(context.TODO())
if test.expectErr && !strings.Contains(err.Error(), test.errorString) {
t.Errorf(test.name, err)
}
t.Run(test.name, func(t *testing.T) {
err := test.policy.Validate(context.TODO())
if test.expectErr {
require.NotNil(t, err)
require.EqualError(t, err, test.errorString)
} else {
require.Nil(t, err)
}
})
}
}

Expand All @@ -102,9 +116,9 @@ func TestKeyValidation(t *testing.T) {
policy ClusterImagePolicy
}{
{
name: "Should fail when key has multiple properties: %v",
name: "Should fail when key has multiple properties",
expectErr: true,
errorString: "expected exactly one, got both",
errorString: "expected exactly one, got both: spec.authorities[0].key.data, spec.authorities[0].key.kms, spec.authorities[0].key.secretref",
policy: ClusterImagePolicy{
Spec: ClusterImagePolicySpec{
Images: []ImagePattern{
Expand All @@ -124,9 +138,9 @@ func TestKeyValidation(t *testing.T) {
},
},
{
name: "Should fail when key has mixed valid and invalid data: %v",
name: "Should fail when key has mixed valid and invalid data",
expectErr: true,
errorString: "invalid value: -----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEaEOVJCFtduYr3xqTxeRWSW32CY/s\nTBNZj4oIUPl8JvhVPJ1TKDPlNcuT4YphSt6t3yOmMvkdQbCj8broX6vijw==\n-----END PUBLIC KEY-----\n---somedata---",
errorString: "invalid value: -----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEaEOVJCFtduYr3xqTxeRWSW32CY/s\nTBNZj4oIUPl8JvhVPJ1TKDPlNcuT4YphSt6t3yOmMvkdQbCj8broX6vijw==\n-----END PUBLIC KEY-----\n---somedata---: spec.authorities[0].key.data",
policy: ClusterImagePolicy{
Spec: ClusterImagePolicySpec{
Images: []ImagePattern{
Expand All @@ -145,9 +159,9 @@ func TestKeyValidation(t *testing.T) {
},
},
{
name: "Should fail when key has malformed pubkey data: %v",
name: "Should fail when key has malformed pubkey data",
expectErr: true,
errorString: "invalid value: ---some key data----",
errorString: "invalid value: ---some key data----: spec.authorities[0].key.data",
policy: ClusterImagePolicy{
Spec: ClusterImagePolicySpec{
Images: []ImagePattern{
Expand All @@ -166,9 +180,9 @@ func TestKeyValidation(t *testing.T) {
},
},
{
name: "Should fail when key is empty: %v",
name: "Should fail when key is empty",
expectErr: true,
errorString: "expected exactly one, got neither",
errorString: "expected exactly one, got neither: spec.authorities[0].key.data, spec.authorities[0].key.kms, spec.authorities[0].key.secretref",
policy: ClusterImagePolicy{
Spec: ClusterImagePolicySpec{
Images: []ImagePattern{
Expand All @@ -185,7 +199,7 @@ func TestKeyValidation(t *testing.T) {
},
},
{
name: "Should fail when regex is given: %v",
name: "Should fail when regex is given",
expectErr: true,
errorString: "must not set the field(s): spec.images[0].regex",
policy: ClusterImagePolicy{
Expand All @@ -206,8 +220,8 @@ func TestKeyValidation(t *testing.T) {
},
},
{
name: "Should pass when key has only one property: %v",
errorString: "",
name: "Should pass when key has only one property",
expectErr: false,
policy: ClusterImagePolicy{
Spec: ClusterImagePolicySpec{
Images: []ImagePattern{
Expand All @@ -228,13 +242,15 @@ func TestKeyValidation(t *testing.T) {
}

for _, test := range tests {
err := test.policy.Validate(context.TODO())
if test.expectErr && !strings.Contains(err.Error(), test.errorString) {
t.Errorf(test.name, err)
}
if !test.expectErr && err != nil {
t.Errorf(test.name, err)
}
t.Run(test.name, func(t *testing.T) {
err := test.policy.Validate(context.TODO())
if test.expectErr {
require.NotNil(t, err)
require.EqualError(t, err, test.errorString)
} else {
require.Nil(t, err)
}
})
}
}

Expand All @@ -246,7 +262,7 @@ func TestKeylessValidation(t *testing.T) {
policy ClusterImagePolicy
}{
{
name: "Should fail when keyless is empty: %v",
name: "Should fail when keyless is empty",
expectErr: true,
errorString: "expected exactly one, got neither",
policy: ClusterImagePolicy{
Expand All @@ -265,7 +281,7 @@ func TestKeylessValidation(t *testing.T) {
},
},
{
name: "Should fail when keyless has multiple properties: %v",
name: "Should fail when keyless has multiple properties",
expectErr: true,
errorString: "expected exactly one, got both",
policy: ClusterImagePolicy{
Expand All @@ -291,8 +307,8 @@ func TestKeylessValidation(t *testing.T) {
},
},
{
name: "Should pass when a valid keyless ref is specified: %v",
errorString: "",
name: "Should pass when a valid keyless ref is specified",
expectErr: false,
policy: ClusterImagePolicy{
Spec: ClusterImagePolicySpec{
Images: []ImagePattern{
Expand All @@ -313,14 +329,17 @@ func TestKeylessValidation(t *testing.T) {
},
},
}

for _, test := range tests {
err := test.policy.Validate(context.TODO())
if test.expectErr && !strings.Contains(err.Error(), test.errorString) {
t.Errorf(test.name, err)
}
if !test.expectErr && err != nil {
t.Errorf(test.name, err)
}
t.Run(test.name, func(t *testing.T) {
err := test.policy.Validate(context.TODO())
if test.expectErr {
require.NotNil(t, err)
require.True(t, strings.Contains(err.Error(), test.errorString))
} else {
require.Nil(t, err)
}
})
}
}

Expand All @@ -332,9 +351,9 @@ func TestAuthoritiesValidation(t *testing.T) {
policy ClusterImagePolicy
}{
{
name: "Should fail when keyless is empty: %v",
name: "Should fail when keyless is empty",
expectErr: true,
errorString: "expected exactly one, got both",
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",
policy: ClusterImagePolicy{
Spec: ClusterImagePolicySpec{
Images: []ImagePattern{
Expand All @@ -351,11 +370,10 @@ func TestAuthoritiesValidation(t *testing.T) {
},
},
},

{
name: "Should fail when keyless is empty: %v",
name: "Should fail when keyless is empty",
expectErr: true,
errorString: "At least one authority should be defined",
errorString: "missing field(s): spec.authorities",
policy: ClusterImagePolicy{
Spec: ClusterImagePolicySpec{
Images: []ImagePattern{
Expand All @@ -369,13 +387,15 @@ func TestAuthoritiesValidation(t *testing.T) {
},
}
for _, test := range tests {
err := test.policy.Validate(context.TODO())
if test.expectErr && !strings.Contains(err.Error(), test.errorString) {
t.Errorf(test.name, err)
}
if !test.expectErr && err != nil {
t.Errorf(test.name, err)
}
t.Run(test.name, func(t *testing.T) {
err := test.policy.Validate(context.TODO())
if test.expectErr {
require.NotNil(t, err)
require.EqualError(t, err, test.errorString)
} else {
require.Nil(t, err)
}
})
}
}

Expand All @@ -387,9 +407,9 @@ func TestIdentitiesValidation(t *testing.T) {
policy ClusterImagePolicy
}{
{
name: "Should fail when identities is empty: %v",
name: "Should fail when identities is empty",
expectErr: true,
errorString: "At least one identity must be provided",
errorString: "missing field(s): spec.authorities[0].keyless.identities",
policy: ClusterImagePolicy{
Spec: ClusterImagePolicySpec{
Images: []ImagePattern{
Expand All @@ -408,8 +428,8 @@ func TestIdentitiesValidation(t *testing.T) {
},
},
{
name: "Should pass when identities is valid: %v",
errorString: "",
name: "Should pass when identities is valid",
expectErr: false,
policy: ClusterImagePolicy{
Spec: ClusterImagePolicySpec{
Images: []ImagePattern{
Expand All @@ -433,12 +453,14 @@ func TestIdentitiesValidation(t *testing.T) {
},
}
for _, test := range tests {
err := test.policy.Validate(context.TODO())
if test.expectErr && !strings.Contains(err.Error(), test.errorString) {
t.Errorf(test.name, err)
}
if !test.expectErr && err != nil {
t.Errorf(test.name, err)
}
t.Run(test.name, func(t *testing.T) {
err := test.policy.Validate(context.TODO())
if test.expectErr {
require.NotNil(t, err)
require.EqualError(t, err, test.errorString)
} else {
require.Nil(t, err)
}
})
}
}

0 comments on commit d1dd6f4

Please sign in to comment.