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
* using int32 for nat discovery IPPortPair

Signed-off-by: Stephen Kitt <skitt@redhat.com>

pkg/apis/submariner.io/v1/endpoint.go

@@ -63,7 +63,6 @@ func parsePort(port string) (int32, error) {
 		return -1, errors.Errorf("port %s is > 65535", port)
 	}
 
-	//nolint:gosec // We can safely ignore integer conversion error
 	return int32(portInt), nil
 }
 

pkg/cableengine/healthchecker/healthchecker.go

@@ -41,8 +41,8 @@ type Config struct {
 	WatcherConfig      *watcher.Config
 	EndpointNamespace  string
 	ClusterID          string
-	PingInterval       uint
-	MaxPacketLossCount uint
+	PingInterval       int
+	MaxPacketLossCount int
 	NewPinger          func(pinger.Config) pinger.Interface
 }
 
@@ -147,7 +147,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

pkg/natdiscovery/proto/natdiscovery.proto

@@ -67,7 +67,7 @@ message SubmarinerNATDiscoveryResponse {
 
 message IPPortPair {
   string IP = 1;
-  uint32 port = 2;
+  int32 port = 2;
 }
 
 message EndpointDetails {

pkg/natdiscovery/request_handle.go

@@ -38,7 +38,7 @@ func (nd *natDiscovery) handleRequestFromAddress(req *proto.SubmarinerNATDiscove
 		},
 		Receiver: req.Sender,
 		ReceivedSrc: &proto.IPPortPair{
-			Port: uint32(addr.Port), //nolint:gosec // We can safely ignore integer conversion error
+			Port: int32(addr.Port), //nolint:gosec // We can safely ignore integer conversion error
 			IP:   addr.IP.String(),
 		},
 	}

pkg/natdiscovery/request_handle_internal_test.go

@@ -174,11 +174,11 @@ func createMalformedRequest(mangleFunction func(*natproto.SubmarinerNATDiscovery
 		},
 		UsingSrc: &natproto.IPPortPair{
 			IP:   testRemotePrivateIP,
-			Port: uint32(natproto.DefaultPort),
+			Port: natproto.DefaultPort,
 		},
 		UsingDst: &natproto.IPPortPair{
 			IP:   testLocalPrivateIP,
-			Port: uint32(natproto.DefaultPort),
+			Port: natproto.DefaultPort,
 		},
 	}
 

pkg/natdiscovery/request_send.go

@@ -87,11 +87,11 @@ func (nd *natDiscovery) sendCheckRequestToTargetIP(remoteNAT *remoteEndpointNAT,
 		},
 		UsingSrc: &natproto.IPPortPair{
 			IP:   sourceIP,
-			Port: uint32(nd.serverPort),
+			Port: nd.serverPort,
 		},
 		UsingDst: &natproto.IPPortPair{
 			IP:   targetIP,
-			Port: uint32(targetPort),
+			Port: targetPort,
 		},
 	}
 

pkg/natdiscovery/request_send_internal_test.go

@@ -67,12 +67,12 @@ var _ = When("a request is sent", func() {
 		It("should set the using source fields correctly", func() {
 			Expect(request.UsingSrc).NotTo(BeNil())
 			Expect(request.UsingSrc.IP).To(Equal(testLocalPrivateIP))
-			Expect(request.UsingSrc.Port).To(Equal(uint32(testLocalNATPort)))
+			Expect(request.UsingSrc.Port).To(Equal(testLocalNATPort))
 		})
 
 		It("should set the using destination fields correctly", func() {
 			Expect(request.UsingDst).NotTo(BeNil())
-			Expect(request.UsingDst.Port).To(Equal(uint32(testRemoteNATPort)))
+			Expect(request.UsingDst.Port).To(Equal(testRemoteNATPort))
 			Expect(request.UsingDst.IP).To(Equal(srcIP))
 		})
 

pkg/pinger/pinger.go

@@ -51,14 +51,14 @@ type LatencyInfo struct {
 }
 
 var (
-	defaultMaxPacketLossCount uint = 5
+	defaultMaxPacketLossCount = 5
 
 	defaultPingInterval = 1 * time.Second
 
 	// The RTT will be stored and will be used to calculate the statistics until
 	// the size is reached. Once the size is reached the array will be reset and
 	// the last elements will be added to the array for statistics.
-	size uint64 = 1000
+	size int64 = 1000
 
 	// Even though we set up the pinger to run continuously, we still have to give it a non-zero timeout else it will
 	// fail so set a really long one.
@@ -78,15 +78,15 @@ type Config struct {
 	IP                 string
 	Interval           time.Duration
 	Timeout            time.Duration
-	MaxPacketLossCount uint
+	MaxPacketLossCount int
 }
 
 type pingerImpl struct {
 	sync.Mutex
 	ip                 string
 	pingInterval       time.Duration
 	pingTimeout        time.Duration
-	maxPacketLossCount uint
+	maxPacketLossCount int
 	statistics         statistics
 	failureMsg         string
 	connectionStatus   ConnectionStatus
@@ -101,7 +101,7 @@ func NewPinger(config Config) Interface {
 		maxPacketLossCount: config.MaxPacketLossCount,
 		statistics: statistics{
 			size:         size,
-			previousRtts: make([]uint64, size),
+			previousRtts: make([]int64, size),
 		},
 		stopCh: make(chan struct{}),
 	}
@@ -172,7 +172,7 @@ func (p *pingerImpl) 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()
 
@@ -198,7 +198,7 @@ func (p *pingerImpl) doPing() error {
 
 		p.connectionStatus = Connected
 		p.failureMsg = ""
-		p.statistics.update(uint64(packet.Rtt.Nanoseconds()))
+		p.statistics.update(packet.Rtt.Nanoseconds())
 
 		pinger.PacketsSent = 0
 		pinger.PacketsRecv = 0
@@ -223,8 +223,8 @@ func (p *pingerImpl) GetLatencyInfo() *LatencyInfo {
 	p.Lock()
 	defer p.Unlock()
 
-	toDurationString := func(v uint64) string {
-		return time.Duration(v).String() //nolint:gosec // We can safely ignore integer conversion error
+	toDurationString := func(v int64) string {
+		return time.Duration(v).String()
 	}
 
 	return &LatencyInfo{

pkg/pinger/statistics.go

@@ -23,19 +23,19 @@ import (
 )
 
 type statistics struct {
-	previousRtts []uint64
-	sum          uint64
-	mean         uint64
-	stdDev       uint64
-	lastRtt      uint64
-	minRtt       uint64
-	maxRtt       uint64
-	sqrDiff      uint64
-	index        uint64
-	size         uint64
+	previousRtts []int64
+	sum          int64
+	mean         int64
+	stdDev       int64
+	lastRtt      int64
+	minRtt       int64
+	maxRtt       int64
+	sqrDiff      int64
+	index        int64
+	size         int64
 }
 
-func (s *statistics) update(rtt uint64) {
+func (s *statistics) update(rtt int64) {
 	s.lastRtt = rtt
 
 	if s.index == s.size {
@@ -47,9 +47,8 @@ func (s *statistics) update(rtt uint64) {
 		s.sum = s.previousRtts[0] + s.previousRtts[1]
 		s.mean = s.sum / 2
 
-		//nolint:gosec // Ignore "integer overflow conversion uint64 -> int64" - we subtract uint64's and want to preserve sign.
-		s.sqrDiff = uint64(int64(s.previousRtts[0]-s.mean)*int64(s.previousRtts[0]-s.mean) +
-			int64(s.previousRtts[1]-s.mean)*int64(s.previousRtts[1]-s.mean))
+		s.sqrDiff = (s.previousRtts[0]-s.mean)*(s.previousRtts[0]-s.mean) +
+			(s.previousRtts[1]-s.mean)*(s.previousRtts[1]-s.mean)
 	}
 
 	if s.index+1 > 1 {
@@ -66,9 +65,8 @@ func (s *statistics) update(rtt uint64) {
 		oldMean := s.mean
 		s.mean = s.sum / (s.index + 1)
 
-		//nolint:gosec // Ignore "integer overflow conversion uint64 -> int64" - we subtract uint64's and want to preserve sign.
-		s.sqrDiff += uint64(int64(rtt-oldMean) * int64(rtt-s.mean))
-		s.stdDev = uint64(math.Sqrt(float64(s.sqrDiff / (s.index + 1))))
+		s.sqrDiff += (rtt - oldMean) * (rtt - s.mean)
+		s.stdDev = int64(math.Sqrt(float64(s.sqrDiff / (s.index + 1))))
 	} else {
 		s.sum = rtt
 		s.sqrDiff = 0

pkg/pinger/statistics_internal_test.go

@@ -25,48 +25,48 @@ import (
 
 var _ = Describe("Statistics", func() {
 	const (
-		testMinRTT     = 404351
-		testMaxRTT     = 1048263
-		testLastRTT    = 1044609
-		testNewMinRTT  = 404300
-		testNewMaxRTT  = 1048264
-		testNewLastRTT = 609555
+		testMinRTT     int64 = 404351
+		testMaxRTT     int64 = 1048263
+		testLastRTT    int64 = 1044609
+		testNewMinRTT  int64 = 404300
+		testNewMaxRTT  int64 = 1048264
+		testNewLastRTT int64 = 609555
 	)
 
 	When("update is called with a sample space", func() {
 		It("should correctly compute the statistics", func() {
 			size := 10
 			statistics := &statistics{
-				size:         uint64(size),
-				previousRtts: make([]uint64, size),
+				size:         int64(size),
+				previousRtts: make([]int64, size),
 			}
 
-			sampleSpace := [10]uint64{testMinRTT, 490406, 530333, 609556, 609650, 685106, 726265, 785707, testMaxRTT, testLastRTT}
-			expectedMean := 693424
-			expectedSD := 205994
+			sampleSpace := [10]int64{testMinRTT, 490406, 530333, 609556, 609650, 685106, 726265, 785707, testMaxRTT, testLastRTT}
+			expectedMean := int64(693424)
+			expectedSD := int64(205994)
 
 			for _, v := range sampleSpace {
 				statistics.update(v)
 			}
 
-			Expect(statistics.maxRtt).To(Equal(uint64(testMaxRTT)))
-			Expect(statistics.minRtt).To(Equal(uint64(testMinRTT)))
-			Expect(statistics.lastRtt).To(Equal(uint64(testLastRTT)))
-			Expect(statistics.mean).To(Equal(uint64(expectedMean)))
-			Expect(statistics.stdDev).To(Equal(uint64(expectedSD)))
+			Expect(statistics.maxRtt).To(Equal(testMaxRTT))
+			Expect(statistics.minRtt).To(Equal(testMinRTT))
+			Expect(statistics.lastRtt).To(Equal(testLastRTT))
+			Expect(statistics.mean).To(Equal(expectedMean))
+			Expect(statistics.stdDev).To(Equal(expectedSD))
 
 			statistics.update(testNewMinRTT)
 			statistics.update(testNewMaxRTT)
 			statistics.update(testNewLastRTT)
 
-			newExpectedMean := 830998
-			newExpectedSD := 272450
+			newExpectedMean := int64(830998)
+			newExpectedSD := int64(272450)
 
-			Expect(statistics.maxRtt).To(Equal(uint64(testNewMaxRTT)))
-			Expect(statistics.minRtt).To(Equal(uint64(testNewMinRTT)))
-			Expect(statistics.lastRtt).To(Equal(uint64(testNewLastRTT)))
-			Expect(statistics.mean).To(Equal(uint64(newExpectedMean)))
-			Expect(statistics.stdDev).To(Equal(uint64(newExpectedSD)))
+			Expect(statistics.maxRtt).To(Equal(testNewMaxRTT))
+			Expect(statistics.minRtt).To(Equal(testNewMinRTT))
+			Expect(statistics.lastRtt).To(Equal(testNewLastRTT))
+			Expect(statistics.mean).To(Equal(newExpectedMean))
+			Expect(statistics.stdDev).To(Equal(newExpectedSD))
 		})
 	})
 })

pkg/routeagent_driver/handlers/healthchecker/healthchecker.go

@@ -40,8 +40,8 @@ import (
 )
 
 type Config struct {
-	PingInterval             uint
-	MaxPacketLossCount       uint
+	PingInterval             int
+	MaxPacketLossCount       int
 	HealthCheckerEnabled     bool
 	RouteAgentUpdateInterval time.Duration
 	NewPinger                func(pinger.Config) pinger.Interface
@@ -147,7 +147,7 @@ func (h *controller) processEndpointCreatedOrUpdated(endpoint *submarinerv1.Endp
 	}
 
 	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)
 	}
 
 	if h.config.MaxPacketLossCount != 0 {

pkg/routeagent_driver/handlers/healthchecker/healthchecker_test.go

@@ -281,8 +281,7 @@ func newTestDriver() *testDriver {
 
 		config.NewPinger = func(pingerCfg pinger.Config) pinger.Interface {
 			defer GinkgoRecover()
-			Expect(pingerCfg.Interval).To(Equal(time.Second *
-				time.Duration(config.PingInterval))) //nolint:gosec // We can safely ignore integer conversion error
+			Expect(pingerCfg.Interval).To(Equal(time.Second * time.Duration(config.PingInterval)))
 			Expect(pingerCfg.MaxPacketLossCount).To(Equal(config.MaxPacketLossCount))
 
 			p, ok := t.pingerMap[pingerCfg.IP]

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"`
 }

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,