ci: enable more linters (#815)

Signed-off-by: Matthieu MOREL <[email protected]>
Co-authored-by: James Peach <[email protected]>

.github/workflows/golangci-lint.yaml

@@ -15,4 +15,4 @@ jobs:
           only-new-issues: true
 
           # Optional: golangci-lint command line arguments.
-          args: --timeout=10m0s
+          args: --verbose

.golangci.yml

@@ -1,13 +1,21 @@
-run:
-  timeout: 10m
+issues:
+  fix: true
+  exclude-rules:
+    - linters:
+      - gosec
+      text: 'G101'
+      path: 'pkg/test/resource/v3/secret.go'
+  max-issues-per-linter: 0
+  max-same-issues: 0
 
 linters:
   presets:
   - bugs
 
   enable:
   - bodyclose
-  - gofmt
+  - errorlint
+  - gofumpt
   - goimports
   - gosec
   - misspell
@@ -30,5 +38,5 @@ linters-settings:
   exhaustive:
     default-signifies-exhaustive: true
 
-issues:
-  fix: true
+run:
+  timeout: 10m

Makefile

@@ -44,7 +44,7 @@ lint:
 		--rm \
 		--volume $$(pwd):/src \
 		--workdir /src \
-		golangci/golangci-lint:v1.52.2 \
+		golangci/golangci-lint:latest \
 	golangci-lint -v run
 
 #-----------------

pkg/cache/v3/cache.go

@@ -167,8 +167,10 @@ type RawDeltaResponse struct {
 	marshaledResponse atomic.Value
 }
 
-var _ Response = &RawResponse{}
-var _ DeltaResponse = &RawDeltaResponse{}
+var (
+	_ Response      = &RawResponse{}
+	_ DeltaResponse = &RawDeltaResponse{}
+)
 
 // PassthroughResponse is a pre constructed xDS response that need not go through marshaling transformations.
 type PassthroughResponse struct {
@@ -195,8 +197,10 @@ type DeltaPassthroughResponse struct {
 	ctx context.Context
 }
 
-var _ Response = &PassthroughResponse{}
-var _ DeltaResponse = &DeltaPassthroughResponse{}
+var (
+	_ Response      = &PassthroughResponse{}
+	_ DeltaResponse = &DeltaPassthroughResponse{}
+)
 
 // GetDiscoveryResponse performs the marshaling the first time its called and uses the cached response subsequently.
 // This is necessary because the marshaled response does not change across the calls.
@@ -225,7 +229,7 @@ func (r *RawResponse) GetDiscoveryResponse() (*discovery.DiscoveryResponse, erro
 		marshaledResponse = &discovery.DiscoveryResponse{
 			VersionInfo: r.Version,
 			Resources:   marshaledResources,
-			TypeUrl:     r.Request.TypeUrl,
+			TypeUrl:     r.GetRequest().GetTypeUrl(),
 		}
 
 		r.marshaledResponse.Store(marshaledResponse)
@@ -256,7 +260,7 @@ func (r *RawDeltaResponse) GetDeltaDiscoveryResponse() (*discovery.DeltaDiscover
 			marshaledResources[i] = &discovery.Resource{
 				Name: name,
 				Resource: &anypb.Any{
-					TypeUrl: r.DeltaRequest.TypeUrl,
+					TypeUrl: r.GetDeltaRequest().GetTypeUrl(),
 					Value:   marshaledResource,
 				},
 				Version: version,
@@ -266,7 +270,7 @@ func (r *RawDeltaResponse) GetDeltaDiscoveryResponse() (*discovery.DeltaDiscover
 		marshaledResponse = &discovery.DeltaDiscoveryResponse{
 			Resources:         marshaledResources,
 			RemovedResources:  r.RemovedResources,
-			TypeUrl:           r.DeltaRequest.TypeUrl,
+			TypeUrl:           r.GetDeltaRequest().GetTypeUrl(),
 			SystemVersionInfo: r.SystemVersionInfo,
 		}
 		r.marshaledResponse.Store(marshaledResponse)
@@ -322,14 +326,14 @@ func (r *RawResponse) maybeCreateTTLResource(resource types.ResourceWithTTL) (ty
 			if err != nil {
 				return nil, "", err
 			}
-			rsrc.TypeUrl = r.Request.TypeUrl
+			rsrc.TypeUrl = r.GetRequest().GetTypeUrl()
 			wrappedResource.Resource = rsrc
 		}
 
 		return wrappedResource, deltaResourceTypeURL, nil
 	}
 
-	return resource.Resource, r.Request.TypeUrl, nil
+	return resource.Resource, r.GetRequest().GetTypeUrl(), nil
 }
 
 // GetDiscoveryResponse returns the final passthrough Discovery Response.
@@ -354,19 +358,22 @@ func (r *DeltaPassthroughResponse) GetDeltaRequest() *discovery.DeltaDiscoveryRe
 
 // GetVersion returns the response version.
 func (r *PassthroughResponse) GetVersion() (string, error) {
-	if r.DiscoveryResponse != nil {
-		return r.DiscoveryResponse.VersionInfo, nil
+	discoveryResponse, _ := r.GetDiscoveryResponse()
+	if discoveryResponse != nil {
+		return discoveryResponse.GetVersionInfo(), nil
 	}
 	return "", fmt.Errorf("DiscoveryResponse is nil")
 }
+
 func (r *PassthroughResponse) GetContext() context.Context {
 	return r.ctx
 }
 
 // GetSystemVersion returns the response version.
 func (r *DeltaPassthroughResponse) GetSystemVersion() (string, error) {
-	if r.DeltaDiscoveryResponse != nil {
-		return r.DeltaDiscoveryResponse.SystemVersionInfo, nil
+	deltaDiscoveryResponse, _ := r.GetDeltaDiscoveryResponse()
+	if deltaDiscoveryResponse != nil {
+		return deltaDiscoveryResponse.GetSystemVersionInfo(), nil
 	}
 	return "", fmt.Errorf("DeltaDiscoveryResponse is nil")
 }

pkg/cache/v3/cache_test.go

@@ -4,6 +4,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"google.golang.org/protobuf/encoding/protojson"
 	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/types/known/anypb"
@@ -28,24 +29,24 @@ func TestResponseGetDiscoveryResponse(t *testing.T) {
 	}
 
 	discoveryResponse, err := resp.GetDiscoveryResponse()
-	assert.Nil(t, err)
-	assert.Equal(t, discoveryResponse.VersionInfo, resp.Version)
-	assert.Equal(t, len(discoveryResponse.Resources), 1)
+	require.NoError(t, err)
+	assert.Equal(t, discoveryResponse.GetVersionInfo(), resp.Version)
+	assert.Len(t, discoveryResponse.GetResources(), 1)
 
 	cachedResponse, err := resp.GetDiscoveryResponse()
-	assert.Nil(t, err)
+	require.NoError(t, err)
 	assert.Same(t, discoveryResponse, cachedResponse)
 
 	r := &route.RouteConfiguration{}
-	err = anypb.UnmarshalTo(discoveryResponse.Resources[0], r, proto.UnmarshalOptions{})
-	assert.Nil(t, err)
-	assert.Equal(t, r.Name, resourceName)
+	err = anypb.UnmarshalTo(discoveryResponse.GetResources()[0], r, proto.UnmarshalOptions{})
+	require.NoError(t, err)
+	assert.Equal(t, resourceName, r.GetName())
 }
 
 func TestPassthroughResponseGetDiscoveryResponse(t *testing.T) {
 	routes := []types.Resource{&route.RouteConfiguration{Name: resourceName}}
 	rsrc, err := anypb.New(routes[0])
-	assert.Nil(t, err)
+	require.NoError(t, err)
 	dr := &discovery.DiscoveryResponse{
 		TypeUrl:     resource.RouteType,
 		Resources:   []*anypb.Any{rsrc},
@@ -57,14 +58,14 @@ func TestPassthroughResponseGetDiscoveryResponse(t *testing.T) {
 	}
 
 	discoveryResponse, err := resp.GetDiscoveryResponse()
-	assert.Nil(t, err)
-	assert.Equal(t, discoveryResponse.VersionInfo, resp.DiscoveryResponse.VersionInfo)
-	assert.Equal(t, len(discoveryResponse.Resources), 1)
+	require.NoError(t, err)
+	assert.Equal(t, "v", discoveryResponse.GetVersionInfo())
+	assert.Len(t, discoveryResponse.GetResources(), 1)
 
 	r := &route.RouteConfiguration{}
-	err = anypb.UnmarshalTo(discoveryResponse.Resources[0], r, proto.UnmarshalOptions{})
-	assert.Nil(t, err)
-	assert.Equal(t, r.Name, resourceName)
+	err = anypb.UnmarshalTo(discoveryResponse.GetResources()[0], r, proto.UnmarshalOptions{})
+	require.NoError(t, err)
+	assert.Equal(t, resourceName, r.GetName())
 	assert.Equal(t, discoveryResponse, dr)
 }
 
@@ -78,27 +79,27 @@ func TestHeartbeatResponseGetDiscoveryResponse(t *testing.T) {
 	}
 
 	discoveryResponse, err := resp.GetDiscoveryResponse()
-	assert.Nil(t, err)
-	assert.Equal(t, discoveryResponse.VersionInfo, resp.Version)
-	assert.Equal(t, len(discoveryResponse.Resources), 1)
-	assert.False(t, isTTLResource(discoveryResponse.Resources[0]))
+	require.NoError(t, err)
+	assert.Equal(t, discoveryResponse.GetVersionInfo(), resp.Version)
+	require.Len(t, discoveryResponse.GetResources(), 1)
+	assert.False(t, isTTLResource(discoveryResponse.GetResources()[0]))
 
 	cachedResponse, err := resp.GetDiscoveryResponse()
-	assert.Nil(t, err)
+	require.NoError(t, err)
 	assert.Same(t, discoveryResponse, cachedResponse)
 
 	r := &route.RouteConfiguration{}
-	err = anypb.UnmarshalTo(discoveryResponse.Resources[0], r, proto.UnmarshalOptions{})
-	assert.Nil(t, err)
-	assert.Equal(t, r.Name, resourceName)
+	err = anypb.UnmarshalTo(discoveryResponse.GetResources()[0], r, proto.UnmarshalOptions{})
+	require.NoError(t, err)
+	assert.Equal(t, resourceName, r.GetName())
 }
 
 func isTTLResource(resource *anypb.Any) bool {
 	wrappedResource := &discovery.Resource{}
-	err := protojson.Unmarshal(resource.Value, wrappedResource)
+	err := protojson.Unmarshal(resource.GetValue(), wrappedResource)
 	if err != nil {
 		return false
 	}
 
-	return wrappedResource.Resource == nil
+	return wrappedResource.GetResource() == nil
 }

pkg/cache/v3/delta_test.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"google.golang.org/protobuf/testing/protocmp"
 
 	core "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
@@ -240,7 +241,7 @@ func TestSnapshotDeltaCacheWatchTimeout(t *testing.T) {
 	defer cancel()
 
 	err := c.SetSnapshot(ctx, key, fixture.snapshot())
-	assert.EqualError(t, err, context.Canceled.Error())
+	require.EqualError(t, err, context.Canceled.Error())
 
 	// Now reset the snapshot with a consuming channel. This verifies that if setting the snapshot fails,
 	// we can retry by setting the same snapshot. In other words, we keep the watch open even if we failed
@@ -253,13 +254,13 @@ func TestSnapshotDeltaCacheWatchTimeout(t *testing.T) {
 	}()
 
 	err = c.SetSnapshot(context.WithValue(context.Background(), testKey{}, "bar"), key, fixture.snapshot())
-	assert.NoError(t, err)
+	require.NoError(t, err)
 
 	// The channel should get closed due to the watch trigger.
 	select {
 	case response := <-watchTriggeredCh:
 		// Verify that we pass the context through.
-		assert.Equal(t, response.GetContext().Value(testKey{}), "bar")
+		assert.Equal(t, "bar", response.GetContext().Value(testKey{}))
 	case <-time.After(time.Second):
 		t.Fatalf("timed out")
 	}

pkg/cache/v3/fixtures_test.go

@@ -31,7 +31,6 @@ func (f *fixtureGenerator) snapshot() *cache.Snapshot {
 			rsrc.ExtensionConfigType: {testExtensionConfig},
 		},
 	)
-
 	if err != nil {
 		panic(err.Error())
 	}

pkg/cache/v3/linear.go

@@ -187,7 +187,7 @@ func (cache *LinearCache) respondDelta(request *DeltaRequest, value chan DeltaRe
 	if len(resp.Resources) > 0 || len(resp.RemovedResources) > 0 {
 		if cache.log != nil {
 			cache.log.Debugf("[linear cache] node: %s, sending delta response for typeURL %s with resources: %v removed resources: %v with wildcard: %t",
-				request.GetNode().GetId(), request.TypeUrl, GetResourceNames(resp.Resources), resp.RemovedResources, state.IsWildcard())
+				request.GetNode().GetId(), request.GetTypeUrl(), GetResourceNames(resp.Resources), resp.RemovedResources, state.IsWildcard())
 		}
 		value <- resp
 		return resp
@@ -299,7 +299,7 @@ func (cache *LinearCache) GetResources() map[string]types.Resource {
 }
 
 func (cache *LinearCache) CreateWatch(request *Request, _ stream.StreamState, value chan Response) func() {
-	if request.TypeUrl != cache.typeURL {
+	if request.GetTypeUrl() != cache.typeURL {
 		value <- nil
 		return nil
 	}
@@ -312,8 +312,8 @@ func (cache *LinearCache) CreateWatch(request *Request, _ stream.StreamState, va
 	// strip version prefix if it is present
 	var lastVersion uint64
 	var err error
-	if strings.HasPrefix(request.VersionInfo, cache.versionPrefix) {
-		lastVersion, err = strconv.ParseUint(request.VersionInfo[len(cache.versionPrefix):], 0, 64)
+	if strings.HasPrefix(request.GetVersionInfo(), cache.versionPrefix) {
+		lastVersion, err = strconv.ParseUint(request.GetVersionInfo()[len(cache.versionPrefix):], 0, 64)
 	} else {
 		err = errors.New("mis-matched version prefix")
 	}
@@ -323,11 +323,11 @@ func (cache *LinearCache) CreateWatch(request *Request, _ stream.StreamState, va
 
 	if err != nil {
 		stale = true
-		staleResources = request.ResourceNames
-	} else if len(request.ResourceNames) == 0 {
+		staleResources = request.GetResourceNames()
+	} else if len(request.GetResourceNames()) == 0 {
 		stale = lastVersion != cache.version
 	} else {
-		for _, name := range request.ResourceNames {
+		for _, name := range request.GetResourceNames() {
 			// When a resource is removed, its version defaults 0 and it is not considered stale.
 			if lastVersion < cache.versionVector[name] {
 				stale = true
@@ -340,15 +340,15 @@ func (cache *LinearCache) CreateWatch(request *Request, _ stream.StreamState, va
 		return nil
 	}
 	// Create open watches since versions are up to date.
-	if len(request.ResourceNames) == 0 {
+	if len(request.GetResourceNames()) == 0 {
 		cache.watchAll[value] = struct{}{}
 		return func() {
 			cache.mu.Lock()
 			defer cache.mu.Unlock()
 			delete(cache.watchAll, value)
 		}
 	}
-	for _, name := range request.ResourceNames {
+	for _, name := range request.GetResourceNames() {
 		set, exists := cache.watches[name]
 		if !exists {
 			set = make(watches)
@@ -359,7 +359,7 @@ func (cache *LinearCache) CreateWatch(request *Request, _ stream.StreamState, va
 	return func() {
 		cache.mu.Lock()
 		defer cache.mu.Unlock()
-		for _, name := range request.ResourceNames {
+		for _, name := range request.GetResourceNames() {
 			set, exists := cache.watches[name]
 			if exists {
 				delete(set, value)