feat(eks): programmatic definition of kubernetes resources #3510

eladb · 2019-08-01T16:04:07Z

Enable programmatically defining Kubernetes resources and applying them through CloudFormation to the EKS cluster.

This enables the following:

Defining additional IAM roles that can master the cluster.
Programmatically managing IAM mapping through the aws-auth ConfigMap
New nodes can join the cluster automatically as their IAM instance role is auto-mapped
Users can now define Kubernetes resources and applications programmatically.

Sadly, in order for this to be possible, we had to use a custom resource (credits @NetaNir) in order to provision the EKS cluster. See the README file for details on why.

Please read the contribution guidelines and follow the pull-request checklist.

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

This change allows defining arbitrary Kubernetes resources within an EKS cluster.

moofish32 · 2019-08-01T19:35:15Z

A general question here, what is thought process behind kubectl vs using a k8s client via lambda? It feels a little weird using lambda like a CLI runner, but there are some advantages. The same question starts to come with aws eks commands that are not in the SDKs.

packages/@aws-cdk/aws-eks/README.md

packages/@aws-cdk/aws-eks/lib/manifest-resource.ts

moofish32

@eladb

Can you talk about all the YAML parsing work? I didn't really review it, but I'm not sure exactly what drove the creation?

I left a few comments based on a quick pass.

packages/@aws-cdk/aws-eks/README.md

packages/@aws-cdk/aws-eks/lib/aws-auth-mapping.ts

packages/@aws-cdk/aws-eks/README.md

packages/@aws-cdk/aws-eks/lib/cluster-resource.ts

packages/@aws-cdk/aws-eks/lib/cluster.ts

moofish32 · 2019-08-04T01:18:02Z

packages/@aws-cdk/aws-eks/lib/cluster.ts

@@ -273,6 +361,7 @@ export class Cluster extends Resource implements ICluster {

    this.addAutoScalingGroup(asg, {
      maxPods: maxPodsForInstanceType(options.instanceType),
+      mapRole: options.mapRole,
    });

    return asg;


If a user wants to add IAM statements to the role for the worker nodes they would use this return right?

const iamEnabledNodes = eksCluster.addCapacity( ... ) iamEnabledNodes.addToRolePolicy( // special IAM permissions for managing IAM to pods // )

I've seen a lot of patterns where teams run sensitive workloads on a specific node group, that they might have previously run on master nodes.

I think this can be resolved. The ergonomics here are a little awkward for me. Add hands back an entity that you can't access from the class. However, I think the use case is met.

packages/@aws-cdk/aws-eks/lib/manifest-resource.ts

packages/@aws-cdk/aws-eks/lib/manifest-resource/index.py

packages/@aws-cdk/aws-eks/README.md

packages/@aws-cdk/aws-eks/lib/cluster.ts

rix0rrr · 2019-08-05T09:39:06Z

packages/@aws-cdk/aws-eks/lib/kubectl-layer.ts

+    const uniqueId = crypto.createHash('md5').update(this.node.path).digest("hex");
+    const version = props.version || '1.13.7';
+
+    this.stack.templateOptions.transforms = [ 'AWS::Serverless-2016-10-31' ]; // required for AWS::Serverless


This needs a check for existing transforms and conflicts therein.

Ping. This is still a recipe for disaster if people combine with other transforms, or other versions of the Serverless transform.

I'll add some support in Stack for this (stack.addTransform()). Let me open an issue.

packages/@aws-cdk/aws-eks/lib/kubectl-layer.ts

packages/@aws-cdk/aws-eks/lib/manifest-resource/yaml/composer.py

This adds the capability of adding a target to an event rule that belongs to a different account than the rule itself. Required for things like cross-account CodePipelines with source actions triggered by events.

…3487) Our logic for trimming the length of the default IAM policy name was not working, as it wasn't updated when logicalId became a Token rather than a literate string, and so it was never actually triggered (it just checked that the display name of the Token was less than 128 characters, which it always is). The fix is to resolve the logical ID Token before applying the trimming logic. Fixes #3402

See CHANGELOG

Add new addContainerDependencies method to allow for container dependencies Fixes #2490

see #3106

Grants on the `alias/aws/sqs` KMS key alias are not necessary since the key will implicitly allow for it's intended usage to be fulfilled (as opposed to how you have to manage grants yourself when using a user-managed key instead). This removes the statement that was generated using an invalid resource entry. Fixes #2794

Fixes #3264 I'm trying to allow a lambda function in another account to be able to invoke my CDK generated lambda function. This works through the CLI like so: aws lambda add-permission --function-name=myFunction --statement-id=ABoldStatement --action=lambda:InvokeFunction --principal=arn:aws:iam::{account_id}:role/a_lambda_execution_role But CDK doesn't seem to allow me to add an ArnPrincipal doing something like this: myFunction.grantInvoke(new iam.ArnPrincipal(props.myARN)) With the error: Invalid principal type for Lambda permission statement: ArnPrincipal. Supported: AccountPrincipal, ServicePrincipal This PR allows ArnPrincipal to be passed to lambda.grantInvoke. There might be some additional validation required on the exact ARN as I believe only some ARNs are supported by lambda add-permission

* rename `KubernetesManifest` to `KubernetesResource` and `addResource` * move AWS Auth APIs to `cluster.awsAuth` and expose `AwsAuth` * remove the yaml library (we can just use a JSON stream) * add support for adding accounts to aws-auth * fix cluster deletion bug * move kubctl app info to constants

eladb · 2019-08-06T11:01:13Z

@moofish32 @rix0rrr can you please take another look? I think I've addressed most of your comments.

rix0rrr

Provisional approval, do what you will with my previous comments :). How does that work these days?

mergify · 2019-09-23T14:46:48Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

Elad Ben-Israel added 4 commits July 29, 2019 09:08

chore: update package-lock.json

4b128bb

feat(eks): define kubernetes resources

2a9dd48

This change allows defining arbitrary Kubernetes resources within an EKS cluster.

nice!

51d2fea

update readme

a944536

eladb requested review from skinny85 and SoManyHs as code owners August 1, 2019 16:04

Elad Ben-Israel added 2 commits August 1, 2019 19:04

Merge remote-tracking branch 'origin/master' into benisrae/eks-kubectl

386c3d6

Update README.md

91b4305

arun-gupta reviewed Aug 1, 2019

View reviewed changes

packages/@aws-cdk/aws-eks/README.md Show resolved Hide resolved

NGL321 assigned eladb Aug 1, 2019

eladb commented Aug 2, 2019

View reviewed changes

packages/@aws-cdk/aws-eks/lib/manifest-resource.ts Outdated Show resolved Hide resolved

eladb commented Aug 2, 2019

View reviewed changes

packages/@aws-cdk/aws-eks/lib/manifest-resource.ts Outdated Show resolved Hide resolved

moofish32 reviewed Aug 4, 2019

View reviewed changes

rix0rrr reviewed Aug 5, 2019

View reviewed changes

eladb mentioned this pull request Aug 5, 2019

[aws-eks] Default vpc #3541

Closed

1 task

eladb removed their assignment Aug 5, 2019

skinny85 and others added 13 commits August 6, 2019 13:56

feat(events): ability to add cross-account targets (#3323)

7fdb502

This adds the capability of adding a target to an event rule that belongs to a different account than the rule itself. Required for things like cross-account CodePipelines with source actions triggered by events.

chore(ci): add mergify config file (#3502)

d9598a4

chore: update jsii to 0.14.3 (#3513)

27ff73f

v1.3.0 (#3516)

15b806b

See CHANGELOG

fix: typo in restapi.ts (#3530)

c223827

feat(ecs): container dependencies (#3032)

1ae4c13

Add new addContainerDependencies method to allow for container dependencies Fixes #2490

feat(s3-deployment): CloudFront invalidation (#3213)

297d91a

see #3106

docs(core): findChild gets direct child only (#3512)

75d06fd

doc(iam): update references to addManagedPolicy (#3511)

fd02fc0

chore(contrib): remove API stabilization disclaimer

0200ec8

jogold and others added 2 commits August 6, 2019 13:57

fix(ssm): add GetParameters action to grantRead() (#3546)

ede2f8e

eladb requested a review from RomainMuller as a code owner August 6, 2019 10:58

Merge remote-tracking branch 'origin/master' into benisrae/eks-kubectl

54bda92

Elad Ben-Israel added 2 commits August 6, 2019 14:02

addManifest => addResource

1c1c886

update test expectations

1f877ea

NGL321 assigned eladb Aug 6, 2019

add unit test for customresrouce.ref

0f42503

rix0rrr reviewed Aug 7, 2019

View reviewed changes

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Aug 7, 2019

rix0rrr previously approved these changes Aug 7, 2019

View reviewed changes

fix sample link

121ce0f

eladb dismissed rix0rrr’s stale review via 121ce0f August 7, 2019 08:36

eladb removed the pr/do-not-merge This PR should not be merged at this time. label Aug 7, 2019

rix0rrr approved these changes Aug 7, 2019

View reviewed changes

Merge branch 'master' into benisrae/eks-kubectl

a7c86c2

mergify bot merged commit 4e11d86 into master Aug 7, 2019

RomainMuller deleted the benisrae/eks-kubectl branch August 10, 2019 00:38

eladb mentioned this pull request Aug 13, 2019

[aws-eks] EKS data plane cannot be created #3100

Closed

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 23, 2019

gricey432 mentioned this pull request Jan 8, 2024

eks: Remove the Cluster custom resource in favour of L1 #28609

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eks): programmatic definition of kubernetes resources #3510

feat(eks): programmatic definition of kubernetes resources #3510

eladb commented Aug 1, 2019 •

edited

Loading

moofish32 commented Aug 1, 2019

moofish32 left a comment

moofish32 Aug 4, 2019

moofish32 Aug 7, 2019

rix0rrr Aug 5, 2019

rix0rrr Aug 7, 2019

eladb Aug 7, 2019

eladb commented Aug 6, 2019

rix0rrr left a comment

mergify bot commented Sep 23, 2019

feat(eks): programmatic definition of kubernetes resources #3510

feat(eks): programmatic definition of kubernetes resources #3510

Conversation

eladb commented Aug 1, 2019 • edited Loading

moofish32 commented Aug 1, 2019

moofish32 left a comment

Choose a reason for hiding this comment

moofish32 Aug 4, 2019

Choose a reason for hiding this comment

moofish32 Aug 7, 2019

Choose a reason for hiding this comment

rix0rrr Aug 5, 2019

Choose a reason for hiding this comment

rix0rrr Aug 7, 2019

Choose a reason for hiding this comment

eladb Aug 7, 2019

Choose a reason for hiding this comment

eladb commented Aug 6, 2019

rix0rrr left a comment

Choose a reason for hiding this comment

mergify bot commented Sep 23, 2019

eladb commented Aug 1, 2019 •

edited

Loading