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

Brushes off some lint #452

Merged
merged 1 commit into from
Oct 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/client_cmd/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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 {
Expand All @@ -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{
Expand Down
4 changes: 2 additions & 2 deletions src/config/config_impl.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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))
}
Expand Down
5 changes: 2 additions & 3 deletions src/config_check_cmd/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package main
import (
"flag"
"fmt"
"io/ioutil"
"os"
"path/filepath"

Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down
5 changes: 1 addition & 4 deletions src/limiter/base_limiter.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions src/redis/cache_impl.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
2 changes: 1 addition & 1 deletion src/redis/driver_impl.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion src/server/health.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
35 changes: 23 additions & 12 deletions src/server/server_impl.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package server

import (
"bytes"
"context"
"expvar"
"fmt"
Expand All @@ -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"
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason we're removing the more descriptive error msgs in the responses? Is it for consistency, security?

Copy link
Contributor Author

@ChuckCrawford ChuckCrawford Oct 9, 2023

Choose a reason for hiding this comment

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

Ah my apologies! I have updated the PR description to provide more clarity around why I made these overall changes here.

In this exact line you commented on we are still doing the same thing; I just refactored it into a shared function for consistency.

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
}

Expand All @@ -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
}

Expand All @@ -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":
Expand Down
3 changes: 1 addition & 2 deletions src/stats/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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 {
Expand Down
8 changes: 4 additions & 4 deletions test/common/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand All @@ -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() {
Expand Down
Loading
Loading