fix(core): cross stack references to string lists break #22873

comcalvi · 2022-11-11T00:10:57Z

Previously only strings could be referenced across stacks, due to a CloudFormation requirement that all Outputs evaluate to strings. This PR allows string lists to be referenced across stacks, like VpcEndpoint DnsEntries, by wrapping the exports / imports in Fn::Splits and Fn::Joins. This does not work across environments.

This also makes ssm.StringListParameter use Fn.split(value) instead of value.split().

This is part of the fix to #21682. Cross-env support will be added in a future PR.

Adds a new public method, exportListValue(), to the Stack class to avoid breaking changes.

Implementation requires adding type hints to IResolvable and modifying the codegen to populate them correctly.

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

gitpod-io · 2022-11-11T00:11:03Z

packages/@aws-cdk/aws-ssm/lib/parameter.ts

packages/@aws-cdk/core/lib/private/refs.ts

packages/@aws-cdk/core/lib/stack.ts

packages/@aws-cdk/core/test/stack.test.ts

packages/@aws-cdk/aws-cloudformation/test/integ.core-cross-stack-string-list-references.ts

packages/@aws-cdk/aws-ec2/test/vpc.test.ts

packages/@aws-cdk/core/lib/stack.ts

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

rix0rrr

Some polishing but conditionally approved

rix0rrr · 2022-12-06T09:17:58Z

packages/@aws-cdk/core/lib/cfn-output.ts

+    } else if (Array.isArray(props.value)) {
+      // `props.value` is a string, but because cross-stack exports allow passing any,
+      // we need to check for lists here.
+      throw new Error(`CloudFormation output was given a string list instead of a string at path "${this.node.path}"`);


Does this definitely fail in CloudFormation, or does it implicitly join or something?

it definitely fails

Template format error: The Value field of every Outputs member must evaluate to a String.

rix0rrr · 2022-12-06T09:20:08Z

packages/@aws-cdk/core/lib/private/refs.ts


-  const importExpr = exportingStack.exportValue(reference);
+  if (Array.isArray((exportingStack as any).determineImportValue(reference))) {


Is this not the moment where we would inspect the type hint of reference ?

rix0rrr · 2022-12-06T09:26:29Z

packages/@aws-cdk/core/lib/stack.ts

+    if (Array.isArray(importValue)) {
+      throw new Error('Attempted to export a list value from `exportValue()`: use `exportStringListValue()` instead');
+    }


For some reason this makes me uneasy. Can you do inspections on the value's type and type hints, before we go to export it, instead?

rix0rrr · 2022-12-06T09:26:40Z

packages/@aws-cdk/core/lib/stack.ts

+    const importValue = this.determineImportValue(exportedValue);
+
+    if (!Array.isArray(importValue)) {
+      throw new Error('Attempted to export a string value from `exportStringListValue()`: use `exportValue()` instead');


For some reason this makes me uneasy. Can you do inspections on the value's type and type hints, before we go to export it, instead?

mergify · 2022-12-06T22:59:35Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2022-12-07T16:13:11Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2022-12-07T21:05:05Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

TheRealAmazonKendra · 2022-12-07T23:47:40Z

@Mergifyio requeue

mergify · 2022-12-07T23:48:14Z

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

aws-cdk-automation · 2022-12-08T00:26:47Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 8bef0bb
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2022-12-08T00:26:56Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Previously only strings could be referenced across stacks, due to a CloudFormation requirement that all `Output`s evaluate to strings. This PR allows string lists to be referenced across stacks, like [VpcEndpoint DnsEntries](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ec2-vpcendpoint.html#aws-resource-ec2-vpcendpoint-return-values), by wrapping the exports / imports in `Fn::Split`s and `Fn::Join`s. This does not work across environments. This also makes `ssm.StringListParameter` use `Fn.split(value)` instead of `value.split()`. This is part of the fix to aws#21682. Cross-env support will be added in a future PR. Adds a new public method, `exportListValue()`, to the `Stack` class to avoid breaking changes. Implementation requires adding type hints to `IResolvable` and modifying the codegen to populate them correctly. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

comcalvi added 7 commits November 7, 2022 07:15

working linked app example, but still needs testing + edge cases

5cce6a1

tmp

dcc3465

working intra-env

f177221

integ test + ssm bugfix

c2364dd

integ

1491766

undo

063590a

edge case

bc71472

aws-cdk-automation requested a review from a team November 11, 2022 00:11

github-actions bot added the p2 label Nov 11, 2022

mergify bot added the contribution/core This is a PR that came from AWS. label Nov 11, 2022

comcalvi commented Nov 11, 2022

View reviewed changes

packages/@aws-cdk/aws-ssm/lib/parameter.ts Show resolved Hide resolved

comcalvi requested review from corymhall and rix0rrr November 11, 2022 19:34

changed return type of to any

183d70c

comcalvi force-pushed the crossStack branch from ad0fdaa to 183d70c Compare November 11, 2022 21:06

comcalvi added 4 commits November 14, 2022 10:18

remove breaking change

3b9fa84

docs

9a055ce

synta

e065be7

docs

4129840

comcalvi changed the title ~~fix(core): cross stack references to string lists~~ fix(core): cross stack references to string lists break Nov 14, 2022

rix0rrr requested changes Nov 15, 2022

View reviewed changes

comcalvi added 2 commits November 15, 2022 14:51

comments

a7b9e1f

leftover

468cd82

rix0rrr reviewed Nov 16, 2022

View reviewed changes

packages/@aws-cdk/aws-ec2/test/vpc.test.ts Show resolved Hide resolved

rix0rrr reviewed Nov 16, 2022

View reviewed changes

packages/@aws-cdk/core/lib/stack.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/core/lib/stack.ts Outdated Show resolved Hide resolved

comcalvi added 4 commits November 16, 2022 08:03

remove Fn::Ref (it's not valid CFN)

ecc6aab

added test for name

9e454c3

global const

8b5239c

outputs will throw if given string lists

c6110c6

remove more unused type hints

cd2f636

comcalvi added 8 commits December 2, 2022 11:06

clean up

3d600db

rename

95914ff

remove from iam

8933739

clean up

d324356

comments

e6ae434

update genspec to use correct enum

2cfa06a

comment

b571cd9

update output error message

c1e1c25

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Dec 6, 2022

rix0rrr approved these changes Dec 6, 2022

View reviewed changes

comments

caf451f

comcalvi removed the pr/do-not-merge This PR should not be merged at this time. label Dec 6, 2022

Merge branch 'main' into crossStack

f23fbdb

Merge branch 'main' into crossStack

8bef0bb

mergify bot merged commit 3ddb8cf into aws:main Dec 8, 2022

comcalvi mentioned this pull request Jan 5, 2023

HostedZone: Referencing array attributes cross-stack breaks with Template Format Error #22846

Closed

comcalvi mentioned this pull request Jan 25, 2023

Automatic token exports can not be arrays #5897

Closed

comcalvi mentioned this pull request Feb 27, 2023

(core): CDK creates unsupported list outputs from cross-stack references #21682

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): cross stack references to string lists break #22873

fix(core): cross stack references to string lists break #22873

comcalvi commented Nov 11, 2022 •

edited

Loading

gitpod-io bot commented Nov 11, 2022

rix0rrr left a comment

rix0rrr Dec 6, 2022

comcalvi Dec 6, 2022

comcalvi Dec 6, 2022

rix0rrr Dec 6, 2022

rix0rrr Dec 6, 2022

rix0rrr Dec 6, 2022

mergify bot commented Dec 6, 2022

mergify bot commented Dec 7, 2022

mergify bot commented Dec 7, 2022

TheRealAmazonKendra commented Dec 7, 2022

mergify bot commented Dec 7, 2022

aws-cdk-automation commented Dec 8, 2022

mergify bot commented Dec 8, 2022


		const importExpr = exportingStack.exportValue(reference);
		if (Array.isArray((exportingStack as any).determineImportValue(reference))) {

fix(core): cross stack references to string lists break #22873

fix(core): cross stack references to string lists break #22873

Conversation

comcalvi commented Nov 11, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Nov 11, 2022

rix0rrr left a comment

Choose a reason for hiding this comment

rix0rrr Dec 6, 2022

Choose a reason for hiding this comment

comcalvi Dec 6, 2022

Choose a reason for hiding this comment

comcalvi Dec 6, 2022

Choose a reason for hiding this comment

rix0rrr Dec 6, 2022

Choose a reason for hiding this comment

rix0rrr Dec 6, 2022

Choose a reason for hiding this comment

rix0rrr Dec 6, 2022

Choose a reason for hiding this comment

mergify bot commented Dec 6, 2022

mergify bot commented Dec 7, 2022

mergify bot commented Dec 7, 2022

TheRealAmazonKendra commented Dec 7, 2022

mergify bot commented Dec 7, 2022

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

aws-cdk-automation commented Dec 8, 2022

AWS CodeBuild CI Report

mergify bot commented Dec 8, 2022

comcalvi commented Nov 11, 2022 •

edited

Loading