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

Fix matching scheme #6

Merged
merged 9 commits into from
Feb 6, 2017
Merged
Show file tree
Hide file tree
Changes from 7 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
61 changes: 52 additions & 9 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
- [Example 1](#example-1)
- [Example 2](#example-2)
- [Example 3](#example-3)
- [Example 4](#example-4)
- [Loading Configuration](#loading-configuration)
- [Rate limit statistics](#rate-limit-statistics)
- [Debug Port](#debug-port)
Expand Down Expand Up @@ -61,9 +62,9 @@ The rate limit configuration file format is YAML (mainly so that comments are su

### Definitions

* **Domain:** A domain is a container for a set of rate limits. All domains known to the rate limit service must be
* **Domain:** A domain is a container for a set of rate limits. All domains known to the Ratelimit service must be
globally unique. They serve as a way for different teams/projects to have rate limit configurations that don't conflict.
* **Descriptor:** A descriptor is a list of key/value pairs owned by a domain that the rate limit service uses to
* **Descriptor:** A descriptor is a list of key/value pairs owned by a domain that the Ratelimit service uses to
select the correct rate limit to use when limiting. Descriptors are case-sensitive. Examples of descriptors are:
* ("database", "users")
* ("message_type", "marketing"),("to_number","2061234567")
Expand Down Expand Up @@ -110,7 +111,7 @@ future based on customer demand.

Let's start with a simple example:

```
```yaml
domain: mongo_cps
descriptors:
- key: database
Expand All @@ -135,7 +136,7 @@ request per second rate limit.

A slightly more complex example:

```
```yaml
domain: messaging
descriptors:
# Only allow 5 marketing messages a day
Expand Down Expand Up @@ -178,13 +179,14 @@ RateLimitRequest:
descriptor: ("to_number", "2061111111")
```

And the service with rate limit against *all* matching rules and return an aggregate result.
And the service with rate limit against *all* matching rules and return an aggregate result; a logical OR of all
Copy link
Contributor

Choose a reason for hiding this comment

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

will instead of with?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

the individual rate limit decisions.

#### Example 3

One last example to illustrate matching order.
An example to illustrate matching order.

```
```yaml
domain: edge_proxy_per_ip
descriptors:
- key: ip_address
Expand All @@ -205,12 +207,53 @@ be configured to make a rate limit service call with the descriptor ("ip_address
get 10 requests per second as
would any other IP. However, the configuration also contains a second configuration that explicitly defines a
value along with the same key. If the descriptor ("ip_address", "50.0.0.5") is received, the service will
*attempt the most specific match possible*. This means both the most nested matching descriptor entry, as well as
the most specific at any descriptor list level. Thus, key/value is always attempted as a match before just key.
*attempt the most specific match possible*. This means
the most specific descriptor at the same level as your request. Thus, key/value is always attempted as a match before just key.

#### Example 4

The Ratelimit service matches requests to configuration entries with the same level, i.e
same number of tuples in the request's descriptor as nested levels of descriptors
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should this same level description go up higher? I don't know...

Copy link
Member Author

@junr03 junr03 Feb 4, 2017

Choose a reason for hiding this comment

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

You could argue for or against. Each example explains something about matching, and how this works so it takes all four examples to understand.

in the configuration file. For instance, the following request:

```
RateLimitRequest:
domain: example4
descriptor: ("key", "value"),("subkey", "subvalue")
```

Would **not** match the following configuration, even though the first descriptor in
Copy link
Contributor

Choose a reason for hiding this comment

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

run together sentence.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

the request matches the descriptor in the configuration, because the request has
two tuples in the descriptor.

```yaml
domain: example4
descriptors:
- key: key
value: value
rate_limit:
- requests_per_unit: 300
unit: second
```

However, it would match the following configuration:

```yaml
domain: example4
descriptors:
- key: key
value: value
descriptors:
- key: subkey
rate_limit:
- requests_per_unit: 300
unit: second
```

## Loading Configuration

The Ratelimit service uses a library written by Lyft called [goruntime](https://github.com/lyft/goruntime) to do configuration loading. Goruntime monitors
>>>>>>> master
Copy link
Member

Choose a reason for hiding this comment

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

merge conflict

Copy link
Member Author

Choose a reason for hiding this comment

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

removed.

a designated path, and watches for symlink swaps to files in the directory tree to reload configuration files.

The path to watch can be configured via the [settings](https://github.com/lyft/ratelimit/blob/master/src/settings/settings.go)
Expand Down
8 changes: 6 additions & 2 deletions src/config/config_impl.go
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ func (this *rateLimitConfigImpl) GetLimit(
}

descriptorsMap := value.descriptors
for _, entry := range descriptor.Entries {
for i, entry := range descriptor.Entries {
// First see if key_value is in the map. If that isn't in the map we look for just key
// to check for a default value.
finalKey := entry.Key + "_" + entry.Value
Expand All @@ -265,7 +265,11 @@ func (this *rateLimitConfigImpl) GetLimit(

if nextDescriptor != nil && nextDescriptor.limit != nil {
logger.Debugf("found rate limit: %s", finalKey)
Copy link
Contributor

Choose a reason for hiding this comment

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

should this logger statement be moved into the if below?

Copy link
Member Author

Choose a reason for hiding this comment

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

I dont think so. In debug we want to know about all the ratelimits found, the next logger message would let us know if the ratelimit was not chosen.

rateLimit = nextDescriptor.limit
if (i == len(descriptor.Entries) - 1) {
rateLimit = nextDescriptor.limit
} else {
logger.Debugf("request depth does not match config depth, there are more entries in the request's descriptor")
}
}

if nextDescriptor != nil && len(nextDescriptor.descriptors) > 0 {
Expand Down
12 changes: 12 additions & 0 deletions test/config/basic_config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -40,3 +40,15 @@ descriptors:
rate_limit:
unit: day
requests_per_unit: 1

- key: key5
value: value5
rate_limit:
unit: day
requests_per_unit: 15
descriptors:
- key: subkey5
value: subvalue5
rate_limit:
unit: day
requests_per_unit: 25
10 changes: 10 additions & 0 deletions test/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,16 @@ func TestBasicConfig(t *testing.T) {
&pb.RateLimitDescriptor{[]*pb.RateLimitDescriptor_Entry{{"key1", "value1"}}})
assert.Nil(rl)

rl = rlConfig.GetLimit(
nil, "test-domain",
&pb.RateLimitDescriptor{[]*pb.RateLimitDescriptor_Entry{{"key2", "value2"}, {"subkey", "subvalue"}}})
assert.Nil(rl)

rl = rlConfig.GetLimit(
nil, "test-domain",
&pb.RateLimitDescriptor{[]*pb.RateLimitDescriptor_Entry{{"key5", "value5"}, {"subkey5", "subvalue"}}})
Copy link
Contributor

Choose a reason for hiding this comment

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

What are you trying to test here?

Copy link
Member Author

Choose a reason for hiding this comment

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

The config did not get pushed in the branch. Just updated. I am testing that RL will not return a limit for two tuples, even though there was a descriptor with less nesting in the config that matched.

assert.Nil(rl)

rl = rlConfig.GetLimit(
nil, "test-domain",
&pb.RateLimitDescriptor{
Expand Down