Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Nat router #10

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix: Nat router #10

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e082fdf
fix nat
zacharyblasczyk Dec 9, 2024
817ebce
fix
zacharyblasczyk Dec 9, 2024
ade740d
fix
zacharyblasczyk Dec 9, 2024
542a8fb
bump the helm chart version
zacharyblasczyk Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions modules/networking/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,15 @@ resource "google_service_networking_connection" "this" {
service = "servicenetworking.googleapis.com"
reserved_peering_ranges = [google_compute_global_address.this.name]
}

resource "google_compute_router" "this" {
name = "${var.namespace}-router"
network = google_compute_network.this.id
}

resource "google_compute_router_nat" "this" {
name = "${var.namespace}-nat"
router = google_compute_router.this.name
nat_ip_allocate_option = "AUTO_ONLY"
source_subnetwork_ip_ranges_to_nat = "ALL_SUBNETWORKS_ALL_IP_RANGES"
}
Comment on lines +32 to +37
Copy link

@coderabbitai coderabbitai bot Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance NAT configuration with security and performance settings.

The current NAT configuration uses the most permissive settings which might not be optimal for production:

  1. Consider restricting NAT to specific subnets instead of ALL_SUBNETWORKS_ALL_IP_RANGES if not all instances need internet access
  2. Add the following recommended configurations:
    • Configure minimum/maximum ports per VM
    • Set appropriate timeouts
    • Enable logging for better troubleshooting
 resource "google_compute_router_nat" "this" {
   name                               = "${var.namespace}-nat"
   router                             = google_compute_router.this.name
   nat_ip_allocate_option             = "AUTO_ONLY"
-  source_subnetwork_ip_ranges_to_nat = "ALL_SUBNETWORKS_ALL_IP_RANGES"
+  source_subnetwork_ip_ranges_to_nat = "LIST_OF_SUBNETWORKS"
+  subnetwork {
+    name                    = google_compute_subnetwork.this.id
+    source_ip_ranges_to_nat = ["ALL_IP_RANGES"]
+  }
+  min_ports_per_vm = 64
+  max_ports_per_vm = 4096
+  log_config {
+    enable = true
+    filter = "ERRORS_ONLY"
+  }
+  tcp_established_idle_timeout_sec = 1200
+  tcp_transitory_idle_timeout_sec = 30
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
resource "google_compute_router_nat" "this" {
name = "${var.namespace}-nat"
router = google_compute_router.this.name
nat_ip_allocate_option = "AUTO_ONLY"
source_subnetwork_ip_ranges_to_nat = "ALL_SUBNETWORKS_ALL_IP_RANGES"
}
resource "google_compute_router_nat" "this" {
name = "${var.namespace}-nat"
router = google_compute_router.this.name
nat_ip_allocate_option = "AUTO_ONLY"
source_subnetwork_ip_ranges_to_nat = "LIST_OF_SUBNETWORKS"
subnetwork {
name = google_compute_subnetwork.this.id
source_ip_ranges_to_nat = ["ALL_IP_RANGES"]
}
min_ports_per_vm = 64
max_ports_per_vm = 4096
log_config {
enable = true
filter = "ERRORS_ONLY"
}
tcp_established_idle_timeout_sec = 1200
tcp_transitory_idle_timeout_sec = 30
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will work as the pod ip's are 10.32.x.x

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zacharyblasczyk, understood. Thank you for pointing that out—the pod IPs being in the 10.32.x.x range means the suggested NAT configuration changes may not be appropriate here. Please disregard my earlier suggestion.

Comment on lines +26 to +37
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Missing firewall rules could impact network security and functionality

The verification confirms that the networking module lacks any firewall rules configuration. This is a significant security consideration as:

  • No egress rules are defined to control outbound traffic through the NAT gateway
  • No ingress rules exist to protect the VPC
  • The current setup might default to overly permissive or restrictive rules depending on the GCP defaults

Required actions:

  • Add google_compute_firewall rules to explicitly define:
    • Egress rules for internet access through NAT
    • Ingress rules for internal communication
    • Protocol and port restrictions
  • Consider implementing a separate firewall module or adding firewall resources to the existing networking module
🔗 Analysis chain

Verify firewall rules for the NAT setup.

The NAT configuration looks good, but ensure that appropriate firewall rules exist to:

  1. Allow egress traffic from private instances through NAT
  2. Control which instances can access the internet
  3. Define allowed protocols and ports

Let me check the content of the found file to see if it contains any relevant network configuration or TODOs.

Let me check if there are any other Terraform files in the networking module that might contain firewall rules.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for existing firewall rules in the Terraform configs
rg -l "google_compute_firewall" .

# Look for any network-related TODO comments
rg -l "TODO.*network|firewall|egress|ingress" .

Length of output: 114

Script:

#!/bin/bash
# Check content of the found file
cat ./modules/helm_release/main.tf

# Let's also check for any firewall-related resources or configurations in the networking module
cat ./modules/networking/main.tf

Length of output: 4934

Script:

#!/bin/bash
# List all terraform files in the networking module
ls -la ./modules/networking/

# Check for firewall rules in any variables or outputs files
cat ./modules/networking/variables.tf 2>/dev/null
cat ./modules/networking/outputs.tf 2>/dev/null

Length of output: 898

2 changes: 1 addition & 1 deletion variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ variable "google_auth" {

variable "chart_version" {
type = string
default = "0.2.8"
default = "0.3.6"
}

variable "github_bot" {
Expand Down
Loading