Skip to content
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

fix: Allow EKS addons version config #303

Merged
merged 5 commits into from
Oct 15, 2024

Conversation

flamarion
Copy link
Contributor

Given the possibility of running different Kubernetes versions, the add-ons config must also be possible to configure to be compatible.

https://docs.aws.amazon.com/eks/latest/userguide/addon-compat.html

@flamarion flamarion force-pushed the fix_allow_eks_addons_version_config branch from 97b657c to 55fffea Compare October 14, 2024 10:35
Copy link
Contributor

@danielpanzella danielpanzella left a comment

Choose a reason for hiding this comment

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

Should we add some documentation, or pointers to relevant EKS docs for determining the correct addon versions

modules/app_eks/variables.tf Outdated Show resolved Hide resolved
@zacharyblasczyk
Copy link
Contributor

@danielpanzella, if we are going to expose variables for this, what do you think about (psudocode)

variable "eks_addon_versions" {
  description = "Map of EKS add-ons and their versions."
  type = map(string)
  default = {
    "aws-efs-csi-driver" = "v2.0.4-eksbuild.1",
    "aws-ebs-csi-driver" = "v1.31.0-eksbuild.1",
    "coredns"            = "v1.10.1-eksbuild.11",
    "kube-proxy"         = "v1.28.8-eksbuild.5",
    "vpc-cni"            = "v1.18.2-eksbuild.1"
  }
}

resource "aws_eks_addon" "default" {
  for_each               = var.eks_addon_versions
  cluster_name           = var.namespace
  addon_name             = each.key
  addon_version          = each.value
  resolve_conflicts      = "OVERWRITE"

  depends_on = [
    aws_eks_addon.vpc_cni
  ]
}

We could also make a local that has the map for each eks version say 1.26-1.29, and references the local default for the eks version unless you pass in your own map.

Not sure if the

 depends_on = [
    aws_eks_addon.vpc_cni
  ]  

Will be graceful, but I am sure we could just separate that one out if it is an issue.

Then we are only adding one variable that is extensible, and not adding n variables that we have to maintain and might create confusion.

@danielpanzella
Copy link
Contributor

@zacharyblasczyk since the user would or could be overriding the map as a whole, that allows them to accidentally leave out a required addon, no?

@zacharyblasczyk
Copy link
Contributor

We would have a merge like this:

locals {
  eks_addon_versions = merge(local.required_addons, var.eks_addon_versions_override)
}

@zacharyblasczyk
Copy link
Contributor

So nothing should be left out.

@flamarion
Copy link
Contributor Author

@zacharyblasczyk and @danielpanzella, is it ok to make it simple for now and allow the override?
I fixed what was suggested about the default duplication and docs.

@flamarion flamarion enabled auto-merge (squash) October 15, 2024 19:15
@flamarion flamarion merged commit 6316472 into main Oct 15, 2024
4 checks passed
@flamarion flamarion deleted the fix_allow_eks_addons_version_config branch October 15, 2024 20:42
jsbroks pushed a commit that referenced this pull request Oct 15, 2024
### [5.0.1](v5.0.0...v5.0.1) (2024-10-15)

### Bug Fixes

* Allow EKS addons version config ([#303](#303)) ([6316472](6316472))
* Update CODEOWNERS ([#304](#304)) ([0e7017b](0e7017b))
@jsbroks
Copy link
Member

jsbroks commented Oct 15, 2024

This PR is included in version 5.0.1 🎉

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.

4 participants