feat: switch global ratelimit config provider to xds-grpc-sotw based server #1416
Conversation
Signed-off-by: Shawnh2 <shawnhxh@outlook.com>
|
that was quick @shawnh2 ! will take another look when its out of draft |
Signed-off-by: Shawnh2 <shawnhxh@outlook.com>
Signed-off-by: Shawnh2 <shawnhxh@outlook.com>
Signed-off-by: Shawnh2 <shawnhxh@outlook.com>
Signed-off-by: Shawnh2 <shawnhxh@outlook.com>
Codecov Report
@@ Coverage Diff @@
## main #1416 +/- ##
==========================================
- Coverage 61.98% 61.92% -0.07%
==========================================
Files 79 79
Lines 11388 11354 -34
==========================================
- Hits 7059 7031 -28
+ Misses 3870 3862 -8
- Partials 459 461 +2
|
| }, | ||
| { | ||
| Name: ConfigGrpcXdsServerURL, | ||
| Value: net.JoinHostPort(XdsGrpcSotwConfigServerHost, strconv.Itoa(XdsGrpcSotwConfigServerPort)), |
There was a problem hiding this comment.
Do we need to check if XdsGrpcSotwConfigServerPort is a valid port ?
There was a problem hiding this comment.
right now the DefaultXdsServerPort does not have a check either.
gateway/internal/xds/bootstrap/bootstrap.go
Lines 29 to 30 in 89b8b39
maybe we can make these two ports configureable, and done as a follow-up?
Signed-off-by: Shawnh2 <shawnhxh@outlook.com>
|
@shawnh2 I think this will speed up ratelimit gateway/test/e2e/tests/ratelimit.go Line 38 in 89b8b39 |
| Name: listener.Name, | ||
| Config: str, | ||
| } | ||
| ratelimitInfra.ServiceConfigs = append(ratelimitInfra.ServiceConfigs, c) |
There was a problem hiding this comment.
curious why is the service config needed now that we are using xds ?
There was a problem hiding this comment.
because for ratelimitInfra, we only use it to update the configMap envoy-ratelimit:
that configMap envoy-ratelimit is used when we run ratelimit provider in FILE mode.
like the discuss above, someone may want to override the XDS mode to FILE manually. so if people does that, we can still make sure that ratelimit provider is working.
There was a problem hiding this comment.
imo we should remove all the configmap logic, its more maintenance burden for this project to support multiple config methods. afaik there is no use case to support both, users would like to configure ratelimiting and are not concerned with how ratelimiting is being implemented
There was a problem hiding this comment.
agreed. I have removed the configmap logic, so only XDS mode is supported.
In the meantime, I've change the structure of ratelimitInfra, to make it simpler. we still need ratelimitInfra to publish the changes to ratelimit infra, so the svc and deployment resources will be created or deleted.
|
PR looks good ! just added some minor comments |
Signed-off-by: Shawnh2 <shawnhxh@outlook.com>
@zirain hi, I shrink the sleep time down to 30s now, to see how it goes. i notice that there're still some TODO in ratelimit e2e test, i can help with that as follow-up. gateway/test/e2e/tests/ratelimit.go Line 36 in a9a8528 |
Signed-off-by: Shawnh2 <shawnhxh@outlook.com>
Signed-off-by: Shawnh2 <shawnhxh@outlook.com>
Signed-off-by: Shawnh2 <shawnhxh@outlook.com>
Signed-off-by: Shawnh2 <shawnhxh@outlook.com>
Signed-off-by: Shawnh2 <shawnhxh@outlook.com>
What type of PR is this?
feat(global-ratelimit): switch config provider to xds-grpc-sotw based
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #1254