infra PR2: add GCP Terraform baseline modules#17
Merged
Conversation
📚 Documentation Statusℹ️ No documentation-related changes detected This comment is automatically generated by the documentation workflow. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+51
to
+57
| - `project_id` (string) | ||
| - `secret_ids` (set(string)) | ||
| - `runner_service_account_email` (string) | ||
|
|
||
| Outputs: | ||
| - `secret_ids` | ||
| - `secret_resource_ids` |
There was a problem hiding this comment.
The secrets module is documented but wasn't actually added in this PR
Suggested change
| - `project_id` (string) | |
| - `secret_ids` (set(string)) | |
| - `runner_service_account_email` (string) | |
| Outputs: | |
| - `secret_ids` | |
| - `secret_resource_ids` |
Prompt To Fix With AI
This is a comment left during a code review.
Path: infra/terraform/README.md
Line: 51-57
Comment:
The `secrets` module is documented but wasn't actually added in this PR
```suggestion
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+14
to
+29
| ## Module contracts | ||
|
|
||
| ### `network` | ||
| Inputs: | ||
| - `project_id` (string) | ||
| - `region` (string) | ||
| - `network_name` (string) | ||
| - `subnet_name` (string) | ||
| - `subnet_cidr` (string) | ||
| - `allow_ssh_cidrs` (list(string), default `[]`) | ||
|
|
||
| Outputs: | ||
| - `network_name` | ||
| - `network_self_link` | ||
| - `subnetwork_name` | ||
| - `subnetwork_self_link` |
There was a problem hiding this comment.
enable_private_google_access is missing from the documented inputs (used in modules/network/variables.tf and modules/network/main.tf:14)
Prompt To Fix With AI
This is a comment left during a code review.
Path: infra/terraform/README.md
Line: 14-29
Comment:
`enable_private_google_access` is missing from the documented inputs (used in `modules/network/variables.tf` and `modules/network/main.tf:14`)
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+31
to
+46
| ### `runner_vm` | ||
| Inputs: | ||
| - `project_id` (string) | ||
| - `zone` (string) | ||
| - `instance_name` (string) | ||
| - `machine_type` (string, default `e2-standard-2`) | ||
| - `network_self_link` (string) | ||
| - `subnetwork_self_link` (string) | ||
| - `create_service_account` (bool, default `true`) | ||
| - `service_account_email` (string, optional when `create_service_account=true`) | ||
| - `startup_script` (string, optional) | ||
|
|
||
| Outputs: | ||
| - `instance_name` | ||
| - `instance_self_link` | ||
| - `instance_external_ip` |
There was a problem hiding this comment.
Missing several variables from documentation: service_account_id, metadata, tags, boot_image, boot_disk_size_gb, boot_disk_type (all defined in modules/runner_vm/variables.tf)
Prompt To Fix With AI
This is a comment left during a code review.
Path: infra/terraform/README.md
Line: 31-46
Comment:
Missing several variables from documentation: `service_account_id`, `metadata`, `tags`, `boot_image`, `boot_disk_size_gb`, `boot_disk_type` (all defined in `modules/runner_vm/variables.tf`)
How can I resolve this? If you propose a fix, please make it concise.
|
|
||
| - `modules/network`: VPC, subnet, and baseline firewall rules. | ||
| - `modules/runner_vm`: Compute Engine runner VM with Docker-friendly bootstrap hooks. | ||
| - `modules/secrets`: Secret Manager secret containers and runner service account access grants. |
There was a problem hiding this comment.
The modules/secrets module is documented here but doesn't exist in this PR. Either remove this documentation or add the missing module.
Suggested change
| - `modules/secrets`: Secret Manager secret containers and runner service account access grants. | |
| - `modules/observability`: Log-based metric and optional alert policy for runtime errors. |
Prompt To Fix With AI
This is a comment left during a code review.
Path: infra/terraform/README.md
Line: 11
Comment:
The `modules/secrets` module is documented here but doesn't exist in this PR. Either remove this documentation or add the missing module.
```suggestion
- `modules/observability`: Log-based metric and optional alert policy for runtime errors.
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+49
to
+58
| ### `secrets` | ||
| Inputs: | ||
| - `project_id` (string) | ||
| - `secret_ids` (set(string)) | ||
| - `runner_service_account_email` (string) | ||
|
|
||
| Outputs: | ||
| - `secret_ids` | ||
| - `secret_resource_ids` | ||
|
|
There was a problem hiding this comment.
Documentation for secrets module exists but the module itself is missing from this PR. Remove this entire section or implement the module.
Suggested change
| ### `secrets` | |
| Inputs: | |
| - `project_id` (string) | |
| - `secret_ids` (set(string)) | |
| - `runner_service_account_email` (string) | |
| Outputs: | |
| - `secret_ids` | |
| - `secret_resource_ids` | |
| ### `observability` |
Prompt To Fix With AI
This is a comment left during a code review.
Path: infra/terraform/README.md
Line: 49-58
Comment:
Documentation for `secrets` module exists but the module itself is missing from this PR. Remove this entire section or implement the module.
```suggestion
### `observability`
```
How can I resolve this? If you propose a fix, please make it concise.| - `network_name` (string) | ||
| - `subnet_name` (string) | ||
| - `subnet_cidr` (string) | ||
| - `allow_ssh_cidrs` (list(string), default `[]`) |
There was a problem hiding this comment.
Missing enable_private_google_access input (bool, default true) from the module contract
Prompt To Fix With AI
This is a comment left during a code review.
Path: infra/terraform/README.md
Line: 23
Comment:
Missing `enable_private_google_access` input (bool, default `true`) from the module contract
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+31
to
+42
| ### `runner_vm` | ||
| Inputs: | ||
| - `project_id` (string) | ||
| - `zone` (string) | ||
| - `instance_name` (string) | ||
| - `machine_type` (string, default `e2-standard-2`) | ||
| - `network_self_link` (string) | ||
| - `subnetwork_self_link` (string) | ||
| - `create_service_account` (bool, default `true`) | ||
| - `service_account_email` (string, optional when `create_service_account=true`) | ||
| - `startup_script` (string, optional) | ||
|
|
There was a problem hiding this comment.
Missing several inputs from the module contract: service_account_id, metadata, tags, boot_image, boot_disk_size_gb, boot_disk_type. These are all optional with defaults but should be documented for completeness.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: infra/terraform/README.md
Line: 31-42
Comment:
Missing several inputs from the module contract: `service_account_id`, `metadata`, `tags`, `boot_image`, `boot_disk_size_gb`, `boot_disk_type`. These are all optional with defaults but should be documented for completeness.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| service_account { | ||
| email = local.resolved_service_account_email | ||
| scopes = ["https://www.googleapis.com/auth/cloud-platform"] |
There was a problem hiding this comment.
Using cloud-platform scope grants full access to all GCP APIs. Consider using more restrictive scopes based on actual needs (e.g., compute-rw, logging-write, monitoring-write) to follow the principle of least privilege.
Prompt To Fix With AI
This is a comment left during a code review.
Path: infra/terraform/modules/runner_vm/main.tf
Line: 41
Comment:
Using `cloud-platform` scope grants full access to all GCP APIs. Consider using more restrictive scopes based on actual needs (e.g., `compute-rw`, `logging-write`, `monitoring-write`) to follow the principle of least privilege.
How can I resolve this? If you propose a fix, please make it concise.
| - `create_service_account` (bool, default `true`) | ||
| - `service_account_id` (string, default `neural-runner`) | ||
| - `service_account_email` (string, required when `create_service_account=false`) | ||
| - `service_account_scopes` (list(string), default least-privilege runner scopes) |
There was a problem hiding this comment.
documentation claims "least-privilege runner scopes" but the actual default includes full secretmanager scope (line 57 of modules/runner_vm/variables.tf), which grants access to all Secret Manager operations
Prompt To Fix With AI
This is a comment left during a code review.
Path: infra/terraform/README.md
Line: 43
Comment:
documentation claims "least-privilege runner scopes" but the actual default includes full `secretmanager` scope (line 57 of `modules/runner_vm/variables.tf`), which grants access to all Secret Manager operations
How can I resolve this? If you propose a fix, please make it concise.| network = var.network_self_link | ||
| subnetwork = var.subnetwork_self_link | ||
|
|
||
| access_config {} |
There was a problem hiding this comment.
always assigns a public IP - consider making this optional via a variable for deployments that don't require external access
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: infra/terraform/modules/runner_vm/main.tf
Line: 32
Comment:
always assigns a public IP - consider making this optional via a variable for deployments that don't require external access
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+61
to
+72
| variable "startup_script" { | ||
| description = "Startup script for VM bootstrap" | ||
| type = string | ||
| default = <<-EOT | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
|
|
||
| apt-get update | ||
| apt-get install -y docker.io | ||
| systemctl enable docker | ||
| systemctl start docker | ||
| EOT |
There was a problem hiding this comment.
startup script installs docker but doesn't add any users to the docker group - only root can run docker commands without sudo
Prompt To Fix With AI
This is a comment left during a code review.
Path: infra/terraform/modules/runner_vm/variables.tf
Line: 61-72
Comment:
startup script installs docker but doesn't add any users to the `docker` group - only root can run docker commands without sudo
How can I resolve this? If you propose a fix, please make it concise.
|
|
||
| output "instance_external_ip" { | ||
| description = "External IP address for the runner VM" | ||
| value = google_compute_instance.runner.network_interface[0].access_config[0].nat_ip |
There was a problem hiding this comment.
access_config[0] will error when assign_public_ip=false (empty list)
Suggested change
| value = google_compute_instance.runner.network_interface[0].access_config[0].nat_ip | |
| value = length(google_compute_instance.runner.network_interface[0].access_config) > 0 ? google_compute_instance.runner.network_interface[0].access_config[0].nat_ip : null |
Prompt To Fix With AI
This is a comment left during a code review.
Path: infra/terraform/modules/runner_vm/outputs.tf
Line: 13
Comment:
`access_config[0]` will error when `assign_public_ip=false` (empty list)
```suggestion
value = length(google_compute_instance.runner.network_interface[0].access_config) > 0 ? google_compute_instance.runner.network_interface[0].access_config[0].nat_ip : null
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+17
to
+34
| resource "google_compute_firewall" "allow_internal" { | ||
| project = var.project_id | ||
| name = "${var.network_name}-allow-internal" | ||
| network = google_compute_network.this.name | ||
|
|
||
| allow { | ||
| protocol = "tcp" | ||
| } | ||
|
|
||
| allow { | ||
| protocol = "udp" | ||
| } | ||
|
|
||
| allow { | ||
| protocol = "icmp" | ||
| } | ||
|
|
||
| source_ranges = [var.subnet_cidr] |
There was a problem hiding this comment.
firewall rule allows ALL TCP and UDP ports from subnet CIDR without restrictions - consider limiting to specific ports needed for internal services
Suggested change
| resource "google_compute_firewall" "allow_internal" { | |
| project = var.project_id | |
| name = "${var.network_name}-allow-internal" | |
| network = google_compute_network.this.name | |
| allow { | |
| protocol = "tcp" | |
| } | |
| allow { | |
| protocol = "udp" | |
| } | |
| allow { | |
| protocol = "icmp" | |
| } | |
| source_ranges = [var.subnet_cidr] | |
| resource "google_compute_firewall" "allow_internal" { | |
| project = var.project_id | |
| name = "${var.network_name}-allow-internal" | |
| network = google_compute_network.this.name | |
| allow { | |
| protocol = "icmp" | |
| } | |
| # Add specific port ranges as needed, e.g.: | |
| # allow { | |
| # protocol = "tcp" | |
| # ports = ["80", "443", "8080"] | |
| # } | |
| source_ranges = [var.subnet_cidr] | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: infra/terraform/modules/network/main.tf
Line: 17-34
Comment:
firewall rule allows ALL TCP and UDP ports from subnet CIDR without restrictions - consider limiting to specific ports needed for internal services
```suggestion
resource "google_compute_firewall" "allow_internal" {
project = var.project_id
name = "${var.network_name}-allow-internal"
network = google_compute_network.this.name
allow {
protocol = "icmp"
}
# Add specific port ranges as needed, e.g.:
# allow {
# protocol = "tcp"
# ports = ["80", "443", "8080"]
# }
source_ranges = [var.subnet_cidr]
}
```
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| filter = <<-EOT | ||
| resource.type="gce_instance" | ||
| resource.labels.instance_id:* |
There was a problem hiding this comment.
trailing whitespace after resource.labels.instance_id:*
Suggested change
| resource.labels.instance_id:* | |
| resource.labels.instance_id:* |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: infra/terraform/modules/observability/main.tf
Line: 7
Comment:
trailing whitespace after `resource.labels.instance_id:*`
```suggestion
resource.labels.instance_id:*
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| @@ -1,3 +1,5 @@ | |||
| """Helpers for resolving Polymarket US auth credentials from env/files.""" | |||
There was a problem hiding this comment.
polymarket auth changes (docstrings/formatting) are unrelated to the Terraform infrastructure PR - consider separating into a dedicated commit for cleaner git history
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: neural/auth/polymarket_us_env.py
Line: 1
Comment:
polymarket auth changes (docstrings/formatting) are unrelated to the Terraform infrastructure PR - consider separating into a dedicated commit for cleaner git history
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
|
|
||
| lifecycle { | ||
| precondition { | ||
| condition = var.create_service_account || var.service_account_email != null |
There was a problem hiding this comment.
precondition allows empty string for service_account_email (empty string != null), which would cause runtime failure. Add empty string check:
Suggested change
| condition = var.create_service_account || var.service_account_email != null | |
| condition = var.create_service_account || (var.service_account_email != null && var.service_account_email != "") |
Prompt To Fix With AI
This is a comment left during a code review.
Path: infra/terraform/modules/runner_vm/main.tf
Line: 55
Comment:
precondition allows empty string for `service_account_email` (empty string != null), which would cause runtime failure. Add empty string check:
```suggestion
condition = var.create_service_account || (var.service_account_email != null && var.service_account_email != "")
```
How can I resolve this? If you propose a fix, please make it concise.
|
|
||
| - `modules/network`: VPC, subnet, and baseline firewall rules. | ||
| - `modules/runner_vm`: Compute Engine runner VM with Docker-friendly bootstrap hooks. | ||
| - `modules/secrets`: Secret Manager secret containers and runner service account access grants. |
There was a problem hiding this comment.
modules/secrets is documented but doesn't exist in this PR - remove from list or add the module
Prompt To Fix With AI
This is a comment left during a code review.
Path: infra/terraform/README.md
Line: 11
Comment:
`modules/secrets` is documented but doesn't exist in this PR - remove from list or add the module
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+60
to
+68
| ### `secrets` | ||
| Inputs: | ||
| - `project_id` (string) | ||
| - `secret_ids` (set(string)) | ||
| - `runner_service_account_email` (string) | ||
|
|
||
| Outputs: | ||
| - `secret_ids` | ||
| - `secret_resource_ids` |
There was a problem hiding this comment.
entire secrets module section should be removed - the module doesn't exist in this PR
Prompt To Fix With AI
This is a comment left during a code review.
Path: infra/terraform/README.md
Line: 60-68
Comment:
entire `secrets` module section should be removed - the module doesn't exist in this PR
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+45
to
+58
| resource "google_compute_firewall" "allow_ssh" { | ||
| count = length(var.allow_ssh_cidrs) > 0 ? 1 : 0 | ||
|
|
||
| project = var.project_id | ||
| name = "${var.network_name}-allow-ssh" | ||
| network = google_compute_network.this.name | ||
|
|
||
| allow { | ||
| protocol = "tcp" | ||
| ports = ["22"] | ||
| } | ||
|
|
||
| source_ranges = var.allow_ssh_cidrs | ||
| } |
There was a problem hiding this comment.
SSH firewall rule applies to all instances in the VPC - add target_tags to restrict SSH access to only tagged instances (e.g., instances with the "neural-runner" tag)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: infra/terraform/modules/network/main.tf
Line: 45-58
Comment:
SSH firewall rule applies to all instances in the VPC - add `target_tags` to restrict SSH access to only tagged instances (e.g., instances with the "neural-runner" tag)
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
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
1 participant
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
infra/terraformbaseline for GCP with pinned provider/core versionsnetwork(VPC, subnet, baseline firewall)runner_vm(Compute Engine runner + service account support)secrets(Secret Manager containers + IAM access grants)observability(log metric + optional alert policy).terraform.lock.hclinfra/terraform/README.mdValidation
terraform init -backend=false(ininfra/terraform)terraform fmt -check -recursive(ininfra/terraform)terraform validate(root + each module)Greptile Summary
This PR establishes a solid Terraform foundation for GCP infrastructure with four modular components:
Core Changes:
networkmodule: Creates VPC with custom subnet, private Google access, and firewall rules for internal traffic and SSHrunner_vmmodule: Provisions Compute Engine instances with optional service accounts, Docker bootstrap script, shielded instance config, and optional public IPssecretsmodule: Manages Secret Manager containers and grants runner service account access via IAM bindingsobservabilitymodule: Sets up log-based metrics for error tracking with optional alert policiesNotable Security Features:
Documentation:
The code addresses many concerns from previous review threads - the secrets module exists, docker permissions are handled, public IP is optional, and service account scopes are appropriately restrictive.
Confidence Score: 4/5
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[VPC Network] --> B[Subnet with Private Google Access] B --> C[Runner VM Instance] C --> D[Service Account] D --> E[IAM Scopes: logging, monitoring, storage] C --> F[Shielded Instance Config] C --> G[Optional Public IP] H[Secret Manager Secrets] --> I[IAM Binding] I --> D J[Log Metric] --> K{Error Logs?} K -->|Yes| L[Alert Policy] L --> M[Notification Channels] N[Firewall Rules] --> B N --> O[SSH Access] N --> P[Internal ICMP + Ports] C --> Q[Docker Bootstrap Script] Q --> R[Add users to docker group]Last reviewed commit: 77ce7ee