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

Updates Dev guide #1590

Merged
merged 7 commits into from
Apr 5, 2022

Conversation

DarshitChanpura
Copy link
Member

@DarshitChanpura DarshitChanpura commented Jan 28, 2022

Description

Updates Developer guide to add more explanation to the steps

  • Documentation
  • Updating steps

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.

Signed-off-by: Darshit Chanpura <[email protected]>
@DarshitChanpura DarshitChanpura requested a review from a team January 28, 2022 05:14
@codecov-commenter
Copy link

codecov-commenter commented Jan 28, 2022

Codecov Report

Merging #1590 (e08fe5b) into main (5b994af) will decrease coverage by 1.69%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #1590      +/-   ##
============================================
- Coverage     64.57%   62.88%   -1.70%     
- Complexity     3215     3264      +49     
============================================
  Files           247      253       +6     
  Lines         17351    18102     +751     
  Branches       3082     3247     +165     
============================================
+ Hits          11205    11383     +178     
- Misses         4597     5064     +467     
- Partials       1549     1655     +106     
Impacted Files Coverage Δ
...ensearch/security/action/whoami/WhoAmIRequest.java 0.00% <0.00%> (-100.00%) ⬇️
...nsearch/security/action/whoami/WhoAmIResponse.java 0.00% <0.00%> (-82.86%) ⬇️
.../security/action/whoami/TransportWhoAmIAction.java 33.33% <0.00%> (-58.34%) ⬇️
...nsearch/security/ssl/ExternalSecurityKeyStore.java 8.00% <0.00%> (-40.00%) ⬇️
...rity/action/configupdate/ConfigUpdateResponse.java 64.28% <0.00%> (-35.72%) ⬇️
...rch/security/transport/SecurityRequestHandler.java 57.89% <0.00%> (-11.85%) ⬇️
...search/security/configuration/DlsFlsValveImpl.java 59.25% <0.00%> (-10.97%) ⬇️
.../org/opensearch/security/auth/BackendRegistry.java 48.43% <0.00%> (-10.94%) ⬇️
...urity/ssl/transport/SecuritySSLNettyTransport.java 62.36% <0.00%> (-6.79%) ⬇️
.../action/configupdate/ConfigUpdateNodeResponse.java 81.81% <0.00%> (-5.69%) ⬇️
... and 41 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 5b994af...e08fe5b. Read the comment docs.

davidlago
davidlago previously approved these changes Jan 28, 2022
@DarshitChanpura DarshitChanpura requested a review from a team February 17, 2022 03:39
davidlago
davidlago previously approved these changes Feb 17, 2022
@@ -84,7 +94,10 @@ Detected OpenSearch Security Version: *
### (Ignore the SSL certificate warning because we installed self-signed demo certificates)
```

Now if we start our server again and try the original `curl localhost:9200`, it will fail. Try this one instead: `curl -XGET https://localhost:9200 -u 'admin:admin' --insecure`. You can also make this call to return the authenticated user details:
Now if we start our server again and try the original `curl localhost:9200`, it will fail.
Try this one instead: `curl -XGET https://localhost:9200 -u 'admin:admin' --insecure`. It should succeed.
Copy link
Member

Choose a reason for hiding this comment

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

Feel free to create a new issue
--insecure feels like a bad option to advertise. After the script has finished running can we add a step to install the CA cert that was generated from the install script so this option can be removed?

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 is something that we can discuss and set what needs to be displayed here.

DEVELOPER_GUIDE.md Outdated Show resolved Hide resolved
DEVELOPER_GUIDE.md Outdated Show resolved Hide resolved

To get started, follow the [getting started section](https://github.com/opensearch-project/OpenSearch/blob/main/DEVELOPER_GUIDE.md#getting-started) of OpenSearch's developer guide. This will get you up and running with OpenSearch built from source code. Get to the point where you can run a successful `curl localhost:9200` call (you can skip the `./gradlew check` step to save some time. Great! now kill the server with `Ctrl+c` and copy the built code to a new folder somewhere else where you'll be running your service (run this from the base directory of the OpenSearch fork you cloned above):
>Please note:\
> OpenSearch builds on `gradle`, and this plugin is packaged using `maven`.
Copy link
Member

Choose a reason for hiding this comment

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

Could we reference the instructions for how to install the jdk versions we support, OpenSearch has a markdown doc we could link?

@peternied
Copy link
Member

peternied commented Feb 24, 2022

@DarshitChanpura Could I help getting this PR merged?

@DarshitChanpura
Copy link
Member Author

@DarshitChanpura Could I help getting this PR merged?

@peternied yes..that would be awesome!

@peternied
Copy link
Member

peternied commented Apr 4, 2022

@DarshitChanpura I've created a PR with some of this feedback inside it, thanks for waiting for me to get around to this :)

DarshitChanpura#1

Recommendations for the dev guide
@peternied peternied merged commit 3f7fffb into opensearch-project:main Apr 5, 2022
@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
* Updates Dev guide

Signed-off-by: Darshit Chanpura <[email protected]>
Co-authored-by: Peter Nied <[email protected]>
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