Skip to content
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

Fix chunked headers #553

Merged
merged 9 commits into from
Apr 10, 2024

Conversation

AndreKurait
Copy link
Member

@AndreKurait AndreKurait commented Apr 5, 2024

Description

Currently when the replayer accumulates a traffic stream with the HTTP Request Header spanning multiple observations, the chunking logic causes all headers to be lost. This is occurring in the writeHeadersAsChunks method of NettyJsonToByteBufHandler because the netty CompositeByteBuf does not behave as the logic expected it to regarding writing to added component buffers.

This change moves to using a standard netty byte buff with the output stream for headers and slices afterwards to achieve the desired distribution.

This change also includes enhancements to HttpJsonTransformingConsumerTest to test more cases and permutations regarding chunking and transformations.

This change also includes a fix to header equivalency checks when header names are duplicated.

This change also changed some logic for line endings to adhere to the recommended http implementation of CRLF line endings.

  • Category: Bug Fix
  • Why these changes are required? Handle larger headers that span multiple observations
  • What is the old behavior before changes and new behavior after changes? Headers would be dropped yeilding Bad Request when parsing, now headers process normally and request is replayed as expected.

Issues Resolved

MIGRATIONS-1645

Is this a backport? If so, please add backport PR # and/or commits #

Testing

Unit Testing

Check List

  • [x ] New functionality includes testing
    • [ x] All tests pass, including unit test, integration test and doctest
  • New functionality has been documented
  • [x ] Commits are signed per the DCO using --signoff

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.

@@ -118,7 +132,7 @@ private void walkMaps(Object o) {
try (var sampleStream = HttpJsonTransformingConsumer.class.getResourceAsStream(
"/requests/raw/post_formUrlEncoded_withFixedLength.txt")) {
var allBytes = sampleStream.readAllBytes();
Arrays.fill(allBytes, allBytes.length-10, allBytes.length, (byte)' ');
Arrays.fill(allBytes, allBytes.length-30, allBytes.length, (byte)' ');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was needed otherwise netty converted the request instead of parsing in the HttpServerCodec/HttpObjectDecoder. Created a separate item to evaluate behavior and correct if needed https://opensearch.atlassian.net/browse/MIGRATIONS-1647

@AndreKurait AndreKurait marked this pull request as ready for review April 5, 2024 17:46
Copy link

codecov bot commented Apr 5, 2024

Codecov Report

Attention: Patch coverage is 82.05128% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 76.07%. Comparing base (e330325) to head (b04d2a1).
Report is 10 commits behind head on main.

Files Patch % Lines
...search/migrations/replay/HttpByteBufFormatter.java 75.00% 3 Missing ⚠️
...y/datahandlers/http/NettyJsonToByteBufHandler.java 82.35% 1 Missing and 2 partials ⚠️
...tyDecodedHttpRequestPreliminaryConvertHandler.java 66.66% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #553      +/-   ##
============================================
- Coverage     76.29%   76.07%   -0.23%     
- Complexity     1409     1410       +1     
============================================
  Files           155      155              
  Lines          6063     6074      +11     
  Branches        548      548              
============================================
- Hits           4626     4621       -5     
- Misses         1070     1082      +12     
- Partials        367      371       +4     
Flag Coverage Δ
unittests 76.07% <82.05%> (-0.23%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@gregschohn gregschohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change the runtime exception. That's a linter violation.
I'd also like to dig in more and get more context about the ByteBuf release

@@ -29,6 +29,7 @@
@Slf4j
public class HttpByteBufFormatter {

private static final String LINE_DELIMITER = "\r\n";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this should be parameterized to not give those reading logs the false sense that CRLFs may have been included on all requests and responses passed through this class.
I DO think that it's valuable to output these as CRLF, especially when that was the original convention and maybe for some downstream processing. I'd prefer to see this a param that's by default set to use CRLF everywhere except for when we're just using this class this for log output.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to be a passed in parameter

return false;
}
// Depends on header size check above for correctness
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the comment. That's why the size check was important and not just an optimization :)

Copy link
Collaborator

@gregschohn gregschohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good.

@AndreKurait AndreKurait merged commit 49355f9 into opensearch-project:main Apr 10, 2024
6 of 7 checks passed
@AndreKurait AndreKurait deleted the FixChunkedHeaders branch April 10, 2024 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants