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.

Sign up for GitHub

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

Jump to bottom

[Backport 2.11] Use new instance of Decompressor on channel initialization #3599

Merged
merged 2 commits into from
Oct 26, 2023
Merged

[Backport 2.11] Use new instance of Decompressor on channel initialization #3599

merged 2 commits into from
Oct 26, 2023

Conversation

opensearch-trigger-bot[bot]
Copy link
Contributor

Backport 499db78 from #3583.

@github-actions @peternied
Use new instance of Decompressor on channel initialization (#3583)
57407ed 
### 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

- opensearch-project/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
- [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](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

---------

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Peter Nied <[email protected]>
Co-authored-by: Peter Nied <[email protected]>
(cherry picked from commit 499db78)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Oct 25, 2023 •
edited
Loading

Codecov Report

Merging #3599 (b6b3184) into 2.11 (75f83bc) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               2.11    #3599      +/-   ##
============================================
- Coverage     64.85%   64.80%   -0.05%     
+ Complexity     3613     3610       -3     
============================================
  Files           273      273              
  Lines         20026    20024       -2     
  Branches       3346     3346              
============================================
- Hits          12987    12977      -10     
- Misses         5396     5404       +8     
  Partials       1643     1643
Files Coverage Δ
...curity/http/SecurityNonSslHttpServerTransport.java 100.00% <100.00%> (ø)
.../ssl/http/netty/Netty4ConditionalDecompressor.java 100.00% <ø> (ø)
...ttp/netty/SecuritySSLNettyHttpServerTransport.java 95.65% <100.00%> (-0.19%) ⬇️

... and 5 files with indirect coverage changes

@DarshitChanpura
Fix hc5 client imports
b6b3184 
Signed-off-by: Darshit Chanpura <[email protected]>
@RyanL1997 RyanL1997 merged commit f27dee2 into 2.11 Oct 26, 2023
@RyanL1997 RyanL1997 deleted the backport/backport-3583-to-2.11 branch October 26, 2023 05:31
@peternied peternied mentioned this pull request Oct 27, 2023
Closed
70 tasks
peternied added a commit to peternied/security that referenced this pull request Nov 21, 2023
@opensearch-trigger-bot @github-actions
@peternied @DarshitChanpura
[Backport 2.11] Use new instance of Decompressor on channel initializ…
71f73d0 
…ation (opensearch-project#3599)

Backport 499db78 from opensearch-project#3583.

---------

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Peter Nied <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Signed-off-by: Darshit Chanpura <[email protected]>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Peter Nied <[email protected]>
Co-authored-by: Darshit Chanpura <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cwperks cwperks cwperks approved these changes

@DarshitChanpura DarshitChanpura DarshitChanpura approved these changes

@RyanL1997 RyanL1997 RyanL1997 approved these changes

@cliu123 cliu123 Awaiting requested review from cliu123 cliu123 is a code owner

@davidlago davidlago Awaiting requested review from davidlago

@peternied peternied Awaiting requested review from peternied peternied is a code owner

@stephen-crawford stephen-crawford Awaiting requested review from stephen-crawford

@reta reta Awaiting requested review from reta reta is a code owner

@willyborankin willyborankin Awaiting requested review from willyborankin willyborankin is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cwperks @DarshitChanpura @RyanL1997