From 45728ff9dcc238ab35b73508c9a524c86d8752d5 Mon Sep 17 00:00:00 2001 From: Stephen Kitt Date: Fri, 30 Aug 2024 10:53:44 +0200 Subject: [PATCH] Rework integer type selection Following the recent gosec-related int work (to annotate known-safe conversions), this furthers that by: * using ints instead of uints for healthcheck configuration and tracking * using uint32 instead of int for IP pool sizes, since their use is tied to uint32 representations of IP addresses * using fmt.Sprintf instead of manual string concatenation to construct the connectivity verification command Signed-off-by: Stephen Kitt --- .../healthchecker/healthchecker.go | 6 ++-- pkg/cableengine/healthchecker/pinger.go | 8 ++--- pkg/ipam/ippool.go | 29 ++++++++++--------- pkg/types/types.go | 4 +-- test/e2e/dataplane/tcp_gn_pod_connectivity.go | 4 +-- test/e2e/framework/dataplane.go | 16 +++++----- test/external/dataplane/gn_connectivity.go | 2 +- 7 files changed, 36 insertions(+), 33 deletions(-) diff --git a/pkg/cableengine/healthchecker/healthchecker.go b/pkg/cableengine/healthchecker/healthchecker.go index 03f3adeaf9..ebe6e3b119 100644 --- a/pkg/cableengine/healthchecker/healthchecker.go +++ b/pkg/cableengine/healthchecker/healthchecker.go @@ -55,8 +55,8 @@ type Config struct { WatcherConfig *watcher.Config EndpointNamespace string ClusterID string - PingInterval uint - MaxPacketLossCount uint + PingInterval int + MaxPacketLossCount int NewPinger func(PingerConfig) PingerInterface } @@ -161,7 +161,7 @@ func (h *controller) endpointCreatedOrUpdated(obj runtime.Object, _ int) bool { } if h.config.PingInterval != 0 { - pingerConfig.Interval = time.Second * time.Duration(h.config.PingInterval) //nolint:gosec // We can safely ignore integer conversion error + pingerConfig.Interval = time.Second * time.Duration(h.config.PingInterval) } newPingerFunc := h.config.NewPinger diff --git a/pkg/cableengine/healthchecker/pinger.go b/pkg/cableengine/healthchecker/pinger.go index 8f2d44336b..ef2675d45d 100644 --- a/pkg/cableengine/healthchecker/pinger.go +++ b/pkg/cableengine/healthchecker/pinger.go @@ -34,7 +34,7 @@ const ( ) var ( - defaultMaxPacketLossCount uint = 5 + defaultMaxPacketLossCount = 5 defaultPingInterval = 1 * time.Second @@ -59,7 +59,7 @@ type PingerConfig struct { IP string Interval time.Duration Timeout time.Duration - MaxPacketLossCount uint + MaxPacketLossCount int } type pingerInfo struct { @@ -67,7 +67,7 @@ type pingerInfo struct { ip string pingInterval time.Duration pingTimeout time.Duration - maxPacketLossCount uint + maxPacketLossCount int statistics statistics failureMsg string connectionStatus ConnectionStatus @@ -153,7 +153,7 @@ func (p *pingerInfo) doPing() error { } // Pinger will mark a connection as an error if the packet loss reaches the threshold - if uint(pinger.PacketsSent-pinger.PacketsRecv) > p.maxPacketLossCount { + if pinger.PacketsSent-pinger.PacketsRecv > p.maxPacketLossCount { p.Lock() defer p.Unlock() diff --git a/pkg/ipam/ippool.go b/pkg/ipam/ippool.go index 6fc0fbbdb2..8b7077aef9 100644 --- a/pkg/ipam/ippool.go +++ b/pkg/ipam/ippool.go @@ -26,6 +26,7 @@ import ( "sync" "github.com/emirpasic/gods/maps/treemap" + "github.com/emirpasic/gods/utils" "github.com/pkg/errors" "github.com/submariner-io/submariner/pkg/globalnet/metrics" ) @@ -33,7 +34,7 @@ import ( type IPPool struct { cidr string network *net.IPNet - size int + size uint32 available *treemap.Map // int IP is the key, string IP is the value mutex sync.RWMutex } @@ -46,7 +47,7 @@ func NewIPPool(cidr string) (*IPPool, error) { ones, totalbits := network.Mask.Size() - size := int(math.Exp2(float64(totalbits-ones))) - 2 // don't count net and broadcast + size := uint32(math.Exp2(float64(totalbits-ones))) - 2 // don't count net and broadcast if size < 2 { return nil, fmt.Errorf("invalid prefix for CIDR %q", cidr) } @@ -55,39 +56,39 @@ func NewIPPool(cidr string) (*IPPool, error) { cidr: cidr, network: network, size: size, - available: treemap.NewWithIntComparator(), + available: treemap.NewWith(utils.UInt32Comparator), } startingIP := ipToInt(pool.network.IP) + 1 - for i := 0; i < pool.size; i++ { + for i := uint32(0); i < pool.size; i++ { intIP := startingIP + i ip := intToIP(intIP).String() pool.available.Put(intIP, ip) } - metrics.RecordAvailability(cidr, size) + metrics.RecordAvailability(cidr, int(size)) return pool, nil } -func ipToInt(ip net.IP) int { +func ipToInt(ip net.IP) uint32 { intIP := ip if len(ip) == 16 { intIP = ip[12:16] } - return int(binary.BigEndian.Uint32(intIP)) + return binary.BigEndian.Uint32(intIP) } -func intToIP(ip int) net.IP { +func intToIP(ip uint32) net.IP { netIP := make(net.IP, 4) - binary.BigEndian.PutUint32(netIP, uint32(ip)) //nolint:gosec // int -> uint32 conversion won't overflow here + binary.BigEndian.PutUint32(netIP, ip) return netIP } -func StringIPToInt(stringIP string) int { +func StringIPToInt(stringIP string) uint32 { return ipToInt(net.ParseIP(stringIP)) } @@ -125,11 +126,11 @@ func (p *IPPool) Allocate(num int) ([]string, error) { retIPs := make([]string, num) - var prevIntIP, firstIntIP, current int + var prevIntIP, firstIntIP, current uint32 iter := p.available.Iterator() for iter.Next() { - intIP := iter.Key().(int) + intIP := iter.Key().(uint32) retIPs[current] = iter.Value().(string) if current == 0 || prevIntIP+1 != intIP { @@ -144,7 +145,7 @@ func (p *IPPool) Allocate(num int) ([]string, error) { prevIntIP = intIP current++ - if current == num { + if int(current) == num { for i := 0; i < num; i++ { p.available.Remove(firstIntIP) @@ -183,7 +184,7 @@ func (p *IPPool) Reserve(ips ...string) error { return nil } - intIPs := make([]int, num) + intIPs := make([]uint32, num) p.mutex.Lock() defer p.mutex.Unlock() diff --git a/pkg/types/types.go b/pkg/types/types.go index e376a645ec..6f5c8f3c9e 100644 --- a/pkg/types/types.go +++ b/pkg/types/types.go @@ -46,7 +46,7 @@ type SubmarinerSpecification struct { HealthCheckEnabled bool `default:"true"` Uninstall bool HaltOnCertError bool `split_words:"true"` - HealthCheckInterval uint - HealthCheckMaxPacketLossCount uint + HealthCheckInterval int + HealthCheckMaxPacketLossCount int MetricsPort int `default:"32780"` } diff --git a/test/e2e/dataplane/tcp_gn_pod_connectivity.go b/test/e2e/dataplane/tcp_gn_pod_connectivity.go index acc676ebcb..45971eadb7 100644 --- a/test/e2e/dataplane/tcp_gn_pod_connectivity.go +++ b/test/e2e/dataplane/tcp_gn_pod_connectivity.go @@ -98,8 +98,8 @@ var _ = Describe("Basic TCP connectivity tests across overlapping clusters witho err := util.Update(context.Background(), resource.ForService(framework.KubeClients[lpConfig.Cluster], f.Namespace), service, func(existing *v1.Service) (*v1.Service, error) { - existing.Spec.Ports[0].Port = lpConfig.Port - existing.Spec.Ports[0].TargetPort = intstr.FromInt32(lpConfig.Port) + existing.Spec.Ports[0].Port = int32(lpConfig.Port) + existing.Spec.Ports[0].TargetPort = intstr.FromInt(lpConfig.Port) return existing, nil }) Expect(err).To(Succeed()) diff --git a/test/e2e/framework/dataplane.go b/test/e2e/framework/dataplane.go index d6ac4283d9..a28927a795 100644 --- a/test/e2e/framework/dataplane.go +++ b/test/e2e/framework/dataplane.go @@ -21,7 +21,6 @@ package framework import ( "context" "fmt" - "strconv" . "github.com/onsi/gomega" resourceUtil "github.com/submariner-io/admiral/pkg/resource" @@ -157,12 +156,15 @@ func verifyGlobalnetDatapathConnectivity(p tcp.ConnectivityTestParams, gn Global } verifyConnectivity := func(listener *framework.NetworkPod, connector *framework.NetworkPod) { - cmd := []string{"sh", "-c", "for j in $(seq 1 " + strconv.FormatUint(uint64(connector.Config.NumOfDataBufs), 10) + "); do echo" + - " [dataplane] connector says " + connector.Config.Data + "; done" + - " | for i in $(seq " + strconv.FormatUint(uint64(listener.Config.ConnectionAttempts), 10) + ");" + - " do if nc -v " + remoteIP + " " + strconv.FormatUint(uint64(connector.Config.Port), 10) + " -w " + - strconv.FormatUint(uint64(listener.Config.ConnectionTimeout), 10) + ";" + - " then break; else sleep " + strconv.FormatUint(uint64(listener.Config.ConnectionTimeout/2), 10) + "; fi; done"} + cmd := []string{ + "sh", + "-c", + fmt.Sprintf("for j in $(seq 1 %d); do echo [dataplane] connector says %s; done"+ + " | for i in $(seq %d); do if nc -v %s %d -w %d; then break; else sleep %d; fi; done", + connector.Config.NumOfDataBufs, connector.Config.Data, listener.Config.ConnectionAttempts, remoteIP, connector.Config.Port, + listener.Config.ConnectionTimeout, listener.Config.ConnectionTimeout/2, + ), + } stdOut, _, err := p.Framework.ExecWithOptions(context.TODO(), &framework.ExecOptions{ Command: cmd, diff --git a/test/external/dataplane/gn_connectivity.go b/test/external/dataplane/gn_connectivity.go index 41e27aaae5..dc3e54e8af 100644 --- a/test/external/dataplane/gn_connectivity.go +++ b/test/external/dataplane/gn_connectivity.go @@ -513,7 +513,7 @@ func getPodGlobalIPs(p testParams, g globalnetTestParams, np *framework.NetworkP func createHeadlessTCPServiceWithoutSelector(f *framework.Framework, cluster framework.ClusterIndex, svcName, portName string, port int32, ) *v1.Service { - serviceSpec := f.NewService(svcName, portName, port, v1.ProtocolTCP, nil, true) + serviceSpec := f.NewService(svcName, portName, int(port), v1.ProtocolTCP, nil, true) sc := framework.KubeClients[cluster].CoreV1().Services(f.Namespace) return f.CreateService(sc, serviceSpec)