Skip to content

Add jvm option to allow security manager#5194

Merged
saratvemulapalli merged 1 commit intoopensearch-project:mainfrom
adnapibar:jsm-option-allow
Nov 10, 2022
Merged

Add jvm option to allow security manager#5194
saratvemulapalli merged 1 commit intoopensearch-project:mainfrom
adnapibar:jsm-option-allow

Conversation

@adnapibar
Copy link
Copy Markdown
Contributor

Signed-off-by: Rabi Panda adnapibar@gmail.com

Description

This change explicitly sets JVM options to allow security manager.

Issues Resolved

Resolves #5193

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

@adnapibar adnapibar added the :test Adding or fixing a test label Nov 10, 2022
@adnapibar adnapibar requested review from a team and reta as code owners November 10, 2022 05:54
This change explicitly sets JVM options to  allow security manager.

Signed-off-by: Rabi Panda <adnapibar@gmail.com>
@github-actions
Copy link
Copy Markdown
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Copy Markdown
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #5194 (c18302d) into main (650039c) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##               main    #5194      +/-   ##
============================================
- Coverage     70.98%   70.94%   -0.04%     
+ Complexity    58169    58109      -60     
============================================
  Files          4708     4708              
  Lines        277559   277559              
  Branches      40189    40189              
============================================
- Hits         197016   196925      -91     
- Misses        64385    64430      +45     
- Partials      16158    16204      +46     
Impacted Files Coverage Δ
...rg/opensearch/gradle/OpenSearchTestBasePlugin.java 7.05% <0.00%> (ø)
...g/opensearch/tools/launchers/SystemJvmOptions.java 0.00% <ø> (ø)
...ava/org/opensearch/action/NoSuchNodeException.java 0.00% <0.00%> (-50.00%) ⬇️
.../org/opensearch/client/indices/AnalyzeRequest.java 31.00% <0.00%> (-45.00%) ⬇️
...regations/metrics/AbstractHyperLogLogPlusPlus.java 51.72% <0.00%> (-44.83%) ⬇️
.../search/aggregations/pipeline/HoltLinearModel.java 20.33% <0.00%> (-42.38%) ⬇️
...cluster/coordination/PublishClusterStateStats.java 33.33% <0.00%> (-37.51%) ⬇️
...pensearch/action/ingest/DeletePipelineRequest.java 31.25% <0.00%> (-37.50%) ⬇️
...search/search/aggregations/pipeline/EwmaModel.java 24.44% <0.00%> (-33.34%) ⬇️
...a/org/opensearch/search/profile/ProfileResult.java 72.58% <0.00%> (-24.20%) ⬇️
... and 446 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@saratvemulapalli
Copy link
Copy Markdown
Member

@adnapibar do you want to back to 2.x?

@saratvemulapalli saratvemulapalli merged commit e10c925 into opensearch-project:main Nov 10, 2022
} else {
test.systemProperty("java.locale.providers", "SPI,COMPAT");
test.jvmArgs("--illegal-access=warn");
test.jvmArgs("--illegal-access=warn", "-Djava.security.manager=allow");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@adnapibar thanks for the change, I believe we have to make it conditional based on JDK (the new allow setting was introduced in JDK-18 and onwards), otherwise JVM fails to start

Copy link
Copy Markdown
Contributor Author

@adnapibar adnapibar Nov 10, 2022

Choose a reason for hiding this comment

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

@reta thanks! for pointing that out, I'm updating it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:test Adding or fixing a test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tests fail due to security manager issue

4 participants