-
-
Notifications
You must be signed in to change notification settings - Fork 4.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
Support custom IAM roles for cluster and workers #338
Support custom IAM roles for cluster and workers #338
Conversation
3efb8a5
to
5380b7c
Compare
Hey @erks ! Thanks for the PR and sorry for the delay. I will review this PR soon. It's quite complex so taking me a little longer to find the time 🙂 |
@max-rocket-internet thanks for taking a look! |
b01d610
to
efa5dc6
Compare
workers.tf
Outdated
@@ -38,7 +38,7 @@ resource "aws_launch_configuration" "workers" { | |||
name_prefix = "${aws_eks_cluster.this.name}-${lookup(var.worker_groups[count.index], "name", count.index)}" | |||
associate_public_ip_address = "${lookup(var.worker_groups[count.index], "public_ip", local.workers_group_defaults["public_ip"])}" | |||
security_groups = ["${local.worker_security_group_id}", "${var.worker_additional_security_group_ids}", "${compact(split(",",lookup(var.worker_groups[count.index],"additional_security_group_ids", local.workers_group_defaults["additional_security_group_ids"])))}"] | |||
iam_instance_profile = "${element(aws_iam_instance_profile.workers.*.id, count.index)}" | |||
iam_instance_profile = "${element(coalescelist(aws_iam_instance_profile.workers.*.id, data.aws_iam_instance_profile.custom_worker_group_iam_instance_profile.*.id), count.index)}" |
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.
I assume this is my own ignorance but in order to get this to work for me I had to change this line to
iam_instance_profile = "${element(coalescelist(aws_iam_instance_profile.workers.*.id, data.aws_iam_instance_profile.custom_worker_group_iam_instance_profile.*.name), count.index)}"
This appears to be in line with description of the parameter in hashicorp's docs.
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.
thanks. fixed.
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.
Downloaded module for testing purposes, was able to successfully spin up EKS clusters with custom IAM permissions using this PR.
just rebased from master again. |
Me too! |
I just rebased this so I can test it now... |
OK, since we have a couple of people who have tested it and I also did, I think we merge 🙂 I do think that we are really approaching the limits of Terraform complexity now. It's quite hard to follow some of this code. I just hope Terraform 0.12 can help claw back some readability and simplicity soon 😅 |
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.
Thank you @erks 💙
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. |
PR o'clock
Description
Fixes #282
Checklist
terraform fmt
andterraform validate
both work from the root andexamples/eks_test_fixture
directories (look in CI for an example)