gnodet-bot commented on code in PR #26524: URL: https://github.com/apache/camel/pull/26524#discussion_r4029332789
########## components/camel-opa/src/main/java/org/apache/camel/component/opa/OpaHttpClient.java: ########## @@ -0,0 +1,78 @@ +/* + * 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.camel.component.opa; + +import java.io.IOException; +import java.io.InputStream; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; +import java.time.Duration; + +import javax.net.ssl.SSLContext; + +/** + * The HTTP transport the OPA SDK uses to reach the server. + * <p/> + * Supplied rather than left to the SDK, whose default {@code SpeakeasyHTTPClient} is a one-liner around + * {@code HttpClient.newHttpClient()} with two consequences a policy decision point cannot afford: + * <ul> + * <li><b>Nothing bounds the call.</b> That factory sets no connect timeout and the SDK sets no request timeout, so both + * are the JDK default of "wait indefinitely". A server that accepts the connection and then goes quiet parks the + * calling thread for ever - and a component that fails closed never reaches the point of denying, it simply stops. + * {@code failOpen} does not help, because it sits downstream of a call that never returns.</li> + * <li><b>It builds a client per request.</b> On the Java 17 baseline {@link HttpClient} is not {@link AutoCloseable}, + * so each one holds its selector thread and executor until it is collected - once per message, on the path an + * {@code OpaSecurityPolicy} guards.</li> + * </ul> + * One client is built here per evaluator and reused, and every request is re-issued carrying a timeout. + */ +class OpaHttpClient implements com.styra.opa.openapi.utils.HTTPClient { + + private static final String AUTHORIZATION = "Authorization"; + + private final HttpClient client; + private final Duration requestTimeout; + private final String bearerToken; + + OpaHttpClient(long connectionTimeout, long requestTimeout, SSLContext sslContext, String bearerToken) { + HttpClient.Builder builder = HttpClient.newBuilder() + .connectTimeout(Duration.ofMillis(connectionTimeout)); + if (sslContext != null) { + builder.sslContext(sslContext); + } + this.client = builder.build(); + this.requestTimeout = Duration.ofMillis(requestTimeout); + // the SDK has no constructor taking a transport and headers together, so the token is applied here + // instead of through OPAClient(String, Map) - the request that goes out is the same either way + this.bearerToken = bearerToken; + } + + @Override + public HttpResponse<InputStream> send(HttpRequest request) throws IOException, InterruptedException { + // the SDK hands us a finished request, and HttpRequest is immutable - rebuilding it is the only way to + // attach a timeout. The filter keeps every header the SDK set, dropping only an Authorization we are + // about to replace, because header() appends rather than overwrites + HttpRequest.Builder builder = HttpRequest + .newBuilder(request, (name, value) -> bearerToken == null || !AUTHORIZATION.equalsIgnoreCase(name)) + .timeout(requestTimeout); + if (bearerToken != null) { + builder.header(AUTHORIZATION, "Bearer " + bearerToken); + } + return client.send(builder.build(), HttpResponse.BodyHandlers.ofInputStream()); + } +} Review Comment: ⚠️ **`HttpClient` is never explicitly closed when the endpoint stops.** `OpaEndpoint.doStop()` checks `evaluator instanceof AutoCloseable`, but `OpaRestEvaluator` does not implement `AutoCloseable` (only `OpaWasmEvaluator` does). The `HttpClient` inside this class therefore outlives the endpoint lifecycle. On Java 21, `HttpClient` implements `AutoCloseable` and owns a `SelectorManager` thread. In embedded or hot-deploy environments that repeatedly start/stop routes, this leaks one thread per `OpaRestEvaluator` instance. Daemon threads won't block JVM exit, but they do accumulate in long-lived JVMs. Fix: implement `AutoCloseable` on `OpaHttpClient` (and by extension make `OpaRestEvaluator` implement `AutoCloseable` too, so `doStop()` can reach it via the existing `instanceof` pattern): ```suggestion class OpaHttpClient implements com.styra.opa.openapi.utils.HTTPClient, AutoCloseable { ``` And add a `close()` method that calls `client.close()` when running on Java 21+ (guard with `if (client instanceof AutoCloseable c) c.close();` to stay compatible with the Java 17 baseline). ########## components/camel-opa/src/main/java/org/apache/camel/component/opa/security/OpaSecurityPolicy.java: ########## @@ -211,6 +224,60 @@ public void setFailOpen(boolean failOpen) { this.failOpen = failOpen; } + /** + * The policy is a bean rather than a {@code CamelContextAware} service, so the context comes from the route it is + * wrapping - which is the only place one is available. + */ + private SSLContext createSslContext(CamelContext camelContext) { + if (sslContextParameters == null) { + return null; + } + try { + return sslContextParameters.createSSLContext(camelContext); + } catch (GeneralSecurityException | IOException e) { + // beforeWrap cannot throw checked exceptions, and a policy whose TLS configuration is broken must not + // start a route that would then talk to OPA over the JVM default trust material instead + throw new RuntimeCamelException("Could not build the SSLContext for policy " + policyPath, e); + } + } + + public long getConnectionTimeout() { + return connectionTimeout; + } + + /** + * How long to wait for the connection to the OPA server to be established. The SDK's own transport applies no + * timeout, so a server that never answers would otherwise park the routing thread rather than letting the policy + * fail closed. + */ + public void setConnectionTimeout(long connectionTimeout) { + this.connectionTimeout = connectionTimeout; + } + + public long getRequestTimeout() { + return requestTimeout; + } + + /** + * How long to wait for the decision once connected. A request that times out is an evaluation failure rather than a + * deny, so the policy denies the exchange unless {@code failOpen} is set. + */ + public void setRequestTimeout(long requestTimeout) { + this.requestTimeout = requestTimeout; + } + + public SSLContextParameters getSslContextParameters() { + return sslContextParameters; + } + + /** + * TLS configuration for the connection to the OPA server. Needed to trust a server whose certificate comes from a + * private CA, and to present a client certificate to a server requiring mutual TLS. + */ + public void setSslContextParameters(SSLContextParameters sslContextParameters) { Review Comment: ⚠️ **`OpaSecurityPolicy` silently ignores `useGlobalSslContextParameters`.** `OpaEndpoint.createSslContext()` falls back to `getComponent().retrieveGlobalSslContextParameters()` when no per-endpoint SSL is set. `OpaSecurityPolicy.createSslContext()` (line 231) just returns `null` — there is no global SSL fallback path. A user who sets `useGlobalSslContextParameters=true` on the `OpaComponent` and then secures a route via `<securityPolicy>` rather than `<to opa:...>` will get no TLS — silently. The component applies plain HTTP where they expected HTTPS. The fix is either: 1. Add a `useGlobalSslContextParameters` field and wire it via Spring/Camel XML — but `OpaSecurityPolicy` is a free bean, not component-bound, so there is no `OpaComponent` to query. 2. **Document the limitation**: in this setter's Javadoc, explicitly state that `useGlobalSslContextParameters` on the component has no effect when using `OpaSecurityPolicy` directly — users must set `sslContextParameters` on the policy bean itself. Option 2 is the right call given the architectural constraint, but it must be done: ```suggestion /** * TLS configuration for the connection to the OPA server. Needed to trust a server whose certificate comes from a * private CA, and to present a client certificate to a server requiring mutual TLS. * <p/> * Note: {@code useGlobalSslContextParameters} on the {@link org.apache.camel.component.opa.OpaComponent} has no * effect here. {@code OpaSecurityPolicy} is a standalone bean and is not bound to any component instance, so the * global SSL context cannot be resolved automatically. Set this field explicitly when TLS is required. */ public void setSslContextParameters(SSLContextParameters sslContextParameters) { this.sslContextParameters = sslContextParameters; } ``` ########## components/camel-opa/src/test/java/org/apache/camel/component/opa/OpaRestTransportTest.java: ########## @@ -0,0 +1,102 @@ +/* + * 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.camel.component.opa; + +import java.io.IOException; +import java.net.ServerSocket; +import java.net.Socket; +import java.util.ArrayList; +import java.util.List; + +import org.apache.camel.Exchange; +import org.apache.camel.test.junit6.CamelTestSupport; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * The transport underneath {@code evaluationMode=rest}. + * <p/> + * A server that refuses a connection fails fast and is already covered; what is not, and what an operator actually + * meets, is a server that <em>accepts</em> and then says nothing. The SDK's own transport applies no timeout of any + * kind, so that case parked the routing thread for ever - and a component that fails closed never reached the point of + * denying, it simply stopped. + */ +public class OpaRestTransportTest extends CamelTestSupport { + + private final List<Closeable> open = new ArrayList<>(); + + private interface Closeable extends AutoCloseable { + @Override Review Comment: 🔍 **Race between accepter thread and `@AfterEach` cleanup.** The accepter thread calls `open.add(accepted::close)` from inside the thread body. If `closeSockets()` runs before the thread stores the accepted socket in `open`, the socket leaks (and the port isn't freed). In practice the `requestTimeout=500ms` means there's plenty of time, but the race is real: a fast test runner or a slow I/O thread could trigger it. Simpler and race-free: hold the `ServerSocket` reference and close it in `@AfterEach`. Closing the `ServerSocket` both terminates the accepter thread (via `SocketException`) and releases the port. The accepted socket is implicitly released when the peer (OPA client) disconnects after its timeout: ```suggestion @AfterEach void closeSockets() throws Exception { for (Closeable c : open) { c.close(); } open.clear(); } ``` Alternatively, store the `accepted` socket via a `CountDownLatch` or `AtomicReference` so `closeSockets()` can wait for the accepter thread to finish before clearing. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
