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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cyclic Reference Error (test-cdk-dev/test-cdk-dev-ecs.AWS::URLSuffix) would create a cyclic reference. #3970

Closed
btotharye opened this issue Sep 6, 2019 · 9 comments 路 Fixed by #4011
Closed

Cyclic Reference Error (test-cdk-dev/test-cdk-dev-ecs.AWS::URLSuffix) would create a cyclic reference. #3970

btotharye opened this issue Sep 6, 2019 · 9 comments 路 Fixed by #4011
Assignees
@rix0rrr
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container @aws-cdk/core Related to core CDK functionality bug This issue is a bug. language/python Related to Python bindings

Comments

@btotharye
Copy link

btotharye commented Sep 6, 2019 •
edited

馃悰 Bug Report

What is the problem?

So when trying to setup a multi stack setup it works fine until I add some parts in around ecs.FargateTaskDefinition requirement then I get the following error:

jsii.errors.JavaScriptError:
  Error: 'test-cdk-dev/test-cdk-dev-ecs' depends on 'test-cdk-dev' (test-cdk-dev/test-cdk-dev-ecs/DefaultTargetGroup/Resource -> test-cdk-dev/test-cdk-dev-vpc/Resource.Ref). Adding this dependency (test-cdk-dev/test-cdk-dev-ecs/ECSFargateTask/Resource -> test-cdk-dev/test-cdk-dev-ecs.AWS::URLSuffix) would create a cyclic reference.
      at Stack.addDependency (/private/var/folders/47/mkh399hd7wb7zfr_jqnd9pqw0000gp/T/jsii-kernel-2fluXy/node_modules/@aws-cdk/core/lib/stack.js:147:19)
      at CfnReference.consumeFromStack (/private/var/folders/47/mkh399hd7wb7zfr_jqnd9pqw0000gp/T/jsii-kernel-2fluXy/node_modules/@aws-cdk/core/lib/private/cfn-reference.js:105:28)
      at Stack.prepare (/private/var/folders/47/mkh399hd7wb7zfr_jqnd9pqw0000gp/T/jsii-kernel-2fluXy/node_modules/@aws-cdk/core/lib/stack.js:350:31)
      at Function.prepare (/private/var/folders/47/mkh399hd7wb7zfr_jqnd9pqw0000gp/T/jsii-kernel-2fluXy/node_modules/@aws-cdk/core/lib/construct.js:89:27)
      at Function.synth (/private/var/folders/47/mkh399hd7wb7zfr_jqnd9pqw0000gp/T/jsii-kernel-2fluXy/node_modules/@aws-cdk/core/lib/construct.js:52:14)
      at App.synth (/private/var/folders/47/mkh399hd7wb7zfr_jqnd9pqw0000gp/T/jsii-kernel-2fluXy/node_modules/@aws-cdk/core/lib/app.js:67:52)
      at .env/lib/python3.7/site-packages/jsii/_embedded/jsii/jsii-runtime.js:6498:51
      at Kernel._wrapSandboxCode (.env/lib/python3.7/site-packages/jsii/_embedded/jsii/jsii-runtime.js:7134:19)
      at .env/lib/python3.7/site-packages/jsii/_embedded/jsii/jsii-runtime.js:6498:25
      at Kernel._ensureSync (.env/lib/python3.7/site-packages/jsii/_embedded/jsii/jsii-runtime.js:7102:20)
      at Kernel.invoke (.env/lib/python3.7/site-packages/jsii/_embedded/jsii/jsii-runtime.js:6497:26)
      at KernelHost.processRequest (.env/lib/python3.7/site-packages/jsii/_embedded/jsii/jsii-runtime.js:6191:28)
      at KernelHost.run (.env/lib/python3.7/site-packages/jsii/_embedded/jsii/jsii-runtime.js:6137:14)
      at .env/lib/python3.7/site-packages/jsii/_embedded/jsii/jsii-runtime.js:6137:45
      at KernelHost.processRequest (.env/lib/python3.7/site-packages/jsii/_embedded/jsii/jsii-runtime.js:6233:16)
      at KernelHost.run (.env/lib/python3.7/site-packages/jsii/_embedded/jsii/jsii-runtime.js:6137:14)
      at .env/lib/python3.7/site-packages/jsii/_embedded/jsii/jsii-runtime.js:6137:45
      at KernelHost.processRequest (.env/lib/python3.7/site-packages/jsii/_embedded/jsii/jsii-runtime.js:6233:16)

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "app.py", line 33, in <module>
    app.synth()
  File ".env/lib/python3.7/site-packages/aws_cdk/core/__init__.py", line 2840, in synth
    return jsii.invoke(self, "synth", [])
  File ".env/lib/python3.7/site-packages/jsii/_kernel/__init__.py", line 104, in wrapped
    return _recursize_dereference(kernel, fn(kernel, *args, **kwargs))
  File ".env/lib/python3.7/site-packages/jsii/_kernel/__init__.py", line 267, in invoke
    args=_make_reference_for_native(self, args),
  File ".env/lib/python3.7/site-packages/jsii/_kernel/providers/process.py", line 346, in invoke
    return self._process.send(request, InvokeResponse)
  File ".env/lib/python3.7/site-packages/jsii/_kernel/providers/process.py", line 316, in send
    raise JSIIError(resp.error) from JavaScriptError(resp.stack)
jsii.errors.JSIIError: 'test-cdk-dev/test-cdk-dev-ecs' depends on 'test-cdk-dev' (test-cdk-dev/test-cdk-dev-ecs/DefaultTargetGroup/Resource -> test-cdk-dev/test-cdk-dev-vpc/Resource.Ref). Adding this dependency (test-cdk-dev/test-cdk-dev-ecs/ECSFargateTask/Resource -> test-cdk-dev/test-cdk-dev-ecs.AWS::URLSuffix) would create a cyclic reference.
Subprocess exited with error 1

I have example code in a gist at https://gist.github.com/btotharye/d9de3ed45b17cc4ba87ea7d902b8ec65 if you look at the ecs_stack.py file you will see that on lines 49-73 if I remove this part everything works fine, but if I add that part back in I get the URLSuffix cylic error and I'm not sure why.

Reproduction Steps

I basically can do a cdk diff '*' -c stage=dev on the gist at https://gist.github.com/btotharye/d9de3ed45b17cc4ba87ea7d902b8ec65 I have omitted the cdk.json for some special values but its nothing special just props I pass in.

Verbose Log

Environment

  • CDK CLI Version: 1.6.1 (build a09203a)
  • Module Version: aws_ecs.FargateTaskDefinition
  • OS: OSX Mojave
  • Language: Python

Other information
@btotharye btotharye added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 6, 2019
@SomayaB SomayaB added language/python Related to Python bindings @aws-cdk/aws-ecs Related to Amazon Elastic Container labels Sep 6, 2019
@iamhopaul123
Copy link
Contributor

Hi @btotharye,

This might not be a bug and the error will not appear if you put api_repo to "vpc_stack.py" and pass the ECR repository as a parameter to EcsStack, so that the cyclic reference problem will be solved.

Also a nitpick: pearson_vpn_connection should be vpn_connection

@btotharye
Copy link
Author

Ill test and that was just something I forgot to remove to hide things...

@btotharye
Copy link
Author

Also @iamhopaul123 is there any easier way than having to pass all these objects like I am or is that my only way?

@iamhopaul123
Copy link
Contributor

Not so sure about that since I personally mostly use TypeScript. However, from my experience using TypeScript, you might also try putting them (VpcStack, EcsStack) together if the construct is not that complicated. However, if you want to separate them out. I need to do more investigation about the dependency rule.

@iamhopaul123
Copy link
Contributor

@btotharye And also putting them into two separate files will actually results in two CFN templates (two different stacks). Not sure if that's what you want. I would suggest use one stack will be much easier.

@btotharye
Copy link
Author

@iamhopaul123 I only started down the multi stack path because on a recent call AWS said it would be better to do it this way and split up the VPC and such into one stack, then have the ECS etc be in another stack.

I'm still not sure how the ECR image location in this circumstance would also cause the cyclic reference? I'm going to update it and see if it fixes it by moving it to the vpc stack it was just odd IMO.

Can you shed any light why the ECR repo in this case would cause the cyclic reference?

@btotharye
Copy link
Author

So updating the stack and moving the ECR reference to the vpc stack resolved the cyclic issue but still not sure why that had to be there necessarily, maybe I don't understand fully how some of the multi stack stuff and dependencies work.

@rix0rrr rix0rrr added @aws-cdk/core Related to core CDK functionality and removed needs-triage This issue or PR still needs to be triaged. labels Sep 9, 2019
@iamhopaul123
Copy link
Contributor

@btotharye From my understanding it seems like when you do a nested stack, all the resources of the nested stack has to come from the parent stack. And this might be the reason why you cannot put ECR reference in the nested stack because AWS::URLSuffix does not come from its parent. However, I don't think nested stacks is currently supported in CDK (see here).

Also I think the most common use cases for multiple stacks are deploying in different regions (see here), or separate staging/QA instances of the same app (see here).

@rix0rrr
Copy link
Contributor

rix0rrr commented Sep 10, 2019

I think this might be an issue with the stack object losing its identity when being passed over the jsii barrier. Seeing the last line of the error message:

[...] Adding this dependency (serviceName-dev/serviceName-dev-ecs/ECSFargateTask/Resource -> serviceName-dev/serviceName-dev-ecs.AWS::URLSuffix) would create a cyclic reference.

In this line, it looks like the serviceName-dev-ecs stack is taking a dependency on itself. Have to investigate further though.

rix0rrr added a commit that referenced this issue Sep 10, 2019
@rix0rrr
fix(core): stack.urlSuffix is no longer scoped
5a760c9 
`stack.urlSuffix` used to be a scoped Token. By the use of roles defined
in another stack (using a `ServicePrincipal` which uses a `urlSuffix`
token), this could lead to unintentional stack references.

Two changes here:

* URL Suffix (seems to) only change when Partition changes. Since
  Partition is unscoped (cross-partition references won't work anyway),
  we might as well make URL Suffix unscoped too.
* `ServicePrincipalToken` should not have used the stack's `urlSuffix`,
  but constructed an unscoped `URL_SUFFIX` itself, since it was
  never intended to potentially create a cross-stack reference. It
  couldn't have, since it doesn't know where it is being defined,
  it just knows where it's being used.

Technically, the second change isn't necessary anymore after we
apply the first, but I made both anyway since the bug is still
resolved even if find out we need roll back the first change
because of a future region build.

Fixes #3970.
@rix0rrr rix0rrr mentioned this issue Sep 10, 2019
Merged
@mergify mergify bot closed this as completed in #4011 Sep 13, 2019
mergify bot pushed a commit that referenced this issue Sep 13, 2019
@rix0rrr @mergify
fix(core): stack.urlSuffix is no longer scoped (#4011)
82e08bc 
`stack.urlSuffix` used to be a scoped Token. By the use of roles defined
in another stack (using a `ServicePrincipal` which uses a `urlSuffix`
token), this could lead to unintentional stack references.

Two changes here:

* URL Suffix (seems to) only change when Partition changes. Since
  Partition is unscoped (cross-partition references won't work anyway),
  we might as well make URL Suffix unscoped too.
* `ServicePrincipalToken` should not have used the stack's `urlSuffix`,
  but constructed an unscoped `URL_SUFFIX` itself, since it was
  never intended to potentially create a cross-stack reference. It
  couldn't have, since it doesn't know where it is being defined,
  it just knows where it's being used.

Technically, the second change isn't necessary anymore after we
apply the first, but I made both anyway since the bug is still
resolved even if find out we need roll back the first change
because of a future region build.

Fixes #3970.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rix0rrr rix0rrr

Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container @aws-cdk/core Related to core CDK functionality bug This issue is a bug. language/python Related to Python bindings
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(core): stack.urlSuffix is no longer scoped aws/aws-cdk
4 participants
@rix0rrr @btotharye @SomayaB @iamhopaul123