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

[aws-eks] expose EncryptionConfig in eks.Cluster Construct #9140

Closed
nicolai86 opened this issue Jul 17, 2020 · 10 comments · Fixed by #9438
Closed

[aws-eks] expose EncryptionConfig in eks.Cluster Construct #9140

nicolai86 opened this issue Jul 17, 2020 · 10 comments · Fixed by #9438
Assignees
Labels
@aws-cdk/aws-eks Related to Amazon Elastic Kubernetes Service effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1

Comments

@nicolai86
Copy link

nicolai86 commented Jul 17, 2020

❓ General Issue

Cloudformation supports passing in an EncryptionConfig when creating an EKS cluster.
CDK only exposes EncryptionConfig in the CfnCluster construct, not the high level eks.Cluster Construct.

The Question

Can we expose encryptionConfig as part of ClusterProps to allow this to be managed with the higher level CDK construct?

Environment

  • CDK CLI Version: 1.51.0
  • Module Version: 1.51.0
  • Node.js Version: v12.18.2
  • OS: OSX Mojave
  • Language (Version): TypeScript

Other information

CfnCluster exposes EncryptionConfig
https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-eks.CfnClusterProps.html#encryptionconfig
ClusterProps does not expose EncryptionConfig
https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-eks.ClusterProps.html

@nicolai86 nicolai86 added guidance Question that needs advice or information. needs-triage This issue or PR still needs to be triaged. labels Jul 17, 2020
@github-actions github-actions bot added the @aws-cdk/aws-eks Related to Amazon Elastic Kubernetes Service label Jul 17, 2020
@eladb eladb modified the milestone: EKS Dev Preview Jul 22, 2020
@eladb eladb assigned iliapolo and unassigned eladb Aug 4, 2020
@davidsung
Copy link
Contributor

davidsung commented Aug 4, 2020

Hi @iliapolo , I could contribute PR for this issue if nobody else started working on this. Could you help reviewing my PR? I have done all the logic and need to finish the integration test before submitting the PR. Thanks a lot!

@iliapolo
Copy link
Contributor

iliapolo commented Aug 4, 2020

@davidsung that would be great! Happy to review

@SomayaB SomayaB removed the needs-triage This issue or PR still needs to be triaged. label Aug 4, 2020
@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Aug 5, 2020
@SaikiranDaripelli
Copy link

Is there any workaround until this is merged? as it is not possible to edit the config after cluster creation.

@iliapolo
Copy link
Contributor

iliapolo commented Aug 5, 2020

@SaikiranDaripelli If you are using kubectlEnabled: false, you can use escape hatches to manipulate the underlying CFN configuration.

If you are using kubectlEnabled: true (or just omitted) there currently is no way since it is implemented with a custom resource.

However, I wouldn't recommend using kubectlEnabled: false and escape hatches as this will soon be deprecated and eventually removed. See discussion here: #9332.

I dont imagine it would take very long for this PR to be merged. Stay tuned.

eladb pushed a commit that referenced this issue Aug 5, 2020
Introduce an option `secretsEncryptionKey` which, if specified, will configure the cluster to use a KMS key for encrypting Kubernetes secrets.

This option can only be specified when the cluster is first created and currently cannot be updated due to a limitation in the EKS service.

Resolves #9140
@eladb
Copy link
Contributor

eladb commented Aug 5, 2020

#9472

@arul-gupta
Copy link

There also seems to an issue with AWS lambda runtime used to create custom resource. The latest nodejs12.x runtime uses AWS-SDK v2.631.0 which doesn't seem to have support for encryption. I get the following error:

Failed to create resource. Error: Unexpected key 'encryptionConfig' found in params
at invokeUserFunction (/var/task/framework.js:85:19)
at process._tickCallback (internal/process/next_tick.js:68:7)
Remote function error: UnexpectedParameter: Unexpected key 'encryptionConfig' found in params
at ParamValidator.fail (/var/runtime/node_modules/aws-sdk/lib/param_validator.js:50:37)
at ParamValidator.validateStructure (/var/runtime/node_modules/aws-sdk/lib/param_validator.js:77:14)
at ParamValidator.validateMember (/var/runtime/node_modules/aws-sdk/lib/param_validator.js:88:21)
at ParamValidator.validate (/var/runtime/node_modules/aws-sdk/lib/param_validator.js:34:10)
at Request.VALIDATE_PARAMETERS (/var/runtime/node_modules/aws-sdk/lib/event_listeners.js:126:42)
at Request.callListeners (/var/runtime/node_modules/aws-sdk/lib/sequential_executor.js:106:20)
at callNextListener (/var/runtime/node_modules/aws-sdk/lib/sequential_executor.js:96:12)
at /var/runtime/node_modules/aw

AWS-SDK added support for envelope encryption only in v2.634.0.

@iliapolo iliapolo added this to the EKS Dev Preview milestone Aug 10, 2020
@iliapolo iliapolo added feature-request A feature should be added or improved. effort/medium Medium work item – several days of effort p1 and removed guidance Question that needs advice or information. labels Aug 10, 2020
@iliapolo iliapolo assigned eladb and unassigned iliapolo Aug 10, 2020
@eladb
Copy link
Contributor

eladb commented Aug 10, 2020

Indeed this API is still not supported by the AWS Lambda node.js runtime. Let's wait until lambda the SDK version in their runtime and then we can merge this.

@davidsung
Copy link
Contributor

As figured out by @pahud, the aws-sdk version in the Lambda node12.x runtime env in most AWS regions is now 2.712.0 (already tested on us-east-1, us-east-2, ap-southeast-1, ap-northeast-1, ap-southeast-2, eu-central-1, eu-west-1, eu-west-2, sa-east-1). We thought the docs https://docs.aws.amazon.com/lambda/latest/dg/lambda-runtimes.html is outdated.
Screenshot 2020-08-10 at 9 46 37 PM

Tested with Lambda function:

const aws = require('aws-sdk');

exports.handler = async (event) => {
  console.log("request:", JSON.stringify(event, undefined, 2));
  const response = {
    statusCode: 200,
    body: JSON.stringify({version: aws.VERSION}),
  };
  return response;
};

@eladb
Copy link
Contributor

eladb commented Aug 10, 2020

Thanks for testing this. I am following up with the Lambda team to validate.

@iliapolo iliapolo changed the title [eks] expose EncryptionConfig in eks.Cluster Construct [aws-eks] expose EncryptionConfig in eks.Cluster Construct Aug 16, 2020
@davidsung
Copy link
Contributor

@eladb Kindly take note that the version specified in the Lambda runtime docs has been updated to 2.721.0

@eladb eladb removed their assignment Aug 17, 2020
davidsung added a commit to davidsung/aws-cdk that referenced this issue Aug 21, 2020
add envelope encryption support upon eks cluster creation
add update event handling for secretEncryptionKey mutation attempt
add test case for checking update event handling
update readme

Closes aws#9140

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@mergify mergify bot closed this as completed in #9438 Aug 21, 2020
mergify bot pushed a commit that referenced this issue Aug 21, 2020
feat(eks): envelope encryption for secrets

This PR adds envelope encryption support for Amazon EKS. Added a new key `secretsEncryptionKey` in `ClusterProps` for users to specify their own KMS CMK upon cluster creation:
```ts
new eks.Cluster(this, 'Cluster', {
  version: eks.KubernetesVersion.V1_16,
  secretsEncryptionKey,
});
```

Closes: #9140 

----

## Considerations

1. Confirmed `Secrets Encryption` is enabled in the provisioned Amazon EKS (both standard resource `AWS::EKS::Cluster` and custom resource `Custom::AWSCDK-EKS-Cluster`) after running an integration test from scratch. 
2. By inspecting the CloudTrail logs after the integration test, confirmed the exact KMS IAM permission required for the cluster creation role as `['kms:Encrypt', 'kms:Decrypt', 'kms:DescribeKey', 'kms:CreateGrant']`. Note: The encryption provider is using its own way to  generate data encryption key, not using KMS GenerateDataKey, and hence IAM permission`kms:GenerateDataKey*` is not required.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@iliapolo iliapolo removed the in-progress This issue is being actively worked on. label Aug 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-eks Related to Amazon Elastic Kubernetes Service effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1
Projects
None yet
7 participants