fix: cloud sql socket routing for feast/cron and safer destroy image cleanup#68
Merged
Conversation
The Cloud SQL lockdown removed the 0.0.0.0/0 authorized network, so direct TCP to the instance public IP no longer works. Both cloud_run templates still wired the public IP into the Feast and cron modules (postgres_host, feast_online_store_host, database_url), which times out at runtime. - repoint the postgres_host module output to the /cloudsql socket dir - switch remaining public-IP DSN references to their _cloud_sql variants - pass cloud_sql_connection_name to the cron module in the generic template so jobs mount the /cloudsql volume (mlflow template already did)
destroy deleted a hardcoded mlops-images Artifact Registry repo and the Cloud Build staging bucket unconditionally. A custom build-images --repository was missed while mlops-images was deleted anyway, and in a project shared by multiple workspaces, destroying one deleted images the others still run on. - add --repository (default mlops-images) matching build-images - add --keep-images to skip repo and staging bucket cleanup entirely - confirm interactively before deleting when --yes is not passed - document both flags in README and cli-commands
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
summary
Addresses findings 1 and 2 from the code review of #66, one commit each:
fix: route feast and cron db access through the cloud sql socket— the Cloud SQL lockdown removed the0.0.0.0/0authorized network, but both cloud_run templates still wired the instance public IP into the Feast and cron modules (postgres_host,feast_online_store_host,database_url), so those connections time out. Repoints thepostgres_hostmodule output to the/cloudsql/<connection_name>socket dir, switches remaining public-IP DSN references to their_cloud_sqlvariants, and passescloud_sql_connection_nameto the cron module in the generic template so jobs mount the/cloudsqlvolume.fix: make destroy image cleanup repo-aware and optional— destroy deleted a hardcodedmlops-imagesrepo and the Cloud Build staging bucket unconditionally, which misses custombuild-images --repositorynames and deletes images shared by other workspaces in the same project. Adds--repository(defaultmlops-images) and--keep-images, plus an interactive confirm when--yesis not passed. Docs updated.test plan
uv run --with pytest pytest tests/ -q→ 63 passedpython3 -m compileall -q src/→ cleanmlflow_main.tf.j2(mlflow+fastapi+grafana+feast+cron, postgres) exactly asdeploydoes →terraform init -backend=false && terraform validate→ validcloud_sql_connection_namedeployml destroy --helpshows new flags; no-workspace early-exit path unchangednotes
main.tf.j2(stacks without MLflow) failsterraform validateon its own with a duplicatemodule "workflow_orchestration_cron"block (same bug class as the Grafana duplicate fixed in fix: stop emitting the Grafana module twice in the cloud_run template #62) and a duplicatebackend_store_uriargument in the Feast module block. Confirmed present on unmodifieddev.related