Added alpine based dockerfile and mysql database setup via docker run… #940
Conversation
There was a problem hiding this comment.
Please also see our contribution guidelines. In particular, every change needs a corresponding JIRA issue, and that JIRA issue needs to be included in each commit message (see established git history for the formatting convention we use for this).
| "$SQLSERVERQL_AUTO_CREATE_ACCOUNTS" | ||
|
|
||
| set_optional_property \ | ||
| "sqlserver-instance" \ | ||
| "$SQLSERVER_INSTANCE" | ||
| "$SQLSERVERQL_INSTANCE" |
There was a problem hiding this comment.
Why were these renamed?
| echo "RESPONSE: $RESPONSE"; | ||
| if [ "$RESPONSE" == "" ]; then | ||
| if [[ $retries -le $RETRIES ]] ; then | ||
| echo "Retired $retries"; |
There was a problem hiding this comment.
What do you mean by "retired"?
| # Check database access | ||
| for retries in $(seq 0 $((RETRIES+ 1))); do | ||
| RESPONSE=$(echo "exit" | timeout $SLEEP_SHORT telnet $DB_HOST $DB_PORT|grep Connected); | ||
| echo "RESPONSE: $RESPONSE"; | ||
| if [ "$RESPONSE" == "" ]; then | ||
| if [[ $retries -le $RETRIES ]] ; then | ||
| echo "Retired $retries"; | ||
| echo "Expected $DB_HOST currently not available, wait $SLEEP_SHORT seconds."; | ||
| sleep $SLEEP_SHORT; | ||
| else | ||
| for retries2 in $(seq 0 $((RETRIES+ 1))); do | ||
| RESPONSE=$(echo "exit" | timeout $SLEEP_SHORT telnet $DB_HOST $DB_PORT|grep Connected); | ||
| if [ "$RESPONSE" == "" ]; then | ||
| if [[ $retries2 -le $RETRIES ]] ; then | ||
| echo "Retired $retries2"; | ||
| echo "Expected $DB_HOST currently not available, wait $SLEEP_LONG seconds."; | ||
| sleep $SLEEP_LONG; | ||
| else | ||
| echo "Expected $DB_HOST currently not available, and reached all reries limit, exiting"; | ||
| exit 1; | ||
| fi | ||
| else | ||
| echo "Expected $DB_HOST accessed successfuly, continue database init processes." | ||
| break; | ||
| fi | ||
| done | ||
| fi | ||
| else | ||
| echo "Expected $DB_HOST accessed successfuly, continue database init processes." | ||
| break; | ||
| fi | ||
| done |
There was a problem hiding this comment.
- The logic here feels overcomplicated for what it is intended to achieve (wait for the database to come up). Why so many nested loops and layers of retrying?
- I'm on the fence on building this functionality into the Docker image. Perhaps adding automatic initialization into the database support within the webapp itself would make more sense? That would also allow for automatic application of schema updates while maintaining separation of concerns.
There was a problem hiding this comment.
Perhaps adding automatic initialization into the database support within the webapp itself would make more sense?
+1
| if [[ "$MYSQL_USER" != "" && "$MYSQL_PASSWORD" != "" && "$MYSQL_DATABASE" != "" && "$MYSQL_HOSTNAME" != "" ]]; then | ||
|
|
||
| if [ "$MYSQL_PORT" == "" ]; then | ||
| DB_PORT=3306; | ||
| else | ||
| DB_PORT=$MYSQL_PORT; | ||
| fi | ||
|
|
||
| DB_HOST=$MYSQL_HOSTNAME; | ||
| DB_NAME=$MYSQL_DATABASE; | ||
| DB_USER=$MYSQL_USER; | ||
| DB_PASSWORD=$MYSQL_PASSWORD; | ||
| DB_ENGINE="/usr/bin/mariadb" | ||
|
|
||
|
|
||
| UUID=$(openssl rand -hex 16) | ||
| SALT=$(echo ${UUID:0:8}-${UUID:8:4}-${UUID:12:4}-${UUID:16:4}-${UUID:20:12} | openssl sha256 -hex | awk '{print toupper($2)}'); | ||
| HASH=$(echo -n "$ADMIN_PASSWORD$SALT" | openssl sha256 -hex | awk '{print toupper($2)}') | ||
| cp /opt/guacamole/mysql/schema/*.sql /tmp/ | ||
| sed -i "s/guacadmin/$ADMIN_NAME/g" /tmp/002-create-admin-user.sql | ||
| sed -i "s/FE24ADC5E11E2B25288D1704ABE67A79E342ECC26064CE69C5B3177795A82264/$SALT/" /tmp/002-create-admin-user.sql | ||
| sed -i "s/CA458A7D494E3BE824F5E1E175A1556C0F8EEF2C2D7DF3633BEC4A29C4411960/$HASH/" /tmp/002-create-admin-user.sql |
There was a problem hiding this comment.
This would need to be implemented for each supported database, not just MySQL/MariaDB.
| if [ "$SLEEP_SHORT" == "" ]; then | ||
| SLEEP_SHORT=5 | ||
| fi | ||
|
|
||
| if [ "$SLEEP_LONG" == "" ]; then | ||
| SLEEP_LONG=10 | ||
| fi | ||
|
|
||
| if [ "$RETRIES" == "" ]; then | ||
| RETRIES=5 | ||
| fi |
There was a problem hiding this comment.
If new environment variables are to be supported by this image, the naming should be more specific. SLEEP_SHORT, SLEEP_LONG, and RETRIES are too broad.
Dockerfile.alpine
Outdated
There was a problem hiding this comment.
Why a second Dockerfile vs. modifying the existing Dockerfile?
| for i in $(ls /tmp/*.sql); do | ||
| CREATE_DATABASE=$($DB_ENGINE -u $DB_USER -p$DB_PASSWORD $DB_NAME -h $DB_HOST --port $DB_PORT < $i); | ||
| if [[ "$(echo $?)" == "1" ]]; then | ||
| echo "Database already exists"; | ||
| exit 2; | ||
| else | ||
| echo "Database automaticaly initialized." ; | ||
| fi | ||
| done |
There was a problem hiding this comment.
This is dangerous. Should a user ever manage to write a file to /tmp with a .sql extension, that file would be automatically run at subsequent startup as a privileged database user. Merely writing files to /tmp is not a privileged operation, so this would be privilege escalation.
A modification has been made during container image creation, allowing for a significantly smaller Docker image to be generated from the source by utilizing the new file (Dockerfile.alpine).
The application's startup script has been adjusted to enable the specification of the Docker runtime, including the first (admin) user's name and corresponding password (via the ADMIN_NAME and ADMIN_PASSWORD variables). Simultaneously, database initialization has been automated (currently supporting only MySQL databases) through the invocation of a separate script. This script checks whether the database server is accessible and if a database has already been created.