*: add lint CI

Signed-off-by: xhe <xw897002528@gmail.com>

.github/workflows/common.yml

@@ -10,6 +10,10 @@ on:
         type: string
         description: "makefile target"
         required: true
+      lint:
+        type: boolean
+        description: "run with lint or not"
+        required: true
       ref:
         type: string
         description: "checkout specific ref"
@@ -58,7 +62,14 @@ jobs:
           key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }}
           restore-keys: |
             ${{ runner.os }}-go-
-      - run: make ${{ inputs.target }}
+      - name: "lint programs"
+        if: ${{ inputs.lint }}
+        uses: golangci/golangci-lint-action@v3
+        with:
+          version: latest
+      - name: make ${{ inputs.target }} 
+        if: ${{ inputs.target != "" }}
+        run: make ${{ inputs.target }}
       - name: "set up tmate session if necessary"
         if: ${{ failure() && inputs.debug }}
         uses: mxschmitt/action-tmate@v3

.github/workflows/main.yml

@@ -43,6 +43,15 @@ jobs:
       ref: ${{ inputs.ref || github.ref }}
       target: "build"
 
+  lint:
+    needs: build
+    uses: ./.github/workflows/common.yml
+    with:
+      debug: ${{ github.event_name == 'workflow_dispatch' && github.event.inputs.debug }}
+      ref: ${{ inputs.ref || github.ref }}
+      lint: true
+      target: ""
+
   test:
     needs: build
     uses: ./.github/workflows/common.yml

.golangci.yaml

@@ -1,99 +1,19 @@
 # options for analysis running
 run:
-  # default concurrency is a available CPU number
-  concurrency: 4
-
-  # timeout for analysis, e.g. 30s, 5m, default is 1m
-  timeout: 20m
-
-  # exit code when at least one issue was found, default is 1
+  timeout: 10m
   issues-exit-code: 1
 
-  # include test files or not, default is true
-  tests: false
-
-  # list of build tags, all linters use it. Default is empty list.
-  build-tags:
-
-  # default is true. Enables skipping of directories:
-  #   vendor$, third_party$, testdata$, examples$, Godeps$, builtin$
-  skip-dirs-use-default: true
-
-  # which dirs to skip: they won't be analyzed;
-  # can use regexp here: generated.*, regexp is applied on full path;
-  # default value is empty list, but next dirs are always skipped independently
-  # from this option's value:
-  #   	vendor$, third_party$, testdata$, examples$, Godeps$, builtin$
-  #  skip-dirs:
-  #    - ^test.*
-
-  # by default isn't set. If set we pass it to "go list -mod={option}". From "go help modules":
-  # If invoked with -mod=readonly, the go command is disallowed from the implicit
-  # automatic updating of go.mod described above. Instead, it fails when any changes
-  # to go.mod are needed. This setting is most useful to check that go.mod does
-  # not need updates, such as in a continuous integration and testing system.
-  # If invoked with -mod=vendor, the go command assumes that the vendor
-  # directory holds the correct copies of dependencies and ignores
-  # the dependency descriptions in go.mod.
-  modules-download-mode: readonly
-
-  # which files to skip: they will be analyzed, but issues from them
-  # won't be reported. Default value is empty list, but there is
-  # no need to include all autogenerated files, we confidently recognize
-  # autogenerated files. If it's not please let us know.
-  skip-files:
-  #  - ".*\\.my\\.go$"
-  #  - lib/bad.go
-
-# all available settings of specific linters
 linters-settings:
-  misspell:
-    # Correct spellings using locale preferences for US or UK.
-    # Default is to use a neutral variety of English.
-    # Setting locale to US will correct the British spelling of 'colour' to 'color'.
-    # locale: US
-    ignore-words:
-      - rela # This is for elf.SHT_RELA
 
 issues:
-  # Excluding configuration per-path, per-linter, per-text and per-source
   exclude-rules:
-    - linters: [staticcheck]
-      text: "SA1019"                  # this is rule for deprecated method
-    - linters: [staticcheck]
-      text: "SA9003: empty branch"
-    - linters: [staticcheck]
-      text: "SA2001: empty critical section"
-    - linters: [goerr113]
-      text: "do not define dynamic errors, use wrapped static errors instead" # This rule to avoid opinionated check fmt.Errorf("text")
-    - path: _test\.go                # Skip 1.13 errors check for test files
+    - path: _test\.go
       linters:
-        - goerr113
+        - gosec
+
 linters:
-  disable-all: true
   enable:
-    - deadcode
-    - goerr113
+    - gosec
     - gofmt
     - goimports
-    - gosimple
-    - govet
-    - ineffassign
     - misspell
-    - staticcheck
-    - stylecheck
-    - varcheck
-
-# To enable later if makes sense
-#    - errcheck
-#    - gocyclo
-#    - golint
-#    - gosec
-#    - gosimple
-#    - lll
-#    - maligned
-#    - misspell
-#    - prealloc
-#    - structcheck
-#    - typecheck
-#    - unused

Makefile

@@ -29,7 +29,7 @@ endif
 IMAGE_TAG ?= latest
 EXECUTABLE_TARGETS := $(patsubst cmd/%,cmd_%,$(wildcard cmd/*))
 
-.PHONY: cmd_% build test docker
+.PHONY: cmd_% build test docker ./bin/golangci-lint ./bin/gocovmerge
 
 default: cmd
 
@@ -44,14 +44,20 @@ cmd_%: SOURCE=$(patsubst cmd_%,./cmd/%,$@)
 cmd_%:
 	go build $(BUILDFLAGS) -o $(OUTPUT) $(SOURCE)
 
+lint: ./bin/golangci-lint
+	$(GOBIN)/golangci-lint run
+
 test: ./bin/gocovmerge
 	rm -f .cover.*
 	go test -coverprofile=.cover.pkg ./...
 	cd lib && go test -coverprofile=../.cover.lib ./...
-	./bin/gocovmerge .cover.* > .cover
+	$(GOBIN)/gocovmerge .cover.* > .cover
 	rm -f .cover.*
 	go tool cover -html=.cover -o .cover.html
 
+./bin/golangci-lint:
+	GOBIN=$(GOBIN) go install github.com/golangci/golangci-lint/cmd/golangci-lint@latest
+
 ./bin/gocovmerge:
 	GOBIN=$(GOBIN) go install github.com/wadey/gocovmerge@master
 

cmd/tiproxy/main.go

@@ -16,7 +16,6 @@
 package main
 
 import (
-	"io/ioutil"
 	"os"
 
 	"github.com/pingcap/TiProxy/lib/config"
@@ -40,7 +39,7 @@ func main() {
 	logLevel := rootCmd.PersistentFlags().String("log_level", "", "log level")
 
 	rootCmd.RunE = func(cmd *cobra.Command, _ []string) error {
-		proxyConfigData, err := ioutil.ReadFile(*configFile)
+		proxyConfigData, err := os.ReadFile(*configFile)
 		if err != nil {
 			return err
 		}

pkg/manager/config/utils.go

@@ -1,4 +1,3 @@
-
 // Copyright 2022 PingCAP, Inc.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");

pkg/manager/logger/builder.go

@@ -35,14 +35,18 @@ func buildEncoder(cfg *config.Log) (zapcore.Encoder, error) {
 		return nil, err
 	}
 	registerEncoders.Do(func() {
-		zap.RegisterEncoder("tidb", func(cfg zapcore.EncoderConfig) (zapcore.Encoder, error) {
+		if err = zap.RegisterEncoder("tidb", func(cfg zapcore.EncoderConfig) (zapcore.Encoder, error) {
 			return encoder, nil
-		})
-		zap.RegisterEncoder("newtidb", func(cfg zapcore.EncoderConfig) (zapcore.Encoder, error) {
+		}); err != nil {
+			return
+		}
+		if err = zap.RegisterEncoder("newtidb", func(cfg zapcore.EncoderConfig) (zapcore.Encoder, error) {
 			return cmd.NewTiDBEncoder(cfg), nil
-		})
+		}); err != nil {
+			return
+		}
 	})
-	return encoder, nil
+	return encoder, err
 }
 
 func buildLevel(cfg *config.Log) (zap.AtomicLevel, error) {

pkg/metrics/metrics.go

@@ -110,7 +110,7 @@ func (mm *MetricsManager) pushMetric(ctx context.Context, addr string, interval
 
 // registerProxyMetrics registers metrics.
 func registerProxyMetrics() {
-	prometheus.DefaultRegisterer.Unregister(prometheus.NewGoCollector())
+	prometheus.DefaultRegisterer.Unregister(collectors.NewGoCollector())
 	prometheus.MustRegister(collectors.NewGoCollector(collectors.WithGoCollections(collectors.GoRuntimeMetricsCollection | collectors.GoRuntimeMemStatsCollection)))
 
 	prometheus.MustRegister(ConnGauge)

pkg/proxy/backend/authenticator.go

@@ -96,7 +96,7 @@ func (auth *Authenticator) handshakeFirstTime(clientIO, backendIO *pnet.PacketIO
 	addr := backendIO.RemoteAddr().String()
 	if auth.serverAddr != "" {
 		// NOTE: should use DNS name as much as possible
-		// Usally certs are signed with domain instead of IP addrs
+		// Usually certs are signed with domain instead of IP addrs
 		// And `RemoteAddr()` will return IP addr
 		addr = auth.serverAddr
 	}

pkg/proxy/backend/backend_conn.go

@@ -18,8 +18,8 @@ import (
 	"net"
 	"time"
 
-	pnet "github.com/pingcap/TiProxy/pkg/proxy/net"
 	"github.com/pingcap/TiProxy/lib/util/errors"
+	pnet "github.com/pingcap/TiProxy/pkg/proxy/net"
 )
 
 const (

pkg/proxy/backend/backend_conn_mgr.go

@@ -18,7 +18,6 @@ import (
 	"context"
 	"crypto/tls"
 	"encoding/binary"
-	"encoding/json"
 	"fmt"
 	"strings"
 	"sync"
@@ -282,20 +281,6 @@ func (mgr *BackendConnManager) tryRedirect(ctx context.Context) {
 	mgr.backendConn = newConn
 }
 
-// The original db in the auth info may be dropped during the session, so we need to authenticate with the current db.
-// The user may be renamed during the session, but the session cannot detect it, so this will affect the user.
-// TODO: this may be a security problem: a different new user may just be renamed to this user name.
-func (mgr *BackendConnManager) updateAuthInfoFromSessionStates(sessionStates []byte) error {
-	var statesMap map[string]string
-	if err := json.Unmarshal(sessionStates, &statesMap); err != nil {
-		return errors.Wrapf(err, "unmarshal session states error")
-	}
-	// The currentDBKey may be omitted if it's empty. In this case, we still need to update it.
-	currentDB := statesMap[currentDBKey]
-	mgr.authenticator.updateCurrentDB(currentDB)
-	return nil
-}
-
 // Redirect implements RedirectableConn.Redirect interface. It redirects the current session to the newAddr.
 // Note that the caller requires the function to be non-blocking.
 func (mgr *BackendConnManager) Redirect(newAddr string) {

pkg/proxy/backend/cmd_processor_exec.go

@@ -18,8 +18,8 @@ import (
 	"encoding/binary"
 	"strings"
 
-	pnet "github.com/pingcap/TiProxy/pkg/proxy/net"
 	"github.com/pingcap/TiProxy/lib/util/errors"
+	pnet "github.com/pingcap/TiProxy/pkg/proxy/net"
 	"github.com/pingcap/tidb/parser"
 	"github.com/pingcap/tidb/parser/mysql"
 	"github.com/siddontang/go/hack"
@@ -196,7 +196,7 @@ func (cp *CmdProcessor) forwardQueryCmd(clientIO, backendIO *pnet.PacketIO, requ
 		case mysql.LocalInFileHeader:
 			serverStatus, err = cp.forwardLoadInFile(clientIO, backendIO, request)
 		default:
-			serverStatus, err = cp.forwardResultSet(clientIO, backendIO, request, response)
+			serverStatus, err = cp.forwardResultSet(clientIO, backendIO, request)
 		}
 		if err != nil {
 			return err
@@ -239,8 +239,9 @@ func (cp *CmdProcessor) forwardLoadInFile(clientIO, backendIO *pnet.PacketIO, re
 	return serverStatus, errors.Errorf("unexpected response, cmd:%d resp:%d", mysql.ComQuery, response[0])
 }
 
-func (cp *CmdProcessor) forwardResultSet(clientIO, backendIO *pnet.PacketIO, request, response []byte) (uint16, error) {
+func (cp *CmdProcessor) forwardResultSet(clientIO, backendIO *pnet.PacketIO, request []byte) (uint16, error) {
 	if cp.capability&mysql.ClientDeprecateEOF == 0 {
+		var response []byte
 		// read columns
 		for {
 			var err error

pkg/proxy/backend/cmd_processor_query.go

@@ -18,8 +18,8 @@ import (
 	"encoding/binary"
 
 	gomysql "github.com/go-mysql-org/go-mysql/mysql"
-	pnet "github.com/pingcap/TiProxy/pkg/proxy/net"
 	"github.com/pingcap/TiProxy/lib/util/errors"
+	pnet "github.com/pingcap/TiProxy/pkg/proxy/net"
 	"github.com/pingcap/tidb/parser/mysql"
 	"github.com/siddontang/go/hack"
 )

pkg/proxy/backend/mock_backend_test.go

@@ -25,19 +25,19 @@ import (
 
 type backendConfig struct {
 	// for auth
-	tlsConfig  *tls.Config
-	authPlugin string
-	salt         []byte
-	columns      int
-	loops        int
-	params       int
-	rows         int
-	respondType  respondType // for cmd
-	stmtNum      int
-	capability   uint32
-	status       uint16
-	authSucceed  bool
-	switchAuth   bool
+	tlsConfig   *tls.Config
+	authPlugin  string
+	salt        []byte
+	columns     int
+	loops       int
+	params      int
+	rows        int
+	respondType respondType // for cmd
+	stmtNum     int
+	capability  uint32
+	status      uint16
+	authSucceed bool
+	switchAuth  bool
 	// for both auth and cmd
 	abnormalExit bool
 }

pkg/proxy/backend/mock_client_test.go

@@ -24,20 +24,20 @@ import (
 
 type clientConfig struct {
 	// for auth
-	tlsConfig    *tls.Config
-	sql          string
-	username     string
-	dbName       string
-	authPlugin   string
-	dataBytes    []byte
-	attrs        []byte
-	authData     []byte
-	filePkts     int
-	prepStmtID   int
-	capability   uint32
-	collation    uint8
+	tlsConfig  *tls.Config
+	sql        string
+	username   string
+	dbName     string
+	authPlugin string
+	dataBytes  []byte
+	attrs      []byte
+	authData   []byte
+	filePkts   int
+	prepStmtID int
+	capability uint32
+	collation  uint8
 	// for cmd
-	cmd          byte
+	cmd byte
 	// for both auth and cmd
 	abnormalExit bool
 }
@@ -206,7 +206,7 @@ func (mc *mockClient) requestPrepare(packetIO *pnet.PacketIO) error {
 		}
 	}
 	for i := 0; i < expectedPacketNum; i++ {
-		if response, err = packetIO.ReadPacket(); err != nil {
+		if _, err = packetIO.ReadPacket(); err != nil {
 			return err
 		}
 	}