gnodet commented on code in PR #26524: URL: https://github.com/apache/camel/pull/26524#discussion_r4029396019
########## 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: Fixed in 68b0ec2f6885. `OpaHttpClient` now implements `AutoCloseable` with a `close()` that delegates to `client.close()` (guarded with `instanceof AutoCloseable` to remain compatible with the Java 17 baseline where `HttpClient` is not `AutoCloseable`). `OpaRestEvaluator` also implements `AutoCloseable` and holds a direct reference to the transport so `doStop()`'s existing `instanceof AutoCloseable` pattern reaches it. ########## 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: Fixed in 68b0ec2f6885. Added the note to `setSslContextParameters` Javadoc explaining that `useGlobalSslContextParameters` on `OpaComponent` has no effect when using `OpaSecurityPolicy` directly, and that the field must be set explicitly. ########## 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: Fixed in 68b0ec2f6885. Dropped `open.add(accepted::close)` from the accepter thread entirely — the `ServerSocket` is already registered in `open`, and closing it both terminates the accepter thread (via `SocketException`) and releases the port. The accepted socket is released when the OPA client side disconnects after its timeout, so no explicit close is needed. Also removed the now-unused `java.net.Socket` import. -- 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]
