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

Updates tar distribution to conform to changes in install demo configuration script in security plugin #4250

Merged
merged 12 commits into from
Jan 16, 2024
Merged

Updates tar distribution to conform to changes in install demo configuration script in security plugin #4250

Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
142590d
Updates tar distribution to conform to changes in install demo config…
DarshitChanpura Nov 29, 2023
0b2a10f
Merge remote-tracking branch 'upstream/main' into tar-dist-update
DarshitChanpura Dec 5, 2023
7b37d44
Updates log statement
DarshitChanpura Dec 7, 2023
78f3345
Updates info log
DarshitChanpura Dec 8, 2023
595bb91
Merge remote-tracking branch 'upstream/main' into tar-dist-update
DarshitChanpura Dec 13, 2023
bc713dd
Updates the info message to reflect env variable name change
DarshitChanpura Dec 13, 2023
d50eff1
Corrects info logs
DarshitChanpura Dec 14, 2023
433c0bc
Captializes security plugin name in info logs
DarshitChanpura Dec 14, 2023
6c76e91
Merge remote-tracking branch 'upstream/main' into tar-dist-update
DarshitChanpura Jan 11, 2024
805c86f
Updates multiple echos with a single printf
DarshitChanpura Jan 11, 2024
1f4b6b0
Merge remote-tracking branch 'upstream/main' into tar-dist-update
DarshitChanpura Jan 16, 2024
840a220
Change to echo -e
DarshitChanpura Jan 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion scripts/startup/tar/linux/opensearch-tar-install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ cd $OPENSEARCH_HOME
KNN_LIB_DIR=$OPENSEARCH_HOME/plugins/opensearch-knn/lib
##Security Plugin
if [ -d "$OPENSEARCH_HOME/plugins/opensearch-security" ]; then
bash $OPENSEARCH_HOME/plugins/opensearch-security/tools/install_demo_configuration.sh -y -i -s
echo -e "OpenSearch 2.12.0 onwards, the OpenSearch Security Plugin introduces a change that requires an initial password for 'admin' user. \nPlease define an environment variable 'OPENSEARCH_INITIAL_ADMIN_PASSWORD' with a strong password string. \nIf a password is not provided, the setup will quit."
bash $OPENSEARCH_HOME/plugins/opensearch-security/tools/install_demo_configuration.sh -y -i -s || exit 1
Copy link
Member

Choose a reason for hiding this comment

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

After talking to @prudhvigodithi we should remove the exit 1, and add set -e to the install script, as the original design removed that due to chmod 777 /dev/shm, which has been removed some time ago.

Copy link
Member

Choose a reason for hiding this comment

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

More conversation with @DarshitChanpura and seems like set -e might have more impact on docker side. He will get more info on that and we will need some more discussion on it with @prudhvigodithi.

I am ok with either approach, just think it would be better if we are consistent across all distributions.

Copy link
Member Author

Choose a reason for hiding this comment

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

set -e affects the entire file setup and analyzing that change is out of scope, IMO. @prudhvigodithi @peterzhuamazon please lmk if we should indeed use set -e. TAR is much simple to analyze and can be changed to use set -e, however, docker setup is bit more involved in terms of understanding all possible code paths. Windows however doesn't have set -e from what I understand, and hence will require some form of || exit /b 1.

So, for consistency purposes I'd vote on keeping it as is with || exit 1. This has been extensively tested on TAR, docker and Windows, and the setup works as expected with this change.

@prudhvigodithi @peterzhuamazon @rishabh6788 thoughts?

echo "done security"
fi

Expand Down
Loading