Lint Cleanup (#452)

* Resolves most go staticcheck errors. * Replaces a few deprecated packages / functions. * Makes some minor changes to error responses on the /json endpoint. Signed-off-by: Charlie Crawford <[email protected]>
envoyproxy · Oct 9, 2023 · b979623 · b979623
1 parent a34dbf9
commit b979623
Show file tree

Hide file tree

Showing 14 changed files with 100 additions and 93 deletions.
diff --git a/src/client_cmd/main.go b/src/client_cmd/main.go
@@ -13,6 +13,7 @@ import (
 	"golang.org/x/net/context"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/credentials"
+	"google.golang.org/grpc/credentials/insecure"
 
 	"github.com/envoyproxy/ratelimit/src/utils"
 
@@ -95,7 +96,7 @@ func main() {
 		tlsConfig := utils.TlsConfigFromFiles(*grpcTlsCertFile, *grpcTlsKeyFile, *grpcServerTlsCACert, utils.ServerCA, false)
 		dialOptions = append(dialOptions, grpc.WithTransportCredentials(credentials.NewTLS(tlsConfig)))
 	} else {
-		dialOptions = append(dialOptions, grpc.WithInsecure())
+		dialOptions = append(dialOptions, grpc.WithTransportCredentials(insecure.NewCredentials()))
 	}
 	conn, err := grpc.Dial(*dialString, dialOptions...)
 	if err != nil {
@@ -106,9 +107,8 @@ func main() {
 	defer conn.Close()
 	c := pb.NewRateLimitServiceClient(conn)
 	desc := make([]*pb_struct.RateLimitDescriptor, len(descriptorsValue.descriptors))
-	for i, v := range descriptorsValue.descriptors {
-		desc[i] = v
-	}
+	copy(desc, descriptorsValue.descriptors)
+
 	response, err := c.ShouldRateLimit(
 		context.Background(),
 		&pb.RateLimitRequest{

diff --git a/src/config/config_impl.go b/src/config/config_impl.go
@@ -151,7 +151,7 @@ func (this *rateLimitDescriptor) loadDescriptors(config RateLimitConfigToLoad, p
 				if validUnit {
 					panic(newRateLimitConfigError(
 						config.Name,
-						fmt.Sprintf("should not specify rate limit unit when unlimited")))
+						"should not specify rate limit unit when unlimited"))
 				}
 			} else if !validUnit {
 				panic(newRateLimitConfigError(
@@ -234,7 +234,7 @@ func validateYamlKeys(fileName string, config_map map[interface{}]interface{}) {
 		// the yaml's keys we don't panic here.
 		case nil:
 		default:
-			errorText := fmt.Sprintf("error checking config")
+			errorText := "error checking config"
 			logger.Debugf(errorText)
 			panic(newRateLimitConfigError(fileName, errorText))
 		}

diff --git a/src/config_check_cmd/main.go b/src/config_check_cmd/main.go
@@ -3,7 +3,6 @@ package main
 import (
 	"flag"
 	"fmt"
-	"io/ioutil"
 	"os"
 	"path/filepath"
 
@@ -36,7 +35,7 @@ func main() {
 	fmt.Printf("checking rate limit configs...\n")
 	fmt.Printf("loading config directory: %s\n", *configDirectory)
 
-	files, err := ioutil.ReadDir(*configDirectory)
+	files, err := os.ReadDir(*configDirectory)
 	if err != nil {
 		fmt.Printf("error opening directory %s: %s\n", *configDirectory, err.Error())
 		os.Exit(1)
@@ -46,7 +45,7 @@ func main() {
 	for _, file := range files {
 		finalPath := filepath.Join(*configDirectory, file.Name())
 		fmt.Printf("opening config file: %s\n", finalPath)
-		bytes, err := ioutil.ReadFile(finalPath)
+		bytes, err := os.ReadFile(finalPath)
 		if err != nil {
 			fmt.Printf("error reading file %s: %s\n", finalPath, err.Error())
 			os.Exit(1)

diff --git a/src/limiter/base_limiter.go b/src/limiter/base_limiter.go
@@ -73,10 +73,7 @@ func (this *BaseRateLimiter) IsOverLimitWithLocalCache(key string) bool {
 
 func (this *BaseRateLimiter) IsOverLimitThresholdReached(limitInfo *LimitInfo) bool {
 	limitInfo.overLimitThreshold = limitInfo.limit.Limit.RequestsPerUnit
-	if limitInfo.limitAfterIncrease > limitInfo.overLimitThreshold {
-		return true
-	}
-	return false
+	return limitInfo.limitAfterIncrease > limitInfo.overLimitThreshold
 }
 
 // Generates response descriptor status based on cache key, over the limit with local cache, over the limit and

diff --git a/src/redis/cache_impl.go b/src/redis/cache_impl.go
@@ -18,8 +18,8 @@ func NewRateLimiterCacheImplFromSettings(s settings.Settings, localCache *freeca
 		perSecondPool = NewClientImpl(srv.Scope().Scope("redis_per_second_pool"), s.RedisPerSecondTls, s.RedisPerSecondAuth, s.RedisPerSecondSocketType,
 			s.RedisPerSecondType, s.RedisPerSecondUrl, s.RedisPerSecondPoolSize, s.RedisPerSecondPipelineWindow, s.RedisPerSecondPipelineLimit, s.RedisTlsConfig, s.RedisHealthCheckActiveConnection, srv)
 	}
-	var otherPool Client
-	otherPool = NewClientImpl(srv.Scope().Scope("redis_pool"), s.RedisTls, s.RedisAuth, s.RedisSocketType, s.RedisType, s.RedisUrl, s.RedisPoolSize,
+
+	otherPool := NewClientImpl(srv.Scope().Scope("redis_pool"), s.RedisTls, s.RedisAuth, s.RedisSocketType, s.RedisType, s.RedisUrl, s.RedisPoolSize,
 		s.RedisPipelineWindow, s.RedisPipelineLimit, s.RedisTlsConfig, s.RedisHealthCheckActiveConnection, srv)
 
 	return NewFixedRateLimitCacheImpl(

diff --git a/src/redis/driver_impl.go b/src/redis/driver_impl.go
@@ -119,7 +119,7 @@ func NewClientImpl(scope stats.Scope, useTls bool, auth, redisSocketType, redisT
 		client, err = poolFunc(redisSocketType, url)
 	case "cluster":
 		urls := strings.Split(url, ",")
-		if implicitPipelining == false {
+		if !implicitPipelining {
 			panic(RedisError("Implicit Pipelining must be enabled to work with Redis Cluster Mode. Set values for REDIS_PIPELINE_WINDOW or REDIS_PIPELINE_LIMIT to enable implicit pipelining"))
 		}
 		logger.Warnf("Creating cluster with urls %v", urls)

diff --git a/src/server/health.go b/src/server/health.go
@@ -33,7 +33,7 @@ const (
 func areAllComponentsHealthy(healthMap map[string]bool) bool {
 	allComponentsHealthy := true
 	for _, value := range healthMap {
-		if value == false {
+		if !value {
 			allComponentsHealthy = false
 			break
 		}

diff --git a/src/server/server_impl.go b/src/server/server_impl.go
@@ -1,7 +1,6 @@
 package server
 
 import (
-	"bytes"
 	"context"
 	"expvar"
 	"fmt"
@@ -18,13 +17,13 @@ import (
 
 	"google.golang.org/grpc/credentials"
 	"google.golang.org/grpc/keepalive"
+	"google.golang.org/protobuf/encoding/protojson"
 
 	"github.com/envoyproxy/ratelimit/src/provider"
 	"github.com/envoyproxy/ratelimit/src/stats"
 
 	"github.com/coocood/freecache"
 	pb "github.com/envoyproxy/go-control-plane/envoy/service/ratelimit/v3"
-	"github.com/golang/protobuf/jsonpb"
 	"github.com/gorilla/mux"
 	reuseport "github.com/kavu/go_reuseport"
 	"github.com/lyft/goruntime/loader"
@@ -79,24 +78,28 @@ func (server *server) AddDebugHttpEndpoint(path string, help string, handler htt
 // example usage from cURL with domain "dummy" and descriptor "perday":
 // echo '{"domain": "dummy", "descriptors": [{"entries": [{"key": "perday"}]}]}' | curl -vvvXPOST --data @/dev/stdin localhost:8080/json
 func NewJsonHandler(svc pb.RateLimitServiceServer) func(http.ResponseWriter, *http.Request) {
-	// Default options include enums as strings and no identation.
-	m := &jsonpb.Marshaler{}
-
 	return func(writer http.ResponseWriter, request *http.Request) {
 		var req pb.RateLimitRequest
 
 		ctx := context.Background()
 
-		if err := jsonpb.Unmarshal(request.Body, &req); err != nil {
+		body, err := io.ReadAll(request.Body)
+		if err != nil {
+			logger.Warnf("error: %s", err.Error())
+			writeHttpStatus(writer, http.StatusBadRequest)
+			return
+		}
+
+		if err := protojson.Unmarshal(body, &req); err != nil {
 			logger.Warnf("error: %s", err.Error())
-			http.Error(writer, err.Error(), http.StatusBadRequest)
+			writeHttpStatus(writer, http.StatusBadRequest)
 			return
 		}
 
 		resp, err := svc.ShouldRateLimit(ctx, &req)
 		if err != nil {
 			logger.Warnf("error: %s", err.Error())
-			http.Error(writer, err.Error(), http.StatusBadRequest)
+			writeHttpStatus(writer, http.StatusBadRequest)
 			return
 		}
 
@@ -109,12 +112,16 @@ func NewJsonHandler(svc pb.RateLimitServiceServer) func(http.ResponseWriter, *ht
 		defer span.End()
 
 		logger.Debugf("resp:%s", resp)
+		if resp == nil {
+			logger.Error("nil response")
+			writeHttpStatus(writer, http.StatusInternalServerError)
+			return
+		}
 
-		buf := bytes.NewBuffer(nil)
-		err = m.Marshal(buf, resp)
+		jsonResp, err := protojson.Marshal(resp)
 		if err != nil {
 			logger.Errorf("error marshaling proto3 to json: %s", err.Error())
-			http.Error(writer, "error marshaling proto3 to json: "+err.Error(), http.StatusInternalServerError)
+			writeHttpStatus(writer, http.StatusInternalServerError)
 			return
 		}
 
@@ -124,10 +131,14 @@ func NewJsonHandler(svc pb.RateLimitServiceServer) func(http.ResponseWriter, *ht
 		} else if resp.OverallCode == pb.RateLimitResponse_OVER_LIMIT {
 			writer.WriteHeader(http.StatusTooManyRequests)
 		}
-		writer.Write(buf.Bytes())
+		writer.Write(jsonResp)
 	}
 }
 
+func writeHttpStatus(writer http.ResponseWriter, code int) {
+	http.Error(writer, http.StatusText(code), code)
+}
+
 func getProviderImpl(s settings.Settings, statsManager stats.Manager, rootStore gostats.Store) provider.RateLimitConfigProvider {
 	switch s.ConfigType {
 	case "FILE":

diff --git a/src/stats/manager.go b/src/stats/manager.go
@@ -2,7 +2,6 @@ package stats
 
 import (
 	gostats "github.com/lyft/gostats"
-	stats "github.com/lyft/gostats"
 )
 
 // Manager is the interface that wraps initialization of stat structures.
@@ -17,7 +16,7 @@ type Manager interface {
 	// Multiple calls to this method are idempotent.
 	NewServiceStats() ServiceStats
 	// Returns the stats.Store wrapped by the Manager.
-	GetStatsStore() stats.Store
+	GetStatsStore() gostats.Store
 }
 
 type ManagerImpl struct {

diff --git a/test/common/common.go b/test/common/common.go
@@ -12,8 +12,8 @@ import (
 	"testing"
 	"time"
 
-	"github.com/golang/protobuf/proto"
 	"github.com/stretchr/testify/assert"
+	"google.golang.org/protobuf/proto"
 
 	pb_struct "github.com/envoyproxy/go-control-plane/envoy/extensions/common/ratelimit/v3"
 	pb "github.com/envoyproxy/go-control-plane/envoy/service/ratelimit/v3"
@@ -111,7 +111,7 @@ func startCacheProcess(ctx context.Context, command string, args []string, port
 
 	if err1 != nil || err2 != nil {
 		cancel()
-		return nil, fmt.Errorf("Problem starting %s subprocess: %v / %v", command, err1, err2)
+		return nil, fmt.Errorf("problem starting %s subprocess: %v / %v", command, err1, err2)
 	}
 
 	// You'd think cmd.Stdout = os.Stdout would make more sense here, but
@@ -127,13 +127,13 @@ func startCacheProcess(ctx context.Context, command string, args []string, port
 	err := cmd.Start()
 	if err != nil {
 		cancel()
-		return nil, fmt.Errorf("Problem starting %s subprocess: %v", command, err)
+		return nil, fmt.Errorf("problem starting %s subprocess: %v", command, err)
 	}
 
 	err = WaitForTcpPort(ctx, port, 1*time.Second)
 	if err != nil {
 		cancel()
-		return nil, fmt.Errorf("Timed out waiting for %s to start up and accept connections: %v", command, err)
+		return nil, fmt.Errorf("timed out waiting for %s to start up and accept connections: %v", command, err)
 	}
 
 	return func() {