From bb403259770d661dbbb493893523f327fa78a800 Mon Sep 17 00:00:00 2001 From: Yury Gargay Date: Mon, 4 Sep 2023 17:03:44 +0200 Subject: [PATCH] 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 --- .github/workflows/golang-test-darwin.yml | 6 +- .github/workflows/golang-test-linux.yml | 12 ++-- .github/workflows/golangci-lint.yml | 9 ++- .github/workflows/release.yml | 48 +++++++++------- .../workflows/test-infrastructure-files.yml | 4 +- .golangci.yaml | 54 ++++++++++++++++++ base62/base62.go | 3 +- client/cmd/status.go | 6 +- client/internal/acl/manager.go | 3 +- client/internal/config_test.go | 3 - client/internal/dns/server_test.go | 2 +- client/internal/engine.go | 12 ++-- client/internal/engine_test.go | 2 +- client/internal/routemanager/client.go | 5 +- client/server/server.go | 5 +- iface/module_linux.go | 2 +- iface/wg_configurer_nonandroid.go | 3 - management/server/account.go | 4 +- management/server/account_test.go | 12 ---- management/server/activity/sqlite/sqlite.go | 56 ++++++++++++------- management/server/ephemeral_test.go | 2 +- management/server/http/groups_handler.go | 6 +- .../http/middleware/auth_middleware_test.go | 6 +- management/server/idp/auth0_test.go | 1 + management/server/idp/authentik.go | 19 +++++-- management/server/idp/authentik_test.go | 8 ++- management/server/idp/google_workspace.go | 10 ++-- management/server/idp/idp.go | 2 +- management/server/idp/keycloak_test.go | 1 + management/server/idp/zitadel_test.go | 1 + management/server/jwtclaims/jwtValidator.go | 4 +- management/server/management_proto_test.go | 5 -- management/server/peer_test.go | 13 +---- management/server/route.go | 13 ++--- management/server/user.go | 34 +++++------ 35 files changed, 215 insertions(+), 161 deletions(-) create mode 100644 .golangci.yaml diff --git a/.github/workflows/golang-test-darwin.yml b/.github/workflows/golang-test-darwin.yml index 6cdaa239b..97fdeabe8 100644 --- a/.github/workflows/golang-test-darwin.yml +++ b/.github/workflows/golang-test-darwin.yml @@ -15,14 +15,14 @@ jobs: runs-on: macos-latest steps: - name: Install Go - uses: actions/setup-go@v2 + uses: actions/setup-go@v4 with: go-version: "1.20.x" - name: Checkout code - uses: actions/checkout@v2 + uses: actions/checkout@v3 - name: Cache Go modules - uses: actions/cache@v2 + uses: actions/cache@v3 with: path: ~/go/pkg/mod key: macos-go-${{ hashFiles('**/go.sum') }} diff --git a/.github/workflows/golang-test-linux.yml b/.github/workflows/golang-test-linux.yml index d21ecd784..13061f6eb 100644 --- a/.github/workflows/golang-test-linux.yml +++ b/.github/workflows/golang-test-linux.yml @@ -18,13 +18,13 @@ jobs: runs-on: ubuntu-latest steps: - name: Install Go - uses: actions/setup-go@v2 + uses: actions/setup-go@v4 with: go-version: "1.20.x" - name: Cache Go modules - uses: actions/cache@v2 + uses: actions/cache@v3 with: path: ~/go/pkg/mod key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }} @@ -32,7 +32,7 @@ jobs: ${{ runner.os }}-go- - name: Checkout code - uses: actions/checkout@v2 + uses: actions/checkout@v3 - name: Install dependencies run: sudo apt update && sudo apt install -y -q libgtk-3-dev libayatana-appindicator3-dev libgl1-mesa-dev xorg-dev gcc-multilib @@ -47,13 +47,13 @@ jobs: runs-on: ubuntu-latest steps: - name: Install Go - uses: actions/setup-go@v2 + uses: actions/setup-go@v4 with: go-version: "1.20.x" - name: Cache Go modules - uses: actions/cache@v2 + uses: actions/cache@v3 with: path: ~/go/pkg/mod key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }} @@ -61,7 +61,7 @@ jobs: ${{ runner.os }}-go- - name: Checkout code - uses: actions/checkout@v2 + uses: actions/checkout@v3 - name: Install dependencies run: sudo apt update && sudo apt install -y -q libgtk-3-dev libayatana-appindicator3-dev libgl1-mesa-dev xorg-dev diff --git a/.github/workflows/golangci-lint.yml b/.github/workflows/golangci-lint.yml index f5d1835ce..2a5c51c8a 100644 --- a/.github/workflows/golangci-lint.yml +++ b/.github/workflows/golangci-lint.yml @@ -8,14 +8,13 @@ jobs: name: lint runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 + - name: Checkout code + uses: actions/checkout@v3 - name: Install Go - uses: actions/setup-go@v2 + uses: actions/setup-go@v4 with: go-version: "1.20.x" - name: Install dependencies run: sudo apt update && sudo apt install -y -q libgtk-3-dev libayatana-appindicator3-dev libgl1-mesa-dev xorg-dev - name: golangci-lint - uses: golangci/golangci-lint-action@v2 - with: - args: --timeout=6m \ No newline at end of file + uses: golangci/golangci-lint-action@v3 \ No newline at end of file diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index f682fe274..3feefdd49 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -29,20 +29,24 @@ concurrency: jobs: release: runs-on: ubuntu-latest + env: + flags: "" steps: + - if: ${{ !startsWith(github.ref, 'refs/tags/v') }} + run: echo "flags=--snapshot" >> $GITHUB_ENV - name: Checkout - uses: actions/checkout@v2 + uses: actions/checkout@v3 with: fetch-depth: 0 # It is required for GoReleaser to work properly - name: Set up Go - uses: actions/setup-go@v2 + uses: actions/setup-go@v4 with: go-version: "1.20" - name: Cache Go modules - uses: actions/cache@v1 + uses: actions/cache@v3 with: path: ~/go/pkg/mod key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }} @@ -56,10 +60,10 @@ jobs: run: git --no-pager diff --exit-code - name: Set up QEMU - uses: docker/setup-qemu-action@v1 + uses: docker/setup-qemu-action@v2 - name: Set up Docker Buildx - uses: docker/setup-buildx-action@v1 + uses: docker/setup-buildx-action@v2 - name: Login to Docker hub if: github.event_name != 'pull_request' @@ -82,10 +86,10 @@ jobs: run: rsrc -arch 386 -ico client/ui/netbird.ico -manifest client/manifest.xml -o client/resources_windows_386.syso - name: Run GoReleaser - uses: goreleaser/goreleaser-action@v2 + uses: goreleaser/goreleaser-action@v4 with: version: ${{ env.GORELEASER_VER }} - args: release --rm-dist + args: release --rm-dist ${{ env.flags }} env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} HOMEBREW_TAP_GITHUB_TOKEN: ${{ secrets.HOMEBREW_TAP_GITHUB_TOKEN }} @@ -93,7 +97,7 @@ jobs: UPLOAD_YUM_SECRET: ${{ secrets.PKG_UPLOAD_SECRET }} - name: upload non tags for debug purposes - uses: actions/upload-artifact@v2 + uses: actions/upload-artifact@v3 with: name: release path: dist/ @@ -102,17 +106,19 @@ jobs: release_ui: runs-on: ubuntu-latest steps: + - if: ${{ !startsWith(github.ref, 'refs/tags/v') }} + run: echo "flags=--snapshot" >> $GITHUB_ENV - name: Checkout - uses: actions/checkout@v2 + uses: actions/checkout@v3 with: fetch-depth: 0 # It is required for GoReleaser to work properly - name: Set up Go - uses: actions/setup-go@v2 + uses: actions/setup-go@v4 with: go-version: "1.20" - name: Cache Go modules - uses: actions/cache@v1 + uses: actions/cache@v3 with: path: ~/go/pkg/mod key: ${{ runner.os }}-ui-go-${{ hashFiles('**/go.sum') }} @@ -132,17 +138,17 @@ jobs: - name: Generate windows rsrc run: rsrc -arch amd64 -ico client/ui/netbird.ico -manifest client/ui/manifest.xml -o client/ui/resources_windows_amd64.syso - name: Run GoReleaser - uses: goreleaser/goreleaser-action@v2 + uses: goreleaser/goreleaser-action@v4 with: version: ${{ env.GORELEASER_VER }} - args: release --config .goreleaser_ui.yaml --rm-dist + args: release --config .goreleaser_ui.yaml --rm-dist ${{ env.flags }} env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} HOMEBREW_TAP_GITHUB_TOKEN: ${{ secrets.HOMEBREW_TAP_GITHUB_TOKEN }} UPLOAD_DEBIAN_SECRET: ${{ secrets.PKG_UPLOAD_SECRET }} UPLOAD_YUM_SECRET: ${{ secrets.PKG_UPLOAD_SECRET }} - name: upload non tags for debug purposes - uses: actions/upload-artifact@v2 + uses: actions/upload-artifact@v3 with: name: release-ui path: dist/ @@ -151,19 +157,21 @@ jobs: release_ui_darwin: runs-on: macos-11 steps: + - if: ${{ !startsWith(github.ref, 'refs/tags/v') }} + run: echo "flags=--snapshot" >> $GITHUB_ENV - name: Checkout - uses: actions/checkout@v2 + uses: actions/checkout@v3 with: fetch-depth: 0 # It is required for GoReleaser to work properly - name: Set up Go - uses: actions/setup-go@v2 + uses: actions/setup-go@v4 with: go-version: "1.20" - name: Cache Go modules - uses: actions/cache@v1 + uses: actions/cache@v3 with: path: ~/go/pkg/mod key: ${{ runner.os }}-ui-go-${{ hashFiles('**/go.sum') }} @@ -175,15 +183,15 @@ jobs: - name: Run GoReleaser id: goreleaser - uses: goreleaser/goreleaser-action@v2 + uses: goreleaser/goreleaser-action@v4 with: version: ${{ env.GORELEASER_VER }} - args: release --config .goreleaser_ui_darwin.yaml --rm-dist + args: release --config .goreleaser_ui_darwin.yaml --rm-dist ${{ env.flags }} env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - name: upload non tags for debug purposes - uses: actions/upload-artifact@v2 + uses: actions/upload-artifact@v3 with: name: release-ui-darwin path: dist/ diff --git a/.github/workflows/test-infrastructure-files.yml b/.github/workflows/test-infrastructure-files.yml index fdebc882e..d196ce0e0 100644 --- a/.github/workflows/test-infrastructure-files.yml +++ b/.github/workflows/test-infrastructure-files.yml @@ -24,12 +24,12 @@ jobs: run: sudo apt-get install -y curl - name: Install Go - uses: actions/setup-go@v2 + uses: actions/setup-go@v4 with: go-version: "1.20.x" - name: Cache Go modules - uses: actions/cache@v2 + uses: actions/cache@v3 with: path: ~/go/pkg/mod key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }} diff --git a/.golangci.yaml b/.golangci.yaml new file mode 100644 index 000000000..5034db708 --- /dev/null +++ b/.golangci.yaml @@ -0,0 +1,54 @@ +run: + # Timeout for analysis, e.g. 30s, 5m. + # Default: 1m + timeout: 6m + +# This file contains only configs which differ from defaults. +# All possible options can be found here https://github.com/golangci/golangci-lint/blob/master/.golangci.reference.yml +linters-settings: + errcheck: + # Report about not checking of errors in type assertions: `a := b.(MyStruct)`. + # Such cases aren't reported by default. + # Default: false + check-type-assertions: false + + govet: + # Enable all analyzers. + # Default: false + enable-all: false + enable: + - nilness + +linters: + disable-all: true + enable: + ## enabled by default + - errcheck # checking for unchecked errors, these unchecked errors can be critical bugs in some cases + - gosimple # specializes in simplifying a code + - govet # reports suspicious constructs, such as Printf calls whose arguments do not align with the format string + - ineffassign # detects when assignments to existing variables are not used + - staticcheck # is a go vet on steroids, applying a ton of static analysis checks + - typecheck # like the front-end of a Go compiler, parses and type-checks Go code + - unused # checks for unused constants, variables, functions and types + ## disable by default but the have interesting results so lets add them + - bodyclose # checks whether HTTP response body is closed successfully + - nilerr # finds the code that returns nil even if it checks that the error is not nil + - nilnil # checks that there is no simultaneous return of nil error and an invalid value + - sqlclosecheck # checks that sql.Rows and sql.Stmt are closed + - wastedassign # wastedassign finds wasted assignment statements +issues: + # Maximum count of issues with the same text. + # Set to 0 to disable. + # Default: 3 + max-same-issues: 5 + + exclude-rules: + - path: sharedsock/filter.go + linters: + - unused + - path: client/firewall/iptables/rule.go + linters: + - unused + - path: mock.go + linters: + - nilnil \ No newline at end of file diff --git a/base62/base62.go b/base62/base62.go index d2525f704..efafbc768 100644 --- a/base62/base62.go +++ b/base62/base62.go @@ -18,10 +18,9 @@ func Encode(num uint32) string { } var encoded strings.Builder - remainder := uint32(0) for num > 0 { - remainder = num % base + remainder := num % base encoded.WriteByte(alphabet[remainder]) num /= base } diff --git a/client/cmd/status.go b/client/cmd/status.go index 5d741462b..9dfd042f8 100644 --- a/client/cmd/status.go +++ b/client/cmd/status.go @@ -109,9 +109,9 @@ func statusFunc(cmd *cobra.Command, args []string) error { ctx := internal.CtxInitState(context.Background()) - resp, _ := getStatus(ctx, cmd) + resp, err := getStatus(ctx, cmd) if err != nil { - return nil + return err } if resp.GetStatus() == string(internal.StatusNeedsLogin) || resp.GetStatus() == string(internal.StatusLoginFailed) { @@ -133,7 +133,7 @@ func statusFunc(cmd *cobra.Command, args []string) error { outputInformationHolder := convertToStatusOutputOverview(resp) - statusOutputString := "" + var statusOutputString string switch { case detailFlag: statusOutputString = parseToFullDetailSummary(outputInformationHolder) diff --git a/client/internal/acl/manager.go b/client/internal/acl/manager.go index d4b6930a0..9a9e624d6 100644 --- a/client/internal/acl/manager.go +++ b/client/internal/acl/manager.go @@ -146,12 +146,11 @@ func (d *DefaultManager) ApplyFiltering(networkMap *mgmProto.NetworkMap) { // if this rule is member of rule selection with more than DefaultIPsCountForSet // it's IP address can be used in the ipset for firewall manager which supports it ipset := ipsetByRuleSelectors[d.getRuleGroupingSelector(r)] - ipsetName := "" if ipset.name == "" { d.ipsetCounter++ ipset.name = fmt.Sprintf("nb%07d", d.ipsetCounter) } - ipsetName = ipset.name + ipsetName := ipset.name pairID, rulePair, err := d.protoRuleToFirewallRule(r, ipsetName) if err != nil { log.Errorf("failed to apply firewall rule: %+v, %v", r, err) diff --git a/client/internal/config_test.go b/client/internal/config_test.go index 25e8f7b2e..8bd8d8d61 100644 --- a/client/internal/config_test.go +++ b/client/internal/config_test.go @@ -23,9 +23,6 @@ func TestGetConfig(t *testing.T) { assert.Equal(t, config.ManagementURL.String(), DefaultManagementURL) assert.Equal(t, config.AdminURL.String(), DefaultAdminURL) - if err != nil { - return - } managementURL := "https://test.management.url:33071" adminURL := "https://app.admin.url:443" path := filepath.Join(t.TempDir(), "config.json") diff --git a/client/internal/dns/server_test.go b/client/internal/dns/server_test.go index 73349de89..119ac684c 100644 --- a/client/internal/dns/server_test.go +++ b/client/internal/dns/server_test.go @@ -777,7 +777,7 @@ func createWgInterfaceWithBind(t *testing.T) (*iface.WGIface, error) { newNet, err := stdnet.NewNet(nil) if err != nil { t.Fatalf("create stdnet: %v", err) - return nil, nil + return nil, err } wgIface, err := iface.NewWGIFace("utun2301", "100.66.100.2/24", iface.DefaultMTU, nil, newNet) diff --git a/client/internal/engine.go b/client/internal/engine.go index 038f39e5c..8a6c08642 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -992,14 +992,12 @@ func (e *Engine) parseNATExternalIPMappings() []string { log.Warnf("invalid external IP, %s, ignoring external IP mapping '%s'", external, mapping) break } - if externalIP != nil { - mappedIP := externalIP.String() - if internalIP != nil { - mappedIP = mappedIP + "/" + internalIP.String() - } - mappedIPs = append(mappedIPs, mappedIP) - log.Infof("parsed external IP mapping of '%s' as '%s'", mapping, mappedIP) + mappedIP := externalIP.String() + if internalIP != nil { + mappedIP = mappedIP + "/" + internalIP.String() } + mappedIPs = append(mappedIPs, mappedIP) + log.Infof("parsed external IP mapping of '%s' as '%s'", mapping, mappedIP) } if len(mappedIPs) != len(e.config.NATExternalIPs) { log.Warnf("one or more external IP mappings failed to parse, ignoring all mappings") diff --git a/client/internal/engine_test.go b/client/internal/engine_test.go index d1c46181c..9f17ff36b 100644 --- a/client/internal/engine_test.go +++ b/client/internal/engine_test.go @@ -1046,7 +1046,7 @@ func startManagement(dataDir string) (*grpc.Server, string, error) { peersUpdateManager := server.NewPeersUpdateManager() eventStore := &activity.InMemoryEventStore{} if err != nil { - return nil, "", nil + return nil, "", err } accountManager, err := server.BuildManager(store, peersUpdateManager, nil, "", "", eventStore) diff --git a/client/internal/routemanager/client.go b/client/internal/routemanager/client.go index cf4cbe91b..62fe4dfc1 100644 --- a/client/internal/routemanager/client.go +++ b/client/internal/routemanager/client.go @@ -155,7 +155,10 @@ func (c *clientNetwork) startPeersStatusChangeWatcher() { func (c *clientNetwork) removeRouteFromWireguardPeer(peerKey string) error { state, err := c.statusRecorder.GetPeer(peerKey) - if err != nil || state.ConnStatus != peer.StatusConnected { + if err != nil { + return err + } + if state.ConnStatus != peer.StatusConnected { return nil } diff --git a/client/server/server.go b/client/server/server.go index b7cca947f..6748f62ab 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -3,10 +3,11 @@ package server import ( "context" "fmt" - "github.com/netbirdio/netbird/client/internal/auth" "sync" "time" + "github.com/netbirdio/netbird/client/internal/auth" + log "github.com/sirupsen/logrus" "google.golang.org/grpc/codes" "google.golang.org/grpc/metadata" @@ -315,7 +316,7 @@ func (s *Server) WaitSSOLogin(callerCtx context.Context, msg *proto.WaitSSOLogin tokenInfo, err := s.oauthAuthFlow.flow.WaitToken(waitCTX, flowInfo) if err != nil { if err == context.Canceled { - return nil, nil + return nil, nil //nolint:nilnil } s.mutex.Lock() s.oauthAuthFlow.expiresAt = time.Now() diff --git a/iface/module_linux.go b/iface/module_linux.go index 5f244d2c3..e943c0ba7 100644 --- a/iface/module_linux.go +++ b/iface/module_linux.go @@ -161,7 +161,7 @@ func getModulePath(name string) (string, error) { } if err != nil { // skip broken files - return nil + return nil //nolint:nilerr } if !info.Type().IsRegular() { diff --git a/iface/wg_configurer_nonandroid.go b/iface/wg_configurer_nonandroid.go index 70ec5dc04..6749c0966 100644 --- a/iface/wg_configurer_nonandroid.go +++ b/iface/wg_configurer_nonandroid.go @@ -146,9 +146,6 @@ func (c *wGConfigurer) removeAllowedIP(peerKey string, allowedIP string) error { } } - if err != nil { - return err - } peer := wgtypes.PeerConfig{ PublicKey: peerKeyParsed, UpdateOnly: true, diff --git a/management/server/account.go b/management/server/account.go index 26aeed3c5..4c707af3a 100644 --- a/management/server/account.go +++ b/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) { diff --git a/management/server/account_test.go b/management/server/account_test.go index 6002b7a3a..64fd90524 100644 --- a/management/server/account_test.go +++ b/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 diff --git a/management/server/activity/sqlite/sqlite.go b/management/server/activity/sqlite/sqlite.go index 02c2143e4..a4c85cf60 100644 --- a/management/server/activity/sqlite/sqlite.go +++ b/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 } diff --git a/management/server/ephemeral_test.go b/management/server/ephemeral_test.go index 554d2a028..a763f4cef 100644 --- a/management/server/ephemeral_test.go +++ b/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) { diff --git a/management/server/http/groups_handler.go b/management/server/http/groups_handler.go index 30c78e21b..d409623df 100644 --- a/management/server/http/groups_handler.go +++ b/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 } } diff --git a/management/server/http/middleware/auth_middleware_test.go b/management/server/http/middleware/auth_middleware_test.go index 8c8c941b0..608bf42fa 100644 --- a/management/server/http/middleware/auth_middleware_test.go +++ b/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) } }) } diff --git a/management/server/idp/auth0_test.go b/management/server/idp/auth0_test.go index 0814b4b69..0be401e65 100644 --- a/management/server/idp/auth0_test.go +++ b/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") diff --git a/management/server/idp/authentik.go b/management/server/idp/authentik.go index 586348fee..0898f1c94 100644 --- a/management/server/idp/authentik.go +++ b/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) diff --git a/management/server/idp/authentik_test.go b/management/server/idp/authentik_test.go index 5cf8f2b2c..c70a84efd 100644 --- a/management/server/idp/authentik_test.go +++ b/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") diff --git a/management/server/idp/google_workspace.go b/management/server/idp/google_workspace.go index efe457fdd..2e65497dc 100644 --- a/management/server/idp/google_workspace.go +++ b/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) diff --git a/management/server/idp/idp.go b/management/server/idp/idp.go index 48afd5c32..a1b55b183 100644 --- a/management/server/idp/idp.go +++ b/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 { diff --git a/management/server/idp/keycloak_test.go b/management/server/idp/keycloak_test.go index 115306d7d..0c33fc137 100644 --- a/management/server/idp/keycloak_test.go +++ b/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") diff --git a/management/server/idp/zitadel_test.go b/management/server/idp/zitadel_test.go index 28d26aedd..b558bba73 100644 --- a/management/server/idp/zitadel_test.go +++ b/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") diff --git a/management/server/jwtclaims/jwtValidator.go b/management/server/jwtclaims/jwtValidator.go index d15327566..b564e4f4e 100644 --- a/management/server/jwtclaims/jwtValidator.go +++ b/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) { diff --git a/management/server/management_proto_test.go b/management/server/management_proto_test.go index 792d05187..66661dbf8 100644 --- a/management/server/management_proto_test.go +++ b/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, diff --git a/management/server/peer_test.go b/management/server/peer_test.go index 822856e6a..36e96df43 100644 --- a/management/server/peer_test.go +++ b/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) diff --git a/management/server/route.go b/management/server/route.go index c02729a72..f51b7c2db 100644 --- a/management/server/route.go +++ b/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()) diff --git a/management/server/user.go b/management/server/user.go index b3556957d..8ee036df7 100644 --- a/management/server/user.go +++ b/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") }