-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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 additional permissions to Karpenter EKS IRSA role for native node termination handling support #304
feat: Add additional permissions to Karpenter EKS IRSA role for native node termination handling support #304
Conversation
Signed-off-by: Fernando Miguel <github@FernandoMiguel.net>
Hey @bryantbiggs, any plans when it got merged since Karpenter 0.19.0 was released yesterday and this can be useful now. |
I have to double check, since I believe more statements were added since I open this PR. |
Signed-off-by: Fernando Miguel <github@FernandoMiguel.net>
open for review |
@@ -583,6 +584,19 @@ data "aws_iam_policy_document" "karpenter_controller" { | |||
actions = ["iam:PassRole"] | |||
resources = var.karpenter_controller_node_iam_role_arns | |||
} | |||
|
|||
statement { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets make this dynamic if an SQS queue arn is provided
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bryantbiggs what do you mean with dynamic?
i've already created a var for karpenter_sqs_queue_arn
and a local
statement { | ||
sid = "KarpenterEventPolicySQS" | ||
effect = "Allow" | ||
resources = ["arn:aws:sqs:${data.aws_region.current.name}:${local.account_id}:${var.karpenter_controller_cluster_id}"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets add a variable for this karpenter_sqs_queue_arn
since we are not guaranteed that the queue name will always be the cluster name
looks good - two small details to address and we should be able to merge, thanks again @FernandoMiguel 🎉 note - this is not the complete picture for Karpenter starting with v0.19. Karpenter is evolving into data plane management and now more resources are coming into play. With that, I am looking to add a sub-module to handle nearly all things related to Karpenter infrastructure (note - it will not deploy the controller, just provision the necessary AWS resources). You can follow along here https://github.com/terraform-aws-modules/terraform-aws-eks/compare/master...bryantbiggs:terraform-aws-eks:feat/karpenter-sub-module?expand=1 - its still very early and I need to update the example but I think this is more like what we'll need for Karpenter going forward |
let me do another round of changes. thanks for the review |
Signed-off-by: Fernando Miguel <github@FernandoMiguel.net>
waiting on a review. |
## [5.8.0](v5.7.0...v5.8.0) (2022-11-21) ### Features * Add additional permissions to Karpenter EKS IRSA role for native node termination handling support ([#304](#304)) ([d6865d2](d6865d2))
This PR is included in version 5.8.0 🎉 |
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. |
Signed-off-by: Fernando Miguel github@FernandoMiguel.net
Description
The next version of Karpenter adds support for NTH: https://github.com/aws/karpenter/pull/2546/files
Motivation and Context
This PR adds the IAM changes to this module.
Breaking Changes
How Has This Been Tested?
examples/*
to demonstrate and validate my change(s)examples/*
projectspre-commit run -a
on my pull request