davsclaus commented on code in PR #26437:
URL: https://github.com/apache/camel/pull/26437#discussion_r4011961014
##########
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:
Non-blocking: a top-level key that itself contains a dot would now be split.
Rego rule names can't have dots so it's unlikely, but a direct
`map.get(allowKey)` before walking the path keeps the "exactly as before"
promise for free.
##########
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:
Non-blocking: this is now a per-message WARN carrying the whole decision
document. A misconfigured `allowKey` will flood the log on a busy route, and a
package query whose `allow` has no default (the `deny[msg]` pattern without
`default allow := false` — the shape in
`deniesWhenTheDecisionObjectHasNoVerdictButKeepsTheRawDocument`) hits it on
every legitimate deny. Suggest warning once per evaluator and keeping the raw
document at DEBUG — the `CamelOpaDecision` header already carries it.
--
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]
