Skip to content

Commit

Permalink
fix(bgp_policies): add empty DS set checking
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
aauren committed Oct 7, 2023
1 parent aeb51ba commit 367aedf
Showing 1 changed file with 90 additions and 0 deletions.
90 changes: 90 additions & 0 deletions pkg/controllers/routing/bgp_policies.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand All @@ -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{
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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
}

0 comments on commit 367aedf

Please sign in to comment.