Skip to content

Conversation

@wbertelsen
Copy link
Contributor

@wbertelsen wbertelsen commented Dec 11, 2019

PR o'clock

Description

This change adds the IAM role used for each managed node group to the
aws-auth config map. This fixes an issue where managed nodes can not
access the EKS kubernetes API server.

This should fix #624

Checklist

This change adds the IAM role used for each managed node group to the
aws-auth config map. This fixes an issue where managed nodes could not
access the EKS kubernetes API server.
@bjmask
Copy link
Contributor

bjmask commented Dec 17, 2019

How can I help this get approved?

@max-rocket-internet
Copy link
Contributor

I'm still not sure how to integrate the managed node groups into this module.

See here for some background: #635

The thing is, we have this now:

  • Worker groups with LC
  • Work groups with LT
  • Managed node groups
  • Fargate (coming soon)

And I have no clusters using Managed node groups to test with.

@wbertelsen
Copy link
Contributor Author

@max-rocket-internet I've been watching the discussion in #635 and I buy the idea that the worker/node groups could be submodules.

However, managed node groups are broken on master without this patch, so I think we should work with other testers to get this merged, or revert the original node group PR (#602) as broken.

I've tested this myself on a real cluster, but obviously there should be another set of eyes on it.

@andres-de-castro: Are you able to test this out on a cluster and report back if it works?
If you're not already aware, you can specify any git ref when sourcing the module:

 source          = "github.com/wbertelsen/terraform-aws-eks?ref=fix-node-group-aws-auth"

@TBeijen
Copy link

TBeijen commented Dec 20, 2019

I'll take a look. Was trying out latest master and indeed noticed a second plan attempting to delete the managed-nodes from aws-auth. Probably added by AWS EKS itself (similar to how it updates VPC & subnet tags)

@max-rocket-internet
Copy link
Contributor

@TBeijen that would be great. I think then we merge this and make a new release with the node group stuff included.

@TBeijen
Copy link

TBeijen commented Dec 20, 2019

Ok, ran some tests:

  • Added managed node group to a cluster with 1 launch-template based worker group using latest master 583c32d

  • Noticed 2nd plan would remove the rolearn: arn:aws:iam::NNNNNN:role/apps-1-managed-node-groups from mapRoles

  • Switched to github.com/wbertelsen/terraform-aws-eks?ref=fix-node-group-aws-auth, ran plan & apply. Observed that:

    • Plan showed preserving the aws-auth entry
    • aws-auth values are now quoted. Looks like valid yaml so...
    • After apply I wasn't locked out of my cluster. Nodes from both launch template and managed node group reported as Ready.
  • Removed the node_groups block from TF code (testing if it renders fine without any managed node groups):

    • Plan and apply looked and ran ok.
    • After apply I wasn't locked out of my cluster. Nodes reported Ready, aws-auth looks good.

So, based on the above I'd say this fixes aws-auth as it's expected to do.

Copy link
Contributor

@max-rocket-internet max-rocket-internet left a comment

Choose a reason for hiding this comment

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

Thank you @wbertelsen

@max-rocket-internet max-rocket-internet merged commit bad9604 into terraform-aws-modules:master Dec 20, 2019
@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 20, 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.

Terraform apply breaks communications between workers and masters

4 participants