Skip to content

Commit

Permalink
Introduce golangci-lint and fix findings
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
oxzi committed Oct 4, 2024
1 parent e8ed78e commit b451873
Show file tree
Hide file tree
Showing 10 changed files with 41 additions and 26 deletions.
8 changes: 8 additions & 0 deletions .github/workflows/go.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion cmd/icingadb-migrate/convert.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion cmd/icingadb-migrate/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package main

import (
"context"
"crypto/sha1"
"crypto/sha1" // #nosec G505 -- cryptographic weak SHA1
"database/sql"
_ "embed"
"encoding/hex"
Expand Down
4 changes: 2 additions & 2 deletions cmd/icingadb-migrate/misc.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
}
Expand Down
8 changes: 4 additions & 4 deletions cmd/icingadb/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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")
Expand All @@ -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"))
Expand Down
9 changes: 5 additions & 4 deletions pkg/icingadb/cleanup.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
},
Expand All @@ -57,15 +58,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
}
}
Expand Down
12 changes: 7 additions & 5 deletions pkg/icingadb/delta_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 7 additions & 1 deletion pkg/icingadb/history/retention.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/icinga/icingadb/pkg/icingaredis/telemetry"
"github.com/pkg/errors"
"go.uber.org/zap"
"math"
"time"
)

Expand Down Expand Up @@ -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)
Expand Down
7 changes: 3 additions & 4 deletions pkg/icingadb/types/comment_type.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"encoding/json"
"github.com/icinga/icinga-go-library/types"
"github.com/pkg/errors"
"math"
"strconv"
)

Expand Down Expand Up @@ -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)
}
Expand Down
7 changes: 3 additions & 4 deletions pkg/icingadb/types/notification_type.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"encoding"
"github.com/icinga/icinga-go-library/types"
"github.com/pkg/errors"
"math"
"strconv"
)

Expand All @@ -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)
}
Expand Down

0 comments on commit b451873

Please sign in to comment.