fix review findings

Author: sbueringer

internal/runtime/client/client.go

@@ -204,14 +204,13 @@ func (c *client) CallAllExtensions(ctx context.Context, hook runtimecatalog.Hook
 	}
 
 	// Aggregate all responses into a single response.
+	// Note: we only get here if all the extension handlers succeeded.
 	aggregateSuccessfulResponses(response, responses)
 
-	if err != nil {
-		return errors.Wrap(err, "failed to aggregate responses")
-	}
 	return nil
 }
 
+// aggregateSuccessfulResponses aggregates all successful responses into a single response.
 func aggregateSuccessfulResponses(aggregatedResponse runtimehooksv1.ResponseObject, responses []runtimehooksv1.ResponseObject) {
 	// At this point the Status should always be ResponseStatusSuccess and the Message should be empty.
 	// So let's set those values to avoid keeping values that could have been set by the caller of CallAllExtensions.
@@ -223,6 +222,8 @@ func aggregateSuccessfulResponses(aggregatedResponse runtimehooksv1.ResponseObje
 		return
 	}
 
+	// Note: as all responses have the same type we can assume now that
+	// they all implement the RetryResponseObject interface.
 	aggregatedRetryableResponse := aggregatedResponse.(runtimehooksv1.RetryResponseObject)
 
 	for _, resp := range responses {

internal/runtime/client/client_test.go

@@ -391,11 +391,25 @@ func TestClient_CallExtension(t *testing.T) {
 			testServer: testServerConfig{
 				start: false,
 			},
+			args: args{
+				hook:     fakev1alpha1.FakeHook,
+				name:     "valid-extension",
+				request:  &fakev1alpha1.SecondFakeRequest{},
+				response: &fakev1alpha1.SecondFakeResponse{},
+			},
+			wantErr: true,
+		},
+		{
+			name:                       "should fail when hook GVH does not match the registered ExtensionHandler",
+			registeredExtensionConfigs: []runtimev1.ExtensionConfig{validExtensionHandlerWithFailPolicy},
+			testServer: testServerConfig{
+				start: false,
+			},
 			args: args{
 				hook:     fakev1alpha1.SecondFakeHook,
 				name:     "valid-extension",
-				request:  &fakev1alpha1.FakeRequest{},
-				response: &fakev1alpha1.FakeResponse{},
+				request:  &fakev1alpha1.SecondFakeRequest{},
+				response: &fakev1alpha1.SecondFakeResponse{},
 			},
 			wantErr: true,
 		},
@@ -612,7 +626,7 @@ func Test_client_CallAllExtensions(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name:                       "should succeed when no corresponding hook is registered",
+			name:                       "should succeed when no ExtensionHandlers are registered for the hook",
 			registeredExtensionConfigs: []runtimev1.ExtensionConfig{},
 			testServer: testServerConfig{
 				start: false,
@@ -735,31 +749,31 @@ func Test_aggregateResponses(t *testing.T) {
 	}{
 		{
 			name:              "Aggregate response if there is only one response",
-			aggregateResponse: fakeResponse(),
+			aggregateResponse: fakeSuccessResponse(),
 			responses: []runtimehooksv1.ResponseObject{
-				fakeResponse(),
+				fakeSuccessResponse(),
 			},
-			want: fakeResponse(),
+			want: fakeSuccessResponse(),
 		},
 		{
 			name:              "Aggregate retry response if there is only one response",
-			aggregateResponse: fakeRetryableResponse(0),
+			aggregateResponse: fakeRetryableSuccessResponse(0),
 			responses: []runtimehooksv1.ResponseObject{
-				fakeRetryableResponse(5),
+				fakeRetryableSuccessResponse(5),
 			},
-			want: fakeRetryableResponse(5),
+			want: fakeRetryableSuccessResponse(5),
 		},
 		{
 			name:              "Aggregate retry responses to lowest non-zero retryAfterSeconds value",
-			aggregateResponse: fakeRetryableResponse(0),
+			aggregateResponse: fakeRetryableSuccessResponse(0),
 			responses: []runtimehooksv1.ResponseObject{
-				fakeRetryableResponse(0),
-				fakeRetryableResponse(1),
-				fakeRetryableResponse(5),
-				fakeRetryableResponse(4),
-				fakeRetryableResponse(3),
+				fakeRetryableSuccessResponse(0),
+				fakeRetryableSuccessResponse(1),
+				fakeRetryableSuccessResponse(5),
+				fakeRetryableSuccessResponse(4),
+				fakeRetryableSuccessResponse(3),
 			},
-			want: fakeRetryableResponse(1),
+			want: fakeRetryableSuccessResponse(1),
 		},
 	}
 	for _, tt := range tests {
@@ -839,7 +853,7 @@ func registry(configs []runtimev1.ExtensionConfig) runtimeregistry.ExtensionRegi
 	return registry
 }
 
-func fakeResponse() *fakev1alpha1.FakeResponse {
+func fakeSuccessResponse() *fakev1alpha1.FakeResponse {
 	return &fakev1alpha1.FakeResponse{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       "FakeResponse",
@@ -852,7 +866,7 @@ func fakeResponse() *fakev1alpha1.FakeResponse {
 	}
 }
 
-func fakeRetryableResponse(retryAfterSeconds int32) *fakev1alpha1.RetryableFakeResponse {
+func fakeRetryableSuccessResponse(retryAfterSeconds int32) *fakev1alpha1.RetryableFakeResponse {
 	return &fakev1alpha1.RetryableFakeResponse{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       "FakeResponse",