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

Apply taints using taint block in aws_eks_node_group resource #78

Conversation

cvittoriasona
Copy link
Contributor

what

  • Use the taint block in aws-eks-node-group to apply taints instead of kubelet args now that they're fully supported.
  • Uprev AWS provider requirement to >=3.43 to use the taint block in aws-eks-node-group
  • Adds tests to validate taints are applied to nodegroup successfully.

why

  • Allows var.userdata_override_base64 with var.kubernetes_taints
  • Enforces valid taints as part of tf plan step.
  • Taints show up in the plan rather than obfuscated in the base64 encoded user_data

references

@jamengual
Copy link
Contributor

/test all

@Nuru Nuru added the do not merge Do not merge this PR, doing so would cause problems label Jul 31, 2021
Copy link
Contributor

@Nuru Nuru left a comment

Choose a reason for hiding this comment

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

@cvittoriasona Thank you for this contribution. We appreciate the effort.

This module is not currently accepting PRs due to the master branch containing breaking changes that will be replaced. Until the master branch is fixed, PRs are on hold.

Meanwhile, would you please verify that you can use var.userdata_override_base64 with var.kubernetes_taints. The AWS documentation says:

When specifying an AMI, Amazon EKS doesn't merge any user data. Rather, you're responsible for supplying the required bootstrap commands for nodes to join the cluster. If your nodes fail to join the cluster, the Amazon EKS CreateNodegroup and UpdateNodegroupVersion actions also fail.

When the user supplies userdata or taints, this module specifies a launch template with a custom AMI ID, and currentlyvar.kubernetes_taints is ignored if you use var.userdata_override_base64. The thing is, I am pretty sure that the taint block also works by setting userdata parameters passed to bootstrap.sh so taints will continue to be ignored when you specify var.userdata_override_base64.

So we are probably going to reject this request because it imposes the extra requirement on AWS Terraform provider version for not enough gain.

@cvittoriasona
Copy link
Contributor Author

cvittoriasona commented Aug 2, 2021

@Nuru - fair enough; I only question if specifying kubernetes_taints using this module should cause the module to specify a custom AMI ID and override of the EKS API managed user_data bootstrap script, which changes the behavior of the EKS Managed Node Group, now that taints are directly supported.

This is probably more complex than is absolutely needed for this module, but would be interested in:

  1. Applying labels/taints through EKS API using the labels and taint attribute/block for aws_eks_node_group
  2. Appending to EKS-managed user_data using cloudinit_config per docs. Supports:
    a. EKS-API managed defaults and options, for ex taints/labels, for kubelet
    b. Docker daemon config changes or other user bootstrap script changes that don't touch the kubelet
  3. Complete override of user_data in the launch template. Allows for full customization of the bootstrap script. Similar implementation to what exists.

That said, this change would need to be reworked anyway to support the above.

@Nuru
Copy link
Contributor

Nuru commented Aug 7, 2021

@cvittoriasona worte

@Nuru - fair enough; I only question if specifying kubernetes_taints using this module should cause the module to specify a custom AMI ID and override of the EKS API managed user_data bootstrap script, which changes the behavior of the EKS Managed Node Group, now that taints are directly supported.

This module does not "change the behavior of the EKS Managed Node Group", it mimics it. The user_data this module supplies is a simple modification of what EKS supplies, just adding features. I am pretty sure that all the resource attribute options do is make the same kind of modifications to user data that this module makes. I encourage you to investigate and report back.

@Nuru Nuru mentioned this pull request Aug 29, 2021
@Nuru Nuru closed this in #84 Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Do not merge this PR, doing so would cause problems
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants