Skip to content

Commit 9f572ba

Browse files
azychazych
committed
fix: ensure order in multi-line errors returned by crdupgradesafety validators
1 parent 7061d92 commit 9f572ba

File tree

5 files changed

+206
-9
lines changed

5 files changed

+206
-9
lines changed

‎internal/operator-controller/rukpak/preflights/crdupgradesafety/change_validator.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,9 @@ package crdupgradesafety
88
import (
99
"errors"
1010
"fmt"
11+
"maps"
1112
"reflect"
13+
"slices"
1214

1315
"github.com/openshift/crd-schema-checker/pkg/manifestcomparators"
1416
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
@@ -63,7 +65,9 @@ func (cv *ChangeValidator) Validate(old, new apiextensionsv1.CustomResourceDefin
6365
continue
6466
}
6567

66-
for field, diff := range diffs {
68+
for _, field := range slices.Sorted(maps.Keys(diffs)) {
69+
diff := diffs[field]
70+
6771
handled := false
6872
for _, validation := range cv.Validations {
6973
ok, err := validation(diff)

‎internal/operator-controller/rukpak/preflights/crdupgradesafety/change_validator_test.go‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ package crdupgradesafety_test
77

88
import (
99
"errors"
10+
"fmt"
1011
"testing"
1112

1213
"github.com/stretchr/testify/assert"
@@ -130,12 +131,15 @@ func TestFlattenSchema(t *testing.T) {
130131
}
131132

132133
func TestChangeValidator(t *testing.T) {
134+
validationErr1 := errors.New(`version "v1alpha1", field "^" has unknown change, refusing to determine that change is safe`)
135+
validationErr2 := errors.New(`version "v1alpha1", field "^": fail`)
136+
133137
for _, tc := range []struct {
134138
name string
135139
changeValidator *crdupgradesafety.ChangeValidator
136140
old apiextensionsv1.CustomResourceDefinition
137141
new apiextensionsv1.CustomResourceDefinition
138-
shouldError bool
142+
expectedError error
139143
}{
140144
{
141145
name: "no changes, no error",
@@ -242,7 +246,7 @@ func TestChangeValidator(t *testing.T) {
242246
},
243247
},
244248
},
245-
shouldError: true,
249+
expectedError: validationErr1,
246250
},
247251
{
248252
name: "changes, validation failed, change fully handled, error",
@@ -279,15 +283,18 @@ func TestChangeValidator(t *testing.T) {
279283
},
280284
},
281285
},
282-
shouldError: true,
286+
expectedError: validationErr2,
283287
},
284288
{
285-
name: "changes, validation failed, change not fully handled, error",
289+
name: "changes, validation failed, change not fully handled, ordered error",
286290
changeValidator: &crdupgradesafety.ChangeValidator{
287291
Validations: []crdupgradesafety.ChangeValidation{
288292
func(_ crdupgradesafety.FieldDiff) (bool, error) {
289293
return false, errors.New("fail")
290294
},
295+
func(_ crdupgradesafety.FieldDiff) (bool, error) {
296+
return false, errors.New("error")
297+
},
291298
},
292299
},
293300
old: apiextensionsv1.CustomResourceDefinition{
@@ -316,12 +323,16 @@ func TestChangeValidator(t *testing.T) {
316323
},
317324
},
318325
},
319-
shouldError: true,
326+
expectedError: fmt.Errorf("%w\n%s\n%w", validationErr2, `version "v1alpha1", field "^": error`, validationErr1),
320327
},
321328
} {
322329
t.Run(tc.name, func(t *testing.T) {
323330
err := tc.changeValidator.Validate(tc.old, tc.new)
324-
assert.Equal(t, tc.shouldError, err != nil, "should error? - %v", tc.shouldError)
331+
if tc.expectedError != nil {
332+
assert.EqualError(t, err, tc.expectedError.Error())
333+
} else {
334+
assert.NoError(t, err)
335+
}
325336
})
326337
}
327338
}

‎internal/operator-controller/rukpak/preflights/crdupgradesafety/checks.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"cmp"
66
"errors"
77
"fmt"
8+
"maps"
89
"reflect"
910
"slices"
1011

@@ -45,7 +46,9 @@ func (c *ServedVersionValidator) Validate(old, new apiextensionsv1.CustomResourc
4546
continue
4647
}
4748

48-
for field, diff := range diffs {
49+
for _, field := range slices.Sorted(maps.Keys(diffs)) {
50+
diff := diffs[field]
51+
4952
handled := false
5053
for _, validation := range c.Validations {
5154
ok, err := validation(diff)

‎internal/operator-controller/rukpak/preflights/crdupgradesafety/checks_test.go‎

Lines changed: 180 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"fmt"
66
"testing"
77

8+
"github.com/stretchr/testify/assert"
89
"github.com/stretchr/testify/require"
910
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
1011
"k8s.io/utils/ptr"
@@ -983,3 +984,182 @@ func TestOrderKappsValidateErr(t *testing.T) {
983984
})
984985
}
985986
}
987+
988+
func TestServedVersionValidator(t *testing.T) {
989+
validationErr1 := errors.New(`version "v1alpha1", field "^" has unknown change, refusing to determine that change is safe`)
990+
validationErr2 := errors.New(`version upgrade "v1alpha1" to "v1alpha2", field "^": fail`)
991+
992+
for _, tc := range []struct {
993+
name string
994+
servedVersionValidator *ServedVersionValidator
995+
new apiextensionsv1.CustomResourceDefinition
996+
expectedError error
997+
}{
998+
{
999+
name: "no changes, no error",
1000+
servedVersionValidator: &ServedVersionValidator{
1001+
Validations: []ChangeValidation{
1002+
func(_ FieldDiff) (bool, error) {
1003+
return false, errors.New("should not run")
1004+
},
1005+
},
1006+
},
1007+
new: apiextensionsv1.CustomResourceDefinition{
1008+
Spec: apiextensionsv1.CustomResourceDefinitionSpec{
1009+
Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
1010+
{
1011+
Name: "v1alpha1",
1012+
Served: true,
1013+
Schema: &apiextensionsv1.CustomResourceValidation{
1014+
OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{},
1015+
},
1016+
},
1017+
},
1018+
},
1019+
},
1020+
},
1021+
{
1022+
name: "changes, validation successful, change is fully handled, no error",
1023+
servedVersionValidator: &ServedVersionValidator{
1024+
Validations: []ChangeValidation{
1025+
func(_ FieldDiff) (bool, error) {
1026+
return true, nil
1027+
},
1028+
},
1029+
},
1030+
new: apiextensionsv1.CustomResourceDefinition{
1031+
Spec: apiextensionsv1.CustomResourceDefinitionSpec{
1032+
Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
1033+
{
1034+
Name: "v1alpha1",
1035+
Served: true,
1036+
Schema: &apiextensionsv1.CustomResourceValidation{
1037+
OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{},
1038+
},
1039+
},
1040+
{
1041+
Name: "v1alpha2",
1042+
Served: true,
1043+
Schema: &apiextensionsv1.CustomResourceValidation{
1044+
OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{
1045+
ID: "foo",
1046+
},
1047+
},
1048+
},
1049+
},
1050+
},
1051+
},
1052+
},
1053+
{
1054+
name: "changes, validation successful, change not fully handled, error",
1055+
servedVersionValidator: &ServedVersionValidator{
1056+
Validations: []ChangeValidation{
1057+
func(_ FieldDiff) (bool, error) {
1058+
return false, nil
1059+
},
1060+
},
1061+
},
1062+
new: apiextensionsv1.CustomResourceDefinition{
1063+
Spec: apiextensionsv1.CustomResourceDefinitionSpec{
1064+
Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
1065+
{
1066+
Name: "v1alpha1",
1067+
Served: true,
1068+
Schema: &apiextensionsv1.CustomResourceValidation{
1069+
OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{},
1070+
},
1071+
},
1072+
{
1073+
Name: "v1alpha2",
1074+
Served: true,
1075+
Schema: &apiextensionsv1.CustomResourceValidation{
1076+
OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{
1077+
ID: "foo",
1078+
},
1079+
},
1080+
},
1081+
},
1082+
},
1083+
},
1084+
expectedError: validationErr1,
1085+
},
1086+
{
1087+
name: "changes, validation failed, change fully handled, error",
1088+
servedVersionValidator: &ServedVersionValidator{
1089+
Validations: []ChangeValidation{
1090+
func(_ FieldDiff) (bool, error) {
1091+
return true, errors.New("fail")
1092+
},
1093+
},
1094+
},
1095+
new: apiextensionsv1.CustomResourceDefinition{
1096+
Spec: apiextensionsv1.CustomResourceDefinitionSpec{
1097+
Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
1098+
{
1099+
Name: "v1alpha1",
1100+
Served: true,
1101+
Schema: &apiextensionsv1.CustomResourceValidation{
1102+
OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{},
1103+
},
1104+
},
1105+
{
1106+
Name: "v1alpha2",
1107+
Served: true,
1108+
Schema: &apiextensionsv1.CustomResourceValidation{
1109+
OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{
1110+
ID: "foo",
1111+
},
1112+
},
1113+
},
1114+
},
1115+
},
1116+
},
1117+
expectedError: validationErr2,
1118+
},
1119+
{
1120+
name: "changes, validation failed, change not fully handled, ordered error",
1121+
servedVersionValidator: &ServedVersionValidator{
1122+
Validations: []ChangeValidation{
1123+
func(_ FieldDiff) (bool, error) {
1124+
return false, errors.New("fail")
1125+
},
1126+
func(_ FieldDiff) (bool, error) {
1127+
return false, errors.New("error")
1128+
},
1129+
},
1130+
},
1131+
new: apiextensionsv1.CustomResourceDefinition{
1132+
Spec: apiextensionsv1.CustomResourceDefinitionSpec{
1133+
Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
1134+
{
1135+
Name: "v1alpha1",
1136+
Served: true,
1137+
Schema: &apiextensionsv1.CustomResourceValidation{
1138+
OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{},
1139+
},
1140+
},
1141+
{
1142+
Name: "v1alpha2",
1143+
Served: true,
1144+
Schema: &apiextensionsv1.CustomResourceValidation{
1145+
OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{
1146+
ID: "foo",
1147+
},
1148+
},
1149+
},
1150+
},
1151+
},
1152+
},
1153+
expectedError: fmt.Errorf("%w\n%s\n%w", validationErr2, `version upgrade "v1alpha1" to "v1alpha2", field "^": error`, validationErr1),
1154+
},
1155+
} {
1156+
t.Run(tc.name, func(t *testing.T) {
1157+
err := tc.servedVersionValidator.Validate(apiextensionsv1.CustomResourceDefinition{}, tc.new)
1158+
if tc.expectedError != nil {
1159+
assert.EqualError(t, err, tc.expectedError.Error())
1160+
} else {
1161+
assert.NoError(t, err)
1162+
}
1163+
})
1164+
}
1165+
}

‎internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,6 @@ func (p *Preflight) runPreflight(ctx context.Context, rel *release.Release) erro
113113

114114
err = p.validator.Validate(*oldCrd, *newCrd)
115115
if err != nil {
116-
err = orderKappsValidateErr(err)
117116
validateErrors = append(validateErrors, fmt.Errorf("validating upgrade for CRD %q failed: %w", newCrd.Name, err))
118117
}
119118
}

0 commit comments

Comments
 (0)