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

feat!: Add support for Outposts, remove node security group, add support for addon preserve and most_recent configurations #2250

Merged
merged 36 commits into from
Dec 5, 2022

Conversation

bryantbiggs
Copy link
Member

@bryantbiggs bryantbiggs commented Sep 29, 2022

Description

List of backwards incompatible changes

  • The cluster_id output used to output the name of the cluster. This is due to the fact that the cluster name is a unique constraint and therefore its set as the unique identifier within Terraform's state map. However, starting with local EKS clusters created on Outposts, there is now an attribute returned from the aws eks create-cluster API named id. The cluster_id has been updated to return this value which means that for current, standard EKS clusters created in the AWS cloud, no value will be returned (at the time of this writing) for cluster_id and only local EKS clusters on Outposts will return a value that looks like a UUID/GUID. Users should switch all instances of cluster_id to use cluster_name before upgrading to v19. Reference
  • Minimum supported version of Terraform AWS provider updated to v4.45 to support latest features provided via the resources utilized.
  • Minimum supported version of Terraform updated to v1.0
  • Individual security group created per EKS managed node group or self managed node group has been removed. This configuration went mostly un-used and would often cause confusion ("Why is there an empty security group attached to my nodes?"). This functionality can easily be replicated by user's providing one or more externally created security groups to attach to nodes launched from the node group.
  • Previously, var.iam_role_additional_policies (one for each of the following: cluster IAM role, EKS managed node group IAM role, self-managed node group IAM role, and Fargate Profile IAM role) accepted a list of strings. This worked well for policies that already existed but failed for policies being created at the same time as the cluster due to the well known issue of unkown values used in a for_each loop. To rectify this issue in v19.x, two changes were made:
    1. var.iam_role_additional_policies was changed from type list(string) to type map(string) -> this is a breaking change. More information on managing this change can be found below, under Terraform State Moves
    2. The logic used in the root module for this variable was changed to replace the use of try() with lookup(). More details on why can be found here
  • The cluster name has been removed from the Karpenter module event rule names. Due to the use of long cluster names appending to the provided naming scheme, the cluster name has moved to a ClusterName tag and the event rule name is now a prefix. This guarantees that users can have multiple instances of Karpenter withe their respective event rules/SQS queue without name collisions, while also still being able to identify which queues and event rules belong to which cluster.

Added

  • Support for setting preserve as well as most_recent on addons.
    • preserve indicates if you want to preserve the created resources when deleting the EKS add-on
    • most_recent indicates if you want to use the most recent revision of the add-on or the default version (default)

Modified

  • cluster_security_group_additional_rules and node_security_group_additional_rules have been modified to use lookup() instead of try() to avoid the well known issue of unkown values within a for_each loop
  • block_device_mappings previously required a map of maps but has since changed to an array of maps. Users can remove the outer key for each block device mapping and replace the outermost map {} with an array []. There are no state changes required for this change.
  • node_security_group_ntp_ipv4_cidr_block previously defaulted to ["0.0.0.0/0"] and now defaults to ["169.254.169.123/32"] (Referenc: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/set-time.html)
  • node_security_group_ntp_ipv6_cidr_block previously defaulted to ["::/0"] and now defaults to ["fd00:ec2::123/128"] (Referenc: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/set-time.html)
  • create_kms_key previously defaulted to false and now defaults to true. Clusters created with this module now default to enabling secret encryption by default with a customer managed KMS key created by this module
  • cluster_encryption_config previously used a type of list(any) and now uses a type of any -> users can simply remove the outer [...] brackets on v19.x
    • cluster_encryption_config previously defaulted to [] and now defaults to {resources = ["secrets"]} to encrypt secrets by default
  • cluster_endpoint_public_access previously defaulted to true and now defaults to false. Clusters created with this module now default to private only access to the cluster endpoint
    • cluster_endpoint_private_access previously defaulted to false and now defaults to true
  • The addon configuration now sets "OVERWRITE" as the default value for resolve_conflicts to ease addon upgrade management. Users can opt out of this by instead setting "NONE" as the value for resolve_conflicts
  • The kms module used has been updated from v1.0.2 to v1.1.0 - no material changes other than updated to latest
⚠️ See UPGRADE-19.0.md for full list of changes and upgrade guidance

Motivation and Context

Breaking Changes

How Has This Been Tested?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using one or more of the provided examples/* projects
  • I have executed pre-commit run -a on my pull request

@bryantbiggs bryantbiggs changed the title refactor: Change default NTP CIDR blocks to use those provided by Amazon feat!: Add support for Outposts, remove node security group, add support for addon preserve and most_recent configurations Sep 29, 2022
Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

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

Looks pretty good! Just a few minor comments here and there.

docs/UPGRADE-19.0.md Outdated Show resolved Hide resolved
docs/UPGRADE-19.0.md Outdated Show resolved Hide resolved
docs/UPGRADE-19.0.md Outdated Show resolved Hide resolved
docs/UPGRADE-19.0.md Outdated Show resolved Hide resolved
docs/UPGRADE-19.0.md Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
modules/eks-managed-node-group/main.tf Outdated Show resolved Hide resolved
variables.tf Show resolved Hide resolved
Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

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

Fixed in 87ced51

When specifying create_kms_key = false, this error happens.

cluster_encryption_config = {
    provider_key_arn = aws_kms_key.eks.arn
    resources        = ["secrets"]
}
create_kms_key = false
╷
│ Error: Unsupported attribute
│ 
│   on .terraform/modules/eks/main.tf line 350, in resource "aws_iam_policy" "cluster_encryption":
│  350:         Resource = var.create_kms_key ? [module.kms.key_arn] : [for config in var.cluster_encryption_config : config.provider_key_arn]
│ 
│ Can't access attributes on a primitive-typed value (string).
╵

Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

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

After a small fix in the upgrade guide, it looks great to me. I have successfully applied this PR and migrated from v18. No problems so far :)

@bryantbiggs
Copy link
Member Author

FYI - we are validating deployment of EKS on Outposts in local mode and further changes may be added

Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

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

LGTM (still)

modules/eks-managed-node-group/variables.tf Show resolved Hide resolved
@bryantbiggs bryantbiggs force-pushed the refactor/v19 branch 3 times, most recently from f106c6c to 7146a82 Compare October 31, 2022 14:33
@bryantbiggs
Copy link
Member Author

bug found in provider, converting to draft until fixed and supported hashicorp/terraform-provider-aws#27560

@bryantbiggs bryantbiggs marked this pull request as draft October 31, 2022 20:48
main.tf Outdated Show resolved Hide resolved
@bryantbiggs bryantbiggs marked this pull request as ready for review December 5, 2022 21:23
@bryantbiggs bryantbiggs merged commit b2e97ca into terraform-aws-modules:master Dec 5, 2022
@bryantbiggs bryantbiggs deleted the refactor/v19 branch December 5, 2022 21:26
antonbabenko pushed a commit that referenced this pull request Dec 5, 2022
## [19.0.0](v18.31.2...v19.0.0) (2022-12-05)

### ⚠ BREAKING CHANGES

* Add support for Outposts, remove node security group, add support for addon `preserve` and `most_recent` configurations (#2250)

### Features

* Add support for Outposts, remove node security group, add support for addon `preserve` and `most_recent` configurations ([#2250](#2250)) ([b2e97ca](b2e97ca))
@antonbabenko
Copy link
Member

This PR is included in version 19.0.0 🎉

spr-mweber3 pushed a commit to spring-media/terraform-aws-eks that referenced this pull request Jan 4, 2023
…ort for addon `preserve` and `most_recent` configurations (terraform-aws-modules#2250)

Co-authored-by: Anton Babenko <anton@antonbabenko.com>
Resolves undefined
spr-mweber3 pushed a commit to spring-media/terraform-aws-eks that referenced this pull request Jan 4, 2023
## [19.0.0](terraform-aws-modules/terraform-aws-eks@v18.31.2...v19.0.0) (2022-12-05)

### ⚠ BREAKING CHANGES

* Add support for Outposts, remove node security group, add support for addon `preserve` and `most_recent` configurations (terraform-aws-modules#2250)

### Features

* Add support for Outposts, remove node security group, add support for addon `preserve` and `most_recent` configurations ([terraform-aws-modules#2250](terraform-aws-modules#2250)) ([b2e97ca](terraform-aws-modules@b2e97ca))
@github-actions
Copy link

github-actions bot commented Jan 5, 2023

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants