From 399175b6e67d3def39ba5b1302c228651c541112 Mon Sep 17 00:00:00 2001
From: Aaron U'Ren <aauren@gmail.com>
Date: Tue, 3 Jan 2023 16:48:13 -0600
Subject: [PATCH] fix(bgp_policies): add empty DS set checking

Without this logic, it appears that sometimes GoBGP is inclined to match
unintentional routes in policy because of the MATCHSET_ANY declaration
and the way that it interacts with empty sets.

In my testing, without this logic I found that it often resulted in
various routes not being advertised correctly and not even showing up in
GoBGP itself. My current guess is that policy keeps GoBGP from importing
the route into the RIB even from the Protobuf socket connection that
kube-router establishes directly.
---
 pkg/controllers/routing/bgp_policies.go | 90 +++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/pkg/controllers/routing/bgp_policies.go b/pkg/controllers/routing/bgp_policies.go
index a6596e93ea..e4d5196103 100644
--- a/pkg/controllers/routing/bgp_policies.go
+++ b/pkg/controllers/routing/bgp_policies.go
@@ -604,6 +604,17 @@ func (nrc *NetworkRoutingController) addExportPolicies() error {
 
 		// statement to represent the export policy to permit advertising node's IPv4 & IPv6 pod CIDRs
 		for _, podSet := range []string{podCIDRSet, podCIDRSetV6} {
+			podSetEmpty, err := nrc.emptyCheckDefinedSets([]gobgpapi.ListDefinedSetRequest{
+				{DefinedType: gobgpapi.DefinedType_PREFIX, Name: podSet},
+			})
+			if err != nil {
+				return err
+			}
+			// if the set is empty, then skip it, so we don't have unintentional matches
+			if podSetEmpty {
+				continue
+			}
+
 			statements = append(statements,
 				&gobgpapi.Statement{
 					Conditions: &gobgpapi.Conditions{
@@ -637,9 +648,31 @@ func (nrc *NetworkRoutingController) addExportPolicies() error {
 		}
 
 		for _, peerSet := range []string{externalPeerSet, externalPeerSetV6} {
+			peerSetEmpty, err := nrc.emptyCheckDefinedSets([]gobgpapi.ListDefinedSetRequest{
+				{DefinedType: gobgpapi.DefinedType_NEIGHBOR, Name: peerSet},
+			})
+			if err != nil {
+				return err
+			}
+			// if the set is empty, then skip it, so we don't have unintentional matches
+			if peerSetEmpty {
+				continue
+			}
+
 			for _, serviceVIPSet := range []string{serviceVIPsSet, serviceVIPsSetV6} {
 				// statement to represent the export policy to permit advertising Service VIP's
 				// only to the global BGP peer or node specific BGP peer
+				vipSetEmpty, err := nrc.emptyCheckDefinedSets([]gobgpapi.ListDefinedSetRequest{
+					{DefinedType: gobgpapi.DefinedType_PREFIX, Name: serviceVIPSet},
+				})
+				if err != nil {
+					return err
+				}
+				// if the set is empty, then skip it, so we don't have unintentional matches
+				if vipSetEmpty {
+					continue
+				}
+
 				statements = append(statements, &gobgpapi.Statement{
 					Conditions: &gobgpapi.Conditions{
 						PrefixSet: &gobgpapi.MatchSet{
@@ -659,6 +692,17 @@ func (nrc *NetworkRoutingController) addExportPolicies() error {
 				for _, podSet := range []string{podCIDRSet, podCIDRSetV6} {
 					// if we are configured to advertise POD CIDRs then add export policies for all of our IPv4 and IPv6
 					// peers for all IPv4 and IPv6 POD CIDRs
+					podCIDREmpty, err := nrc.emptyCheckDefinedSets([]gobgpapi.ListDefinedSetRequest{
+						{DefinedType: gobgpapi.DefinedType_PREFIX, Name: podSet},
+					})
+					if err != nil {
+						return err
+					}
+					// if the set is empty, then skip it, so we don't have unintentional matches
+					if podCIDREmpty {
+						continue
+					}
+
 					statements = append(statements, &gobgpapi.Statement{
 						Conditions: &gobgpapi.Conditions{
 							PrefixSet: &gobgpapi.MatchSet{
@@ -742,7 +786,29 @@ func (nrc *NetworkRoutingController) addImportPolicies() error {
 		RouteAction: gobgpapi.RouteAction_REJECT,
 	}
 	for _, peerSet := range []string{allPeerSet, allPeerSetV6} {
+		anyEmpty, err := nrc.emptyCheckDefinedSets([]gobgpapi.ListDefinedSetRequest{
+			{DefinedType: gobgpapi.DefinedType_NEIGHBOR, Name: peerSet},
+		})
+		if err != nil {
+			return err
+		}
+		// If set is empty, skip it so that we don't accidentally match VIPs against a blank set
+		if anyEmpty {
+			continue
+		}
 		for _, vipSet := range []string{serviceVIPsSet, serviceVIPsSetV6} {
+			vipSetEmpty, err := nrc.emptyCheckDefinedSets([]gobgpapi.ListDefinedSetRequest{
+				{DefinedType: gobgpapi.DefinedType_NEIGHBOR, Name: peerSet},
+				{DefinedType: gobgpapi.DefinedType_PREFIX, Name: vipSet},
+			})
+			if err != nil {
+				return err
+			}
+			// If either set is empty, skip it so that we don't accidentally match VIPs against a blank set
+			if vipSetEmpty {
+				continue
+			}
+
 			statements = append(statements, &gobgpapi.Statement{
 				Conditions: &gobgpapi.Conditions{
 					PrefixSet: &gobgpapi.MatchSet{
@@ -856,3 +922,27 @@ func (nrc *NetworkRoutingController) getDefinedSetFromGoBGP(name string,
 		})
 	return
 }
+
+// emptyCheckDefinedSets Query requested sets from GoBGP to ensure that any of them aren't empty. Referencing an empty
+// set in policy with MatchSet_ANY will cause it to match all items incorrectly and cause us to stop announcing
+// important BGP paths, so we take extra precaution that we don't do this.
+func (nrc *NetworkRoutingController) emptyCheckDefinedSets(defSets []gobgpapi.ListDefinedSetRequest) (bool, error) {
+	for idx := range defSets {
+		ds, err := nrc.getDefinedSetFromGoBGP(defSets[idx].Name, defSets[idx].DefinedType)
+		if err != nil {
+			return false, err
+		}
+		//nolint:exhaustive // We have a default here we don't need to exhaustively list all possible gobgpapi types
+		switch defSets[idx].DefinedType {
+		case gobgpapi.DefinedType_PREFIX:
+			if len(ds.Prefixes) < 1 {
+				return true, nil
+			}
+		default:
+			if len(ds.List) < 1 {
+				return true, nil
+			}
+		}
+	}
+	return false, nil
+}