Skip to content

feat(tf): base infra for coyote - #262

Open
ashayshub wants to merge 1 commit into
mainfrom
ashay/base-tf-structure
Open

feat(tf): base infra for coyote#262
ashayshub wants to merge 1 commit into
mainfrom
ashay/base-tf-structure

Conversation

@ashayshub

@ashayshub ashayshub commented Feb 27, 2026

Copy link
Copy Markdown
Collaborator

This PR creates the base infra for coyote eks.

@ashayshub
ashayshub requested a review from a team February 27, 2026 20:20
@ashayshub
ashayshub requested a review from a team as a code owner February 27, 2026 20:20
@github-actions

github-actions Bot commented Feb 27, 2026

Copy link
Copy Markdown

Test Coverage: 73.48%

@ashayshub
ashayshub force-pushed the ashay/base-tf-structure branch from b818094 to 543bbfd Compare February 27, 2026 20:26
@ashayshub
ashayshub marked this pull request as draft February 27, 2026 20:56
@ashayshub ashayshub changed the title feat(tf): base infra for coyote [DRAFT] feat(tf): base infra for coyote Feb 27, 2026
@ashayshub
ashayshub force-pushed the ashay/base-tf-structure branch 2 times, most recently from 6276754 to 47f3596 Compare March 2, 2026 16:09
@ashayshub ashayshub changed the title [DRAFT] feat(tf): base infra for coyote feat(tf): base infra for coyote Mar 2, 2026
@ashayshub
ashayshub marked this pull request as ready for review March 2, 2026 16:12
@ashayshub
ashayshub force-pushed the ashay/base-tf-structure branch 2 times, most recently from ad7233e to 08c9df1 Compare March 2, 2026 17:43
@ashayshub
ashayshub marked this pull request as draft March 2, 2026 19:43
@ashayshub
ashayshub force-pushed the ashay/base-tf-structure branch 4 times, most recently from fdf1359 to c212590 Compare March 4, 2026 19:51
@ashayshub
ashayshub marked this pull request as ready for review March 4, 2026 19:51
name = local.eks_cluster_name
kubernetes_version = var.k8s_version
endpoint_private_access = true
endpoint_public_access = true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we avoid this? What do we do in the clickhouse clusters?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We can avoid it. Should I enable just the private access in this PR itself or a separate one that includes the tailscale config ? Fyi: It can be disabled later.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok, we can do it later. Let's just make sure it's on the list.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I checked this. In order to achieve this we need to have the terraform runner able to connect to the private endpoint of the eks. This way the terraform runner running the terraform code will be able to connect to the EKS. For this we may have to use self hosted terraform runner with the tailscale configured. I don't think this is required but we can do this at a later stage if at all needed for more security.

@@ -0,0 +1,39 @@
resource "helm_release" "metrics_server" {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I have implemented this in the EKS module itself along with a few other addons.

value = module.net.eks_subnet_ids
}

# eks

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why commented out?

Comment thread terraform/infra/providers.tf Outdated
}
}

# provider "datadog" {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I don't have the valid datadog creds for dev.

@svix-james

Copy link
Copy Markdown
Contributor

Ok, some things from actually reviewing the infra in AWS

  • Let's ditch the pet names -- we already have an animal name for the project, adding another random animal name on there is confusing 😆. If we really need this, can we just add a few random [a-z] characters instead?
  • There are 12 subnets.... I guess maybe we do need that many. It just seems insane.
  • What is the difference between svix node group and coyote node group?
  • Do we need modules nested under infra folder? Can there not just be a top-level modules folder under terraform? TBH I'm not clear if that would make more sense or not.
  • I honestly don't like the name edge for the module. I'm not exactly sure what it means. Maybe lb or ingress instead?

@ashayshub
ashayshub force-pushed the ashay/base-tf-structure branch 2 times, most recently from fecafe2 to 4fcd12c Compare March 5, 2026 22:20
@ashayshub

Copy link
Copy Markdown
Collaborator Author
  • Let's ditch the pet names -- we already have an animal name for the project, adding another random animal name on there is confusing 😆. If we really need this, can we just add a few random [a-z] characters instead?
    --> will do
  • There are 12 subnets.... I guess maybe we do need that many. It just seems insane.
    --> So
  • 1 this does not cost us.
  • 2 It is impossible later to add availabilty zones to the EKS cluster
  • 3 It would help us in testing the cluster later without needing to make changes
  • What is the difference between svix node group and coyote node group?
    ---> One is in the database subnet group and another is in the general subnet group
  • Do we need modules nested under infra folder? Can there not just be a top-level modules folder under terraform? TBH I'm not clear if that would make more sense or not.
    --> We need to segregate the infra into core infrastructure vs application infra structure. We name the terraform workspace likewise so that it is faster to deploy application which are not intertwined directly with the core infra. Apart from that it makes sense to sub-modularize the infra into network, eks etc..
  • I honestly don't like the name edge for the module. I'm not exactly sure what it means. Maybe lb or ingress instead?
    ---> I can name it ingress. The resource that resides on the brink of the infra is usually known as "edge" resource in the infra parlance. Later on adding a new service/ feature unrelated to ingress would help in calling it edge.

@svix-jbrown svix-jbrown left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wow, this is a lot of config that's totally distinct from and not compatible with the other two EKS setups we have. What's the long-term plan here? Are we committing to maintaining a totally-separate stack of terraform for this project? Is this just to set up for testing the helm chart/operator/whatever and tear down later?

On an architectural note, I'm skeptical of having the same terraform config create the EKS cluster and also install resources on it; this caused an endless number of problems for clickhouse (the official recommendation is that the config for a provider should not depend on the outcome of something else inside the same terraform state), which is why we have two totally independent terraform instances there. Ideally we would be using Terraform Stacks to get independent execution for the lower level that creates the network/EKS cluster, and the higher level that installs resources onto the cluster. If we don't want to use Stacks, we could also do it by having two different terraform states like we do for clickhouse (or we can just accept that sometimes this will fail in obscure ways).

}

module "eks_sa_role_lb_ctrl" {
source = "terraform-aws-modules/iam/aws//modules/iam-role-for-service-accounts"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

spurious slash

Suggested change
source = "terraform-aws-modules/iam/aws//modules/iam-role-for-service-accounts"
source = "terraform-aws-modules/iam/aws/modules/iam-role-for-service-accounts"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Not spurious, it is scoped for the iam-role-for-service-accounts submodule.

Comment on lines +29 to +59
most_recent = true
}

kube-proxy = {
most_recent = true
}

vpc-cni = {
most_recent = true
before_compute = true
}

aws-ebs-csi-driver = {
most_recent = true
service_account_role_arn = module.ebs_csi_irsa.arn
}

eks-pod-identity-agent = {
most_recent = true
}

metrics-server = {
most_recent = true
}

kube-state-metrics = {
most_recent = true
}

external-dns = {
most_recent = true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

setting most_recent on these seems like a footgun; it means that sometimes a random terraform apply will have a side-effect of bumping a component. I kind of think we should be pinning all of the versions (which I think we do for the other EKS cluster).

@ashayshub ashayshub Mar 6, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Most of these will work well for the cluster_version they have released it against. I wasn't in favour of using addons for cert-manager and external-dns but gave in considering the ease of use. I agree any major version change and I'll be in cold waters. I am taking out cert-manager and external-dns out of these addons section and am going to use a plain helm_release. For one reason: the lack of documentation on what goes behind it. I tried looking it up and it is not easy.

Comment on lines +128 to +145
"system-node-group-0" = {
instance_types = var.system_instance_types,
min_size = var.system_min_node_count,
max_size = var.system_max_node_count,
desired_size = var.system_desired_node_count,
kubernetes_version = var.k8s_version
ami_type = var.system_ami_type,
enable_efa_only = false, # this is enabled by default and might not allow common instance types?
use_latest_ami_release_version = true,
subnet_ids = var.eks_subnet_ids
taints = {
critical_addons = {
key = "CriticalAddonsOnly",
value = "true",
effect = "NO_SCHEDULE"
}
}
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

node groups with multiple AZs in them are dangerous because they'll break anything with an attached PVC, because the autoscaling controller doesn't know how to handle them. We should start this out right, with one node group of each class in each subnet.

@ashayshub ashayshub Mar 6, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

  • Would the coyote application place a criteria on users to deploy the db in a nodegroup without multi-az? Most clusters using this would have multi-az already into it? Shouldn't we architect the k8s operator in such a way that the operator chooses a primary zone for the db app (depending on the cloud) and sticks with it for the PVC with headless service that allows one to connect to a specific zone?

  • Will this db app need multi-az clustering at some point in time (I don't know when that will be if at all) say read replicas across AZ for better perf(?

Comment thread terraform/infra/modules/eks/cluster.tf Outdated
kubernetes_version = var.k8s_version
ami_type = var.system_ami_type,
enable_efa_only = false, # this is enabled by default and might not allow common instance types?
use_latest_ami_release_version = true,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same comment as before; setting this means that a random terraform apply will do the (downtime-inducing!) AMI upgrade on the node group. We should be pinning AMI releases and only upgrading by hand (and with staging/prod rollouts).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

True. Will fix it.

}

# auth
authentication_mode = "API_AND_CONFIG_MAP"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we actually need the config map? I know it was deprecated a long time ago and I am optimistic that we can just use IAM principals for new stuff

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I have just explicitly set the default value here. I can remove it but let's go with the defaults unless aws changes it?

# only ipv4
# for eks subnet, mimicking private subnet from
# https://github.com/terraform-aws-modules/terraform-aws-vpc/blob/cf18c37591f860908e2223b4f488787e8a5f74f3/main.tf

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we also want to create network endpoints for S3 (and any other services we use that have free endpoints) in here?


#ToDo: Dedicated AZ for monitoring/node affinity based on availability zone
# : for ebs vicinity
resource "helm_release" "kube_prometheus_stack" {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is there a reason this is set up totally differently than the prometheus that we use for everything else? It's not using the same repository, it's not on the same version, and it doesn't have the same config.

@@ -0,0 +1,16 @@
resource "kubernetes_storage_class_v1" "gp3_ebs" {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why is this in the observability section instead of in the main EKS cluster section?

also, in the other clusters we intentionally disable gp2 so it doesn't get picked; is that no longer necessary?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

So this SC was only scoped for observability but i'll change it and use a general one since I guess it isn't a norm to scope it for particular module. One SC would be enough for all the cluster requirements.


parameters = {
type = "gp3"
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we should be enabling default encryption on ebs volumes

Suggested change
}
parameters = {
type = "gp3"
encrypted = "true"

Comment thread terraform/infra/variables.tf Outdated

variable "app_instance_types" {
type = list(string)
default = ["t3.micro"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think anything will fit on a t3.micro.... I don't even think kubelet will fit on a t3.micro.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Agree. will fix it.

@ashayshub
ashayshub force-pushed the ashay/base-tf-structure branch 2 times, most recently from 41556a5 to 5087c6f Compare March 13, 2026 14:31
@ashayshub
ashayshub force-pushed the ashay/base-tf-structure branch from 5087c6f to 05e43bc Compare March 16, 2026 21:07
@ashayshub
ashayshub force-pushed the ashay/base-tf-structure branch 3 times, most recently from 9d6bbc9 to 632c6a0 Compare March 17, 2026 20:31
@ashayshub
ashayshub force-pushed the ashay/base-tf-structure branch from 632c6a0 to 0a9aefe Compare March 18, 2026 14:09
@svix-james

Copy link
Copy Markdown
Contributor

Can we move this under the infra folder, just to avoid cluttering top-level any further?

@github-actions

github-actions Bot commented Apr 9, 2026

Copy link
Copy Markdown
TestsPassed ☑️SkippedFailed ❌️Time ⏱
Diom CI JUnit262 ran261 passed0 skipped1 failed7m 32s 402ms
TestResultTime ⏱
Diom CI JUnit
diom-backend
diom-backend.core::cluster::serialized_state_machine::tests::test_serialize_to_file_round_trip❌ failure583ms

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants