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

Default to error code if message is non-existent #4794

Merged
merged 3 commits into from Jan 24, 2019

Conversation

Projects
4 participants
@drexler
Copy link
Contributor

drexler commented Mar 2, 2018

What did you implement:

Closes #4752

How did you implement it:

Based on #4752, it appears that when calling the headObject API : https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/S3.html#headObject-property and there's a problem with the IAM credentials used - say they're invalid or lacks sufficient credentials to perform the operation; AWS returns a 403 error with a null message which is presented to the user by Serverless. This PR basically checks for that and defaults to error code (a string representation) as the default.

How can we verify it:

using the following:

import * as aws from 'aws-sdk'

 export async function getObjectMetadata () : Promise<any> {
    const params = {
        Bucket: 'api.payroll',
        Key: '3.9.0.11.zip'
    };

    const s3 = new aws.S3({
        apiVersion: '2006-03-01',
        credentials: {
            accessKeyId: 'AKIABadKeys1738',
            secretAccessKey: '3d9DSsshItsSecretWhBoV'
        },
        region: `us-east-1`
    });

    try {
        const artifactInfo = await s3.headObject(params).promise();
        console.log(`Artifact metadata: ${artifactInfo}`);

    } catch(error) {
        console.log(error);
    } 

 }

returns

{ Forbidden: null
    at Request.extractError (/mnt/c/repos/demo/serverless-s3api/node_modules/aws-sdk/lib/services/s3.js:557:35)
    at Request.callListeners (/mnt/c/repos/demo/serverless-s3api/node_modules/aws-sdk/lib/sequential_executor.js:105:20)
    at Request.emit (/mnt/c/repos/demo/serverless-s3api/node_modules/aws-sdk/lib/sequential_executor.js:77:10)
    at Request.emit (/mnt/c/repos/demo/serverless-s3api/node_modules/aws-sdk/lib/request.js:683:14)
    at Request.transition (/mnt/c/repos/demo/serverless-s3api/node_modules/aws-sdk/lib/request.js:22:10)
    at AcceptorStateMachine.runTo (/mnt/c/repos/demo/serverless-s3api/node_modules/aws-sdk/lib/state_machine.js:14:12)
    at /mnt/c/repos/demo/serverless-s3api/node_modules/aws-sdk/lib/state_machine.js:26:10
    at Request.<anonymous> (/mnt/c/repos/demo/serverless-s3api/node_modules/aws-sdk/lib/request.js:38:9)
    at Request.<anonymous> (/mnt/c/repos/demo/serverless-s3api/node_modules/aws-sdk/lib/request.js:685:12)
    at Request.callListeners (/mnt/c/repos/demo/serverless-s3api/node_modules/aws-sdk/lib/sequential_executor.js:115:18)
  message: null,
  code: 'Forbidden',
  region: null,
  time: 2018-03-05T17:21:32.354Z,
  requestId: '236FA74437F93B81',
  extendedRequestId: '308YBE4qsWSgIHQ51QZj+QYC5zDR40Szw6WXnRDYhbgfz+zWAiNdX8QOYbYyfT6GvYGckV7HHno=',
  cfId: undefined,
  statusCode: 403,
  retryable: false,
  retryDelay: 56.37456143863713 }

Todos:

  • Make sure code coverage hasn't dropped
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below
  • Write tests
  • Fix linting errors

Is this ready for review?: YES
Is it a breaking change?: NO

@drexler drexler force-pushed the drexler:default-error-message branch from ee028f9 to 78d0c5d Mar 2, 2018

@horike37

This comment has been minimized.

Copy link
Member

horike37 commented Mar 5, 2018

Hi @drexler.
Thank you for the PR, but could you fill out the PR template?
For reviewing that, it is very important for us to know what problem is, why and how you have decided the approach.

@drexler

This comment has been minimized.

Copy link
Contributor Author

drexler commented Mar 5, 2018

@horike37 updated the PR description to help with the review.

@pmuens pmuens added the pr/in-review label Jan 15, 2019

@pmuens
Copy link
Member

pmuens left a comment

Great! Thanks for taking the time to fix this @drexler 👍 👌

Would be nice if we could have a unit test for this. Other than that I think it's GTM :shipit:

@pmuens pmuens self-assigned this Jan 23, 2019

@pmuens pmuens added this to In progress in Serverless via automation Jan 23, 2019

@pmuens pmuens changed the title default to error code if message is non-existent Default to error code if message is non-existent Jan 23, 2019

@drexler

This comment has been minimized.

Copy link
Contributor Author

drexler commented Jan 23, 2019

@pmuens Thanks for the review. I'll wire up some tests for this.

@drexler drexler force-pushed the drexler:default-error-message branch from 78d0c5d to 67ee7c6 Jan 24, 2019

@drexler drexler force-pushed the drexler:default-error-message branch from c72e6d4 to 911bedd Jan 24, 2019

@drexler

This comment has been minimized.

Copy link
Contributor Author

drexler commented Jan 24, 2019

@pmuens added the cover unit test.

Serverless automation moved this from In progress to Reviewer approved Jan 24, 2019

@pmuens

pmuens approved these changes Jan 24, 2019

Copy link
Member

pmuens left a comment

Great! Thanks for jumping in and updating @drexler 👍

I just merged another PR a few minutes ago which resulted in conflicts with this one. However I pulled your changes, resolved the conflicts and added another test which test whether an existing error message is preferred when both, the error code and the message are given.

After that I took this PR for a spin and it works great. Will merge once CI a-greens...

:shipit:

@pmuens pmuens merged commit 0aa672e into serverless:master Jan 24, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Serverless automation moved this from Reviewer approved to Done Jan 24, 2019

@pmuens pmuens added this to the 1.36.4 milestone Feb 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment