-
Notifications
You must be signed in to change notification settings - Fork 280
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
Extracts demo configuration script to a java tool #3669
Extracts demo configuration script to a java tool #3669
Conversation
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
…tion and removes temp scripts Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
…e directory path Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
…t as execution environment Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
CI
Script is doing what it is supposed to. I will update the CI task to pass option |
…-validation Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
src/main/java/org/opensearch/security/tools/InstallDemoConfiguration.java
Outdated
Show resolved
Hide resolved
…uction to demo Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Spotbugs CI check will be fixed once #3676 is merged |
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
a7defce
to
a2c327d
Compare
@cwperks @RyanL1997 @scrawfor99 @peternied Could I get some reviews on this ? |
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3669 +/- ##
==========================================
+ Coverage 64.88% 65.25% +0.36%
==========================================
Files 292 297 +5
Lines 20776 21127 +351
Branches 3409 3451 +42
==========================================
+ Hits 13481 13786 +305
- Misses 5606 5643 +37
- Partials 1689 1698 +9
|
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @DarshitChanpura , thanks for putting this together. In generally, this is java tool is well structured and clear, and I just left some comments.
src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java
Show resolved
Hide resolved
src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java
Show resolved
Hide resolved
src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java
Show resolved
Hide resolved
src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a first pass. This change looks good to me.
What tests should be added for this PR? I'd love to have a bwc test where the old script is run in 2.11 w/ the bash script and another with the Java version of the script and show that the output is identical.
src/main/java/org/opensearch/security/tools/democonfig/ExecutionEnvironment.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/tools/democonfig/Installer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good to me, just a few comments but overall great job with this.
Can we also unit tests each of these parts now?
src/main/java/org/opensearch/security/tools/democonfig/CertificateGenerator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/tools/democonfig/Installer.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/tools/democonfig/Installer.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java
Show resolved
Hide resolved
I have a follow-up PR that I'm working on to specifically add tests for this. |
…m variables to all-caps Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
@RyanL1997 @cwperks @scrawfor99 Thank you for the detailed feedback!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me ! Thanks DC :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Nice work @DarshitChanpura !
Hi @scrawfor99 and @DarshitChanpura. Since I previously resolved the comment, I will reply it here for future reference. I think the unit tests has its own test configuration and that will be fine, but it does impact the integration test. But we can have a workaround in CI. You can reference to the experimental work I did here: And the impact tests is in this comment: https://github.com/opensearch-project/security/pull/3449/files#r1346368658 |
17748b9
into
opensearch-project:main
…#3669) This change extracts the core demo configuration install script to java tool to allow re-usability while leveraging that OpenSearch itself is JAVA process. All other tools shipped by security are java tools, and now with this PR, demo configuration setup will also be a java tool. --------- Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
…t for Bundled JDK for this tool and updates DEVELOPER_GUIDE.md (#3845) ### Description Backports following commits related to demo configuration tool from main to 2.x: - [x] [17748b9](17748b9) from #3669 - [x] [4496440](4496440) from #3734 - [x] [06d8c29](06d8c29) from #3777 - [x] [e698315](e698315) from #3807 - [x] [9d11524](9d11524) from #3843 - [x] [62aed21](62aed21) from #3850 - [x] [ceabe13](ceabe13) from #3844 ### Issues Resolved - Related to #3827 ### Testing - automated tests ### Check List - [x] New functionality includes testing - [x] 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: Darshit Chanpura <dchanp@amazon.com> Signed-off-by: Darshit Chanpura <35282393+DarshitChanpura@users.noreply.github.com>
Description
This change extracts the core demo configuration install script to java tool to allow re-usability while leveraging that OpenSearch itself is JAVA process. All other tools shipped by security are java tools, and now with this PR, demo configuration setup will also be a java tool.
Category : Enhancement, Refactoring
Why these changes are required?
The need to implement these changes arose from the requirement to replace default admin credentials with a custom provided admin password.
What is the old behavior before changes and new behavior after changes?
The script execution behavior has changed slightly. Here's how:
-t
to skip password validation for test environments.Issues Resolved
install_demo_configuration
script to a standalone Java Tool #3633Testing
Automated testing will be implemented in a follow-up PR.
Manual testing.
Tests:
1. Works without any options:
a. `-h`
b. `-y`
c. `-i`
d. `-c`
e. `-s`
3. Sets the custom admin password, if one is supplied
4. Generates a random password, if one is not supplied
5. Accepts a new option `-t` to disable password validation for test environment
Check List
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.