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

[appmesh] VirtualNode in separate stack from Mesh creates circular dependency #10049

Closed
buzzsurfr opened this issue Aug 29, 2020 · 9 comments · Fixed by #15237
Closed

[appmesh] VirtualNode in separate stack from Mesh creates circular dependency #10049

buzzsurfr opened this issue Aug 29, 2020 · 9 comments · Fixed by #15237
Labels
@aws-cdk/aws-appmesh Related to AWS App Mesh bug This issue is a bug. effort/medium Medium work item – several days of effort p1
Milestone

Comments

@buzzsurfr
Copy link

Adding a VirtualNode to a Mesh (where the Mesh is in one stack and the VirtualNode is in another) creates an circular reference, but shouldn't.

Reproduction Steps

Sample code available at buzzsurfr/appmesh-cross-stack-bug

Sample code in master branch works, but uncommenting the following lines...

https://github.com/buzzsurfr/appmesh-cross-stack-bug/blob/e374561e516c328ed90de2add287b2d81153a642/lib/meshed-service-stack.ts#L103-L112

...then run:

npm run build
cdk ls

What did you expect to happen?

This shouldn't be a circular reference and should work.

What actually happened?

/Users/username/Sandbox/cdk/appmesh-cross-stack-bug/node_modules/@aws-cdk/core/lib/stack.ts:688
      throw new Error(`'${target.node.path}' depends on '${this.node.path}' (${cycle.join(', ')}). Adding this dependency (${reason}) would create a cyclic reference.`);
            ^
Error: 'InfrastructureStack' depends on 'MeshedServiceStack' (InfrastructureStack -> MeshedServiceStack/service/CloudmapService/Resource.Name). Adding this dependency (MeshedServiceStack -> InfrastructureStack/Mesh/Resource.MeshName) would create a cyclic reference.
    at MeshedServiceStack._addAssemblyDependency (/Users/username/Sandbox/cdk/appmesh-cross-stack-bug/node_modules/@aws-cdk/core/lib/stack.ts:688:13)
    at Object.addDependency (/Users/username/Sandbox/cdk/appmesh-cross-stack-bug/node_modules/@aws-cdk/core/lib/deps.ts:52:20)
    at MeshedServiceStack.addDependency (/Users/username/Sandbox/cdk/appmesh-cross-stack-bug/node_modules/@aws-cdk/core/lib/stack.ts:445:5)
    at resolveValue (/Users/username/Sandbox/cdk/appmesh-cross-stack-bug/node_modules/@aws-cdk/core/lib/private/refs.ts:101:12)
    at Object.resolveReferences (/Users/username/Sandbox/cdk/appmesh-cross-stack-bug/node_modules/@aws-cdk/core/lib/private/refs.ts:31:24)
    at Object.prepareApp (/Users/username/Sandbox/cdk/appmesh-cross-stack-bug/node_modules/@aws-cdk/core/lib/private/prepare-app.ts:36:5)
    at Object.synthesize (/Users/username/Sandbox/cdk/appmesh-cross-stack-bug/node_modules/@aws-cdk/core/lib/private/synthesis.ts:21:3)
    at App.synth (/Users/username/Sandbox/cdk/appmesh-cross-stack-bug/node_modules/@aws-cdk/core/lib/stage.ts:175:23)
    at process.<anonymous> (/Users/username/Sandbox/cdk/appmesh-cross-stack-bug/node_modules/@aws-cdk/core/lib/app.ts:112:45)
    at Object.onceWrapper (events.js:422:26)
Subprocess exited with error 1

Environment

  • CLI Version : 1.61.1 (build 347918f)
  • Framework Version: ^1.61.1
  • Node.js Version: v13.12.0
  • OS : macOS Catalina 10.15.6
  • Language (Version): TypeScript (3.9.7)

Other


This is 🐛 Bug Report

@buzzsurfr buzzsurfr added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Aug 29, 2020
@github-actions github-actions bot added the @aws-cdk/aws-appmesh Related to AWS App Mesh label Aug 29, 2020
@skinny85 skinny85 added effort/medium Medium work item – several days of effort p1 and removed needs-triage This issue or PR still needs to be triaged. labels Sep 5, 2020
@skinny85 skinny85 added this to the [DevPreview] AppMesh milestone Dec 1, 2020
@Seiya6329
Copy link
Contributor

I was able to reproduce the error.

Although I am still investing the root cause, I was able to avoid the error by creating the mesh resource inside the meshed-service-stack instead of creating inside the infrastructure-stack.

For sample code. please refer https://github.com/Seiya6329/appmesh-cross-stack-bug/tree/wayaround

Next step:
Identify the root cause.

@skinny85
Copy link
Contributor

Minimal reproduction:

import * as cdk from '@aws-cdk/core';
import * as ec2 from '@aws-cdk/aws-ec2';
import * as ecs from '@aws-cdk/aws-ecs';
import * as appmesh from '@aws-cdk/aws-appmesh';

export class MeshStack extends cdk.Stack {
    public readonly cluster: ecs.Cluster;
    public readonly mesh: appmesh.Mesh;

    constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
        super(scope, id, props);

        const vpc = new ec2.Vpc(this, 'Vpc');
        this.cluster = new ecs.Cluster(this, 'Cluster', {
            vpc: vpc,
            defaultCloudMapNamespace: {
                name: 'cdkbug.local',
            },
        });
        this.mesh = new appmesh.Mesh(this, 'Mesh');
    }
}

interface ServiceStackProps extends cdk.StackProps {
    readonly cluster: ecs.ICluster,
    readonly mesh: appmesh.IMesh,
}

export class ServiceStack extends cdk.Stack {
    constructor(scope: cdk.Construct, id: string, props: ServiceStackProps) {
        super(scope, id, props);

        const taskDef = new ecs.FargateTaskDefinition(this, 'TaskDef');
        const appContainer = taskDef.addContainer('echo', {
            image: ecs.ContainerImage.fromRegistry('hashicorp/http-echo'),
        });
        appContainer.addPortMappings({ containerPort: 5678 });
        const service = new ecs.FargateService(this, "service", {
            cluster: props.cluster,
            taskDefinition: taskDef,
            cloudMapOptions: {}, // empty is fine for reproducing this bug
        });

        const virtualNode = props.mesh.addVirtualNode('vn', {
            /* ******** uncomment below 3 lines to reproduce the error ******** */
            // serviceDiscovery:  appmesh.ServiceDiscovery.cloudMap({
            //     service: service.cloudMapService!,
            // }),
            listeners: [appmesh.VirtualNodeListener.http({
                port: appContainer.containerPort,
            })],
        });
        // this also doesn't work:
        // const virtualNode2 = new appmesh.VirtualNode(this, 'vn2', {
        //     mesh: props.mesh,
        // });

        // this works fine:
        const virtualRouter = props.mesh.addVirtualRouter('vr', {
            listeners: [appmesh.VirtualRouterListener.http(appContainer.containerPort)],
        });
        // this works fine too:
        const defaultRoute = virtualRouter.addRoute('vr-route-default', {
            routeSpec: appmesh.RouteSpec.http({
                weightedTargets: [{
                    virtualNode: virtualNode,
                    weight: 100,
                }],
            }),
        });
    }
}

const app = new cdk.App();

const meshStack = new MeshStack(app, 'MeshStack');
new ServiceStack(app, 'ServiceStack', {
    cluster: meshStack.cluster,
    mesh: meshStack.mesh,
});

@skinny85
Copy link
Contributor

The fact this also causes the cycle to happen:

        const virtualNode2 = new appmesh.VirtualNode(this, 'vn2', {
            mesh: props.mesh,
        });

worries me a little bit.

@Seiya6329
Copy link
Contributor

Seiya6329 commented Jun 21, 2021

Thanks @skinny85 for pasting the minimal reproduction sample code here.

Just to highlight the root cause, new appmesh.VirtualNode() scenario is causing the MeshStack to reference ServiceStack because:

  • VirtualNode resource is created in this stack
  • VirtualRouter resource is created in same stack where Mesh resource is created
  • Route resource is created in same stack where Mesh and VirtualRouter resources are created
  • Route resource is referring the VirtualNode resource that is created in this stack

@Seiya6329
Copy link
Contributor

Seiya6329 commented Jun 21, 2021

I believe the concern is what would be the probability for a user to face this issue.

Essentially there are two high-level scenarios to reproduce this error. To simplify, I will refer StackA and StackB as an example while a user may have much more stacks.

Scenario 1: Uses a method to create a resource

  1. Mesh resource is created in Stack A
  2. Stack B refers Mesh resource from Stack A
  3. One of the following resources are created in Stack B using the .add...() method
    • Virtual Router
    • Virtual Node
    • Virtual Gateway
  4. One of the resources created in the Line Add script to sign arbitrary files using the key stored in Secrets Manager #3 refers a value from an object that is created in the same stack.

Scenario 2: Uses a mixture of method and constructor to create a resource

  1. Mesh resource is created in Stack A
  2. Stack B refers Mesh resource from Stack A
  3. VirtualNode is created using the method and VirtualRouter is created using the constructor in Stack B (*can be opposite way)
  4. Route is added to VirtualRouter
  5. Stack B is referring some resource other than the Mesh from Stack A.

Note: I am pretty sure Scenario #2 can be reproduced using VirtualGateway and VirtualService but I haven't tried it yet.


I can imagine a user uses Scenario 1 setup. For example, create Mesh resource in one resource and pass it as a resource to other stacks that manages individual microservices (maybe in the form of ECS cluter?).

For Scenario 2 (same one as @skinny85 is having a slight concern), I am actually not worried too much here. To achieve this scenario, user must use both method and constructors to create resources and although it is definitely possible, it is less consistent to write a code this way.

Bottom Line:

  • I think this is low risk to leave it as it is because it could be the rare scenario to face this error and there are multiple workaround to avoid the error.
  • I am suggesting to fix this error only if expected behavior of .add...() type of methods is to add a resource to this stack where the method is being called strictly.

@dfezzie and @alexbrjo - Please feel free to share your thoughts here.

@skinny85
Copy link
Contributor

I think I agree with your conclusions @Seiya6329. Looks like I was worried about that new VirtualNode(... case for nothing 🙂.

This looks to be just a matter of various references between CDK constructs causing dependencies between Stacks. I don't think there's anything unusual here for AppMesh.

For the problem posted originally in the issue, it's just a matter of changing from:

        const virtualNode = props.mesh.addVirtualNode('vn', {
            serviceDiscovery: appmesh.ServiceDiscovery.cloudMap({
                service: service.cloudMapService!,
            }),
            listeners: [appmesh.VirtualNodeListener.http({
                port: appContainer.containerPort,
            })],
        });

to:

        const virtualNode = new appmesh.VirtualNode(this, 'vn', {
            mesh: props.mesh,
            serviceDiscovery: appmesh.ServiceDiscovery.cloudMap({
                service: service.cloudMapService!,
            }),
            listeners: [appmesh.VirtualNodeListener.http({
                port: appContainer.containerPort,
            })],
        });

to solve the cycle.

I think the only possible action from this might be adding some clarification to the documentation of the various add*() methods in AppMesh that they create a resource in the same Stack as the object they are called on (the Mesh in case of VirtualNode), which might be different than the current Stack. But I think that 's as far as any code changes are needed here.

@Seiya6329
Copy link
Contributor

I think the only possible action from this might be adding some clarification to the documentation of the various add*() methods in AppMesh that they create a resource in the same Stack as the object they are called on (the Mesh in case of VirtualNode), which might be different than the current Stack.

This is really great point and has been suggested by @dfezzie as well.

Conclusion:

  • No actions to change the code
  • Update the CDK documentation to clarify the difference of using .add() method and the constructor to create a resource.

@Seiya6329
Copy link
Contributor

ETA for updating the documentation: 7/1/21

@mergify mergify bot closed this as completed in #15237 Jun 22, 2021
mergify bot pushed a commit that referenced this issue Jun 22, 2021
…od to create a resource (#15237)

This closes #10049.

#### REV
- Updating the doc for`.add...()` methods to specify that the resource is created in same stack where the `Mesh` exists
- Updating `README` file to clarify the difference between using the constructor and method to create a resource.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

hollanddd pushed a commit to hollanddd/aws-cdk that referenced this issue Aug 26, 2021
…od to create a resource (aws#15237)

This closes aws#10049.

#### REV
- Updating the doc for`.add...()` methods to specify that the resource is created in same stack where the `Mesh` exists
- Updating `README` file to clarify the difference between using the constructor and method to create a resource.

----

*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
@aws-cdk/aws-appmesh Related to AWS App Mesh bug This issue is a bug. effort/medium Medium work item – several days of effort p1
Projects
None yet
3 participants