Move remaining STRR services and jobs to Cloud SQL IAM auth - #1764
Move remaining STRR services and jobs to Cloud SQL IAM auth#1764Jacky-Pham wants to merge 10 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| CLOUDSQL_REQUIRED_ENVS = ( | ||
| "CLOUDSQL_INSTANCE_CONNECTION_NAME", | ||
| "DATABASE_NAME", | ||
| "DATABASE_USERNAME", | ||
| ) | ||
|
|
||
|
|
||
| def _is_deployed_gcp() -> bool: | ||
| return bool(os.getenv("K_SERVICE") or os.getenv("CLOUD_RUN_JOB")) | ||
|
|
||
|
|
||
| def _use_cloudsql_iam() -> bool: | ||
| return bool(os.getenv("CLOUDSQL_INSTANCE_CONNECTION_NAME")) or _is_deployed_gcp() | ||
|
|
||
|
|
||
| def _require_cloudsql_env(): | ||
| missing = [ | ||
| env_name for env_name in CLOUDSQL_REQUIRED_ENVS if not os.getenv(env_name) | ||
| ] | ||
| if missing: | ||
| raise RuntimeError( | ||
| f"Missing Cloud SQL IAM environment variables: {', '.join(missing)}" | ||
| ) | ||
|
|
||
|
|
||
| def _cloudsql_ip_type() -> str: | ||
| ip_type_name = os.getenv("CLOUDSQL_IP_TYPE", "PUBLIC").upper() | ||
| if ip_type_name not in ("PUBLIC", "PRIVATE"): | ||
| raise RuntimeError("CLOUDSQL_IP_TYPE must be PUBLIC or PRIVATE") | ||
| return ip_type_name | ||
|
|
||
|
|
||
| def _cloudsql_engine_options() -> dict: | ||
| config = DBConfig( | ||
| instance_name=os.environ["CLOUDSQL_INSTANCE_CONNECTION_NAME"], | ||
| database=os.environ["DATABASE_NAME"], | ||
| user=os.environ["DATABASE_USERNAME"], | ||
| ip_type=_cloudsql_ip_type(), | ||
| schema="", | ||
| ) | ||
| return {"creator": lambda: getconn(config)} | ||
|
|
||
|
|
||
| def _local_database_uri() -> str: | ||
| db_user = os.getenv("DATABASE_USERNAME", "") | ||
| db_password = os.getenv("DATABASE_PASSWORD", "") | ||
| db_name = os.getenv("DATABASE_NAME", "") | ||
| db_host = os.getenv("DATABASE_HOST", "") | ||
| db_port = int(os.getenv("DATABASE_PORT", "5432")) | ||
|
|
||
| if db_unix_socket := os.getenv("DATABASE_UNIX_SOCKET", None): | ||
| return f"postgresql+pg8000://{db_user}:{db_password}@/{db_name}?unix_sock={db_unix_socket}/.s.PGSQL.5432" | ||
|
|
||
| return f"postgresql+pg8000://{db_user}:{db_password}@{db_host}:{db_port}/{db_name}" | ||
|
|
||
|
|
||
| def _database_settings() -> tuple[str, dict]: | ||
| if _use_cloudsql_iam(): | ||
| _require_cloudsql_env() | ||
| return "postgresql+pg8000://", _cloudsql_engine_options() | ||
|
|
||
| return _local_database_uri(), {} |
There was a problem hiding this comment.
aren't all these lines simply duplicated in every config.py files in other services. this is the configuraiton i was pointing to before when i asked for centralization. sorry if that wasnt clear enough. lemme know if you have any questions
There was a problem hiding this comment.
Addressed in 7cf048e. I moved the full local/Cloud SQL IAM environment-to-SQLAlchemy configuration into the shared cloud-sql-connector package (bcgov/sbc-connect-common#77), replaced the duplicated blocks in all seven jobs and both queue services, and pinned the helper commit in each lockfile. Shared helper tests pass (19/19); auto-approval passes 21/21; the focused strr-email config tests pass 3/3.
|
CI container builds could not fetch the companion commit from the upstream repository before bcgov/sbc-connect-common#77 merges, so the dependency is now pinned to the exact commit on my fork. Once the companion PR merges, I will switch the source URL back to bcgov before this PR is merged. |
|
|
|
||
| import importlib | ||
|
|
||
| import pytest |
There was a problem hiding this comment.
do we need this test file now that the code is moved out of this repo. we can just add a common fixture and reuse that
3 participants
What changed
I moved the remaining STRR DB clients over to Cloud SQL automatic IAM auth:
strr-emailstrr-payjobs/interactions-update, with the env names standardized and the existingDB_USER==vault typo fixedDeployed GCP config now uses the Cloud SQL Python Connector with
enable_iam_auth=True,CLOUDSQL_INSTANCE_CONNECTION_NAME,DATABASE_NAME,DATABASE_USERNAME, andCLOUDSQL_IP_TYPE=PUBLIC.Local dev remains on the existing password/socket path as long as
CLOUDSQL_INSTANCE_CONNECTION_NAMEis unset and the app is not running on Cloud Run.1Password / rollout notes
I will need the 1Password values updated before these are rolled out:
DATABASE_IAM_USERNAMEforsa-api@bcrbk9-<env>.iamDATABASE_JOB_IAM_USERNAMEforsa-job@bcrbk9-<env>.iamDATABASE_INSTANCE_CONNECTION_NAMEshould point at the STRR Cloud SQL instance connection nameDATABASE_NAMEshould staystrr-dbCLOUDSQL_IP_TYPEshould stayPUBLICbcrbk9-testidentity/instance because UAT deploys into the test projectHeads up
The jobs run as
sa-job. A few of these jobs write to the DB, so ifsa-jobis still readonly onstrr-db, those jobs will connect with IAM but fail once they try to write. Before rolling out the writing jobs, SRE needs to either givesa-jobthe right DB role or decide which jobs should stay off IAM until that grant is fixed.Testing
python3 -m compileallon the touched config/database/test filespoetry run pytest tests/unit/test_config_cloudsql_iam.py --no-covinqueue_services/strr-emailpoetry run pytest tests/unit/test_config_cloudsql_iam.py --no-covinjobs/auto-approvalpoetry run pytest tests/unit/test_database_cloud.py tests/unit/test_database.py::test_cloud_sql_missing_vars_raises_error tests/unit/test_gcp_iam_vaults.py --no-covinjobs/interactions-updategit diff --check