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

Addresses changes made to security demo config install tool #233

Merged
merged 3 commits into from
Apr 16, 2024
Merged

Addresses changes made to security demo config install tool #233

merged 3 commits into from
Apr 16, 2024

Conversation

DarshitChanpura
Copy link
Member

Description

There were recent changes to security demo configuration setup which now requires a custom admin Password to be setup. This PR updates the documentation and related code change.

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has documentation added
  • Commits are signed as 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.

@DarshitChanpura DarshitChanpura mentioned this pull request Dec 15, 2023
Closed
2 tasks
dlvenable
dlvenable reviewed Dec 15, 2023
View reviewed changes
Copy link
Member

@dlvenable dlvenable left a comment

Choose a reason for hiding this comment

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

@DarshitChanpura , Thank you for making this change.

When will this new password be the default in OpenSearch?

@derek-ho
Copy link

@dlvenable this change will be made in 2.12.0. Also myStrongPassword123! is not the new default - it is set as in the env file of this PR. However, I double checked the CI workflow, and it seems like the latest this repo is running is 2.1.0, so I think this PR is not necessary. Please let me know if you agree - if at some point later on this PR starts running CI with 2.12.0 or onwards, there may be changes needed. Otherwise I will close this out.

@DarshitChanpura
Copy link
Member Author

@dlvenable @asifsmohammed @oeyh Can we get some reviews on this? Also, as @derek-ho mentioned, if this PR is not required please feel free to close it.

@DarshitChanpura
Copy link
Member Author

@dlvenable @asifsmohammed @oeyh Can we get some reviews on this?

dlvenable
dlvenable requested changes Jan 31, 2024
View reviewed changes
scripts/docker.env Outdated
JRUBY_OPTS=-Xregexp.interruptible=true -Xcompile.invokedynamic=true -Xjit.threshold=0 -J-XX:+UseParallelGC -J-XX:+PrintCommandLineFlags -v -W1
OPENSEARCH_INITIAL_ADMIN_PASSWORD=myStrongPassword123!
Copy link
Member

Choose a reason for hiding this comment

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

If we change this to admin, then we can use admin below and it should work for older versions of OpenSearch. Can we make that change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it work for 2.12 and later? If not and you think this PR is not required until 2.12 we can mark it as draft

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, admin is recognized as weak password and so the setup would fail. Hence we defined a new password with relatively strong value myStrongPassword123!

scripts/logstash-run.sh Outdated
@@ -6,7 +6,7 @@ export PATH=$BUILD_DIR/gradle/bin:$PATH
SERVICE_URL="http://integration:9200"

if [[ "$SECURE_INTEGRATION" == "true" ]]; then
SERVICE_URL="https://integration:9200 -k -u admin:admin"
SERVICE_URL="https://integration:9200 -k -u admin:myStrongPassword123!"
Copy link
Member

Choose a reason for hiding this comment

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

You could set this to admin to help the build pass.

Copy link
Member Author

Choose a reason for hiding this comment

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

i don't think that is the root-cause of failures but I maybe wrong here.
In the latest CI run, the test run with logstash 7.17.1 with OpenSearch 2.1.0 seem to have passed[1] but the same failed[2] for 1.3.4, when neither of these versions contain admin changes.
[1] - https://github.com/opensearch-project/logstash-output-opensearch/actions/runs/7227042889/job/19693902342?pr=233
[2] - https://github.com/opensearch-project/logstash-output-opensearch/actions/runs/7227042889/job/19693901900?pr=233

@DarshitChanpura
Copy link
Member Author

@dlvenable @oeyh @asifsmohammed Now that 2.12 is released this should be unblocked. Would one of you mind bringing this home?

@dblock
Copy link
Member

dblock commented Feb 27, 2024

@DarshitChanpura rebase? Let's see a green CI?

@DarshitChanpura
Copy link
Member Author

DarshitChanpura commented Feb 27, 2024 •
edited
Loading

@dblock the branch is already pointing to latest commit on main. The CI failures seem unrelated to the changes in this PR: #233 (comment).

Maybe flaky? A retry of CI tasks could help determine if they are indeed flaky. @dlvenable Thoughts?

@dblock
Copy link
Member

dblock commented Feb 27, 2024

The run is too old to be retried via the GitHub UX, do a git commit --amend then force push and it will kick it off.

@DarshitChanpura
Copy link
Member Author

done.

@dblock
Copy link
Member

dblock commented Feb 28, 2024

Not sure if it's related, but spec failures look legit, someone will need to look at those.

@DarshitChanpura
Copy link
Member Author

DarshitChanpura commented Feb 28, 2024 •
edited
Loading

@dlvenable @asifsmohammed @oeyh Can one of you help resolve the CI and bring this home?

Edit; looking into CI failures, seems like a version check is required.

@DarshitChanpura DarshitChanpura force-pushed the main branch 6 times, most recently from 80eb3fa to 07fc6e8 Compare February 28, 2024 20:44
derek-ho
derek-ho approved these changes Feb 29, 2024
View reviewed changes
Copy link

@derek-ho derek-ho left a comment

Choose a reason for hiding this comment

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

Thanks for bringing this home @DarshitChanpura

@DarshitChanpura DarshitChanpura force-pushed the main branch 3 times, most recently from 8af1aac to bfc0064 Compare February 29, 2024 16:04
@DarshitChanpura DarshitChanpura force-pushed the main branch 2 times, most recently from 823b7e5 to 14a7cd5 Compare February 29, 2024 16:18
@DarshitChanpura
Fixes remainder admin occurences, adds version check for admin passwo…
81fff85 
…rd determination and adds 2.12.0 to CI matrix

Signed-off-by: Darshit Chanpura <[email protected]>
@DarshitChanpura
Copy link
Member Author

@dblock @dlvenable @asifsmohammed @oeyh CI is green now. Mind adding some reviews?

@dblock
Copy link
Member

dblock commented Feb 29, 2024

LGTM, I'm not a maintainer, leaving this to others!

@dlvenable dlvenable merged commit f45b8c7 into opensearch-project:main Apr 16, 2024
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derek-ho derek-ho derek-ho approved these changes

@oeyh oeyh oeyh approved these changes

@dlvenable dlvenable dlvenable approved these changes

@asifsmohammed asifsmohammed Awaiting requested review from asifsmohammed

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

Successfully merging this pull request may close these issues.
5 participants
@DarshitChanpura @derek-ho @dblock @dlvenable @oeyh