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

[BUG] Error: LogicalId defined outside of stack #684

Open
kornicameister opened this issue Jan 18, 2024 · 17 comments
Open

[BUG] Error: LogicalId defined outside of stack #684

kornicameister opened this issue Jan 18, 2024 · 17 comments
Assignees
Labels
backlog bug Something isn't working needs-triage

Comments

@kornicameister
Copy link

Describe the bug

I have encountered a bug that was also reported here: #271.

When trying to generate a graph for a stack that uses cdk-temp-stack, cdk synth fails with error also reported in #271

Expected Behavior

Possible to generate a graph.

Current Behavior

Error: LogicalId defined outside of stack: AutoDestructDeleteStackServiceRoleADD04D27 - Node:CFN_RESOURCE::xxxAutoDestructDeleteStackServiceRole11230D55
    at new Node (/Users/tomasztrebski/dev-ay/ay5/node_modules/@aws/pdk/cdk-graph/core/graph.ts:1173:15)
    at new CfnResourceNode (/Users/tomasztrebski/dev-ay/ay5/node_modules/@aws/pdk/cdk-graph/core/graph.ts:2051:5)
    at visit (/Users/tomasztrebski/dev-ay/ay5/node_modules/@aws/pdk/cdk-graph/core/compute.ts:266:18)
    at visit (/Users/tomasztrebski/dev-ay/ay5/node_modules/@aws/pdk/cdk-graph/core/compute.ts:296:9)
    at visit (/Users/tomasztrebski/dev-ay/ay5/node_modules/@aws/pdk/cdk-graph/core/compute.ts:296:9)
    at visit (/Users/tomasztrebski/dev-ay/ay5/node_modules/@aws/pdk/cdk-graph/core/compute.ts:296:9)
    at visit (/Users/tomasztrebski/dev-ay/ay5/node_modules/@aws/pdk/cdk-graph/core/compute.ts:296:9)
    at visit (/Users/tomasztrebski/dev-ay/ay5/node_modules/@aws/pdk/cdk-graph/core/compute.ts:296:9)
    at computeGraph (/Users/tomasztrebski/dev-ay/ay5/node_modules/@aws/pdk/cdk-graph/core/compute.ts:306:3)
    at CdkGraph._synthesize (/Users/tomasztrebski/dev-ay/ay5/node_modules/@aws/pdk/cdk-graph/cdk-graph.ts:284:31)

Reproduction Steps

Originally I thought the problem comes from using TimeToLive from @cloudcomponents/cdk-temp-stack.
However building a short repro with cdk.Stack does not reveal the problem.
Reason why I'm bringing up cdk.Stack is that I have built my custom stack that sets up some defaults like permissionBoundary, synethizer and TimeToLive to match regulations in my company.

Error occurs if I add MyStack to an cdk.App but does not for ordinary cdk.Stack.
I cannot share the full code of my stack however it roughly does this:

#!/usr/bin/env node
import { CdkGraph } from '@aws/pdk/cdk-graph';
import { TimeToLive } from '@cloudcomponents/cdk-temp-stack';
import * as cdk from 'aws-cdk-lib';
import 'source-map-support/register';

import { IConstruct } from 'constructs';

const app = new cdk.App();
const env = {
  account:
    process.env['CDK_DEPLOY_ACCOUNT'] || process.env['CDK_DEFAULT_ACCOUNT'],
  region: process.env['CDK_DEPLOY_REGION'] || process.env['CDK_DEFAULT_REGION'],
};

class Stack extends cdk.Stack {
  constructor(scope: cdk.App, id: string, props: cdk.StackProps) {
    super(scope, id, {
      ...props,
      terminationProtection: false,
      analyticsReporting: false,
      permissionsBoundary: cdk.PermissionsBoundary.fromName('test'),
      synthesizer: new cdk.DefaultStackSynthesizer({
        qualifier: 'xx',
        dockerTagPrefix: 'xx',
      }),
    });

    cdk.Aspects.of(this).add(new ApplyDestroyPolicyAspect());
    new TimeToLive(this, 'AutoDestruct', {
      ttl: cdk.Duration.days(1),
    });
  }
}

class ApplyDestroyPolicyAspect implements cdk.IAspect {
  visit(node: IConstruct): void {
    if (node instanceof cdk.CfnResource) {
      node.applyRemovalPolicy(cdk.RemovalPolicy.DESTROY);
    }
  }
}

new Stack(app, 'Test', {
  env,
  stackName: 'xxx',
});

new CdkGraph(app);

Error is not throw here though.
I can share a resulting cloudformation template though:

{
 "Description": "[testing] XXX",
 "Resources": {
  "AutoDestructDeleteStackServiceRoleADD04D27": {
   "Type": "AWS::IAM::Role",
   "Properties": {
    "AssumeRolePolicyDocument": {
     "Statement": [
      {
       "Action": "sts:AssumeRole",
       "Effect": "Allow",
       "Principal": {
        "Service": "lambda.amazonaws.com"
       }
      }
     ],
     "Version": "2012-10-17"
    },
    "ManagedPolicyArns": [
     {
      "Fn::Join": [
       "",
       [
        "arn:",
        {
         "Ref": "AWS::Partition"
        },
        ":iam::aws:policy/service-role/AWSLambdaBasicExecutionRole"
       ]
      ]
     }
    ],
    "PermissionsBoundary": "arn:aws:iam::123456789012:policy/ays-cdk-v1-permissions-boundary"
   },
   "UpdateReplacePolicy": "Delete",
   "DeletionPolicy": "Delete",
   "Metadata": {
    "aws:cdk:path": "xxx/AutoDestruct/DeleteStack/ServiceRole/Resource"
   }
  },
  "AutoDestructDeleteStackServiceRoleDefaultPolicy42286F0B": {
   "Type": "AWS::IAM::Policy",
   "Properties": {
    "PolicyDocument": {
     "Statement": [
      {
       "Action": "*",
       "Effect": "Allow",
       "Resource": "*"
      }
     ],
     "Version": "2012-10-17"
    },
    "PolicyName": "AutoDestructDeleteStackServiceRoleDefaultPolicy42286F0B",
    "Roles": [
     {
      "Ref": "AutoDestructDeleteStackServiceRoleADD04D27"
     }
    ]
   },
   "UpdateReplacePolicy": "Delete",
   "DeletionPolicy": "Delete",
   "Metadata": {
    "aws:cdk:path": "xxx/AutoDestruct/DeleteStack/ServiceRole/DefaultPolicy/Resource"
   }
  },
  "AutoDestructDeleteStack239CEEE3": {
   "Type": "AWS::Lambda::Function",
   "Properties": {
    "Code": {
     "S3Bucket": "cdk-cdk-v1-assets-123456789012-eu-central-1",
     "S3Key": "ece017a7d7cfba4a1602f6d267cf5a02781708db95bbf4ff8c2394796f26b7a2.zip"
    },
    "Handler": "index.handler",
    "Role": {
     "Fn::GetAtt": [
      "AutoDestructDeleteStackServiceRoleADD04D27",
      "Arn"
     ]
    },
    "Runtime": "nodejs14.x"
   },
   "DependsOn": [
    "AutoDestructDeleteStackServiceRoleDefaultPolicy42286F0B",
    "AutoDestructDeleteStackServiceRoleADD04D27"
   ],
   "UpdateReplacePolicy": "Delete",
   "DeletionPolicy": "Delete",
   "Metadata": {
    "aws:cdk:path": "xxx/AutoDestruct/DeleteStack/Resource",
    "aws:asset:path": "asset.ece017a7d7cfba4a1602f6d267cf5a02781708db95bbf4ff8c2394796f26b7a2",
    "aws:asset:is-bundled": false,
    "aws:asset:property": "Code"
   }
  },
  "AutoDestructTimeToLive6FA1EF2B": {
   "Type": "AWS::Events::Rule",
   "Properties": {
    "ScheduleExpression": "rate(3 hours)",
    "State": "ENABLED",
    "Targets": [
     {
      "Arn": {
       "Fn::GetAtt": [
        "AutoDestructDeleteStack239CEEE3",
        "Arn"
       ]
      },
      "Id": "Target0",
      "Input": {
       "Fn::Join": [
        "",
        [
         "{\"stackId\":\"",
         {
          "Ref": "AWS::StackId"
         },
         "\"}"
        ]
       ]
      }
     }
    ]
   },
   "UpdateReplacePolicy": "Delete",
   "DeletionPolicy": "Delete",
   "Metadata": {
    "aws:cdk:path": "xxx/AutoDestruct/TimeToLive/Resource"
   }
  },
  "AutoDestructTimeToLiveAllowEventRulexxxAutoDestructDeleteStackC6581343BEFD32FE": {
   "Type": "AWS::Lambda::Permission",
   "Properties": {
    "Action": "lambda:InvokeFunction",
    "FunctionName": {
     "Fn::GetAtt": [
      "AutoDestructDeleteStack239CEEE3",
      "Arn"
     ]
    },
    "Principal": "events.amazonaws.com",
    "SourceArn": {
     "Fn::GetAtt": [
      "AutoDestructTimeToLive6FA1EF2B",
      "Arn"
     ]
    }
   },
   "UpdateReplacePolicy": "Delete",
   "DeletionPolicy": "Delete",
   "Metadata": {
    "aws:cdk:path": "xxx/AutoDestruct/TimeToLive/AllowEventRulexxxAutoDestructDeleteStackC6581343"
   }
  }
 },
 "Parameters": {
  "BootstrapVersion": {
   "Type": "AWS::SSM::Parameter::Value<String>",
   "Default": "/cdk-bootstrap/cdk-v1/version",
   "Description": "Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store. [cdk:skip]"
  }
 },
 "Rules": {
  "CheckBootstrapVersion": {
   "Assertions": [
    {
     "Assert": {
      "Fn::Not": [
       {
        "Fn::Contains": [
         [
          "1",
          "2",
          "3",
          "4",
          "5"
         ],
         {
          "Ref": "BootstrapVersion"
         }
        ]
       }
      ]
     },
     "AssertDescription": "CDK bootstrap stack version 6 required. Please run 'cdk bootstrap' with a recent version of the CDK CLI."
    }
   ]
  }
 }
}

Possible Solution

No response

Additional Information/Context

CDK: 2.121.1

PDK version used

0.22.50

What languages are you seeing this issue on?

Typescript

Environment details (OS name and version, etc.)

MacOS Sierra

@kornicameister kornicameister added bug Something isn't working needs-triage labels Jan 18, 2024
@kornicameister
Copy link
Author

cc @JeremyJonas , I noticed you've been handling the other bug.
I figure it's worth mentioning you.

@JeremyJonas
Copy link
Contributor

Hi @kornicameister, could you explain a bit more details about your "custom" stack vs "ordinary stack"? And from your comment the sample code provide works right, as you stated no error with "ordinary stack" and your example is an "ordinary stack" right?

Given the AutoDestructDeleteStackServiceRoleADD04D27 role resource is inside of the Stack, seems that the inference of the stack is not getting detected. We use the Stack.of logic for this.

Does your custom stack evaluate to true for Stack.isStack(yourStack) and does Stack.of(yourConstruct) evaluate to your custom stack?

  • // Infer the stack this construct belongs to
    let stack: Graph.StackNode | undefined;
    try {
    if (construct.node.scope) {
    const stackUUID = getConstructUUID(Stack.of(construct));
    stack = store.getStack(stackUUID);
    }
    } catch {
    // ignore - expected to throw if construct is not contained in a stack
    }
  • Note the catch ignore is targeting CDK managed stuff (like metadata tree) outside of a stack, not resources like in your case.

Code where error occurs:

this.logicalId = props.logicalId;
if (this.logicalId) {
if (this.stack == null) {
throw new Error(
`LogicalId defined outside of stack: ${this.logicalId} - ${String(
this
)}`
);
}
this.store.recordLogicalId(this.stack, this.logicalId, this);

@kornicameister
Copy link
Author

kornicameister commented Jan 22, 2024

@JeremyJonas yes, those tests you asked about, everything seems to be just fine.

@kornicameister
Copy link
Author

kornicameister commented Jan 22, 2024

@JeremyJonas as to what this stack of mine really is. It is simply speaking a class that extends from cdk.Stack and set things up according to my taste. The idea was that no matter what my team decides to take for a spin they always adhere to certain regulations I've laid out among which you can find:

  • auto destruction of testing stacks with some sane defaults and lookup strategies for overrides
  • synthetizing
  • match organization setup

And that's really all that is. For most of the time my stack does not really create extra resources, just sets values later passed into constructor of cdk.Stack. The only time something is created is for testing configuration which is exactly what fails.


Update: I actually made some more tests. Now I noticed that cdk synth + MyStack fails to create a graph with this error:

Error: LogicalId defined outside of stack: StackName - Node:OUTPUT::aysauthapiexampleproductionStackName97FD9A29

So we're failing now on the output which makes me think that whatever construct gets created in my stack is something...flawed.

I don't know what that can be... maybe something messed up on jsii part and packaging the construct/stack?


Update2: I removed the outputs from 1st update and now I get the failure on another resource. Error is exactly the same, just resource ID changes.

@JeremyJonas
Copy link
Contributor

Hi @kornicameister, does Stack.isStack(yourCustomStack) equal true? If it properly extends Stack it should, and that is the way we detect the stack associated with a resource. Please ensure that your custom stack is a Stack, and that the resources within your custom stack return reference to your custom stack when you call Stack.of(failingResource).

Let me know if all these conditions are still true, which means an issue on our end, otherwise means need to provide support for detecting stacks like yours and will need to go a bit deeper into understanding how it is implemented and why it does not get reported as a stack.

@kornicameister
Copy link
Author

kornicameister commented Jan 26, 2024

@JeremyJonas I created couple of tests to verify what you wanted:

it('should return true if stack is a stack', () => {
	expect(cdk.Stack.isStack(stack)).toBe(true);
});
it('should return the same stack from cdk.Stack.of', () => {
  expect(cdk.Stack.of(stack)).toBe(stack);
});
it('should return the same stack from cdk.Stack.of with another resource', () => {
   const ttl = new TimeToLive(stack, 'TTL', {
      ttl: cdk.Duration.hours(1),
    });
  expect(cdk.Stack.of(ttl)).toBe(stack);
});

here's the result:

✓ should return true if stack is a stack (6 ms)
✓ should return the same stack from cdk.Stack.of with another resource (6 ms)
✓ should return the same stack from cdk.Stack.of (6 ms)

@JeremyJonas
Copy link
Contributor

Hi @kornicameister, was away for a bit sorry for delay. Your test disproves my initial thoughts on this one. Without being able to repro this not sure how to resolve. Any chance you could scrap together an example repro of it? You could also run in inspect mode with breakpoint at the error, maybe can provide more details, which would be my first step if running your repro code.

If all else fails, we could try adding a flag to ignore the error. But I am definitely curious to know what is actually causing it.

@JeremyJonas JeremyJonas self-assigned this Feb 11, 2024
@kornicameister
Copy link
Author

I can try, but wonder if that's gonna work out.

@JeremyJonas one thing... can a fact that this error occurs when using said stack of mine via NPM package.
You see I have a library of re-usable CDK components that I ship via NPM package.
Might be I have something messed up on this end?

@mteichtahl
Copy link
Contributor

mteichtahl commented Feb 13, 2024 via email

@kornicameister
Copy link
Author

@mteichtahl I am not using projen. My setup is based on top of pure jsii + lerna for mono-repo support.

That's my package.json (note: redacted):

{
  "name": "xxx",
  "version": "0.0.0",
  "description": "Collection of CDK components",
  "license": "UNLICENSED",
  "main": "dist/index.js",
  "scripts": {
    "build": "jsii -vvv --compress-assembly --fix-peer-dependencies",
    "postbuild": "npm run docgen",
    "build:watch": "jsii -w",
    "docgen": "npx rimraf README.md && jsii-docgen -o README.md -r",
    "prepackage": "npm run build",
    "package": "npx rimraf dist/ && jsii-pacmak -vvv",
    "test:unit": "jest",
    "test:unit:watch": "jest --watch --watchman"
  },
  "types": "dist/index.d.ts",
  "dependencies": {
    "@cloudcomponents/cdk-temp-stack": "^2.1.0"
  },
  "peerDependencies": {
    "aws-cdk-lib": "^2.98",
    "constructs": "^10.3.0"
  },
  "devDependencies": {
    "aws-cdk-lib": "2.115.0",
    "constructs": "10.3.0",
    "jsii": "~5.3.7",
    "jsii-diff": "^1.94.0",
    "jsii-docgen": "^10.3.7",
    "jsii-pacmak": "^1.94.0",
    "jsii-reflect": "^1.94.0",
    "jsii-rosetta": "~5.3.4"
  },
  "engines": {
    "node": ">=18.16.0"
  },
  "jsii": {
    "outdir": "dist",
    "targets": {},
    "tsc": {
      "outDir": "dist",
      "rootDir": "src"
    },
    "versionFormat": "short"
  }
}

I do not have any other jsii configuration file for that.
Build seems to be running great and I can use package without any issue in other repositories.

@andreaspoldi
Copy link

+1, same thing happens simply with EKS L3 construct

`import { Stack, StackProps } from "aws-cdk-lib";
import { Construct } from "constructs";
import { KubectlV29Layer } from '@aws-cdk/lambda-layer-kubectl-v29';
import * as eks from 'aws-cdk-lib/aws-eks';

export class ApplicationStack extends Stack {
constructor(scope: Construct, id: string, props?: StackProps) {
super(scope, id, props);

// provisioning a cluster
const cluster = new eks.Cluster(this, 'hello-eks', {
  version: eks.KubernetesVersion.V1_29,
  kubectlLayer: new KubectlV29Layer(this, 'kubectl'),
});

// apply a kubernetes manifest to the cluster
cluster.addManifest('mypod', {
  apiVersion: 'v1',
  kind: 'Pod',
  metadata: { name: 'mypod' },
  spec: {
    containers: [
      {
        name: 'hello',
        image: 'paulbouwer/hello-kubernetes:1.5',
        ports: [ { containerPort: 8080 } ],
      },
    ],
  },
});

}
}`

running 'npm run build' throws

/Users/andrea.spoldi/architects/pdk/prova/node_modules/@aws/pdk/cdk-graph/core/graph.ts:1173 throw new Error( ^ Error: LogicalId defined outside of stack: Handler886CB40B - Node:CFN_RESOURCE::infradevawscdkawseksKubectlProviderHandler14CEB732

@kornicameister
Copy link
Author

Not that I am happy...but I am happy that we have something to reproduce an error.

I tried reproducing that but without any luck.

@sebastianrothbucher
Copy link

I have the same situation with EKS higher-level constructs. After some debugging, I found a temp solution (i.e. a hack), don't have a lasting idea yet. And it only seems to be a problem when using PDK along with CDK.

Here's the patch that gets things going again - though not great:

--- node_modules/@aws/pdk/cdk-graph/core/compute.orig   2024-04-11 20:35:50
+++ node_modules/@aws/pdk/cdk-graph/core/compute.js     2024-04-11 19:53:19
@@ -101,6 +101,7 @@
                 node = new Graph.StackNode(nodeProps);
                 break;
             }
+            case 'aws-cdk-lib.aws_eks.KubectlProvider':
             case types_1.ConstructInfoFqnEnum.NESTED_STACK: {
                 // NB: handle NestedStack<->CfnStack as single Node with NestedStack construct as source
                 // https://github.com/aws/aws-cdk/blob/119c92f65bf26c3fdf4bb818a4a4812022a3744a/packages/%40aws-cdk/core/lib/nested-stack.ts#L119
\ No newline at end of file

@sebastianrothbucher
Copy link

Reason seems to be: classes having custom fqn are instanceof NestedStack, but the info gets lost here

@sebastianrothbucher
Copy link

btw: it only appears with PDK and automatic diagram generation

@kornicameister
Copy link
Author

@sebastianrothbucher did you manage to get a fix? need some more info?

@sebastianrothbucher
Copy link

I'm good with the fix above, thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog bug Something isn't working needs-triage
Projects
None yet
Development

No branches or pull requests

5 participants