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

fix: Sub-modules output the correct eks worker iam arn when workers utilize custom iam role #1912

Merged
merged 3 commits into from
Mar 17, 2022

Conversation

kongdewen
Copy link
Contributor

Description

When sub-module generate outputs, it needs to consider if the worker instance is using custom iam role, if so, it should directly output the variable associated with it. otherwise, it will return empty string cause damage to the cluster if not carefully use these outputs.

Motivation and Context

when we patch the aws_auth config following

# we have to combine the configmap created by the eks module with the externally created node group/profile sub-modules
aws_auth_configmap_yaml = <<-EOT
${chomp(module.eks.aws_auth_configmap_yaml)}
- rolearn: ${module.eks_managed_node_group.iam_role_arn}
username: system:node:{{EC2PrivateDNSName}}
groups:
- system:bootstrappers
- system:nodes
- rolearn: ${module.self_managed_node_group.iam_role_arn}
username: system:node:{{EC2PrivateDNSName}}
groups:
- system:bootstrappers
- system:nodes
- rolearn: ${module.fargate_profile.fargate_profile_pod_execution_role_arn}
username: system:node:{{SessionName}}
groups:
- system:bootstrappers
- system:nodes
- system:node-proxier
EOT
}
resource "null_resource" "patch" {
triggers = {
kubeconfig = base64encode(local.kubeconfig)
cmd_patch = "kubectl patch configmap/aws-auth --patch \"${local.aws_auth_configmap_yaml}\" -n kube-system --kubeconfig <(echo $KUBECONFIG | base64 --decode)"
}
provisioner "local-exec" {
interpreter = ["/bin/bash", "-c"]
environment = {
KUBECONFIG = self.triggers.kubeconfig
}
command = self.triggers.cmd_patch
}
}
.

we found out that rolearn from module.eks.aws_auth_configmap_yaml was empty

 kubectl -n kube-system describe configmap/aws-auth
Name:         aws-auth
Namespace:    kube-system
Labels:       <none>
Annotations:  <none>

Data
====
mapRoles:
----
- rolearn:
  username: system:node:{{EC2PrivateDNSName}}
  groups:
    - system:bootstrappers
    - system:nodes
- rolearn: {custom_iam}
  username: {custom_iam}
  groups:
    - xxxx:xxxx

Events:  <none>

Breaking Changes

n/a

How Has This Been Tested?

After the update, the issue we saw has been fixed(we only use EKS managed node group). so the self managed nodegroup and fargate portion is unverified yet, but since it's an easy fix, kinda confident it will work.

@kongdewen kongdewen changed the title fix: output the correct eks worker iam arn when workers utilize custom iam role fix: Sub-modules output the correct eks worker iam arn when workers utilize custom iam role Mar 2, 2022
outputs.tf Outdated
@@ -171,8 +171,8 @@ output "aws_auth_configmap_yaml" {
value = templatefile("${path.module}/templates/aws_auth_cm.tpl",
{
eks_managed_role_arns = [for group in module.eks_managed_node_group : group.iam_role_arn]
self_managed_role_arns = [for group in module.self_managed_node_group : group.iam_role_arn if group.platform != "windows"]
win32_self_managed_role_arns = [for group in module.self_managed_node_group : group.iam_role_arn if group.platform == "windows"]
self_managed_role_arns = [for group in module.self_managed_node_group : group.iam_instance_profile_arn if group.platform != "windows"]
Copy link
Member

Choose a reason for hiding this comment

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

the other changes above look ok (need to test still), but I don't think this is correct. I do believe we want to use the role that the instance profile utilizes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @bryantbiggs for the quick follow up. I think iam_instance_profile_arn is the actual the iam instance profile? https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/modules/self-managed-node-group/main.tf#L149-L151

Copy link
Member

Choose a reason for hiding this comment

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

Instance profile is not what is used for access within the cluster, its just a construct used by the EC2 service in order to assume the role.

This has been like this for quite some time so I am highly confident it is correct as is otherwise we'd have a large number of users complaining that their self-managed node groups fail to join the cluster

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @bryantbiggs for the clarification! revert this part of change

Instance profile is not what is used for access within the cluster, its just a construct used by the EC2 service in order to assume the role.
Copy link
Member

@bryantbiggs bryantbiggs left a comment

Choose a reason for hiding this comment

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

thanks for the PR @kongdewen - good to go @antonbabenko 👍🏽

@antonbabenko antonbabenko merged commit 06a3469 into terraform-aws-modules:master Mar 17, 2022
antonbabenko pushed a commit that referenced this pull request Mar 17, 2022
### [18.10.2](v18.10.1...v18.10.2) (2022-03-17)

### Bug Fixes

* Sub-modules output the correct eks worker iam arn when workers utilize custom iam role ([#1912](#1912)) ([06a3469](06a3469))
@antonbabenko
Copy link
Member

This PR is included in version 18.10.2 🎉

thunderbird86 pushed a commit to thunderbird86/terraform-aws-eks that referenced this pull request Mar 22, 2022
spr-mweber3 pushed a commit to spring-media/terraform-aws-eks that referenced this pull request Apr 26, 2022
spr-mweber3 pushed a commit to spring-media/terraform-aws-eks that referenced this pull request Apr 26, 2022
### [18.10.2](terraform-aws-modules/terraform-aws-eks@v18.10.1...v18.10.2) (2022-03-17)

### Bug Fixes

* Sub-modules output the correct eks worker iam arn when workers utilize custom iam role ([terraform-aws-modules#1912](terraform-aws-modules#1912)) ([06a3469](terraform-aws-modules@06a3469))
it-without-politics pushed a commit to it-without-politics/terraform-aws-eks that referenced this pull request May 23, 2022
it-without-politics pushed a commit to it-without-politics/terraform-aws-eks that referenced this pull request May 23, 2022
### [18.10.2](terraform-aws-modules/terraform-aws-eks@v18.10.1...v18.10.2) (2022-03-17)

### Bug Fixes

* Sub-modules output the correct eks worker iam arn when workers utilize custom iam role ([terraform-aws-modules#1912](terraform-aws-modules#1912)) ([06a3469](terraform-aws-modules@06a3469))
baibailiha added a commit to baibailiha/terraform-aws-eks that referenced this pull request Sep 13, 2022
### [18.10.2](terraform-aws-modules/terraform-aws-eks@v18.10.1...v18.10.2) (2022-03-17)

### Bug Fixes

* Sub-modules output the correct eks worker iam arn when workers utilize custom iam role ([#1912](terraform-aws-modules/terraform-aws-eks#1912)) ([69711d2](terraform-aws-modules/terraform-aws-eks@69711d2))
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants