Skip to content

Conversation

@zhangzqs
Copy link
Collaborator

@zhangzqs zhangzqs commented Nov 4, 2025

No description provided.

包括 data.tf、main.tf、mysql_master.sh、mysql_slave.sh、outputs.tf、variables.tf、versions.tf 等文件
@gemini-code-assist
Copy link

Summary of Changes

Hello @zhangzqs, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

此拉取请求引入了用于在 Qiniu 云平台上部署 MySQL 数据库的 Terraform 配置。它提供了两种部署模式:一种是高可用的主从复制集群,另一种是独立的单节点实例。这些配置旨在简化 MySQL 基础设施的自动化部署和管理,通过可配置的变量和自动化脚本,用户可以快速地创建和配置满足其特定需求的 MySQL 环境。

Highlights

  • MySQL 主从复制集群: 新增 Terraform 配置,用于在 Qiniu 云上部署一个 MySQL 主从复制集群,包括一个主节点和多个从节点。
  • MySQL 单节点实例: 新增 Terraform 配置,用于在 Qiniu 云上部署一个独立的 MySQL 单节点实例。
  • 自动化配置脚本: 提供了 'mysql_master.sh', 'mysql_slave.sh', 和 'mysql_standalone.sh' 脚本,用于自动化 MySQL 服务器的安装、基本配置、用户创建以及主从复制设置。
  • 参数化和验证: 所有配置都通过 Terraform 变量进行参数化,并包含严格的输入验证,确保部署的灵活性和安全性,特别是对 MySQL 用户名和密码的验证。
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

这个 PR 增加了用于创建 MySQL 单节点实例和主从复制集群的 Terraform 模块,代码结构清晰。审查中发现了一些需要解决的问题,包括安全漏洞(例如,为复制用户使用管理员密码、在 shell 脚本中暴露密码、开放的绑定地址)、正确性错误(脚本语法错误、变量名不正确、脆弱的等待逻辑)以及可维护性方面的改进(使配置更健壮和灵活)。请查看具体的审查意见。

echo "MySQL not found, installing..."
apt-get update
DEBIAN_FRONTEND=noninteractive apt-get install -y mysql-client-8.0 mysql-server-8.0 mysql-router mysql-shell
fi``

Choose a reason for hiding this comment

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

critical

这里有一个语法错误。fi``` 应该是 fi`。多余的反引号会导致脚本执行失败。

Suggested change
fi``
fi

default = ""

validation {
condition = var.mysql_db_name == null ? true : (

Choose a reason for hiding this comment

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

high

验证条件 var.mysql_db_name == null 是不正确的。因为该变量的 default""(空字符串),而不是 null。当用户不提供这个值时,var.mysql_db_name 的值是 ""== null 的判断会失败,导致验证逻辑出错。应该使用 var.mysql_db_name == ""

    condition = var.mysql_db_name == "" ? true : (

mysql_master_ip = qiniu_compute_instance.mysql_primary_node.private_ip_addresses[0].ipv4
mysql_server_id = tostring(count.index + 2) // 从库ID从2开始递增
mysql_replication_username = local.replication_username
mysql_replication_password = var.mysql_password

Choose a reason for hiding this comment

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

high

将管理员密码 var.mysql_password 同时用作复制用户的密码存在严重安全风险。如果复制用户的密码泄露,攻击者可能会尝试用它来登录管理账户。建议为复制用户创建一个独立的密码。您可以通过一个新的变量(例如 var.mysql_replication_password)来传递,或者使用 random_password 资源生成一个随机密码。

    mysql_replication_password = var.mysql_replication_password


# 重启 MySQL 服务
systemctl restart mysql
sleep 1 # 等待 MySQL 服务重启完成

Choose a reason for hiding this comment

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

medium

使用 sleep 1 来等待 MySQL 重启完成是不可靠的。在负载较高或启动较慢的机器上,1秒可能不足以让 MySQL 完全准备好,导致后续命令失败。建议使用一个循环来检查 MySQL 服务的状态,例如通过 mysqladmin ping,直到服务可用为止。

Suggested change
sleep 1 # 等待 MySQL 服务重启完成
while ! mysqladmin ping --silent; do sleep 1; done

Comment on lines 10 to 18
variable "instance_system_disk_size" {
type = number
description = "System disk size in GiB"

validation {
condition = var.instance_system_disk_size > 0
error_message = "instance_system_disk_size parameter must be a positive integer"
}
}

Choose a reason for hiding this comment

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

medium

变量 instance_system_disk_size 没有设置默认值,而 replication 模块中的同名变量设置了 default = 20。为了保持一致性并提高模块易用性,建议在此处也添加一个默认值。

variable "instance_system_disk_size" {
  type        = number
  description = "System disk size in GiB"
  default     = 20

  validation {
    condition     = var.instance_system_disk_size > 0
    error_message = "instance_system_disk_size parameter must be a positive integer"
  }
}

@zhangzqs
Copy link
Collaborator Author

/review

@zhangzqs
Copy link
Collaborator Author

/code review下

@xgopilot
Copy link

xgopilot bot commented Nov 11, 2025

🔍 Code Review Results

After analyzing the MySQL Terraform module code for both standalone and replication configurations, I've identified several issues that need attention:

🔒 Security Issues

mysql/standalone/mysql_standalone.sh:30-34 & mysql/replication/mysql_master.sh:38-48

mysql -uroot <<EOF
ALTER USER 'root'@'localhost' IDENTIFIED BY '${mysql_password}';
CREATE USER '${mysql_username}'@'%' IDENTIFIED BY '${mysql_password}';
GRANT ALL PRIVILEGES ON *.* TO '${mysql_username}'@'%' WITH GRANT OPTION;
FLUSH PRIVILEGES;
EOF

Critical Security Flaw: The root user password change and user creation is done without authentication, which only works because MySQL 8.0 initially allows passwordless root access. However, this creates a security window and is not a reliable approach. The script should use the mysql_secure_installation approach or configure authentication properly.

mysql/standalone/mysql_standalone.sh:31 & mysql/replication/mysql_master.sh:40-41

CREATE USER '${mysql_username}'@'%' IDENTIFIED BY '${mysql_password}';
GRANT ALL PRIVILEGES ON *.* TO '${mysql_username}'@'%' WITH GRANT OPTION;

Security Risk: Creating users with '%' wildcard allows connections from any IP address. This grants unrestricted network access to the MySQL instance, which is a significant security risk. Consider restricting to specific IP ranges or VPC subnets.

mysql/replication/mysql_slave.sh:44-52

mysql -uroot <<EOF
  CHANGE MASTER TO
    MASTER_HOST = '${mysql_master_ip}',
    MASTER_USER = '${mysql_replication_username}',
    MASTER_PASSWORD = '${mysql_replication_password}',
    MASTER_AUTO_POSITION = 1;
  START SLAVE;
  SHOW SLAVE STATUS\G;
EOF

Authentication Issue: The slave setup uses passwordless root access, which may fail if the master node has already set a root password and replicated it to the slave.

🐛 Bug Fixes Needed

mysql/replication/mysql_slave.sh:50

START SLAVE;

Deprecated Command: START SLAVE is deprecated in MySQL 8.0. Should use START REPLICA instead.

mysql/replication/mysql_slave.sh:51

SHOW SLAVE STATUS\G;

Deprecated Command: SHOW SLAVE STATUS is deprecated. Should use SHOW REPLICA STATUS instead.

⚠️ Reliability Issues

mysql/standalone/mysql_standalone.sh:26 & mysql/replication/mysql_master.sh:36

while ! mysqladmin ping --silent; do sleep 1; done

Missing Timeout: The while loop should have a timeout to prevent infinite waiting if MySQL fails to start properly.

mysql/replication/mysql_slave.sh:38-41

while ! mysqladmin ping -h "${mysql_master_ip}" -u"${mysql_replication_username}" -p"${mysql_replication_password}" --silent; do
  echo "Waiting for MySQL master at ${mysql_master_ip} to be ready..."
  sleep 2
done

Missing Timeout: This loop also needs a timeout mechanism and should handle authentication failures properly.

📝 Configuration Issues

mysql/replication/variables.tf:85-87

validation {
  condition     = var.mysql_replica_count >= 1 && var.mysql_replica_count <= 2
  error_message = "mysql_replica_count must be between 1 and 2"
}

Arbitrary Limitation: The restriction to maximum 2 replicas seems arbitrary and may be too limiting for production use cases.

mysql/replication/mysql_master.sh:27 & mysql/replication/mysql_slave.sh:28

expire_logs_days = 7 # Master only
# Missing log retention policy on slaves

Inconsistent Configuration: The master sets log retention but slaves don't have consistent log cleanup policies.

💡 Best Practice Improvements

mysql/standalone/data.tf:7-11 & mysql/replication/data.tf:8-12

ubuntu_image_id = one([
  for item in data.qiniu_compute_images.available_official_images.items : item
  if item.os_distribution == "Ubuntu" && item.os_version == "24.04 LTS"
]).id

Error Handling: The one() function will fail if no Ubuntu 24.04 LTS image is found. Should add error handling or fallback logic.

Shell Scripts - Missing Error Context
All shell scripts use set -e but don't provide meaningful error messages when commands fail. Consider adding error handling functions.

🎯 Positive Aspects

  • Good use of variable validation with regex patterns for password complexity
  • Proper use of sensitive = true for password variables
  • Appropriate use of placement groups for anti-affinity in replication setup
  • GTID-based replication configuration is modern and correct
  • Proper binlog configuration for replication

📋 Recommendations Summary

  1. High Priority: Fix authentication security issues and deprecated MySQL commands
  2. Medium Priority: Add proper timeout mechanisms and error handling
  3. Low Priority: Consider relaxing replica count limitations and improving image selection logic

👉 Want me to re-run this review? Just type /review in the comments! For more usage examples, visit CodeAgent GitHub Repository.

@xgopilot
Copy link

xgopilot bot commented Nov 11, 2025

🔍 Code Review Summary

This PR introduces well-structured MySQL Terraform modules with good variable validation and resource organization. However, several critical security vulnerabilities and important code quality issues need immediate attention before production deployment.

🚨 Critical Security Issues

  1. MySQL exposed to internet without network security controls
  2. Overprivileged database users with ALL PRIVILEGES
  3. Cleartext password transmission in shell commands
  4. Missing Terraform version constraints causing potential compatibility issues

📋 Key Improvements Needed

  • Add network security groups/firewall rules
  • Implement least privilege database access
  • Extract duplicate code between standalone/replication modules
  • Add comprehensive module documentation (README.md)

The modules show solid Terraform knowledge but require security hardening for production use.

@zhangzqs zhangzqs merged commit 92430bb into qiniu:main Nov 12, 2025
@zhangzqs zhangzqs deleted the add-mysql branch November 12, 2025 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants