HY-love-sleep commented on PR #7153:
URL: https://github.com/apache/shenyu/pull/7153#issuecomment-5757101550
Thanks for the careful review — both blocking points were real and are fixed
in `06f2b6109`.
**1. The scan ran on the event loop — fixed.**
You were right: only the redis path hopped threads, and every request after
the first takes the cached
path. `dictionary()` now ends with `publishOn(Schedulers.boundedElastic())`,
so both branches are
covered before the scan. I also took your second suggestion:
`buildFailureLinks()` now resolves the
matched words of every node once (`outputs = own word + fail.outputs`), so
`search(...)` no longer walks
the failure chain and is linear in the length of the text.
**2. The rejection echoed the matched words — fixed.**
The client now receives `Request rejected: sensitive content detected`; the
matched words go to the
plugin log only. While there, the rejection code follows the `waf` plugin
(403) instead of the
plugin-invented 1500, and the test asserts that the response body does
**not** contain the matched word.
**3. fail strategy — done.**
`failClosed` is a rule level option now (default `false`, i.e. the current
behaviour). With
`failClosed: true` a rule rejects the request when the dictionary cannot be
read and no cached dictionary
is available; a cached dictionary, even a stale one, is still enforced.
**4 / 5 / 6**
- **4 (redis connection not released)** agreed, and as you said it is
pre-existing:
`RedisConnectionFactory` exposes no `destroy()` and
`AiTokenLimiterPluginHandler` has the same shape. I
would rather do it as a separate small PR on `shenyu-infra-redis` (add
`destroy()`, call it before
replacing the factory) and keep this one about the plugin.
- **5 (no bound on the body)** agreed; it needs a policy decision (skip the
scan and warn, or reject, and
how that interacts with `failClosed`). It is in the follow-up issue below.
- **6** `removePlugin()` is implemented (releases the cached template and
properties). I deliberately did
**not** drop the `DICTIONARIES` entry in `removeRule()`: that cache is
keyed by the rule's `redisKey`,
so one dictionary can be shared by several rules and dropping it on a
single rule removal would force
the others to rebuild. Happy to change it if you prefer the rebuild.
**7** Follow-up issue opened: #7154 — the `plugin` / `plugin_handle` /
`resource` rows, the console rule
form and the body size bound. It is linked from the PR description as well,
so this PR stays focused on
the plugin implementation.
Local, on `06f2b6109`: 38 tests green (including the new fail-closed paths,
the stale dictionary fallback
and `removePlugin`), checkstyle 0, RAT ok, `shenyu-bootstrap -am package` ok.
--
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]
