From 1aa1d99e15ab4e7db4ec21425718670007df4f96 Mon Sep 17 00:00:00 2001 From: Furkat Gofurov Date: Tue, 28 Oct 2025 17:25:04 +0200 Subject: [PATCH 1/2] Refactor: Use GetAllExtensions in CallAllExtensions to eliminate code duplication Previously, CallAllExtensions duplicated the extension filtering logic (namespace matching, registry listing, GVK lookup) that already existed in GetAllExtensions. This refactoring makes CallAllExtensions call GetAllExtensions to get the list of matching handlers. Signed-off-by: Furkat Gofurov --- internal/runtime/client/client.go | 25 +++++-------------------- 1 file changed, 5 insertions(+), 20 deletions(-) diff --git a/internal/runtime/client/client.go b/internal/runtime/client/client.go index 718c187981fa..37fbec5a9c4a 100644 --- a/internal/runtime/client/client.go +++ b/internal/runtime/client/client.go @@ -229,10 +229,6 @@ func (c *client) CallAllExtensions(ctx context.Context, hook runtimecatalog.Hook if err != nil { return errors.Wrapf(err, "failed to call extension handlers for hook %q: failed to compute GroupVersionHook", hookName) } - forObjectGVK, err := apiutil.GVKForObject(forObject, c.client.Scheme()) - if err != nil { - return errors.Wrapf(err, "failed to call extension handlers for hook %q: failed to get GroupVersionKind for the object the hook is executed for", hookName) - } // Make sure the request is compatible with the hook. if err := c.catalog.ValidateRequest(gvh, request); err != nil { return errors.Wrapf(err, "failed to call extension handlers for hook %q: request object is invalid for hook", gvh.GroupHook()) @@ -242,33 +238,22 @@ func (c *client) CallAllExtensions(ctx context.Context, hook runtimecatalog.Hook return errors.Wrapf(err, "failed to call extension handlers for hook %q: response object is invalid for hook", gvh.GroupHook()) } - registrations, err := c.registry.List(gvh.GroupHook()) + // Get all matching extension handlers for this hook and object. + matchingHandlers, err := c.GetAllExtensions(ctx, hook, forObject) if err != nil { return errors.Wrapf(err, "failed to call extension handlers for hook %q", gvh.GroupHook()) } - log.V(4).Info(fmt.Sprintf("Calling all extensions of hook %q for %s %s", hookName, forObjectGVK.Kind, klog.KObj(forObject))) responses := []runtimehooksv1.ResponseObject{} - for _, registration := range registrations { + for _, handlerName := range matchingHandlers { // Creates a new instance of the response parameter. responseObject, err := c.catalog.NewResponse(gvh) if err != nil { - return errors.Wrapf(err, "failed to call extension handlers for hook %q: failed to call extension handler %q", gvh.GroupHook(), registration.Name) + return errors.Wrapf(err, "failed to call extension handlers for hook %q: failed to call extension handler %q", gvh.GroupHook(), handlerName) } tmpResponse := responseObject.(runtimehooksv1.ResponseObject) - // Compute whether the object the call is being made for matches the namespaceSelector - namespaceMatches, err := c.matchNamespace(ctx, registration.NamespaceSelector, forObject.GetNamespace()) - if err != nil { - return errors.Wrapf(err, "failed to call extension handlers for hook %q: failed to call extension handler %q", gvh.GroupHook(), registration.Name) - } - // If the object namespace isn't matched by the registration NamespaceSelector skip the call. - if !namespaceMatches { - log.V(5).Info(fmt.Sprintf("skipping extension handler %q as object '%s/%s' does not match selector %q of ExtensionConfig", registration.Name, forObject.GetNamespace(), forObject.GetName(), registration.NamespaceSelector)) - continue - } - - err = c.CallExtension(ctx, hook, forObject, registration.Name, request, tmpResponse) + err = c.CallExtension(ctx, hook, forObject, handlerName, request, tmpResponse) // If one of the extension handlers fails lets short-circuit here and return early. if err != nil { log.Error(err, "failed to call extension handlers") From 634bd3b2c82bf27abc8ab181ac80c56f16012bac Mon Sep 17 00:00:00 2001 From: Furkat Gofurov Date: Tue, 28 Oct 2025 17:30:49 +0200 Subject: [PATCH 2/2] Refactor: Extract response status validation into helper function Signed-off-by: Furkat Gofurov --- internal/runtime/client/client.go | 39 +++++++++++++++++-------------- 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/internal/runtime/client/client.go b/internal/runtime/client/client.go index 37fbec5a9c4a..61aa3704bbf1 100644 --- a/internal/runtime/client/client.go +++ b/internal/runtime/client/client.go @@ -32,6 +32,7 @@ import ( "strings" "time" + "github.com/go-logr/logr" "github.com/pkg/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" @@ -123,15 +124,8 @@ func (c *client) Discover(ctx context.Context, extensionConfig *runtimev1.Extens } // Check to see if the response is not a success and handle the failure accordingly. - if response.GetStatus() != runtimehooksv1.ResponseStatusSuccess { - if response.GetStatus() == runtimehooksv1.ResponseStatusFailure { - log.Info(fmt.Sprintf("Failed to discover extension %q: got failure response with message %v", extensionConfig.Name, response.GetMessage())) - // 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 - return nil, errors.Errorf("failed to discover extension %q: got failure response, please check controller logs for errors", extensionConfig.Name) - } - // Handle unknown status. - log.Info(fmt.Sprintf("Failed to discover extension %q: got unknown response status %q with message %v", extensionConfig.Name, response.GetStatus(), response.GetMessage())) - return nil, errors.Errorf("failed to discover extension %q: got unknown response status %q, please check controller logs for errors", extensionConfig.Name, response.GetStatus()) + if err := validateResponseStatus(log, response, "discover extension", extensionConfig.Name); err != nil { + return nil, err } // Check to see if the response is valid. @@ -396,15 +390,8 @@ func (c *client) CallExtension(ctx context.Context, hook runtimecatalog.Hook, fo } // If the received response is not a success then return an error. - if response.GetStatus() != runtimehooksv1.ResponseStatusSuccess { - if response.GetStatus() == runtimehooksv1.ResponseStatusFailure { - log.Info(fmt.Sprintf("Failed to call extension handler %q: got failure response with message %v", name, response.GetMessage())) - // 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 - return errors.Errorf("failed to call extension handler %q: got failure response, please check controller logs for errors", name) - } - // Handle unknown status. - log.Info(fmt.Sprintf("Failed to call extension handler %q: got unknown response status %q with message %v", name, response.GetStatus(), response.GetMessage())) - return errors.Errorf("failed to call extension handler %q: got unknown response status %q, please check controller logs for errors", name, response.GetStatus()) + if err := validateResponseStatus(log, response, "call extension handler", name); err != nil { + return err } if retryResponse, ok := response.(runtimehooksv1.RetryResponseObject); ok && retryResponse.GetRetryAfterSeconds() != 0 { @@ -733,3 +720,19 @@ func ExtensionNameFromHandlerName(registeredHandlerName string) (string, error) } return parts[1], nil } + +// validateResponseStatus checks if the response status is successful and returns an error otherwise. +// It logs appropriate messages for failure and unknown statuses. +func validateResponseStatus(log logr.Logger, response runtimehooksv1.ResponseObject, operationName, targetName string) error { + if response.GetStatus() != runtimehooksv1.ResponseStatusSuccess { + if response.GetStatus() == runtimehooksv1.ResponseStatusFailure { + log.Info(fmt.Sprintf("Failed to %s %q: got failure response with message %v", operationName, targetName, response.GetMessage())) + // 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 + return errors.Errorf("failed to %s %q: got failure response, please check controller logs for errors", operationName, targetName) + } + // Handle unknown status. + log.Info(fmt.Sprintf("Failed to %s %q: got unknown response status %q with message %v", operationName, targetName, response.GetStatus(), response.GetMessage())) + return errors.Errorf("failed to %s %q: got unknown response status %q, please check controller logs for errors", operationName, targetName, response.GetStatus()) + } + return nil +}