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

feat(eks): support adding k8s resources to imported clusters #9802

Merged
merged 19 commits into from
Sep 2, 2020

Conversation

eladb
Copy link
Contributor

@eladb eladb commented Aug 18, 2020

Allow adding Kubernetes resources such as manifests and Helm charts to imported clusters (eks.Cluster.fromAttributes).

To enable this behavior, when the cluster is imported, users will have to specify additional information:

  • kubectlRole - an IAM role that can issue kubectl commands against the cluster
  • kubectlEnvironment (optional) - environment variables for kubectl.
  • kubectlPrivateSubnets and kubectlSecurityGroup - required if the cluster's k8s endpoint is private

Resolves #5383

BREAKING CHANGE: when importing EKS clusters using eks.Cluster.fromClusterAttributes, the clusterArn attribute is not supported anymore, and will always be derived from clusterName.

  • eks: Only a single eks.Cluster is allowed per CloudFormation stack.
  • eks: The securityGroups attribute of ClusterAttributes is now securityGroupIds.

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

Allow adding Kubernetes resources such as manifests and Helm charts to imported clusters (`eks.Cluster.fromAttributes`).

To enable this behavior, when the cluster is imported, users will have to specify additional information:

 - `kubectlRole` - an IAM role that can issue kubectl commands against the cluster
 - `kubectlEnvironment` (optional) - environment variables for `kubectl`.
 - `kubectlPrivateSubnets` and `kubectlSecurityGroup` - required if the cluster's k8s endpoint is private

Resolves #5383
packages/@aws-cdk/aws-eks/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-eks/README.md Show resolved Hide resolved
packages/@aws-cdk/aws-eks/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-eks/lib/cluster.ts Show resolved Hide resolved
packages/@aws-cdk/aws-eks/lib/cluster.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-eks/lib/cluster.ts Show resolved Hide resolved
packages/@aws-cdk/aws-eks/lib/cluster.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-eks/lib/cluster.ts Show resolved Hide resolved
packages/@aws-cdk/aws-eks/lib/cluster.ts Show resolved Hide resolved
packages/@aws-cdk/aws-eks/lib/kubectl-provider.ts Outdated Show resolved Hide resolved
@eladb eladb requested a review from iliapolo September 2, 2020 08:25
@eladb eladb added the pr/do-not-merge This PR should not be merged at this time. label Sep 2, 2020
@iliapolo
Copy link
Contributor

iliapolo commented Sep 2, 2020

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Sep 2, 2020

Command refresh: success

@iliapolo
Copy link
Contributor

iliapolo commented Sep 2, 2020

@eladb Any reason the do-not-merge label is still here?

@eladb eladb removed the pr/do-not-merge This PR should not be merged at this time. label Sep 2, 2020
@eladb
Copy link
Contributor Author

eladb commented Sep 2, 2020

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Sep 2, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify
Copy link
Contributor

mergify bot commented Sep 2, 2020

Command refresh: success

@mergify
Copy link
Contributor

mergify bot commented Sep 2, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 4439481 into master Sep 2, 2020
@mergify mergify bot deleted the benisrae/eks-add-manifest-to-imported-cluster branch September 2, 2020 12:06
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: fa0473f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@pahud
Copy link
Contributor

pahud commented Sep 2, 2020

This is amazing! Thank you for the great work!

@eladb
Copy link
Contributor Author

eladb commented Sep 2, 2020

This is amazing! Thank you for the great work!

Thanks! Looking forward for feedback :-)

@itajaja
Copy link

itajaja commented Sep 15, 2020

hey, just updated. we have two clusters in our stack. does this mean we cannot update, or is there a way to go around this?

@iliapolo
Copy link
Contributor

@itajaja This limitation actually short-circuits some problems you might encounter with multiple clusters in a stack with regards to endpoint access configuration. Is it possible for you to split the cluster to different stacks?

@itajaja
Copy link

itajaja commented Sep 15, 2020

what kinds of problems? are they solvable through aws-cdk alone?

Is it possible for you to split the cluster to different stacks?

it would be quite awkward.

@iliapolo
Copy link
Contributor

@itajaja

what kinds of problems? are they solvable through aws-cdk alone?

Currently no. The problem is that having multiple clusters in the same stack would force those clusters to have a publicly accessible endpoint (i.e only EndpointAccess.PUBLIC).

Apologies for the inconvenience, we have created an issue to gather feedback on this limitation and think of possible solutions. Would be great if you can share your use case.

mergify bot pushed a commit that referenced this pull request Sep 25, 2020
…ependency and breaks deployment (#10536)

In version [`1.62.0`](https://github.com/aws/aws-cdk/releases/tag/v1.62.0) we introduced the ability to run `kubectl` commands on imported clusters. (See #9802).

Part of this change included some refactoring with regards to how we use and create the `KubectlProvider`.
Looks like we didn't consistently apply the same logic across all constructs that use it.

Case in point:

https://github.com/aws/aws-cdk/blob/e349004a522e2123c1e93bd3402dd7c3f9c5c17c/packages/%40aws-cdk/aws-eks/lib/k8s-manifest.ts#L58

Notice that here we use `this` as the scope to the `getOrCreate` call. Same goes for:

https://github.com/aws/aws-cdk/blob/e349004a522e2123c1e93bd3402dd7c3f9c5c17c/packages/%40aws-cdk/aws-eks/lib/k8s-object-value.ts#L64

However, `KubernetesPatch` use `scope` instead.

https://github.com/aws/aws-cdk/blob/e349004a522e2123c1e93bd3402dd7c3f9c5c17c/packages/%40aws-cdk/aws-eks/lib/k8s-patch.ts#L74

This means that the entire `scope` of the `KubernetesPatch` now depends, among others, on the `kubectlBarrier`. 
The scope will usually be either the cluster itself (when using `FargateCluster`), or the entire stack (when using `new KubernetesPatch`). In any case, the scope will most likely contain the cluster VPC.

This creates the following dependency cycle: `Cluster => ClusterVpc => KubectlBarrier => Cluster`.

The fix aligns the `KubernetesPatch` behavior to all other `kubectl` constructs and uses `this` as the scope, which will only add dependency on the barrier to the custom resource representing the patch.

Fixes #10528
Fixes #10537

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
iliapolo added a commit that referenced this pull request Sep 25, 2020
…ependency and breaks deployment (#10536)

In version [`1.62.0`](https://github.com/aws/aws-cdk/releases/tag/v1.62.0) we introduced the ability to run `kubectl` commands on imported clusters. (See #9802).

Part of this change included some refactoring with regards to how we use and create the `KubectlProvider`.
Looks like we didn't consistently apply the same logic across all constructs that use it.

Case in point:

https://github.com/aws/aws-cdk/blob/e349004a522e2123c1e93bd3402dd7c3f9c5c17c/packages/%40aws-cdk/aws-eks/lib/k8s-manifest.ts#L58

Notice that here we use `this` as the scope to the `getOrCreate` call. Same goes for:

https://github.com/aws/aws-cdk/blob/e349004a522e2123c1e93bd3402dd7c3f9c5c17c/packages/%40aws-cdk/aws-eks/lib/k8s-object-value.ts#L64

However, `KubernetesPatch` use `scope` instead.

https://github.com/aws/aws-cdk/blob/e349004a522e2123c1e93bd3402dd7c3f9c5c17c/packages/%40aws-cdk/aws-eks/lib/k8s-patch.ts#L74

This means that the entire `scope` of the `KubernetesPatch` now depends, among others, on the `kubectlBarrier`. 
The scope will usually be either the cluster itself (when using `FargateCluster`), or the entire stack (when using `new KubernetesPatch`). In any case, the scope will most likely contain the cluster VPC.

This creates the following dependency cycle: `Cluster => ClusterVpc => KubectlBarrier => Cluster`.

The fix aligns the `KubernetesPatch` behavior to all other `kubectl` constructs and uses `this` as the scope, which will only add dependency on the barrier to the custom resource representing the patch.

Fixes #10528
Fixes #10537

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[aws-eks] eks.Cluster.fromAttributes: Allow adding k8s resources on imported clusters
5 participants