refactor: Update to use EKS module which follows EKS Blueprints v5 direction #397
Conversation
✅ Deploy Preview for eks-workshop ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
| most_recent = true | ||
| } | ||
|
|
||
| resource "aws_eks_addon" "vpc_cni" { |
There was a problem hiding this comment.
Based on @niallthomson's findings, the EKS module was updated to allow configuring addons before compute is ready terraform-aws-modules/terraform-aws-eks#2478 by setting before_compute = true in the addon definition map
In addition, there is now a variable dataplane_wait_duration within the module that users can set a variable time duration between the control plane becoming active, and the start of downstream compute resource provisioning. Currently it defaults to "30s"
| locals { | ||
| ebs_csi_blocker = try(module.eks_blueprints_kubernetes_addons.aws_ebs_csi_driver.release_metadata.metadata.status, "") | ||
| } | ||
|
|
||
| resource "aws_iam_instance_profile" "karpenter" { |
There was a problem hiding this comment.
once https://github.com/aws-ia/terraform-aws-eks-blueprints-addons is ready, this will go away. This sub-module provisions all of the necessary AWS infrastructure resources, including this profile. The new addons module will use this plus the helm release for karpenter which will alleviate the need for this resource
| ] | ||
|
|
||
| eks_cluster_id = module.eks_blueprints.eks_cluster_id | ||
| eks_cluster_id = module.eks.cluster_name |
There was a problem hiding this comment.
You will see a lot of *cluster_id references being replaced with *cluster_name. You can see more details about this change here hashicorp/terraform-provider-aws#27560
| @@ -431,14 +413,10 @@ module "eks_blueprints_kubernetes_addons" { | |||
| module "eks_blueprints_ack_addons" { | |||
| source = "github.com/aws-ia/terraform-aws-eks-ack-addons?ref=v1.1.0" | |||
|
|
|||
| depends_on = [ | |||
There was a problem hiding this comment.
the module waits for the nodegroups, which they wait for the VPC CNI due to the before_compute which means this explicit dependency can be removed
| @@ -474,17 +452,15 @@ resource "random_string" "fluentbit_log_group" { | |||
| } | |||
|
|
|||
| locals { | |||
| oidc_url = replace(data.aws_eks_cluster.cluster.identity[0].oidc[0].issuer, "https://", "") | |||
There was a problem hiding this comment.
the EKS module outputs an attribute oidc_provider which is the issuer url with the https:// stripped. It is named oidc_provider because this is the name assigned in the console, regardless of whether the provider was created by EKS or not
cluster_oidc_issuer_url is the form with the fully qualified URL including https://
tl;dr - OIDC provider is whats used for IRSA, not the issuer URL
|
|
||
| cluster_name = var.environment_name | ||
| cluster_version = var.cluster_version | ||
| kms_key_enable_default_policy = true |
There was a problem hiding this comment.
replicates prior functionality to ensure no KMS key conflicts https://github.com/terraform-aws-modules/terraform-aws-kms/blob/87be9ccb61f073b58ed56de0456214c34d793841/main.tf#L89-L103
The KMS key created by the EKS module makes the cluster creator identity the key admin so this can be considered redundant, but for those using assumed roles with Terraform they can become confused since their identity wouldn't have access unless they explicitly added their role into the policy generated
| @@ -44,122 +58,77 @@ module "eks_blueprints" { | |||
| type = "ingress" | |||
| self = true | |||
| } | |||
| # Recommended outbound traffic for Node groups | |||
| egress_all = { | |||
There was a problem hiding this comment.
These rules are now added by default with the EKS module when node_security_group_enable_recommended_rules is true (which is the default) - https://github.com/terraform-aws-modules/terraform-aws-eks/blob/df1b62548c1d8c7117f4ab45c5b494de64b34cb8/node_groups.tf#L129-L180
The default rules do permit 1025-65535 for cross security group communication, but I suspect that with the sample applications there are additional ports (i.e. 80) required. I wasn't sure what was used so I left the custom rule addition for all cross security group communication open (rule directly above this)
|
|
||
| ami_type = "AL2_x86_64" |
There was a problem hiding this comment.
The way EKS Blueprints managed this is a bit different from the EKS module. By default the ami_type is AL2_x86_64, and the release version is selected at the time of creation. I'm not sure what the explicit release version is used for in this context, typically we pull the latest version which includes the latest patches/etc. at the time of nodegroup creation. Let me know if this release version need to be put back
|
|
||
| k8s_taints = [{ key = "systemComponent", value = "true", effect = "NO_SCHEDULE" }] |
There was a problem hiding this comment.
I did have one question - this nodegroup, "managed-system" has a taint, and the nodegroup directly below, "managed-ondemand-tainted", does not despite its name containing taint. Should the taint be on that nodegroup?
| # Reference docs https://docs.aws.amazon.com/eks/latest/userguide/cni-increase-ip-addresses.html | ||
| command = <<-EOT | ||
| sleep 30 | ||
| kubectl set env daemonset aws-node -n kube-system POD_SECURITY_GROUP_ENFORCING_MODE=standard --kubeconfig <(echo $KUBECONFIG | base64 --decode) |
There was a problem hiding this comment.
added into the VPC CNI custom configuration so all of this can be removed. Let me know if there is a reason this needs to be done this way instead of the custom config (currently both routes are shown/used)
|
Going to close this since the rearchitecture work will move away from this design |
What this PR does / why we need it:
Which issue(s) this PR fixes:
Reference aws-ia/terraform-aws-eks-blueprints#1421
Quality checks
make testormake e2e-testand it was successfulI tried to run
make testbut got this:And when I try
make e2e-test:I don't see an
e2e-testtarget in the Makefile, and I'm not sure what a "module" is to target withmake test. Any pointers on how to test would be appreciatedBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.