-
Notifications
You must be signed in to change notification settings - Fork 1
54 Initial Fixes #82
base: master
Are you sure you want to change the base?
54 Initial Fixes #82
Conversation
…d (might need to rearrange so it pops more)
…for breaking out IPFIX-RITA's installer to something nicer
… of the installer. Still in the testing/debugging stages
…and additional work
…d mongo check and test/debug non-interactive commands
…ll having awk(ward) problems
…d (might need to rearrange so it pops more)
…for breaking out IPFIX-RITA's installer to something nicer
… of the installer. Still in the testing/debugging stages
…and additional work
…d mongo check and test/debug non-interactive commands
…ll having awk(ward) problems
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.
Went through everything but the new install script proper.
Everything looks good so far.
We need to change the README.md to reflect the new restrictions of the docker and docker-compose versions.
Would it be possible to move the install-scripts/ folder inside of dev-scripts/ ?
I'll begin looking at the install script proper.
cp "$INSTALLER_SCRIPTS_DIR/install_ipfix-rita.sh" "$INSTALLER_DIR" | ||
sed -i "s|INSTALLATION_DIR=\"REPLACE_WITH_INSTALL_DIR\"|INSTALLATION_DIR=\"$INSTALLATION_DIR\"|g" $INSTALLER_DIR/install_ipfix-rita.sh | ||
sed -i "s|INSTALLATION_ETC_DIR=\"REPLACE_WITH_ETC_DIR\"|INSTALLATION_ETC_DIR=\"$INSTALLATION_ETC_DIR\"|g" $INSTALLER_DIR/install_ipfix-rita.sh | ||
sed -i "s|DOCKER_IMAGES=\"./REPLACE_WITH_TARBALL\"|DOCKER_IMAGES=\"$DOCKER_IMAGE_OUT\"|g" $INSTALLER_DIR/install_ipfix-rita.sh |
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.
If these are the only variables we need to set in the install_ipfix-rita.sh script, I'm okay with using sed.
If this grows, we may want to turn to something like envsubst
MIN_VERSION_MAJOR=1 | ||
MIN_VERSION_MINOR=17 | ||
MAX_VERSION_MAJOR=1 | ||
MAX_VERSION_MINOR=23 |
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.
We'll want to document these min/max versions of docker-compose. The min is already documented, but the max is not in the README
fi | ||
|
||
MIN_VERSION_MAJOR=17 | ||
MIN_VERSION_MINOR=05 |
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.
I listed 17.06 as the minimum in the readme, but that can be changed. I think I misread the Docker changelog when looking for when multi-stage builds were supported. For Docker EE its 17.06 for CE its 17.05. Since we use CE for everything, I think 17.05 is fine. The readme will need to be changed to reflect this.
MIN_VERSION_MAJOR=17 | ||
MIN_VERSION_MINOR=05 | ||
MAX_VERSION_MAJOR=18 | ||
MAX_VERSION_MINOR=09 |
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.
We need to list the max version in the readme
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.
If we get the usage_text fixed up and the read/writes all redirected to &2, I think we're ready to roll.
TBH, I'm not certain what all should be on &2 and what shouldn't be. I tried to note any echo's or read's without it.
while [ -n "$1" ]; do | ||
case "$1" in | ||
rita|Rita|RITA) | ||
if [ -n "$2" ] && ! echo "$2" | egrep -iq '(^rita$|^aih$)' ; then |
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.
Is && ! echo "$2" | egrep -iq '(^rita$|^aih$)'
necessary here?
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.
I believe the ! echo "$2" is but the next egrep might not be, we will definately need a second variable since rita addres-to-send-to must be bundled to denote a non-interactive install
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.
I tried removing the egrep and it didn't work so I've added it back in
rita_system="$2" | ||
shift | ||
else | ||
rita_system='127.0.0.1' |
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.
Running ./install_ipfix-rita rita
will install ipfix-rita with the understanding that rita is on localhost?
That doesn't seem entirely obvious to me. This should either be documented or removed since the usage_text asks users to explicitly enter 127.0.0.1
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.
Currently this is in line with the install_acm script from AI-Hunter.
|
||
# Install docker if needed | ||
chmod +x "install-scripts/install_docker.sh" | ||
sudo install-scripts/install_docker.sh |
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.
It looks like we require administrator privileges up above with:
if [[ $EUID -ne 0 ]]; then
echo "This script must be run with administrator privileges."
exit 1
fi
So, we should be able to drop the sudo invocations throughout the install script
|
||
|
||
|
||
do_system_tests () { |
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.
This isn't called. Could we remove the function for the time being or fix it up?
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.
Need to add this in
fi | ||
} #End of askYN | ||
|
||
check_remote_mongo() { |
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.
This will likely need to be a helper script or a rather large function to take care of all the security measures.
#echo "cannot connect to $@" | ||
fi | ||
else | ||
echo "Please supply an ssh target as a command line parameter to can_ssh" |
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.
Does this need >&2
RITA_MONGO_AUTH="null" | ||
#instead of prompting get the RITA address and split from there to fill in our | ||
# config file | ||
echo "" |
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.
I think we want >
or <
&2
from here to the bottom of this function.
# Stop if there are any errors | ||
set -e | ||
# Change dir to script dir | ||
echo "tyring to cd to $(dirname "$BASH_SOURCE[0]")" |
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.
We can probably remove this line.
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.
Thanks, that was a debugging echo
_OLD_DIR=$(pwd); cd "$(dirname "$BASH_SOURCE[0]")"; | ||
|
||
if [[ $EUID -ne 0 ]]; then | ||
echo "This script must be run with administrator privileges." |
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 like we're missing >&2
from here to the bottom of the file. (Mainly at the bottom)
…ing if the user doesn't have a mongo connection string
…ser's understand how everything is packaged in a release.
I ran the installer in a vm and noticed the following issues:
This might stem from the ipfix-rita wrapper script. EDIT: Indeed it does. The copy of ubuntu 16 I am using to test does not symlink /etc/localtime, instead it copies the needed file there. In that case |
…output is sent to stderr
c126074
to
c12e04a
Compare
Merged master into feature branch in order to bring in changes necessary for testing.
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.
Running the installer and just hitting enter breaks since the recommended MongoDB URI is deleted before it gets saved into the converter config.
echo "" >&2 | ||
|
||
echo "What MongoDB URI should IPFIX-RITA use to contact the RITA database [$RITA_MONGO_URI]: " >&2 | ||
read RITA_MONGO_URI <&2 |
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.
This deletes the default RITA_MONGO_URI for the docker host.
The original code reads the response into the default REPLY variable to avoid this.
read -p "What MongoDB URI should IPFIX-RITA use to contact the RITA database [\$RITA_MONGO_URI]: " -r
if [ -n "\$REPLY" ]; then
RITA_MONGO_URI="\$REPLY"
fi
#TODO, this cases issues, we have mongodb://127.0.0.1:27017 then we | ||
# have ritauser@mongodb://127.0.0.1:27017 need to detect if they have | ||
# mongodb and the port before trying to validate | ||
rita_system=`validated_ssh_target rita "$rita_system"` |
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.
We don't need to access the rita system with ssh in order to install ipfix-rita. (Unless we plan on installing rita as well)
Partially fulfills the requirements of Issue #54
The install script should now