-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
feat: Init terraform #1
Conversation
modules/database/main.tf
Outdated
resource "random_pet" "postgres" { | ||
length = 2 | ||
keepers = { | ||
namespace = var.database_version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it just formatted POSTGRES_16
so I think we can just use the version directly
modules/database/main.tf
Outdated
database_version = var.database_version | ||
|
||
settings { | ||
tier = var.tier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically we aren't doing anything with the tier but felt wrong to hardcode directly here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are doing this fine within the module, but you should pass this all the way through to the top level variables file so it is configurable.
# Disable client certificate authentication, which reduces the attack surface | ||
# for the cluster by disabling this deprecated feature. It defaults to false, | ||
# but this will make it explicit and quiet some security tooling. | ||
master_auth { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kept this because the comment seemed like we should keep it
modules/gke/main.tf
Outdated
|
||
resource "random_pet" "node_pool" { | ||
keepers = { | ||
machine_type = var.machine_type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above, didn't feel right to hardcode the machine type here
modules/gke/main.tf
Outdated
"https://www.googleapis.com/auth/sqlservice.admin", | ||
] | ||
|
||
shielded_instance_config { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
read into what this is, seems like we should keep it for security
modules/gke/variables.tf
Outdated
variable "node_count" { | ||
description = "The number of nodes in the cluster" | ||
type = number | ||
default = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unsure what this should be
modules/networking/main.tf
Outdated
network = google_compute_network.vpc.self_link | ||
} | ||
|
||
resource "google_compute_global_address" "private_ip_address" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
turns out we actually need this so that the pod can talk to the cloudsql instance since it lives outside the VPC
modules/gke/variables.tf
Outdated
variable "machine_type" { | ||
description = "The machine type for the cluster" | ||
type = string | ||
default = "n2-standard-4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looked at the comparison chart and n2 is listed as "general purpose", I don't think we need the super high performant ones
main.tf
Outdated
source = "terraform-google-modules/project-factory/google//modules/project_services" | ||
version = "~> 14.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
16 is the latest. If this is greenfield, is there a reason not to use the latest?
https://registry.terraform.io/modules/terraform-google-modules/project-factory/google/16.0.0
modules/database/main.tf
Outdated
database_version = var.database_version | ||
|
||
settings { | ||
tier = var.tier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are doing this fine within the module, but you should pass this all the way through to the top level variables file so it is configurable.
|
||
resource "google_compute_subnetwork" "default" { | ||
name = "${var.namespace}-subnet" | ||
ip_cidr_range = "10.10.0.0/16" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a huge range just FYI.
https://www.davidc.net/sites/default/subnets/subnets.html?network=10.10.0.0&mask=16&division=1.0
65534 ip's.
versions.tf
Outdated
required_providers { | ||
google = { | ||
source = "hashicorp/google" | ||
version = "~> 5.40" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5.42
main.tf
Outdated
source = "./modules/gke" | ||
namespace = var.namespace | ||
|
||
deletion_protection = var.gke_delete_protection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about protection for redis & db
main.tf
Outdated
network_connection_string = try(module.networking.network_connection_string) | ||
network_self_link = try(module.networking.network_self_link) | ||
subnetwork_self_link = try(module.networking.subnetwork_self_link) | ||
network_id = try(module.networking.network_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whats the point of the try?
modules/service_accounts/main.tf
Outdated
|
||
resource "google_service_account" "this" { | ||
account_id = substr(random_id.this.dec, 0, 30) | ||
display_name = "Ctrlplane" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe prefix with namespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use deletion_protection
everywhere.
This issue has been resolved in version 1.0.1 🎉 |
No description provided.