fix: address review feedback for HCP finalizer in AWSEndpointService

- Switch HCP watch from handler.Funcs{UpdateFunc:...} to
  handler.EnqueueRequestsFromMapFunc to handle Create events on
  controller restart (matching Azure PLS pattern)
- Add DeletionTimestamp guard in reconcileHCPDeletion to prevent
  dual deletion path race under MaxConcurrentReconciles > 1
- Document convergent multi-CR coordination pattern
- Use RequeueAfter: time.Second instead of Requeue: true on conflicts
  to prevent tight retry loops under contention
- Fix overstated comment about finalizer guarantee, acknowledge
  edge case where controller hasn't reconciled before HCP deletion

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: OpenShift CI Bot

control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go

@@ -46,8 +46,8 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
-	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
+	"sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	"sigs.k8s.io/controller-runtime/pkg/source"
@@ -381,7 +381,9 @@ func (r *AWSEndpointServiceReconciler) SetupWithManager(mgr ctrl.Manager) error
 			RateLimiter:             workqueue.NewTypedItemExponentialFailureRateLimiter[reconcile.Request](3*time.Second, 30*time.Second),
 			MaxConcurrentReconciles: 10,
 		}).
-		Watches(&hyperv1.HostedControlPlane{}, handler.Funcs{UpdateFunc: r.enqueueOnHCPChange(mgr)}).
+		Watches(&hyperv1.HostedControlPlane{}, handler.EnqueueRequestsFromMapFunc(
+			r.mapHCPToAWSEndpointService(),
+		)).
 		Build(r)
 	if err != nil {
 		return fmt.Errorf("failed setting up with a controller manager: %w", err)
@@ -391,48 +393,42 @@ func (r *AWSEndpointServiceReconciler) SetupWithManager(mgr ctrl.Manager) error
 	return nil
 }
 
-// enqueueOnHCPChange triggers reconciliation of all AWSEndpointService CRs in the namespace when:
-//   - the HCP's EndpointAccess value changes, OR
-//   - the HCP is being deleted and has our finalizer (to trigger AWS resource cleanup)
-func (r *AWSEndpointServiceReconciler) enqueueOnHCPChange(mgr ctrl.Manager) func(context.Context, event.UpdateEvent, workqueue.TypedRateLimitingInterface[reconcile.Request]) {
-	return func(ctx context.Context, e event.UpdateEvent, q workqueue.TypedRateLimitingInterface[reconcile.Request]) {
-		logger := mgr.GetLogger()
-		newHCP, isOk := e.ObjectNew.(*hyperv1.HostedControlPlane)
-		if !isOk {
-			logger.Info("WARNING: enqueueOnHCPChange: new resource is not of type HostedControlPlane")
-			return
-		}
-		oldHCP, isOk := e.ObjectOld.(*hyperv1.HostedControlPlane)
-		if !isOk {
-			logger.Info("WARNING: enqueueOnHCPChange: old resource is not of type HostedControlPlane")
-			return
-		}
-
-		shouldEnqueue := false
-
-		// Enqueue when HCP is being deleted and has our finalizer, so we can
-		// clean up AWS resources before HCP credentials are torn down.
-		if !newHCP.DeletionTimestamp.IsZero() && controllerutil.ContainsFinalizer(newHCP, hcpAWSPrivateLinkFinalizerName) {
-			shouldEnqueue = true
-		}
-
-		// Enqueue when EndpointAccess changes (existing behavior).
-		if newHCP.Spec.Platform.AWS != nil && oldHCP.Spec.Platform.AWS != nil && newHCP.Spec.Platform.AWS.EndpointAccess != oldHCP.Spec.Platform.AWS.EndpointAccess {
-			shouldEnqueue = true
+// mapHCPToAWSEndpointService maps HostedControlPlane events to the AWSEndpointService CRs
+// in the same namespace. This uses EnqueueRequestsFromMapFunc (instead of handler.Funcs)
+// so that all event types — Create, Update, Delete — trigger the mapping. This is critical
+// for controller restarts: if the CPO restarts while an HCP is being deleted (DeletionTimestamp
+// already set), the informer cache sync generates a Create event, not an Update. Using
+// handler.Funcs{UpdateFunc: ...} would miss this, defeating the purpose of the HCP finalizer.
+//
+// The function only enqueues CRs when the HCP has our finalizer, avoiding unnecessary
+// reconciliations for unrelated HCPs. EndpointAccess changes (previously detected via
+// old/new comparison) are picked up by the reconciler's periodic 5-minute requeue.
+func (r *AWSEndpointServiceReconciler) mapHCPToAWSEndpointService() handler.MapFunc {
+	return func(ctx context.Context, obj client.Object) []reconcile.Request {
+		hcp, ok := obj.(*hyperv1.HostedControlPlane)
+		if !ok {
+			return nil
 		}
 
-		if !shouldEnqueue {
-			return
+		// Only trigger reconciliation when the HCP has our finalizer; this avoids
+		// unnecessary reconciliations for HCPs that are not related to AWS PrivateLink.
+		if !controllerutil.ContainsFinalizer(hcp, hcpAWSPrivateLinkFinalizerName) {
+			return nil
 		}
 
 		awsEndpointServiceList := &hyperv1.AWSEndpointServiceList{}
-		if err := r.List(ctx, awsEndpointServiceList, client.InNamespace(newHCP.Namespace)); err != nil {
-			logger.Error(err, "enqueueOnHCPChange: cannot list awsendpointservices")
-			return
+		if err := r.List(ctx, awsEndpointServiceList, client.InNamespace(hcp.Namespace)); err != nil {
+			log.FromContext(ctx).Error(err, "mapHCPToAWSEndpointService: cannot list AWSEndpointService resources", "namespace", hcp.Namespace)
+			return nil
 		}
+
+		var requests []reconcile.Request
 		for i := range awsEndpointServiceList.Items {
-			q.Add(reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&awsEndpointServiceList.Items[i])})
+			requests = append(requests, reconcile.Request{
+				NamespacedName: client.ObjectKeyFromObject(&awsEndpointServiceList.Items[i]),
+			})
 		}
+		return requests
 	}
 }
 
@@ -471,11 +467,12 @@ func (r *AWSEndpointServiceReconciler) Reconcile(ctx context.Context, req ctrl.R
 		// the non-deletion path. If the HCP still exists, initialize from it so that
 		// getClients can succeed and deletion can proceed.
 		//
-		// The HCP finalizer (hcpAWSPrivateLinkFinalizerName) added during normal
-		// reconciliation ensures the HCP remains available during this cleanup.
-		// For SharedVPC clusters, this guarantees the cross-account role ARNs can
-		// always be read from the HCP spec. See reconcileHCPDeletion for the
-		// HCP-deletion cleanup path.
+		// The HCP finalizer (hcpAWSPrivateLinkFinalizerName), when present, blocks HCP
+		// deletion so the HCP remains available during this cleanup. Note: the finalizer
+		// is only added after a successful normal reconciliation. If the controller has
+		// not yet reconciled (e.g., controller was down since cluster creation), the HCP
+		// may be deleted before the finalizer is placed, and this best-effort initialization
+		// is the only protection. See reconcileHCPDeletion for the HCP-deletion cleanup path.
 		hcpList := &hyperv1.HostedControlPlaneList{}
 		if err := r.List(ctx, hcpList, &client.ListOptions{Namespace: req.Namespace}); err == nil && len(hcpList.Items) == 1 {
 			r.awsClientBuilder.initializeWithHCP(log, &hcpList.Items[0])
@@ -615,7 +612,9 @@ func (r *AWSEndpointServiceReconciler) ensureHCPFinalizer(ctx context.Context, h
 	controllerutil.AddFinalizer(hcp, hcpAWSPrivateLinkFinalizerName)
 	if err := r.Patch(ctx, hcp, client.MergeFromWithOptions(originalHCP, client.MergeFromWithOptimisticLock{})); err != nil {
 		if apierrors.IsConflict(err) {
-			return ctrl.Result{Requeue: true}, nil
+			// Use RequeueAfter to avoid a tight retry loop when multiple AWSEndpointService
+			// reconcilers concurrently try to patch the same HCP.
+			return ctrl.Result{RequeueAfter: time.Second}, nil
 		}
 		return ctrl.Result{}, fmt.Errorf("failed to add HCP finalizer: %w", err)
 	}
@@ -626,11 +625,29 @@ func (r *AWSEndpointServiceReconciler) ensureHCPFinalizer(ctx context.Context, h
 // and removing the HCP finalizer. This ensures AWS credentials remain valid during
 // the cleanup process, which is critical for SharedVPC clusters where the cross-account
 // role ARNs are sourced from the HCP spec.
+//
+// Convergent multi-CR coordination:
+// With MaxConcurrentReconciles: 10 and the HCP watch enqueuing ALL CRs on HCP deletion,
+// multiple reconcilers run this function concurrently — one per AWSEndpointService CR.
+// Each reconciler cleans up its own CR's AWS resources, then checks whether all CRs in
+// the namespace are done. Only the last reconciler to finish (the one that finds no pending
+// CRs) removes the HCP finalizer. Earlier reconcilers see pending CRs and return
+// RequeueAfter to retry; when they re-enter, they find the HCP finalizer already removed
+// and return early at the top of this function. This convergent pattern produces a small
+// number of no-op requeues but is correct and self-healing.
 func (r *AWSEndpointServiceReconciler) reconcileHCPDeletion(ctx context.Context, awsEndpointService *hyperv1.AWSEndpointService, hcp *hyperv1.HostedControlPlane, log logr.Logger) (ctrl.Result, error) {
 	if !controllerutil.ContainsFinalizer(hcp, hcpAWSPrivateLinkFinalizerName) {
 		return ctrl.Result{}, nil
 	}
 
+	// If the AWSEndpointService itself is being deleted, let the existing CR deletion
+	// path (which returns early at the top of Reconcile) handle cleanup. This prevents
+	// the two deletion paths from racing under MaxConcurrentReconciles > 1, where the
+	// CR deletion path and HCP deletion path could both try to clean up the same CR.
+	if !awsEndpointService.DeletionTimestamp.IsZero() {
+		return ctrl.Result{}, nil
+	}
+
 	log.Info("HCP is being deleted, cleaning up AWS PrivateLink resources before removing HCP finalizer")
 
 	// Initialize AWS clients from the HCP — guaranteed to be available because
@@ -680,7 +697,9 @@ func (r *AWSEndpointServiceReconciler) reconcileHCPDeletion(ctx context.Context,
 	controllerutil.RemoveFinalizer(hcp, hcpAWSPrivateLinkFinalizerName)
 	if err := r.Patch(ctx, hcp, client.MergeFromWithOptions(originalHCP, client.MergeFromWithOptimisticLock{})); err != nil {
 		if apierrors.IsConflict(err) {
-			return ctrl.Result{Requeue: true}, nil
+			// Use RequeueAfter to avoid a tight retry loop when multiple AWSEndpointService
+			// reconcilers concurrently try to patch the same HCP.
+			return ctrl.Result{RequeueAfter: time.Second}, nil
 		}
 		return ctrl.Result{}, fmt.Errorf("failed to remove HCP finalizer: %w", err)
 	}

control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller_test.go

@@ -26,17 +26,14 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/client-go/util/workqueue"
 
 	ctrl "sigs.k8s.io/controller-runtime"
 	crclient "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
-	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
-	"github.com/go-logr/logr"
 	"github.com/google/go-cmp/cmp"
 	"go.uber.org/mock/gomock"
 )
@@ -1820,7 +1817,11 @@ func TestEnsureHCPFinalizer(t *testing.T) {
 			} else {
 				g.Expect(err).ToNot(HaveOccurred())
 			}
-			g.Expect(result.Requeue).To(Equal(tc.expectRequeue))
+			if tc.expectRequeue {
+				g.Expect(result.RequeueAfter).To(Equal(time.Second))
+			} else {
+				g.Expect(result.IsZero()).To(BeTrue())
+			}
 
 			updatedHCP := &hyperv1.HostedControlPlane{}
 			err = fakeClient.Get(ctx, types.NamespacedName{
@@ -2353,34 +2354,24 @@ func TestReconcileHCPDeletionClientErrors(t *testing.T) {
 				g.Expect(err).ToNot(HaveOccurred())
 			}
 			if tc.expectRequeue {
-				g.Expect(result.Requeue).To(BeTrue())
+				g.Expect(result.RequeueAfter).To(Equal(time.Second))
 			}
 		})
 	}
 }
 
-type fakeManagerForLogger struct {
-	ctrl.Manager
-	logger logr.Logger
-}
-
-func (m *fakeManagerForLogger) GetLogger() logr.Logger {
-	return m.logger
-}
-
-func TestEnqueueOnHCPChange(t *testing.T) {
+func TestMapHCPToAWSEndpointService(t *testing.T) {
 	now := metav1.NewTime(time.Now())
 
 	testCases := []struct {
 		name           string
-		oldHCP         *hyperv1.HostedControlPlane
-		newHCP         *hyperv1.HostedControlPlane
+		hcp            *hyperv1.HostedControlPlane
 		existingCRs    []crclient.Object
 		expectEnqueued int
 	}{
 		{
-			name: "When HCP is being deleted with our finalizer it should enqueue all CRs",
-			oldHCP: &hyperv1.HostedControlPlane{
+			name: "When HCP has our finalizer it should enqueue all CRs",
+			hcp: &hyperv1.HostedControlPlane{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:       "test-hcp",
 					Namespace:  "clusters-test",
@@ -2392,37 +2383,21 @@ func TestEnqueueOnHCPChange(t *testing.T) {
 					},
 				},
 			},
-			newHCP: &hyperv1.HostedControlPlane{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:              "test-hcp",
-					Namespace:         "clusters-test",
-					DeletionTimestamp: &now,
-					Finalizers:        []string{hcpAWSPrivateLinkFinalizerName},
-				},
-				Spec: hyperv1.HostedControlPlaneSpec{
-					Platform: hyperv1.PlatformSpec{
-						AWS: &hyperv1.AWSPlatformSpec{EndpointAccess: hyperv1.Private},
-					},
-				},
-			},
 			existingCRs: []crclient.Object{
 				&hyperv1.AWSEndpointService{ObjectMeta: metav1.ObjectMeta{Name: "kube-apiserver-private", Namespace: "clusters-test"}},
 				&hyperv1.AWSEndpointService{ObjectMeta: metav1.ObjectMeta{Name: "private-router", Namespace: "clusters-test"}},
 			},
 			expectEnqueued: 2,
 		},
 		{
-			name: "When EndpointAccess changes it should enqueue all CRs",
-			oldHCP: &hyperv1.HostedControlPlane{
-				ObjectMeta: metav1.ObjectMeta{Name: "test-hcp", Namespace: "clusters-test"},
-				Spec: hyperv1.HostedControlPlaneSpec{
-					Platform: hyperv1.PlatformSpec{
-						AWS: &hyperv1.AWSPlatformSpec{EndpointAccess: hyperv1.Public},
-					},
+			name: "When HCP is being deleted with our finalizer it should enqueue all CRs",
+			hcp: &hyperv1.HostedControlPlane{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:              "test-hcp",
+					Namespace:         "clusters-test",
+					DeletionTimestamp: &now,
+					Finalizers:        []string{hcpAWSPrivateLinkFinalizerName},
 				},
-			},
-			newHCP: &hyperv1.HostedControlPlane{
-				ObjectMeta: metav1.ObjectMeta{Name: "test-hcp", Namespace: "clusters-test"},
 				Spec: hyperv1.HostedControlPlaneSpec{
 					Platform: hyperv1.PlatformSpec{
 						AWS: &hyperv1.AWSPlatformSpec{EndpointAccess: hyperv1.Private},
@@ -2431,20 +2406,13 @@ func TestEnqueueOnHCPChange(t *testing.T) {
 			},
 			existingCRs: []crclient.Object{
 				&hyperv1.AWSEndpointService{ObjectMeta: metav1.ObjectMeta{Name: "kube-apiserver-private", Namespace: "clusters-test"}},
+				&hyperv1.AWSEndpointService{ObjectMeta: metav1.ObjectMeta{Name: "private-router", Namespace: "clusters-test"}},
 			},
-			expectEnqueued: 1,
+			expectEnqueued: 2,
 		},
 		{
-			name: "When HCP is deleted without our finalizer it should not enqueue",
-			oldHCP: &hyperv1.HostedControlPlane{
-				ObjectMeta: metav1.ObjectMeta{Name: "test-hcp", Namespace: "clusters-test"},
-				Spec: hyperv1.HostedControlPlaneSpec{
-					Platform: hyperv1.PlatformSpec{
-						AWS: &hyperv1.AWSPlatformSpec{EndpointAccess: hyperv1.Private},
-					},
-				},
-			},
-			newHCP: &hyperv1.HostedControlPlane{
+			name: "When HCP does not have our finalizer it should not enqueue",
+			hcp: &hyperv1.HostedControlPlane{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:              "test-hcp",
 					Namespace:         "clusters-test",
@@ -2462,26 +2430,15 @@ func TestEnqueueOnHCPChange(t *testing.T) {
 			expectEnqueued: 0,
 		},
 		{
-			name: "When no relevant HCP changes occur it should not enqueue",
-			oldHCP: &hyperv1.HostedControlPlane{
-				ObjectMeta: metav1.ObjectMeta{Name: "test-hcp", Namespace: "clusters-test"},
-				Spec: hyperv1.HostedControlPlaneSpec{
-					Platform: hyperv1.PlatformSpec{
-						AWS: &hyperv1.AWSPlatformSpec{EndpointAccess: hyperv1.Private},
-					},
-				},
-			},
-			newHCP: &hyperv1.HostedControlPlane{
-				ObjectMeta: metav1.ObjectMeta{Name: "test-hcp", Namespace: "clusters-test"},
-				Spec: hyperv1.HostedControlPlaneSpec{
-					Platform: hyperv1.PlatformSpec{
-						AWS: &hyperv1.AWSPlatformSpec{EndpointAccess: hyperv1.Private},
-					},
+			name: "When HCP has our finalizer but no CRs exist it should return empty",
+			hcp: &hyperv1.HostedControlPlane{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "test-hcp",
+					Namespace:  "clusters-test",
+					Finalizers: []string{hcpAWSPrivateLinkFinalizerName},
 				},
 			},
-			existingCRs: []crclient.Object{
-				&hyperv1.AWSEndpointService{ObjectMeta: metav1.ObjectMeta{Name: "kube-apiserver-private", Namespace: "clusters-test"}},
-			},
+			existingCRs:    nil,
 			expectEnqueued: 0,
 		},
 	}
@@ -2502,20 +2459,12 @@ func TestEnqueueOnHCPChange(t *testing.T) {
 				Client: fakeClient,
 			}
 
-			fakeMgr := &fakeManagerForLogger{logger: ctrl.Log.WithName("test")}
-			handler := reconciler.enqueueOnHCPChange(fakeMgr)
-
-			q := workqueue.NewTypedRateLimitingQueue(
-				workqueue.NewTypedItemExponentialFailureRateLimiter[reconcile.Request](time.Second, time.Second),
-			)
-			defer q.ShutDown()
+			mapFunc := reconciler.mapHCPToAWSEndpointService()
 
-			handler(context.Background(), event.UpdateEvent{
-				ObjectOld: tc.oldHCP,
-				ObjectNew: tc.newHCP,
-			}, q)
+			ctx := ctrl.LoggerInto(context.Background(), ctrl.Log.WithName("test"))
+			requests := mapFunc(ctx, tc.hcp)
 
-			g.Expect(q.Len()).To(Equal(tc.expectEnqueued))
+			g.Expect(len(requests)).To(Equal(tc.expectEnqueued))
 		})
 	}
 }