oscerd commented on code in PR #26677:
URL: https://github.com/apache/camel/pull/26677#discussion_r4061629801
##########
components/camel-opa/src/test/java/org/apache/camel/component/opa/OpaWasmIT.java:
##########
@@ -97,20 +146,16 @@ void keepsTheEntrypointAcrossPooledReuse() {
assertThat(out.getMessage().getHeader(OpaConstants.DECISION))
.as("message %d was still decided by authz/decision", i)
.isInstanceOf(Map.class);
Review Comment:
Restored — and you're right that I dropped it, though the mechanism is
slightly different from what you describe.
`isInstanceOf(Map.class)` **does** separate the two rules on its own:
`authz/allow` returns a bare boolean, so entrypoint 0 would fail the type
check. What the content assertion adds is that *any* rule returning an object
would satisfy the type alone, so the pair is what actually pins the entrypoint
rather than merely its shape.
Either way it was in the original and I lost it moving the test to an IT, so
it's back with a comment saying why both assertions are there.
Worth noting this prompted a systematic check rather than a spot fix — I
diffed assertion counts per method between the deleted test and the IT, which
turned up something neither of us had seen:
`acceptsTheBundleTarballOpaBuildActuallyEmits` had vanished entirely in the
move. That one matters more, because every bundle in the IT is now a tarball,
so the bare `.wasm` branch of `loadPolicy` had silently lost all coverage.
Added back as `acceptsABareModuleAsWellAsTheBundleTarball`, extracting
`policy.wasm` from the compiled bundle.
_Claude Code on behalf of @oscerd_
##########
components/camel-opa/src/test/java/org/apache/camel/component/opa/OpaWasmIT.java:
##########
@@ -121,54 +166,8 @@ void appliesTheDataDocumentPackedInTheBundle() {
}
@Test
- void failsClosedOnAnUndefinedDecisionJustLikeTheRestEngine() {
- // authz/strict_allow has no default, so for mallory the rule is
undefined. The WASM ABI returns an
- // empty array where the REST client raises an error; both must reach
the route the same way.
- Exchange out = template.request(
-
"opa:authz/strict_allow?evaluationMode=wasm&policyBundle=classpath:authz.wasm",
- e -> e.getMessage().setHeader("user", "mallory"));
-
-
assertThat(out.getException()).isInstanceOf(OpaPolicyEvaluationException.class);
-
assertThat(out.getMessage().getHeader(OpaConstants.DECISION_ALLOW)).isNull();
- }
-
- @Test
- void acceptsTheBundleTarballOpaBuildActuallyEmits() {
- Exchange out = template.request(
-
"opa:authz/allow?evaluationMode=wasm&policyBundle=classpath:authz-bundle.tar.gz",
- e -> e.getMessage().setHeader("user", "alice"));
-
- assertThat(out.getException()).isNull();
-
assertThat(out.getMessage().getHeader(OpaConstants.DECISION_ALLOW)).isEqualTo(true);
- }
-
- @Test
- void requiresAPolicyBundle() {
- // the check runs when the endpoint starts, so a misconfiguration
fails fast rather than once per message
- assertThatThrownBy(() ->
template.request("opa:authz/allow?evaluationMode=wasm", e -> {
- }))
- .isInstanceOf(ResolveEndpointFailedException.class)
- .hasMessageContaining("policyBundle is required");
- }
-
- @Test
- void rejectsAPoolSizeBelowOne() {
- // the pool rejects it as well, but as "maxSize must be positive" -
its own parameter rather than the
- // option that was set, which is what the operator has to go looking
for
- assertThatThrownBy(() -> template.request(
-
"opa:authz/allow?evaluationMode=wasm&policyBundle=classpath:authz.wasm&poolSize=0",
e -> {
- }))
- .isInstanceOf(ResolveEndpointFailedException.class)
- .hasMessageContaining("poolSize must be at least 1");
- }
-
- @Test
- @Timeout(60)
void keepsThePoolUsableAfterRepeatedEvaluationFailures() {
Review Comment:
Restored, and thank you — dropping it was careless rather than deliberate.
Your reasoning is exactly right: that test exists because a mishandled
permit wedges `template.request()`, and without the annotation a broken pool
hangs the run with no indication of which test is stuck or why. The annotation
is no less valid under `-Pit`; if anything more so, since an IT run is the one
people are least likely to be watching.
_Claude Code on behalf of @oscerd_
##########
components/camel-opa/src/test/java/org/apache/camel/component/opa/OpaWasmIT.java:
##########
@@ -25,45 +29,82 @@
import java.util.stream.IntStream;
import org.apache.camel.Exchange;
-import org.apache.camel.ResolveEndpointFailedException;
+import org.apache.camel.test.infra.opa.services.OpaWasmBundleBuilder;
import org.apache.camel.test.junit6.CamelTestSupport;
+import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.Timeout;
+import org.junit.jupiter.api.io.TempDir;
import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.assertThatThrownBy;
/**
- * In-process evaluation of a bundle produced by {@code opa build -t wasm}.
+ * In-process evaluation of a bundle compiled from the very {@code authz.rego}
that {@link OpaIT} uploads to a real OPA
+ * server.
* <p/>
- * The bundle here was compiled from the same {@code authz.rego} the REST
tests use, so the assertions double as a check
- * that a route sees the same decision whichever engine evaluated it.
+ * Sharing one policy between the two classes is the point rather than a
convenience: the component promises that a
+ * route sees the same decision whichever engine evaluated it, and that
promise is only tested if both engines are asked
+ * about the same rules. It also closes the way that promise was broken before
- a compiled bundle committed beside the
+ * Rego drifted from it, and the two suites asserted opposite things about
{@code authz/decision} while both stayed
+ * green (CAMEL-24741). Nothing is committed now; the bundle is built from the
policy under test.
*/
-public class OpaWasmEvaluatorTest extends CamelTestSupport {
+public class OpaWasmIT extends CamelTestSupport {
- private static final String WASM =
"opa:authz/allow?evaluationMode=wasm&policyBundle=classpath:authz.wasm";
+ @TempDir
+ static Path bundles;
+
+ private static String authz;
+ private static String roles;
+
+ private static String resource(String name) throws Exception {
+ try (InputStream in = OpaWasmIT.class.getResourceAsStream(name)) {
Review Comment:
Applied. It now throws naming the resource:
```java
if (in == null) {
throw new IllegalStateException("Test resource not found on the
classpath: " + name);
}
```
The failure mode you describe is the annoying kind — a `@BeforeAll` NPE
pointing at `readAllBytes` tells you nothing about which of the four resources
is missing, and this class loads `authz.rego`, `wasm-data/roles.rego` and
`wasm-data/data.json`.
_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]
