Update GitHub Actions and Enhance golangci-lint (#1075)

This PR showcases the implementation of additional linter rules. I've updated the golangci-lint GitHub Actions to the latest available version. This update makes sure that the tool works the same way locally - assuming being updated regularly - and with the GitHub Actions.

I've also taken care of keeping all the GitHub Actions up to date, which helps our code stay current. But there's one part, goreleaser that's a bit tricky to test on our computers. So, it's important to take a close look at that.

To make it easier to understand what I've done, I've made separate changes for each thing that the new linters found. This should help the people reviewing the changes see what's going on more clearly. Some of the changes might not be obvious at first glance.

Things to consider for the future
CI runs on Ubuntu so the static analysis only happens for Linux. Consider running it for the rest: Darwin, Windows

management/server/account.go

@@ -1022,7 +1022,7 @@ func (am *DefaultAccountManager) lookupUserInCacheByEmail(email string, accountI
 		}
 	}
 
-	return nil, nil
+	return nil, nil //nolint:nilnil
 }
 
 // lookupUserInCache looks up user in the IdP cache and returns it. If the user wasn't found, the function returns nil
@@ -1045,7 +1045,7 @@ func (am *DefaultAccountManager) lookupUserInCache(userID string, account *Accou
 		}
 	}
 
-	return nil, nil
+	return nil, nil //nolint:nilnil
 }
 
 func (am *DefaultAccountManager) refreshCache(accountID string) ([]*idp.UserData, error) {

management/server/account_test.go

@@ -784,10 +784,6 @@ func TestAccountManager_AddPeer(t *testing.T) {
 	serial := account.Network.CurrentSerial() // should be 0
 
 	setupKey, err := manager.CreateSetupKey(account.Id, "test-key", SetupKeyReusable, time.Hour, nil, 999, userID, false)
-	if err != nil {
-		return
-	}
-
 	if err != nil {
 		t.Fatal("error creating setup key")
 		return
@@ -931,10 +927,6 @@ func TestAccountManager_NetworkUpdates(t *testing.T) {
 	}
 
 	setupKey, err := manager.CreateSetupKey(account.Id, "test-key", SetupKeyReusable, time.Hour, nil, 999, userID, false)
-	if err != nil {
-		return
-	}
-
 	if err != nil {
 		t.Fatal("error creating setup key")
 		return
@@ -1115,10 +1107,6 @@ func TestAccountManager_DeletePeer(t *testing.T) {
 	}
 
 	setupKey, err := manager.CreateSetupKey(account.Id, "test-key", SetupKeyReusable, time.Hour, nil, 999, userID, false)
-	if err != nil {
-		return
-	}
-
 	if err != nil {
 		t.Fatal("error creating setup key")
 		return

management/server/activity/sqlite/sqlite.go

@@ -3,13 +3,14 @@ package sqlite
 import (
 	"database/sql"
 	"encoding/json"
-	"fmt"
+
 	"github.com/netbirdio/netbird/management/server/activity"
 
 	// sqlite driver
-	_ "github.com/mattn/go-sqlite3"
 	"path/filepath"
 	"time"
+
+	_ "github.com/mattn/go-sqlite3"
 )
 
 const (
@@ -24,15 +25,20 @@ const (
 		"meta TEXT," +
 		" target_id TEXT);"
 
-	selectStatement = "SELECT id, activity, timestamp, initiator_id, target_id, account_id, meta" +
-		" FROM events WHERE account_id = ? ORDER BY timestamp %s LIMIT ? OFFSET ?;"
-	insertStatement = "INSERT INTO events(activity, timestamp, initiator_id, target_id, account_id, meta) " +
+	selectDescQuery = "SELECT id, activity, timestamp, initiator_id, target_id, account_id, meta" +
+		" FROM events WHERE account_id = ? ORDER BY timestamp DESC LIMIT ? OFFSET ?;"
+	selectAscQuery = "SELECT id, activity, timestamp, initiator_id, target_id, account_id, meta" +
+		" FROM events WHERE account_id = ? ORDER BY timestamp ASC LIMIT ? OFFSET ?;"
+	insertQuery = "INSERT INTO events(activity, timestamp, initiator_id, target_id, account_id, meta) " +
 		"VALUES(?, ?, ?, ?, ?, ?)"
 )
 
 // Store is the implementation of the activity.Store interface backed by SQLite
 type Store struct {
-	db *sql.DB
+	db                  *sql.DB
+	insertStatement     *sql.Stmt
+	selectAscStatement  *sql.Stmt
+	selectDescStatement *sql.Stmt
 }
 
 // NewSQLiteStore creates a new Store with an event table if not exists.
@@ -48,7 +54,27 @@ func NewSQLiteStore(dataDir string) (*Store, error) {
 		return nil, err
 	}
 
-	return &Store{db: db}, nil
+	insertStmt, err := db.Prepare(insertQuery)
+	if err != nil {
+		return nil, err
+	}
+
+	selectDescStmt, err := db.Prepare(selectDescQuery)
+	if err != nil {
+		return nil, err
+	}
+
+	selectAscStmt, err := db.Prepare(selectAscQuery)
+	if err != nil {
+		return nil, err
+	}
+
+	return &Store{
+		db:                  db,
+		insertStatement:     insertStmt,
+		selectDescStatement: selectDescStmt,
+		selectAscStatement:  selectAscStmt,
+	}, nil
 }
 
 func processResult(result *sql.Rows) ([]*activity.Event, error) {
@@ -90,13 +116,9 @@ func processResult(result *sql.Rows) ([]*activity.Event, error) {
 
 // Get returns "limit" number of events from index ordered descending or ascending by a timestamp
 func (store *Store) Get(accountID string, offset, limit int, descending bool) ([]*activity.Event, error) {
-	order := "DESC"
+	stmt := store.selectDescStatement
 	if !descending {
-		order = "ASC"
-	}
-	stmt, err := store.db.Prepare(fmt.Sprintf(selectStatement, order))
-	if err != nil {
-		return nil, err
+		stmt = store.selectAscStatement
 	}
 
 	result, err := stmt.Query(accountID, limit, offset)
@@ -110,12 +132,6 @@ func (store *Store) Get(accountID string, offset, limit int, descending bool) ([
 
 // Save an event in the SQLite events table
 func (store *Store) Save(event *activity.Event) (*activity.Event, error) {
-
-	stmt, err := store.db.Prepare(insertStatement)
-	if err != nil {
-		return nil, err
-	}
-
 	var jsonMeta string
 	if event.Meta != nil {
 		metaBytes, err := json.Marshal(event.Meta)
@@ -125,7 +141,7 @@ func (store *Store) Save(event *activity.Event) (*activity.Event, error) {
 		jsonMeta = string(metaBytes)
 	}
 
-	result, err := stmt.Exec(event.Activity, event.Timestamp, event.InitiatorID, event.TargetID, event.AccountID, jsonMeta)
+	result, err := store.insertStatement.Exec(event.Activity, event.Timestamp, event.InitiatorID, event.TargetID, event.AccountID, jsonMeta)
 	if err != nil {
 		return nil, err
 	}

management/server/ephemeral_test.go

@@ -31,7 +31,7 @@ type MocAccountManager struct {
 
 func (a MocAccountManager) DeletePeer(accountID, peerID, userID string) (*Peer, error) {
 	delete(a.store.account.Peers, peerID)
-	return nil, nil
+	return nil, nil //nolint:nilnil
 }
 
 func TestNewManager(t *testing.T) {

management/server/http/groups_handler.go

@@ -231,10 +231,8 @@ func (h *GroupsHandler) GetGroup(w http.ResponseWriter, r *http.Request) {
 
 		util.WriteJSONObject(w, toGroupResponse(account, group))
 	default:
-		if err != nil {
-			util.WriteError(status.Errorf(status.NotFound, "HTTP method not found"), w)
-			return
-		}
+		util.WriteError(status.Errorf(status.NotFound, "HTTP method not found"), w)
+		return
 	}
 }
 

management/server/http/middleware/auth_middleware_test.go

@@ -115,8 +115,10 @@ func TestAuthMiddleware_Handler(t *testing.T) {
 
 			handlerToTest.ServeHTTP(rec, req)
 
-			if rec.Result().StatusCode != tc.expectedStatusCode {
-				t.Errorf("expected status code %d, got %d", tc.expectedStatusCode, rec.Result().StatusCode)
+			result := rec.Result()
+			defer result.Body.Close()
+			if result.StatusCode != tc.expectedStatusCode {
+				t.Errorf("expected status code %d, got %d", tc.expectedStatusCode, result.StatusCode)
 			}
 		})
 	}

management/server/idp/auth0_test.go

@@ -133,6 +133,7 @@ func TestAuth0_RequestJWTToken(t *testing.T) {
 					t.Fatal(err)
 				}
 			}
+			defer res.Body.Close()
 			body, err := io.ReadAll(res.Body)
 			assert.NoError(t, err, "unable to read the response body")
 

management/server/idp/authentik.go

@@ -3,10 +3,6 @@ package idp
 import (
 	"context"
 	"fmt"
-	"github.com/golang-jwt/jwt"
-	"github.com/netbirdio/netbird/management/server/telemetry"
-	log "github.com/sirupsen/logrus"
-	"goauthentik.io/api/v3"
 	"io"
 	"net/http"
 	"net/url"
@@ -14,6 +10,11 @@ import (
 	"strings"
 	"sync"
 	"time"
+
+	"github.com/golang-jwt/jwt"
+	"github.com/netbirdio/netbird/management/server/telemetry"
+	log "github.com/sirupsen/logrus"
+	"goauthentik.io/api/v3"
 )
 
 // AuthentikManager authentik manager client instance.
@@ -236,6 +237,7 @@ func (am *AuthentikManager) UpdateUserAppMetadata(userID string, appMetadata App
 	if err != nil {
 		return err
 	}
+	defer resp.Body.Close()
 
 	if am.appMetrics != nil {
 		am.appMetrics.IDPMetrics().CountUpdateUserAppMetadata()
@@ -267,6 +269,7 @@ func (am *AuthentikManager) GetUserDataByID(userID string, appMetadata AppMetada
 	if err != nil {
 		return nil, err
 	}
+	defer resp.Body.Close()
 
 	if am.appMetrics != nil {
 		am.appMetrics.IDPMetrics().CountGetUserDataByID()
@@ -294,6 +297,7 @@ func (am *AuthentikManager) GetAccount(accountID string) ([]*UserData, error) {
 	if err != nil {
 		return nil, err
 	}
+	defer resp.Body.Close()
 
 	if am.appMetrics != nil {
 		am.appMetrics.IDPMetrics().CountGetAccount()
@@ -330,6 +334,7 @@ func (am *AuthentikManager) GetAllAccounts() (map[string][]*UserData, error) {
 	if err != nil {
 		return nil, err
 	}
+	defer resp.Body.Close()
 
 	if am.appMetrics != nil {
 		am.appMetrics.IDPMetrics().CountGetAllAccounts()
@@ -389,6 +394,7 @@ func (am *AuthentikManager) CreateUser(email, name, accountID, invitedByEmail st
 	if err != nil {
 		return nil, err
 	}
+	defer resp.Body.Close()
 
 	if am.appMetrics != nil {
 		am.appMetrics.IDPMetrics().CountCreateUser()
@@ -416,6 +422,7 @@ func (am *AuthentikManager) GetUserByEmail(email string) ([]*UserData, error) {
 	if err != nil {
 		return nil, err
 	}
+	defer resp.Body.Close()
 
 	if am.appMetrics != nil {
 		am.appMetrics.IDPMetrics().CountGetUserByEmail()
@@ -469,10 +476,11 @@ func (am *AuthentikManager) getUserGroupByName(name string) (string, error) {
 		return "", err
 	}
 
-	groupList, _, err := am.apiClient.CoreApi.CoreGroupsList(ctx).Name(name).Execute()
+	groupList, resp, err := am.apiClient.CoreApi.CoreGroupsList(ctx).Name(name).Execute()
 	if err != nil {
 		return "", err
 	}
+	defer resp.Body.Close()
 
 	if groupList != nil {
 		if len(groupList.Results) > 0 {
@@ -485,6 +493,7 @@ func (am *AuthentikManager) getUserGroupByName(name string) (string, error) {
 	if err != nil {
 		return "", err
 	}
+	defer resp.Body.Close()
 
 	if resp.StatusCode != http.StatusCreated {
 		return "", fmt.Errorf("unable to create user group, statusCode: %d", resp.StatusCode)

management/server/idp/authentik_test.go

@@ -2,13 +2,14 @@ package idp
 
 import (
 	"fmt"
-	"github.com/netbirdio/netbird/management/server/telemetry"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 	"io"
 	"strings"
 	"testing"
 	"time"
+
+	"github.com/netbirdio/netbird/management/server/telemetry"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestNewAuthentikManager(t *testing.T) {
@@ -133,6 +134,7 @@ func TestAuthentikRequestJWTToken(t *testing.T) {
 					t.Fatal(err)
 				}
 			} else {
+				defer resp.Body.Close()
 				body, err := io.ReadAll(resp.Body)
 				assert.NoError(t, err, "unable to read the response body")
 

management/server/idp/google_workspace.go

@@ -4,15 +4,16 @@ import (
 	"context"
 	"encoding/base64"
 	"fmt"
+	"net/http"
+	"strings"
+	"time"
+
 	"github.com/netbirdio/netbird/management/server/telemetry"
 	log "github.com/sirupsen/logrus"
 	"golang.org/x/oauth2/google"
 	admin "google.golang.org/api/admin/directory/v1"
 	"google.golang.org/api/googleapi"
 	"google.golang.org/api/option"
-	"net/http"
-	"strings"
-	"time"
 )
 
 // GoogleWorkspaceManager Google Workspace manager client instance.
@@ -271,7 +272,8 @@ func getGoogleCredentials(serviceAccountKey string) (*google.Credentials, error)
 		admin.AdminDirectoryUserScope,
 	)
 	if err == nil {
-		return creds, err
+		// No need to fallback to the default Google credentials path
+		return creds, nil
 	}
 
 	log.Debugf("failed to retrieve Google credentials from ServiceAccountKey: %v", err)

management/server/idp/idp.go

@@ -92,7 +92,7 @@ func NewManager(config Config, appMetrics telemetry.AppMetrics) (Manager, error)
 
 	switch strings.ToLower(config.ManagerType) {
 	case "none", "":
-		return nil, nil
+		return nil, nil //nolint:nilnil
 	case "auth0":
 		auth0ClientConfig := config.Auth0ClientCredentials
 		if config.ClientConfig != nil {

management/server/idp/keycloak_test.go

@@ -145,6 +145,7 @@ func TestKeycloakRequestJWTToken(t *testing.T) {
 					t.Fatal(err)
 				}
 			} else {
+				defer resp.Body.Close()
 				body, err := io.ReadAll(resp.Body)
 				assert.NoError(t, err, "unable to read the response body")
 

management/server/idp/zitadel_test.go

@@ -124,6 +124,7 @@ func TestZitadelRequestJWTToken(t *testing.T) {
 					t.Fatal(err)
 				}
 			} else {
+				defer resp.Body.Close()
 				body, err := io.ReadAll(resp.Body)
 				assert.NoError(t, err, "unable to read the response body")
 

management/server/jwtclaims/jwtValidator.go

@@ -141,7 +141,7 @@ func (m *JWTValidator) ValidateAndParse(token string) (*jwt.Token, error) {
 		if m.options.CredentialsOptional {
 			log.Debugf("no credentials found (CredentialsOptional=true)")
 			// No error, just no token (and that is ok given that CredentialsOptional is true)
-			return nil, nil
+			return nil, nil //nolint:nilnil
 		}
 
 		// If we get here, the required token is missing
@@ -219,7 +219,7 @@ func getPemCert(token *jwt.Token, jwks *Jwks) (string, error) {
 		return generatePemFromJWK(jwks.Keys[k])
 	}
 
-	return "", errors.New("unable to find appropriate key")
+	return cert, errors.New("unable to find appropriate key")
 }
 
 func generatePemFromJWK(jwk JSONWebKey) (string, error) {

management/server/management_proto_test.go

@@ -141,11 +141,6 @@ func Test_SyncProtocol(t *testing.T) {
 		return
 	}
 
-	if err != nil {
-		t.Fatal(err)
-		return
-	}
-
 	sync, err := client.Sync(context.TODO(), &mgmtProto.EncryptedMessage{
 		WgPubKey: key.PublicKey().String(),
 		Body:     message,

management/server/peer_test.go

@@ -79,10 +79,6 @@ func TestAccountManager_GetNetworkMap(t *testing.T) {
 	}
 
 	setupKey, err := manager.CreateSetupKey(account.Id, "test-key", SetupKeyReusable, time.Hour, nil, 999, userId, false)
-	if err != nil {
-		return
-	}
-
 	if err != nil {
 		t.Fatal("error creating setup key")
 		return
@@ -332,10 +328,6 @@ func TestAccountManager_GetPeerNetwork(t *testing.T) {
 	}
 
 	setupKey, err := manager.CreateSetupKey(account.Id, "test-key", SetupKeyReusable, time.Hour, nil, 999, userId, false)
-	if err != nil {
-		return
-	}
-
 	if err != nil {
 		t.Fatal("error creating setup key")
 		return
@@ -406,14 +398,11 @@ func TestDefaultAccountManager_GetPeer(t *testing.T) {
 
 	// two peers one added by a regular user and one with a setup key
 	setupKey, err := manager.CreateSetupKey(account.Id, "test-key", SetupKeyReusable, time.Hour, nil, 999, adminUser, false)
-	if err != nil {
-		return
-	}
-
 	if err != nil {
 		t.Fatal("error creating setup key")
 		return
 	}
+
 	peerKey1, err := wgtypes.GeneratePrivateKey()
 	if err != nil {
 		t.Fatal(err)

management/server/route.go

@@ -1,15 +1,16 @@
 package server
 
 import (
+	"net/netip"
+	"strconv"
+	"unicode/utf8"
+
 	"github.com/netbirdio/netbird/management/proto"
 	"github.com/netbirdio/netbird/management/server/activity"
 	"github.com/netbirdio/netbird/management/server/status"
 	"github.com/netbirdio/netbird/route"
 	"github.com/rs/xid"
 	log "github.com/sirupsen/logrus"
-	"net/netip"
-	"strconv"
-	"unicode/utf8"
 )
 
 const (
@@ -104,12 +105,6 @@ func (am *DefaultAccountManager) checkPrefixPeerExists(accountID, peerID string,
 
 	routesWithPrefix := account.GetRoutesByPrefix(prefix)
 
-	if err != nil {
-		if s, ok := status.FromError(err); ok && s.Type() == status.NotFound {
-			return nil
-		}
-		return status.Errorf(status.InvalidArgument, "failed to parse prefix %s", prefix.String())
-	}
 	for _, prefixRoute := range routesWithPrefix {
 		if prefixRoute.Peer == peerID {
 			return status.Errorf(status.AlreadyExists, "failed to add route with prefix %s - peer already has this route", prefix.String())

management/server/user.go

@@ -405,13 +405,13 @@ func (am *DefaultAccountManager) CreatePAT(accountID string, initiatorUserID str
 		return nil, err
 	}
 
-	targetUser := account.Users[targetUserID]
-	if targetUser == nil {
-		return nil, status.Errorf(status.NotFound, "targetUser not found")
+	targetUser, ok := account.Users[targetUserID]
+	if !ok {
+		return nil, status.Errorf(status.NotFound, "user not found")
 	}
 
-	executingUser := account.Users[initiatorUserID]
-	if targetUser == nil {
+	executingUser, ok := account.Users[initiatorUserID]
+	if !ok {
 		return nil, status.Errorf(status.NotFound, "user not found")
 	}
 
@@ -447,13 +447,13 @@ func (am *DefaultAccountManager) DeletePAT(accountID string, initiatorUserID str
 		return status.Errorf(status.NotFound, "account not found: %s", err)
 	}
 
-	targetUser := account.Users[targetUserID]
-	if targetUser == nil {
+	targetUser, ok := account.Users[targetUserID]
+	if !ok {
 		return status.Errorf(status.NotFound, "user not found")
 	}
 
-	executingUser := account.Users[initiatorUserID]
-	if targetUser == nil {
+	executingUser, ok := account.Users[initiatorUserID]
+	if !ok {
 		return status.Errorf(status.NotFound, "user not found")
 	}
 
@@ -497,13 +497,13 @@ func (am *DefaultAccountManager) GetPAT(accountID string, initiatorUserID string
 		return nil, status.Errorf(status.NotFound, "account not found: %s", err)
 	}
 
-	targetUser := account.Users[targetUserID]
-	if targetUser == nil {
+	targetUser, ok := account.Users[targetUserID]
+	if !ok {
 		return nil, status.Errorf(status.NotFound, "user not found")
 	}
 
-	executingUser := account.Users[initiatorUserID]
-	if targetUser == nil {
+	executingUser, ok := account.Users[initiatorUserID]
+	if !ok {
 		return nil, status.Errorf(status.NotFound, "user not found")
 	}
 
@@ -529,13 +529,13 @@ func (am *DefaultAccountManager) GetAllPATs(accountID string, initiatorUserID st
 		return nil, status.Errorf(status.NotFound, "account not found: %s", err)
 	}
 
-	targetUser := account.Users[targetUserID]
-	if targetUser == nil {
+	targetUser, ok := account.Users[targetUserID]
+	if !ok {
 		return nil, status.Errorf(status.NotFound, "user not found")
 	}
 
-	executingUser := account.Users[initiatorUserID]
-	if targetUser == nil {
+	executingUser, ok := account.Users[initiatorUserID]
+	if !ok {
 		return nil, status.Errorf(status.NotFound, "user not found")
 	}