Skip to content

🌱 Deduplicate extension filtering and response validation logic #12905

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 30, 2025
+26 −38
Merged

🌱 Deduplicate extension filtering and response validation logic #12905

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1aa1d99
Refactor: Use GetAllExtensions in CallAllExtensions to eliminate code…
furkatgofurov7 Oct 28, 2025
634bd3b
Refactor: Extract response status validation into helper function
furkatgofurov7 Oct 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 26 additions & 38 deletions internal/runtime/client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -229,10 +223,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())
Expand All @@ -242,33 +232,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")
Expand Down Expand Up @@ -411,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 {
Expand Down Expand Up @@ -748,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
}