pass valid props to root stack #1675

renevatium · 2024-06-24T08:51:27Z

Problem

Currently, it is not possible to pass instantiation props to the Amplify root CDK Stack, and, by convention, any Nested Stack created through backend.createStack. This prevents certain resource deployments: An EdgeFunction for instance requires the region to be explicitly set. Furthermore, while Nested Stacks inherit their region from their parent stack, Amplify has a single root node convention, so multiple root stacks are not an option. Evaluation in CDK appears to be strict so the only way to do deploy an EdgeFunction from an Amplify project looks to be at instantiation of an explicit region on the root Stack.

Note: I used 'main' instead of 'root' because that is the naming convention used in the current code.

See #1663 for more information on trying to deploy an EdgeFunction with current version.

Changes

This implementation specifically avoids breaking changes, but does require argument mutation. The relevant args are not typed as const so this may be fine, but if it goes against an established convention, let me know as I can update it easily enough. This was needed because the relevant calls are being made as default argument values in both BackendFactory constructor and createDefaultStack respectively.

This PR introduces a mainStackProps argument to the defineBackend function. This gets passed down the callstack and used to define the root Stack in the AmplifyStack class.

Props for root CDK Stack

AWS documentation for aws-cdk-lib.StackProps

Props are picked to ensure explicit addition of new StackProps is required.
- description?: string
- env?: Environment
- tags?: { [key: string]: string }
- analyticsReporting?: boolean
- crossRegionReferences?: boolean
- suppressTemplateIndentation?: boolean
StackProps incompatible with Amplify's intended hierarchy, build or deployment processes are omitted:
- stackName: Conflicts with dynamic resource naming.
- synthesizer: Conflicts with managed deployments and resource references.
- terminationProtection: Conflicts with sandbox/app delete.
- permissionsBoundary: Conflicts with single root Stack ethos (i.e. Unable to create Role prior to defineBackend).

Props are passed down from defineBackend. e.g:

defineBackend({
  auth,
  storage,
}, {
  description: 'test-description',
  env: {
    region: 'us-east-1',
    account: '[YOUR_ACCOUNT_ID]'
  },
  tags: {
    'test-tag': 'test-tag-value',
  },
  analyticsReporting: true,
  crossRegionReferences: true,
  suppressTemplateIndentation: true,
})

Validation

All existing unit, e2e and deployment tests pass. Coverage is unchanged.

Using the npm proxy, I have fully deployed and tested the EdgeFunction case with various configurations.

This change simply passes props tested in aws-cdk-lib/core/test/stack.test.ts. i.e It already follows defined convention. As such, I am not sure what automated testing would actually be useful. I did try though, so please let me know if you would like me to some tests back in?

I started out writing unit tests, but struggled to find a way to validate anything useful there. I could potentially check that ${AWS::Region} does not exist in the generated Stack Template but that seems like a stretch.

I wrote e2e and deployment tests, but they don't really do much to actually test anything in scope and they add a lot of unnecessary overhead. The deployment tests for instance, require that I update the test stack generation process and/or write the tests without following the conventions established by the existing test structure. Again, if you would prefer these be in, I can put them back.

Checklist

[] If this PR includes a functional change to the runtime behavior of the code, I have added or updated automated test coverage for this change.
[] If this PR requires a change to the Project Architecture README, I have included that update in this PR.
[] If this PR requires a docs update, I have linked to that docs PR above.
[] If this PR modifies E2E tests, makes changes to resource provisioning, or makes SDK calls, I have run the PR checks with the run-e2e label set.

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

changeset-bot · 2024-06-24T08:51:31Z

🦋 Changeset detected

Latest commit: 898a4cc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@aws-amplify/backend	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

sobolk · 2024-06-24T15:32:02Z

packages/backend/API.md

@@ -70,7 +71,7 @@ export { ConstructFactoryGetInstanceProps }
 export { defineAuth }

 // @public
-export const defineBackend: <T extends DefineBackendProps>(constructFactories: T) => Backend<T>;
+export const defineBackend: <T extends DefineBackendProps>(constructFactories: T, mainStackProps?: MainStackProps) => Backend<T>;


We should prepare this API for further expansion. I.e. main stack props might not be the only extension we do in the future.

Therefore, we should have defineBackend(constructFactories T, props?: DefineBackendProps)
and then DefineBackendProps should have mainStackProps: MainStackProps as optional property.

Have updated in commits/93b77dc.

sobolk · 2024-06-24T15:33:02Z

packages/backend/API.md

@@ -89,6 +90,9 @@ export { GenerateContainerEntryProps }

 export { ImportPathVerifier }

+// @public
+export type MainStackProps = Pick<StackProps, 'description' | 'env' | 'tags' | 'analyticsReporting' | 'crossRegionReferences' | 'suppressTemplateIndentation'>;


Is this minimal set of picked properties to solve the problem? If not, please narrow it.

Wasn't minimal for my specific issue. I have narrowed it in e931b42

sobolk · 2024-06-24T15:35:41Z

packages/backend/src/backend_factory.ts

+    stack?: Stack,
+    mainStackProps?: MainStackProps


stack and mainStackProps should be mutually exclusive.

Have added constructor overloads to ensure this exclusivity in 3b24347

renevatium · 2024-06-24T23:57:58Z

@sobolk Thank you for reviewing my PR.

Please let me know if I should rebase into a single commit at some point or if you prefer the history.

sobolk · 2024-06-25T00:11:31Z

@sobolk Thank you for reviewing my PR.

Please let me know if I should rebase into a single commit at some point or if you prefer the history.

We're using "squash merge" strategy for pull request so rebase is not necessary. It's actually not desired as it makes incremental reviews impossible to track in GitHub UI sometimes.

sobolk · 2024-06-25T00:13:56Z

packages/backend/API.md

+export type DefineBackendConstructFactories = Record<string, ConstructFactory<ResourceProvider & Partial<ResourceAccessAcceptorFactory<never>>>> & {
    [K in keyof BackendBase]?: never;
 };

+// @public (undocumented)
+export type DefineBackendProps = {
+    mainStackProps: MainStackProps;
+};
+


I'm sorry I didn't realize that DefineBackendProps already exist as a type while providing initial feedback.

We should revert back to existing naming there (otherwise this is breaking change) and name new type something different.

Perhaps DefineBackendOptions. @Amplifiyer @edwardfoyle what do you think?

I have changed the new arg to DefineBackendOptions, but can change again easily enough, and DefineBackendConstructFactories has been reverted to DefineBackendProps.

… rename new prop

…into root-stack-props

edwardfoyle · 2024-06-25T15:50:24Z

packages/backend/src/backend_factory.ts

+  constructor(
+    constructFactories: T,
+    stack: Stack,
+    props?: ExplicitOmit<DefineBackendOptions, 'mainStackProps'>


does a type of never here not work?

never would prevent passing anything in props, but only mainStackProps conflicts with a passed Stack. While there are currently no other props in the DefineBackendOptions type, @sobolk requested this implementation for the further expansion of the API and backend options could potentially be used for anything in the process tree, not only for the root Stack config. Semantically it also makes sense to allow DefineBackendOptions to be passed to defineBackend even if a Stack is passed, as long as those options aren't specific to the root Stack definition.

FYI I did try with built-in Omit, but it's not explicit so doesn't prevent passing mainStackProps. Hence the ExplicitOmit implementation.

We should maybe change the props key to options though, for consistency sake. Thoughts?

josefaidt · 2024-06-25T19:26:07Z

Hey @renevatium 👋 thanks for taking the time to file this! We agree this is something we'd like to address, though are actively discussing this internally to gauge the direction.

renevatium · 2024-06-25T19:40:23Z

Hi @josefaidt 👋 No problem at all, big proponent of Amplify and keen to be more a part of the community.

Considering it a point of conversation right now, I'll put my 2 cents in: Currently, this PR is a decent solution for my issue, but if I consider future flexibility, it does seem like exposing the explicit functionality necessary to define the App and Root Stack as independent objects and passing them into the (exposed) BackendFactory may be the most flexible state. I realize this conflicts with convention > config, but with CDK available in gen2, it feels like more custom config options/structure on the backend side will offer devs the most control with the least (necessary) limitation. Also, this would align with the current unit testing methodology, which doesn't use defineBackend, but rather explicitly defines App/Stack objects.

sisygoboom · 2024-08-28T20:59:58Z

Amplify needs this desperately, without it I can't add a function created via defineFunction to a vpc imported into a backend.createStack. Are you any closer to a solution @josefaidt - its been 3 months now?
Error: Cannot retrieve value from context provider vpc-provider since account/region are not specified at the stack level. Configure "env" with an account and region when you define your stack.

pass valid props to root stack

e3dfa31

renevatium marked this pull request as ready for review June 24, 2024 09:02

renevatium requested review from a team as code owners June 24, 2024 09:02

sobolk reviewed Jun 24, 2024

View reviewed changes

renevatium added 6 commits June 24, 2024 23:42

mainStackProps as key in DefineBackendProps

93b77dc

pick only MainStackProps relevant to region handling

e931b42

fix missing update:api for #e931b42

1f79750

mainStackProps in DefineBackendProps should be optional

28a8743

better import path for MainStackProps

93e0222

BackendFactory constructor overloading for conditional prop exclusion

3b24347

sobolk reviewed Jun 25, 2024

View reviewed changes

renevatium added 3 commits June 25, 2024 06:26

revert DefineBackendConstructFactories back to DefineBackendProps and…

279f393

… rename new prop

revert and update backend API doc

5fb4e03

Merge branch 'main' of https://github.com/aws-amplify/amplify-backend …

898a4cc

…into root-stack-props

edwardfoyle reviewed Jun 25, 2024

View reviewed changes

ykethan mentioned this pull request Jul 8, 2024

stacks which use EdgeFunctions must have an explicitly set region #1725

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pass valid props to root stack #1675

pass valid props to root stack #1675

renevatium commented Jun 24, 2024 •

edited

Loading

changeset-bot bot commented Jun 24, 2024 •

edited

Loading

sobolk Jun 24, 2024

renevatium Jun 24, 2024 •

edited

Loading

sobolk Jun 24, 2024

renevatium Jun 24, 2024

sobolk Jun 24, 2024

renevatium Jun 24, 2024

renevatium commented Jun 24, 2024

sobolk commented Jun 25, 2024

sobolk Jun 25, 2024

renevatium Jun 25, 2024 •

edited

Loading

edwardfoyle Jun 25, 2024

renevatium Jun 25, 2024

renevatium Jun 25, 2024

josefaidt commented Jun 25, 2024

renevatium commented Jun 25, 2024

sisygoboom commented Aug 28, 2024

pass valid props to root stack #1675

Are you sure you want to change the base?

pass valid props to root stack #1675

Conversation

renevatium commented Jun 24, 2024 • edited Loading

Problem

Changes

Props for root CDK Stack

Validation

Checklist

changeset-bot bot commented Jun 24, 2024 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

renevatium Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renevatium commented Jun 24, 2024

sobolk commented Jun 25, 2024

Choose a reason for hiding this comment

renevatium Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josefaidt commented Jun 25, 2024

renevatium commented Jun 25, 2024

sisygoboom commented Aug 28, 2024

renevatium commented Jun 24, 2024 •

edited

Loading

changeset-bot bot commented Jun 24, 2024 •

edited

Loading

renevatium Jun 24, 2024 •

edited

Loading

renevatium Jun 25, 2024 •

edited

Loading