Skip to content

Commit 305b752

Browse files
furkatgofurov7furkatgofurov7
committed
Add defensive response status checking in runtime client
Check for `response.GetStatus() != Success` instead of `response.GetStatus() == Failure` to handle unknown/invalid status values defensively. This ensures the runtime client properly validates that responses actually have a Success status rather than just checking they're not Failure. Signed-off-by: Furkat Gofurov <[email protected]>
1 parent 6d490b2 commit 305b752

File tree

7 files changed

+110
-22
lines changed

7 files changed

+110
-22
lines changed

‎controlplane/kubeadm/internal/controllers/inplace_canupdatemachine_test.go‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,7 @@ func Test_canExtensionsUpdateMachine(t *testing.T) {
227227
_ = unstructured.SetNestedField(desiredInfraMachine.Object, "in-place updated world", "spec", "hello")
228228

229229
responseWithEmptyPatches := &runtimehooksv1.CanUpdateMachineResponse{
230+
CommonResponse: runtimehooksv1.CommonResponse{Status: runtimehooksv1.ResponseStatusSuccess},
230231
MachinePatch: runtimehooksv1.Patch{
231232
PatchType: runtimehooksv1.JSONPatchType,
232233
Patch: []byte("[]"),
@@ -343,6 +344,7 @@ func Test_canExtensionsUpdateMachine(t *testing.T) {
343344
extensionHandlers: []string{"test-update-extension"},
344345
callExtensionResponses: map[string]runtimehooksv1.ResponseObject{
345346
"test-update-extension": &runtimehooksv1.CanUpdateMachineResponse{
347+
CommonResponse: runtimehooksv1.CommonResponse{Status: runtimehooksv1.ResponseStatusSuccess},
346348
MachinePatch: patchToUpdateMachine,
347349
InfrastructureMachinePatch: patchToUpdateInfraMachine,
348350
BootstrapConfigPatch: patchToUpdateKubeadmConfig,
@@ -362,12 +364,15 @@ func Test_canExtensionsUpdateMachine(t *testing.T) {
362364
extensionHandlers: []string{"test-update-extension-1", "test-update-extension-2", "test-update-extension-3"},
363365
callExtensionResponses: map[string]runtimehooksv1.ResponseObject{
364366
"test-update-extension-1": &runtimehooksv1.CanUpdateMachineResponse{
365-
MachinePatch: patchToUpdateMachine,
367+
CommonResponse: runtimehooksv1.CommonResponse{Status: runtimehooksv1.ResponseStatusSuccess},
368+
MachinePatch: patchToUpdateMachine,
366369
},
367370
"test-update-extension-2": &runtimehooksv1.CanUpdateMachineResponse{
371+
CommonResponse: runtimehooksv1.CommonResponse{Status: runtimehooksv1.ResponseStatusSuccess},
368372
InfrastructureMachinePatch: patchToUpdateInfraMachine,
369373
},
370374
"test-update-extension-3": &runtimehooksv1.CanUpdateMachineResponse{
375+
CommonResponse: runtimehooksv1.CommonResponse{Status: runtimehooksv1.ResponseStatusSuccess},
371376
BootstrapConfigPatch: patchToUpdateKubeadmConfig,
372377
},
373378
},
@@ -403,6 +408,7 @@ func Test_canExtensionsUpdateMachine(t *testing.T) {
403408
extensionHandlers: []string{"test-update-extension"},
404409
callExtensionResponses: map[string]runtimehooksv1.ResponseObject{
405410
"test-update-extension": &runtimehooksv1.CanUpdateMachineResponse{
411+
CommonResponse: runtimehooksv1.CommonResponse{Status: runtimehooksv1.ResponseStatusSuccess},
406412
MachinePatch: patchToUpdateMachine,
407413
InfrastructureMachinePatch: emptyPatch,
408414
BootstrapConfigPatch: emptyPatch,
@@ -954,6 +960,7 @@ func Test_applyPatchesToRequest(t *testing.T) {
954960
_ = unstructured.SetNestedField(patchedInfraMachine.Object, "in-place updated world", "spec", "hello")
955961

956962
responseWithEmptyPatches := &runtimehooksv1.CanUpdateMachineResponse{
963+
CommonResponse: runtimehooksv1.CommonResponse{Status: runtimehooksv1.ResponseStatusSuccess},
957964
MachinePatch: runtimehooksv1.Patch{
958965
PatchType: runtimehooksv1.JSONPatchType,
959966
Patch: []byte("[]"),
@@ -1040,6 +1047,7 @@ func Test_applyPatchesToRequest(t *testing.T) {
10401047
},
10411048
},
10421049
resp: &runtimehooksv1.CanUpdateMachineResponse{
1050+
CommonResponse: runtimehooksv1.CommonResponse{Status: runtimehooksv1.ResponseStatusSuccess},
10431051
MachinePatch: patchToUpdateMachine,
10441052
InfrastructureMachinePatch: patchToUpdateInfraMachine,
10451053
BootstrapConfigPatch: patchToUpdateKubeadmConfig,
@@ -1062,6 +1070,7 @@ func Test_applyPatchesToRequest(t *testing.T) {
10621070
},
10631071
},
10641072
resp: &runtimehooksv1.CanUpdateMachineResponse{
1073+
CommonResponse: runtimehooksv1.CommonResponse{Status: runtimehooksv1.ResponseStatusSuccess},
10651074
MachinePatch: jsonMergePatchToUpdateMachine,
10661075
InfrastructureMachinePatch: jsonMergePatchToUpdateInfraMachine,
10671076
BootstrapConfigPatch: jsonMergePatchToUpdateKubeadmConfig,
@@ -1084,6 +1093,7 @@ func Test_applyPatchesToRequest(t *testing.T) {
10841093
},
10851094
},
10861095
resp: &runtimehooksv1.CanUpdateMachineResponse{
1096+
CommonResponse: runtimehooksv1.CommonResponse{Status: runtimehooksv1.ResponseStatusSuccess},
10871097
MachinePatch: patchToUpdateMachineStatus,
10881098
InfrastructureMachinePatch: patchToUpdateInfraMachineStatus,
10891099
BootstrapConfigPatch: patchToUpdateKubeadmConfigStatus,
@@ -1106,6 +1116,7 @@ func Test_applyPatchesToRequest(t *testing.T) {
11061116
},
11071117
},
11081118
resp: &runtimehooksv1.CanUpdateMachineResponse{
1119+
CommonResponse: runtimehooksv1.CommonResponse{Status: runtimehooksv1.ResponseStatusSuccess},
11091120
MachinePatch: runtimehooksv1.Patch{
11101121
// PatchType is missing
11111122
Patch: []byte(`[{"op":"add","path":"/status","value":{"observedGeneration": 10}}]`),
@@ -1124,6 +1135,7 @@ func Test_applyPatchesToRequest(t *testing.T) {
11241135
},
11251136
},
11261137
resp: &runtimehooksv1.CanUpdateMachineResponse{
1138+
CommonResponse: runtimehooksv1.CommonResponse{Status: runtimehooksv1.ResponseStatusSuccess},
11271139
MachinePatch: runtimehooksv1.Patch{
11281140
PatchType: "UnknownType",
11291141
Patch: []byte(`[{"op":"add","path":"/status","value":{"observedGeneration": 10}}]`),

‎internal/controllers/extensionconfig/extensionconfig_controller_test.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -516,6 +516,9 @@ func discoveryHandler(handlerList ...string) func(http.ResponseWriter, *http.Req
516516
Kind: "DiscoveryResponse",
517517
APIVersion: runtimehooksv1.GroupVersion.String(),
518518
},
519+
CommonResponse: runtimehooksv1.CommonResponse{
520+
Status: runtimehooksv1.ResponseStatusSuccess,
521+
},
519522
Handlers: handlers,
520523
}
521524
respBody, err := json.Marshal(response)

‎internal/controllers/topology/cluster/patches/engine_test.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -416,6 +416,9 @@ func TestApply(t *testing.T) {
416416
},
417417
externalPatchResponses: map[string]runtimehooksv1.ResponseObject{
418418
"patch-infrastructureCluster": &runtimehooksv1.GeneratePatchesResponse{
419+
CommonResponse: runtimehooksv1.CommonResponse{
420+
Status: runtimehooksv1.ResponseStatusSuccess,
421+
},
419422
Items: []runtimehooksv1.GeneratePatchesResponseItem{
420423
{
421424
UID: "1",
@@ -453,6 +456,9 @@ func TestApply(t *testing.T) {
453456
},
454457
externalPatchResponses: map[string]runtimehooksv1.ResponseObject{
455458
"patch-infrastructureCluster": &runtimehooksv1.GeneratePatchesResponse{
459+
CommonResponse: runtimehooksv1.CommonResponse{
460+
Status: runtimehooksv1.ResponseStatusSuccess,
461+
},
456462
Items: []runtimehooksv1.GeneratePatchesResponseItem{
457463
{
458464
UID: "1",
@@ -493,6 +499,9 @@ func TestApply(t *testing.T) {
493499

494500
externalPatchResponses: map[string]runtimehooksv1.ResponseObject{
495501
"patch-infrastructureCluster": &runtimehooksv1.GeneratePatchesResponse{
502+
CommonResponse: runtimehooksv1.CommonResponse{
503+
Status: runtimehooksv1.ResponseStatusSuccess,
504+
},
496505
Items: []runtimehooksv1.GeneratePatchesResponseItem{
497506
{
498507
UID: "1",
@@ -515,6 +524,9 @@ func TestApply(t *testing.T) {
515524
},
516525
},
517526
"patch-controlPlane": &runtimehooksv1.GeneratePatchesResponse{
527+
CommonResponse: runtimehooksv1.CommonResponse{
528+
Status: runtimehooksv1.ResponseStatusSuccess,
529+
},
518530
Items: []runtimehooksv1.GeneratePatchesResponseItem{
519531
{
520532
UID: "2",

‎internal/runtime/client/client.go‎

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -122,11 +122,16 @@ func (c *client) Discover(ctx context.Context, extensionConfig *runtimev1.Extens
122122
return nil, errors.Wrapf(err, "failed to discover extension %q", extensionConfig.Name)
123123
}
124124

125-
// Check to see if the response is a failure and handle the failure accordingly.
126-
if response.GetStatus() == runtimehooksv1.ResponseStatusFailure {
127-
log.Info(fmt.Sprintf("Failed to discover extension %q: got failure response with message %v", extensionConfig.Name, response.GetMessage()))
128-
// Don't add the message to the error as it is may be unique causing too many reconciliations. Ref: https://github.com/kubernetes-sigs/cluster-api/issues/6921
129-
return nil, errors.Errorf("failed to discover extension %q: got failure response, please check controller logs for errors", extensionConfig.Name)
125+
// Check to see if the response is not a success and handle the failure accordingly.
126+
if response.GetStatus() != runtimehooksv1.ResponseStatusSuccess {
127+
if response.GetStatus() == runtimehooksv1.ResponseStatusFailure {
128+
log.Info(fmt.Sprintf("Failed to discover extension %q: got failure response with message %v", extensionConfig.Name, response.GetMessage()))
129+
// Don't add the message to the error as it is may be unique causing too many reconciliations. Ref: https://github.com/kubernetes-sigs/cluster-api/issues/6921
130+
return nil, errors.Errorf("failed to discover extension %q: got failure response, please check controller logs for errors", extensionConfig.Name)
131+
}
132+
// Handle unknown status.
133+
log.Info(fmt.Sprintf("Failed to discover extension %q: got unknown response status %q with message %v", extensionConfig.Name, response.GetStatus(), response.GetMessage()))
134+
return nil, errors.Errorf("failed to discover extension %q: got unknown response status %q, please check controller logs for errors", extensionConfig.Name, response.GetStatus())
130135
}
131136

132137
// Check to see if the response is valid.
@@ -405,11 +410,16 @@ func (c *client) CallExtension(ctx context.Context, hook runtimecatalog.Hook, fo
405410
return errors.Wrapf(err, "failed to call extension handler %q", name)
406411
}
407412

408-
// If the received response is a failure then return an error.
409-
if response.GetStatus() == runtimehooksv1.ResponseStatusFailure {
410-
log.Info(fmt.Sprintf("Failed to call extension handler %q: got failure response with message %v", name, response.GetMessage()))
411-
// Don't add the message to the error as it is may be unique causing too many reconciliations. Ref: https://github.com/kubernetes-sigs/cluster-api/issues/6921
412-
return errors.Errorf("failed to call extension handler %q: got failure response, please check controller logs for errors", name)
413+
// If the received response is not a success then return an error.
414+
if response.GetStatus() != runtimehooksv1.ResponseStatusSuccess {
415+
if response.GetStatus() == runtimehooksv1.ResponseStatusFailure {
416+
log.Info(fmt.Sprintf("Failed to call extension handler %q: got failure response with message %v", name, response.GetMessage()))
417+
// Don't add the message to the error as it is may be unique causing too many reconciliations. Ref: https://github.com/kubernetes-sigs/cluster-api/issues/6921
418+
return errors.Errorf("failed to call extension handler %q: got failure response, please check controller logs for errors", name)
419+
}
420+
// Handle unknown status.
421+
log.Info(fmt.Sprintf("Failed to call extension handler %q: got unknown response status %q with message %v", name, response.GetStatus(), response.GetMessage()))
422+
return errors.Errorf("failed to call extension handler %q: got unknown response status %q, please check controller logs for errors", name, response.GetStatus())
413423
}
414424

415425
if retryResponse, ok := response.(runtimehooksv1.RetryResponseObject); ok && retryResponse.GetRetryAfterSeconds() != 0 {

‎internal/runtime/client/client_test.go‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -761,6 +761,42 @@ func TestClient_CallExtension(t *testing.T) {
761761
wantErr: true,
762762
wantResponseCached: false,
763763
},
764+
{
765+
name: "should fail when calling ExtensionHandler with unknown response status and FailurePolicyFail",
766+
registeredExtensionConfigs: []runtimev1.ExtensionConfig{validExtensionHandlerWithFailPolicy},
767+
testServer: testServerConfig{
768+
start: true,
769+
responses: map[string]testServerResponse{
770+
"/*": response(runtimehooksv1.ResponseStatus("Unknown")),
771+
},
772+
},
773+
args: args{
774+
hook: fakev1alpha1.FakeHook,
775+
name: "valid-extension",
776+
request: &fakev1alpha1.FakeRequest{},
777+
response: &fakev1alpha1.FakeResponse{},
778+
},
779+
wantErr: true,
780+
wantResponseCached: false,
781+
},
782+
{
783+
name: "should fail when calling ExtensionHandler with unknown response status and FailurePolicyIgnore",
784+
registeredExtensionConfigs: []runtimev1.ExtensionConfig{validExtensionHandlerWithIgnorePolicy},
785+
testServer: testServerConfig{
786+
start: true,
787+
responses: map[string]testServerResponse{
788+
"/*": response(runtimehooksv1.ResponseStatus("Unknown")),
789+
},
790+
},
791+
args: args{
792+
hook: fakev1alpha1.FakeHook,
793+
name: "valid-extension",
794+
request: &fakev1alpha1.FakeRequest{},
795+
response: &fakev1alpha1.FakeResponse{},
796+
},
797+
wantErr: true,
798+
wantResponseCached: false,
799+
},
764800
}
765801

766802
for _, tt := range tests {

‎internal/runtime/client/fake/fake_client.go‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -154,8 +154,11 @@ func (fc *RuntimeClient) CallAllExtensions(ctx context.Context, hook runtimecata
154154
panic("cannot update response")
155155
}
156156

157-
if response.GetStatus() == runtimehooksv1.ResponseStatusFailure {
158-
return errors.Errorf("runtime hook %q failed", gvh)
157+
if response.GetStatus() != runtimehooksv1.ResponseStatusSuccess {
158+
if response.GetStatus() == runtimehooksv1.ResponseStatusFailure {
159+
return errors.Errorf("runtime hook %q failed", gvh)
160+
}
161+
return errors.Errorf("runtime hook %q got unknown response status %q", gvh, response.GetStatus())
159162
}
160163
return nil
161164
}
@@ -179,9 +182,12 @@ func (fc *RuntimeClient) CallExtension(ctx context.Context, _ runtimecatalog.Hoo
179182
panic("cannot update response")
180183
}
181184

182-
// If the received response is a failure then return an error.
183-
if response.GetStatus() == runtimehooksv1.ResponseStatusFailure {
184-
return errors.Errorf("ExtensionHandler %s failed with message %s", name, response.GetMessage())
185+
// If the received response is not a success then return an error.
186+
if response.GetStatus() != runtimehooksv1.ResponseStatusSuccess {
187+
if response.GetStatus() == runtimehooksv1.ResponseStatusFailure {
188+
return errors.Errorf("ExtensionHandler %s failed with message %s", name, response.GetMessage())
189+
}
190+
return errors.Errorf("ExtensionHandler %s got unknown response status %q", name, response.GetStatus())
185191
}
186192
return nil
187193
}

‎test/extension/handlers/topologymutation/handler_integration_test.go‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -437,8 +437,11 @@ func (i injectRuntimeClient) CallExtension(ctx context.Context, hook runtimecata
437437
return err
438438
}
439439
i.runtimeExtension.DiscoverVariables(ctx, reqCopy, resp.(*runtimehooksv1.DiscoverVariablesResponse))
440-
if resp.GetStatus() == runtimehooksv1.ResponseStatusFailure {
441-
return errors.Errorf("failed to call extension handler: got failure response: %v", resp.GetMessage())
440+
if resp.GetStatus() != runtimehooksv1.ResponseStatusSuccess {
441+
if resp.GetStatus() == runtimehooksv1.ResponseStatusFailure {
442+
return errors.Errorf("failed to call extension handler: got failure response: %v", resp.GetMessage())
443+
}
444+
return errors.Errorf("failed to call extension handler: got unknown response status %q", resp.GetStatus())
442445
}
443446
return nil
444447
case runtimecatalog.HookName(runtimehooksv1.GeneratePatches):
@@ -447,8 +450,11 @@ func (i injectRuntimeClient) CallExtension(ctx context.Context, hook runtimecata
447450
return err
448451
}
449452
i.runtimeExtension.GeneratePatches(ctx, reqCopy, resp.(*runtimehooksv1.GeneratePatchesResponse))
450-
if resp.GetStatus() == runtimehooksv1.ResponseStatusFailure {
451-
return errors.Errorf("failed to call extension handler: got failure response: %v", resp.GetMessage())
453+
if resp.GetStatus() != runtimehooksv1.ResponseStatusSuccess {
454+
if resp.GetStatus() == runtimehooksv1.ResponseStatusFailure {
455+
return errors.Errorf("failed to call extension handler: got failure response: %v", resp.GetMessage())
456+
}
457+
return errors.Errorf("failed to call extension handler: got unknown response status %q", resp.GetStatus())
452458
}
453459
return nil
454460
case runtimecatalog.HookName(runtimehooksv1.ValidateTopology):
@@ -457,8 +463,11 @@ func (i injectRuntimeClient) CallExtension(ctx context.Context, hook runtimecata
457463
return err
458464
}
459465
i.runtimeExtension.ValidateTopology(ctx, reqCopy, resp.(*runtimehooksv1.ValidateTopologyResponse))
460-
if resp.GetStatus() == runtimehooksv1.ResponseStatusFailure {
461-
return errors.Errorf("failed to call extension handler: got failure response: %v", resp.GetMessage())
466+
if resp.GetStatus() != runtimehooksv1.ResponseStatusSuccess {
467+
if resp.GetStatus() == runtimehooksv1.ResponseStatusFailure {
468+
return errors.Errorf("failed to call extension handler: got failure response: %v", resp.GetMessage())
469+
}
470+
return errors.Errorf("failed to call extension handler: got unknown response status %q", resp.GetStatus())
462471
}
463472
return nil
464473
}

0 commit comments

Comments
 (0)