Merge pull request #87 from authzed/ensure-component-nil-fix

fix: bail from EnsureComponentByHash.Handle when newObj signals requeue

Author: vroldanbet

component/ensure_component.go

@@ -55,6 +55,11 @@ func (e *EnsureComponentByHash[K, A]) Handle(ctx context.Context) {
 	ownedObjs := e.List(ctx, e.nn.MustValue(ctx))
 
 	newObj := e.newObj(ctx)
+	// newObj may have signaled a requeue (e.g. RequeueAPIErr) which cancels
+	// the handler context; bail out before dereferencing the result.
+	if ctx.Err() != nil {
+		return
+	}
 	hash := e.Hash(newObj)
 	newObj = newObj.WithAnnotations(map[string]string{e.HashAnnotationKey: hash})
 

component/ensure_component_test.go

@@ -2,8 +2,10 @@ package component
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
@@ -174,3 +176,83 @@ func TestEnsureServiceHandler(t *testing.T) {
 		})
 	}
 }
+
+// TestEnsureComponentByHashStopsWhenNewObjSignalsRequeue guards against a panic
+// where a newObj factory both calls a queue operation (e.g. RequeueAPIErr) and
+// returns nil. RequeueAPIErr cancels the handler context but does not unwind
+// the goroutine, so without a ctx.Err() check the framework would dereference
+// the nil to attach a hash annotation and segfault. Reproduces the panic
+// observed at authzed/internal#10787.
+func TestEnsureComponentByHashStopsWhenNewObjSignalsRequeue(t *testing.T) {
+	const (
+		hashKey    = "example.com/component-hash"
+		ownerIndex = "owner"
+	)
+
+	ctx, cancel := context.WithCancel(t.Context())
+	defer cancel()
+
+	serviceGVR := corev1.SchemeGroupVersion.WithResource("services")
+	scheme := runtime.NewScheme()
+	require.NoError(t, corev1.AddToScheme(scheme))
+	client := clientfake.NewSimpleDynamicClient(scheme)
+	informerFactory := dynamicinformer.NewDynamicSharedInformerFactory(client, 0)
+	require.NoError(t, informerFactory.ForResource(serviceGVR).Informer().AddIndexers(map[string]cache.IndexFunc{
+		ownerIndex: func(_ interface{}) ([]string, error) {
+			return []string{types.NamespacedName{Namespace: "test", Name: "owner"}.String()}, nil
+		},
+	}))
+	informerFactory.Start(ctx.Done())
+	informerFactory.WaitForCacheSync(ctx.Done())
+	indexer := typed.NewIndexer[*corev1.Service](informerFactory.ForResource(serviceGVR).Informer().GetIndexer())
+	ctxOwner := typedctx.WithDefault[types.NamespacedName](types.NamespacedName{Namespace: "test", Name: "owner"})
+
+	queueOps := queue.NewQueueOperationsCtx()
+
+	// Simulate the manager plumbing: a real Operations whose cancel func
+	// cancels the handler-scoped context, so RequeueAPIErr propagates via
+	// ctx.Err() the same way it does in production.
+	handlerCtx, cancelHandler := context.WithCancel(ctx)
+	defer cancelHandler()
+	ops := queue.NewOperations(func() {}, func(time.Duration) {}, cancelHandler)
+	handlerCtx = queueOps.WithValue(handlerCtx, ops)
+
+	simulatedErr := errors.New("simulated upstream failure")
+	applyCalled := false
+	deleteCalled := false
+
+	h := handler.NewHandler(NewEnsureComponentByHash(
+		NewHashableComponent[*corev1.Service](
+			NewIndexedComponent(
+				indexer,
+				ownerIndex,
+				func(_ context.Context) labels.Selector {
+					return labels.SelectorFromSet(map[string]string{
+						"example.com/component": "the-main-service-component",
+					})
+				}),
+			hash.NewObjectHash(), hashKey),
+		ctxOwner,
+		queueOps,
+		func(_ context.Context, _ *applycorev1.ServiceApplyConfiguration) (*corev1.Service, error) {
+			applyCalled = true
+			return nil, nil
+		},
+		func(_ context.Context, _ types.NamespacedName) error {
+			deleteCalled = true
+			return nil
+		},
+		func(ctx context.Context) *applycorev1.ServiceApplyConfiguration {
+			queueOps.RequeueAPIErr(ctx, simulatedErr)
+			return nil
+		}), "ensureService")
+
+	require.NotPanics(t, func() {
+		h.Handle(handlerCtx)
+	})
+
+	require.False(t, applyCalled, "must not apply when newObj signaled requeue")
+	require.False(t, deleteCalled, "must not delete when newObj signaled requeue")
+	require.Equal(t, simulatedErr, ops.Error(), "Operations must record the requeue error")
+	require.Error(t, handlerCtx.Err(), "handler context must be cancelled by RequeueAPIErr")
+}