Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
Implement strict decoding for JetStream API requests #5858
Changes from all commits
50f27e4
b2553c0
7bc2273
4ec16f2
f50cf20
42abb6b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 settingDisallowUnknownFields()
based on theStrict
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 lessThere 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 winThere 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.