From 641737f32b6ce151f5870a9d8123dc0faf436121 Mon Sep 17 00:00:00 2001 From: Andrew Stucki Date: Wed, 22 Feb 2023 14:55:40 -0500 Subject: [PATCH] [API Gateway] Fix infinite loop in controller and binding non-accepted routes and gateways (#16377) --- agent/consul/gateways/controller_gateways.go | 52 ++-- .../gateways/controller_gateways_test.go | 259 +++++++++++++++++- agent/structs/config_entry_status.go | 10 + 3 files changed, 286 insertions(+), 35 deletions(-) diff --git a/agent/consul/gateways/controller_gateways.go b/agent/consul/gateways/controller_gateways.go index b1197d0ba4..cfc5a25ba7 100644 --- a/agent/consul/gateways/controller_gateways.go +++ b/agent/consul/gateways/controller_gateways.go @@ -409,7 +409,6 @@ func (r *apiGatewayReconciler) reconcileRoute(_ context.Context, req controller. } var triggerOnce sync.Once - validTargets := true for _, service := range route.GetServiceNames() { _, chainSet, err := store.ReadDiscoveryChainConfigEntries(ws, service.Name, pointerTo(service.EnterpriseMeta)) if err != nil { @@ -422,11 +421,6 @@ func (r *apiGatewayReconciler) reconcileRoute(_ context.Context, req controller. r.controller.AddTrigger(req, ws.WatchCtx) }) - if chainSet.IsEmpty() { - updater.SetCondition(conditions.routeInvalidDiscoveryChain(errServiceDoesNotExist)) - continue - } - // 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{ @@ -438,28 +432,16 @@ func (r *apiGatewayReconciler) reconcileRoute(_ context.Context, req controller. Entries: chainSet, }) if err != nil { - // we only really need to return the first error for an invalid - // discovery chain, but we still want to set watches on everything in the - // store - if validTargets { - updater.SetCondition(conditions.routeInvalidDiscoveryChain(err)) - validTargets = false - } + updater.SetCondition(conditions.routeInvalidDiscoveryChain(err)) continue } if chain.Protocol != string(route.GetProtocol()) { - if validTargets { - updater.SetCondition(conditions.routeInvalidDiscoveryChain(errInvalidProtocol)) - validTargets = false - } + updater.SetCondition(conditions.routeInvalidDiscoveryChain(errInvalidProtocol)) continue } - // this makes sure we don't override an already set status - if validTargets { - updater.SetCondition(conditions.routeAccepted()) - } + updater.SetCondition(conditions.routeAccepted()) } // if we have no upstream targets, then set the route as invalid @@ -467,17 +449,6 @@ func (r *apiGatewayReconciler) reconcileRoute(_ context.Context, req controller. // we'll do it here too just in case if len(route.GetServiceNames()) == 0 { updater.SetCondition(conditions.routeNoUpstreams()) - validTargets = false - } - - if !validTargets { - // we return early, but need to make sure we're removed from all referencing - // gateways and our status is updated properly - updated := []*structs.BoundAPIGatewayConfigEntry{} - for _, modifiedGateway := range removeRoute(requestToResourceRef(req), meta...) { - updated = append(updated, modifiedGateway.BoundGateway) - } - return finalize(updated) } // the route is valid, attempt to bind it to all gateways @@ -575,6 +546,8 @@ type gatewayMeta struct { // the map values are pointers so that we can update them directly // and have the changes propagate back to the container gateways. boundListeners map[string]*structs.BoundAPIGatewayListener + + generator *gatewayConditionGenerator } // getAllGatewayMeta returns a pre-constructed list of all valid gateway and state @@ -701,11 +674,22 @@ func (g *gatewayMeta) bindRoute(listener *structs.APIGatewayListener, bound *str return false, nil } + // check to make sure we're not binding to an invalid gateway + if !g.Gateway.Status.MatchesConditionStatus(g.generator.gatewayAccepted()) { + return false, fmt.Errorf("failed to bind route to gateway %s: gateway has not been accepted", g.Gateway.Name) + } + + // check to make sure we're not binding to an invalid route + status := route.GetStatus() + if !status.MatchesConditionStatus(g.generator.routeAccepted()) { + return false, fmt.Errorf("failed to bind route to gateway %s: route has not been accepted", g.Gateway.Name) + } + if route, ok := route.(*structs.HTTPRouteConfigEntry); ok { // check our hostnames hostnames := route.FilteredHostnames(listener.GetHostname()) if len(hostnames) == 0 { - return false, fmt.Errorf("failed to bind route to gateway %s: listener %s is does not have any hostnames that match the route", route.GetName(), g.Gateway.Name) + return false, fmt.Errorf("failed to bind route to gateway %s: listener %s is does not have any hostnames that match the route", g.Gateway.Name, listener.Name) } } @@ -809,6 +793,8 @@ func (g *gatewayMeta) setConflicts(updater *structs.StatusUpdater) { // initialize sets up the listener maps that we use for quickly indexing the listeners in our binding logic func (g *gatewayMeta) initialize() *gatewayMeta { + g.generator = newGatewayConditionGenerator() + // set up the maps for fast access g.boundListeners = make(map[string]*structs.BoundAPIGatewayListener, len(g.BoundGateway.Listeners)) for i, listener := range g.BoundGateway.Listeners { diff --git a/agent/consul/gateways/controller_gateways_test.go b/agent/consul/gateways/controller_gateways_test.go index 0c47809c76..805a5738ce 100644 --- a/agent/consul/gateways/controller_gateways_test.go +++ b/agent/consul/gateways/controller_gateways_test.go @@ -49,6 +49,11 @@ func TestBoundAPIGatewayBindRoute(t *testing.T) { Protocol: structs.ListenerProtocolTCP, }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().gatewayAccepted(), + }, + }, }, }, route: &structs.TCPRouteConfigEntry{ @@ -61,6 +66,11 @@ func TestBoundAPIGatewayBindRoute(t *testing.T) { SectionName: "Listener", }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().routeAccepted(), + }, + }, }, expectedBoundGateway: structs.BoundAPIGatewayConfigEntry{ Kind: structs.BoundAPIGateway, @@ -116,6 +126,11 @@ func TestBoundAPIGatewayBindRoute(t *testing.T) { Protocol: structs.ListenerProtocolTCP, }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().gatewayAccepted(), + }, + }, }, }, route: &structs.TCPRouteConfigEntry{ @@ -127,6 +142,11 @@ func TestBoundAPIGatewayBindRoute(t *testing.T) { Name: "Gateway", }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().routeAccepted(), + }, + }, }, expectedBoundGateway: structs.BoundAPIGatewayConfigEntry{ Kind: structs.BoundAPIGateway, @@ -417,6 +437,11 @@ func TestBindRoutesToGateways(t *testing.T) { Protocol: structs.ListenerProtocolTCP, }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().gatewayAccepted(), + }, + }, }, }, }, @@ -431,6 +456,11 @@ func TestBindRoutesToGateways(t *testing.T) { SectionName: "Listener", }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().routeAccepted(), + }, + }, }, }, expectedBoundAPIGateways: []*structs.BoundAPIGatewayConfigEntry{ @@ -521,6 +551,11 @@ func TestBindRoutesToGateways(t *testing.T) { Protocol: structs.ListenerProtocolTCP, }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().gatewayAccepted(), + }, + }, }, }, { @@ -541,6 +576,11 @@ func TestBindRoutesToGateways(t *testing.T) { Protocol: structs.ListenerProtocolTCP, }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().gatewayAccepted(), + }, + }, }, }, }, @@ -560,6 +600,11 @@ func TestBindRoutesToGateways(t *testing.T) { SectionName: "Listener", }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().routeAccepted(), + }, + }, }, }, expectedBoundAPIGateways: []*structs.BoundAPIGatewayConfigEntry{ @@ -624,6 +669,11 @@ func TestBindRoutesToGateways(t *testing.T) { Protocol: structs.ListenerProtocolTCP, }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().gatewayAccepted(), + }, + }, }, }, }, @@ -638,6 +688,11 @@ func TestBindRoutesToGateways(t *testing.T) { SectionName: "Listener 2", }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().routeAccepted(), + }, + }, }, }, expectedBoundAPIGateways: []*structs.BoundAPIGatewayConfigEntry{ @@ -691,6 +746,11 @@ func TestBindRoutesToGateways(t *testing.T) { Protocol: structs.ListenerProtocolTCP, }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().gatewayAccepted(), + }, + }, }, }, }, @@ -705,6 +765,11 @@ func TestBindRoutesToGateways(t *testing.T) { SectionName: "", }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().routeAccepted(), + }, + }, }, }, expectedBoundAPIGateways: []*structs.BoundAPIGatewayConfigEntry{ @@ -770,6 +835,11 @@ func TestBindRoutesToGateways(t *testing.T) { Protocol: structs.ListenerProtocolTCP, }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().gatewayAccepted(), + }, + }, }, }, }, @@ -784,6 +854,11 @@ func TestBindRoutesToGateways(t *testing.T) { SectionName: "", }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().routeAccepted(), + }, + }, }, }, expectedBoundAPIGateways: []*structs.BoundAPIGatewayConfigEntry{ @@ -843,6 +918,11 @@ func TestBindRoutesToGateways(t *testing.T) { Protocol: structs.ListenerProtocolTCP, }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().gatewayAccepted(), + }, + }, }, }, { @@ -871,6 +951,11 @@ func TestBindRoutesToGateways(t *testing.T) { Protocol: structs.ListenerProtocolTCP, }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().gatewayAccepted(), + }, + }, }, }, }, @@ -890,6 +975,11 @@ func TestBindRoutesToGateways(t *testing.T) { SectionName: "Listener 2", }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().routeAccepted(), + }, + }, }, }, expectedBoundAPIGateways: []*structs.BoundAPIGatewayConfigEntry{ @@ -968,6 +1058,11 @@ func TestBindRoutesToGateways(t *testing.T) { Protocol: structs.ListenerProtocolTCP, }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().gatewayAccepted(), + }, + }, }, }, }, @@ -982,6 +1077,11 @@ func TestBindRoutesToGateways(t *testing.T) { SectionName: "Listener 2", }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().routeAccepted(), + }, + }, }, }, expectedBoundAPIGateways: []*structs.BoundAPIGatewayConfigEntry{ @@ -1027,6 +1127,11 @@ func TestBindRoutesToGateways(t *testing.T) { Protocol: structs.ListenerProtocolTCP, }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().gatewayAccepted(), + }, + }, }, }, { @@ -1047,6 +1152,11 @@ func TestBindRoutesToGateways(t *testing.T) { Protocol: structs.ListenerProtocolTCP, }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().gatewayAccepted(), + }, + }, }, }, }, @@ -1061,6 +1171,11 @@ func TestBindRoutesToGateways(t *testing.T) { SectionName: "Listener 1", }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().routeAccepted(), + }, + }, }, &structs.TCPRouteConfigEntry{ Name: "TCP Route 2", @@ -1072,6 +1187,11 @@ func TestBindRoutesToGateways(t *testing.T) { SectionName: "Listener 2", }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().routeAccepted(), + }, + }, }, }, expectedBoundAPIGateways: []*structs.BoundAPIGatewayConfigEntry{ @@ -1128,6 +1248,11 @@ func TestBindRoutesToGateways(t *testing.T) { Protocol: structs.ListenerProtocolHTTP, }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().gatewayAccepted(), + }, + }, }, }, }, @@ -1142,6 +1267,11 @@ func TestBindRoutesToGateways(t *testing.T) { SectionName: "Listener", }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().routeAccepted(), + }, + }, }, }, expectedBoundAPIGateways: []*structs.BoundAPIGatewayConfigEntry{}, @@ -1181,6 +1311,11 @@ func TestBindRoutesToGateways(t *testing.T) { Protocol: structs.ListenerProtocolTCP, }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().gatewayAccepted(), + }, + }, }, }, }, @@ -1195,6 +1330,11 @@ func TestBindRoutesToGateways(t *testing.T) { SectionName: "", }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().routeAccepted(), + }, + }, }, }, expectedBoundAPIGateways: []*structs.BoundAPIGatewayConfigEntry{ @@ -1289,6 +1429,11 @@ func TestBindRoutesToGateways(t *testing.T) { Protocol: structs.ListenerProtocolTCP, }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().gatewayAccepted(), + }, + }, }, }, }, @@ -1302,6 +1447,11 @@ func TestBindRoutesToGateways(t *testing.T) { Kind: structs.APIGateway, }, }, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().routeAccepted(), + }, + }, }, }, expectedBoundAPIGateways: []*structs.BoundAPIGatewayConfigEntry{}, @@ -1430,7 +1580,7 @@ func TestAPIGatewayController(t *testing.T) { }, }, }, - "tcp-route-no-gateways-invalid-targets": { + "tcp-route-not-accepted-bind": { requests: []controller.Request{{ Kind: structs.TCPRoute, Name: "tcp-route", @@ -1444,6 +1594,27 @@ func TestAPIGatewayController(t *testing.T) { Services: []structs.TCPService{{ Name: "tcp-upstream", }}, + Parents: []structs.ResourceReference{{ + Name: "api-gateway", + EnterpriseMeta: *defaultMeta, + }}, + }, + &structs.APIGatewayConfigEntry{ + Kind: structs.APIGateway, + Name: "api-gateway", + EnterpriseMeta: *defaultMeta, + Listeners: []structs.APIGatewayListener{{ + Name: "listener", + Port: 80, + }}, + }, + &structs.BoundAPIGatewayConfigEntry{ + Kind: structs.BoundAPIGateway, + Name: "api-gateway", + EnterpriseMeta: *defaultMeta, + Listeners: []structs.BoundAPIGatewayListener{{ + Name: "listener", + }}, }, }, finalEntries: []structs.ConfigEntry{ @@ -1453,10 +1624,41 @@ func TestAPIGatewayController(t *testing.T) { EnterpriseMeta: *defaultMeta, Status: structs.Status{ Conditions: []structs.Condition{ - conditions.routeInvalidDiscoveryChain(errServiceDoesNotExist), + conditions.routeAccepted(), + conditions.routeUnbound(structs.ResourceReference{ + Name: "api-gateway", + EnterpriseMeta: *defaultMeta, + }, errors.New("failed to bind route to gateway api-gateway: gateway has not been accepted")), }, }, }, + &structs.APIGatewayConfigEntry{ + Kind: structs.APIGateway, + Name: "api-gateway", + EnterpriseMeta: *defaultMeta, + Listeners: []structs.APIGatewayListener{{ + Name: "listener", + Port: 80, + }}, + Status: structs.Status{ + Conditions: []structs.Condition{ + conditions.gatewayListenerNoConflicts(structs.ResourceReference{ + Kind: structs.APIGateway, + Name: "api-gateway", + SectionName: "listener", + EnterpriseMeta: *defaultMeta, + }), + }, + }, + }, + &structs.BoundAPIGatewayConfigEntry{ + Kind: structs.BoundAPIGateway, + Name: "api-gateway", + EnterpriseMeta: *defaultMeta, + Listeners: []structs.BoundAPIGatewayListener{{ + Name: "listener", + }}, + }, }, }, "tcp-route-no-gateways-invalid-targets-bad-protocol": { @@ -1748,6 +1950,11 @@ func TestAPIGatewayController(t *testing.T) { Name: "gateway", EnterpriseMeta: *defaultMeta, }}, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().routeAccepted(), + }, + }, }, &structs.ServiceConfigEntry{ Kind: structs.ServiceDefaults, @@ -1763,6 +1970,11 @@ func TestAPIGatewayController(t *testing.T) { Protocol: structs.ListenerProtocolTCP, Port: 22, }}, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().gatewayAccepted(), + }, + }, }, &structs.BoundAPIGatewayConfigEntry{ Kind: structs.BoundAPIGateway, @@ -1840,6 +2052,11 @@ func TestAPIGatewayController(t *testing.T) { Name: "gateway", EnterpriseMeta: *defaultMeta, }}, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().routeAccepted(), + }, + }, }, &structs.ServiceConfigEntry{ Kind: structs.ServiceDefaults, @@ -1931,6 +2148,11 @@ func TestAPIGatewayController(t *testing.T) { Name: "gateway", EnterpriseMeta: *defaultMeta, }}, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().routeAccepted(), + }, + }, }, &structs.TCPRouteConfigEntry{ Kind: structs.TCPRoute, @@ -1944,6 +2166,11 @@ func TestAPIGatewayController(t *testing.T) { Name: "gateway", EnterpriseMeta: *defaultMeta, }}, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().routeAccepted(), + }, + }, }, &structs.ServiceConfigEntry{ Kind: structs.ServiceDefaults, @@ -2061,6 +2288,11 @@ func TestAPIGatewayController(t *testing.T) { Name: "gateway", EnterpriseMeta: *defaultMeta, }}, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().routeAccepted(), + }, + }, }, &structs.HTTPRouteConfigEntry{ Kind: structs.HTTPRoute, @@ -2076,6 +2308,11 @@ func TestAPIGatewayController(t *testing.T) { Name: "gateway", EnterpriseMeta: *defaultMeta, }}, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().routeAccepted(), + }, + }, }, &structs.ServiceConfigEntry{ Kind: structs.ServiceDefaults, @@ -2174,6 +2411,14 @@ func TestAPIGatewayController(t *testing.T) { Kind: structs.TCPRoute, Name: "tcp-route", Meta: acl.DefaultEnterpriseMeta(), + }, { + Kind: structs.TCPRoute, + Name: "tcp-route", + Meta: acl.DefaultEnterpriseMeta(), + }, { + Kind: structs.HTTPRoute, + Name: "http-route", + Meta: acl.DefaultEnterpriseMeta(), }, { Kind: structs.HTTPRoute, Name: "http-route", @@ -2327,6 +2572,11 @@ func TestAPIGatewayController(t *testing.T) { Name: "gateway", EnterpriseMeta: *defaultMeta, }}, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().routeAccepted(), + }, + }, }, &structs.TCPRouteConfigEntry{ Kind: structs.TCPRoute, @@ -2340,6 +2590,11 @@ func TestAPIGatewayController(t *testing.T) { Name: "gateway", EnterpriseMeta: *defaultMeta, }}, + Status: structs.Status{ + Conditions: []structs.Condition{ + newGatewayConditionGenerator().routeAccepted(), + }, + }, }, &structs.ServiceConfigEntry{ Kind: structs.ServiceDefaults, diff --git a/agent/structs/config_entry_status.go b/agent/structs/config_entry_status.go index 5485a6ccaf..85140f3e5a 100644 --- a/agent/structs/config_entry_status.go +++ b/agent/structs/config_entry_status.go @@ -50,6 +50,16 @@ type Status struct { Conditions []Condition } +func (s *Status) MatchesConditionStatus(condition Condition) bool { + for _, c := range s.Conditions { + if c.IsCondition(&condition) && + c.Status == condition.Status { + return true + } + } + return false +} + func (s Status) SameConditions(other Status) bool { if len(s.Conditions) != len(other.Conditions) { return false