Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve cosigned validation error messages #1618

Merged
merged 1 commit into from
Mar 16, 2022
Merged

improve cosigned validation error messages #1618

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation.go
View file
Open in desktop
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"))
hectorj2f marked this conversation as resolved.
Show resolved Hide resolved
}
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
155 changes: 88 additions & 67 deletions pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ package v1alpha1

import (
"context"
"strings"
"testing"

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

Expand All @@ -30,9 +30,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 +45,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 +57,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 +71,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 +84,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 +115,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 +137,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 +158,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 +179,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 +198,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 +219,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 +241,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,9 +261,9 @@ 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",
errorString: "expected exactly one, got neither: spec.authorities[0].keyless.ca-cert, spec.authorities[0].keyless.identities, spec.authorities[0].keyless.url",
policy: ClusterImagePolicy{
Spec: ClusterImagePolicySpec{
Images: []ImagePattern{
Expand All @@ -265,9 +280,9 @@ 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",
errorString: "expected exactly one, got both: spec.authorities[0].keyless.ca-cert, spec.authorities[0].keyless.identities, spec.authorities[0].keyless.url",
policy: ClusterImagePolicy{
Spec: ClusterImagePolicySpec{
Images: []ImagePattern{
Expand All @@ -291,8 +306,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 +328,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.EqualError(t, err, test.errorString)
} else {
require.Nil(t, err)
}
})
}
}

Expand All @@ -332,9 +350,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-cert, spec.authorities[0].keyless.identities, spec.authorities[0].keyless.url",
policy: ClusterImagePolicy{
Spec: ClusterImagePolicySpec{
Images: []ImagePattern{
Expand All @@ -351,11 +369,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 +386,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 +406,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 +427,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 +452,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)
}
})
}
}