-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
fix(apigateway): api key required for cors 'options' http method #8778
fix(apigateway): api key required for cors 'options' http method #8778
Conversation
6f39e17
to
9281256
Compare
Skip the test target to speed up building up to the module. This will significantly improve the speed to arrive to a working module after check out. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…ws#8739) - bump aws-lambda-layer-kubectl [2.0.0](https://github.com/aws-samples/aws-lambda-layer-kubectl/releases/tag/2.0.0) with kubectl v1.16.8-eks-e16311, awscli 1.18.86 and helm 3.2.4 - update the helm repo of the kubernetes-dashboard in the integ test to `https://kubernetes.github.io/dashboard/` from [Helm Hub](https://hub.helm.sh/charts/k8s-dashboard) as `https://kubernetes-charts.storage.googleapis.com` has been deprecated ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
aws#8785) Root cause here was that `LambdaIntegration` used the deprecated `restApi` property on its binding Method. This property does not work on imported RestApi and has been superceded by the property `api`. A further bug that was discovered was that the API `methodArn()` on the Method construct did not work on imported RestApi. This has been fixed up such that if a `deploymentStage` is not defined for the imported RestApi, the ARN will return '*' as its stage (shorthand for 'all stages'). fixes aws#8679 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This change increases the timeout to `yarn install` to stabilize the Gitpod prebuilds. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
6d56c2e
to
88824d7
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this PR 😊
Some initial comments below.
yarn install --frozen-lockfile | ||
yarn install --frozen-lockfile --network-timeout 1000000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intentional?
const desc = `${method.restApi.node.uniqueId}.${method.httpMethod}.${method.resource.path.replace(/\//g, '.')}`; | ||
const desc = `${method.api.node.uniqueId}.${method.httpMethod}.${method.resource.path.replace(/\//g, '.')}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like changes from this commit 05aaf42 are showing up here. Not sure what happened with git history here.
Could you fix this up?
@@ -192,12 +193,18 @@ export class Method extends Resource { | |||
authorizer._attachToApi(this.api); | |||
} | |||
|
|||
const methodProps: CfnMethodProps = { | |||
if (this.httpMethod === 'OPTIONS') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if we should instead apply this to only the OPTIONS
method created as part of the CORS setup -
return this.addMethod('OPTIONS', new MockIntegration({ |
Could there be use cases where users want API key to be applied to the OPTIONS
method as well?
In the PR description, can you summarize the problem, root cause and your solution to this (something like this 05aaf42). This helps in posterity to understand the changes made when looking through commit history. |
924c117
to
ebfd5f2
Compare
This PR 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. |
Changes
Notes
Note sure if this is going to be a breaking change, probably not since
OPTIONS
is meant to be used as atell me server's capabilities
kind. referencecloses #8615
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license