You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "adutra (via GitHub)" <gi...@apache.org> on 2025/01/15 10:15:48 UTC
[PR] Fix RateLimiterFilter configuration [polaris]
adutra opened a new pull request, #765:
URL: https://github.com/apache/polaris/pull/765
The filter itself was missing some important annotations.
Its configuration has been simplified slightly as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix RateLimiterFilter configuration [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra merged PR #765:
URL: https://github.com/apache/polaris/pull/765
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix RateLimiterFilter configuration [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #765:
URL: https://github.com/apache/polaris/pull/765#discussion_r1916303341
##########
service/common/src/main/java/org/apache/polaris/service/ratelimiter/RateLimiterFilter.java:
##########
@@ -29,6 +33,9 @@
/** Request filter that returns a 429 Too Many Requests if the rate limiter says so */
@Provider
+@PreMatching
+@Priority(Priorities.USER)
Review Comment:
The original Dropwizard filter did not specify any priority. Accordding to JAX-RS, this means priority `USER`. I think it's better to be clear about this and specify the priority.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix RateLimiterFilter configuration [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #765:
URL: https://github.com/apache/polaris/pull/765#discussion_r1916911244
##########
service/common/src/main/java/org/apache/polaris/service/ratelimiter/RateLimiterFilter.java:
##########
@@ -29,6 +33,9 @@
/** Request filter that returns a 429 Too Many Requests if the rate limiter says so */
@Provider
+@PreMatching
+@Priority(Priorities.USER)
Review Comment:
Good point. Still, if the authenticator is merely validating access tokens, that should be fine, I guess... With JWT, the validation effort is not much higher than the effort to keep track of the request rate, I guess.
That said, I think it would be worth moving the loading of principal information out the the authentication code and do that after all request checks are performed (including the rate limit filter).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix RateLimiterFilter configuration [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #765:
URL: https://github.com/apache/polaris/pull/765#discussion_r1916889020
##########
service/common/src/main/java/org/apache/polaris/service/ratelimiter/TokenBucketConfiguration.java:
##########
@@ -20,14 +20,9 @@
import java.time.Duration;
-public interface RateLimiterConfiguration {
+public interface TokenBucketConfiguration {
- TokenBucketConfiguration tokenBucket();
+ long requestsPerSecond();
- interface TokenBucketConfiguration {
-
- long requestsPerSecond();
-
- Duration window();
- }
+ Duration window();
Review Comment:
nit: Can we use new config doc generating annotations already?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix RateLimiterFilter configuration [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #765:
URL: https://github.com/apache/polaris/pull/765#discussion_r1916938430
##########
service/common/src/main/java/org/apache/polaris/service/ratelimiter/RateLimiterFilter.java:
##########
@@ -29,6 +33,9 @@
/** Request filter that returns a 429 Too Many Requests if the rate limiter says so */
@Provider
+@PreMatching
+@Priority(Priorities.USER)
Review Comment:
You make good points as well. We could also consider this Quarkus extension:
https://docs.quarkiverse.io/quarkus-bucket4j/dev/index.html
Although, it's not a filter, but rather applies at REST resource level.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix RateLimiterFilter configuration [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #765:
URL: https://github.com/apache/polaris/pull/765#discussion_r1916915106
##########
service/common/src/main/java/org/apache/polaris/service/ratelimiter/TokenBucketConfiguration.java:
##########
@@ -20,14 +20,9 @@
import java.time.Duration;
-public interface RateLimiterConfiguration {
+public interface TokenBucketConfiguration {
- TokenBucketConfiguration tokenBucket();
+ long requestsPerSecond();
- interface TokenBucketConfiguration {
-
- long requestsPerSecond();
-
- Duration window();
- }
+ Duration window();
Review Comment:
Question for @snazy I guess. If so, I can look into it next.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix RateLimiterFilter configuration [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #765:
URL: https://github.com/apache/polaris/pull/765#discussion_r1916877307
##########
service/common/src/main/java/org/apache/polaris/service/ratelimiter/RateLimiterFilter.java:
##########
@@ -29,6 +33,9 @@
/** Request filter that returns a 429 Too Many Requests if the rate limiter says so */
@Provider
+@PreMatching
+@Priority(Priorities.USER)
Review Comment:
Using `USER` means that this filter cannot be abused by unauthenticated requests – but also means, that malicious requests will hit the authenticator filter first, thus defeating the purpose of a rate limiter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org