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] private subnets from other Cluster or Vpc can't be used when importing cluster #10287

Closed
stefanolczak opened this issue Sep 10, 2020 · 5 comments · Fixed by #10459
Closed
Assignees
Labels
@aws-cdk/aws-eks Related to Amazon Elastic Kubernetes Service bug This issue is a bug. effort/small Small work item – less than a day of effort p1

Comments

@stefanolczak
Copy link

#9802 introduced a way to import EKS cluster and add k8s resources to it. It can be also used to split resources across multiple stacks when deploying EKS from CDK. This is very helpful because there is a limit of 200 CF resources per stack. But there is a bug in current implementation that requires that kubectlPrivateSubnets doesn't include subnet ids that are tokens created from other constructs. This blocks from importing EKS cluster from object created in CDK. To make it work VPC has to be imported from lookup which has it own drawbacks. I believe this bug is easy to fix by changing the way identifiers are generated and passed to ec2.Subnet.fromSubnetId() here:

this.kubectlPrivateSubnets = props.kubectlPrivateSubnetIds ? props.kubectlPrivateSubnetIds.map(subnetid => ec2.Subnet.fromSubnetId(this, `KubectlSubnet${subnetid}`, subnetid)) : undefined;

Not including the subnedId in identifier should fix the issue.

Reproduction Steps

  1. Create EKS cluster with private endpoint access.
  2. Import the cluster in new stack by passing kubectlPrivateSubnets from created object.

Code example:

#!/usr/bin/env python3

from aws_cdk import (
    core,
    aws_ec2,
    aws_eks
)


class VpcStack(core.Stack):

    def __init__(self, app: core.App) -> None:
        super().__init__(app, 'vpc-stack')

        self.vpc = aws_ec2.Vpc(
            scope=self,
            id='vpc'
        )


class EksStack(core.Stack):
    def __init__(self, app: core.App, vpc: aws_ec2.Vpc) -> None:
        super().__init__(app, 'eks-stack')

        self.eks_cluster = aws_eks.Cluster(
            scope=self,
            id='eks',
            cluster_name='eks-cluster',
            default_capacity=0,
            version=aws_eks.KubernetesVersion.V1_17,
            vpc=vpc,
            endpoint_access=aws_eks.EndpointAccess.PRIVATE
        )


class ImportedEksStack(core.Stack):
    def __init__(self, app: core.App, eks_cluster: aws_eks.Cluster) -> None:
        super().__init__(app, 'imported-eks-stack')

        aws_eks.Cluster.from_cluster_attributes(
            scope=self,
            id='eks',
            cluster_name=eks_cluster.cluster_name,
            kubectl_role_arn=eks_cluster.kubectl_role.role_arn,
            kubectl_private_subnet_ids=[subnet.subnet_id for subnet in eks_cluster.kubectl_private_subnets],
            kubectl_security_group_id=eks_cluster.kubectl_security_group.security_group_id,
            vpc=eks_cluster.vpc
        )


app = core.App()
vpc_stack = VpcStack(app)
eks_stack = EksStack(app, vpc_stack.vpc)
ImportedEksStack(app, eks_stack.eks_cluster)

app.synth()

What did you expect to happen?

Cluster can be imported from other cluster object by passing all required information from it.
Also it would be nice to have simpler way to import cluster from existing object. Like I said it supports common scenario of splitting k8s resources across multiple stacks to avoid CF limit of 200 resources per stack.

What actually happened?

Template was unable to synthesize and CDK throws an error:

  Error: Cannot use tokens in construct ID: KubectlSubnet${Token[TOKEN.94]}
      at new Construct (/tmp/jsii-kernel-uf3pya/node_modules/@aws-cdk/core/lib/construct-compat.js:36:19)
      at new Resource (/tmp/jsii-kernel-uf3pya/node_modules/@aws-cdk/core/lib/resource.js:15:9)
      at new ImportedSubnet (/tmp/jsii-kernel-uf3pya/node_modules/@aws-cdk/aws-ec2/lib/vpc.js:935:9)
      at Function.fromSubnetAttributes (/tmp/jsii-kernel-uf3pya/node_modules/@aws-cdk/aws-ec2/lib/vpc.js:670:16)
      at Function.fromSubnetId (/tmp/jsii-kernel-uf3pya/node_modules/@aws-cdk/aws-ec2/lib/vpc.js:677:21)
      at ImportedCluster.kubectlPrivateSubnets.props.kubectlPrivateSubnetIds.props.kubectlPrivateSubnetIds.map.subnetid (/tmp/jsii-kernel-uf3pya/node_modules/@aws-cdk/aws-eks/lib/cluster.js:697:127)
      at Array.map (<anonymous>)
      at new ImportedCluster (/tmp/jsii-kernel-uf3pya/node_modules/@aws-cdk/aws-eks/lib/cluster.js:697:100)
      at Function.fromClusterAttributes (/tmp/jsii-kernel-uf3pya/node_modules/@aws-cdk/aws-eks/lib/cluster.js:307:16)

Environment

  • CLI Version : 1.62.0

Other


This is 🐛 Bug Report

@stefanolczak stefanolczak added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 10, 2020
@github-actions github-actions bot added the @aws-cdk/aws-eks Related to Amazon Elastic Kubernetes Service label Sep 10, 2020
@iliapolo iliapolo added effort/small Small work item – less than a day of effort p1 and removed needs-triage This issue or PR still needs to be triaged. labels Sep 10, 2020
@stefanolczak
Copy link
Author

@iliapolo is there a chance it will be fixed anytime soon? We are hitting limit of resources in one CF stack and this issue is blocking us from splitting k8s resources into multiple stacks.

@iliapolo
Copy link
Contributor

@stefanolczak Can I ask why are you both creating and importing a cluster in the same CDK application? Mind sharing the full use-case?

@iliapolo iliapolo added the in-progress This issue is being actively worked on. label Sep 21, 2020
@mergify mergify bot closed this as completed in #10459 Sep 21, 2020
mergify bot pushed a commit that referenced this issue Sep 21, 2020
…nets` (#10459)

Don't use the subnet id as the construct id as it may be a token. 

Fixes #10287

----

*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 Sep 21, 2020
@stefanolczak
Copy link
Author

I’m splitting kubernetes manifests and helm charts into multiple stacks to avoid limit of 200 resources in one CF stack. Is there a better way to do it? @iliapolo

@iliapolo
Copy link
Contributor

You could just pass and use the cluster construct between stacks.

const cluster = new eks.Cluster(clusterStack, ...);
const resource = new eks.KubernetesManifest(manifestsStack, 'Resource', {
  cluster: cluster,
  manifest: ...
})

Something like this? Same goes for charts.

The part i'm struggling to understand in your code is:

class ImportedEksStack(core.Stack):
    def __init__(self, app: core.App, eks_cluster: aws_eks.Cluster) -> None:
        super().__init__(app, 'imported-eks-stack')

        aws_eks.Cluster.from_cluster_attributes(
            scope=self,
            id='eks',
            cluster_name=eks_cluster.cluster_name,
            kubectl_role_arn=eks_cluster.kubectl_role.role_arn,
            kubectl_private_subnet_ids=[subnet.subnet_id for subnet in eks_cluster.kubectl_private_subnets],
            kubectl_security_group_id=eks_cluster.kubectl_security_group.security_group_id,
            vpc=eks_cluster.vpc
        )

Why do you need to instantiate an imported cluster when you already have a reference for a regular cluster? You can still use it even in cross-stack, CDK will create the necessary outputs and parameters.

@stefanolczak
Copy link
Author

stefanolczak commented Sep 22, 2020

Oh I didn't realize that I can create manifests and helm charts this way and I was using the addManifest() and addChart() method always. That's why I was looking for a way to call the methods on imported cluster. The new approach you proposed is a lot easier. Thanks

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 bug This issue is a bug. effort/small Small work item – less than a day of effort p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants