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

aws-events-targets: API Gateway target should accept IRestApi and not RestApi #16423

Closed
AWS-MattB opened this issue Sep 9, 2021 · 7 comments · Fixed by #29397
Closed

aws-events-targets: API Gateway target should accept IRestApi and not RestApi #16423

AWS-MattB opened this issue Sep 9, 2021 · 7 comments · Fixed by #29397
Labels
@aws-cdk/aws-events-targets bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@AWS-MattB
Copy link
Contributor

The ApiGateway target in aws-events-targets requires a RestApi from aws-apigateway as part of the constructor. This is actually too restrictive and, as far as I can tell, the implementation does not require that particular subclass. The inheritance hierarchy in aws-apigateway is as follows. The current requirements preclude using a SpecRestApi as a target, without good reason.

export class SpecRestApi extends RestApiBase {}
export class RestApi extends RestApiBase {}
export abstract class RestApiBase extends Resource implements IRestApi {}

The implementation of ApiGateway makes use of this.restApi.arnForExecuteApi() and this.restApi.deploymentStage.stageName, both of which are defined in IRestApi.

Reproduction Steps

        const swaggerDefinition = getSwaggerDefinition(...);
        this.api = new SpecRestApi(this, "RestApi", {
            apiDefinition: ApiDefinition.fromInline(swaggerDefinition),
        })

        // @ts-ignore
        let uglyHack : RestApi = this.api;
        const event_bridge_rule = new Rule(this, "EventBridgeRule", {
            eventPattern: {source: ["aws.ecs"], detailType: ["ECS Task State Change"]},
            targets: [new ApiGateway(uglyHack, {....})]
        })

What did you expect to happen?

The ApiGateway target from aws-events-targets should accept an IRestApi so that a Rest API built from a swagger or OpenAPI definition can be used as a target for an Event Bridge Rule.

What actually happened?

Only RestApi are accepted, so I'm precluded from using a SpecRestApi, which is a sibling to RestApi.

Environment

  • CDK CLI Version : 1.120.0
  • Framework Version: 1.120.0
  • Node.js Version: v12.22.6
  • OS : all
  • Language (Version): all

This is 🐛 Bug Report

@AWS-MattB AWS-MattB added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 9, 2021
@nija-at nija-at removed the @aws-cdk/aws-apigateway Related to Amazon API Gateway label Sep 14, 2021
@nija-at nija-at removed their assignment Sep 14, 2021
@rix0rrr rix0rrr added effort/small Small work item – less than a day of effort p2 and removed needs-triage This issue or PR still needs to be triaged. labels Sep 17, 2021
@rix0rrr
Copy link
Contributor

rix0rrr commented Sep 17, 2021

I agree. Feel free to submit a PR with the change.

@rix0rrr rix0rrr removed their assignment Sep 17, 2021
AWS-MattB pushed a commit to AWS-MattB/aws-cdk that referenced this issue Sep 17, 2021
Fixes: aws#16423

When creating an Event Bridge target for an API Gateway, the
superinterface IRestApi should be consumed instead of the
concrete class RestApi.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
AWS-MattB pushed a commit to AWS-MattB/aws-cdk that referenced this issue Sep 18, 2021
Fixes: aws#16423

When creating an Event Bridge target for an API Gateway, the
superinterface IRestApi should be consumed instead of the
concrete class RestApi.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
AWS-MattB pushed a commit to AWS-MattB/aws-cdk that referenced this issue Sep 22, 2021
Fixes: aws#16423

When creating an Event Bridge target for an API Gateway, the
superinterface IRestApi should be consumed instead of the
concrete class RestApi.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
AWS-MattB pushed a commit to AWS-MattB/aws-cdk that referenced this issue Sep 22, 2021
Fixes: aws#16423

When creating an Event Bridge target for an API Gateway, the
superinterface IRestApi should be consumed instead of the
concrete class RestApi.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
AWS-MattB pushed a commit to AWS-MattB/aws-cdk that referenced this issue Sep 28, 2021
Fixes: aws#16423

When creating an Event Bridge target for an API Gateway, the
superinterface IRestApi should be consumed instead of the
concrete class RestApi.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
AWS-MattB pushed a commit to AWS-MattB/aws-cdk that referenced this issue Sep 28, 2021
Fixes: aws#16423

When creating an Event Bridge target for an API Gateway, the
superinterface IRestApi should be consumed instead of the
concrete class RestApi.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
AWS-MattB pushed a commit to AWS-MattB/aws-cdk that referenced this issue Oct 1, 2021
Fixes: aws#16423

When creating an Event Bridge target for an API Gateway, the
superinterface IRestApi should be consumed instead of the
concrete class RestApi.

----

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

mcwebb commented Sep 23, 2022

I'm also facing this issue, I can see there was a PR that was closed but I can't see why it was closed, does this need a new PR opened?

@AshRhazaly
Copy link

Facing the issue as well, why was the PR closed?

@Sevi7
Copy link

Sevi7 commented Dec 28, 2022

Same here

n-k1 pushed a commit to n-k1/aws-cdk that referenced this issue Mar 7, 2024
n-k1 pushed a commit to n-k1/aws-cdk that referenced this issue Mar 7, 2024
n-k1 pushed a commit to n-k1/aws-cdk that referenced this issue Mar 7, 2024
n-k1 pushed a commit to n-k1/aws-cdk that referenced this issue Mar 7, 2024
n-k1 pushed a commit to n-k1/aws-cdk that referenced this issue Mar 8, 2024
n-k1 pushed a commit to n-k1/aws-cdk that referenced this issue Mar 8, 2024
n-k1 pushed a commit to n-k1/aws-cdk that referenced this issue Mar 18, 2024
n-k1 pushed a commit to n-k1/aws-cdk that referenced this issue Mar 22, 2024
n-k1 pushed a commit to n-k1/aws-cdk that referenced this issue Mar 22, 2024
n-k1 pushed a commit to n-k1/aws-cdk that referenced this issue Mar 22, 2024
@abhigyandwiji
Copy link

abhigyandwiji commented Apr 4, 2024

Looking forward to the change. Planning to use it

@sharmagg
Copy link

sharmagg commented Apr 4, 2024

I'm facing the exact same issue

n-k1 pushed a commit to n-k1/aws-cdk that referenced this issue Apr 23, 2024
n-k1 pushed a commit to n-k1/aws-cdk that referenced this issue Apr 23, 2024
n-k1 pushed a commit to n-k1/aws-cdk that referenced this issue Apr 23, 2024
@mergify mergify bot closed this as completed in #29397 Apr 24, 2024
mergify bot pushed a commit that referenced this issue Apr 24, 2024
#29397)

### Issue

Closes [#16423](#16423).

### Reason for this change

The CDK construct to set an APIGatway as the target for an EventBridge rule only accepted RestApi. It should instead accept the interface IRestApi.

### Description of changes

This change was attempted once earlier, but [the PR](#16542) was closed.

Changed the `ApiGateway` target's constructor's first argument from `api.RestApi` to `api.IRestApi`.

To fix compatibility failures caused by the previous `restApi` being a public member of the class, I've followed the approach suggested [here](#16542 (comment)). Some other alternatives are suggested [here](#16542 (comment)).

### Description of how you validated changes

Added a unit test that creates a dummy `SpecRestApi`. Also added an integ test that creates a `SpecRestApi` from a dummy OpenAPI spec and creates a rule on the default event bus with the APIGateway as target. Nothing is executed in the integ test. The assertion only checks if the target is created using the `ListTargetsByRule` API.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

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

⚠️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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-events-targets bug This issue is a bug. effort/small Small work item – less than a day of effort p2
Projects
None yet
8 participants