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] Multiple object reference to Nested Stack causes Circular Dependency. #10020

Closed
ten-lac opened this issue Aug 27, 2020 · 1 comment · Fixed by #10339
Closed

[aws-eks] Multiple object reference to Nested Stack causes Circular Dependency. #10020

ten-lac opened this issue Aug 27, 2020 · 1 comment · Fixed by #10339
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

@ten-lac
Copy link

ten-lac commented Aug 27, 2020

❓ General Issue

The Cluster interface creates circular dependency errors when multiple references to differing NestedStack child-objects. I'm not sure if this is an implementation error with Cluster or an inherent design flaw with NestedStack. It seems like a DAG would solve for this issue.

Tool Spec

  • node@12
  • cdk@1.6
  • language=js

Error

Circular dependency between resources: [ControlPlaneEKSCreationRole58D1869A, ControlPlaneEKSKubectlReadyBarrier1C298D16, ControlPlaneEKSKubectlProviderSecurityGroup68FF96CB, ControlPlaneEKSCreationRoleDefaultPolicy5BFFD31E, NetworkNestedStackNetworkNestedStackResource0124E108, ControlPlaneEKSAwsAuthmanifestD1CB63E6, ControlPlaneEKSC6E4919A]

Code Snippet

const { App, Construct, Stack } = require('@aws-cdk/core');
const { Cluster } = require('@aws-cdk/aws-eks');
const { SecurityGroup, Vpc } = require('@aws-cdk/aws-ec2');

class MyApp extends App {
    constructor(scope, id, props) {
        super(scope, id, props);

        new MyStack(this, 'MyUnderlay');
    }
}

class MyStack extends Stack {
    constructor(scope, id, props) {
        super(scope, id, props);

        const { vpc, eksControlPlaneSecurityGroup } = new Network(this, 'MyNetwork');
        new ControlPlane(this, 'MyControlPlane', { vpc, eksControlPlaneSecurityGroup });
    }
}

class Network extends NestedStack {
    constructor(scope, id, props) {
        super(scope, id, props);

        const vpc = new Vpc(this, 'Vpc', {
            cidr: '20.1.0.0/16',
            enableDnsHostnames: true,
            enableDnsSupport: true,
            maxAzs: 99
        });

        const eksControlPlaneSecurityGroup = new SecurityGroup(this, 'EKSControlPlane', {
            securityGroupName: 'MyControlePlaneSG',
            vpc,
            allowAllOutbound: false
        });

        Object.assign(this, { vpc, eksControlPlaneSecurityGroup });
    }
}

class ControlPlane extends Construct {
    constructor(scope, id, props) {
        super(scope, id, props);

        const { vpc, eksControlPlaneSecurityGroup } = props;

        new Cluster(this, 'MyEKS', {
            clusterName: 'fake-cluster',
            version: KubernetesVersion.V1_17,
            vpc,
            securityGroup: eksControlPlaneSecurityGroup,  // removing this resolves the circular dep error observed
            defaultCapacity: 0
        });
    }
}
@ten-lac ten-lac added guidance Question that needs advice or information. needs-triage This issue or PR still needs to be triaged. labels Aug 27, 2020
@github-actions github-actions bot added the @aws-cdk/aws-eks Related to Amazon Elastic Kubernetes Service label Aug 27, 2020
@ten-lac ten-lac changed the title [aws-eks] Multiple object reference from Nested Stack causes Circular Dependency. [aws-eks] Multiple object reference to Nested Stack causes Circular Dependency. Aug 27, 2020
@iliapolo
Copy link
Contributor

Related to #9536

@iliapolo iliapolo added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 and removed guidance Question that needs advice or information. labels Aug 30, 2020
@iliapolo iliapolo added this to the EKS Dev Preview milestone Aug 30, 2020
@iliapolo iliapolo removed the needs-triage This issue or PR still needs to be triaged. label Aug 30, 2020
@iliapolo iliapolo added the in-progress This issue is being actively worked on. label Sep 13, 2020
@mergify mergify bot closed this as completed in #10339 Sep 14, 2020
mergify bot pushed a commit that referenced this issue Sep 14, 2020
…s are used (#10339)

@eladb While working on this for the `aws-eks-next` module, i've started to second guess whether we really need to force cluster replacement.

This little PR fixes all the circular dependency issues as well as corrects the `cluster.connections` to include the EKS managed cluster security group. **It does not require cluster replacement**.

Merging this PR will leave two quirks/limitations:

- The cluster will have a redundant control plane security group, just like it has now. While redundant, it doesn't hurt, and doesn't really justify a cluster replacement. 

- One cluster per stack, just like we have now. I think we've already agreed that this limitation also does not justify cluster replacement, and the only reason we wanted to implement it is because we wanted to piggy back on the previous argument, which I now believe is not strong enough.

WDYT?

--------

This PR resolves a few problem related to circular dependencies when the cluster is configured with security groups from other stacks.

## Problem

Currently, the cluster creates a dedicated security group for the `KubectlProvider` and configures the main control plane security group to allow connections from it:

https://github.com/aws/aws-cdk/blob/f5c5d060ffee19ac3daa47cbf6df8d3563133433/packages/%40aws-cdk/aws-eks/lib/cluster.ts#L918-L924

This eventually creates ingress rules in the stack of control plane security group. These ingress rules refer to the `KubectlProvider` creates in the cluster stack, hence, a dependency if formed between the control plane security group stack, and the cluster stack.

`Control Plane Security Group Stack => Cluster Stack` (Control plane sg needs to allow connections from kubectl sg)

On the other hand, the cluster stack naturally depends on the control plane security group stack because the cluster needs the security group when its created. And so:

`Cluster Stack => Control Plane Security Group Stack` (Cluster needs the security group)
  
When these two stacks are different, a circular dependency is created.

## Solution

Do not create a security group for the `KubectlProvider` at all, and simply re-use the EKS managed security group. This security group is already configured to allow connections to the control plane, so if we simply attach it to the `KubectlProvider`, everything should work.

This avoids the creation of the first dependency direction.

Fixes #9754 
Fixes #10020 
Fixes #9536

----

*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 15, 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 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