oscerd commented on code in PR #26436:
URL: https://github.com/apache/camel/pull/26436#discussion_r4013546632
##########
components/camel-opa/src/main/java/org/apache/camel/component/opa/OpaProducerHealthCheck.java:
##########
@@ -16,16 +16,10 @@
*/
package org.apache.camel.component.opa;
-import java.net.URI;
-import java.net.http.HttpClient;
-import java.net.http.HttpRequest;
-import java.net.http.HttpResponse;
-import java.time.Duration;
import java.util.Map;
import org.apache.camel.health.HealthCheckResultBuilder;
import org.apache.camel.impl.health.AbstractHealthCheck;
-import org.apache.camel.util.ObjectHelper;
import org.apache.camel.util.URISupport;
Review Comment:
Not applying this one — the import is still live.
`URISupport.sanitizeUri(id)` is used on line 41, in the constructor that builds
the check id:
```java
super("camel", "producer:opa-" + URISupport.sanitizeUri(id));
```
Removing the import would not compile. (@davsclaus caught the same thing
below.)
_Claude Code on behalf of @oscerd_
##########
components/camel-opa/src/main/java/org/apache/camel/component/opa/security/OpaSecurityPolicy.java:
##########
@@ -67,6 +70,7 @@ public OpaSecurityPolicy(String serverUrl, String policyPath)
{
@Override
public void beforeWrap(Route route, NamedNode definition) {
Review Comment:
Fixed — `registerHealthCheck(route)` now runs at the end of `beforeWrap`,
after `StringHelper.notEmpty(policyPath, …)`.
I kept it outside the `evaluator == null` block rather than inlining it as
suggested, because registration has its own idempotence guard (`healthCheck !=
null`) and its own skip conditions (an injected `opaClient`, an empty
`serverUrl`) that are independent of whether the evaluator has been built.
Putting it last gives the ordering you want without tying the two lifecycles
together.
`registersNothingForAPolicyThatFailsValidation` covers it: it asserts the
exception still comes out and that no `…/null` check is left behind.
_Claude Code on behalf of @oscerd_
##########
components/camel-opa/src/main/java/org/apache/camel/component/opa/OpaProducerHealthCheck.java:
##########
@@ -16,16 +16,10 @@
*/
package org.apache.camel.component.opa;
-import java.net.URI;
-import java.net.http.HttpClient;
-import java.net.http.HttpRequest;
-import java.net.http.HttpResponse;
-import java.time.Duration;
import java.util.Map;
import org.apache.camel.health.HealthCheckResultBuilder;
import org.apache.camel.impl.health.AbstractHealthCheck;
-import org.apache.camel.util.ObjectHelper;
import org.apache.camel.util.URISupport;
Review Comment:
Confirmed, and kept — `URISupport.sanitizeUri(id)` is still used on line 41
to build the check id, so the suggested removal would not compile. Declined on
the other thread.
_Claude Code on behalf of @oscerd_
##########
components/camel-opa/src/main/java/org/apache/camel/component/opa/security/OpaSecurityPolicy.java:
##########
@@ -67,6 +70,7 @@ public OpaSecurityPolicy(String serverUrl, String policyPath)
{
@Override
public void beforeWrap(Route route, NamedNode definition) {
+ registerHealthCheck(route);
Review Comment:
Done — registration moved to the end of `beforeWrap`, after the `notEmpty`
check, with a test asserting nothing is left in the registry when validation
fails.
_Claude Code on behalf of @oscerd_
##########
components/camel-opa/src/main/java/org/apache/camel/component/opa/security/OpaSecurityPolicy.java:
##########
@@ -77,6 +81,24 @@ public void beforeWrap(Route route, NamedNode definition) {
}
}
+ /**
+ * Registers a readiness check for the OPA server, once per policy however
many routes it wraps.
+ * <p/>
+ * Skipped when an {@code opaClient} was injected: that client can point
anywhere and this policy has no way to ask
+ * it where, so probing the configured URL would report on a server it may
never talk to.
+ */
+ private void registerHealthCheck(Route route) {
+ if (healthCheck != null || opaClient != null ||
ObjectHelper.isEmpty(serverUrl)) {
+ return;
+ }
+ HealthCheckRegistry registry =
HealthCheckRegistry.get(route.getCamelContext());
+ if (registry == null) {
+ return;
+ }
+ healthCheck = new OpaSecurityPolicyHealthCheck(serverUrl, bearerToken,
policyPath);
Review Comment:
Agreed, and deferring as you suggest — the id dedup bounds it, so the worst
case today is a stale check for a policy whose route was removed, not unbounded
growth.
Tracked as [CAMEL-24751](https://issues.apache.org/jira/browse/CAMEL-24751)
so it doesn't get lost. The ref-count wants `OpaSecurityProcessor.doStop` to
know how many wrapped processors are left, which is a bigger change than this
PR should carry.
_Claude Code on behalf of @oscerd_
##########
components/camel-opa/src/main/java/org/apache/camel/component/opa/security/OpaSecurityPolicy.java:
##########
@@ -77,6 +81,24 @@ public void beforeWrap(Route route, NamedNode definition) {
}
}
+ /**
+ * Registers a readiness check for the OPA server, once per policy however
many routes it wraps.
+ * <p/>
+ * Skipped when an {@code opaClient} was injected: that client can point
anywhere and this policy has no way to ask
+ * it where, so probing the configured URL would report on a server it may
never talk to.
+ */
+ private void registerHealthCheck(Route route) {
Review Comment:
Good point — added `healthCheckEnabled` (default `true`) on
`OpaSecurityPolicy`, checked in `registerHealthCheck`, with
`registersNothingWhenTheCheckIsDisabled` covering it.
I kept the default on rather than matching the producer's opt-in, for the
reason you give: the policy denies every exchange it guards while OPA is
unreachable, so a route that is up but cannot reach OPA genuinely is not ready.
The doc now says that explicitly and points at `healthCheckEnabled=false` — for
a `failOpen` policy, say — instead of at `camel.health.exclude-pattern`.
_Claude Code on behalf of @oscerd_
--
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]
