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-apigateway] ApiKey is not supported for SpecRestApi #11079

Closed
marcogrcr opened this issue Oct 23, 2020 · 2 comments · Fixed by #11235
Closed

[aws-apigateway] ApiKey is not supported for SpecRestApi #11079

marcogrcr opened this issue Oct 23, 2020 · 2 comments · Fixed by #11235
Assignees
Labels
@aws-cdk/aws-apigateway Related to Amazon API Gateway effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. in-progress This issue is being actively worked on. p1

Comments

@marcogrcr
Copy link

There's an API disparity for ApiKey between RestApi and SpecRestApi constructs:

export interface ApiKeyProps extends ApiKeyOptions {
/**
* [disable-awslint:ref-via-interface]
* A list of resources this api key is associated with.
* @default none
*/
readonly resources?: RestApi[];

Notice how resources is of type RestApi[] instead of IRestApi[], making this impossible:

class MyStack extends Stack {
  constructor(scope: Construct, id: string, props: StackProps) {
    super(scope, id, props);

    const api = new SpecRestApi(this, "MyApi", {
      apiDefinition: ApiDefinition.fromInline({ /* (... )*/ }),
    });
    new ApiKey(this, "MyApi", {
      // ERROR: Type 'SpecRestApi' is missing the following properties from type 'RestApi': methods, deployments, url, addApiKey, and 2 more.
      resources: [api],
    });
  }
}

Additionally, the convenience method:

public addApiKey(id: string, options?: ApiKeyOptions): IApiKey {
return new ApiKey(this, id, {
resources: [this],
...options,
});
}

Is only available in RestApi. It should probably be moved to RestApiBase so SpecRestApi can benefit from it as well.

Fortunately, this disparity doesn't make it impossible to create/associate an API key to an API:

class MyStack extends Stack {
  constructor(scope: Construct, id: string, props: StackProps) {
    super(scope, id, props);

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

    // option 1
    new UsagePlan(this, "MyUsagePlan", {
      apiKey: new ApiKey(this, "MyApi"),
      apiStages: [{ stage: api.deploymentStage }],
    });

    // option 2
    api.addUsagePlan("MyUsagePlan", {
      apiStages: [{ stage: api.deploymentStage }],
    });
  }
}

As a side note, option 2 from the previous example has an unnecessary requirement to assign a value to apiStages in order to bind it to the API.

I believe this could be improved by changing:

public addUsagePlan(id: string, props: UsagePlanProps = {}): UsagePlan {
return new UsagePlan(this, id, props);
}

to:

public addUsagePlan(id: string, props: UsagePlanProps = {}): UsagePlan {
  return new UsagePlan(this, id, {
    apiStages: [
      { stage: this.deploymentStage },
      ...(props.apiStages?.filter((x) => x.stage != this.deploymentStage) ||
        []),
    ],
    ...props,
  });
}

Reproduction Steps

See description above

What did you expect to happen?

  1. RestApi and SpecRestApi have API parity for dealing with API keys.
  2. Calling RestApiBase.prototype.addUsagePlan() automatically binds the usage plan to the RestApiBase instance.

What actually happened?

  1. The APIs are different.
  2. Calling RestApiBase.prototype.addUsagePlan() does not bind the usage plan to the RestApiBase instance. It has to be specified in the apiStages field of the UsagePlanProps parameter.

Environment

  • CLI Version: 1.68.0
  • Framework Version: 1.69.0
  • Node.js Version: 12.13.0
  • OS: macOS 10.14.6
  • Language (Version): typescript@4.0.3

Other

N/A


This is 🐛 Bug Report

@marcogrcr marcogrcr added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 23, 2020
@github-actions github-actions bot added the @aws-cdk/aws-apigateway Related to Amazon API Gateway label Oct 23, 2020
@nija-at
Copy link
Contributor

nija-at commented Oct 28, 2020

Marking this as a feature request to support API keys with SpecRestApi.

@nija-at nija-at added feature-request A feature should be added or improved. p1 effort/small Small work item – less than a day of effort and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 28, 2020
@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Nov 2, 2020
@mergify mergify bot closed this as completed in #11235 Nov 6, 2020
mergify bot pushed a commit that referenced this issue Nov 6, 2020
fix(apigateway): ApiKey not supported for SpecRestApi

closes #11079 

----

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

github-actions bot commented Nov 6, 2020

⚠️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-apigateway Related to Amazon API Gateway effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. in-progress This issue is being actively worked on. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants