Use new instance of Decompressor on channel initialization

[cwperks]

Description

Resolves an issue with decompression that can lead to concurrent gzipped requests failing. This removes the @Sharable annotation from the Netty4ConditionalDecompressor and creates a new instance of the decompressor on channel initialization.

Netty4ConditionalDecompressor is an HttpContentDecompressor which is a subclass of HttpContentDecoder - a stateful handler. Netty docs on @Sharable annotation: https://netty.io/4.0/api/io/netty/channel/ChannelHandler.Sharable.html

• Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)

Bug fix

Issues Resolved

• [Regression] Request decompression was broken in 2.11 OpenSearch#10802

Testing

Tested by running OpenSearch w fluentbit and Merge_Log on. See files below which can reproduce the issue from the linked error.

I opened this PR as draft pending an integration test to validate the behavior.

docker-compose.yml

version: '3'
services:
  opensearch: # This is also the hostname of the container within the Docker network (i.e. https://opensearch-node1/)
    image: opensearchproject/opensearch:latest # Specifying the latest available image - modify if you want a specific version
    container_name: opensearch
    environment:
      - cluster.name=opensearch-cluster # Name the cluster
      - node.name=opensearch # Name the node that will run in this container
      - discovery.type=single-node
      - bootstrap.memory_lock=true # Disable JVM heap memory swapping
      - "OPENSEARCH_JAVA_OPTS=-Xms512m -Xmx512m" # Set min and max JVM heap sizes to at least 50% of system RAM
    ulimits:
      memlock:
        soft: -1 # Set memlock to unlimited (no soft or hard limit)
        hard: -1
      nofile:
        soft: 65536 # Maximum number of open files for the opensearch user - set to at least 65536
        hard: 65536
    volumes:
      - opensearch-data1:/usr/share/opensearch/data # Creates volume called opensearch-data1 and mounts it to the container
      # - /Users/craigperkins/Projects/OpenSearch/security/build/distributions/opensearch-security-2.11.0.0-SNAPSHOT.jar:/usr/share/opensearch/plugins/opensearch-security/opensearch-security-2.11.0.0.jar
    ports:
      - 9200:9200 # REST API
      - 9600:9600 # Performance Analyzer
    networks:
      - opensearch-net # All of the containers will join the same Docker bridge network
  fluent-bit:
    image: fluent/fluent-bit
    volumes:
      - ./fluent-bit.conf:/fluent-bit/etc/fluent-bit.conf
    depends_on:
      - opensearch
    networks:
      - opensearch-net

volumes:
  opensearch-data1:
  opensearch-data2:

networks:
  opensearch-net:

fluent-bit.conf

[INPUT]
  Name dummy
  Dummy {"top": {".dotted": "value"}}

[OUTPUT]
  Name es
  Host opensearch
  Port 9200
  HTTP_User admin
  HTTP_Passwd admin
  Replace_Dots On
  Suppress_Type_Name On
  Compress gzip
  tls On
  tls.verify Off
  net.keepalive Off

[FILTER]
  Name kubernetes
  Match kube.*
  Buffer_Size 256KB
  Merge_Log On
  Keep_Log On

Check List

• New functionality includes testing
• New functionality has been documented
• 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.

[codecov]

Codecov Report

Merging #3583 (78242e8) into main (4f89b4a) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #3583      +/-   ##
============================================
+ Coverage     64.90%   64.92%   +0.01%     
- Complexity     3637     3640       +3     
============================================
  Files           284      284              
  Lines         20618    20616       -2     
  Branches       3390     3390              
============================================
+ Hits          13383    13384       +1     
+ Misses         5555     5551       -4     
- Partials       1680     1681       +1     

Files	Coverage Δ
...curity/http/SecurityNonSslHttpServerTransport.java	100.00% <100.00%> (ø)
.../ssl/http/netty/Netty4ConditionalDecompressor.java	100.00% <ø> (ø)
...ttp/netty/SecuritySSLNettyHttpServerTransport.java	85.36% <100.00%> (-0.35%)	⬇️

... and 1 file with indirect coverage changes

[peternied]

Thanks @cwperks - how do we know that the Header Verifier is safe to keep as is?

[stephen-crawford]

Looks good to me

[DarshitChanpura]

Thank you @cwperks & @peternied for fixing this!

[wbeckler]

Is backporting this to 2.11 a violation of semver as outlined in OpenSearch's patch version policy, which says "Small improvements and features will be backported to a new minor version.... Security fixes will be backported to a new patch version."

Maybe this should only go into 2.12, so as to not violate the expectation that 2.11.1 would only contain security fixes.

[cwperks]

@wbeckler 2.11.1 is being created for this fix: opensearch-project/opensearch-build#4161 (comment)

[bbarani]

As per official semVar guidelines, Patch version Z (x.y.Z | x > 0) MUST be incremented if only backward compatible bug fixes are introduced. A bug fix is defined as an internal change that fixes incorrect behavior.

[dblock]

@bbarani that assumes that we make x.y.z.N for security fixes, doesn’t it?