Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add golangci lint to github action workflow #1068

Merged
merged 15 commits into from
Aug 31, 2023
Merged
32 changes: 32 additions & 0 deletions .github/workflows/golangci.lint.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
name: CI Lint

on:
pull_request:
branches:
- main
paths:
- "**/*.go"
- ".github/workflows/golangci.lint.yml"

env:
GO_VERSION: "1.20"

jobs:

analysis:
runs-on: ubuntu-latest
steps:

- name: Set up Go
uses: actions/setup-go@v4
with:
go-version: ${{ env.GO_VERSION }}

- name: Check out code into the Go module directory
uses: actions/checkout@v3

- name: golangci-lint
uses: golangci/golangci-lint-action@v3
with:
version: v1.54
args: --timeout=30m --disable=errcheck
4 changes: 0 additions & 4 deletions cmd/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,8 @@ package cmd

import (
"log"
"math/rand"
"os"
"sync"
"time"

"github.com/megaease/easegress/v2/pkg/api"
"github.com/megaease/easegress/v2/pkg/cluster"
Expand All @@ -40,8 +38,6 @@ import (

// RunServer runs Easegress server.
func RunServer() {
rand.Seed(time.Now().UnixNano())

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rand.Seed is deprecated since golang 1.20

opt := option.New()
if err := opt.Parse(); err != nil {
common.Exit(1, err.Error())
Expand Down
3 changes: 1 addition & 2 deletions pkg/cluster/cluster_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@
package cluster

import (
"crypto/rand"
"fmt"
"math/rand"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace with math/rand with crypto/rand for rand.Read

"os"
"path"
"path/filepath"
Expand Down Expand Up @@ -48,7 +48,6 @@ func getRandomString(n int) string {
}

func TestMain(m *testing.M) {
rand.Seed(time.Now().UnixNano())
logger.InitNop()
// logger.InitMock()
tempDir = path.Join(tempDir, getRandomString(6))
Expand Down
12 changes: 5 additions & 7 deletions pkg/filters/headerlookup/headerlookup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"net/http"
"os"
"sort"
"sync"
"testing"
"time"

Expand Down Expand Up @@ -79,9 +78,8 @@ func TestValidate(t *testing.T) {
assert := assert.New(t)
clusterInstance, _ := createClusterAndSyncer()

var mockMap sync.Map
supervisor := supervisor.NewMock(
nil, clusterInstance, mockMap, mockMap, nil, nil, false, nil, nil)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sync.Map contains lock, can't be copy.

nil, clusterInstance, nil, nil, false, nil, nil)

const validYaml = `
name: headerLookup
Expand Down Expand Up @@ -191,9 +189,8 @@ headerSetters:

clusterInstance, syncerChannel := createClusterAndSyncer()

var mockMap sync.Map
supervisor := supervisor.NewMock(
nil, clusterInstance, mockMap, mockMap, nil, nil, false, nil, nil)
nil, clusterInstance, nil, nil, false, nil, nil)

// let's put data to 'foobar'
foobar := `
Expand Down Expand Up @@ -225,6 +222,7 @@ extra-entry: "extra"
assert.Equal("123456789", hdr1)

hl, err = createHeaderLookup(config, hl, supervisor)
assert.Nil(err)
ctx, header = prepareCtxAndHeader(t)

// update key-value store
Expand All @@ -245,6 +243,7 @@ extra-entry: "extra"
assert.Equal("77341", header.Get("user-ext-id"))

hl, err = createHeaderLookup(config, hl, supervisor)
assert.Nil(err)
ctx, header = prepareCtxAndHeader(t)
header.Set("X-AUTH-USER", "foobar")
// delete foobar completely
Expand Down Expand Up @@ -274,9 +273,8 @@ headerSetters:
headerKey: "user-ext-id"
`
clusterInstance, _ := createClusterAndSyncer()
var mockMap sync.Map
supervisor := supervisor.NewMock(
nil, clusterInstance, mockMap, mockMap, nil, nil, false, nil, nil)
nil, clusterInstance, nil, nil, false, nil, nil)
bobbanana := `
ext-id: "333"
extra-entry: "extra"
Expand Down
3 changes: 0 additions & 3 deletions pkg/filters/meshadaptor/meshadaptor.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
proxy "github.com/megaease/easegress/v2/pkg/filters/proxies/httpproxy"
"github.com/megaease/easegress/v2/pkg/protocols/httpprot"
"github.com/megaease/easegress/v2/pkg/protocols/httpprot/httpheader"
"github.com/megaease/easegress/v2/pkg/util/pathadaptor"
)

const (
Expand Down Expand Up @@ -52,8 +51,6 @@
// MeshAdaptor is filter MeshAdaptor.
MeshAdaptor struct {
spec *Spec

pa *pathadaptor.PathAdaptor
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not used.

}

// Spec is HTTPAdaptor Spec.
Expand Down Expand Up @@ -92,7 +89,7 @@
}

// Inherit inherits previous generation of MeshAdaptor.
func (ra *MeshAdaptor) Inherit(previousGeneration filters.Filter) {

Check warning on line 92 in pkg/filters/meshadaptor/meshadaptor.go

View workflow job for this annotation

GitHub Actions / analysis

parameter 'previousGeneration' seems to be unused, consider removing or renaming it as _
ra.Init()
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/filters/oidcadaptor/oidcadaptor.go
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ func (o *OIDCAdaptor) handleOIDCCallback(ctx *context.Context) string {
if err != nil {
return filterResp(rw, http.StatusForbidden, err.Error())
}
oidcToken, err := o.fetchOIDCToken(authCode, state, spec, err, rw, req)
oidcToken, err := o.fetchOIDCToken(authCode, state, spec, rw, req)
if err != nil {
return errorResp(rw, "fetch OIDC token error: "+err.Error())
}
Expand Down Expand Up @@ -293,7 +293,7 @@ func (o *OIDCAdaptor) handleOIDCCallback(ctx *context.Context) string {
return ""
}

func (o *OIDCAdaptor) fetchOIDCToken(authCode string, state string, spec *Spec, err error, rw *httpprot.Response, req *httpprot.Request) (*oidcIDToken, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this err is assigned before use in this function, so remove it.

func (o *OIDCAdaptor) fetchOIDCToken(authCode string, state string, spec *Spec, rw *httpprot.Response, req *httpprot.Request) (*oidcIDToken, error) {
// client_secret_post || client_secret_basic
tokenFormData := url.Values{
"client_id": {o.spec.ClientID},
Expand Down
15 changes: 7 additions & 8 deletions pkg/filters/opafilter/opafilter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,13 @@ func createOPAFilter(yamlConfig string, prev *OPAFilter, supervisor *supervisor.
}

type testCase struct {
req func() *http.Request
status int
shouldHandlerError bool
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not used.

shouldRegoError bool
readBody bool
policy string
defaultStatus int
includedHeaders string
req func() *http.Request
status int
shouldRegoError bool
readBody bool
policy string
defaultStatus int
includedHeaders string
}

func TestOpaPolicyInFilter(t *testing.T) {
Expand Down
4 changes: 2 additions & 2 deletions pkg/filters/proxies/grpcproxy/codec.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func (GrpcCodec) Unmarshal(data []byte, v interface{}) error {
return proto.Unmarshal(data, v.(proto.Message))
}

// String return codec name
func (GrpcCodec) String() string {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change interface from grpc.Codec to grpc/encoding.Codec, method change.

// Name return codec name
func (GrpcCodec) Name() string {
return codecName
}
2 changes: 1 addition & 1 deletion pkg/filters/proxies/grpcproxy/codec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import (

func TestGrpcCodecString(t *testing.T) {
grpcCodec := GrpcCodec{}
assert.Equal(t, codecName, grpcCodec.String())
assert.Equal(t, codecName, grpcCodec.Name())
}

func TestCodecMarshalUnmarshal(t *testing.T) {
Expand Down
56 changes: 36 additions & 20 deletions pkg/filters/proxies/grpcproxy/pool.go
Original file line number Diff line number Diff line change
Expand Up @@ -345,8 +345,8 @@ func (sp *ServerPool) biTransport(ctx *serverPoolContext, proxyAsClientStream gr
// Explicitly *do not Close* c2sErrChan and c2sErrChan, otherwise the select below will not terminate.
// Channels do not have to be closed, it is just a control flow mechanism, see
// https://groups.google.com/forum/#!msg/golang-nuts/pZwdYRGxCIk/qpbHxRRPJdUJ
c2sErrChan := sp.forwardE2E(ctx.stdr, proxyAsClientStream, nil)
s2cErrChan := sp.forwardE2E(proxyAsClientStream, ctx.stdw, ctx.resp.RawHeader())
c2sErrChan := sp.forwardS2C(ctx.stdr, proxyAsClientStream, nil)
s2cErrChan := sp.forwardC2S(proxyAsClientStream, ctx.stdw, ctx.resp.RawHeader())
// We don't know which side is going to stop sending first, so we need a select between the two.
for {
select {
Expand Down Expand Up @@ -381,7 +381,7 @@ func (sp *ServerPool) buildOutputResponse(spCtx *serverPoolContext, s *status.St
spCtx.SetOutputResponse(spCtx.resp)
}

func (sp *ServerPool) forwardE2E(src grpc.Stream, dst grpc.Stream, header *grpcprot.Header) chan error {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grpc.Stream is deprecated, so change it grpc.ClientStream and grpc.ServerStream. Split forwardE2E to forwardC2S and forwardS2C.

func (sp *ServerPool) forwardC2S(src grpc.ClientStream, dst grpc.ServerStream, header *grpcprot.Header) chan error {
ret := make(chan error, 1)
go func() {
f := &emptypb.Empty{}
Expand All @@ -391,24 +391,40 @@ func (sp *ServerPool) forwardE2E(src grpc.Stream, dst grpc.Stream, header *grpcp
return
}
if i == 0 {
if cs, ok := src.(grpc.ClientStream); ok {
// This is a bit of a hack, but client to server headers are only readable after first client msg is
// received but must be written to server stream before the first msg is flushed.
// This is the only place to do it nicely.
md, err := cs.Header()
if err != nil {
ret <- err
return
}
if header != nil {
md = metadata.Join(header.GetMD(), md)
}

if err = dst.(grpc.ServerStream).SendHeader(md); err != nil {
ret <- err
return
}
// This is a bit of a hack, but client to server headers are only readable after first client msg is
// received but must be written to server stream before the first msg is flushed.
// This is the only place to do it nicely.
md, err := src.Header()
if err != nil {
ret <- err
return
}
if header != nil {
md = metadata.Join(header.GetMD(), md)
}

if err = dst.SendHeader(md); err != nil {
ret <- err
return
}
}
if err := dst.SendMsg(f); err != nil {
ret <- err
return
}
}
}()
return ret
}

func (sp *ServerPool) forwardS2C(src grpc.ServerStream, dst grpc.ClientStream, header *grpcprot.Header) chan error {
ret := make(chan error, 1)
go func() {
f := &emptypb.Empty{}
for i := 0; ; i++ {
if err := src.RecvMsg(f); err != nil {
ret <- err // this can be io.EOF which is happy case
return
}
if err := dst.SendMsg(f); err != nil {
ret <- err
Expand Down
5 changes: 3 additions & 2 deletions pkg/filters/proxies/grpcproxy/proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,9 @@ var (
}
defaultDialOpts = []grpc.DialOption{
grpc.WithTransportCredentials(insecure.NewCredentials()),
grpc.WithCodec(&GrpcCodec{}),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grpc.WithCodec is deprecated. Update it to grpc.WithDefaultCallOptions(grpc.ForceCodec(_)) by following official doc.

grpc.WithBlock()}
grpc.WithDefaultCallOptions(grpc.ForceCodec(&GrpcCodec{})),
grpc.WithBlock(),
}
)

var _ filters.Filter = (*Proxy)(nil)
Expand Down
7 changes: 3 additions & 4 deletions pkg/filters/proxies/httpproxy/pool_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ package httpproxy

import (
"net/http"
"sync"
"testing"

"github.com/megaease/easegress/v2/pkg/context"
Expand Down Expand Up @@ -93,7 +92,7 @@ servers:
assert.NoError(spec.Validate())

p := &Proxy{}
p.super = supervisor.NewMock(option.New(), nil, sync.Map{}, sync.Map{}, nil,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sync.Map{} cant be copied because of lock.

p.super = supervisor.NewMock(option.New(), nil, nil,
nil, false, nil, nil)
sp := NewServerPool(p, spec, "test")
policies := map[string]resilience.Policy{}
Expand Down Expand Up @@ -135,7 +134,7 @@ servers:
assert.NoError(spec.Validate())

p := &Proxy{}
p.super = supervisor.NewMock(option.New(), nil, sync.Map{}, sync.Map{}, nil,
p.super = supervisor.NewMock(option.New(), nil, nil,
nil, false, nil, nil)
sp := NewServerPool(p, spec, "test")
spCtx := &serverPoolContext{
Expand Down Expand Up @@ -178,7 +177,7 @@ func TestCopyCORSHeaders(t *testing.T) {
dst.Add("X-Dst", "dst")

p := &Proxy{}
p.super = supervisor.NewMock(option.New(), nil, sync.Map{}, sync.Map{}, nil,
p.super = supervisor.NewMock(option.New(), nil, nil,
nil, false, nil, nil)
sp := NewServerPool(p, &ServerPoolSpec{}, "test")
dst = sp.mergeResponseHeader(dst, src)
Expand Down
3 changes: 1 addition & 2 deletions pkg/filters/proxies/httpproxy/proxy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
"net/http/httptest"
"os"
"strings"
"sync"
"sync/atomic"
"testing"
"time"
Expand Down Expand Up @@ -58,7 +57,7 @@ func newTestProxy(yamlConfig string, assert *assert.Assertions) *Proxy {

proxy := kind.CreateInstance(spec).(*Proxy)

proxy.super = supervisor.NewMock(option.New(), nil, sync.Map{}, sync.Map{}, nil,
proxy.super = supervisor.NewMock(option.New(), nil, nil,
nil, false, nil, nil)

proxy.Init()
Expand Down
9 changes: 2 additions & 7 deletions pkg/filters/proxies/loadbalance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ package proxies

import (
"fmt"
"math/rand"
"net/http"
"os"
"sync"
Expand Down Expand Up @@ -85,8 +84,6 @@ func TestGeneralLoadBalancer(t *testing.T) {
}

func TestRandomLoadBalancePolicy(t *testing.T) {
rand.Seed(0)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deprecated.


counter := [10]int{}
servers := prepareServers(10)

Expand Down Expand Up @@ -126,23 +123,21 @@ func TestRoundRobinLoadBalancePolicy(t *testing.T) {
}

func TestWeightedRandomLoadBalancePolicy(t *testing.T) {
rand.Seed(0)

counter := [10]int{}
servers := prepareServers(10)

lb := NewGeneralLoadBalancer(&LoadBalanceSpec{Policy: LoadBalancePolicyWeightedRandom}, servers)
lb.Init(nil, nil, nil)

for i := 0; i < 1000; i++ {
for i := 0; i < 50000; i++ {
svr := lb.ChooseServer(nil)
counter[svr.Weight-1]++
}

v := 0
for i := 0; i < 10; i++ {
if v >= counter[i] {
t.Error("possibility is not weighted even")
t.Errorf("possibility is not weighted even %v", counter)
}
v = counter[i]
}
Expand Down
Loading
Loading