fix: add PR review suggestions

pkg/bgp/id.go

@@ -6,8 +6,8 @@ import (
  "fmt"
  "hash/fnv"
  "net"
- "regexp"
  "strconv"
+ "strings"
 
  "github.com/cloudnativelabs/kube-router/v2/pkg/utils"
  gobgp "github.com/osrg/gobgp/v3/pkg/packet/bgp"
@@ -49,11 +49,10 @@ func ValidateCommunity(arg string) error {
  return nil
  }
 
- _regexpCommunity := regexp.MustCompile(`(\d+):(\d+)`)
- elems := _regexpCommunity.FindStringSubmatch(arg)
- if len(elems) == 3 {
- if _, err := strconv.ParseUint(elems[1], 10, CommunityMaxPartSize); err == nil {
- if _, err = strconv.ParseUint(elems[2], 10, CommunityMaxPartSize); err == nil {
+ elem1, elem2, found := strings.Cut(arg, ":")
+ if found {
+ if _, err := strconv.ParseUint(elem1, 10, CommunityMaxPartSize); err == nil {
+ if _, err = strconv.ParseUint(elem2, 10, CommunityMaxPartSize); err == nil {
  return nil
  }
  }

pkg/controllers/routing/network_routes_controller.go

@@ -69,6 +69,20 @@ const (
  ipv4MaskMinBits = 32
 )
 
+// RouteSyncer is an interface that defines the methods needed to sync routes to the kernel's routing table
+type RouteSyncer interface {
+ AddInjectedRoute(dst *net.IPNet, route *netlink.Route)
+ DelInjectedRoute(dst *net.IPNet)
+ Run(stopCh <-chan struct{}, wg *sync.WaitGroup)
+ SyncLocalRouteTable()
+}
+
+// PolicyBasedRouting is an interface that defines the methods needed to enable/disable policy based routing
+type PolicyBasedRouter interface {
+ Enable() error
+ Disable() error
+}
+
 // NetworkRoutingController is struct to hold necessary information required by controller
 type NetworkRoutingController struct {
  krNode utils.NodeAware
@@ -124,8 +138,8 @@ type NetworkRoutingController struct {
  podIPv6CIDRs []string
  CNIFirewallSetup *sync.Cond
  ipsetMutex *sync.Mutex
- routeSyncer routes.RouteSyncer
- pbr routes.PBRer
+ routeSyncer RouteSyncer
+ pbr PolicyBasedRouter
  tunneler tunnels.Tunneler
 
  nodeLister cache.Indexer
@@ -165,13 +179,13 @@ func (nrc *NetworkRoutingController) Run(healthChan chan<- *healthcheck.Controll
 
  nrc.CNIFirewallSetup.Broadcast()
 
- nrc.pbr = routes.NewPBR(nrc.krNode, nrc.podIPv4CIDRs, nrc.podIPv6CIDRs)
+ nrc.pbr = routes.NewPolicyBasedRules(nrc.krNode, nrc.podIPv4CIDRs, nrc.podIPv6CIDRs)
 
  // Handle ipip tunnel overlay
  if nrc.enableOverlays {
  klog.V(1).Info("Tunnel Overlay enabled in configuration.")
  klog.V(1).Info("Setting up overlay networking.")
- err = nrc.pbr.EnablePolicyBasedRouting()
+ err = nrc.pbr.Enable()
  if err != nil {
  klog.Errorf("Failed to enable required policy based routing: %s", err.Error())
  }
@@ -187,7 +201,7 @@ func (nrc *NetworkRoutingController) Run(healthChan chan<- *healthcheck.Controll
  } else {
  klog.V(1).Info("Tunnel Overlay disabled in configuration.")
  klog.V(1).Info("Cleaning up old overlay networking if needed.")
- err = nrc.pbr.DisablePolicyBasedRouting()
+ err = nrc.pbr.Disable()
  if err != nil {
  klog.Errorf("Failed to disable policy based routing: %s", err.Error())
  }
@@ -1365,8 +1379,8 @@ func NewNetworkRoutingController(clientset kubernetes.Interface,
  nrc.autoMTU = kubeRouterConfig.AutoMTU
  nrc.enableOverlays = kubeRouterConfig.EnableOverlay
  nrc.overlayType = kubeRouterConfig.OverlayType
- overlayEncap, err := tunnels.ParseEncapType(kubeRouterConfig.OverlayEncap)
- if err != nil {
+ overlayEncap, ok := tunnels.ParseEncapType(kubeRouterConfig.OverlayEncap)
+ if !ok {
  return nil, fmt.Errorf("unknown --overlay-encap option '%s' selected, unable to continue", overlayEncap)
  }
  overlayEncapPort, err := tunnels.ParseEncapPort(kubeRouterConfig.OverlayEncapPort)

pkg/routes/pbr.go

@@ -15,20 +15,16 @@ const (
  CustomTableName = "kube-router"
 )
 
-type PBR struct {
+// PolicyBasedRules is a struct that holds all of the information needed for manipulating policy based routing rules
+type PolicyBasedRules struct {
  nfa utils.NodeFamilyAware
  podIPv4CIDRs []string
  podIPv6CIDRs []string
 }
 
-type PBRer interface {
- EnablePolicyBasedRouting() error
- DisablePolicyBasedRouting() error
-}
-
-// NewPBR creates a new PBR object which will be used to manipulate policy based routing rules
-func NewPBR(nfa utils.NodeFamilyAware, podIPv4CIDRs, podIPv6CIDRs []string) *PBR {
- return &PBR{
+// NewPolicyBasedRules creates a new PBR object which will be used to manipulate policy based routing rules
+func NewPolicyBasedRules(nfa utils.NodeFamilyAware, podIPv4CIDRs, podIPv6CIDRs []string) *PolicyBasedRules {
+ return &PolicyBasedRules{
  nfa: nfa,
  podIPv4CIDRs: podIPv4CIDRs,
  podIPv6CIDRs: podIPv6CIDRs,
@@ -60,9 +56,9 @@ func ipRuleAbstraction(ipProtocol, ipOp, cidr string) error {
  return nil
 }
 
-// setup a custom routing table that will be used for policy based routing to ensure traffic originating
-// on tunnel interface only leaves through tunnel interface irrespective rp_filter enabled/disabled
-func (pbr *PBR) EnablePolicyBasedRouting() error {
+// Enable setup a custom routing table that will be used for policy based routing to ensure traffic
+// originating on tunnel interface only leaves through tunnel interface irrespective rp_filter enabled/disabled
+func (pbr *PolicyBasedRules) Enable() error {
  err := utils.RouteTableAdd(CustomTableID, CustomTableName)
  if err != nil {
  return fmt.Errorf("failed to update rt_tables file: %s", err)
@@ -86,7 +82,8 @@ func (pbr *PBR) EnablePolicyBasedRouting() error {
  return nil
 }
 
-func (pbr *PBR) DisablePolicyBasedRouting() error {
+// Disable removes the custom routing table that was used for policy based routing
+func (pbr *PolicyBasedRules) Disable() error {
  err := utils.RouteTableAdd(CustomTableID, CustomTableName)
  if err != nil {
  return fmt.Errorf("failed to update rt_tables file: %s", err)

pkg/routes/route_sync.go

@@ -9,14 +9,6 @@ import (
  "k8s.io/klog/v2"
 )
 
-// RouteSyncer is an interface that defines the methods needed to sync routes to the kernel's routing table
-type RouteSyncer interface {
- AddInjectedRoute(dst *net.IPNet, route *netlink.Route)
- DelInjectedRoute(dst *net.IPNet)
- Run(stopCh <-chan struct{}, wg *sync.WaitGroup)
- SyncLocalRouteTable()
-}
-
 // RouteSync is a struct that holds all of the information needed for syncing routes to the kernel's routing table
 type RouteSync struct {
  routeTableStateMap map[string]*netlink.Route

pkg/tunnels/linux_tunnels.go

@@ -6,6 +6,7 @@ import (
  "fmt"
  "net"
  "os/exec"
+ "slices"
  "strconv"
  "strings"
 
@@ -39,17 +40,6 @@ var (
 // EncapType represents the type of encapsulation used for an overlay tunnel in kube-router.
 type EncapType string
 
-// IsValid checks if the encapsulation type is valid by comparing it against a list of valid types.
-// It returns true if the encapsulation type is valid, otherwise it returns false.
-func (e EncapType) IsValid() bool {
- for _, validType := range validEncapTypes {
- if e == validType {
- return true
- }
- }
- return false
-}
-
 // ParseEncapType parses the given string and returns an Encap type if valid.
 // It returns an error if the encapsulation type is invalid.
 //
@@ -58,13 +48,13 @@ func (e EncapType) IsValid() bool {
 //
 // Returns:
 // - Encap: The parsed encapsulation type.
-// - error: An error if the encapsulation type is invalid.
-func ParseEncapType(encapType string) (EncapType, error) {
+// - bool: A boolean indicating whether the encapsulation type is valid.
+func ParseEncapType(encapType string) (EncapType, bool) {
  encap := EncapType(encapType)
- if !encap.IsValid() {
- return "", fmt.Errorf("invalid encapsulation type: %s", encapType)
+ if !slices.Contains(validEncapTypes, encap) {
+ return "", false
  }
- return encap, nil
+ return encap, true
 }
 
 type EncapPort uint16
@@ -92,26 +82,25 @@ type Tunneler interface {
 }
 
 type OverlayTunnel struct {
- krNode  utils.NodeIPAndFamilyAware
- overlayEncapPort EncapPort
- overlayEncap  EncapType
+ krNode utils.NodeIPAware
+ encapPort EncapPort
+ encapType EncapType
 }
 
-func NewOverlayTunnel(krNode utils.NodeIPAndFamilyAware, overlayEncap EncapType,
- overlayEncapPort EncapPort) *OverlayTunnel {
+func NewOverlayTunnel(krNode utils.NodeIPAware, encapType EncapType, encapPort EncapPort) *OverlayTunnel {
  return &OverlayTunnel{
- krNode:  krNode,
- overlayEncapPort: overlayEncapPort,
- overlayEncap: overlayEncap,
+ krNode: krNode,
+ encapPort: encapPort,
+ encapType: encapType,
  }
 }
 
 func (o *OverlayTunnel) EncapType() EncapType {
- return o.overlayEncap
+ return o.encapType
 }
 
 func (o *OverlayTunnel) EncapPort() EncapPort {
- return o.overlayEncapPort
+ return o.encapPort
 }
 
 // setupOverlayTunnel attempts to create a tunnel link and corresponding routes for IPIP based overlay networks
@@ -124,7 +113,7 @@ func (o *OverlayTunnel) SetupOverlayTunnel(tunnelName string, nextHop net.IP,
  var ipipMode, fouLinkType string
  isIPv6 := false
  ipBase := make([]string, 0)
- strFormattedEncapPort := strconv.FormatInt(int64(o.overlayEncapPort), 10)
+ strFormattedEncapPort := strconv.FormatInt(int64(o.encapPort), 10)
 
  if nextHop.To4() != nil {
  bestIPForFamily = o.krNode.FindBestIPv4NodeAddress()
@@ -151,7 +140,7 @@ func (o *OverlayTunnel) SetupOverlayTunnel(tunnelName string, nextHop net.IP,
  klog.V(1).Infof("Tunnel interface: %s with encap type %s for the node %s already exists.",
  tunnelName, link.Attrs().EncapType, nextHop.String())
 
- switch o.overlayEncap {
+ switch o.encapType {
  case EncapTypeIPIP:
  if linkFOUEnabled(tunnelName) {
  klog.Infof("Was configured to use ipip tunnels, but found existing fou tunnels in place, cleaning up")
@@ -162,7 +151,7 @@ func (o *OverlayTunnel) SetupOverlayTunnel(tunnelName string, nextHop net.IP,
  CleanupTunnel(nextHopSubnet, tunnelName)
 
  // If we are transitioning from FoU to IPIP we also need to clean up the old FoU port if it exists
- if fouPortAndProtoExist(o.overlayEncapPort, isIPv6) {
+ if fouPortAndProtoExist(o.encapPort, isIPv6) {
  fouArgs := ipBase
  fouArgs = append(fouArgs, "fou", "del", "port", strFormattedEncapPort)
  out, err := exec.Command("ip", fouArgs...).CombinedOutput()
@@ -188,17 +177,17 @@ func (o *OverlayTunnel) SetupOverlayTunnel(tunnelName string, nextHop net.IP,
  // nothing to do here
  if err != nil || recreate {
  klog.Infof("Creating tunnel %s of type %s with encap %s for destination %s",
- tunnelName, fouLinkType, o.overlayEncap, nextHop.String())
+ tunnelName, fouLinkType, o.encapType, nextHop.String())
  cmdArgs := ipBase
- switch o.overlayEncap {
+ switch o.encapType {
  case EncapTypeIPIP:
  // Plain IPIP tunnel without any encapsulation
  cmdArgs = append(cmdArgs, "tunnel", "add", tunnelName, "mode", ipipMode, "local", bestIPForFamily.String(),
  "remote", nextHop.String())
 
  case EncapTypeFOU:
  // Ensure that the FOU tunnel port is set correctly
- if !fouPortAndProtoExist(o.overlayEncapPort, isIPv6) {
+ if !fouPortAndProtoExist(o.encapPort, isIPv6) {
  fouArgs := ipBase
  fouArgs = append(fouArgs, "fou", "add", "port", strFormattedEncapPort, "gue")
  out, err := exec.Command("ip", fouArgs...).CombinedOutput()
@@ -216,7 +205,7 @@ func (o *OverlayTunnel) SetupOverlayTunnel(tunnelName string, nextHop net.IP,
 
  default:
  return nil, fmt.Errorf("unknown tunnel encapsulation was passed: %s, unable to continue with overlay "+
- "setup", o.overlayEncap)
+ "setup", o.encapType)
  }
 
  klog.V(2).Infof("Executing the following command to create tunnel: ip %s", cmdArgs)
@@ -276,6 +265,10 @@ func CleanupTunnel(destinationSubnet *net.IPNet, tunnelName string) {
 // Since linux restricts interface names to 15 characters, we take the sha-256 of the node IP after removing
 // non-entropic characters like '.' and ':', and then use the first 12 bytes of it. This allows us to cater to both
 // long IPv4 addresses and much longer IPv6 addresses.
+//
+// TODO: In the future, we should consider using the hexadecimal byte representation of IPv4 addresses and using a the
+// SHA256 of the hash. Additionally, we should not remove non-entropic characters as it can cause hash collisions as
+// "21.3.0.4" would has the same as "2.13.0.4" without "."'s.
 func GenerateTunnelName(nodeIP string) string {
  // remove dots from an IPv4 address
  strippedIP := strings.ReplaceAll(nodeIP, ".", "")