Reject bulk requests with invalid actions #5302
Conversation
The existing bulk api silently ignores bulk item requests that have an invalid action. This change rejects those requests. Signed-off-by: Rabi Panda <adnapibar@gmail.com>
| @@ -64,7 +64,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), | |||
| ### Fixed | |||
| - Fix 'org.apache.hc.core5.http.ParseException: Invalid protocol version' under JDK 16+ ([#4827](https://github.com/opensearch-project/OpenSearch/pull/4827)) | |||
| - Fixed compression support for h2c protocol ([#4944](https://github.com/opensearch-project/OpenSearch/pull/4944)) | |||
| - Add jvm option to allow security manager ([#5194](https://github.com/opensearch-project/OpenSearch/pull/5194)) | |||
There was a problem hiding this comment.
This shouldn't be included in the CHANGELOG as per the guidelines, removing it.
|
Should this be considered as a breaking change? |
Yes, I think so. |
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
@@ Coverage Diff @@
## main #5302 +/- ##
============================================
- Coverage 71.02% 70.98% -0.05%
+ Complexity 58161 58126 -35
============================================
Files 4704 4704
Lines 277401 277404 +3
Branches 40166 40167 +1
============================================
- Hits 197025 196907 -118
- Misses 64204 64428 +224
+ Partials 16172 16069 -103
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Should we then deprecate in 2.5 and reject in 3.0? |
Yeah, I think a deprecation warning in 2.5 is a good idea. |
| @@ -176,7 +203,7 @@ public void parse( | |||
| + "]" | |||
| ); | |||
| } | |||
| String action = parser.currentName(); | |||
| Action action = Action.of(parser.currentName(), line); | |||
There was a problem hiding this comment.
You could maybe refactor this as Action.consume(parser, line) so that you can handle the failure case above in the same place. I think it currently prints "...expected FIELD_NAME but found [x]" and that could be improved as well to say the "expected one of ..." thing.
Feel free to think of a better name than "consume"
There was a problem hiding this comment.
consume sounds more like it doesn't return anything, let me think of a better name.
the error message does say ... expected one of [create, delete, index, update] but found ...
There was a problem hiding this comment.
Yes, your new error message says that. I was suggesting to make it say that for the case on line 195 where a non-field name is encountered as well.
Action.parse might work.
There was a problem hiding this comment.
On line 195, the check is the type of token encountered, so I think we can keep the error message as is.
Signed-off-by: Rabi Panda <adnapibar@gmail.com>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
server/src/main/java/org/opensearch/action/bulk/BulkRequestParser.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/action/bulk/BulkRequestParser.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Rabi Panda <adnapibar@gmail.com>
Gradle Check (Jenkins) Run Completed with:
|
reta
added
backport 2.x
|
@reta I don't think we can backport this as it is to 2.x to 2.4 as it seems to be breaking ? Also on a related note, the backport labels only work after a PR is merged. |
It seems like we just fixing the bug, behavior wise it is breaking, yes, but I think the parser should have been rejecting invalid request since the beginning. @dblock wdyt?
Sure. |
I agree that these requests should have been rejected, but the fact remains it is a breaking change in behavior. If we're being super strict about compatibility then this change should not be backported. The counter argument is that any user that would be impacted by this likely has a bug, and perhaps a serious one (e.g. using "remove" instead of "delete" and their intent is just being ignored). I would lean towards the second argument, and if this change actually breaks a user's workflow then that is a good thing because it uncovers a bug. But I could be swayed that our commitment to semantic versioning should be the priority. I'm also curious what @dblock thinks. |
reta
removed
backport 2.x
Thanks @andrross Removing the backport lables for now :-) we could always backport later |
|
🤔 So the request would simply be ignored in the past, and now you get a proper error. It is indeed a behavior change, the old API was a noop, so let's not backport it. |
|
To my mind, if you provide a set of responses that are acceptable (create, delete, index, update), the expectation is that these are the only responses that acceptable. This is how we documented our API, which is the first rule of Semver. Our behavior of swallowing the error is a bug because it does not enforce our stated contract. Making the software correctly fulfil it's contract might be changing behavior, but it's not unexpected or new behavior. and does not change how the API contract was supposed to function. Additionally, failing malformed requests silently on batch jobs is one of the worst things we could do -- since it makes it impossible to have confidence in your results. So at the risk of sounding like these guys: I think we should backport to enforce the existing contract and make it easier for people to find bugs in their requests. (BTW, I also recognize this is a thing that reasonable people can disagree about, so I really appreciate the discussion!) |
|
I keep going back and forth, so I think @adnapibar should make the call! |
After reading @CEHENKLE 's comment I think we should backport this to previous versions. We are not breaking the contract of the API, it is only breaking for someone who is incorrectly using the API. Just to be safe, I think we can mention this in the CHANGELOG's that this change might break if the API was incorrectly used previously. (Should we have a breaking section in the CHANGELOG?) |
|
I wouldn't advertise this as "breaking" since we're not breaking the contract of the API :) I'd just add it under the "fixed" section. |
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-5302-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 66c544821f019a30de47f38686e0ce5a3a08cfca
# Push it to GitHub
git push --set-upstream origin backport/backport-5302-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.xThen, create a pull request where the |
|
@adnapibar You'll have to backport manually 😢 |
Yes, I think CHANGELOG causes it, I'll create the backport prs. |
Description
Reject bulk requests that contain invalid actions.
Issues Resolved
Resolves #5299
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.
Signed-off-by: Rabi Panda adnapibar@gmail.com