oscerd commented on PR #26974:
URL: https://github.com/apache/camel/pull/26974#issuecomment-5888212574
One more `failOpen` narrowing, prompted by the parallel camel-opa work in
CAMEL-25139 rather than by a review comment here.
That issue found the OPA gap was wider than 4xx, which made me re-read this
classifier's *shape*. It decided what `failOpen` covers by exclusion — anything
it did not recognise returned "unavailable" — and three failures fell through
that default into an allow although OpenFGA had never answered:
- `FgaInvalidParameterException`, thrown before the request goes out (an
invalid store id), so nothing was asked;
- `SdkSerializationException`, which extends `IOException` and therefore
passed for a transport failure, though it means our own input could not be
turned into a request — the same trap CAMEL-25139 had to exclude Jackson for;
- `InterruptedException` during shutdown, which is the case a comment on
that path already claimed was failing closed. It was not. A confident comment
is not a test.
`failOpen` now applies to a rate limit, a 5xx, an `IOException` and a
timeout, and to nothing else. A failure the component does not recognise denies
rather than being waved through for being unfamiliar, and the interrupt comment
now describes what actually holds the line. Two tests cover the pre-flight and
serialisation paths.
On whether OpenFGA needs the same 500 carve-out CAMEL-25139 applied to OPA:
it does not. Checked against 1.21.0 — OpenFGA reports input problems as 400,
including an unknown relation (`400 "relation 'document#nope' not found"`), and
its Check always answers `{allowed}` or errors, so there is no
undefined-decision state for `failOpen` to swallow. Treating 5xx as unavailable
is safe here in a way it was not for OPA, where 500 carries input-dependent
evaluation errors.
95 unit tests and 12 ITs pass; rebased on current `main`, full reactor
clean, and the PR still deletes nothing outside its own new files.
This is the third time the fail-open boundary on this PR turned out narrower
than it looked, so for what it is worth the generalisable form seems to be: a
fail-open flag should enumerate the failures it covers and treat everything
else as a denial, never the other way round.
---
_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]
