From b4518730121dab82c7435837cf0185b7af3e733c Mon Sep 17 00:00:00 2001 From: Alvar Penning Date: Fri, 4 Oct 2024 15:50:49 +0200 Subject: [PATCH] Introduce golangci-lint and fix findings First, the same GitHub Action used in the icinga-go-library was added to the Go workflow. Then, the findings were addressed. In general, these were mostly missing return check - when being unused anyway -, potential overflows for integer type conversions, and SHA1 warnings. If applicable, the code was slightly modified to perform the necessary checks. Sometimes the linter has detected the checks. Sometimes not and a linter comment was added. --- .github/workflows/go.yml | 8 ++++++++ cmd/icingadb-migrate/convert.go | 2 +- cmd/icingadb-migrate/main.go | 2 +- cmd/icingadb-migrate/misc.go | 4 ++-- cmd/icingadb/main.go | 8 ++++---- pkg/icingadb/cleanup.go | 9 +++++---- pkg/icingadb/delta_test.go | 12 +++++++----- pkg/icingadb/history/retention.go | 8 +++++++- pkg/icingadb/types/comment_type.go | 7 +++---- pkg/icingadb/types/notification_type.go | 7 +++---- 10 files changed, 41 insertions(+), 26 deletions(-) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index b0d2f0e19..b3d61a50a 100644 --- a/.github/workflows/go.yml +++ b/.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: diff --git a/cmd/icingadb-migrate/convert.go b/cmd/icingadb-migrate/convert.go index 0a875e5c1..0fc27dd0b 100644 --- a/cmd/icingadb-migrate/convert.go +++ b/cmd/icingadb-migrate/convert.go @@ -299,7 +299,7 @@ func convertDowntimeRows( Author: row.AuthorName, Comment: row.CommentData, IsFlexible: types.Bool{Bool: row.IsFixed == 0, Valid: true}, - FlexibleDuration: uint64(row.Duration) * 1000, + FlexibleDuration: uint64(row.Duration) * 1000, // #nosec G115 -- flexible_duration is unsigned in Icinga DB's schema ScheduledStartTime: scheduledStart, ScheduledEndTime: scheduledEnd, StartTime: startTime, diff --git a/cmd/icingadb-migrate/main.go b/cmd/icingadb-migrate/main.go index a0fc5644a..086d2826b 100644 --- a/cmd/icingadb-migrate/main.go +++ b/cmd/icingadb-migrate/main.go @@ -2,7 +2,7 @@ package main import ( "context" - "crypto/sha1" + "crypto/sha1" // #nosec G505 -- cryptographic weak SHA1 "database/sql" _ "embed" "encoding/hex" diff --git a/cmd/icingadb-migrate/misc.go b/cmd/icingadb-migrate/misc.go index 3d33ef8c2..538041d6a 100644 --- a/cmd/icingadb-migrate/misc.go +++ b/cmd/icingadb-migrate/misc.go @@ -2,7 +2,7 @@ package main import ( "context" - "crypto/sha1" + "crypto/sha1" // #nosec G505 -- cryptographic weak SHA1 "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 -- cryptographic weak SHA1 if err := objectpacker.PackAny(in, hash); err != nil { panic(err) } diff --git a/cmd/icingadb/main.go b/cmd/icingadb/main.go index d5fe38f1a..bd94686d7 100644 --- a/cmd/icingadb/main.go +++ b/cmd/icingadb/main.go @@ -50,7 +50,7 @@ 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) @@ -58,7 +58,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() }() { 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")) diff --git a/pkg/icingadb/cleanup.go b/pkg/icingadb/cleanup.go index 754e5a33d..07f388247 100644 --- a/pkg/icingadb/cleanup.go +++ b/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,8 @@ func (stmt *CleanupStmt) CleanupOlderThan( return database.CantPerformQuery(err, q) } - rowsDeleted, err = rs.RowsAffected() + i, err := rs.RowsAffected() + rowsDeleted = uint64(i) // #nosec G115 -- negative amount of rows should not be possible return err }, @@ -57,7 +58,7 @@ 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 { @@ -65,7 +66,7 @@ func (stmt *CleanupStmt) CleanupOlderThan( } } - if rowsDeleted < int64(count) { + if rowsDeleted < count { break } } diff --git a/pkg/icingadb/delta_test.go b/pkg/icingadb/delta_test.go index 5067ecd3a..d889c9069 100644 --- a/pkg/icingadb/delta_test.go +++ b/pkg/icingadb/delta_test.go @@ -232,19 +232,21 @@ func benchmarkDelta(b *testing.B, numEntities int) { return e } for i := 0; i < numEntities; i++ { + i := uint64(i) // #nosec G115 -- for loop iterates only over positive numbers + // 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 diff --git a/pkg/icingadb/history/retention.go b/pkg/icingadb/history/retention.go index f10048cf2..77a5c572b 100644 --- a/pkg/icingadb/history/retention.go +++ b/pkg/icingadb/history/retention.go @@ -11,6 +11,7 @@ import ( "github.com/icinga/icingadb/pkg/icingaredis/telemetry" "github.com/pkg/errors" "go.uber.org/zap" + "math" "time" ) @@ -180,9 +181,14 @@ func (r *Retention) Start(ctx context.Context) error { zap.Uint64("retention-days", days), ) + if days > -math.MinInt { + r.logger.Errorf("Skipping history retention for category %s as days number %d is too big", stmt.Category, -days) + continue + } + stmt := stmt periodic.Start(ctx, r.interval, func(tick periodic.Tick) { - olderThan := tick.Time.AddDate(0, 0, -int(days)) + olderThan := tick.Time.AddDate(0, 0, -int(days)) // #nosec G115 -- overflow check above r.logger.Debugf("Cleaning up historical data for category %s from table %s older than %s", stmt.Category, stmt.Table, olderThan) diff --git a/pkg/icingadb/types/comment_type.go b/pkg/icingadb/types/comment_type.go index 86bb092c6..eb11a1256 100644 --- a/pkg/icingadb/types/comment_type.go +++ b/pkg/icingadb/types/comment_type.go @@ -6,6 +6,7 @@ import ( "encoding/json" "github.com/icinga/icinga-go-library/types" "github.com/pkg/errors" + "math" "strconv" ) @@ -36,13 +37,11 @@ func (ct *CommentType) UnmarshalText(text []byte) error { if err != nil { return types.CantParseUint64(err, s) } - - c := CommentType(i) - if uint64(c) != i { - // Truncated due to above cast, obviously too high + if i > math.MaxUint8 { return badCommentType(s) } + c := CommentType(i) if _, ok := commentTypes[c]; !ok { return badCommentType(s) } diff --git a/pkg/icingadb/types/notification_type.go b/pkg/icingadb/types/notification_type.go index ce88c2efb..f431ae979 100644 --- a/pkg/icingadb/types/notification_type.go +++ b/pkg/icingadb/types/notification_type.go @@ -5,6 +5,7 @@ import ( "encoding" "github.com/icinga/icinga-go-library/types" "github.com/pkg/errors" + "math" "strconv" ) @@ -19,13 +20,11 @@ func (nt *NotificationType) UnmarshalText(text []byte) error { if err != nil { return types.CantParseUint64(err, s) } - - n := NotificationType(i) - if uint64(n) != i { - // Truncated due to above cast, obviously too high + if i > math.MaxUint16 { return badNotificationType(s) } + n := NotificationType(i) if _, ok := notificationTypes[n]; !ok { return badNotificationType(s) }