davsclaus commented on code in PR #26524:
URL: https://github.com/apache/camel/pull/26524#discussion_r4033247095
##########
components/camel-opa/src/main/java/org/apache/camel/component/opa/security/OpaSecurityPolicy.java:
##########
@@ -78,7 +89,9 @@ public void beforeWrap(Route route, NamedNode definition) {
StringHelper.notEmpty(policyPath, "policyPath", this);
if (opaClient == null) {
// createClient moved to OpaRestEvaluator when the evaluator
became an abstract base
- opaClient = OpaRestEvaluator.createClient(serverUrl,
bearerToken);
+ opaClient = OpaRestEvaluator.createClient(
+ serverUrl, bearerToken, connectionTimeout,
requestTimeout,
+ createSslContext(route.getCamelContext()));
ownsClient = true;
}
evaluator = new OpaRestEvaluator(
Review Comment:
This is what breaks the build: `createClient` is now `(String,
OpaHttpClient)` and the evaluator constructor takes the transport as its second
argument.
```suggestion
OpaHttpClient transport = null;
if (opaClient == null) {
// createClient moved to OpaRestEvaluator when the evaluator
became an abstract base
transport = OpaRestEvaluator.createTransport(
bearerToken, connectionTimeout, requestTimeout,
createSslContext(route.getCamelContext()));
opaClient = OpaRestEvaluator.createClient(serverUrl,
transport);
ownsClient = true;
}
evaluator = new OpaRestEvaluator(
opaClient, transport, policyPath, allowKey,
includeHeaders, includeProperties, includeBody,
failOpen);
}
```
(`OpaHttpClient` is package-private in `org.apache.camel.component.opa`, so
it will need to become public — or `createTransport` return the `HTTPClient`
interface — for this call site in the `security` sub-package.)
##########
components/camel-opa/src/main/java/org/apache/camel/component/opa/OpaHttpClient.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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,
AutoCloseable {
Review Comment:
Please import rather than fully qualify (CLAUDE.md import rule):
```suggestion
class OpaHttpClient implements HTTPClient, AutoCloseable {
```
with `import com.styra.opa.openapi.utils.HTTPClient;` alongside the other
imports.
##########
components/camel-opa/src/main/java/org/apache/camel/component/opa/OpaHttpClient.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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,
AutoCloseable {
+
+ 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
Review Comment:
The previous `createClient` only added the header when
`ObjectHelper.isNotEmpty(bearerToken)`; with `!= null` an empty `bearerToken=`
now sends `Authorization: Bearer ` (and drops whatever the SDK set).
Normalising here keeps the old behaviour:
```suggestion
this.bearerToken = ObjectHelper.isNotEmpty(bearerToken) ?
bearerToken : null;
```
--
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]
