fix: add PR review suggestions

Author: aauren

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,26 @@ 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,
+func NewOverlayTunnel(krNode utils.NodeIPAware, overlayEncap EncapType,
 	overlayEncapPort EncapPort) *OverlayTunnel {
 	return &OverlayTunnel{
-		krNode:           krNode,
-		overlayEncapPort: overlayEncapPort,
-		overlayEncap:     overlayEncap,
+		krNode:    krNode,
+		encapPort: overlayEncapPort,
+		encapType: overlayEncap,
 	}
 }
 
 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 +114,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 +141,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 +152,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 +178,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 +206,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 +266,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, ".", "")