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.

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

Already on GitHub? Sign in to your account

Cloudfront Origin does not support SpecRestApi #27501

Closed
brianantonelli opened this issue Oct 11, 2023 · 4 comments · Fixed by #27742
Closed

Cloudfront Origin does not support SpecRestApi #27501

brianantonelli opened this issue Oct 11, 2023 · 4 comments · Fixed by #27742
Labels
@aws-cdk/aws-cloudfront Related to Amazon CloudFront effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md p1

Comments

@brianantonelli
Copy link

Describe the bug

The RestApiOrigin constructor only supports RestApi and not RestApiBase. This means a API created via SpecRestApi cannot be used to create a Cloudfront origin.

Expected Behavior

Passing an instance of SpecRestApi to RestApiOrigin should be accepted and create the origin.

Current Behavior

When passing SpecRestApi to RestApiOrigin an error is generated

TypeError: type of argument rest_api must be aws_cdk.aws_apigateway.RestApi; got aws_cdk.aws_apigateway.SpecRestApi instead

Reproduction Steps

  1. Create a RestAPI from a OpenAPI spec file
  2. Create a Cloudfront distribution that contains an API origin and use the RestAPI generated in step 1

Possible Solution

Update RestApiOrigin constructor to accept RestApiBase for restApi

Additional Information/Context

No response

CDK CLI Version

2.96.0 (build e6322aa)

Framework Version

No response

Node.js Version

v18.17.1

OS

MacOS

Language

Python

Language Version

3.11.4

Other information

No response

@brianantonelli brianantonelli added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 11, 2023
@github-actions github-actions bot added the @aws-cdk/aws-cloudfront Related to Amazon CloudFront label Oct 11, 2023
@indrora indrora added p2 and removed needs-triage This issue or PR still needs to be triaged. labels Oct 12, 2023
@indrora
Copy link
Contributor

indrora commented Oct 12, 2023

Thank you. Can you manually construct this using only Cfn resources?

@peterwoodworth
Copy link
Contributor

@indrora I don't see what Cfn resources have to do with this. This is related to our higher level constructs.

@brianantonelli all the RestApiOrigin construct requires is the url property. SpecRestApi doesn't have this property, but it does have urlForPath() which is exactly what the url property is a getter for on RestApi. So, if we add the url property to SpecRestApi, or move it from RestApi to RestApiBase, I don't see why RestApiOrigin couldn't take in a SpecRestApi.

super(cdk.Fn.select(2, cdk.Fn.split('/', restApi.url)), {
originPath: props.originPath ?? `/${cdk.Fn.select(3, cdk.Fn.split('/', restApi.url))}`,

public get url() {
return this.urlForPath();
}

@peterwoodworth peterwoodworth added good first issue Related to contributions. See CONTRIBUTING.md p1 feature-request A feature should be added or improved. effort/small Small work item – less than a day of effort and removed bug This issue is a bug. p2 labels Oct 16, 2023
@hariprakash-j
Copy link
Contributor

hariprakash-j commented Nov 1, 2023

@peterwoodworth @brianantonelli I was able to fix the bug and created a PR #27742 , this should be fixed when after they review it and merge it.

@mergify mergify bot closed this as completed in #27742 Dec 5, 2023
mergify bot pushed a commit that referenced this issue Dec 5, 2023
Moved the url property from RestApi class to the RestApiBase to have the url property on all derived class. This will fix the problem mentioned in the issue where SpecRestApi does not work with the RestApiOrigin construct.

PS. I could not perform the integration tests as the free tier on my personal account expired.

Closes #27501.

----

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

github-actions bot commented Dec 5, 2023

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

chenjane-dev pushed a commit to chenjane-dev/aws-cdk that referenced this issue Dec 5, 2023
Moved the url property from RestApi class to the RestApiBase to have the url property on all derived class. This will fix the problem mentioned in the issue where SpecRestApi does not work with the RestApiOrigin construct.

PS. I could not perform the integration tests as the free tier on my personal account expired.

Closes aws#27501.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-cloudfront Related to Amazon CloudFront effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md p1
Projects
None yet
4 participants