Skip to content

Commit be13ee5

Browse files
helsaawyhelsaawy
authored
Use multi-error for annotation processing (#2419)
* Use multi-error for annotation processing Return all errors from `"internal/oci".ProcessAnnotations`. Update associated tests to check for expected errors. Signed-off-by: Hamza El-Saawy <[email protected]> * PR: always check `errors.Is`; fix `subtest` bug Signed-off-by: Hamza El-Saawy <[email protected]> --------- Signed-off-by: Hamza El-Saawy <[email protected]>
1 parent 7084bd2 commit be13ee5

File tree

2 files changed

+62
-42
lines changed

2 files changed

+62
-42
lines changed

‎internal/oci/annotations.go‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,25 +24,25 @@ var ErrAnnotationExpansionConflict = errors.New("annotation expansion conflict")
2424
var ErrGenericAnnotationConflict = errors.New("specified annotations conflict")
2525

2626
// ProcessAnnotations expands annotations into their corresponding annotation groups.
27-
func ProcessAnnotations(ctx context.Context, s *specs.Spec) (err error) {
27+
func ProcessAnnotations(ctx context.Context, s *specs.Spec) error {
2828
// Named `Process` and not `Expand` since this function may be expanded (pun intended) to
2929
// deal with other annotation issues and validation.
3030

3131
// Rather than give up part of the way through on error, this just emits a warning (similar
3232
// to the `parseAnnotation*` functions) and continues through, so the spec is not left in a
3333
// (partially) unusable form.
34-
// If multiple different errors are to be raised, they should be combined or, if they
35-
// are logged, only the last kept, depending on their severity.
3634

3735
// expand annotations
36+
var errs []error
3837
for key, exps := range annotations.AnnotationExpansions {
3938
// check if annotation is present
4039
if val, ok := s.Annotations[key]; ok {
4140
// ideally, some normalization would occur here (ie, "True" -> "true")
4241
// but strings may be case-sensitive
4342
for _, k := range exps {
4443
if v, ok := s.Annotations[k]; ok && val != v {
45-
err = ErrAnnotationExpansionConflict
44+
err := fmt.Errorf("%w: %q = %q and %q = %q", ErrAnnotationExpansionConflict, key, val, k, v)
45+
errs = append(errs, err)
4646
log.G(ctx).WithFields(logrus.Fields{
4747
logfields.OCIAnnotation: key,
4848
logfields.Value: val,
@@ -60,7 +60,12 @@ func ProcessAnnotations(ctx context.Context, s *specs.Spec) (err error) {
6060
disableHPC := ParseAnnotationsBool(ctx, s.Annotations, annotations.DisableHostProcessContainer, false)
6161
enableHPC := ParseAnnotationsBool(ctx, s.Annotations, annotations.HostProcessContainer, false)
6262
if disableHPC && enableHPC {
63-
err = ErrGenericAnnotationConflict
63+
err := fmt.Errorf("%w: host process container annotations %q = %q and %q = %q",
64+
ErrGenericAnnotationConflict,
65+
annotations.DisableHostProcessContainer, s.Annotations[annotations.DisableHostProcessContainer],
66+
annotations.HostProcessContainer, s.Annotations[annotations.HostProcessContainer])
67+
errs = append(errs, err)
68+
6469
log.G(ctx).WithFields(logrus.Fields{
6570
logfields.OCIAnnotation: annotations.DisableHostProcessContainer,
6671
logfields.Value: s.Annotations[annotations.DisableHostProcessContainer],
@@ -69,7 +74,7 @@ func ProcessAnnotations(ctx context.Context, s *specs.Spec) (err error) {
6974
}).WithError(err).Warning("Host process container and disable host process container cannot both be true")
7075
}
7176

72-
return err
77+
return errors.Join(errs...)
7378
}
7479

7580
// handle specific annotations

‎internal/oci/annotations_test.go‎

Lines changed: 51 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -28,97 +28,112 @@ func TestProccessAnnotations_HostProcessContainer(t *testing.T) {
2828
ctx := context.Background()
2929

3030
testAnnotations := []struct {
31-
name string
32-
an map[string]string
33-
expectedSuccess bool
31+
name string
32+
an map[string]string
33+
errs []error
3434
}{
35+
//
36+
// valid cases
37+
//
38+
3539
{
3640
name: "DisableUnsafeOperations-DisableHostProcessContainer",
3741
an: map[string]string{
3842
annotations.DisableUnsafeOperations: "true",
3943
annotations.DisableHostProcessContainer: "true",
4044
annotations.HostProcessContainer: "false",
4145
},
42-
expectedSuccess: true,
4346
},
4447
{
45-
name: "DisableUnsafeOperations-DisableHostProcessContainer-HostProcessContainer",
48+
name: "HostProcessContainer",
4649
an: map[string]string{
47-
annotations.DisableUnsafeOperations: "true",
48-
annotations.DisableHostProcessContainer: "true",
50+
annotations.DisableUnsafeOperations: "false",
51+
annotations.DisableHostProcessContainer: "false",
4952
annotations.HostProcessContainer: "true",
5053
},
51-
expectedSuccess: false,
5254
},
5355
{
54-
name: "DisableUnsafeOperations-HostProcessContainer",
56+
name: "All false",
5557
an: map[string]string{
56-
annotations.DisableUnsafeOperations: "true",
58+
annotations.DisableUnsafeOperations: "false",
5759
annotations.DisableHostProcessContainer: "false",
58-
annotations.HostProcessContainer: "true",
60+
annotations.HostProcessContainer: "false",
5961
},
60-
expectedSuccess: false,
6162
},
63+
64+
//
65+
// invalid
66+
//
67+
6268
{
63-
name: "DisableUnsafeOperations",
69+
name: "DisableUnsafeOperations-DisableHostProcessContainer-HostProcessContainer",
6470
an: map[string]string{
6571
annotations.DisableUnsafeOperations: "true",
66-
annotations.DisableHostProcessContainer: "false",
67-
annotations.HostProcessContainer: "false",
72+
annotations.DisableHostProcessContainer: "true",
73+
annotations.HostProcessContainer: "true",
6874
},
69-
expectedSuccess: false,
75+
errs: []error{ErrGenericAnnotationConflict},
7076
},
7177
{
72-
name: "HostProcessContainer",
78+
name: "DisableUnsafeOperations",
7379
an: map[string]string{
74-
annotations.DisableUnsafeOperations: "false",
80+
annotations.DisableUnsafeOperations: "true",
7581
annotations.DisableHostProcessContainer: "false",
76-
annotations.HostProcessContainer: "true",
82+
annotations.HostProcessContainer: "false",
7783
},
78-
expectedSuccess: true,
84+
errs: []error{ErrAnnotationExpansionConflict},
7985
},
8086
{
81-
name: "DisableHostProcessContainer-HostProcessContainer",
87+
name: "DisableHostProcessContainer",
8288
an: map[string]string{
8389
annotations.DisableUnsafeOperations: "false",
8490
annotations.DisableHostProcessContainer: "true",
85-
annotations.HostProcessContainer: "true",
91+
annotations.HostProcessContainer: "false",
8692
},
87-
expectedSuccess: false,
93+
errs: []error{ErrAnnotationExpansionConflict},
8894
},
95+
96+
// expansion both conflicts and causes conflict
8997
{
90-
name: "DisableHostProcessContainer",
98+
name: "DisableUnsafeOperations-HostProcessContainer",
9199
an: map[string]string{
92-
annotations.DisableUnsafeOperations: "false",
93-
annotations.DisableHostProcessContainer: "true",
94-
annotations.HostProcessContainer: "false",
100+
annotations.DisableUnsafeOperations: "true",
101+
annotations.DisableHostProcessContainer: "false",
102+
annotations.HostProcessContainer: "true",
95103
},
96-
expectedSuccess: false,
104+
errs: []error{ErrAnnotationExpansionConflict},
97105
},
98106
{
99-
name: "All false",
107+
name: "DisableHostProcessContainer-HostProcessContainer",
100108
an: map[string]string{
101109
annotations.DisableUnsafeOperations: "false",
102-
annotations.DisableHostProcessContainer: "false",
103-
annotations.HostProcessContainer: "false",
110+
annotations.DisableHostProcessContainer: "true",
111+
annotations.HostProcessContainer: "true",
104112
},
105-
expectedSuccess: true,
113+
errs: []error{ErrAnnotationExpansionConflict, ErrGenericAnnotationConflict},
106114
},
107115
}
108116

109117
for _, tt := range testAnnotations {
110-
t.Run(tt.name, func(subtest *testing.T) {
118+
t.Run(tt.name, func(t *testing.T) {
111119
spec := specs.Spec{
112120
Windows: &specs.Windows{},
113121
Annotations: tt.an,
114122
}
115123

116124
err := ProcessAnnotations(ctx, &spec)
117-
if err != nil && tt.expectedSuccess {
125+
if err != nil && len(tt.errs) == 0 {
118126
t.Fatalf("ProcessAnnotations should have succeeded, instead got %v", err)
119127
}
120-
if err == nil && !tt.expectedSuccess {
121-
t.Fatal("ProcessAnnotations should have failed due to conflicting annotations, instead returned success")
128+
if len(tt.errs) > 0 {
129+
if err == nil {
130+
t.Fatalf("ProcessAnnotations succeeded; should have failed with %v", tt.errs)
131+
}
132+
for _, e := range tt.errs {
133+
if !errors.Is(err, e) {
134+
t.Fatalf("ProcessAnnotations should failed with %v", e)
135+
}
136+
}
122137
}
123138
})
124139
}

0 commit comments

Comments
 (0)