fix(api-gateway): populate route WatchSet before watching

.changelog/23857.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+api-gateway: Fix a race condition that could crash Consul servers while reconciling routes with multiple backend services.
+```

agent/consul/controller/controller_test.go

@@ -570,4 +570,18 @@ func TestDiscoveryChainController(t *testing.T) {
 		Name: "foo-2",
 	}))
 	require.True(t, ensureCalled(reconciler.received, "foo-1"))
+
+	// A change after the state read but before AddTrigger closes one of the
+	// WatchSet's channels. Registering that WatchSet must immediately enqueue a
+	// reconciliation so callers can finish populating it before it is watched.
+	ws = memdb.NewWatchSet()
+	ws.Add(store.AbandonCh())
+	_, _, err = store.ReadDiscoveryChainConfigEntries(ws, "foo-3", nil)
+	require.NoError(t, err)
+	require.NoError(t, store.EnsureConfigEntry(2, &structs.ServiceResolverConfigEntry{
+		Kind: structs.ServiceResolver,
+		Name: "foo-3",
+	}))
+	controller.AddTrigger(request, ws.WatchCtx)
+	require.True(t, ensureCalled(reconciler.received, "foo-1"))
 }

agent/consul/gateways/controller_gateways.go

@@ -7,7 +7,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"sync"
 
 	"github.com/hashicorp/go-hclog"
 	"github.com/hashicorp/go-memdb"
@@ -449,19 +448,14 @@ func (r *apiGatewayReconciler) reconcileRoute(_ context.Context, req controller.
 		return nil
 	}
 
-	var triggerOnce sync.Once
-	for _, service := range route.GetServiceNames() {
+	services := route.GetServiceNames()
+	for _, service := range services {
 		_, chainSet, err := store.ReadDiscoveryChainConfigEntries(ws, service.Name, pointerTo(service.EnterpriseMeta))
 		if err != nil {
 			logger.Warn("error reading discovery chain", "error", err)
 			return err
 		}
 
-		// trigger a watch since we now need to check when the discovery chain gets updated
-		triggerOnce.Do(func() {
-			r.controller.AddTrigger(req, ws.WatchCtx)
-		})
-
 		// make sure that we can actually compile a discovery chain based on this route
 		// the main check is to make sure that all of the protocols align
 		chain, err := discoverychain.Compile(discoverychain.CompileRequest{
@@ -488,8 +482,13 @@ func (r *apiGatewayReconciler) reconcileRoute(_ context.Context, req controller.
 	// if we have no upstream targets, then set the route as invalid
 	// this should already happen in the validation check on write, but
 	// we'll do it here too just in case
-	if len(route.GetServiceNames()) == 0 {
+	if len(services) == 0 {
 		updater.SetCondition(routeNoUpstreams())
+	} else {
+		// Populate the WatchSet for every service before handing it to the
+		// controller, which starts watching it asynchronously. A WatchSet is a
+		// map and is not safe to mutate once WatchCtx starts iterating it.
+		r.controller.AddTrigger(req, ws.WatchCtx)
 	}
 
 	// the route is valid, attempt to bind it to all gateways

agent/consul/gateways/controller_gateways_test.go

@@ -4267,6 +4267,73 @@ func TestAPIGatewayController(t *testing.T) {
 	}
 }
 
+func TestAPIGatewayControllerPopulatesWatchSetBeforeRegisteringTrigger(t *testing.T) {
+	t.Parallel()
+
+	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
+	defer cancel()
+
+	store := state.NewStateStore(nil)
+	updater := &Updater{
+		UpdateWithStatus: func(entry structs.ControlledConfigEntry) error { return nil },
+		Update:           func(entry structs.ConfigEntry) error { return nil },
+		Delete:           func(entry structs.ConfigEntry) error { return nil },
+	}
+
+	for i, entry := range []structs.ConfigEntry{
+		&structs.ServiceConfigEntry{
+			Kind:     structs.ServiceDefaults,
+			Name:     "backend-a",
+			Protocol: "http",
+		},
+		&structs.ServiceConfigEntry{
+			Kind:     structs.ServiceDefaults,
+			Name:     "backend-b",
+			Protocol: "http",
+		},
+	} {
+		require.NoError(t, store.EnsureConfigEntry(uint64(i+1), entry))
+	}
+
+	route := &protocolTrackingHTTPRoute{
+		HTTPRouteConfigEntry: &structs.HTTPRouteConfigEntry{
+			Kind: structs.HTTPRoute,
+			Name: "multi-backend-route",
+			Rules: []structs.HTTPRouteRule{
+				{Services: []structs.HTTPService{{Name: "backend-a"}}},
+				{Services: []structs.HTTPService{{Name: "backend-b"}}},
+			},
+		},
+	}
+
+	request := controller.Request{
+		Kind: structs.HTTPRoute,
+		Name: "multi-backend-route",
+		Meta: acl.DefaultEnterpriseMeta(),
+	}
+	triggerCalled := false
+	testController := &noopController{
+		triggers: make(map[controller.Request]struct{}),
+		onAddTrigger: func(actual controller.Request, _ func(context.Context) error) {
+			triggerCalled = true
+			require.Equal(t, request, actual)
+			// GetProtocol is called after each backend's discovery-chain read and
+			// compilation. Both calls must finish before the WatchSet is handed to
+			// the controller's asynchronous trigger.
+			require.Equal(t, len(route.GetServiceNames()), route.protocolCalls)
+		},
+	}
+	reconciler := apiGatewayReconciler{
+		logger:     hclog.Default(),
+		updater:    updater,
+		controller: testController,
+	}
+
+	require.NoError(t, reconciler.reconcileRoute(ctx, request, store, route))
+	require.True(t, triggerCalled)
+	require.True(t, route.Status.MatchesConditionStatus(routeAccepted()))
+}
+
 func TestNewAPIGatewayController(t *testing.T) {
 	t.Parallel()
 
@@ -4290,8 +4357,19 @@ func TestNewAPIGatewayController(t *testing.T) {
 }
 
 type noopController struct {
-	triggers map[controller.Request]struct{}
-	enqueued []controller.Request
+	triggers     map[controller.Request]struct{}
+	enqueued     []controller.Request
+	onAddTrigger func(controller.Request, func(context.Context) error)
+}
+
+type protocolTrackingHTTPRoute struct {
+	*structs.HTTPRouteConfigEntry
+	protocolCalls int
+}
+
+func (r *protocolTrackingHTTPRoute) GetProtocol() structs.APIGatewayListenerProtocol {
+	r.protocolCalls++
+	return r.HTTPRouteConfigEntry.GetProtocol()
 }
 
 func (n *noopController) Run(ctx context.Context) error { return nil }
@@ -4307,6 +4385,9 @@ func (n *noopController) WithQueueFactory(fn func(ctx context.Context, baseBacko
 
 func (n *noopController) AddTrigger(request controller.Request, trigger func(ctx context.Context) error) {
 	n.triggers[request] = struct{}{}
+	if n.onAddTrigger != nil {
+		n.onAddTrigger(request, trigger)
+	}
 }
 
 func (n *noopController) RemoveTrigger(request controller.Request) {