Feature/infra changes - #246
Conversation
Gallery overhaul and media tagging system
chore: update workspace membership check in brand guideline service
docs: add comprehensive development guide
Implement admin dashboard with backend services and UI components, fix reference images on media detail page
…cc-creative-studio into feature/infra-changes
…Izumi Agent deployment into bootstrap script
…-studio into feature/infra-changes
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This Pull Request introduces significant, high-quality infrastructure changes, transitioning Creative Studio to a secure private network with a Private PostgreSQL instance, Cloud Armor WAF protection, and an automated Izumi agent deployment pipeline. The overall organization of Terraform modules and scripting is excellent and aligns with enterprise-grade deployment practices. However, several critical issues must be resolved before merging, including invalid probe endpoints, a non-existent database version, a broken SQL CLI command in the migration script, and a missing utility installation in the release workflow.
🔍 General Feedback
- Excellent Security Design: Locking the database to a private VPC and enforcing Direct VPC egress for Cloud Run is a strong best practice.
- Great Deployment Scripting: The addition of
deploy.shmakes the bootstrapping and configuration process highly intuitive and seamless. - Dynamic Credentials: Great use of Secret Manager and ephemeral secrets in Terraform to keep sensitive passwords out of the state file.
- Documentation Note: Clean up legacy file references in comment sections (such as
platform.tfreferences) to keep the repository maintainable.
…te docker-compose configuration
There was a problem hiding this comment.
This pull request introduces significant infrastructure enhancements to Google Cloud Creative Studio, including modularizing the Terraform setup under a new infrastructure directory, adding Private VPC and Private Services Access configurations, and incorporating automated data migration and Izumi Agent deployment steps. The architectural enhancements follow enterprise security best practices by isolating database traffic and utilizing modern Terraform features.
🔍 General Feedback
- Directory Reorganization: Moving/renaming
infratoinfrastructureis a positive step towards clean modularity, but ensure that all shell scripts referencing these directories have been updated completely to avoid broken paths. - Modern Terraform Features: The adoption of Terraform 1.11+ write-only attributes (
password_wo) and ephemeral blocks is a great security choice that prevents plaintext password exposure in State files. - Enterprise Network Isolation: Transitioning Cloud Run to Direct VPC Egress and Cloud SQL to Private IP only significantly improves the security profile of the application.
…nfrastructure paths and configuration scripts
…-studio into feature/infra-changes
There was a problem hiding this comment.
This pull request introduces significant infrastructure enhancements, renaming the Terraform workspace from infra to infrastructure, modernizing the GCP resources with private network isolation (Cloud SQL Private IP and Direct VPC Egress for Cloud Run), and adding automated deployment pipelines. While the architectural improvements are excellent and security-focused, several critical regression blockers and syntax mismatches must be addressed before merging.
🔍 General Feedback
- Pre-commit Build Regression (Critical): In
infrastructure/pre-commit/Dockerfile.pre-commit(which was renamed with 100% similarity), the file still attempts to copy the entrypoint script using the old pathCOPY infra/pre-commit/entrypoint.sh /entrypoint.sh. Since that folder has been renamed, building the pre-commit environment will fail with a file-not-found error. Please update this to use the new path. - Bootstrapping Mismatches: With the flattening of the Terraform workspace structure directly under
infrastructure/, thebootstrap.shscript continues to reference deleted environments templates and subdirectories. This completely breaks the setup workflow for new environments. - Provider & Version Sanity: The database version (
POSTGRES_18) is non-existent in GCP Cloud SQL, and the CPU capacity format (1000m) is invalid for Cloud Run v2 (which rejects Kubernetes millicore syntax). Correcting these defaults prevents runtime provisioning failures.
…e connections, and update vertex AI agent configuration
There was a problem hiding this comment.
📋 Review Summary
This pull request transitions the Google Cloud Creative Studio deployment pattern to use a secure, private VPC network with Private Services Access for PostgreSQL database connections. This transition significantly enhances database resilience and aligns with enterprise-level Google Cloud architectural best practices. The updated infrastructure modules, backend database helper updates, and deployment scripts are exceptionally cohesive, robust, and well-designed.
🔍 General Feedback
- State-of-the-Art Security: The integration of Terraform v1.11+ write-only attributes (
password_woingoogle_sql_userandsecret_data_woingoogle_secret_manager_secret_version) paired with ephemeral resources is an outstanding, highly secure practice that ensures sensitive plaintext passwords never touch the persistent.tfstatefiles. - VPC Boundary Security: Moving database connectivity to a private network using serverless NEGs, Direct VPC egress on Cloud Run, and Cloud Armor with OWASP rules is highly professional and enforces correct network isolation.
- Excellent Migration Tooling: Providing automated schema migration checks and data transfer scripts (
migrate_to_private_db.sh) prevents data loss when upgrading existing deployments. - Proper Test Coverage: Unit and integration tests are properly updated to mock the new
IPTypes.PRIVATEconnection configurations and Vertex AI reasoning engine initialization flows.
There was a problem hiding this comment.
This pull request introduces significant architectural upgrades to transition the Google Cloud Creative Studio database to a Private VPC connection and reorganize the Terraform infrastructure structure into modular components. While the overall goal and direction are excellent for enterprise security and maintainability, there are several critical configuration mismatches and pathing issues that would prevent successful deployment or execution.
🔍 General Feedback
- Terraform Provider Mismatch (
🔴Critical): Theinfrastructure/main.tfdefines provider constraints of~> 6.50.0, but employs new Google provider features (password_woandsecret_data_wo) that require7.0.0or later, which will cause deployment validation failures. - Duplicate Environment Variables (
🔴Critical): Ininfrastructure/app.tf,AGENT_ENGINE_RESOURCE_NAMEandAGENT_ENGINE_USER_AUTH_TOKEN_KEYare defined twice (both as plaintext env vars and as Secret Manager secrets), leading to a 400 Bad Request error from the Cloud Run v2 API. - Incorrect Pathing in Rename (
🔴Critical): The pre-commit Dockerfile (infrastructure/pre-commit/Dockerfile.pre-commitline 23) still attempts to copyinfra/pre-commit/entrypoint.shfrom the old path instead of the renamedinfrastructure/pre-commit/entrypoint.shpath, which will fail during docker image builds. - AI Agent Service Account Permissions (
🟠High): The dedicated service account for AI Agents (agent_sa) is provisioned but lacks project-level IAM roles (such asroles/aiplatform.user), which will cause Vertex AI Gemini model invocations by the reasoning engine to fail with403 Permission Deniederrors. - Sensitive Data Lifecycle (
🟠High): The database migration scriptmigrate_to_private_db.shleaves the decrypted SQL database dump file in the GCS bucket indefinitely after successful import, presenting a major security risk for sensitive database content.
…gration cleanup, and remove unused agent configuration
There was a problem hiding this comment.
📋 Review Summary
This Pull Request introduces significant infrastructure updates to establish private, secure database connections for the Google Cloud Creative Studio Platform, migrating from public Cloud SQL access to Private Services Access (VPC Peering) and Serverless Direct VPC Egress for Cloud Run. While the architectural transition is highly positive for enterprise security, there are several critical deployment and integration bugs—specifically related to missing GCP service API enablements and randomized authentication variables—that must be resolved before this PR can be safely merged.
🔍 General Feedback
- Positive Highlight on Data Migration: The database migration script (
infrastructure/migration/migrate_to_private_db.sh) is exceptionally well designed. It employs robust retry loops, manages IAM policy bindings dynamically for export/import, and utilizes shell traps to guarantee clean-up of temporary Cloud Storage assets and permissions. - Improved Git Hygiene: Properly ignoring
public/assets/config.jsonwhile introducing the.exampletemplate is an excellent standard practice that prevents developers from accidentally committing local config profiles. - Consistent Testing: Solid updates to the unit tests in
backend/tests/test_database.pyandbackend/tests/agents/test_agent_service.pyensure that private IP routing and agent initialization are continuously verified going forward.
…ment configuration in bootstrap.sh
There was a problem hiding this comment.
The Pull Request implements crucial architectural improvements by modernizing and flattening the infrastructure setup, introducing Private Services Access for PostgreSQL database connections, and updating the database lifecycle cleanup. The separation of environment-specific states using a GCS backend config pattern and the use of ephemeral resources in Terraform represent outstanding Cloud architecture practices. However, critical logic errors in bootstrap.sh (such as unassigned variables and an empty .tfvars file creation) currently block successful execution and need to be resolved before merging.
🔍 General Feedback
- Great job on transitioning the database connection to Private Services Access for enhanced VPC security. This aligns perfectly with enterprise-grade GCP landing zone patterns.
- Excellent usage of modern Terraform features (like
ephemeralresources and write-only properties) to ensure database credentials never leak into state files. - Excellent addition of the database connector cleanup process in the FastAPI lifespan shutdown handler to prevent resource leaks during container recycling.
- Please address the critical bugs identified in the
bootstrap.shscript to ensure developers can successfully deploy the environment end-to-end.
…aving for project and repository configurations
…e support and GitHub Actions testing workflow
There was a problem hiding this comment.
This pull request introduces critical infrastructure and deployment enhancements, transitioning to secure private Cloud SQL connectivity via Direct VPC egress, and a robust offline test suite for the bootstrapper script. The overall architectural design is exceptional, utilizing dynamic secret mapping, VPC peering, and secure parameter state persistence to achieve production-grade isolation. However, several critical version constraints and invalid argument mismatches in the Terraform modules will cause execution to fail immediately on initialization and must be corrected before merge.
🔍 General Feedback
- Dynamic Orchestration: The enhancements to
bootstrap.shto support automated, idempotent, profile-driven executions are extremely well implemented and handle resuming from late-stage steps cleanly. - VPC & Network Isolation: Transitioning the application components (FastAPI Backend, Cloud SQL, and migrations) to a private IP model represents a significant security improvement.
- Code Quality & Verification: The addition of
test_bootstrap_offline.shis an excellent initiative to ensure the complex initialization logic is completely testable and verified offline. - Provider Consistency: Ensure that provider version requirements set in root and child files remain fully synchronized with the
.terraform.lock.hcland environment constraints.
…ets, and refactor bootstrap script for improved interactivity
… bash utility and centralized Terraform variables
… bootstrap scripts for repository configuration
…ip auto-detection for agent engine resource name
…mplify Firebase configuration discovery logic
There was a problem hiding this comment.
This Pull Request successfully transitions the Creative Studio platform to a secure, private infrastructure footprint on Google Cloud. It shifts the Cloud SQL connectivity from public endpoints to a Private Service Access peering setup and introduces robust multi-profile state management for bootstrapping. The changes in both the backend application code and the deployment scripts are exceptionally clean, well-tested, and of high professional quality.
🔍 General Feedback
- Excellent modularization of the Terraform infrastructure files by transitioning from the legacy
infra/directory to a clean, flattenedinfrastructure/layout with dedicated modules. - Strong introduction of automated offline unit testing in
test_bootstrap_offline.sh, validating setup robustness against multiple configurations and non-interactive profiles without requiring actual cloud resources. - High consistency and alignment with security and cloud-native best practices across the application, deployment automation, and CI/CD pipelines.
| vertexai.init( | ||
| project=config_service.PROJECT_ID, | ||
| location=config_service.WORKFLOWS_LOCATION, | ||
| location=config_service.AGENT_LOCATION, |
There was a problem hiding this comment.
| password=config_service.DB_PASS, | ||
| db=config_service.DB_NAME, | ||
| ip_type=IPTypes.PUBLIC, # Adjust if using Private IP | ||
| ip_type=IPTypes.PRIVATE, # Default to Private IP for secure VPC connection |
There was a problem hiding this comment.
With the migration to IPTypes.PRIVATE as the default for secure VPC connections, the singleton DatabaseConnector is more heavily relied upon.
To prevent ConnectorLoopError in multi-loop environments (e.g., during parallel unit tests or background celery/worker tasks), the singleton's get_connector should track the active event loop and re-initialize the connector if the loop changes.
Consider updating the DatabaseConnector class (around lines 75-100) to:
class DatabaseConnector:
_instance = None
_connector: Connector | None = None
_loop: asyncio.AbstractEventLoop | None = None
@classmethod
def get_instance(cls):
if cls._instance is None:
cls._instance = cls()
return cls._instance
def get_connector(self) -> Connector:
current_loop = asyncio.get_running_loop()
if self._connector is None or self._loop != current_loop:
self._connector = Connector(loop=current_loop)
self._loop = current_loop
return self._connector| try: | ||
| from src.database import cleanup_connector | ||
|
|
||
| await cleanup_connector() |
There was a problem hiding this comment.
While cleaning up the Google Cloud SQL connector is crucial, it is also highly recommended to explicitly dispose of the global SQLAlchemy engine during the application lifespan shutdown. This ensures that any remaining pooled database connections are gracefully closed and database resources are freed immediately.
You can update cleanup_connector in backend/src/database.py to also dispose of the engine:
async def cleanup_connector():
"""Closes the Connector and disposes the engine to release resources."""
if "engine" in globals() and engine is not None:
await engine.dispose()
await DatabaseConnector.get_instance().cleanup()| if [ -n "$AUTO_FIREBASE_SITE_ID" ] && [ "$AUTO_FIREBASE_SITE_ID" != "null" ] && [ "$AUTO_FIREBASE_SITE_ID" != "unassigned" ]; then | ||
| info "Using confirmed Firebase Hosting Site ID from profile: ${C_YELLOW}${AUTO_FIREBASE_SITE_ID}${C_RESET}" | ||
| else | ||
| local default_site_name=$( (firebase hosting:sites:list --project "$GCP_PROJECT_ID" --json 2>/dev/null || echo "{}") | jq -r 'try ((.result.sites // []) | (map(select(.type == "DEFAULT_SITE"))[0].name // .[0].name // "")) catch ""' 2>/dev/null || echo "" ) |
There was a problem hiding this comment.
When creating a new Firebase web app via firebase apps:create WEB, propagation through the Firebase API can sometimes take a few seconds. Currently, the script immediately attempts to fetch the APP_ID (line 447) and SDK_CONFIG_JSON (line 448). If the app has not fully propagated, these calls can return empty, causing the script to fail with:
Could not automatically fetch Firebase API Key. Please check your Firebase setup. on line 458.
Recommendation: Consider introducing a brief retry loop (e.g., up to 3 attempts with a 2-second sleep) when resolving APP_ID to make the script significantly more robust against timing/API latency issues.
There was a problem hiding this comment.
This pull request introduces critical modernizations to the infrastructure and deployment automation of Creative Studio. The updates include transitioning Cloud SQL to a secure Private VPC, implementing highly professional offline bootstrap tests, and streamlining the database seeding process via containerized Cloud Run Jobs. The overall code quality, test coverage, and documentation improvements are excellent and represent a major architectural step forward.
🔍 General Feedback
- Exceptional Testing Rigor: The new automated offline unit test suite (
test_bootstrap_offline.sh) is exceptionally comprehensive, incorporating robust mock cli implementations and cleanly verifying multiple deployment edge cases. - Modern Terraform Features: The usage of Terraform 1.10+ ephemeral resources and write-only properties is highly professional and ensures database credentials are never stored in plaintext state files.
- Secure Network Architecture: Migrating the PostgreSQL database to a Private VPC and utilizing Direct VPC Egress for Cloud Run represents a solid enterprise security enhancement.
- Clean DB Seeding: Implementing serverless Cloud Run Jobs inside the VPC boundary for database migrations and seeding cleanly resolves security and access boundary issues.
- Minor Docker and CI/CD Cleanup: Please review the inline suggestions regarding regional parameter overrides in the backend trigger, missing frontend compilation parameters, and uppercase Docker tag requirements in the GitHub release workflow.
| filename = "frontend/cloudbuild-deploy.yaml" | ||
| substitutions = { | ||
| _GCP_PROJECT_ID = var.project_id |
There was a problem hiding this comment.
| filename = "frontend/cloudbuild-deploy.yaml" | |
| substitutions = { | |
| _GCP_PROJECT_ID = var.project_id | |
| substitutions = { | |
| _GCP_PROJECT_ID = var.project_id | |
| _BACKEND_URL = module.compute.service_uri | |
| _FE_SERVICE_NAME = "${var.resource_prefix}-${var.environment}-frontend" | |
| _BACKEND_SERVICE_ID = module.compute.service_name | |
| _FIREBASE_SITE_ID = var.firebase_site_id | |
| } |
| service_account = google_service_account.trigger_sa.id | ||
| filename = "backend/cloudbuild.yaml" | ||
| substitutions = { | ||
| _GCP_PROJECT_ID = var.project_id | ||
| _GCP_REGION = var.region | ||
| _REPO_NAME = module.artifact.repository_name | ||
| _SERVICE_NAME = module.compute.service_name |
There was a problem hiding this comment.
| service_account = google_service_account.trigger_sa.id | |
| filename = "backend/cloudbuild.yaml" | |
| substitutions = { | |
| _GCP_PROJECT_ID = var.project_id | |
| _GCP_REGION = var.region | |
| _REPO_NAME = module.artifact.repository_name | |
| _SERVICE_NAME = module.compute.service_name | |
| substitutions = { | |
| _GCP_PROJECT_ID = var.project_id | |
| _GCP_REGION = var.region | |
| _REGION = var.region | |
| _REPO_NAME = module.artifact.repository_name | |
| _SERVICE_NAME = module.compute.service_name | |
| _DOCKER_IMAGE_URL = "${var.region}-docker.pkg.dev/${var.project_id}/${module.artifact.repository_name}/${var.backend_image_name}" | |
| } |
| - name: Build and push Backend Image | ||
| id: build-push | ||
| uses: docker/build-push-action@v6 | ||
| with: | ||
| context: ./backend | ||
| push: true | ||
| # Tags both 'latest' and the specific release version (e.g., v1.0.0) | ||
| tags: | | ||
| ${{ env.REGISTRY }}/${{ github.repository }}/backend:latest | ||
| ${{ env.REGISTRY }}/${{ github.repository }}/backend:${{ github.event.release.tag_name || github.sha }} | ||
| cache-from: type=gha | ||
| cache-to: type=gha,mode=max | ||
|
|
||
| - name: Install Cosign | ||
| uses: sigstore/cosign-installer@v3.5.0 | ||
|
|
||
| - name: Sign Backend Image | ||
| run: | | ||
| cosign sign --yes "${{ env.REGISTRY }}/${{ github.repository }}/backend@${{ steps.build-push.outputs.digest }}" | ||
|
|
||
| # --- JOB 2: FRONTEND (ZIP Archive for Release Assets) --- | ||
| build-frontend: |
There was a problem hiding this comment.
To fix this, we can introduce a step to lowercase the repository name and store it in an environment variable (REPO_LOWER), then reference ${{ env.REPO_LOWER }} in the build, tag, and sign steps.
| - name: Build and push Backend Image | |
| id: build-push | |
| uses: docker/build-push-action@v6 | |
| with: | |
| context: ./backend | |
| push: true | |
| # Tags both 'latest' and the specific release version (e.g., v1.0.0) | |
| tags: | | |
| ${{ env.REGISTRY }}/${{ github.repository }}/backend:latest | |
| ${{ env.REGISTRY }}/${{ github.repository }}/backend:${{ github.event.release.tag_name || github.sha }} | |
| cache-from: type=gha | |
| cache-to: type=gha,mode=max | |
| - name: Install Cosign | |
| uses: sigstore/cosign-installer@v3.5.0 | |
| - name: Sign Backend Image | |
| run: | | |
| cosign sign --yes "${{ env.REGISTRY }}/${{ github.repository }}/backend@${{ steps.build-push.outputs.digest }}" | |
| # --- JOB 2: FRONTEND (ZIP Archive for Release Assets) --- | |
| build-frontend: | |
| - name: Lowercase repository name | |
| id: lowercase-repo | |
| run: | | |
| echo "REPO_LOWER=${GITHUB_REPOSITORY,,}" >> ${GITHUB_ENV} | |
| - name: Build and push Backend Image | |
| id: build-push | |
| uses: docker/build-push-action@v6 | |
| with: | |
| context: ./backend | |
| push: true | |
| # Tags both 'latest' and the specific release version (e.g., v1.0.0) | |
| tags: | | |
| ${{ env.REGISTRY }}/${{ env.REPO_LOWER }}/backend:latest | |
| ${{ env.REGISTRY }}/${{ env.REPO_LOWER }}/backend:${{ github.event.release.tag_name || github.sha }} | |
| cache-from: type=gha | |
| cache-to: type=gha,mode=max | |
| - name: Install Cosign | |
| uses: sigstore/cosign-installer@v3.5.0 | |
| - name: Sign Backend Image | |
| run: | | |
| cosign sign --yes "${{ env.REGISTRY }}/${{ env.REPO_LOWER }}/backend@${{ steps.build-push.outputs.digest }}" |
| # Initialize Vertex AI SDK | ||
| vertexai.init( | ||
| project=config_service.PROJECT_ID, | ||
| location=config_service.WORKFLOWS_LOCATION, | ||
| location=config_service.AGENT_LOCATION, | ||
| api_transport="grpc", # Options: "grpc" or "rest" | ||
| ) |
There was a problem hiding this comment.
Since vertexai.init is already explicitly called when retrieving the remote agent inside the _get_remote_agent method, this module-level initialization is redundant and can be safely removed.
| # Initialize Vertex AI SDK | |
| vertexai.init( | |
| project=config_service.PROJECT_ID, | |
| location=config_service.WORKFLOWS_LOCATION, | |
| location=config_service.AGENT_LOCATION, | |
| api_transport="grpc", # Options: "grpc" or "rest" | |
| ) | |
| # Vertex AI SDK is initialized lazily when constructing or retrieving the remote agent |
…remove sequential step tracking in bootstrap script
…nt secret access, and update bootstrap environment variables
There was a problem hiding this comment.
This pull request introduces a highly robust, secure, and production-ready infrastructure refactoring, transitioning database access to a Private IP VPC-peered Cloud SQL PostgreSQL deployment. It also implements an automated database migration and seeding workflow using Google Cloud Run Jobs, alongside modular Terraform configurations that establish excellent architectural guardrails. While the structural quality and security postures are exceptional, a few critical errors—including unrecognized/hallucinated Terraform arguments, password generation characters, and local Unix socket parsing mismatches—must be resolved before merging to prevent direct provisioning and bootstrap execution failures.
🔍 General Feedback
- Exceptional Security and Isolation Design: Moving the database to a private-IP-only instance within a custom VPC is an outstanding enterprise-grade security enhancement.
- Robust Peering Guardrails: The introduction of the
peering_completedoutput dependency block to serialize Private Services Access network peering propagation before database creation is an excellent, production-grade fix for classic GCP deployment race conditions. - Clever Cloud Run Secret Version Management: Creating app secret shells with dummy placeholder versions inside Terraform, ignoring changes on
secret_data, and then populating actual secrets cleanly viabootstrap.shis an extremely smart and secure pattern that keeps plaintext secrets out of state files. - Unused API Activation: The API list (
apis_to_enableinvariables.tf) includesdrive.googleapis.com(Google Drive), but a search of the backend codebase indicates it is not used. We suggest removing it to minimize the project's default API enablement footprint. - Empty Locals File: The file
infrastructure/locals.tfonly contains a Google Project data source declaration and no actual local values. Consider consolidating this or renaming it appropriately.
| resource "google_secret_manager_secret_version" "db_secret_version" { | ||
| secret = google_secret_manager_secret.db_secret.id | ||
|
|
||
| # Using a write-only argument prevents the password | ||
| # from being captured in the terraform.tfstate file. | ||
| secret_data_wo = ephemeral.random_password.db_pass.result | ||
| secret_data_wo_version = var.db_password_version | ||
| } |
There was a problem hiding this comment.
| resource "google_secret_manager_secret_version" "db_secret_version" { | |
| secret = google_secret_manager_secret.db_secret.id | |
| # Using a write-only argument prevents the password | |
| # from being captured in the terraform.tfstate file. | |
| secret_data_wo = ephemeral.random_password.db_pass.result | |
| secret_data_wo_version = var.db_password_version | |
| } | |
| resource "google_secret_manager_secret_version" "db_secret_version" { | |
| secret = google_secret_manager_secret.db_secret.id | |
| secret_data = ephemeral.random_password.db_pass.result | |
| } |
| resource "google_sql_user" "app_user" { | ||
| name = var.db_user | ||
| instance = google_sql_database_instance.default.name | ||
| project = var.project_id | ||
|
|
||
| # We read the ephemeral value while creating the DB user, | ||
| # keeping the DB state clean of plaintext passwords. | ||
| password_wo = ephemeral.random_password.db_pass.result | ||
| password_wo_version = var.db_password_version | ||
| } |
There was a problem hiding this comment.
| resource "google_sql_user" "app_user" { | |
| name = var.db_user | |
| instance = google_sql_database_instance.default.name | |
| project = var.project_id | |
| # We read the ephemeral value while creating the DB user, | |
| # keeping the DB state clean of plaintext passwords. | |
| password_wo = ephemeral.random_password.db_pass.result | |
| password_wo_version = var.db_password_version | |
| } | |
| resource "google_sql_user" "app_user" { | |
| name = var.db_user | |
| instance = google_sql_database_instance.default.name | |
| project = var.project_id | |
| password = ephemeral.random_password.db_pass.result | |
| } |
| ephemeral "random_password" "db_pass" { | ||
| length = 24 | ||
| special = true | ||
| } |
There was a problem hiding this comment.
| ephemeral "random_password" "db_pass" { | |
| length = 24 | |
| special = true | |
| } | |
| ephemeral "random_password" "db_pass" { | |
| length = 24 | |
| special = false | |
| } |
| --subnet="$SUBNET_NAME" \ | ||
| --command="python" \ | ||
| --args="-m,bootstrap.bootstrap" \ | ||
| --add-cloudsql-instances="$DB_CONN_NAME" \ |
There was a problem hiding this comment.
When USE_CLOUD_SQL_AUTH_PROXY is true, get_conn_string() in database.py constructs a TCP connection URL using DB_HOST as the hostname. Placing the Unix socket folder /cloudsql/... in the hostname field of a postgresql+asyncpg:// URL is invalid because SQLAlchemy's URL parser will treat /cloudsql/... as part of the database path, leaving the host as None (empty), which causes asyncpg to fall back to connecting to 127.0.0.1:5432 over TCP. Since there is no TCP proxy running inside the Cloud Run Job container, this job will fail immediately with Connection refused.
To fix this, align this configuration with the main Cloud Run backend service: set USE_CLOUD_SQL_AUTH_PROXY=false and completely omit the /cloudsql/ DB_HOST reference. The backend's Python database connector will then securely initialize the connection using the Cloud SQL Python Connector library via private IP within the VPC, exactly as it does in production.
| --add-cloudsql-instances="$DB_CONN_NAME" \ | |
| --set-env-vars="INSTANCE_CONNECTION_NAME=${DB_CONN_NAME},DB_NAME=${DB_NAME},DB_USER=${DB_USER},USE_CLOUD_SQL_AUTH_PROXY=false,PROJECT_ID=${GCP_PROJECT_ID},GENMEDIA_BUCKET=${BUCKET_ASSETS},ADMIN_USER_EMAIL=${CURRENT_USER},ENVIRONMENT=development" \ |
| # Creates the Firebase Hosting site to deploy to | ||
| resource "google_firebase_hosting_site" "frontend" { | ||
| provider = google-beta | ||
| project = var.project_id | ||
| site_id = var.firebase_site_id |
There was a problem hiding this comment.
Please implement the conditional fallback directly in the resource configuration.
| # Creates the Firebase Hosting site to deploy to | |
| resource "google_firebase_hosting_site" "frontend" { | |
| provider = google-beta | |
| project = var.project_id | |
| site_id = var.firebase_site_id | |
| resource "google_firebase_hosting_site" "frontend" { | |
| provider = google-beta | |
| project = var.project_id | |
| site_id = var.firebase_site_id != "" ? var.firebase_site_id : var.project_id | |
| } |
| resource "google_project_iam_member" "secret_accessor" { | ||
| project = var.project_id | ||
| role = "roles/secretmanager.secretAccessor" | ||
| member = "serviceAccount:${google_service_account.trigger_sa.email}" | ||
| } |
There was a problem hiding this comment.
…anagement to use standard resources
There was a problem hiding this comment.
📋 Review Summary
This Pull Request is of exceptionally high quality, implementing a clean and secure "Idempotent Shell" pattern for infrastructure deployment. By moving from public to private Google Cloud SQL and configuring Direct VPC routing and Private Services Access, the deployment achieves production-grade security defaults. The corresponding refactoring of bootstrap.sh to support automated CLI profiles, resumption states, and data migration is beautifully engineered and extremely robust.
🔍 General Feedback
- Clean Architectural Alignment: Transitioning to a flattened
infrastructure/directory and utilizing environment-specific.tfvarsfile configurations significantly simplifies Terraform state management and variables mapping. - Robust Unit Testing: Introducing the
test_bootstrap_offline.shtest suite inside Github Actions is an outstanding engineering practice that guarantees the stability of bash scripts. - Idiomatic Code & Resource Lifecycle Management: Adding Python-level connector cleanup to the FastAPI lifespan lifecycle in
backend/main.pyis an excellent touch for preventing resource leaks. - Secure Default Configurations: Random password generation through Terraform and direct, automated injection into Secret Manager guarantees zero credential leakage.
| step 12 "Updating Remaining Secrets"; info "Navigating to $REPO_ROOT/infrastructure..."; cd "$REPO_ROOT/infrastructure" | ||
| info "Populating values in Secret Manager..."; local TERRAFORM_OUTPUTS=$(terraform output -json 2>/dev/null || echo "{}") | ||
| local FRONTEND_SECRETS=$( (echo "$TERRAFORM_OUTPUTS" 2>/dev/null || echo "{}") | jq -r 'try (.frontend_secrets.value[]?) catch ""' 2>/dev/null || echo "" ); local BACKEND_SECRETS=$( (echo "$TERRAFORM_OUTPUTS" 2>/dev/null || echo "{}") | jq -r 'try (.backend_secrets.value[]?) catch ""' 2>/dev/null || echo "" ) | ||
| local ALL_SECRETS=$(echo "${FRONTEND_SECRETS} ${BACKEND_SECRETS}" | tr ' ' '\n' | sort -u | grep .) |
There was a problem hiding this comment.
🟠 Potential script crash due to set -e when grep finds no matches
In bootstrap.sh, set -e is enabled globally. When FRONTEND_SECRETS and BACKEND_SECRETS are both empty, the pipeline grep . returns a non-zero exit status (exit code 1). Since grep is the final command in the pipeline and pipefail is not set, the pipeline itself exits with 1, which causes the entire script to terminate immediately at this assignment line.
The subsequent check if [ -z "$ALL_SECRETS" ]; ... is never reached because the script has already aborted.
Appending || true to the grep . command (or using grep . || :) ensures that the assignment is safe under set -e and empty input can be handled gracefully by the subsequent conditional check.
| local ALL_SECRETS=$(echo "${FRONTEND_SECRETS} ${BACKEND_SECRETS}" | tr ' ' '\n' | sort -u | grep .) | |
| local ALL_SECRETS=$(echo "${FRONTEND_SECRETS} ${BACKEND_SECRETS}" | tr ' ' '\n' | sort -u | grep . || true) |
3 participants
Fixes #<issue_number_goes_here>