Seekdb optimize #68
Seekdb optimize #68
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request optimizes the seekdb Docker deployment with three main improvements: extended wait times for bootstrap operations, restructured database initialization logic, and a new feature to execute SQL commands and exit. The changes aim to improve reliability and usability of the seekdb Docker image.
- Extended wait time constants from 10 seconds to 300-600 seconds for various bootstrap operations
- Refactored database creation to occur independently of init scripts execution
- Added new functionality to execute SQL commands directly via docker run arguments and exit
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| seekdb/start.sh | Core changes including wait time increases, database initialization refactoring, and new SQL execution feature |
| seekdb/README.md | Updated capitalization from "SeekDB" to "seekdb" for consistency |
| seekdb/README_CN.md | Updated capitalization from "SeekDB" to "seekdb" for consistency (Chinese version) |
| seekdb/Dockerfile | Fixed ENV variable syntax to use equals sign instead of space |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fi | ||
|
|
||
| echo "Executing sql: $*" | ||
| mysql $MYSQL_OPTS -e "$*" |
There was a problem hiding this comment.
The use of "$" for SQL command execution could be problematic when the SQL contains special characters or multiple statements. The "$" concatenates all arguments into a single string separated by spaces, which may not preserve the original quoting and escaping. Consider using "$@" instead, or ensure that the SQL is properly passed as a single quoted argument when calling the script.
| mysql $MYSQL_OPTS -e "$*" | |
| mysql $MYSQL_OPTS -e "$@" |
|
|
||
| ## 数据持久化 | ||
| SeekDB 部署在 `/var/lib/oceanbase` 目录中，如果您想将数据持久化到主机服务器，请将主机服务器上的空目录挂载到此路径。 | ||
| Seekdb 部署在 `/var/lib/oceanbase` 目录中，如果您想将数据持久化到主机服务器，请将主机服务器上的空目录挂载到此路径。 |
There was a problem hiding this comment.
Inconsistent capitalization of "Seekdb". The word appears with capital "S" in the sentence but lowercase "seekdb" is used elsewhere in the document. For consistency with the rest of the README where "seekdb" is used (line 2, 6, 18, 20, etc.), this should be "seekdb" with a lowercase "s".
| Seekdb 部署在 `/var/lib/oceanbase` 目录中，如果您想将数据持久化到主机服务器，请将主机服务器上的空目录挂载到此路径。 | |
| seekdb 部署在 `/var/lib/oceanbase` 目录中，如果您想将数据持久化到主机服务器，请将主机服务器上的空目录挂载到此路径。 |
|
|
||
| ## Data Persistence | ||
| SeekDB deploys in directory /var/lib/oceanbase, if you'd like to persist the data on the host server, please mount an empty directory on the host server to this path. | ||
| Seekdb deploys in directory /var/lib/oceanbase, if you'd like to persist the data on the host server, please mount an empty directory on the host server to this path. |
There was a problem hiding this comment.
Inconsistent capitalization of "Seekdb". The word appears with capital "S" in the sentence but lowercase "seekdb" is used elsewhere in the document. For consistency with the rest of the README where "seekdb" is used (line 2, 6, 18, 20, etc.), this should be "seekdb" with a lowercase "s".
| Seekdb deploys in directory /var/lib/oceanbase, if you'd like to persist the data on the host server, please mount an empty directory on the host server to this path. | |
| seekdb deploys in directory /var/lib/oceanbase, if you'd like to persist the data on the host server, please mount an empty directory on the host server to this path. |
|
|
||
| # Execute the main process | ||
| /usr/libexec/oceanbase/scripts/seekdb_systemd_start | ||
| /usr/libexec/oceanbase/scripts/seekdb_systemd_start 2>/dev/null |
There was a problem hiding this comment.
Redirecting stderr to /dev/null may hide important error messages from the seekdb startup process. If the service fails to start, debugging will be more difficult without error output. Consider redirecting to a log file instead, or at least document why errors are being suppressed here.
| /usr/libexec/oceanbase/scripts/seekdb_systemd_start 2>/dev/null | |
| /usr/libexec/oceanbase/scripts/seekdb_systemd_start 2>>/var/log/seekdb_start.err.log |
| MYSQL_OPTS="-h127.0.0.1 -P2881 -uroot" | ||
| if [ -n "$ROOT_PASSWORD" ]; then | ||
| MYSQL_OPTS="$MYSQL_OPTS -p$ROOT_PASSWORD" | ||
| fi | ||
|
|
||
| if [ -n "$SEEKDB_DATABASE" ]; then | ||
| mysql $MYSQL_OPTS -e "CREATE DATABASE IF NOT EXISTS \`$SEEKDB_DATABASE\`;" | ||
| echo "Database $SEEKDB_DATABASE created." | ||
| MYSQL_OPTS="$MYSQL_OPTS -D$SEEKDB_DATABASE" | ||
| fi | ||
| if [ -n "$SEEKDB_DATABASE" ]; then | ||
| mysql $MYSQL_OPTS -e "CREATE DATABASE IF NOT EXISTS \`$SEEKDB_DATABASE\`;" | ||
| echo "Database $SEEKDB_DATABASE created." | ||
| MYSQL_OPTS="$MYSQL_OPTS -D$SEEKDB_DATABASE" |
There was a problem hiding this comment.
This block builds MYSQL_OPTS by concatenating untrusted ROOT_PASSWORD and SEEKDB_DATABASE into a single string and then passes it unquoted to mysql $MYSQL_OPTS ..., so shell metacharacters in those env vars (e.g., ;, &, backticks, $(...)) can inject additional commands executed with container privileges. In addition, SEEKDB_DATABASE is interpolated directly into the CREATE DATABASE IF NOT EXISTS \"$SEEKDB_DATABASE"`;statement without escaping embedded backticks, so a crafted database name can break out of the identifier and run arbitrary SQL (for exampledb` ; DROP DATABASE mysql;--). To fix this, avoid building a single options string (pass each CLI flag as a separately quoted argument, e.g., mysql -h127.0.0.1 -P2881 -uroot ${ROOT_PASSWORD:+--password="$ROOT_PASSWORD"}) and sanitize or strictly validate SEEKDB_DATABASE before using it in SQL (for example, restrict it to [A-Za-z0-9_]+).
| MYSQL_OPTS="-h127.0.0.1 -P2881 -uroot" | ||
| if [ -n "$ROOT_PASSWORD" ]; then | ||
| MYSQL_OPTS="$MYSQL_OPTS -p$ROOT_PASSWORD" | ||
| fi | ||
|
|
||
| echo "Waiting for seekdb to be ready..." | ||
| for i in $(seq 1 $WAIT_FOR_SERVICE_READY_ATTEMPTS); do | ||
| if mysql $MYSQL_OPTS -e "show databases" >/dev/null 2>&1; then | ||
| echo "seekdb is ready." | ||
| break | ||
| fi | ||
| if [ $i -eq $WAIT_FOR_SERVICE_READY_ATTEMPTS ]; then | ||
| echo "Timeout waiting for seekdb to be ready." | ||
| exit 1 | ||
| fi | ||
| sleep $WAIT_INTERVAL_SECONDS | ||
| done | ||
|
|
||
| if [ -n "$SEEKDB_DATABASE" ]; then | ||
| MYSQL_OPTS="$MYSQL_OPTS -D$SEEKDB_DATABASE" | ||
| fi | ||
|
|
||
| echo "Executing sql: $*" | ||
| mysql $MYSQL_OPTS -e "$*" |
There was a problem hiding this comment.
In this section MYSQL_OPTS is rebuilt by concatenating untrusted ROOT_PASSWORD and SEEKDB_DATABASE into a single string and passed unquoted to mysql $MYSQL_OPTS -e ..., which allows any shell metacharacters in those env vars (such as ;, &, backticks, or $(...)) to inject additional commands executed inside the container. If an attacker can influence these environment variables (for example via a managed service that exposes them as user-configurable), they can achieve arbitrary command execution rather than just controlling SQL. Use the same mitigation as above by avoiding a single options string and instead passing each mysql flag as a separately quoted argument and/or validating ROOT_PASSWORD/SEEKDB_DATABASE to reject dangerous characters.
2 participants
Summary
Solution Description