From 04877f45faffcc5f146825fc88c924c1b224b57c Mon Sep 17 00:00:00 2001
From: cpanato <ctadeu@gmail.com>
Date: Wed, 16 Mar 2022 15:24:16 +0100
Subject: [PATCH] improve cosigned validation error messages

Signed-off-by: cpanato <ctadeu@gmail.com>
---
 .../v1alpha1/clusterimagepolicy_validation.go |   7 +-
 .../clusterimagepolicy_validation_test.go     | 155 ++++++++++--------
 2 files changed, 92 insertions(+), 70 deletions(-)

diff --git a/pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation.go b/pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation.go
index 1ae1a235ada..3bf06ee0ba3 100644
--- a/pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation.go
+++ b/pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation.go
@@ -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
 }
 
@@ -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 {
diff --git a/pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation_test.go b/pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation_test.go
index 264f0735481..955da06d32b 100644
--- a/pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation_test.go
+++ b/pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation_test.go
@@ -16,9 +16,9 @@ package v1alpha1
 
 import (
 	"context"
-	"strings"
 	"testing"
 
+	"github.com/stretchr/testify/require"
 	"knative.dev/pkg/apis"
 )
 
@@ -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{
@@ -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{
@@ -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{
@@ -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{
@@ -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)
+			}
+		})
 	}
 }
 
@@ -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{
@@ -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{
@@ -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{
@@ -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{
@@ -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{
@@ -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{
@@ -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)
+			}
+		})
 	}
 }
 
@@ -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{
@@ -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{
@@ -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{
@@ -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)
+			}
+		})
 	}
 }
 
@@ -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{
@@ -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{
@@ -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)
+			}
+		})
 	}
 }
 
@@ -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{
@@ -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{
@@ -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)
+			}
+		})
 	}
 }