feat(aws-eks): Add L2 Construct for Amazon EKS clusters and worker nodes

[yandy-r]

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

PR Details

This references issue #990 for having native L2 constructs for EKS clusters and worker nodes.

With this very simply put the user can create EKS Control Plane Clusters and worker nodes. Right now there's no abstraction of the cloudformation construct and a user needs to quite a bit of manual input.

Adding worker nodes to the cluster this way is automated and all roles and recommended security groups created.

Working example

const ENV = "dev";
const app = new cdk.App();

const networkStack = new cdk.Stack(app, "Network");

const vpc = new ec2.VpcNetwork(networkStack, "VPC", {
  cidr: "10.244.0.0/16",
  maxAZs: 3,
  natGateways: 0,
  subnetConfiguration: [
    {
      name: "pub",
      cidrMask: 24,
      subnetType: ec2.SubnetType.Public
    }
  ],
  tags: {
    env: `${ENV}`
  }
});
const vpcExport = vpc.export();

const clusterStack = new cdk.Stack(app, "Cluster");

const clusterVpc = ec2.VpcNetworkRef.import(
  clusterStack,
  "ClusterVpc",
  vpcExport
);
const cluster = new eks.Cluster(clusterStack, "Cluster", {
  vpc: clusterVpc,
  vpcPlacement: {
    subnetsToUse: ec2.SubnetType.Public
  }
});
cluster.connections.allowFromAnyIPv4(new ec2.TcpPort(443));

const grp1 = new eks.Nodes(clusterStack, "NodeGroup1", {
  vpc: clusterVpc,
  cluster: cluster,
  minNodes: 3,
  maxNodes: 6,
  sshKeyName: "aws-dev-key"
});
grp1.nodeGroup.connections.allowFromAnyIPv4(new ec2.TcpPort(22));

More than one worker node group can be added and tracked with an Array or worker nodes autoscaling groups. Though nothing's really done with this yet, other than exposing it and ability to access via.. grp.nodeGroups[0].connections.... or similar.

What needs to be added in the future is a way to get the kubeconfig information at deployment time. There was a conversation on gitter about that, started by me and someone mentioned a custom resource could be used.

I will look into options for future release because most of the information for kubeconfig creation is only available at deployment time. Right now there's one more step that needs to be manually implemented for the control plane to see the nodes.

That's patching the storage and creating the auth config-map.

The Contributing Guide

Mentions adding the changelog to an Unreleased section, though I didn't see that in the file and did not want to add if not required. If required, I can happily edit that file.

There are quite a few changed files as well and new ones since it's a brand new feature. Unsure if this was better to have been broken out into multiple PRs.

[ijcd]

Some quick feedback. This structure is missing info on T3 insances, which leads to max being undefined in the instance user-data. Seems to prevent nodes from registering.

export const maxPods = Object.freeze(

#!/bin/bash
set -o xtrace
/etc/eks/bootstrap.sh democlusterClusterA-oChms63INSG8 --use-max-pods undefined

Probably need some typed checking on the hash lookup to prevent misses like this from sneaking through.

[ijcd]

I also ended up adding a securityGroups option to IClusterProps so we could use some imported security groups.

  securityGroups?: [ec2.SecurityGroupRef]

    const securityGroupIds = new Array(this.securityGroupId)
    if (props.securityGroups) {
      for (var sg of props.securityGroups) {
        securityGroupIds.push(sg.securityGroupId)
      }
    }

[yandy-r]

@ijcd Thanks for your comments and trying it out.

For the first one, I definitely need to do some checking or create data structure specific for EKS since I'm using the included InstanceType and not all are supported yet, such as T3 types.

For the second comment, I have a separate branch (no PR yet) as I wanted to start getting this and hopefully clean up both. In that branch, I have multi-stack working.. meaning exporting of properties actually work with a full example. That branch also implements the IConnectable interface for both cluster and worker nodes, that way managing security groups is much easier.

Now with v0.15.0 and the IConnectable implementation being an array, adding security groups is much improved. With that, there's no reason to expose or provide a securityGroup(s) property. I have not submitted a PR for that since I want to make sure the low-hanging stuff gets cleaned out first.

Thanks again and keep it coming.

[ijcd]

@IPyandy Is the multi-stack branch further along? I can give that a go. Right now my nodes are not finding the masters for some reason. Trying to debug why.

[yandy-r]

@IPyandy Is the multi-stack branch further along? I can give that a go. Right now my nodes are not finding the masters for some reason. Trying to debug why.

So the process for after the odes are created are still a bit manual, since the values are only coerced at deploy time, there was no easy way to generate a script to deploy.

I'm working on a custom resource to take care of this, but it's not there yet and neither branch has that. Right now, you have to do this..

Step one (get the kubeconfig)

aws eks update-kubeconfig --name YOUR-CLUSTER-NAME

or specify a file to create

aws eks update-kubeconfig --kubeconfig ~/.kube/aws-dev-eks.yaml --name YOUR-CLUSTER-NAME

Step two (patch GP2)

cat <<EOF | kubectl apply -f -
kind: StorageClass
apiVersion: storage.k8s.io/v1
metadata:
  name: gp2
provisioner: kubernetes.io/aws-ebs
parameters:
  type: gp2
reclaimPolicy: Retain
mountOptions:
  - debug
EOF

Step three (create the auth configmap for the nodes

Make sure you replace the - rolearn with the one created when nodes were deployed.

cat <<EOF | kubectl apply -f -
apiVersion: v1
kind: ConfigMap
metadata:
  name: aws-auth
  namespace: kube-system
data:
  mapRoles: |
    - rolearn: <<ADD WITH NODE ROLE ARN NOT! THE INSTANCE ARN>>
      username: system:node:{{EC2PrivateDNSName}}
      groups:
        - system:bootstrappers
        - system:nodes
EOF

[ijcd]

Oh, interesting. I hadn't realized that. I wonder how the standard CloudFormation templates work? Have you compared your approach to this? (https://aws.amazon.com/about-aws/whats-new/2018/08/new-amazon-eks-optimized-ami-and-cloudformation-template-for-worker-node-provisioning/) I'll study both and see what I can do here.

[yandy-r]

Oh, interesting. I hadn't realized that. I wonder how the standard CloudFormation templates work? Have you compared your approach to this? (https://aws.amazon.com/about-aws/whats-new/2018/08/new-amazon-eks-optimized-ami-and-cloudformation-template-for-worker-node-provisioning/) I'll study both and see what I can do here.

They're two separate things, if you were to deploy the node template from that link, you'd still need to do the above process. The process run by CDK is essentially just automating that template with some added features, the problem comes after the nodes are deployed.

It has to do more with extracting the roleArn and passing that to some kind of script or temp instance to then run the kubectl commands and authenticate the nodes with the cluster. The roleArn is not available until it's deployed, so it needs to be coerced during or after.

Unless you manually create it and pas sit as a reference, but that's counter productive.

[rix0rrr]

The context in this file should not be committed.

I know, it's a pain.

[yandy-r]

crap, yea will fix.

[rix0rrr]

I think it would be good to put this information in the @aws-cdk/aws-ec2 library. This might come in handy to multiple construct libraries.

[rix0rrr]

us-east-2 is missing.

[rix0rrr]

Wouldn't it make more sense to order the map { region -> type -> AMI }?

[yandy-r]

I'll add us-east-2, wasn't available when this was submitted and and the next push should have it, just been tiddying up, but it's queue.

curious; what advantage do you forsee by doing it that way? the region->type->ami map way that is? I'm not opposed, just wondering.

[rix0rrr]

Why not use the enum from the aws-autoscaling package?

[yandy-r]

Thanks for pointing that out, I did this when first started the package to make it "easier" and forgot to clean up, I'll fix.

[rix0rrr]

Is public subnets a desired default? Seems like private is a safer default, and people can opt to use public if they're explicitly placing internet-facing services.

But more generally, it looks to me like this is used to create an AutoScalingGroup. The strategy used in ECS, to add arbitrary sets of "capacity" to a cluster is more flexible in the long run:

https://github.com/awslabs/aws-cdk/blob/master/packages/%40aws-cdk/aws-ecs/lib/cluster.ts#L91

[yandy-r]

The reason I chose public (with least privilege access) was for the cost, as private subnets need the nat for cluster communication, updates and such. But it's not a difficult change, should be flexible.

For the second piece, I wanted to separate control and data plane completely. Since it's possible to keep a "Cluster" up but remove compute capacity at will. Or maybe spin up different instance types, sizes, and families. Maybe spin up a T2 dev worker cluster and a separate production but all tied to the same control plane. I'll take a look at the link, if that's still possible I will rethink it.

Thanks

[rix0rrr]

Does not need to be a visible member.

[rix0rrr]

Does not need to be a visible member.

[rix0rrr]

Does not need to be a visible member.

[rix0rrr]

Don't use map() if you're running it for the side effects. Instead, use forEach():

subnets.forEach(s => subnetIds.push(s.subnetId));

But better yet, separate transformation (subnets to subnetIds) from operation (appending to list):

subnets.push(...subnets.map(s => s.subnetId));

And better better yet, don't do a mutating operation but initialize the list with the values you want directly:

const subnetIds: string[] = subnets.map(s => s.subnetId);

[yandy-r]

Thanks! will do.

[rix0rrr]

Make it possible for users to pass in a role via the props object as well. Probably the final role object is something you want to expose via the public members, so that people can write the following:

resource.grantFrobble(cluster.role);

[yandy-r]

Yup this is actually already there just a separate branch where the "multi-stack" capability also works, I'll merge that part for the next push. Another reason I separated the Nodes and the Cluster and didn't just add capacity was to be able create separate stacks, and destroy them without forcing any type of update on the others.

[moofish32]

A few questions in line with some of the others. Specifically do we need max pods at all in here? What decision are we making from duplicating that list into here, and does the bootstrap script cover it already by default?

[moofish32]

Isn't this defaulted to true in the script? My concern is duplicating the max pod information in two places. The CDK doesn't really need to know this and adding it is another place to maintain it.

[moofish32]

this is not adding a role, it's addingEksNodePermission?

[moofish32]

With VPC we made the user create their own, with ASG we are abstracting some choices. I feel like we should be consistent here. I was thinking either you pass a VPC ref or we create one for you and the same for ASG. I haven't seen the CDK provide helper generates. For example, I could be convinced that a static method existed like defaultEksVpcConfiguration({clusterName: string}) and one for ASG. I don't know how the CDK team thinks about these complex new constructs which consume multiple L2 constructs already.

[moofish32]

Do we really need more than gpuEnabled?: boolean here? Are we expecting many more types?

[moofish32]

I don't see an ability to provide your own AMI ID map. I think that will be needed at release. We will be creating custom AMIs from the packer scripts provided.

I think this really comes to question whether we should enable a BYO ASG instead of duplicating these properties.

[moofish32]

perhaps this is addEksManagedPolicies - I'm open to something better, but we are not adding a role here.

[moofish32]

Cross Stack I think requires an export which means it cannot be done until a stack is deployed. Is there a solution coming or something specific to Security Groups? If we are going to use CrossStackRefs here let's be minimal because if a delete occurs the action will be blocked.

[moofish32]

I prefer to test things in one place. We have a lot of duplicated chunks of CFN in here and if we refactor we'll be changing this in a lot of places. You should be able to just test the part you changed after testing the common stuff in one place.

[moofish32]

why false here? I would think the defaults work for this call and this has to be set to this value.

[moofish32]

I think it's worth adding a test that specifically verifies the required tags are in place. I think you test it in various places, but since it is a unique requirement I think just one specific test for it will help future devs.

[yandy-r]

@rix0rrr @moofish32 Thanks for the comments, the ones not already done, I'll get to them early next week and update this PR then. Going on a weekend trip with the family. Thanks

[sam-goodwin]

Are you still planning on further updates to address the comments? Would like to define a path forward for this change, especially considering the volume of work you've already invested :)

[yandy-r]

@sam-goodwin I was, though was supposed to have a meeting with the open source team to finalize design to avoid the back-and-forth on those details. It never happened, at this point I was waiting on the team to give the Ok to continue. I'd love to finish it and have others assist if needed.

[henrysachs]

Would love to see this merged soon! keep up! :)

[eladb]

@IPyandy thank you so much for this. @rix0rrr have picked it up and we will finalize the work in #1655. Would love your feedback on that PR. Closing for now.