snazy commented on code in PR #278: URL: https://github.com/apache/polaris/pull/278#discussion_r1760713229
########## polaris-service/src/main/java/org/apache/polaris/service/ratelimiter/OpenTelemetryRateLimiter.java: ########## @@ -0,0 +1,38 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.service.ratelimiter; + +/** + * Wrapper around the opentelemetry RateLimiter that implements the Polaris RateLimiter interface + * The opentelemetry limiter uses a credits/balance system. We treat 1 request as 1 credit. + */ +public class OpenTelemetryRateLimiter implements RateLimiter { + private final io.opentelemetry.sdk.internal.RateLimiter rateLimiter; Review Comment: Why import an _internal_ OT type? I'd really like to avoid relying on any internals of any library. ########## polaris-service/src/test/java/org/apache/polaris/service/ratelimiter/AsyncFallbackTest.java: ########## @@ -0,0 +1,75 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.service.ratelimiter; + +import io.dropwizard.testing.ConfigOverride; +import io.dropwizard.testing.ResourceHelpers; +import io.dropwizard.testing.junit5.DropwizardAppExtension; +import io.dropwizard.testing.junit5.DropwizardExtensionsSupport; +import jakarta.ws.rs.core.Response; +import java.util.function.Consumer; +import org.apache.polaris.service.PolarisApplication; +import org.apache.polaris.service.PolarisApplicationIntegrationTest; +import org.apache.polaris.service.config.PolarisApplicationConfig; +import org.apache.polaris.service.test.PolarisConnectionExtension; +import org.apache.polaris.service.test.SnowmanCredentialsExtension; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; + +/** + * Integration test that verifies the timeout behavior for fetching async rate limiters. This is in + * its own test class because the Dropwizard app is per test class and there isn't a great way to + * allow tests to clear the rate limiter cache. + */ +@ExtendWith({ + DropwizardExtensionsSupport.class, + PolarisConnectionExtension.class, + SnowmanCredentialsExtension.class +}) +public class AsyncFallbackTest { Review Comment: This PR introduces async construction behaviors, which deserve a bunch of more testing, especially failure scenarios, but this test asserts only a simple "good case". I suggest to simplify the whole implementation and remove the async behavior entirely. ########## polaris-service/src/main/java/org/apache/polaris/service/ratelimiter/RateLimiterFactory.java: ########## @@ -0,0 +1,42 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.service.ratelimiter; + +import com.fasterxml.jackson.annotation.JsonTypeInfo; +import io.dropwizard.jackson.Discoverable; +import java.util.concurrent.Future; + +/** + * Interface for constructing a rate limiter given the rate limiting key and clock. Notably, rate + * limiter construction may be asynchronous. This allows fetching information related to the key. + * For example, implementors may wish to fetch per-account rate limit values, which require lookup + * in an external database. + */ +@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "type") +public interface RateLimiterFactory extends Discoverable { + /** + * Constructs a rate limiter asynchronously. Callers may choose to set a timeout on construction. + * + * @param key The rate limiting key. Rate limiters may optionally choose to discriminate their + * behavior by the key. + * @param clock The clock which tells you the current time + * @return a Future with the constructed RateLimiter + */ + Future<RateLimiter> createRateLimiter(String key, Clock clock); Review Comment: The clock is rather an implementation concern and shouldn't be provided by the caller. ########## polaris-service/src/main/java/org/apache/polaris/service/ratelimiter/RateLimiterFilter.java: ########## @@ -0,0 +1,94 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.service.ratelimiter; + +import jakarta.annotation.Priority; +import jakarta.servlet.Filter; +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.ServletRequest; +import jakarta.servlet.ServletResponse; +import jakarta.servlet.http.HttpServletResponse; +import jakarta.ws.rs.Priorities; +import jakarta.ws.rs.core.Response; +import java.io.IOException; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import org.apache.polaris.core.context.CallContext; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** Request filter that returns a 429 Too Many Requests if the rate limiter says so */ +@Priority(Priorities.AUTHORIZATION + 1) +public class RateLimiterFilter implements Filter { + private static final Logger LOGGER = LoggerFactory.getLogger(RateLimiterFilter.class); + private static final RateLimiter NO_OP_LIMITER = new NoOpRateLimiter(); + private static final RateLimiter ALWAYS_REJECT_LIMITER = + new OpenTelemetryRateLimiter(0, 0, new ClockImpl()); + private static final Clock CLOCK = new ClockImpl(); + + private final RateLimiterConfig config; + private final Map<String, RateLimiter> perRealmLimiters = new ConcurrentHashMap<>(); + + public RateLimiterFilter(RateLimiterConfig config) { + this.config = config; + } + + /** Returns a 429 if the rate limiter says so. Otherwise, forwards the request along. */ + @Override + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + String realm = CallContext.getCurrentContext().getRealmContext().getRealmIdentifier(); + RateLimiter rateLimiter = maybeBlockToGetRateLimiter(realm); + if (!rateLimiter.tryAcquire()) { + ((HttpServletResponse) response).setStatus(Response.Status.TOO_MANY_REQUESTS.getStatusCode()); + return; + } + chain.doFilter(request, response); + } + + private RateLimiter maybeBlockToGetRateLimiter(String realm) { + return perRealmLimiters.computeIfAbsent(realm, this::createRateLimiterBlocking); Review Comment: Blocking from inside a CHM is really not great, because it can (and eventually will) easily cause deadlocks. See Javadoc of the CHM's `computeIfAbsent` function: `... the computation should be short and simple.` ########## polaris-service/src/main/java/org/apache/polaris/service/ratelimiter/DefaultRateLimiterFactory.java: ########## @@ -0,0 +1,45 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.service.ratelimiter; + +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonTypeName; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.Future; + +/** + * Simple rate limiter factory that just constructs an OpenTelemetryRateLimiter and is + * indiscriminate of the key + */ +@JsonTypeName("default") +public class DefaultRateLimiterFactory implements RateLimiterFactory { + @JsonProperty("requestsPerSecond") + private double requestsPerSecond; + + @JsonProperty("windowSeconds") + private double windowSeconds; + + @Override + public Future<RateLimiter> createRateLimiter(String key, Clock clock) { + return CompletableFuture.supplyAsync( Review Comment: This relies on the fork-join common pool for potentially long-running operations, likely causing other tasks against that pool to either stall and potentially cause deadlocks. ########## polaris-service/src/main/java/org/apache/polaris/service/ratelimiter/ClockImpl.java: ########## @@ -0,0 +1,33 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.service.ratelimiter; + +/** An implementation of our Clock interface using opentelemetry's Clock implementation */ +public class ClockImpl implements Clock { + private final io.opentelemetry.sdk.common.Clock openTelemetryClock; + + public ClockImpl() { + openTelemetryClock = io.opentelemetry.sdk.common.Clock.getDefault(); + } + + @Override + public long nanoTime() { + return openTelemetryClock.nanoTime(); Review Comment: The only thing OT's clock impl does is calling `System.nanoTime()`. ########## polaris-service/src/main/java/org/apache/polaris/service/ratelimiter/RateLimiterFilter.java: ########## @@ -0,0 +1,94 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.service.ratelimiter; + +import jakarta.annotation.Priority; +import jakarta.servlet.Filter; +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.ServletRequest; +import jakarta.servlet.ServletResponse; +import jakarta.servlet.http.HttpServletResponse; +import jakarta.ws.rs.Priorities; +import jakarta.ws.rs.core.Response; +import java.io.IOException; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import org.apache.polaris.core.context.CallContext; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** Request filter that returns a 429 Too Many Requests if the rate limiter says so */ +@Priority(Priorities.AUTHORIZATION + 1) +public class RateLimiterFilter implements Filter { + private static final Logger LOGGER = LoggerFactory.getLogger(RateLimiterFilter.class); + private static final RateLimiter NO_OP_LIMITER = new NoOpRateLimiter(); + private static final RateLimiter ALWAYS_REJECT_LIMITER = + new OpenTelemetryRateLimiter(0, 0, new ClockImpl()); + private static final Clock CLOCK = new ClockImpl(); + + private final RateLimiterConfig config; + private final Map<String, RateLimiter> perRealmLimiters = new ConcurrentHashMap<>(); + + public RateLimiterFilter(RateLimiterConfig config) { + this.config = config; + } + + /** Returns a 429 if the rate limiter says so. Otherwise, forwards the request along. */ + @Override + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + String realm = CallContext.getCurrentContext().getRealmContext().getRealmIdentifier(); + RateLimiter rateLimiter = maybeBlockToGetRateLimiter(realm); Review Comment: IMHO nothing must block here at all for a rate-limiting decision. The goal is to prevent the service from being overloaded, but a blocking implementation holds system resources which can then be used to overload the system. In other words: the rate-limiter implementation itself can be used for a (D)DOS. -- 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: commits-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
