Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

AWS APIgateway: fromRestApiAttributes does not bring all the RestAPI configuration #29079

Open
wascou opened this issue Feb 12, 2024 · 21 comments
Labels
@aws-cdk/aws-apigateway Related to Amazon API Gateway bug This issue is a bug. effort/medium Medium work item – several days of effort p2

Comments

@wascou
Copy link

wascou commented Feb 12, 2024

Describe the bug

I was following this documentation section, where there's a workaround for multiple stack describing api gateway resources:

Breaking up Methods and Resources across Stacks
It is fairly common for REST APIs with a large number of Resources and Methods to hit the CloudFormation limit of 500 resources per stack.

So I broke my stack in several nested stacks and imported back my api in each nested stack using the ``fromRestApiAttributes``` method. The correct object was found but not all the configuration appeared.

Expected Behavior

I was expecting the full object as set during the API creation in the main stack.

Current Behavior

Dumping the object, I had the following:

{
...
    defaultIntegration: undefined,
    defaultMethodOptions: undefined,
    defaultCorsPreflightOptions: undefined
...
}

instead of:

{
...
    defaultIntegration: undefined,
    defaultMethodOptions: undefined,
    defaultCorsPreflightOptions: { allowOrigins: [Array], allowMethods: [Array] },
...
}

Reproduction Steps

The following piece of code as mentionned in the documentation:
https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_apigateway-readme.html#breaking-up-methods-and-resources-across-stacks

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.127.0 (build 6c90efc)

Framework Version

No response

Node.js Version

v21.1.0

OS

Mac

Language

TypeScript

Language Version

No response

Other information

No response

@wascou wascou added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 12, 2024
@github-actions github-actions bot added the @aws-cdk/aws-apigateway Related to Amazon API Gateway label Feb 12, 2024
@wascou
Copy link
Author

wascou commented Feb 13, 2024

And trying to pass the resource directly through the props from one stack to another is not working. Any workaround?

@wascou
Copy link
Author

wascou commented Feb 13, 2024

Ok, escape hatch:

const resourceNode: any = api.node.findChild("Default")
resourceNode.defaultCorsPreflightOptions = defaultCorsPreflightOptions

@pahud
Copy link
Contributor

pahud commented Feb 13, 2024

Did you mean you just define defaultCorsPreflightOptions with values but the synthesized value is undefined?

Can you share your full code here so we can focus on the code.

@pahud pahud added p2 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Feb 13, 2024
@wascou
Copy link
Author

wascou commented Feb 13, 2024

No, i did not mean that.

My project is composed of on main stack and two nested stacks, as defined in the example of the Apigateway CDK modul doc.

MyStack

const commonStack = new CommonNestedStack(
      this,
      "CommonNestStack",
      ...
    )
    const realestate = new BusinessNestedStack(
      this,
      "BusinessNestedStack",
      {commonStack}
    )

Common

class CommonNestedStack extends NestedStack {
  api: RestApi
  
  constructor(scope: Construct, id: string, props: CommonNestedStackProps) {
    super(scope, id, props)

    this.api = new RestApi(this, "RestApi", {
      defaultCorsPreflightOptions,
    })
  }
}

Business

export class BusinessNestedStack extends NestedStack {
  public readonly methods: Method[] = []

  constructor(scope: Construct, id: string, props: ResourceNestedStackProps) {
    super(scope, id, props)

    const api = RestApi.fromRestApiAttributes(this, "RestApi", {
      restApiId: props.cs.api.restApiId,
      rootResourceId: props.cs.api.restApiRootResourceId,
    })
    
    //Normally, I should have api.defaultCorsPreflight set like the one I set in CommonNestedStack
    console.log("API", JSON.stringify(api))
    
    //Any extra resource and methods added to this api there will get the default cors options set. 
    
    
    //The following does the trick, but I have to reset the value
    const resourceNode: any = api.node.findChild("Default")
    resourceNode.defaultCorsPreflightOptions = defaultCorsPreflightOptions
  }
}

@martinduris
Copy link

martinduris commented Mar 19, 2024

Hi,

confirm that behavior has changed. OPTIONS is not propagated to resources created in API.

This happens in our project if:

  • RestApi has been created in Stack B
  • RestApi is "imported" (e.g. used for example RestApi.fromRestApiAttributes) and than used (e.g. ....api.root.addResource('abc'))
    A few weeks ago this worked.

@LJunghansCode
Copy link

LJunghansCode commented Mar 26, 2024

+1

I would like to add we experience this issue and are looking for a workaround.

Our endpoints created via resources added to the "imported" api (fromRestApiAttributes) do not create OPTIONS endpoints and don't seem to carry over the cors configuration, which breaks them.

@LJunghansCode
Copy link

And trying to pass the resource directly through the props from one stack to another is not working. Any workaround?

We also "solved" this by passing the resource via props to our nested stack, however this attached our nested stack resources back to the root api stack which defeated the purpose since we wanted to fix our 500 resource limit issue.

@wascou Could you provide more detail on your escape hatch fix

@LJunghansCode
Copy link

LJunghansCode commented Mar 26, 2024

@pahud Adding our code here hoping to help resolve this.

Inside our root stack

// Create our Restapi inside our rootstack
 this.internalApi = new RestApi(this, id, {
      defaultCorsPreflightOptions,
      deploy: false,
    }); 
// Create our ping api (to reproduce this issue)
  const pingApi = new PingApi(this, "PingApiStack", defaultNestedApiStackProps, {
      methodOptions: defaultCognitoAuthorizedSchema,
      apiKeyMethodOptions: defaultApiKeyAuthorizedSchema,
      defaultCorsPreflightOptions: defaultCorsPreflightOptions,
    });
// Create a deployment
const internalApiDeploy = new DeployStack(this, {
      restApiId: this.internalApi.restApiId,
      methods: [
       // other methods
        ...pingApi.methods,
      ],
      restApiName: "InternalApiStack",
      id: "InternalApiStackDeployment",
      api: this.internalApi,
    });    

And inside our ping stack, these methods will NOT deploy with OPTIONS endpoints or with cors config.

const api = RestApi.fromRestApiAttributes(this, "InternalApiStack", {
      restApiId: props.restApiId,
      rootResourceId: props.rootResourceId,
      restApiName: "InternalApi",
    });
const pingResource = api.root.addResource("ping");
const pingLambda = new NodejsFunction(this, "pingapi", {
      entry: join(__dirname, "..", "lambdas", "ping", "ping.ts"),
      handler: "handler",
      memorySize: LAMBDA_DEFAULTS.MEMORY,
      timeout: LAMBDA_DEFAULTS.TIMEOUT,
      runtime: Runtime.NODEJS_16_X,
    });
const ping1Get = pingResource
      .addResource("1")
      .addMethod("GET", new LambdaIntegration(pingLambda), methodOptions);
this.methods.push(...methods);

The ping endpoints deploy like this

image

And methods that we have attached to the resources created from the standard api deploy like this with config correct.

image

@pahud
Copy link
Contributor

pahud commented Mar 28, 2024

Hi

This is because the resources from the imported API have no idea if the API is having the defaultCorsPreflightOptions enabled and will not addCorsPreflight() for it.

If you look at here, behind the scene it's just addCorsPreflight that creates the Cors Options.

if (this.defaultCorsPreflightOptions) {
this.addCorsPreflight(this.defaultCorsPreflightOptions);
}

So it should be very easy to work it around.

Check out my full working sample below:

stacks.ts

export class ApiStack extends Stack {
  readonly api: agw.RestApi;
  readonly defaultCorsPreflightOptions?: agw.CorsOptions;
  constructor(scope: Construct, id: string, props: StackProps) {
    super(scope, id, props);  


    this.defaultCorsPreflightOptions = {
      allowOrigins: [ 'https://amazon.com' ],
      allowHeaders: agw.Cors.DEFAULT_HEADERS,

    };
  
    this.api = new agw.RestApi(this, 'RestAPI', {
      defaultCorsPreflightOptions: this.defaultCorsPreflightOptions,
    });

  }
}

export interface DeployStackProps extends NestedStackProps {
  readonly restApiId: string;
  readonly rootResourceId: string;
  readonly defaultCorsPreflightOptions?: agw.CorsOptions;
}


export class DeployStack extends NestedStack {
  readonly methods: agw.Method[] = [];
  readonly resources: agw.Resource[] = [];
  
  constructor(scope: Construct, props: DeployStackProps) {
    super(scope, 'integ-restapi-import-DeployStack', props);

    const api = agw.RestApi.fromRestApiAttributes(this, 'RestApi', {
      restApiId: props.restApiId,
      rootResourceId: props.rootResourceId,
    });

    const abc = api.root.addResource('abc');
    this.resources.push(abc);

    const dummyIntegration = new agw.Integration({
      type: agw.IntegrationType.MOCK,
    });

    this.methods.push(abc.addMethod('GET', dummyIntegration, {}))
    

    const deployment = new agw.Deployment(this, 'Deployment', { api });

    for (const method of this.methods) {
      deployment.node.addDependency(method)
    }

    if(props.defaultCorsPreflightOptions) {
      for (const resource of this.resources) {
        resource.addCorsPreflight(props.defaultCorsPreflightOptions)
      };
    }
    
  }
}

app.ts

const app = new App();

const env = { region: process.env.CDK_DEFAULT_REGION, account: process.env.CDK_DEFAULT_ACCOUNT };

const apiStack = new ApiStack(app, 'ApiStack', { env });

new DeployStack(apiStack, {
    restApiId: apiStack.api.restApiId,
    rootResourceId: apiStack.api.root.resourceId, 
    defaultCorsPreflightOptions: apiStack.defaultCorsPreflightOptions,
})

You got it :-)

image

I am not sure if there's any good solution for that but this is a good workaround I believe.

@pahud pahud added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Mar 28, 2024
@LJunghansCode
Copy link

Thanks so much for the example @pahud I appreciate the response.

Did we misunderstand the resource limit nested stack workaround with our implementation? I would think the api fromRestApiAttributes would take on the defaultCorsPreflightOptions that we pass to the first API we create. Just want to clarify we are on the right track before I re-organize to enable the workaround.

I am going to implement this in one of our nested stacks I will update here.

This should be simple to fix if this works, however if this is required for nested stack resources to take on parent stack defaultCorsPreflightOptions we should update the docs here: https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.NestedStack.html

However if this isn't the preferred solution to our general problem I would love your thoughts on how we can re-work our exceeding 500 resource limit api created via cdk.

Thanks again for the help! And love cdk!

@LJunghansCode
Copy link

@pahud Can confirm that in our nested stack resource declaration all we had to do was addCorsPreflight our default options to each resource. thanks so much.

The fix is so simple yet was so unintuitive for me based on the docs of fromRestApiAttributes!

Thanks again and have a great weekend

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Mar 29, 2024
@wascou
Copy link
Author

wascou commented Mar 29, 2024

I'm sorry @pahud but this is not the expected behavior.

Your answer is just using another API component to fix the issue. A workaround is not enough for the problem I pointed, I asked for a change in this construct.

This is not fixing to the fundamental problem of why the settings are not shipped with the object through the different stacks, in a pure OOP style and in ocherence with the constructor of this L2 construct.

In clear, you create an object in one file, calling it later in other file is not giving you the same object. Where is the immuability?

@LJunghansCode
Copy link

+1 @wascou the workaround solves our immediate problem, but this of course doesn't fix the bug, unless like I said this is the expected way to use the cdk lib.

@pahud
Copy link
Contributor

pahud commented Apr 4, 2024

@wascou @LJunghansCode

Just as I mentioned above this is just a workaround, not a good solution. For all p2 issues with workaround, we welcome and appreciate any ideas and pull requests to help us improve the user experience.

@pahud
Copy link
Contributor

pahud commented Apr 4, 2024

I would not say it's a bug but we definitely need to improve the user experience. Generally, any imported resource with fromXxxAttribute() method is actually a reference object that carries all attributes user passes in. So in this case the imported API literally has no idea if the original API is having defaultCorsPreflightOptionsenabled until we allow users to pass this attribute in fromRestApiAttributes()

It might be a solution to add a new prop in RestApiAttributes so when we import that API we can specify defaultCorsPreflightOptions for it.

This might be an option off the top of my head. We welcome any PR to help us improve this user experience.

@pahud pahud added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Apr 24, 2024
Copy link

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Apr 24, 2024
@wascou
Copy link
Author

wascou commented Apr 24, 2024

Up

@github-actions github-actions bot removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Apr 25, 2024
@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Apr 25, 2024
@cd21h
Copy link

cd21h commented May 24, 2024

Please fix.

Lost half of the day because of this.

@enheit
Copy link

enheit commented Jun 2, 2024

Have the same problem.

When "importing" HttpApi likke this

    this.#httpApi = HttpApi.fromHttpApiAttributes(this, "HttpApi", { httpApiId: props.httpApiId })

It doesn't "copy" defaultLambdaAuthorizer

    this.#httpApi = new HttpApi(this, "HttpApi", {
      // cut
      defaultAuthorizer: this.#cookiesLambdaAuthorizer,
    })

@GavinZZ
Copy link
Contributor

GavinZZ commented Jun 17, 2024

I think the solution proposed by @pahud makes sense to me. Since it's a p2 issue and there's a potential workaround provided, we would welcome community contribution. If anyone is interested in taking this issue, feel free to draft a PR and tag me to review. I will try to review ASAP.

@wascou
Copy link
Author

wascou commented Jun 18, 2024

Hi @GavinZZ ,

It's an issue we're still discussing what the construct SHOULD do. Being p2 is up to the team, this was not what I asked for and it's really lowering the developper standards.

If you build construct for developpers to achieve IaC, you MUST give the best developper experience. Creating an object and making an import function somewhere else MUST retrieve all the properties of the object, not only what you think useful.

It's also about ownership. This workaround implies lines of code that are actually under the responsibility of the construct else the construct is not covering the scope properly and breaks its promises.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
@aws-cdk/aws-apigateway Related to Amazon API Gateway bug This issue is a bug. effort/medium Medium work item – several days of effort p2
Projects
None yet
Development

No branches or pull requests

7 participants