feat: enhance MySQL support and improve script logging

[filippolmt]

PR Type

Enhancement, Bug fix

---

Description

• Add permissions_refresh_id variable to force script re-execution without user recreation

• Refactor privilege grant logic into separate null_resource.grant_permissions

• Enhance MySQL 8.x support: remove cloudsqlsuperuser role and clear default roles

• Improve shell scripts with better logging, error handling, and Cloud SQL Auth Proxy v2 syntax

---

Diagram Walkthrough

flowchart LR
  A["permissions_refresh_id variable"] --> B["null_resource.force_permissions_refresh"]
  B --> C["null_resource.execute_cloud_sql_proxy"]
  B --> D["null_resource.grant_permissions"]
  B --> E["null_resource.kill_cloud_sql_proxy"]
  D --> F["execute_sql.sh: revoke cloudsqlsuperuser"]
  D --> G["execute_sql.sh: grant database privileges"]

Loading

File Walkthrough

	Relevant files
Enhancement	5 files variables.tf Add permissions_refresh_id variable with validation            +13/-2    main.tf Refactor privilege grants and add refresh trigger                +41/-3    execute_sql.sh Enhance MySQL 8.x privilege management and logging              +58/-10  execute_cloud_sql_proxy.sh Improve proxy script with v2 syntax and logging                    +31/-12  kill_cloud_sql_proxy.sh Add logging and safer proxy shutdown handling                        +18/-7
Documentation	5 files main.tf Add permissions_refresh_id parameter to module usage          +7/-5      variables.tf Add permissions_refresh_id variable definition                      +6/-0      test.tfvars Add example permissions_refresh_id value                                  +3/-0      README.md Update documentation for MySQL 8.4 and refresh mechanism  +14/-7    CHANGELOG.md Document version 0.5.0 changes                                                      +15/-0
Formatting	1 files versions.tf Remove trailing whitespace                                                              +0/-1

---

[sparkfabrik-ai-bot]

PR Reviewer Guide 🔍

(Review updated until commit 24c34e0)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 No relevant tests

🔒 Security concerns Sensitive information exposure: The CLOUDSQL_PRIVILEGED_USER_PASSWORD is passed as an environment variable to shell scripts and used directly in command-line arguments (line 15-19 in execute_sql.sh with MYSQL_PWD). While environment variables are safer than command-line arguments, the password could still be exposed in process listings or logs. The scripts use set -eu but don't explicitly prevent command echoing that might log sensitive data. Consider adding explicit measures to prevent password leakage in logs, such as disabling command tracing for sensitive operations or using MySQL configuration files instead of environment variables.

⚡ Recommended focus areas for review Error Handling Gap The script attempts to revoke cloudsqlsuperuser role and checks for specific error messages using grep -qi, but the error handling may not cover all failure scenarios. If the revoke operation fails for reasons other than "Operation REVOKE ROLE failed", the script continues execution. Consider whether all MySQL 8.x error cases are properly handled, especially for edge cases like connection failures during the revoke operation. if ! REVOKE_OUTPUT=$(mysql_exec --execute="REVOKE cloudsqlsuperuser FROM ${USER_IDENTIFIER};" 2>&1); then if printf '%s' "${REVOKE_OUTPUT}" | grep -qi "Operation REVOKE ROLE failed"; then log "cloudsqlsuperuser role already absent for ${USER_IDENTIFIER}; continuing." else log "ERROR: Failed to revoke cloudsqlsuperuser role for ${USER_IDENTIFIER}:\n${REVOKE_OUTPUT}" >&2 exit 1 fi else log "Removed cloudsqlsuperuser role from ${USER_IDENTIFIER}." fi Resource Dependencies The null_resource.grant_permissions resource has been separated from google_sql_user.sql_user, but the dependency chain should be verified. The grant operation depends on the user existing, but if the user creation fails or is still in progress, the grant script may execute prematurely. Validate that the explicit depends_on relationships ensure proper sequencing in all scenarios. resource "null_resource" "grant_permissions" { for_each = { for u in var.database_and_user_list : u.user => u } triggers = { user = each.key user_host = each.value.user_host database = each.value.database } lifecycle { replace_triggered_by = [ null_resource.force_permissions_refresh.id ] } provisioner "local-exec" { command = "${path.module}/scripts/execute_sql.sh" environment = { CLOUDSDK_CORE_PROJECT = var.project_id GCLOUD_PROJECT_REGION = var.region CLOUDSQL_INSTANCE_NAME = var.cloudsql_instance_name CLOUDSQL_PROXY_HOST = var.cloudsql_proxy_host CLOUDSQL_PROXY_PORT = var.cloudsql_proxy_port CLOUDSQL_PRIVILEGED_USER_NAME = var.cloudsql_privileged_user_name CLOUDSQL_PRIVILEGED_USER_PASSWORD = var.cloudsql_privileged_user_password MYSQL_VERSION = data.google_sql_database_instance.cloudsql_instance.database_version USER = each.value.user USER_HOST = each.value.user_host DATABASE = each.value.database } interpreter = [ "/bin/sh", "-c" ] } depends_on = [ google_sql_database.sql_database, google_sql_user.sql_user, null_resource.execute_cloud_sql_proxy ] } Race Condition Risk The script checks if cloud_sql_proxy is already running using pgrep, but there's a potential race condition between the check and starting a new instance. If multiple Terraform resources trigger this script simultaneously, multiple proxy instances could be started on the same port, causing conflicts. Consider adding port-specific checks or file-based locking mechanisms. if ! pgrep -x "$CLOUDSQL_PROXY_BIN" >/dev/null; then log "Starting Cloud SQL Auth Proxy (${CLOUDSQL_PROXY_BIN}) for ${CONNECTION_NAME} on localhost:${CLOUDSQL_PROXY_PORT}." "${CLOUDSQL_PROXY_BIN}" "${CONNECTION_NAME}" --port "${CLOUDSQL_PROXY_PORT}" >/dev/null 2>&1 & sleep 1s else log "Cloud SQL Auth Proxy already running; skipping start." fi

[sparkfabrik-ai-bot]

PR Code Suggestions ✨

Latest suggestions up to 24c34e0
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Improve error pattern matching The error detection logic may miss other valid scenarios where the role doesn't exist. Instead of checking for a specific error message substring, verify if the role exists before attempting revocation or handle the MySQL error code directly to avoid false failures. scripts/execute_sql.sh [52-61] if ! REVOKE_OUTPUT=$(mysql_exec --execute="REVOKE cloudsqlsuperuser FROM ${USER_IDENTIFIER};" 2>&1); then - if printf '%s' "${REVOKE_OUTPUT}" | grep -qi "Operation REVOKE ROLE failed"; then + if printf '%s' "${REVOKE_OUTPUT}" | grep -qiE "(Operation REVOKE ROLE failed|role.*not exist|Unknown authorization)"; then log "cloudsqlsuperuser role already absent for ${USER_IDENTIFIER}; continuing." else log "ERROR: Failed to revoke cloudsqlsuperuser role for ${USER_IDENTIFIER}:\n${REVOKE_OUTPUT}" >&2 exit 1 fi else log "Removed cloudsqlsuperuser role from ${USER_IDENTIFIER}." fi Suggestion importance[1-10]: 7 __ Why: The suggestion improves error handling by using a more comprehensive regex pattern to detect various error messages indicating the role doesn't exist. This makes the script more robust across different MySQL error message formats, though the current implementation already handles the primary case correctly.	Medium
	Verify proxy process started successfully Starting the proxy in the background without capturing its PID can lead to orphaned processes if the script fails. Capture the background process PID and verify it started successfully before proceeding to avoid resource leaks. scripts/execute_cloud_sql_proxy.sh [29-35] if ! pgrep -x "$CLOUDSQL_PROXY_BIN" >/dev/null; then log "Starting Cloud SQL Auth Proxy (${CLOUDSQL_PROXY_BIN}) for ${CONNECTION_NAME} on localhost:${CLOUDSQL_PROXY_PORT}." "${CLOUDSQL_PROXY_BIN}" "${CONNECTION_NAME}" --port "${CLOUDSQL_PROXY_PORT}" >/dev/null 2>&1 & + PROXY_PID=$! sleep 1s + if ! kill -0 "$PROXY_PID" 2>/dev/null; then + log "ERROR: Cloud SQL Auth Proxy failed to start." >&2 + exit 1 + fi else log "Cloud SQL Auth Proxy already running; skipping start." fi Suggestion importance[1-10]: 6 __ Why: Adding PID capture and verification improves reliability by detecting if the proxy failed to start immediately. However, the existing code already has a connection check loop (lines 37-45) that will catch startup failures, making this a defensive improvement rather than fixing a critical issue.	Low
	Capture PID before sleep delay The PID retrieval happens after a 5-second sleep, creating a race condition where the proxy could have already terminated or new instances started. Capture the PID immediately when first detected to ensure you're terminating the correct process. scripts/kill_cloud_sql_proxy.sh [14-23] CLOUDSQL_PROXY_BIN="cloud_sql_proxy" if pgrep -x "$CLOUDSQL_PROXY_BIN" >/dev/null; then - log "Detected running ${CLOUDSQL_PROXY_BIN}; waiting 5 seconds before shutdown to avoid race conditions." + PID="$(pgrep -x "$CLOUDSQL_PROXY_BIN")" + log "Detected running ${CLOUDSQL_PROXY_BIN} (PID(s): ${PID}); waiting 5 seconds before shutdown to avoid race conditions." sleep 5s - # Obtain the PID of the running Cloud SQL Auth Proxy and terminate gently. - PID="$(pgrep -x "$CLOUDSQL_PROXY_BIN")" log "Stopping ${CLOUDSQL_PROXY_BIN} (PID(s): ${PID})." - kill "${PID}" || true fi Suggestion importance[1-10]: 5 __ Why: While capturing the PID earlier reduces a theoretical race condition window, the practical impact is minimal since the 5-second sleep is intentionally added to avoid race conditions with other operations. The || true at the end already handles cases where the process no longer exists.	Low

---

Previous suggestions

Suggestions up to commit f514d46

Category	Suggestion	Impact
Possible issue	Fix proxy command argument parsing The proxy command arguments are incorrectly passed as a single string. The connection name and port flag should be separate arguments. This will cause the proxy to fail to start properly. scripts/execute_cloud_sql_proxy.sh [31] -"${PROXY_BIN}" "${CONNECTION_NAME} -p ${CLOUDSQL_PROXY_PORT}" >/dev/null 2>&1 & +"${PROXY_BIN}" "${CONNECTION_NAME}" --port "${CLOUDSQL_PROXY_PORT}" >/dev/null 2>&1 & Suggestion importance[1-10]: 9 __ Why: The current implementation passes the connection name and port flag as a single quoted string, which will cause the Cloud SQL Auth Proxy to fail. The arguments must be separate for proper parsing.	High
Security	Avoid password exposure in process list Passing the password via command-line argument exposes it in process listings and shell history. Use MYSQL_PWD environment variable or a credentials file to avoid security risks. scripts/execute_sql.sh [54] -if ! mysql --host="${CLOUDSQL_PROXY_HOST}" --port="${CLOUDSQL_PROXY_PORT}" --user="${CLOUDSQL_PRIVILEGED_USER_NAME}" --password="${CLOUDSQL_PRIVILEGED_USER_PASSWORD}" --execute="${SQL_COMMANDS}"; then +if ! MYSQL_PWD="${CLOUDSQL_PRIVILEGED_USER_PASSWORD}" mysql --host="${CLOUDSQL_PROXY_HOST}" --port="${CLOUDSQL_PROXY_PORT}" --user="${CLOUDSQL_PRIVILEGED_USER_NAME}" --execute="${SQL_COMMANDS}"; then Suggestion importance[1-10]: 8 __ Why: Using --password on the command line exposes the password in process listings and shell history. Using the MYSQL_PWD environment variable is a more secure approach that prevents this exposure.	Medium

[Copilot]

Pull Request Overview

This PR adds support for MySQL 8.4 and improves the shell scripts handling Cloud SQL Auth Proxy operations with better error handling, logging, and robustness.

• Added MySQL 8.4 support alongside existing 5.7 and 8.0 versions
• Enhanced shell scripts with proper error handling (set -eu, pipefail), structured logging, and improved messaging
• Fixed Cloud SQL Auth Proxy command syntax and improved connection status checks

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
versions.tf	Removed trailing whitespace
variables.tf	Updated description to include MySQL 8.4 support
scripts/kill_cloud_sql_proxy.sh	Added error handling, logging function, and improved messaging
scripts/execute_sql.sh	Added MySQL 8.4 case, structured logging, error handling, and improved SQL command formatting
scripts/execute_cloud_sql_proxy.sh	Refactored with error handling, logging, and fixed proxy command syntax
README.md	Updated documentation to reflect MySQL 8.4 support and added clarification about MySQL 8.x role handling

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[filippolmt]

/improve

[filippolmt]

/describe

[Copilot]

Pull Request Overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[filippolmt]

/improve

[filippolmt]

/describe

[Copilot]

Pull Request Overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull Request Overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[filippolmt]

/describe

[sparkfabrik-ai-bot]

Persistent review updated to latest commit 24c34e0

[Stevesibilia]

LGTM !