Add support for AWS Managed NodeGroups #280

metral · 2019-11-26T22:49:03Z

Proposed changes

feat(nodes): add eks.createManagedNodeGroup() to create AWS Managed Node Groups.

PR includes new examples/test: examples/managed-nodegroups.

Support Details:

Parity compared to self-managed node groups that use CloudFormation Stacks and ASGs.

Security Groups
- Amazon EKS clusters beginning with Kubernetes version 1.14 and platform version eks.3 create a cluster security group as part of cluster creation (or when a cluster is upgraded to this Kubernetes version and platform version). This security group is designed to allow all traffic from the control plane and managed node groups to flow freely between each other
- Amazon EKS managed node groups are automatically configured to use the cluster security group.
- Users cannot provide their own security group, or ingress rules for use with the cluster
- Users can only provide their own security group for remote configuration for Node SSH access.
SSH Key Pair
- Only key pair name can be provided.
Cluster Attributes
- Only k8s labels and AWS tags on the managed node groups are supported.
- kubelet-extra-args is not supported.
  - Taints are not supported.
- Node Updates
  
  Node updates and terminations gracefully drain nodes to ensure that your applications stay available
Instance Type
- Terraform takes instance_types (plural) to be a set, but EKS accepts a single, string value.
  - The Pulumi support is mapped to a pulumi.Output<string>, which is inline with EKS support
  - The variable in Pulumi is instanceTypes (plural per the TF provider), even though its value is singular.
AMI ID
- Instances in a managed node group use the latest version of the Amazon EKS-optimized Amazon Linux 2 AMI for its cluster's Kubernetes version. You can choose between standard and GPU variants of the Amazon EKS-optimized Amazon Linux 2 AMI.

References:

Related issues (optional)

Closes #278

lukehoban

Awesome to see this! A few questions on little details. The one larger question - which I think I know the answer to but would love to hear your thoughts - do we need the createManagedNodeGroup instead of just allowing folks to use aws.eks.NodeGroup directly instead? What benefits do uses get from having the API available here? And are there enough benefits to non-managed NodeGroups that you expect we’ll still want them long term here long term?

nodejs/eks/nodegroup.ts

metral · 2019-11-27T06:28:36Z

The one larger question - which I think I know the answer to but would love to hear your thoughts - do we need the createManagedNodeGroup instead of just allowing folks to use aws.eks.NodeGroup directly instead? What benefits do uses get from having the API available here?

Great point, I'll refactor.

And are there enough benefits to non-managed NodeGroups that you expect we’ll still want them here long term?

I believe we should keep self-managed NodeGroups in maintenance mode for now, and reassess later. Keeping it around allows:

Existing clusters to have a path to use the AWS managed node groups if necessary, and
If the cluster needs maximum configuration, this is the option best suited.

As the Support Details in the OP note, some users may want specificity not yet supported in AWS managed node groups, e.g.: kubelet-extra-args, Taints, custom security groups, and AMI version pinning.

stack72

I haven’t looked at backwards compatibility but I love the simplicity of the API in creating the managedNodeGroup - nice work here!

metral · 2019-11-27T19:38:19Z

Instance Type

TF allows instance_types (plural) to be a set, but EKS appears to only accept a single, string value.

The Pulumi support is mapped to a pulumi.Output<string>, which is inline with EKS support

The variable in Pulumi is instanceTypes (plural per the TF provider), even though its value is singular.

^ Thoughts on support details from OP @stack72 ?

metral · 2019-11-28T03:38:28Z

@joeduffy @lukehoban @stack72

CI is currently broken, but I know where the fix is: the k8s test framework. I have to update it to account for managed nodegroups in addition to CF node groups.

PTAL and review.

stack72 · 2019-11-28T09:37:21Z

Instance Type

TF allows instance_types (plural) to be a set, but EKS appears to only accept a single, string value.

The Pulumi support is mapped to a pulumi.Output<string>, which is inline with EKS support

The variable in Pulumi is instanceTypes (plural per the TF provider), even though its value is singular.

^ Thoughts on support details from OP @stack72 ?

@metral so the AWS CLI actually uses InstanceTypes as well - I think this could be for an upcoming feature - I think we should keep that

Set defaults where possible: - clusterName - scalingConfig - subnets

metral · 2019-11-29T21:18:40Z

CI is ✅

PTAL @lukehoban @joeduffy

nodejs/eks/cluster.ts

nodejs/eks/examples/managed-nodegroups/iam.ts

nodejs/eks/nodegroup.ts

nodejs/eks/cluster.ts

nodejs/eks/examples/managed-nodegroups/iam.ts

metral · 2019-12-03T02:42:12Z

I've addressed feedback where possible, but have not yet identified a path forward for how to manage the aws-auth configmap between self-managed and managed node groups.

To summarize what we're working with:

With self-managed node groups, we will always need to create the configmap as AWS does not create it.
With managed node groups, IME, only the first nodegroup to connect to the cluster gets its role mapped into the aws-auth configmap, which gets automatically created by AWS. This is not documented anywhere that I've found on TF or AWS.
Any subsequent managed node groups that are created after the first one is up will not be automatically added to the configmap. It appears AWS does not handle this scenario.
- The proceeding managed node groups register with the cluster, but then become NotReady shortly after, and the console shows access denied for the nodes to join the cluster.
The TF provider and current AWS API does not seem to take into account user and role mappings for the configmap. The only reason we know the aws-auth configmap gets auto-created and populated with the first managed node group's IAM role mapping is because the creation of the eksNodeAccess configmap errors with an "existing resource" error when we try creating it && using managed node groups simultaneously.
Given that we don't have a kubectl apply or a get & patch approach (Allow a model similar to kubectl apply -f pulumi-kubernetes#264) in the event of an existing configmap, and that we encounter errors removing the eksNodeAccess for existing node group clusters, we must conditionally create or apply the aws-auth configmap based on whether its self or managed node groups. Not to mention, if the user has both self-managed && managed node groups in the same cluster.

The only "clean" way I see of creating the configmap is that we must get & patch / apply the user args for the configmap w/ any data already in the existing configmap, or create the configmap if it does not exist as usual. But this relies on pulumi/pulumi-kubernetes#264 or some temp work around, and I'm not sure how to proceed.

Thoughts @lukehoban @lblackstone?

lukehoban · 2019-12-03T17:13:02Z

I believe that we should be able to keep the original model of creating the aws-auth ConfigMap ourselves for both self-managed and managed NodeGroups. We just need to ensure we have a dependency of the aws.eks.NodeGroup on the ConfigMap to ensure that we always create the ConfigMap prior to AWS creating it itself.

metral · 2019-12-03T17:33:42Z

Feedback has been addressed and CI is currently running with changes to the nodegroup to now depend on the original eksNodeAccess configmap managed by pulumi.

This is working as expected on the managed-nodegroups example/test, along with an existing basic cluster and existing nodegroup cluster.

@lukehoban @lblackstone PTAL

lukehoban

LGTM 🎉

metral force-pushed the metral/managed-ng branch 2 times, most recently from 72a742b to 272178a Compare November 26, 2019 23:10

dependencies: update pulumi/aws to include eks.NodeGroup

be3f3d8

metral force-pushed the metral/managed-ng branch 4 times, most recently from 248fad9 to 761a64e Compare November 26, 2019 23:19

metral changed the title ~~Add support for AWS Managed NodeGroups~~ Add support for AWS Managed NodeGroups with examples/tests Nov 27, 2019

metral changed the title ~~Add support for AWS Managed NodeGroups with examples/tests~~ Add support for AWS Managed NodeGroups Nov 27, 2019

lukehoban reviewed Nov 27, 2019

View reviewed changes

nodejs/eks/nodegroup.ts Outdated Show resolved Hide resolved

nodejs/eks/nodegroup.ts Outdated Show resolved Hide resolved

nodejs/eks/nodegroup.ts Outdated Show resolved Hide resolved

nodejs/eks/nodegroup.ts Outdated Show resolved Hide resolved

lukehoban requested a review from stack72 November 27, 2019 06:00

metral force-pushed the metral/managed-ng branch 5 times, most recently from 17e343f to 236346d Compare November 27, 2019 08:52

stack72 approved these changes Nov 27, 2019

View reviewed changes

metral force-pushed the metral/managed-ng branch from 236346d to e287052 Compare November 28, 2019 03:20

metral marked this pull request as ready for review November 28, 2019 03:20

metral force-pushed the metral/managed-ng branch 3 times, most recently from e815fe7 to c8112c8 Compare November 28, 2019 03:30

metral requested a review from lukehoban November 28, 2019 03:38

metral force-pushed the metral/managed-ng branch from c8112c8 to 6c5f7e7 Compare November 29, 2019 19:32

feat(nodes): add eks.createManagedNodeGroup()

41fc94a

Set defaults where possible: - clusterName - scalingConfig - subnets

metral force-pushed the metral/managed-ng branch from 6c5f7e7 to 41fc94a Compare November 29, 2019 19:40

lukehoban reviewed Dec 2, 2019

View reviewed changes

lblackstone reviewed Dec 2, 2019

View reviewed changes

nodejs/eks/cluster.ts Outdated Show resolved Hide resolved

nodejs/eks/cluster.ts Outdated Show resolved Hide resolved

nodejs/eks/examples/managed-nodegroups/iam.ts Show resolved Hide resolved

metral force-pushed the metral/managed-ng branch from 4f7730a to 50cd020 Compare December 3, 2019 17:28

Address PR feedback and make managed nodegroup depend on aws-auth

7f06aa6

metral force-pushed the metral/managed-ng branch from 50cd020 to 7f06aa6 Compare December 3, 2019 17:32

lblackstone approved these changes Dec 3, 2019

View reviewed changes

lukehoban approved these changes Dec 3, 2019

View reviewed changes

metral merged commit 27c0714 into master Dec 4, 2019

pulumi-bot deleted the metral/managed-ng branch December 4, 2019 00:01

RichardWLaub mentioned this pull request Mar 23, 2020

Getting configmaps "aws-auth" already exists on new cluster creation #355

Closed

metral mentioned this pull request Apr 13, 2020

ManagedNodeGroupOptions doesn't allow SecurityGroup configuration #371

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for AWS Managed NodeGroups #280

Add support for AWS Managed NodeGroups #280

metral commented Nov 26, 2019 •

edited

Loading

lukehoban left a comment

metral commented Nov 27, 2019

stack72 left a comment

metral commented Nov 27, 2019 •

edited

Loading

metral commented Nov 28, 2019 •

edited

Loading

stack72 commented Nov 28, 2019 •

edited by metral

Loading

metral commented Nov 29, 2019

metral commented Dec 3, 2019 •

edited

Loading

lukehoban commented Dec 3, 2019

metral commented Dec 3, 2019 •

edited

Loading

lukehoban left a comment

Add support for AWS Managed NodeGroups #280

Add support for AWS Managed NodeGroups #280

Conversation

metral commented Nov 26, 2019 • edited Loading

Proposed changes

Related issues (optional)

lukehoban left a comment

Choose a reason for hiding this comment

metral commented Nov 27, 2019

stack72 left a comment

Choose a reason for hiding this comment

metral commented Nov 27, 2019 • edited Loading

metral commented Nov 28, 2019 • edited Loading

stack72 commented Nov 28, 2019 • edited by metral Loading

metral commented Nov 29, 2019

metral commented Dec 3, 2019 • edited Loading

lukehoban commented Dec 3, 2019

metral commented Dec 3, 2019 • edited Loading

lukehoban left a comment

Choose a reason for hiding this comment

metral commented Nov 26, 2019 •

edited

Loading

metral commented Nov 27, 2019 •

edited

Loading

metral commented Nov 28, 2019 •

edited

Loading

stack72 commented Nov 28, 2019 •

edited by metral

Loading

metral commented Dec 3, 2019 •

edited

Loading

metral commented Dec 3, 2019 •

edited

Loading