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 gitops support for aws-node-termination-handler #1227

Merged
merged 4 commits into from
Dec 3, 2022

Conversation

mikeinton
Copy link
Contributor

@mikeinton mikeinton commented Dec 2, 2022

What does this PR do?

Adds GitOps enablement for AWS Node Termination Handler as per.

More

  • Yes, I have tested the PR using my local account setup (Provide any test evidence report under Additional Notes)
  • Yes, I have added a new example under examples to support my PR
  • Yes, I have created another PR for add-ons under add-ons repo (if applicable)
  • Yes, I have updated the docs for this feature
  • Yes, I ran pre-commit run -a with this PR

Note: Not all the PRs require a new example and/or doc page. In general:

  • Use an existing example when possible to demonstrate a new addons usage
  • A new docs page under docs/add-ons/* is required for new a new addon

For Moderators

  • E2E Test successfully complete before merge?

Additional Notes

Confirmed that main EKS Blueprints modules handle IAM/IRSA for NTH and hands off helm chart deployment to ArgoCD in a test EKS cluster deployed from forked local copies of this repo and eks-blueprints-add-ons.

Also updated the doc for NTH to include that it is intending to be used for EKS clusters with self_managed_node_groups as NTH shouldn't be necessary when using EKS managed_node_groups as per the comments in this issue - aws/aws-node-termination-handler#186

@mikeinton mikeinton temporarily deployed to EKS Blueprints Test December 2, 2022 21:45 Inactive
@bryantbiggs bryantbiggs changed the title add gitops enablement for aws-node-termination-handler feat: Add gitops support for aws-node-termination-handler Dec 3, 2022
@mikeinton mikeinton temporarily deployed to EKS Blueprints Test December 3, 2022 12:34 Inactive
@bryantbiggs bryantbiggs temporarily deployed to EKS Blueprints Test December 3, 2022 12:45 Inactive
@bryantbiggs bryantbiggs temporarily deployed to EKS Blueprints Test December 3, 2022 12:52 Inactive
@bryantbiggs bryantbiggs merged commit 0c56923 into aws-ia:main Dec 3, 2022
@mikeinton
Copy link
Contributor Author

Thanks for merging this @bryantbiggs.

Noticed one problem here, not sure how the team wants to handle this issue.

│ Error: Invalid index
│
│   on .terraform/modules/eks_blueprints_kubernetes_addons/modules/kubernetes-addons/locals.tf line 14, in locals:
│   14:     awsNodeTerminationHandler = var.enable_aws_node_termination_handler ? module.aws_node_termination_handler[0].argocd_gitops_config : null
│     ├────────────────
│     │ module.aws_node_termination_handler is empty tuple
│
│ The given key does not identify an element in this collection value: the collection has no elements.
╵
make: *** [plan] Error 1

This code path is hit under the following conditions.

  • enable_argocd = false
  • argocd_manage_add_ons = false
  • enable_aws_node_termination_handler = true
  • self_managed_node_groups not set
  • auto_scaling_group_names = module.eks_blueprints.self_managed_node_group_autoscaling_groups not set

I did not encounter this issue from my initial testing as my clusters are all using enable_argocd = true. Today is the first time that I'm testing a new EKS cluster build with no ArgoCD/GitOps.

@oc-christopher-billett
Copy link

Thanks for merging this @bryantbiggs.

Noticed one problem here, not sure how the team wants to handle this issue.

│ Error: Invalid index
│
│   on .terraform/modules/eks_blueprints_kubernetes_addons/modules/kubernetes-addons/locals.tf line 14, in locals:
│   14:     awsNodeTerminationHandler = var.enable_aws_node_termination_handler ? module.aws_node_termination_handler[0].argocd_gitops_config : null
│     ├────────────────
│     │ module.aws_node_termination_handler is empty tuple
│
│ The given key does not identify an element in this collection value: the collection has no elements.
╵
make: *** [plan] Error 1

This code path is hit under the following conditions.

  • enable_argocd = false
  • argocd_manage_add_ons = false
  • enable_aws_node_termination_handler = true
  • self_managed_node_groups not set
  • auto_scaling_group_names = module.eks_blueprints.self_managed_node_group_autoscaling_groups not set

I did not encounter this issue from my initial testing as my clusters are all using enable_argocd = true. Today is the first time that I'm testing a new EKS cluster build with no ArgoCD/GitOps.

Experiencing the same issue, but we are using enable_argocd = true

@mikeinton
Copy link
Contributor Author

@oc-christopher-billett interesting, I've only seen this error when enable_argocd = false. Are you using self managed node groups or managed node groups?

@oc-christopher-billett
Copy link

@oc-christopher-billett interesting, I've only seen this error when enable_argocd = false. Are you using self managed node groups or managed node groups?

Managed Node Groups

@mikeinton
Copy link
Contributor Author

@oc-christopher-billett see the doc link that was also updated in this PR - https://github.com/aws-ia/terraform-aws-eks-blueprints/blob/main/docs/add-ons/aws-node-termination-handler.md#gitops-configuration

As per aws/aws-node-termination-handler#186, Node Termination Handler isn't required for Managed Node Groups.

If you remove enable_aws_node_termination_handler = true or set it to false you should be good to go.

@oc-christopher-billett
Copy link

Thanks I did that anyway to carry on

allamand pushed a commit to allamand/terraform-aws-eks-blueprints that referenced this pull request Dec 15, 2022
)

Co-authored-by: Bryant Biggs <bryantbiggs@gmail.com>
Resolves undefined
allamand pushed a commit to allamand/terraform-aws-eks-blueprints that referenced this pull request Jan 10, 2023
)

Co-authored-by: Bryant Biggs <bryantbiggs@gmail.com>
Resolves undefined
@mk2134226
Copy link

HOw to fix this bug ? i am not using argo cd but want to use node termination handler

@mikeinton
Copy link
Contributor Author

@mk2134226 are you using self_managed_node_groups? Please see the add-on docs here on why NTH is only applicable if you're using self_managed_node_groups.

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.

AWS Node Termination Handler - Support for ArgoCD Gitops
4 participants