-
Notifications
You must be signed in to change notification settings - Fork 99
test: Add test for global rate limiting with load balancing #207
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,7 @@ import ( | |
"os" | ||
"strings" | ||
"testing" | ||
"time" | ||
|
||
guber "github.com/mailgun/gubernator/v2" | ||
"github.com/mailgun/gubernator/v2/cluster" | ||
|
@@ -34,6 +35,10 @@ import ( | |
"github.com/prometheus/common/model" | ||
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/require" | ||
|
||
"google.golang.org/grpc" | ||
"google.golang.org/grpc/credentials/insecure" | ||
"google.golang.org/grpc/resolver" | ||
json "google.golang.org/protobuf/encoding/protojson" | ||
) | ||
|
||
|
@@ -859,6 +864,62 @@ func TestGlobalRateLimits(t *testing.T) { | |
}) | ||
} | ||
|
||
func TestGlobalRateLimitsWithLoadBalancing(t *testing.T) { | ||
owner := cluster.PeerAt(2).GRPCAddress | ||
peer := cluster.PeerAt(0).GRPCAddress | ||
assert.NotEqual(t, owner, peer) | ||
|
||
dialOpts := []grpc.DialOption{ | ||
grpc.WithResolvers(newStaticBuilder()), | ||
grpc.WithTransportCredentials(insecure.NewCredentials()), | ||
grpc.WithDefaultServiceConfig(`{"loadBalancingConfig": [{"round_robin":{}}]}`), | ||
} | ||
|
||
address := fmt.Sprintf("static:///%s,%s", owner, peer) | ||
conn, err := grpc.DialContext(context.Background(), address, dialOpts...) | ||
Comment on lines
+878
to
+879
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you clarify what this is doing? it's not clear to me. are you creating a grpc connection to the owner or the peer? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is creating a static pool of servers to communicate with and creating the connection There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the connection is to which server? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. between the owner and the peer |
||
require.NoError(t, err) | ||
|
||
client := guber.NewV1Client(conn) | ||
|
||
sendHit := func(status guber.Status, assertion func(resp *guber.RateLimitResp), i int) string { | ||
ctx, cancel := context.WithTimeout(context.Background(), clock.Hour*5) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this timeout is waaaay too high. Github actions will not run for more than 10 minutes by default There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just to be clear, this test wasnt ready to be merged as is, hence why it was in draft :) - it was simply to reproduce the culprit behaviour that was reported in the bug. |
||
defer cancel() | ||
resp, err := client.GetRateLimits(ctx, &guber.GetRateLimitsReq{ | ||
Requests: []*guber.RateLimitReq{ | ||
{ | ||
Name: "test_global", | ||
UniqueKey: "account:12345", | ||
Algorithm: guber.Algorithm_LEAKY_BUCKET, | ||
Behavior: guber.Behavior_GLOBAL, | ||
Duration: guber.Minute * 5, | ||
Hits: 1, | ||
Limit: 2, | ||
}, | ||
}, | ||
}) | ||
require.NoError(t, err, i) | ||
gotResp := resp.Responses[0] | ||
assert.Equal(t, "", gotResp.GetError(), i) | ||
assert.Equal(t, status, gotResp.GetStatus(), i) | ||
|
||
if assertion != nil { | ||
assertion(gotResp) | ||
} | ||
|
||
return gotResp.GetMetadata()["owner"] | ||
} | ||
|
||
// Send two hits that should be processed by the owner and the peer and deplete the limit | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
are you asserting this anywhere? I don't see it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no in this case im relying on the grpc load balancer we passed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but we can and should check in order to harden the test |
||
sendHit(guber.Status_UNDER_LIMIT, nil, 1) | ||
sendHit(guber.Status_UNDER_LIMIT, nil, 2) | ||
// sleep to ensure the async forward has occurred and state should be shared | ||
time.Sleep(time.Second * 5) | ||
Comment on lines
+915
to
+916
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think (not sure) you can control this via setting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or, use |
||
|
||
for i := 0; i < 10; i++ { | ||
sendHit(guber.Status_OVER_LIMIT, nil, i+2) | ||
} | ||
} | ||
|
||
func getMetricRequest(t testutil.TestingT, url string, name string) *model.Sample { | ||
resp, err := http.Get(url) | ||
require.NoError(t, err) | ||
|
@@ -1273,3 +1334,46 @@ func getMetric(t testutil.TestingT, in io.Reader, name string) *model.Sample { | |
} | ||
return nil | ||
} | ||
|
||
// staticBuilder implements the `resolver.Builder` interface. | ||
type staticBuilder struct{} | ||
|
||
func newStaticBuilder() resolver.Builder { | ||
return &staticBuilder{} | ||
} | ||
|
||
func (sb *staticBuilder) Build(target resolver.Target, cc resolver.ClientConn, _ resolver.BuildOptions) (resolver.Resolver, error) { | ||
var resolverAddrs []resolver.Address | ||
for _, address := range strings.Split(target.Endpoint(), ",") { | ||
resolverAddrs = append(resolverAddrs, resolver.Address{ | ||
Addr: address, | ||
ServerName: address, | ||
}) | ||
|
||
} | ||
r, err := newStaticResolver(cc, resolverAddrs) | ||
if err != nil { | ||
return nil, err | ||
} | ||
return r, nil | ||
} | ||
|
||
func (sb *staticBuilder) Scheme() string { | ||
return "static" | ||
} | ||
|
||
type staticResolver struct { | ||
cc resolver.ClientConn | ||
} | ||
|
||
func newStaticResolver(cc resolver.ClientConn, addresses []resolver.Address) (resolver.Resolver, error) { | ||
err := cc.UpdateState(resolver.State{Addresses: addresses}) | ||
if err != nil { | ||
return nil, err | ||
} | ||
return &staticResolver{cc: cc}, nil | ||
} | ||
|
||
func (sr *staticResolver) ResolveNow(_ resolver.ResolveNowOptions) {} | ||
|
||
func (sr *staticResolver) Close() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a non-maintainer of this repo, it looks like a code smell to me that the easiest way to assert this behavior is through a functional test and not a unit test. 🤔