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.

Sign up for GitHub

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

Jump to bottom

fix(apigateway): authorizerUri does not resolve to the correct partition #8152

Merged
merged 2 commits into from
Jun 4, 2020
Merged

fix(apigateway): authorizerUri does not resolve to the correct partition #8152

merged 2 commits into from
Jun 4, 2020

Conversation

DRNagar
Copy link
Contributor

@DRNagar DRNagar commented May 22, 2020 •
edited by nija-at
Loading

Add that the authorizerURI includes the correct partition. Previously, it
always used the aws partition.

fixes #8098

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: f875446
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 6e6d2e4
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@nija-at nija-at changed the title fix(aws-apigateway): authorizers authorizerUri add partition fix(apigateway): authorizerUri does not have the partition May 27, 2020
@nija-at nija-at changed the title fix(apigateway): authorizerUri does not have the partition fix(apigateway): authorizerUri does not resolve to the correct partition May 27, 2020
nija-at
nija-at previously requested changes May 27, 2020
View reviewed changes
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making these changes.

A couple of things need to be added here -

  1. This also needs to be fixed in RequestAuthorizer here -

    aws-cdk/packages/@aws-cdk/aws-apigateway/lib/authorizers/lambda.ts

    Line 235 in 0028778

    authorizerUri: `arn:aws:apigateway:${Stack.of(this).region}:lambda:path/2015-03-31/functions/${props.handler.functionArn}/invocations`,
    Perhaps extract the ARN construction into a separate function that both constructors call into.

  2. Update the assertions on the unit tests to verify that the ARN is as expected. Update here -

    aws-cdk/packages/@aws-cdk/aws-apigateway/test/authorizers/test.lambda.ts

    Line 28 in 0028778

    expect(stack).to(haveResource('AWS::ApiGateway::Authorizer', {
    and here -

    aws-cdk/packages/@aws-cdk/aws-apigateway/test/authorizers/test.lambda.ts

    Line 65 in 0028778

    expect(stack).to(haveResource('AWS::ApiGateway::Authorizer', {
    You will need to add an AuthorizerUri property to the haveResource method.

@mergify mergify bot dismissed nija-at’s stale review May 27, 2020 15:10

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: f875446
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@DRNagar
Copy link
Contributor Author

DRNagar commented May 29, 2020

Hi nija-at,

Thank you for the feedback. I have updated the commit accordingly :)

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 63e27b1
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

nija-at
nija-at previously requested changes May 29, 2020
View reviewed changes
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly there. Just one more comment to fix up.

packages/@aws-cdk/aws-apigateway/lib/authorizers/lambda.ts Outdated
Comment on lines 130 to 135
/**
* constructs the authorizerURIArn.
*/
protected authorizerURIArn() {
return `arn:${Stack.of(this).partition}:apigateway:${Stack.of(this).region}:lambda:path/2015-03-31/functions/${this.handler.functionArn}/invocations`;
}
Copy link
Contributor

@nija-at nija-at May 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will export this API as part of this module's public API. That is not necessary here.

Instead, make this a standalone function at the bottom of the file.

  function lambdaAuthorizerArn(handler: Function) {
    return `arn:${Stack.of(handler).partition}:apigateway:${Stack.of(handler).region}:lambda:path/2015-03-31/functions/${handler.functionArn}/invocations`;
  }

@mergify mergify bot dismissed nija-at’s stale review May 29, 2020 19:18

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: ca58f5b
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@DRNagar
Copy link
Contributor Author

DRNagar commented May 29, 2020

I have added the changes :)

nija-at
nija-at previously approved these changes Jun 1, 2020
View reviewed changes
@nija-at nija-at dismissed their stale review June 1, 2020 07:28

build failures

nija-at
nija-at previously requested changes Jun 1, 2020
View reviewed changes
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there is another failing integration test. Please take a look and apply a fix.

fix(aws-apigateway): authorizers authorizerUri add partition
1a0d54b 
The authorizerURI includes the correct partition. Previously, it
always used the aws partition

fixes #<8098>
@mergify mergify bot dismissed nija-at’s stale review June 3, 2020 19:52

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 1a0d54b
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Jun 4, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 105cf35
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Jun 4, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit f455273 into aws:master Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nija-at nija-at nija-at approved these changes

Assignees

@nija-at nija-at

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The API Gateway RequestAuthorizer URI is not partition based
3 participants
@DRNagar @aws-cdk-automation @nija-at