feat!: refactor modules - #732
Conversation
…-root' into feat/move-module-to-root
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request reorganizes the repository by removing legacy files, introducing a new standalone multi-cluster discovery example, and adding a reusable cluster network module. It also updates helper deployment utilities and refactors the binary authorization build image module. The review feedback highlights several key improvement opportunities, including adding a configurable Cloud NAT option to the cluster network module to support multi-region internet egress, ensuring the standalone example explicitly enables Cloud NAT, and refining file operations in the deployer helper to guarantee idempotency and correct symbolic link handling.
|
|
||
| variable "shared_vpc_host" { | ||
| description = "Makes this project a Shared VPC host if 'true' (default 'false')" | ||
| type = bool | ||
| default = false |
There was a problem hiding this comment.
To support internet egress for private GKE clusters in non-shared VPC environments (such as the standalone cluster-multicluster-discovery example), we should introduce a create_cloud_nat variable to allow explicit control over Cloud NAT creation, rather than strictly tying it to shared_vpc_host.
variable "shared_vpc_host" {
description = "Makes this project a Shared VPC host if 'true' (default 'false')"
type = bool
default = false
}
variable "create_cloud_nat" {
description = "Create a Cloud NAT gateway for internet egress if 'true' (default 'false')"
type = bool
default = false
}
| resource "google_compute_router" "nat_router" { | ||
| for_each = var.shared_vpc_host ? { "create" : true } : {} | ||
| name = "nat-router-${var.region}" | ||
| region = var.region | ||
| network = module.cluster_vpc.network_self_link | ||
| project = module.cluster_vpc.project_id | ||
| } | ||
|
|
||
| resource "google_compute_router_nat" "cloud_nat" { | ||
| for_each = google_compute_router.nat_router | ||
| name = "cloud-nat" | ||
| router = each.value.name | ||
| region = each.value.region | ||
| project = module.cluster_vpc.project_id | ||
| nat_ip_allocate_option = "AUTO_ONLY" | ||
| source_subnetwork_ip_ranges_to_nat = "ALL_SUBNETWORKS_ALL_IP_RANGES" | ||
|
|
||
| log_config { | ||
| enable = true | ||
| filter = "ERRORS_ONLY" | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation only creates a single Cloud NAT gateway in the region specified by var.region. However, the module supports creating subnets across multiple regions. In a multi-region deployment (such as the cluster-multicluster-discovery example), subnets in other regions will not have internet egress, causing GKE private nodes in those regions to fail to pull external images or connect to external APIs (like Gitlab).
We can dynamically create a Cloud NAT gateway in each unique region where a subnet is defined by extracting the regions from var.subnets and using for_each over them.
locals {
nat_regions = (var.shared_vpc_host || var.create_cloud_nat) ? distinct([for s in var.subnets : s.subnet_region]) : []
}
resource "google_compute_router" "nat_router" {
for_each = toset(local.nat_regions)
name = "nat-router-${each.value}"
region = each.value
network = module.cluster_vpc.network_self_link
project = module.cluster_vpc.project_id
}
resource "google_compute_router_nat" "cloud_nat" {
for_each = google_compute_router.nat_router
name = "cloud-nat-${each.value}"
router = each.value.name
region = each.value.region
project = module.cluster_vpc.project_id
nat_ip_allocate_option = "AUTO_ONLY"
source_subnetwork_ip_ranges_to_nat = "ALL_SUBNETWORKS_ALL_IP_RANGES"
log_config {
enable = true
filter = "ERRORS_ONLY"
}
}
| module "cluster_network" { | ||
| source = "../../modules/cluster_network" | ||
|
|
||
| vpc_name = "vpc-eab-cluster" | ||
| project_id = var.project_id | ||
| region = var.regions[0] | ||
| shared_vpc_host = false |
There was a problem hiding this comment.
Since shared_vpc_host is set to false for this standalone example, we must explicitly set create_cloud_nat = true to ensure that Cloud NAT gateways are created in all cluster regions. This is required so that the private GKE Autopilot nodes can access external resources (such as pulling Kueue images from registry.k8s.io and syncing configurations from gitlab.com).
module "cluster_network" {
source = "../../modules/cluster_network"
vpc_name = "vpc-eab-cluster"
project_id = var.project_id
region = var.regions[0]
shared_vpc_host = false
create_cloud_nat = true
| // DeleteFile deletes a single file from the src path to the dest path | ||
| func DeleteFile(src string) error { | ||
| _, err := os.Stat(src) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| err = os.RemoveAll(src) | ||
| return err | ||
| } |
There was a problem hiding this comment.
The DeleteFile function is not idempotent because it calls os.Stat(src) first, which will return an error if the file or directory does not exist. Additionally, the comment contains a copy-paste typo mentioning a dest path that does not exist in the function signature. Since os.RemoveAll already handles non-existent paths gracefully and returns nil, we can simplify the function to make it idempotent and correct the comment.
// DeleteFile deletes a file or directory at the given path.
func DeleteFile(src string) error {
return os.RemoveAll(src)
}| return err | ||
| } | ||
| } else { | ||
| } else if !isSymlinkToDir(filepath.Join(src, f.Name())) { |
There was a problem hiding this comment.
The current implementation of CopyDirectory only skips symbolic links that point to directories, while symbolic links pointing to files are still copied (and dereferenced into regular files). This violates the stated behavior in the comment ('It will skip symbolic links'). We can simplify this check and correctly skip all symbolic links by checking the file type directly using f.Type() & os.ModeSymlink == 0.
| } else if !isSymlinkToDir(filepath.Join(src, f.Name())) { | |
| } else if f.Type()&os.ModeSymlink == 0 { |
bfe8c99 to
287d33a
Compare
7d9262a to
b967b1e
Compare
b967b1e to
e641120
Compare
1dcbf0f to
c3d98ac
Compare
6dbc369 to
0109d8c
Compare
035d4ea to
69809aa
Compare
69809aa to
156cafa
Compare
1 participant
Summary
This PR refactors the repository layout by moving all Terraform modules into a top-level
modules/directory and removing legacy step folders (0-bootstrap,1-multitenant,2-multitenant,3-fleetscope,4-appfactory,5-appinfra). It also introduces new reusable infrastructure modules (nat,private_workerpool,standalone-harness), renames existing modules for clarity, and updates the integration test framework.Key Changes
1. Repository Restructuring & Module Consolidation
0-bootstrap,1-multitenant,2-multitenant,3-fleetscope,4-appfactory,5-appinfra).app-group-baseline➔secure-cicd-pipelinecicd-pipeline➔deployment-pipeline2. New Terraform Modules
modules/nat: Added a modular Cloud NAT configuration for VPC networks.modules/private_workerpool: Created a module to handle Cloud Build Private Worker Pools, including network peering, subnet creation, and NAT configuration.modules/standalone-harness: Added a standalone harness module for testing environments.3. Module & Dependency Enhancements
module_dependenciesvariable across modules to manage execution dependencies cleanly without triggering Terraform providerdepends_onlimitations.secure-cicd-pipeline.4. Integration Testing & Test Setup
multi_cluster_discovery_test.goundertest/integration/multi_cluster_discovery/.test/setup/harness/gitlab/to include dedicated VM setup, NAT, and network modules.hub_networkmodule undertest/setup/modules/.Dependecies
5-appinfra/modules/...,3-fleetscope/modules/...) into the rootmodules/folder for better modularity and reusability. Dependes on feat!: moves modules to root #705Breaking Changes
<step>/modules/<module>or<step>/<module>tomodules/<module>. Upstream callers will need to update theirsourceparameters to reference the new paths under
modules/.Testing Strategy
multi_cluster_discovery.