oscerd commented on code in PR #26437:
URL: https://github.com/apache/camel/pull/26437#discussion_r4013646417
##########
components/camel-opa/src/main/java/org/apache/camel/component/opa/OpaPolicyEvaluator.java:
##########
@@ -162,14 +162,33 @@ protected boolean isAllowed(Object decision) {
if (decision instanceof Boolean b) {
return b;
}
- if (decision instanceof Map<?, ?> map && map.get(allowKey) instanceof
Boolean b) {
+ if (readVerdict(decision) instanceof Boolean b) {
return b;
}
- LOG.debug("Policy {} returned a decision with no boolean '{}' verdict,
denying. Decision: {}",
- policyPath, allowKey, decision);
+ // a decision document we cannot read a verdict from is a
configuration problem, not a routine deny, and the
+ // route cannot tell the two apart from the verdict header alone - so
say so at WARN rather than DEBUG
+ LOG.warn("Policy {} returned a decision with no boolean '{}' verdict,
denying. Check that allowKey matches"
Review Comment:
Agreed on both counts — fixed.
The WARN now fires once per evaluator (an `AtomicBoolean` guard) and no
longer carries the decision document; the document moved to a DEBUG line that
still fires every time. You are right that the `deny[msg]` shape without
`default allow := false` hits this on every legitimate deny, which made it a
per-message log of a whole document on a busy route.
The `CamelOpaDecision` header still carries the raw document either way, so
nothing a route can act on was lost.
_Claude Code on behalf of @oscerd_
##########
components/camel-opa/src/main/java/org/apache/camel/component/opa/OpaPolicyEvaluator.java:
##########
@@ -162,14 +162,33 @@ protected boolean isAllowed(Object decision) {
if (decision instanceof Boolean b) {
return b;
}
- if (decision instanceof Map<?, ?> map && map.get(allowKey) instanceof
Boolean b) {
+ if (readVerdict(decision) instanceof Boolean b) {
return b;
}
- LOG.debug("Policy {} returned a decision with no boolean '{}' verdict,
denying. Decision: {}",
- policyPath, allowKey, decision);
+ // a decision document we cannot read a verdict from is a
configuration problem, not a routine deny, and the
+ // route cannot tell the two apart from the verdict header alone - so
say so at WARN rather than DEBUG
+ LOG.warn("Policy {} returned a decision with no boolean '{}' verdict,
denying. Check that allowKey matches"
+ + " the shape the policy returns; the raw document is on the
{} header. Decision: {}",
+ policyPath, allowKey, OpaConstants.DECISION, decision);
return false;
}
+ /**
+ * Reads {@code allowKey} out of the decision document, walking a dotted
path so a verdict nested inside the result
+ * - {@code allowKey=result.allow} against <code>{"result": {"allow":
true}}</code> - can be reached. A key with no
+ * dot is looked up directly, exactly as before.
+ */
+ private Object readVerdict(Object decision) {
+ Object current = decision;
+ for (String segment : allowKey.split("\\.", -1)) {
Review Comment:
Done — `readVerdict` now tries `map.get(allowKey)` on the top-level document
first and only walks the dotted path if that misses.
Added `prefersATopLevelKeyThatItselfContainsADot`, which puts both shapes in
one document (`"com.acme.allow": true` alongside `com.acme.allow` nested and
set to `false`) so the test would fail if the precedence ever flipped.
_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]
