-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement strict decoding for JetStream API requests #5858
base: main
Are you sure you want to change the base?
Conversation
9beb4f6
to
6b84a9b
Compare
return err | ||
} | ||
|
||
return json.Unmarshal(msg, v) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not convinced by this block, in that if strict mode is disabled, we can in effect unmarshal twice. (Unmarshalling JSON is actually quite expensive on CPU so need to be careful on hot paths.)
Why this vs just having one decoder
and setting DisallowUnknownFields()
based on the Strict
setting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My first pass did just that, but was discussed offline with @ripienaar that we want to log marshaling failures even if strict is set to false to have a softer error path but keep the current behavior for existing deployments where it would at-least surface in the logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A misbehaving client is going to a) spam log lines and b) unmarshal twice for each request. I don't think that's wise, needs further discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Derek suggested that for 2.11 while we are in soft mode we should still report the problem so client authors can discover and remediate issues so he suggested this approach. It's nice a nice assist before we enable this by default to reject requests but I agree with you it will spam logs until things are fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think client authors could enable this as an opt-in too, in order to avoid spamming there is rateLimitFormatWarnf
which would log less
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we discussed maybe a header on specific requests - but we have no api behaviours on headers so I am a bit reluctant.
Other option is something at the request level like we have action
on consumer or pedantic now - big ripple effect on clients having to support it and ultimately this does not help users. The point is to identify bad clients that perhaps are not compatible or have bugs. So making it opt-in only for clients that support it is a no win
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rateLimitFormatWarnf is a good idea though for sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that any opt-in for the client misses the point that Derek had - it's about detecting problems with non-tier-1 and older clients, and those will not have such opt-in implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the log lines to use rate limited warnings.
server/jetstream_api.go
Outdated
} | ||
|
||
if err != nil { | ||
s.Errorf("Invalid JetStream request '%s > %s': %s", acc, subject, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is a sample line that gets reported here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[10037] 2024/09/06 14:50:14.087465 [ERR] Invalid JetStream request '$G > $JS.API.STREAM.INFO.ORDERS': json: unknown field "stream_name"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now with rate limit warnings instead of error it is:
[11545] 2024/09/06 15:10:33.449547 [WRN] Invalid JetStream request '$G > $JS.API.STREAM.INFO.ORDERS': json: unknown field "stream_name"
a9b2549
to
c4418f4
Compare
There's one failing test, but this is failing on main as-well so I don't think that's related. |
server/jetstream.go
Outdated
@@ -453,6 +454,7 @@ func (s *Server) enableJetStream(cfg JetStreamConfig) error { | |||
s.Noticef("") | |||
} | |||
s.Noticef("---------------- JETSTREAM ----------------") | |||
s.Noticef(" Strict: %t", cfg.Strict) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove these from banner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd vote to keep it, it's a sanity notice that you got the configuration you expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's only print it if cfg.Strict == true
.
c4418f4
to
92bfdd6
Compare
Ran the meta benchmarks on this as they're pretty request heavy:
|
server/opts.go
Outdated
@@ -2317,6 +2318,9 @@ func parseJetStream(v any, opts *Options, errors *[]error, warnings *[]error) er | |||
for mk, mv := range vv { | |||
tk, mv = unwrapValue(mv, <) | |||
switch strings.ToLower(mk) { | |||
case "strict": | |||
opts.JetStreamStrict = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to do a type assertion here for s, ok := vv.(bool)
, return an error if the type assert fails, and then set JetStreamStrict
to the bool value. Otherwise setting strict: false
in the config does the wrong thing.
Currently failures to unmarshal only give a vague error of "invalid JSON", this adds the original error to it as context. Before: ```` invalid JSON ```` After: ``` invalid JSON: invalid character '\"' after object key ``` Related to but **not** dependent on #5858 Signed-off-by: Casper Beyer <casper@synadia.com>
Mind rebasing this & resolving conflicts please? |
0f7aa53
to
b0144fc
Compare
b0144fc
to
50f27e4
Compare
Please make sure the strict option value is shown in jsz output |
server/jetstream.go
Outdated
@@ -453,6 +454,7 @@ func (s *Server) enableJetStream(cfg JetStreamConfig) error { | |||
s.Noticef("") | |||
} | |||
s.Noticef("---------------- JETSTREAM ----------------") | |||
s.Noticef(" Strict: %t", cfg.Strict) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's only print it if cfg.Strict == true
.
This implements optional strict JSON decoding for JetStream.
The intent of this is to minimize accidental misalignments between server and clients, we've had numerous of these across the various clients, especially for rarely used fields in the request payloads.
Signed-off-by: Casper Beyer casper@synadia.com