diff --git a/pkg/registry/apis/provisioning/controller/connection.go b/pkg/registry/apis/provisioning/controller/connection.go index 87df9e67e41..351236f44bd 100644 --- a/pkg/registry/apis/provisioning/controller/connection.go +++ b/pkg/registry/apis/provisioning/controller/connection.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "net" "strings" "time" @@ -156,13 +157,14 @@ func (cc *ConnectionController) processNextWorkItem(ctx context.Context) bool { return true } - if !apierrors.IsServiceUnavailable(err) { - logger.Info("ConnectionController will not retry") + // Check if error is transient and should be retried + if !isTransientError(err) { + logger.Info("ConnectionController will not retry (non-transient error)") cc.queue.Forget(item) return true } - logger.Info("ConnectionController will retry as service is unavailable") + logger.Info("ConnectionController will retry (transient error)") utilruntime.HandleError(fmt.Errorf("%v failed with: %v", item, err)) cc.queue.AddRateLimited(item) @@ -313,12 +315,77 @@ func (cc *ConnectionController) handleDelete(ctx context.Context, conn *provisio FieldManager: "provisioning-connection-controller", }) if err != nil { - return fmt.Errorf("remove finalizer: %w", err) + // If we can't remove the finalizer, undelete the connection so it can be retried later + // This prevents the connection from being stuck in deletion state + logger.Error("failed to remove finalizer, undeleting connection", "error", err) + undeleteErr := cc.undeleteConnection(ctx, conn, err) + if undeleteErr != nil { + return fmt.Errorf("remove finalizer: %w; failed to undelete: %w", err, undeleteErr) + } + return fmt.Errorf("remove finalizer: %w (connection has been undeleted, deletion can be retried)", err) } return nil } +// undeleteConnection removes the DeletionTimestamp to "undelete" the connection +// This is used when finalizer removal fails, allowing the deletion to be retried later +func (cc *ConnectionController) undeleteConnection(ctx context.Context, conn *provisioning.Connection, originalErr error) error { + logger := logging.FromContext(ctx) + logger.Info("undeleting connection due to finalizer removal failure", "error", originalErr.Error()) + + // Remove DeletionTimestamp by patching it to null + _, err := cc.client.Connections(conn.GetNamespace()). + Patch(ctx, conn.Name, types.JSONPatchType, []byte(`[ + { "op": "remove", "path": "/metadata/deletionTimestamp" } + ]`), metav1.PatchOptions{ + FieldManager: "provisioning-connection-controller", + }) + if err != nil { + logger.Error("failed to undelete connection", "error", err) + return fmt.Errorf("undelete connection: %w", err) + } + + logger.Info("connection undeleted successfully, deletion can be retried") + return nil +} + +// isTransientError determines if an error is transient and should be retried +func isTransientError(err error) bool { + if err == nil { + return false + } + + // Check for Kubernetes API transient errors + if apierrors.IsServiceUnavailable(err) { + return true + } + if apierrors.IsServerTimeout(err) { + return true + } + if apierrors.IsTooManyRequests(err) { + return true + } + if apierrors.IsInternalError(err) { + return true + } + if apierrors.IsTimeout(err) { + return true + } + + // Check for network errors + var netErr net.Error + if errors.As(err, &netErr) { + if netErr.Timeout() { + return true + } + } + + // Check for connection errors + var opErr *net.OpError + return errors.As(err, &opErr) +} + // shouldCheckHealth determines if a connection health check should be performed. func (cc *ConnectionController) shouldCheckHealth(conn *provisioning.Connection) bool { // If the connection has been updated, always check health diff --git a/pkg/registry/apis/provisioning/controller/connection_test.go b/pkg/registry/apis/provisioning/controller/connection_test.go index dff9c9abb2f..f9bf4d8e3ae 100644 --- a/pkg/registry/apis/provisioning/controller/connection_test.go +++ b/pkg/registry/apis/provisioning/controller/connection_test.go @@ -3,15 +3,18 @@ package controller import ( "context" "errors" + "strings" "testing" "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/internalversion" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/watch" "k8s.io/client-go/rest" @@ -527,7 +530,7 @@ func TestConnectionController_handleDelete(t *testing.T) { expectFinalizerRemoved: false, }, { - name: "error removing finalizer, should return error", + name: "error removing finalizer, should undelete connection", connection: &provisioning.Connection{ ObjectMeta: metav1.ObjectMeta{ Name: "test-conn", @@ -542,11 +545,24 @@ func TestConnectionController_handleDelete(t *testing.T) { }, nil) }, connectionSetup: func(m *mockConnectionInterface) { - m.On("Patch", ctx, "test-conn", types.JSONPatchType, mock.Anything, metav1.PatchOptions{ + // First patch fails (remove finalizer) + m.On("Patch", ctx, "test-conn", types.JSONPatchType, mock.MatchedBy(func(data []byte) bool { + return string(data) == `[ + { "op": "remove", "path": "/metadata/finalizers" } + ]` + }), metav1.PatchOptions{ FieldManager: "provisioning-connection-controller", - }, mock.Anything).Return(nil, errors.New("patch error")) + }, mock.Anything).Return(nil, errors.New("patch error")).Once() + // Second patch succeeds (undelete - remove DeletionTimestamp) + m.On("Patch", ctx, "test-conn", types.JSONPatchType, mock.MatchedBy(func(data []byte) bool { + return string(data) == `[ + { "op": "remove", "path": "/metadata/deletionTimestamp" } + ]` + }), metav1.PatchOptions{ + FieldManager: "provisioning-connection-controller", + }, mock.Anything).Return(&provisioning.Connection{}, nil).Once() }, - expectedError: "remove finalizer: patch error", + expectedError: "remove finalizer: patch error (connection has been undeleted, deletion can be retried)", expectFinalizerRemoved: false, }, { @@ -612,12 +628,70 @@ func TestConnectionController_handleDelete(t *testing.T) { connInterface.AssertCalled(t, "Patch", ctx, "test-conn", types.JSONPatchType, mock.Anything, metav1.PatchOptions{ FieldManager: "provisioning-connection-controller", }, mock.Anything) - } else { - connInterface.AssertNotCalled(t, "Patch", ctx, "test-conn", types.JSONPatchType, mock.Anything, metav1.PatchOptions{}, mock.Anything) + } else if tt.expectedError != "" && strings.Contains(tt.expectedError, "undeleted") { + // For undelete case, we expect both patches to be called (remove finalizer fails, then undelete succeeds) + connInterface.AssertNumberOfCalls(t, "Patch", 2) } + // For other error cases (repositories exist), no successful patch should occur repoLister.AssertExpectations(t) connInterface.AssertExpectations(t) }) } } + +func TestIsTransientError(t *testing.T) { + tests := []struct { + name string + err error + expected bool + }{ + { + name: "nil error", + err: nil, + expected: false, + }, + { + name: "service unavailable", + err: apierrors.NewServiceUnavailable("service unavailable"), + expected: true, + }, + { + name: "server timeout", + err: apierrors.NewServerTimeout(schema.GroupResource{}, "operation", 0), + expected: true, + }, + { + name: "too many requests", + err: apierrors.NewTooManyRequests("too many requests", 0), + expected: true, + }, + { + name: "internal error", + err: apierrors.NewInternalError(errors.New("internal error")), + expected: true, + }, + { + name: "not found error", + err: apierrors.NewNotFound(schema.GroupResource{}, "resource"), + expected: false, + }, + { + name: "forbidden error", + err: apierrors.NewForbidden(schema.GroupResource{}, "resource", errors.New("forbidden")), + expected: false, + }, + { + name: "generic error", + err: errors.New("generic error"), + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := isTransientError(tt.err) + assert.Equal(t, tt.expected, result) + }) + } +}