Skip to content

Commit b3da7d4

Browse files
dlipovetskydlipovetsky
committed
fixup! feat: Add preflight checks framework
Remove side effects from the initialization. That is, the checker initialization still decides which checks apply, but we defer side effects, and potential errors, to the checks themselves. This allows us to execute all checks that apply, and get the results to the client. Previously, if initialization failed, the checker returned no checks.
1 parent 306a852 commit b3da7d4

File tree

2 files changed

+115
-74
lines changed

2 files changed

+115
-74
lines changed

‎pkg/webhook/preflight/preflight.go

Lines changed: 44 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ type Checker interface {
3131
//
3232
// Init should not store the context `ctx`, because each check will accept its own context.
3333
// Checks may use both the client and the cluster.
34-
Init(ctx context.Context, client ctrlclient.Client, cluster *clusterv1.Cluster) ([]Check, error)
34+
Init(ctx context.Context, client ctrlclient.Client, cluster *clusterv1.Cluster) []Check
3535
}
3636

3737
type WebhookHandler struct {
@@ -74,43 +74,34 @@ func (h *WebhookHandler) Handle(ctx context.Context, req admission.Request) admi
7474
},
7575
}
7676

77-
// Initialize checkers in parallel.
78-
type ChecksResult struct {
79-
checks []Check
80-
err error
81-
}
82-
checksResultCh := make(chan ChecksResult, len(h.checkers))
83-
77+
// Initialize checkers in parallel and collect all checks.
78+
checkCh := make(chan Check)
8479
wg := &sync.WaitGroup{}
8580
for _, checker := range h.checkers {
8681
wg.Add(1)
87-
result := ChecksResult{}
88-
result.checks, result.err = checker.Init(ctx, h.client, cluster)
89-
checksResultCh <- result
90-
wg.Done()
82+
go func(ctx context.Context, checker Checker) {
83+
checks := checker.Init(ctx, h.client, cluster)
84+
for _, check := range checks {
85+
checkCh <- check
86+
}
87+
wg.Done()
88+
}(ctx, checker)
9189
}
92-
wg.Wait()
93-
close(checksResultCh)
9490

95-
// Collect all checks.
96-
checks := make([]Check, 0)
97-
for checksResult := range checksResultCh {
98-
if checksResult.err != nil {
99-
resp.Allowed = false
100-
resp.Result.Code = http.StatusInternalServerError
101-
resp.Result.Message = "failed to initialize preflight checks"
102-
resp.Result.Details.Causes = append(resp.Result.Details.Causes, metav1.StatusCause{
103-
Type: metav1.CauseTypeInternal,
104-
Message: checksResult.err.Error(),
105-
Field: "", // This concerns the whole cluster.
106-
})
107-
continue
108-
}
109-
checks = append(checks, checksResult.checks...)
91+
// Close the channel when all checkers are done.
92+
go func(wg *sync.WaitGroup, checkCh chan Check) {
93+
wg.Wait()
94+
close(checkCh)
95+
}(wg, checkCh)
96+
97+
// Collect all checks from the channel.
98+
checks := []Check{}
99+
for check := range checkCh {
100+
checks = append(checks, check)
110101
}
111102

112103
// Run all checks in parallel.
113-
resultCh := make(chan CheckResult, len(checks))
104+
resultCh := make(chan CheckResult)
114105
for _, check := range checks {
115106
wg.Add(1)
116107
go func(ctx context.Context, check Check) {
@@ -119,27 +110,29 @@ func (h *WebhookHandler) Handle(ctx context.Context, req admission.Request) admi
119110
wg.Done()
120111
}(ctx, check)
121112
}
122-
wg.Wait()
123-
close(resultCh)
124113

125-
// Collect check results.
114+
// Close the channel when all checks are done.
115+
go func(wg *sync.WaitGroup, resultCh chan CheckResult) {
116+
wg.Wait()
117+
close(resultCh)
118+
}(wg, resultCh)
119+
120+
// Collect all check results from the channel.
121+
internalError := false
126122
for result := range resultCh {
127123
if result.Error {
128124
resp.Allowed = false
129-
resp.Result.Code = http.StatusForbidden
130-
resp.Result.Message = "preflight checks failed"
131125
resp.Result.Details.Causes = append(resp.Result.Details.Causes, metav1.StatusCause{
132126
Type: metav1.CauseTypeInternal,
133127
Field: result.Field,
134128
Message: result.Message,
135129
})
130+
internalError = true
136131
continue
137132
}
138133

139134
if !result.Allowed {
140135
resp.Allowed = false
141-
resp.Result.Code = http.StatusForbidden
142-
resp.Result.Message = "preflight checks failed"
143136
resp.Result.Details.Causes = append(resp.Result.Details.Causes, metav1.StatusCause{
144137
Type: metav1.CauseTypeFieldValueInvalid,
145138
Field: result.Field,
@@ -152,5 +145,19 @@ func (h *WebhookHandler) Handle(ctx context.Context, req admission.Request) admi
152145
}
153146
}
154147

148+
if len(resp.Result.Details.Causes) == 0 {
149+
return resp
150+
}
151+
152+
// Because we have some causes, we construct the response message and code.
153+
resp.Result.Message = "preflight checks failed"
154+
resp.Result.Code = http.StatusForbidden
155+
resp.Result.Reason = metav1.StatusReasonForbidden
156+
if internalError {
157+
// Internal errors take precedence over check failures.
158+
resp.Result.Code = http.StatusInternalServerError
159+
resp.Result.Reason = metav1.StatusReasonInternalError
160+
}
161+
155162
return resp
156163
}

‎pkg/webhook/preflight/preflight_test.go

Lines changed: 71 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,28 @@ import (
2323
)
2424

2525
type mockChecker struct {
26+
name string
2627
checks []Check
27-
err error
2828
}
2929

30-
func (m *mockChecker) Init(_ context.Context, _ ctrlclient.Client, _ *clusterv1.Cluster) ([]Check, error) {
31-
return m.checks, m.err
30+
func (m *mockChecker) Name() string {
31+
return m.name
32+
}
33+
34+
func (m *mockChecker) Init(_ context.Context, _ ctrlclient.Client, _ *clusterv1.Cluster) []Check {
35+
return m.checks
36+
}
37+
38+
type mockDecoder struct {
39+
err error
40+
}
41+
42+
func (m *mockDecoder) Decode(_ admission.Request, _ runtime.Object) error {
43+
return m.err
44+
}
45+
46+
func (m *mockDecoder) DecodeRaw(_ runtime.RawExtension, _ runtime.Object) error {
47+
return m.err
3248
}
3349

3450
func TestHandle(t *testing.T) {
@@ -38,13 +54,16 @@ func TestHandle(t *testing.T) {
3854

3955
tests := []struct {
4056
name string
57+
operation admissionv1.Operation
58+
decoder admission.Decoder
4159
cluster *clusterv1.Cluster
4260
checkers []Checker
4361
checks []Check
4462
expectedResponse admission.Response
4563
}{
4664
{
47-
name: "skip delete operations",
65+
name: "skip delete operations",
66+
operation: admissionv1.Delete,
4867
cluster: &clusterv1.Cluster{
4968
ObjectMeta: metav1.ObjectMeta{
5069
Name: "test-cluster",
@@ -76,7 +95,29 @@ func TestHandle(t *testing.T) {
7695
},
7796
},
7897
},
79-
98+
{
99+
name: "handle decoder error",
100+
decoder: &mockDecoder{
101+
err: fmt.Errorf("decode error"),
102+
},
103+
cluster: &clusterv1.Cluster{
104+
ObjectMeta: metav1.ObjectMeta{
105+
Name: "test-cluster",
106+
Labels: map[string]string{
107+
clusterv1.ProviderNameLabel: "test-provider",
108+
},
109+
},
110+
},
111+
expectedResponse: admission.Response{
112+
AdmissionResponse: admissionv1.AdmissionResponse{
113+
Allowed: false,
114+
Result: &metav1.Status{
115+
Code: http.StatusBadRequest,
116+
Message: "decode error",
117+
},
118+
},
119+
},
120+
},
80121
{
81122
name: "if no checks, then allowed",
82123
cluster: &clusterv1.Cluster{
@@ -214,7 +255,7 @@ func TestHandle(t *testing.T) {
214255
},
215256
},
216257
{
217-
name: "run other checks, despite checker initialization error",
258+
name: "internal error takes precedence in response",
218259
cluster: &clusterv1.Cluster{
219260
ObjectMeta: metav1.ObjectMeta{
220261
Name: "test-cluster",
@@ -231,7 +272,9 @@ func TestHandle(t *testing.T) {
231272
checks: []Check{
232273
func(ctx context.Context) CheckResult {
233274
return CheckResult{
234-
Allowed: true,
275+
Allowed: false,
276+
Error: true,
277+
Message: "internal error",
235278
}
236279
},
237280
},
@@ -250,36 +293,28 @@ func TestHandle(t *testing.T) {
250293
checks: []Check{
251294
func(ctx context.Context) CheckResult {
252295
return CheckResult{
253-
Allowed: false,
254-
Error: true,
255-
Message: "check result error",
296+
Allowed: true,
256297
}
257298
},
258299
},
259300
},
260-
&mockChecker{
261-
err: fmt.Errorf("checker initialization error"),
262-
},
263301
},
264302
expectedResponse: admission.Response{
265303
AdmissionResponse: admissionv1.AdmissionResponse{
266304
Allowed: false,
267305
Result: &metav1.Status{
268-
Code: http.StatusForbidden,
306+
Code: http.StatusInternalServerError,
307+
Reason: metav1.StatusReasonInternalError,
269308
Message: "preflight checks failed",
270309
Details: &metav1.StatusDetails{
271310
Causes: []metav1.StatusCause{
272311
{
273-
Type: metav1.CauseTypeInternal,
274-
Message: "checker initialization error",
312+
Type: metav1.CauseTypeFieldValueInvalid,
313+
Message: "check failed",
275314
},
276315
{
277316
Type: metav1.CauseTypeInternal,
278-
Message: "check result error",
279-
},
280-
{
281-
Type: metav1.CauseTypeFieldValueInvalid,
282-
Message: "check failed",
317+
Message: "internal error",
283318
},
284319
},
285320
},
@@ -291,7 +326,11 @@ func TestHandle(t *testing.T) {
291326

292327
for _, tt := range tests {
293328
t.Run(tt.name, func(t *testing.T) {
329+
// Default the decoder.
294330
decoder := admission.NewDecoder(scheme)
331+
if tt.decoder != nil {
332+
decoder = tt.decoder
333+
}
295334

296335
handler := New(fake.NewClientBuilder().Build(), decoder, tt.checkers...)
297336

@@ -301,9 +340,15 @@ func TestHandle(t *testing.T) {
301340
jsonCluster, err := json.Marshal(tt.cluster)
302341
require.NoError(t, err)
303342

343+
// Default the operation.
344+
operation := admissionv1.Create
345+
if tt.operation != "" {
346+
operation = tt.operation
347+
}
348+
304349
admissionReq := admission.Request{
305350
AdmissionRequest: admissionv1.AdmissionRequest{
306-
Operation: admissionv1.Create,
351+
Operation: operation,
307352
Object: runtime.RawExtension{
308353
Raw: jsonCluster,
309354
},
@@ -321,13 +366,7 @@ func TestHandle(t *testing.T) {
321366

322367
if tt.expectedResponse.Result.Details != nil {
323368
require.NotNil(t, got.Result.Details)
324-
assert.Len(t, got.Result.Details.Causes, len(tt.expectedResponse.Result.Details.Causes))
325-
326-
for i, expectedCause := range tt.expectedResponse.Result.Details.Causes {
327-
assert.Equal(t, expectedCause.Type, got.Result.Details.Causes[i].Type)
328-
assert.Equal(t, expectedCause.Field, got.Result.Details.Causes[i].Field)
329-
assert.Equal(t, expectedCause.Message, got.Result.Details.Causes[i].Message)
330-
}
369+
assert.ElementsMatch(t, tt.expectedResponse.Result.Details.Causes, got.Result.Details.Causes)
331370
}
332371
}
333372
assert.Equal(t, tt.expectedResponse.Warnings, got.Warnings)
@@ -388,7 +427,8 @@ func TestHandleCancelledContext(t *testing.T) {
388427
AdmissionResponse: admissionv1.AdmissionResponse{
389428
Allowed: false,
390429
Result: &metav1.Status{
391-
Code: http.StatusForbidden,
430+
Code: http.StatusInternalServerError,
431+
Reason: metav1.StatusReasonInternalError,
392432
Message: "preflight checks failed",
393433
Details: &metav1.StatusDetails{
394434
Causes: []metav1.StatusCause{
@@ -439,13 +479,7 @@ func TestHandleCancelledContext(t *testing.T) {
439479

440480
if expectedResponse.Result.Details != nil {
441481
require.NotNil(t, got.Result.Details)
442-
assert.Len(t, got.Result.Details.Causes, len(expectedResponse.Result.Details.Causes))
443-
444-
for i, expectedCause := range expectedResponse.Result.Details.Causes {
445-
assert.Equal(t, expectedCause.Type, got.Result.Details.Causes[i].Type)
446-
assert.Equal(t, expectedCause.Field, got.Result.Details.Causes[i].Field)
447-
assert.Equal(t, expectedCause.Message, got.Result.Details.Causes[i].Message)
448-
}
482+
assert.ElementsMatch(t, expectedResponse.Result.Details.Causes, got.Result.Details.Causes)
449483
}
450484
}
451485
assert.Equal(t, expectedResponse.Warnings, got.Warnings)

0 commit comments

Comments
 (0)