Implemented GCP integrations - #24
Conversation
|
shreyas-lyzr
left a comment
shreyas-lyzr
left a comment
There was a problem hiding this comment.
GCP integration PR — well-structured addition. Read all six new source files in full plus the factory routing, tests, and deps. Three findings below: one worth addressing before the next release, two suggestions.
Security pass: no CVEs in any of the 12 new/bumped packages (OSV.dev checked), no hard-coded credentials or secrets, no injection sinks reached by untrusted input, factory routing follows the same credential-key dispatch pattern as AWS/Azure backends. Clean.
Suggestion (mysql IAM TLS default): MySQLSQLBackend.from_gcp_iam_auth does not setdefault('ssl_ca', ...) or otherwise enforce TLS the way PostgresSQLBackend.from_gcp_iam_auth does with setdefault('sslmode', 'require'). GCP docs require TLS for Cloud SQL IAM auth on MySQL as well. The current behaviour leaves a caller who forgets to pass ssl_ca silently connecting unencrypted and likely failing at the auth handshake only at runtime, with a confusing error. Consider mirroring the Postgres approach and at minimum documenting that the caller must supply SSL kwargs — the existing test test_mysql_gcp_iam_defaults does not assert any TLS default, so this gap has no coverage.
Suggestion (health_check permission requirement on pubsub/gcp_pubsub.py:174): health_check() on the pub/sub fan-out backend calls list_topics, which requires roles/pubsub.viewer or roles/pubsub.editor. A send-only service account granted only roles/pubsub.publisher will get a permission-denied (caught and returns False) — the health check appears broken even when the backend can publish fine. The messaging backend (messaging/gcp_pubsub.py) avoids this by calling get_topic / get_subscription instead. Consider switching to get_topic here as well.
Note (_pending dict growth in messaging backend): The _pending dict accumulates every received ack-id until delete(), nack(), or dead_letter() is called. This matches the SQS backend's shape and is the right design, but if messages are consistently received and never acked (e.g. a consumer crash loop), memory grows without bound. This is a pre-existing pattern, not new debt from this PR — worth noting in a follow-up.
| ) | ||
|
|
||
| @classmethod | ||
| def from_gcp_iam_auth( |
There was a problem hiding this comment.
from_gcp_iam_auth does not set a TLS default. The PostgreSQL counterpart (postgresql.py:254) calls connect_kwargs.setdefault('sslmode', 'require'). GCP Cloud SQL IAM auth on MySQL also requires TLS. Without a default here, a caller who omits ssl_ca/ssl_* connects unencrypted and hits a confusing auth error at runtime rather than a clear configuration error. At minimum, consider mirroring the Postgres default or asserting this in a test.
| async def health_check(self) -> bool: | ||
| try: | ||
| client = await self._ensure() | ||
| pager = await client.list_topics(project=f"projects/{self.project}") |
There was a problem hiding this comment.
list_topics requires roles/pubsub.viewer (or editor). A send-only service account granted only roles/pubsub.publisher will always return False here, making the health check appear broken even when publishing works. The messaging backend avoids this by calling get_topic instead — the same fix applies here. See also: the messaging GCP backend at messaging/gcp_pubsub.py:440.
2 participants
No description provided.