Set more lenient timeouts for protection services #1027
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1027 +/- ##
==========================================
+ Coverage 76.15% 84.20% +8.05%
==========================================
Files 111 81 -30
Lines 8986 6679 -2307
==========================================
- Hits 6843 5624 -1219
+ Misses 1852 842 -1010
+ Partials 291 213 -78
Flags with carried forward coverage won't be shown. Click here to find out more.
|
pkg/wasm/utils.go
Outdated
| func AuthServiceTimeout() string { | ||
| return env.GetString("AUTH_SERVICE_TIMEOUT", "5s") | ||
| } | ||
|
|
||
| func RatelimitServiceTimeout() string { | ||
| return env.GetString("RATELIMIT_SERVICE_TIMEOUT", "100ms") | ||
| } | ||
|
|
There was a problem hiding this comment.
Was torn here between var that executes once on startup vs function that calls out to env every time. My other consideration was making these return *string with ptr.To immediately to avoid at every call site
Signed-off-by: Adam Cattermole <[email protected]>
adam-cattermole
force-pushed
the
service-timeouts
branch
from
2e85dde to
f1e4643
Compare
There was a problem hiding this comment.
LGTM
I have listed 7 env vars so far
LOG_LEVELLOG_MODEOPERATOR_NAMESPACERELATED_IMAGE_WASMSHIMRELATED_IMAGE_CONSOLEPLUGINAUTH_SERVICE_TIMEOUTRATELIMIT_SERVICE_TIMEOUT
We should document those env vars for devs and operator release managers. I was thinking on documenting about how to tune those parameters when releasing using OLM (using the subscription object). As well as when using helm charts. I know currently it is not possible to tune those env vars when deploying using helm charts, but this is also something to be done. Leaving here for the record, not asking for this PR.
|
My stance on environment variables is that they are "backdoors" into the system. That's certainly what I had envisioned for these two new ones. So, if we really document them, I think this should be done in such a way that it's clear that this is either "experimental" or whatever, but clearly not supported and/or guaranteed to work moving forward. Discussing this issue with @adam-cattermole on Friday, the intent is to go with somewhat sensible defaults and see how make this a properly supported (and documented) feature moving forward... There is more to this than it may seem and this deserves some real thinking. A default per service is definitely not sensible from a Kuadrant perspective. |
Fair enough. Let's not document those env vars yet. |
The default connection timeout used by the wasm-shim when the value is omitted is 20ms. This could be too short when set to use external redis for rate limiting, or with complex auth policies. By default I have increased this to:
This is also not exposed by the API in anyway, and any changes to the
WasmPluginwill be overridden by the operator, so it's not possible to set these values. For now until we decide how to expose this in the API I've added two env vars:AUTH_SERVICE_TIMEOUTRATELIMIT_SERVICE_TIMEOUTThese let us override the defaults if a user requires.