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.

Sign up for GitHub

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

Jump to bottom

Ratelimiting doesn't work with both clientSelectors (i.e. headers and sourceCIDR) enabled #4351

Closed
yash-nisar opened this issue Sep 27, 2024 · 3 comments · Fixed by #4377
Closed

Ratelimiting doesn't work with both clientSelectors (i.e. headers and sourceCIDR) enabled #4351

yash-nisar opened this issue Sep 27, 2024 · 3 comments · Fixed by #4377
Assignees
@shawnh2
Labels
area/xds-translator kind/bug Something isn't working
Milestone
v1.2.0

Comments

@yash-nisar
Copy link

yash-nisar commented Sep 27, 2024 •
edited
Loading

Description:

What issue is being seen? Describe what should be happening instead of
the bug, for example: Envoy should not crash, the expected value isn't
returned, etc.
Ratelimiting doesn't work with both clientSelectors (i.e. headers and sourceCIDR) enabled. I would expect it to work if the headers match and the request is from the IP in the sourceCIDR range.

Repro steps:

Include sample requests, environment, etc. All data and inputs
required to reproduce the bug.
Followed all steps from quickstart. This is my ratelimit config:

rateLimit:
    global:
      rules:
      - clientSelectors:
        - headers:
          - name: x-user-id
            value: one
          sourceCIDR:
            type: Exact
            value: 0.0.0.0/0
        limit:
          requests: 3
          unit: Hour
    type: Global

Note: If there are privacy concerns, sanitize the data prior to
sharing.

Environment:

Include the environment like gateway version, envoy version and so on.

Logs:

Include the access logs and the Envoy logs.

These are the envoy-ratelimit logs, note this line descriptor does not match any limit, no limits applied.

time="2024-09-27T00:56:04Z" level=debug msg="CertProvider: waiting for runtime update"
time="2024-09-27T00:56:04Z" level=info msg="Starting xDS client connection for rate limit configurations"
time="2024-09-27T00:56:04Z" level=info msg="Dialing xDS Management Server: 'envoy-gateway:18001'"
time="2024-09-27T00:56:04Z" level=warning msg="connecting to redis on redis.redis-system.svc.cluster.local:6379 with pool size 10"
time="2024-09-27T00:56:04Z" level=debug msg="Implicit pipelining enabled: false"
time="2024-09-27T00:56:04Z" level=warning msg="Listening for debug on '0.0.0.0:6070'"
time="2024-09-27T00:56:04Z" level=warning msg="Listening for gRPC on '0.0.0.0:8081'"
time="2024-09-27T00:56:04Z" level=debug msg="Waiting for config update event"
time="2024-09-27T00:56:04Z" level=warning msg="Listening for HTTP on '0.0.0.0:8080'"
time="2024-09-27T00:56:04Z" level=info msg="Connection to xDS Management Server is successful"
time="2024-09-27T00:56:04Z" level=debug msg="loading domain: default/eg/http"
time="2024-09-27T00:56:04Z" level=debug msg="loading descriptor: key=default/eg/http.httproute/default/http-ratelimit/rule/0/match/0/ratelimit_example_httproute/default/http-ratelimit/rule/0/match/0/ratelimit_example"
time="2024-09-27T00:56:04Z" level=debug msg="Creating stats for key: 'default/eg/http.httproute/default/http-ratelimit/rule/0/match/0/ratelimit_example_httproute/default/http-ratelimit/rule/0/match/0/ratelimit_example.masked_remote_address_0.0.0.0/0'"
time="2024-09-27T00:56:04Z" level=debug msg="loading descriptor: key=default/eg/http.httproute/default/http-ratelimit/rule/0/match/0/ratelimit_example_httproute/default/http-ratelimit/rule/0/match/0/ratelimit_example.masked_remote_address_0.0.0.0/0 ratelimit={requests_per_unit=3, unit=HOUR, unlimited=false, shadow_mode=false}"
time="2024-09-27T00:56:04Z" level=debug msg="Setting config retrieved from config provider"
time="2024-09-27T00:56:04Z" level=info msg="Successfully loaded new configuration"
time="2024-09-27T00:56:04Z" level=debug msg="Waiting for config update event"
time="2024-09-27T00:56:39Z" level=debug msg="got descriptor: (httproute/default/http-ratelimit/rule/0/match/0/ratelimit_example=httproute/default/http-ratelimit/rule/0/match/0/ratelimit_example),(rule-0-match-0=rule-0-match-0),(masked_remote_address=0.0.0.0/0)"
time="2024-09-27T00:56:39Z" level=debug msg="starting get limit lookup"
time="2024-09-27T00:56:39Z" level=debug msg="looking up key: httproute/default/http-ratelimit/rule/0/match/0/ratelimit_example_httproute/default/http-ratelimit/rule/0/match/0/ratelimit_example"
time="2024-09-27T00:56:39Z" level=debug msg="iterating to next level"
time="2024-09-27T00:56:39Z" level=debug msg="looking up key: rule-0-match-0_rule-0-match-0"
time="2024-09-27T00:56:39Z" level=debug msg="looking up key: rule-0-match-0"
time="2024-09-27T00:56:39Z" level=debug msg="descriptor does not match any limit, no limits applied"
time="2024-09-27T00:56:39Z" level=debug msg="starting cache lookup"
time="2024-09-27T00:56:39Z" level=debug msg="returning normal response"

I do see that the config is reflected in xds-ir (in gateway logs):

2024-09-27T00:54:27.139Z        INFO    gateway-api     runner/runner.go:186    {
        "accessLog": {
                "text": [
                        {
                                "path": "/dev/stdout"
                        }
                ]
        },
        "http": [
                {
                        "name": "default/eg/http",
                        "address": "0.0.0.0",
                        "port": 10080,
                        "metadata": {
                                "kind": "Gateway",
                                "name": "eg",
                                "namespace": "default",
                                "sectionName": "http"
                        },
                        "hostnames": [
                                "*"
                        ],
                        "routes": [
                                {
                                        "name": "httproute/default/backend/rule/0/match/0/www_example_com",
                                        "hostname": "www.example.com",
                                        "isHTTP2": false,
                                        "pathMatch": {
                                                "name": "",
                                                "prefix": "/",
                                                "distinct": false
                                        },
                                        "destination": {
                                                "name": "httproute/default/backend/rule/0",
                                                "settings": [
                                                        {
                                                                "weight": 1,
                                                                "protocol": "HTTP",
                                                                "endpoints": [
                                                                        {
                                                                                "host": "10.244.0.10",
                                                                                "port": 3000
                                                                        },
                                                                        {
                                                                                "host": "10.244.0.10",
                                                                                "port": 3000
                                                                        }
                                                                ],
                                                                "addressType": "IP"
                                                        }
                                                ]
                                        },
                                        "metadata": {
                                                "kind": "HTTPRoute",
                                                "name": "backend",
                                                "namespace": "default"
                                        }
                                },
                                {
                                        "name": "httproute/default/http-ratelimit/rule/0/match/0/ratelimit_example",
                                        "hostname": "ratelimit.example",
                                        "isHTTP2": false,
                                        "pathMatch": {
                                                "name": "",
                                                "prefix": "/",
                                                "distinct": false
                                        },
                                        "destination": {
                                                "name": "httproute/default/http-ratelimit/rule/0",
                                                "settings": [
                                                        {
                                                                "weight": 1,
                                                                "protocol": "HTTP",
                                                                "endpoints": [
                                                                        {
                                                                                "host": "10.244.0.10",
                                                                                "port": 3000
                                                                        },
                                                                        {
                                                                                "host": "10.244.0.10",
                                                                                "port": 3000
                                                                        }
                                                                ],
                                                                "addressType": "IP"
                                                        }
                                                ]
                                        },
                                        "traffic": {
                                                "rateLimit": {
                                                        "global": {
                                                                "rules": [
                                                                        {
                                                                                "headerMatches": [
                                                                                        {
                                                                                                "name": "x-user-id",
                                                                                                "exact": "one",
                                                                                                "distinct": false
                                                                                        }
                                                                                ],
                                                                                "cidrMatch": {
                                                                                        "cidr": "0.0.0.0/0",
                                                                                        "ip": "0.0.0.0",
                                                                                        "maskLen": 0,
                                                                                        "isIPv6": false,
                                                                                        "distinct": false
                                                                                },
                                                                                "limit": {
                                                                                        "requests": 3,
                                                                                        "unit": "Hour"
                                                                                }
                                                                        }
                                                                ]
                                                        }
                                                }
                                        },
                                        "metadata": {
                                                "kind": "HTTPRoute",
                                                "name": "http-ratelimit",
                                                "namespace": "default"
                                        }
                                }
                        ],
                        "isHTTP2": false,
                        "path": {
                                "mergeSlashes": true,
                                "escapedSlashesAction": "UnescapeAndRedirect"
                        }
                }
        ]
}       {"runner": "gateway-api", "xds-ir": "default/eg"}

Thanks !
@shawnh2
Copy link
Contributor

shawnh2 commented Sep 28, 2024

Looks weird, can you share the output of egctl config rl -n envoy-gateway-system ?

@yash-nisar
Copy link
Author

yash-nisar commented Sep 29, 2024 •
edited
Loading

Looks weird, can you share the output of egctl config rl -n envoy-gateway-system ?

@shawnh2 Sure, here's the output:

default/eg/http.httproute/default/http-ratelimit/rule/0/match/0/ratelimit_example_httproute/default/http-ratelimit/rule/0/match/0/ratelimit_example.masked_remote_address_0.0.0.0/0: unit=HOUR requests_per_unit=3, shadow_mode: false

I think there is a bug in this method: buildRateLimitServiceDescriptors

gateway/internal/xds/translator/ratelimit.go

Line 329 in cbd92e2

func buildRateLimitServiceDescriptors(global *ir.GlobalRateLimit) []*rlsconfv3.RateLimitDescriptor {

It does not consider the case where both the clientSelectors are present.

I made this change to make it work for the time being:

if rule.CIDRMatch != nil {
	// MaskedRemoteAddress case
	pbDesc := new(rlsconfv3.RateLimitDescriptor)
	pbDesc.Key = "masked_remote_address"
	pbDesc.Value = rule.CIDRMatch.CIDR
	rateLimit := rlsconfv3.RateLimitPolicy{
		RequestsPerUnit: uint32(rule.Limit.Requests),
		Unit:            rlsconfv3.RateLimitUnit(rlsconfv3.RateLimitUnit_value[strings.ToUpper(string(rule.Limit.Unit))]),
	}

	if rule.CIDRMatch.Distinct {
		pbDesc.Descriptors = []*rlsconfv3.RateLimitDescriptor{
			{
				Key:       "remote_address",
				RateLimit: &rateLimit,
			},
		}
	} else {
		pbDesc.RateLimit = &rateLimit
	}

	if len(rule.HeaderMatches) != 0 {
		cur.Descriptors = []*rlsconfv3.RateLimitDescriptor{pbDesc}
	} else {
		head = pbDesc
		cur = head
	}
}

for mIdx, match := range rule.HeaderMatches {
	pbDesc := new(rlsconfv3.RateLimitDescriptor)
	// Case for distinct match
	if match.Distinct {
		// RequestHeader case
		pbDesc.Key = getRouteRuleDescriptor(rIdx, mIdx)
	} else {
		// HeaderValueMatch case
		pbDesc.Key = getRouteRuleDescriptor(rIdx, mIdx)
		pbDesc.Value = getRouteRuleDescriptor(rIdx, mIdx)
	}

	// Add the ratelimit values to the last descriptor
	if mIdx == len(rule.HeaderMatches)-1 && rule.CIDRMatch == nil {
		rateLimit := rlsconfv3.RateLimitPolicy{
			RequestsPerUnit: uint32(rule.Limit.Requests),
			Unit:            rlsconfv3.RateLimitUnit(rlsconfv3.RateLimitUnit_value[strings.ToUpper(string(rule.Limit.Unit))]),
		}
		pbDesc.RateLimit = &rateLimit
	}

	if mIdx == 0 {
		head = pbDesc
	} else {
		cur.Descriptors = []*rlsconfv3.RateLimitDescriptor{pbDesc}
	}

	cur = pbDesc
}

@shawnh2
Copy link
Contributor

shawnh2 commented Sep 30, 2024

Thanks for reporting this, it is indeed a bug. Though we have created RL actions for both Headers & CIDR correctly,

gateway/internal/xds/translator/ratelimit.go

Line 142 in cbd92e2

func buildRouteRateLimits(descriptorPrefix string, global *ir.GlobalRateLimit) []*routev3.RateLimit {

but doing wrong when building RL Descriptors.

@shawnh2 shawnh2 added kind/bug Something isn't working area/xds-translator and removed triage labels Sep 30, 2024
@shawnh2 shawnh2 self-assigned this Sep 30, 2024
@shawnh2 shawnh2 added this to the v1.2.0-rc1 milestone Sep 30, 2024
@shawnh2 shawnh2 mentioned this issue Oct 1, 2024
Merged
@arkodg arkodg modified the milestones: v1.2.0-rc1, v1.2.0 Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@shawnh2 shawnh2

Labels
area/xds-translator kind/bug Something isn't working
Projects
None yet
Milestone
v1.2.0
Development

Successfully merging a pull request may close this issue.

fix: ratelimit not working with both headers and cidr matches shawnh2/gateway
3 participants
@arkodg @yash-nisar @shawnh2