-
Notifications
You must be signed in to change notification settings - Fork 1
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
[IBCDPE-1005] eks developer role #17
Conversation
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.
🔥 LGTM! Going to pre-approve - not sure if @BWMac had some comments!
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 think this is the wrong approach, see my comments in PR Sage-Bionetworks-IT/organizations-infra#1191
@zaro0508 You bring up an excellent point that I didn't think of until you brought this up. I actually don't need to create a new role for one to assume in this case. What I can do instead is to add these existing assumed roles as an IAM access entry to the cluster. This prevents the need to assume another role with these permissions, and instead I can directly grant these permissions like: I will update this PR accordingly |
modules/sage-aws-eks/main.tf
Outdated
} | ||
eks_admin_role_entries = { | ||
for idx, role_arn in zipmap(range(length(local.eks_admin_roles)), local.eks_admin_roles) : | ||
"eks_admin_role_${idx}" => { |
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.
Unfortunately this syntax was a bit complex, in part because:
data.aws_iam_roles.administrator-roles.arns
is a set that needs to be converted to a list for some of these other functions to workzipmap
is used here to allow getting the index in the for loop- The index was added to the for loop in the event we were going to add multiple ARNs for each of these access entries the key would conflict if we didn't have a unique 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.
Great changes and thanks for the review khai!
Problem:
Solution:
Testing: