aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.71k stars 3.93k forks source link

(amplify-alpha): `customResponseHeaders` does not support monorepo structures #31758

Open georeeve opened 1 month ago

georeeve commented 1 month ago

Describe the bug

Firstly, I can't tell if this is a CDK or Amplify issue. aws-amplify/amplify-hosting#1995 seems related, but someone in that issue was using customHttp.yml which we can't do on CDK.

I've tried to condense this into a simple example app below. It works without the customResponseHeaders property. However, when I add customResponseHeaders, it still deploys via CloudFormation, but when the Amplify build runs I get the errors below.

Regression Issue

Last Known Working CDK Version

No response

Expected Behavior

The app deploys successfully on Amplify.

Current Behavior

The app errors on Amplify deploy:

!!!Monorepo spec provided without "applications" key
!!!Unable to save headers: CustomerError: Monorepo spec provided without "applications" key

Reproduction Steps

Example Amplify app:

const amplifyApp = new AmplifyApp(this, `AmplifyApp`, {
  buildSpec: BuildSpec.fromObjectToYaml({
    version: "1.0",
    applications: [
      {
        appRoot: "frontend",
        frontend: {
          phases: {
            preBuild: {
              commands: [
                "npm install -g pnpm",
                "pnpm install --frozen-lockfile",
              ],
            },
            build: {
              commands: ["pnpm run build"],
            },
          },
          artifacts: {
            baseDirectory: ".next",
            files: ["**/*"],
          },
          cache: {
            paths: [".next/cache/**/*"],
          },
        },
      },
    ],
  }),
  customResponseHeaders: [
    {
      pattern: "**",
      headers: {
        Test: "test",
      },
    },
  ],
});

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.162.1 (build 10aa526)

Framework Version

2.162.1-alpha.0

Node.js Version

v22.9.0

OS

macOS 14.7 (23H124)

Language

TypeScript

Language Version

TypeScript 5.6.3

Other information

No response

georeeve commented 1 month ago

So it seems like CDK is creating this custom headers yaml, which is fine for regular applications:

customHeaders:
  - pattern: "**"
    headers:
      - key: Test
        value: test

However, for monorepos, this custom headers yaml is required:

applications:
  - appRoot: frontend
    customHeaders:
      - pattern: "**"
        headers:
          - key: Test
            value: test

When I change the yaml in the console, this passes the Amplify deployment. So should we add an optional appRoot property to CDK to generate the monorepo yaml? Something like:

customResponseHeaders: [
    {
        appRoot: 'frontend',
        pattern: '**',
        headers: {
            'Test': 'test',
        },
    },
],

I'm happy to try and submit a PR if so.

ashishdhingra commented 1 month ago

@georeeve Could you please share the following:

Thanks, Ashish

georeeve commented 1 month ago

@ashishdhingra My apologies, I do import { App as AmplifyApp } from '@aws-cdk/aws-amplify-alpha'; in my code due to import naming conflicts with other packages I use.

I've tested this further and my first reply comment sums up what is actually happening. Currently the amplify package does not support adding custom response headers for monorepo structures due to the differences in yaml I outlined above. I've actually made a few changes to the amplify package locally and gotten it working for monorepos, I should be able to get a PR submitted tomorrow if you're willing to take a look?

georeeve commented 1 month ago

Submitted as a draft now: #31771

ashishdhingra commented 1 month ago

@georeeve Thanks for your response. Per CDK Amplify API doc, it does support Adding custom response headers. Please advise if you have tried using the customResponseHeaders property instead of specifying these in buildSpec.

The PR would be reviewed by CDK team on incoming priority.

Thanks, Ashish

georeeve commented 1 month ago

@ashishdhingra I'm a bit confused, I was specifying it in the customResponseHeaders above.

For clarity, Amplify supports two main repository structures. A single repository structure (non-monorepo) and a monorepo structure (see https://docs.amplify.aws/react/deploy-and-host/fullstack-branching/monorepos/). CDK does support setting custom response headers through the customResponseHeaders property, but currently only for non-monorepo structures, as in the background CDK just converts this to the first example code block in this YAML spec: https://docs.aws.amazon.com/amplify/latest/userguide/custom-header-YAML-format.html

My PR additionally adds support for monorepo structures, by converting it to the second example code block above. Given that this limitation is not explicitly stated in the docs, I'm guessing it's an oversight of the initial implementation. @ayush987goyal are you able to provide any more info? Looking through previous PRs, you added the property in #17102.

As an aside, I think you can specify it in the buildSpec instead, but it seems like that's deprecated: https://docs.aws.amazon.com/amplify/latest/userguide/migrate-custom-headers.html

ashishdhingra commented 1 month ago

@georeeve Thanks for clarification and apologies for confusion.