-
Notifications
You must be signed in to change notification settings - Fork 19
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: Install AWS EKS Add-on #167
base: main
Are you sure you want to change the base?
Conversation
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.
Had a few questions, but seems sound overall
modules/app_eks/add-ons.tf
Outdated
|
||
|
||
|
||
|
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.
nit: extra lines
"arn:aws:iam::aws:policy/AmazonEKSWorkerNodePolicy", | ||
"arn:aws:iam::aws:policy/AmazonEKS_CNI_Policy", | ||
"arn:aws:iam::aws:policy/AmazonEC2ContainerRegistryReadOnly", | ||
], var.eks_policy_arns) |
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.
where does this get used?
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.
Remember when we did all the work on the IAM policy documents? (https://github.com/wandb/terraform-aws-wandb/pull/101/files). I think we determined we have to pass these here as well or the policies aren't attached to the node or the cluster. I'm not sure which.
But if you're asking about the policy specific to the CNI driver, that gets used for anything involving networking in EKS, so grabbing elastic ips, setting up container networking, etc.
preserve = false | ||
resolve_conflicts = "OVERWRITE" | ||
service_account_role_arn = aws_iam_role.node.arn | ||
} |
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.
Does this change have any impact on existing workloads?
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 should not have any impact at all.
] | ||
|
||
addon_name = "vpc-cni" | ||
addon_version = "v1.13.0-eksbuild.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.
This version is different than the deleted one, just wanna make sure this is the right one
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.
Yes. One of the things that can be used to differentiate the self-managed plugin vs. the EKS add-on are the version numbers. This particular version is the next "in line" -- most of our clusters, if not all, are running v.1.12 of the plugin, and I'm gunning for keeping the AWS recommendation of upgrading the CNI one minor version at a time.
This PR includes changes to terraform to install the EKS VPC-CNI add-on. This replaces the "self-managed" vpc-cni plugin that is installed on the control plane when the cluster is created.
The EKS Add-on provides promises as to compatibility that are not provided by the self-managed plugin, and these are critical in terms of streamlining the EKS upgrade process.