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.

Sign up for GitHub

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

Jump to bottom

feat: Add support for enabling addons before data plane compute is created #2478

Merged
merged 5 commits into from
Feb 17, 2023
Merged

feat: Add support for enabling addons before data plane compute is created #2478

merged 5 commits into from
Feb 17, 2023

Conversation

bryantbiggs
Copy link
Member

Description

  • Add support for creating addons before data plane compute is created
  • Add dependency for IPv6 policy to ensure policy cleanup/deletion happens without error

Motivation and Context

  • Currently, the addon implementation has a hard dependency on the compute constructs (nodegroups/Fargate). This was added previously due to the fact that the EKS API would attempt to create the addon before there was compute which resulted in a timeout failure. By making the addons depend on the compute, this ensured there were instances available to deploy the addons to. However, this is not valid for ALL addons; namely daemonsets need to be configured and deployed BEFORE compute. With this second addon resource that is not dependent on the compute, coupled with the sleep timer, this gives the addon enough time to provision before compute is created
  • Under the current example and module structure, if you attempt to create the IPv6 CNI IAM policy with the module, and then destroy the cluster, it will error with a dependency violation claiming that the policy cannot be deleted when its used by other resources. By adding the dependency for the policy, this has resolved the dependency conflict and allows the cluster and policies to be destroyed without error
  • Resolves Support for "early" cluster_addons (vpc-cni with ENABLE_PREFIX_DELEGATION) #2467

Breaking Changes

  • No

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 commented Feb 15, 2023
View reviewed changes
main.tf
@@ -85,7 +85,8 @@ resource "aws_eks_cluster" "this" {
aws_iam_role_policy_attachment.this,
aws_security_group_rule.cluster,
aws_security_group_rule.node,
aws_cloudwatch_log_group.this
aws_cloudwatch_log_group.this,
aws_iam_policy.cni_ipv6_policy,
Copy link
Member Author

Choose a reason for hiding this comment

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

this addition now allows the resources to be cleaned up properly when users elect to create the IPv6 policy with the module

bryantbiggs
bryantbiggs commented Feb 15, 2023
View reviewed changes
node_groups.tf
create_duration = var.dataplane_wait_duration

triggers = {
cluster_name = aws_eks_cluster.this[0].name
Copy link
Member Author

Choose a reason for hiding this comment

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

because all of these would trigger a downstream update, they make valid triggers here to ensure the nodegroups/Fargate profiles wait for this duration before proceeding

antonbabenko
antonbabenko reviewed Feb 16, 2023
View reviewed changes
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. I have just a minor suggestion to improve the understandability of it.

node_groups.tf Show resolved Hide resolved
@bryantbiggs bryantbiggs merged commit 78027f3 into terraform-aws-modules:master Feb 17, 2023
@bryantbiggs bryantbiggs deleted the feat/addon-before-compute branch February 17, 2023 12:28
antonbabenko pushed a commit that referenced this pull request Feb 17, 2023
@semantic-release-bot
chore(release): version 19.9.0 [skip ci]
60cc6d7 
## [19.9.0](v19.8.0...v19.9.0) (2023-02-17)

### Features

* Add support for enabling addons before data plane compute is created ([#2478](#2478)) ([78027f3](78027f3))
@antonbabenko
Copy link
Member

This PR is included in version 19.9.0 🎉

code-eg
code-eg reviewed Mar 15, 2023
View reviewed changes
node_groups.tf
@@ -19,6 +19,25 @@ locals {
}
}

# This sleep resource is used to provide a timed gap between the cluster creation and the downstream dependencies
Copy link
Contributor

@code-eg code-eg Mar 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is there any documented reason we cannot use a depends_on object on all compute modules referencing aws_eks_addon.before_compute? Is this purely to avoid module dependencies?

If there was an actual dependency in place, there could be additional benefits (e.g. creating the VPC CNI with IRSA and eliminating the need for removing any instance roles after cluster creation). Anecdotally cluster_addon creation is incredibly variable (I've seen it finish in seconds, and sometime in minutes).

The secondary reason I ask is concerns over downstream dependencies on deletion.

@bryantbiggs bryantbiggs mentioned this pull request Mar 19, 2023
Closed
2 tasks
@code-eg code-eg mentioned this pull request Mar 23, 2023
Merged
3 tasks
@bryantbiggs bryantbiggs mentioned this pull request Apr 11, 2023
Closed
@github-actions
Copy link

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 Apr 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@code-eg code-eg code-eg left review comments

@antonbabenko antonbabenko antonbabenko approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for "early" cluster_addons (vpc-cni with ENABLE_PREFIX_DELEGATION)
3 participants
@bryantbiggs @antonbabenko @code-eg