Merge pull request #821 from Icinga/golangci-lint-init-and-fix

Introduce golangci-lint and fix findings

Author: lippserd

.github/workflows/go.yml

@@ -38,6 +38,14 @@ jobs:
         with:
           install-go: false
 
+      - uses: golangci/golangci-lint-action@v6
+        with:
+          version: latest
+          only-new-issues: true
+
+          # Enable the gosec linter w/o having to create a .golangci.yml config
+          args: -E gosec
+
   vet:
     runs-on: ubuntu-latest
     steps:

cmd/icingadb-migrate/convert.go

@@ -11,6 +11,7 @@ import (
 	"github.com/icinga/icingadb/pkg/icingadb/v1/history"
 	"github.com/jmoiron/sqlx"
 	"github.com/pkg/errors"
+	"math"
 	"strconv"
 	"strings"
 	"time"
@@ -282,6 +283,14 @@ func convertDowntimeRows(
 			cancelTime = actualEnd
 		}
 
+		// The IDO duration is of type bigint, representing seconds, while Icinga DB's FlexibleDuration is an unsigned
+		// bigint, representing milliseconds. In theory, there should be no negative value in the IDO and multiplied
+		// with factor 1,000 should not overflow anything. In theory, at least. To make sure, invalid values are capped.
+		var flexibleDuration uint64
+		if durationSec := row.Duration; durationSec >= 0 && durationSec < math.MaxUint64/1_000 {
+			flexibleDuration = uint64(durationSec) * 1_000
+		}
+
 		downtimeHistory = append(downtimeHistory, &history.DowntimeHistory{
 			DowntimeHistoryEntity: history.DowntimeHistoryEntity{DowntimeId: id},
 			HistoryTableMeta: history.HistoryTableMeta{
@@ -299,7 +308,7 @@ func convertDowntimeRows(
 			Author:             row.AuthorName,
 			Comment:            row.CommentData,
 			IsFlexible:         types.Bool{Bool: row.IsFixed == 0, Valid: true},
-			FlexibleDuration:   uint64(row.Duration) * 1000,
+			FlexibleDuration:   flexibleDuration,
 			ScheduledStartTime: scheduledStart,
 			ScheduledEndTime:   scheduledEnd,
 			StartTime:          startTime,

cmd/icingadb-migrate/main.go

@@ -2,7 +2,7 @@ package main
 
 import (
 	"context"
-	"crypto/sha1"
+	"crypto/sha1" // #nosec G505 -- used as a non-cryptographic hash function to hash IDs
 	"database/sql"
 	_ "embed"
 	"encoding/hex"

cmd/icingadb-migrate/misc.go

@@ -2,7 +2,7 @@ package main
 
 import (
 	"context"
-	"crypto/sha1"
+	"crypto/sha1" // #nosec G505 -- used as a non-cryptographic hash function to hash IDs
 	"github.com/icinga/icinga-go-library/database"
 	"github.com/icinga/icinga-go-library/objectpacker"
 	"github.com/icinga/icinga-go-library/types"
@@ -54,7 +54,7 @@ var objectTypes = map[uint8]string{1: "host", 2: "service"}
 
 // hashAny combines objectpacker.PackAny and SHA1 hashing.
 func hashAny(in interface{}) []byte {
-	hash := sha1.New()
+	hash := sha1.New() // #nosec G401 -- used as a non-cryptographic hash function to hash IDs
 	if err := objectpacker.PackAny(in, hash); err != nil {
 		panic(err)
 	}

cmd/icingadb/main.go

@@ -50,15 +50,15 @@ func run() int {
 	_ = sdnotify.Ready()
 
 	logger := logs.GetLogger()
-	defer logger.Sync()
+	defer func() { _ = logger.Sync() }()
 
 	logger.Infof("Starting Icinga DB daemon (%s)", internal.Version.Version)
 
 	db, err := cmd.Database(logs.GetChildLogger("database"))
 	if err != nil {
 		logger.Fatalf("%+v", errors.Wrap(err, "can't create database connection pool from config"))
 	}
-	defer db.Close()
+	defer func() { _ = db.Close() }()
 	{
 		logger.Infof("Connecting to database at '%s'", db.GetAddr())
 		err := db.Ping()
@@ -112,7 +112,7 @@ func run() int {
 		if err != nil {
 			logger.Fatalf("%+v", errors.Wrap(err, "can't create database connection pool from config"))
 		}
-		defer db.Close()
+		defer func() { _ = db.Close() }()
 		ha = icingadb.NewHA(ctx, db, heartbeat, logs.GetChildLogger("high-availability"))
 
 		telemetryLogger := logs.GetChildLogger("telemetry")
@@ -125,7 +125,7 @@ func run() int {
 		// Give up after 3s, not 5m (default) not to hang for 5m if DB is down.
 		ctx, cancelCtx := context.WithTimeout(context.Background(), 3*time.Second)
 
-		ha.Close(ctx)
+		_ = ha.Close(ctx)
 		cancelCtx()
 	}()
 	s := icingadb.NewSync(db, rc, logs.GetChildLogger("config-sync"))

pkg/icingadb/cleanup.go

@@ -32,7 +32,7 @@ func (stmt *CleanupStmt) CleanupOlderThan(
 	defer db.Log(ctx, q, &counter).Stop()
 
 	for {
-		var rowsDeleted int64
+		var rowsDeleted uint64
 
 		err := retry.WithBackoff(
 			ctx,
@@ -45,7 +45,10 @@ func (stmt *CleanupStmt) CleanupOlderThan(
 					return database.CantPerformQuery(err, q)
 				}
 
-				rowsDeleted, err = rs.RowsAffected()
+				i, err := rs.RowsAffected()
+				if err == nil && i >= 0 {
+					rowsDeleted = uint64(i)
+				}
 
 				return err
 			},
@@ -57,15 +60,15 @@ func (stmt *CleanupStmt) CleanupOlderThan(
 			return 0, err
 		}
 
-		counter.Add(uint64(rowsDeleted))
+		counter.Add(rowsDeleted)
 
 		for _, onSuccess := range onSuccess {
 			if err := onSuccess(ctx, make([]struct{}, rowsDeleted)); err != nil {
 				return 0, err
 			}
 		}
 
-		if rowsDeleted < int64(count) {
+		if rowsDeleted < count {
 			break
 		}
 	}

pkg/icingadb/delta_test.go

@@ -208,14 +208,14 @@ func testDeltaVerifyResult(t *testing.T, name string, expected map[uint64]uint64
 }
 
 func BenchmarkDelta(b *testing.B) {
-	for n := 1 << 10; n <= 1<<20; n <<= 1 {
-		b.Run(strconv.Itoa(n), func(b *testing.B) {
+	for n := uint64(1 << 10); n <= 1<<20; n <<= 1 {
+		b.Run(strconv.FormatUint(n, 10), func(b *testing.B) {
 			benchmarkDelta(b, n)
 		})
 	}
 }
 
-func benchmarkDelta(b *testing.B, numEntities int) {
+func benchmarkDelta(b *testing.B, numEntities uint64) {
 	chActual := make([]chan database.Entity, b.N)
 	chDesired := make([]chan database.Entity, b.N)
 	for i := 0; i < b.N; i++ {
@@ -231,20 +231,20 @@ func benchmarkDelta(b *testing.B, numEntities int) {
 		binary.BigEndian.PutUint64(e.PropertiesChecksum, checksum)
 		return e
 	}
-	for i := 0; i < numEntities; i++ {
+	for i := uint64(0); i < numEntities; i++ {
 		// each iteration writes exactly one entity to each channel
 		var eActual, eDesired database.Entity
 		switch i % 3 {
 		case 0: // distinct IDs
-			eActual = makeEndpoint(1, uint64(i), uint64(i))
-			eDesired = makeEndpoint(2, uint64(i), uint64(i))
+			eActual = makeEndpoint(1, i, i)
+			eDesired = makeEndpoint(2, i, i)
 		case 1: // same ID, same checksum
-			e := makeEndpoint(3, uint64(i), uint64(i))
+			e := makeEndpoint(3, i, i)
 			eActual = e
 			eDesired = e
 		case 2: // same ID, different checksum
-			eActual = makeEndpoint(4, uint64(i), uint64(i))
-			eDesired = makeEndpoint(4, uint64(i), uint64(i+1))
+			eActual = makeEndpoint(4, i, i)
+			eDesired = makeEndpoint(4, i, i+1)
 		}
 		for _, ch := range chActual {
 			ch <- eActual

pkg/icingadb/types/notification_type.go

@@ -3,7 +3,6 @@ package types
 import (
 	"database/sql/driver"
 	"encoding"
-	"github.com/icinga/icinga-go-library/types"
 	"github.com/pkg/errors"
 	"strconv"
 )
@@ -15,17 +14,12 @@ type NotificationType uint16
 func (nt *NotificationType) UnmarshalText(text []byte) error {
 	s := string(text)
 
-	i, err := strconv.ParseUint(s, 10, 64)
+	i, err := strconv.ParseUint(s, 10, 16)
 	if err != nil {
-		return types.CantParseUint64(err, s)
+		return errors.Wrapf(err, "can't parse %q into uint16", s)
 	}
 
 	n := NotificationType(i)
-	if uint64(n) != i {
-		// Truncated due to above cast, obviously too high
-		return badNotificationType(s)
-	}
-
 	if _, ok := notificationTypes[n]; !ok {
 		return badNotificationType(s)
 	}