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

Upgrade to OpenSearch 2.0.0 #1698

Conversation

cliu123
Copy link
Member

@cliu123 cliu123 commented Mar 22, 2022

Description

[Describe what this change achieves]

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

Issues Resolved

#1642

Testing

UT

Check List

  • 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.

build.gradle Outdated Show resolved Hide resolved
@cliu123 cliu123 force-pushed the bump_opensearch_dependency_to_2.0.0 branch 2 times, most recently from bb831fe to 054d29d Compare March 23, 2022 22:08
@peternied
Copy link
Member

@cliu123 Can you rebase? There were some recent version number simplifications that would make sure the plugin install test would use the same OpenSearch version as the build.

@codecov-commenter
Copy link

codecov-commenter commented Apr 1, 2022

Codecov Report

Merging #1698 (a41afd8) into main (00de760) will decrease coverage by 2.49%.
The diff coverage is 61.11%.

@@             Coverage Diff              @@
##               main    #1698      +/-   ##
============================================
- Coverage     62.88%   60.38%   -2.50%     
+ Complexity     3264     3195      -69     
============================================
  Files           253      253              
  Lines         18102    18095       -7     
  Branches       3247     3245       -2     
============================================
- Hits          11383    10927     -456     
- Misses         5064     5586     +522     
+ Partials       1655     1582      -73     
Impacted Files Coverage Δ
...earch/security/dlic/rest/api/MigrateApiAction.java 4.16% <0.00%> (-63.55%) ⬇️
...urity/ssl/transport/SecuritySSLRequestHandler.java 59.52% <0.00%> (ø)
.../org/opensearch/security/support/ConfigHelper.java 80.00% <ø> (-0.40%) ⬇️
...ty/configuration/ConfigurationLoaderSecurity7.java 65.57% <50.00%> (-3.28%) ⬇️
...a/org/opensearch/security/tools/SecurityAdmin.java 37.50% <57.14%> (-8.06%) ⬇️
...security/auditlog/sink/InternalOpenSearchSink.java 80.76% <100.00%> (ø)
...iance/ComplianceIndexingOperationListenerImpl.java 60.86% <100.00%> (ø)
...ity/configuration/DlsFilterLevelActionHandler.java 55.27% <100.00%> (-0.23%) ⬇️
...org/opensearch/security/httpclient/HttpClient.java 85.88% <100.00%> (ø)
...arch/security/dlic/rest/api/ValidateApiAction.java 10.52% <0.00%> (-68.43%) ⬇️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00de760...a41afd8. Read the comment docs.

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Some feedback on the changes

build.gradle Outdated Show resolved Hide resolved
id = _id;

try {
ConfigHelper.fromYamlFile(filepath, CType.fromString(_id), 1, 0, 0);
ConfigHelper.fromYamlFile(filepath, CType.fromString(_id), 2, 0, 0);
Copy link
Member

Choose a reason for hiding this comment

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

What does it mean for this value to switch from 1 -> 2? Additional documentation would be useful in this code.

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 was for migration Config 6 -> Config 7. But OpenSearch doesn't need to support this migration. Removing the 2 test classes for the same reason.

Copy link
Member

Choose a reason for hiding this comment

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

Can we include documentation in the code as to how this value is determine or aligned to. This could also be done with a local variable eg. final int latestSupportedMajorVersion = 2;

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good idea! Can we do that in a separate PR? Let's unblock 2.0.0 build first.

@cliu123 cliu123 force-pushed the bump_opensearch_dependency_to_2.0.0 branch from 8d13900 to a41afd8 Compare April 1, 2022 22:13
@cliu123 cliu123 marked this pull request as ready for review April 1, 2022 23:06
@cliu123 cliu123 requested a review from a team April 1, 2022 23:06
@cliu123 cliu123 force-pushed the bump_opensearch_dependency_to_2.0.0 branch from a41afd8 to b18b727 Compare April 1, 2022 23:10
@cliu123 cliu123 changed the title Bump opensearch dependency to 2.0.0 Upgrade to OpenSearch 2.0.0 Apr 1, 2022
@cliu123 cliu123 force-pushed the bump_opensearch_dependency_to_2.0.0 branch from b18b727 to 2c54fc0 Compare April 1, 2022 23:12
@cliu123 cliu123 marked this pull request as draft April 2, 2022 04:46
Signed-off-by: cliu123 <lc12251109@gmail.com>
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Note; if we are moving away from gradle.properties we should delete it

@cliu123 cliu123 force-pushed the bump_opensearch_dependency_to_2.0.0 branch 3 times, most recently from e89f56e to 3bb7943 Compare April 4, 2022 19:05
@cliu123 cliu123 force-pushed the bump_opensearch_dependency_to_2.0.0 branch 4 times, most recently from 6db9676 to 89badc7 Compare April 4, 2022 20:56
@cliu123 cliu123 marked this pull request as ready for review April 4, 2022 20:56
@@ -173,7 +173,7 @@ public void onResponse(AcknowledgedResponse response) {
public void onResponse(CreateIndexResponse response) {
final List<SecurityDynamicConfiguration<?>> dynamicConfigurations = builder.build();
final ImmutableList.Builder<String> cTypes = ImmutableList.builderWithExpectedSize(dynamicConfigurations.size());
final BulkRequestBuilder br = client.prepareBulk(opendistroIndex, "_doc");
final BulkRequestBuilder br = client.prepareBulk(opendistroIndex);
Copy link
Member

Choose a reason for hiding this comment

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

out of curiosity, why is this change reqd?

@peternied peternied merged commit 4581a3d into opensearch-project:main Apr 5, 2022
@cliu123 cliu123 deleted the bump_opensearch_dependency_to_2.0.0 branch April 19, 2022 21:59
@cliu123 cliu123 mentioned this pull request Apr 20, 2022
3 tasks
wuychn pushed a commit to ochprince/security that referenced this pull request Mar 16, 2023
Signed-off-by: cliu123 <lc12251109@gmail.com>
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.

4 participants