From 3c15cdf07d809fdc6dab63562f2126b84c8a181d Mon Sep 17 00:00:00 2001 From: Carl Nordenfelt Date: Mon, 23 May 2016 15:10:29 +0200 Subject: [PATCH] Fix: No longer delete API Methods on Conflict exceptions. --- _scripts/publish.sh | 2 + lib/commands/ApiMethod.js | 51 ++++++++++++------- lib/service/ApiMethod/ApiMethodService.js | 13 +++-- tests/unit/commands/ApiMethodUnitTest.js | 45 ++++++++++++---- .../ApiMethod/ApiMethodServiceUnitTest.js | 21 +++++--- web/index.html | 44 ++++++++++++++-- 6 files changed, 133 insertions(+), 43 deletions(-) diff --git a/_scripts/publish.sh b/_scripts/publish.sh index bb84d93..e453a78 100755 --- a/_scripts/publish.sh +++ b/_scripts/publish.sh @@ -97,6 +97,8 @@ function publish() { sed -i '.original' "s:{VERSION}:${version}:g" ${templatePath}/${templateName} aws s3 cp ${sourceFileName} s3://${s3BucketName}.eu-west-1/${version}/${sourceFileName} --region eu-west-1 aws s3 cp ${templatePath}/${templateName} s3://${s3BucketName}.eu-west-1/${version}/${templateName} --region eu-west-1 + aws s3 cp ${sourceFileName} s3://${s3BucketName}.us-east-1/${version}/${sourceFileName} --region us-east-1 + aws s3 cp ${templatePath}/${templateName} s3://${s3BucketName}.us-east-1/${version}/${templateName} --region us-east-1 echo "https://s3.amazonaws.com/${s3BucketName}.eu-west-1/${version}/${templateName}" fi diff --git a/lib/commands/ApiMethod.js b/lib/commands/ApiMethod.js index 052688a..26d0f89 100644 --- a/lib/commands/ApiMethod.js +++ b/lib/commands/ApiMethod.js @@ -10,8 +10,19 @@ pub.getParameters = function (event) { }; pub.createResource = function createResource(event, context, eventParams, callback) { - ApiMethodService.createMethod(eventParams, function (error) { + ApiMethodService.createMethod(eventParams, function (error, apiMethod) { if (error) { + // If apiMethod is returned it means that the method was created but something went wrong after + // If this is the case we want the subsequent delete request to delete the method. + // Otherwise, the method should not be touched (it's either not created or it is in fact another method (Conflict error) + if (apiMethod) { + if (typeof error === 'string') { // Sanity check + error = { + message: error + }; + } + error.physicalResourceId = eventParams.params.resourceId + '/' + eventParams.params.method.httpMethod; + } return callback(error); } getForResponse(event, context, eventParams.params.restApiId, eventParams.params.resourceId, eventParams.params.method.httpMethod, callback); @@ -19,19 +30,28 @@ pub.createResource = function createResource(event, context, eventParams, callba }; pub.deleteResource = function deleteResource(event, _context, eventParams, callback) { - var resourceAndMethod = _parsePhysicalResourceId(event.PhysicalResourceId, eventParams.params.resourceId, eventParams.params.method.httpMethod); - ApiMethodService.deleteMethod(eventParams.params.restApiId, resourceAndMethod.resourceId, resourceAndMethod.httpMethod, function (error) { - return callback(error); - }); + var resourceAndMethod = _parsePhysicalResourceId(event.PhysicalResourceId); + if (resourceAndMethod) { + ApiMethodService.deleteMethod(eventParams.params.restApiId, resourceAndMethod.resourceId, resourceAndMethod.httpMethod, function (error) { + return callback(error); + }); + return; + } + callback(); }; pub.updateResource = function updateResource(event, context, eventParams, callback) { - pub.deleteResource(event, context, eventParams, function (error) { - if (error) { - return callback(error); - } - return pub.createResource(event, context, eventParams, callback); - }); + var resourceAndMethod = _parsePhysicalResourceId(event.PhysicalResourceId); + if (resourceAndMethod) { + pub.deleteResource(event, context, eventParams, function (error) { + if (error) { + return callback(error); + } + return pub.createResource(event, context, eventParams, callback); + }); + return; + } + callback('Resource not found'); }; module.exports = pub; @@ -47,19 +67,12 @@ function getForResponse(_event, _context, restApiId, resourceId, httpMethod, cal }); } -function _parsePhysicalResourceId(physicalResourceId, eventResourceId, eventHttpMethod) { +function _parsePhysicalResourceId(physicalResourceId) { var parts = physicalResourceId.split('/'); - // TODO: Candidate for much harder scrutiny. The HTTP Method can be validated to ensure that it is a proper physicalResourceId if (parts.length === 2) { return { resourceId: parts[0], httpMethod: parts[1] }; } - // Fallback if the PhysicalResourceId appears to be incorrectly formatted. - // This happens if the create operation fails and CFN makes a subsequent delete call to clean up. - return { - resourceId: eventResourceId, - httpMethod: eventHttpMethod - }; } diff --git a/lib/service/ApiMethod/ApiMethodService.js b/lib/service/ApiMethod/ApiMethodService.js index 5a6f8c9..a93d452 100644 --- a/lib/service/ApiMethod/ApiMethodService.js +++ b/lib/service/ApiMethod/ApiMethodService.js @@ -56,15 +56,15 @@ pub.createMethod = function createMethod(parameters, callback) { } createIntegration(validParameters, function (integrationError) { if (integrationError) { - return callback(integrationError); + return callback(integrationError, apiMethod); } createResponses(validParameters, function (responseError) { if (responseError) { - return callback(responseError); + return callback(responseError, apiMethod); } createIntegrationResponses(validParameters, function (integrationResponseError) { if (integrationResponseError) { - return callback(integrationResponseError); + return callback(integrationResponseError, apiMethod); } return callback(undefined, apiMethod); }); @@ -187,7 +187,13 @@ function createIntegration(parameters, callback) { } function createResponses(parameters, callback) { + var createdResponses = []; async.mapSeries(parameters.responses, function (response, asyncCallback) { + // Response already created, skip + if (createdResponses.indexOf(response.statusCode) > -1) { + return asyncCallback(); + } + var params = { restApiId: parameters.restApiId, resourceId: parameters.resourceId, @@ -208,6 +214,7 @@ function createResponses(parameters, callback) { if (error) { logger.log('Error ApiMethodService::createResponses', { error: error, params: params }); } + createdResponses.push(response.statusCode); asyncCallback(error); }); }, function (error) { diff --git a/tests/unit/commands/ApiMethodUnitTest.js b/tests/unit/commands/ApiMethodUnitTest.js index cdc56c6..899747b 100644 --- a/tests/unit/commands/ApiMethodUnitTest.js +++ b/tests/unit/commands/ApiMethodUnitTest.js @@ -69,23 +69,43 @@ describe('ApiMethodCommand', function () { describe('createMethod', function () { it('should create resource', function (done) { - testSubject.createResource({}, {}, { params: {method: {}} }, function (error) { + testSubject.createResource({}, {}, { params: { method: {}} }, function (error) { expect(error).to.be.undefined; done(); }); }); it('should fail create resource', function (done) { createMethodStub.yields('createError'); - testSubject.createResource({}, {}, { params: {method: {}} }, function (error) { + testSubject.createResource({}, {}, { params: { method: {}} }, function (error) { expect(error).to.equal('createError'); expect(createMethodStub.called).to.be.true; expect(getForResponseStub.called).to.be.false; done(); }); }); + it('should fail create resource but with a proper physical resource id', function (done) { + createMethodStub.yields('createError', {}); + testSubject.createResource({}, {}, { params: { resourceId: 'test', method: { httpMethod: 'GET' }} }, function (error) { + expect(error.message).to.equal('createError'); + expect(error.physicalResourceId).to.equal('test/GET'); + expect(createMethodStub.called).to.be.true; + expect(getForResponseStub.called).to.be.false; + done(); + }); + }); + it('should fail create resource but with a proper physical resource id', function (done) { + createMethodStub.yields({ code: 'createError' }, {}); + testSubject.createResource({}, {}, { params: { resourceId: 'test', method: { httpMethod: 'GET' }} }, function (error) { + expect(error.code).to.equal('createError'); + expect(error.physicalResourceId).to.equal('test/GET'); + expect(createMethodStub.called).to.be.true; + expect(getForResponseStub.called).to.be.false; + done(); + }); + }); it('should fail if get for response fails', function (done) { getForResponseStub.yields('getForResponseError'); - testSubject.createResource({}, {}, { params: {method: {}} }, function (error) { + testSubject.createResource({}, {}, { params: { method: {}} }, function (error) { expect(error).to.equal('getForResponseError'); expect(createMethodStub.called).to.be.true; expect(getForResponseStub.called).to.be.true; @@ -102,6 +122,13 @@ describe('ApiMethodCommand', function () { done(); }); }); + it('should do nothingif physical resource id is not set properly', function (done) { + testSubject.deleteResource({ PhysicalResourceId: 'dummy' }, {}, { params: {method: {}} }, function (error) { + expect(error).to.be.undefined; + expect(deleteMethodStub.called).to.be.false; + done(); + }); + }); it('should fail delete resource', function (done) { deleteMethodStub.yields('deleteError'); testSubject.deleteResource({ PhysicalResourceId: 'test/GET' }, {}, { params: {method: {}} }, function (error) { @@ -123,13 +150,13 @@ describe('ApiMethodCommand', function () { done(); }); }); - it('should fallback to method from parameters if PhysicalResourceId is not properly set', function (done) { + it('should do nothing if PhysicalResourceId is not properly set', function (done) { testSubject.updateResource({ PhysicalResourceId: 'dummy' }, {}, { params: {method: {}} }, function (error, resource) { - expect(error).to.be.undefined; - expect(resource).to.be.an('object'); - expect(deleteMethodStub.called).to.be.true; - expect(createMethodStub.called).to.be.true; - expect(getForResponseStub.called).to.be.true; + expect(error).to.equal('Resource not found'); + expect(resource).to.be.undefined; + expect(deleteMethodStub.called).to.be.false; + expect(createMethodStub.called).to.be.false; + expect(getForResponseStub.called).to.be.false; done(); }); }); diff --git a/tests/unit/service/ApiMethod/ApiMethodServiceUnitTest.js b/tests/unit/service/ApiMethod/ApiMethodServiceUnitTest.js index 61c8cdd..6782c38 100644 --- a/tests/unit/service/ApiMethod/ApiMethodServiceUnitTest.js +++ b/tests/unit/service/ApiMethod/ApiMethodServiceUnitTest.js @@ -65,7 +65,7 @@ describe('ApiMethodService', function () { 'method.response.header.Access-Control-Allow-Methods': 'GET', 'method.response.header.Access-Control-Allow-Origin': '*' } - }, + } }, requestTemplates: { 'application/json': {} @@ -131,15 +131,18 @@ describe('ApiMethodService', function () { cacheNamespace: '' }, responses: { - default: { - statusCode: "Response.StatusCode", + 'default': { + statusCode: '200', responseTemplates: { ResponseTemplateObject: {}, ResponseTemplateString: '' } }, - ResponseKey: { - statusCode: "Response.StatusCode" + '.*BadResponse1.*': { + statusCode: '500' + }, + '.*BadResponse2.*': { + statusCode: '500' } } } @@ -148,6 +151,8 @@ describe('ApiMethodService', function () { it('should create an api method', function (done) { testSubject.createMethod(params, function (error, apiMethod) { expect(error).to.be.undefined; + expect(putMethodResponseStub.calledTwice).to.be.true; + expect(putIntegrationResponseStub.calledThrice).to.be.true; expect(apiMethod).to.be.an('object'); done(); }); @@ -235,7 +240,7 @@ describe('ApiMethodService', function () { testSubject.createMethod(params, function (error, apiMethod) { expect(error).to.be.an.Error; expect(error).to.equal('putIntegrationError'); - expect(apiMethod).to.be.undefined; + expect(apiMethod).to.an('object'); done(); }); }); @@ -244,7 +249,7 @@ describe('ApiMethodService', function () { testSubject.createMethod(params, function (error, apiMethod) { expect(error).to.be.an.Error; expect(error).to.equal('putMethodResponseError'); - expect(apiMethod).to.be.undefined; + expect(apiMethod).to.an('object'); done(); }); }); @@ -253,7 +258,7 @@ describe('ApiMethodService', function () { testSubject.createMethod(params, function (error, apiMethod) { expect(error).to.be.an.Error; expect(error).to.equal('putIntegrationResponseError'); - expect(apiMethod).to.be.undefined; + expect(apiMethod).to.an('object'); done(); }); }); diff --git a/web/index.html b/web/index.html index 837c83a..5b48106 100644 --- a/web/index.html +++ b/web/index.html @@ -28,7 +28,7 @@ -

native support + and evaluate your use cases. Going with the native support will be better in most cases.. +

+
+

API Gateway for CloudFormation is a set of Custom Resources that allows you to manage your API Gateway setup with CloudFormation. Yes, you read that right! Finally a way to integrate your backend services that you @@ -2329,6 +2345,26 @@

CloudFormation Template Example

Change Log

+
+

1.7.1 (2016-05-23)

+
+
+
    +
  • Fix If an API Method failed to create due to a Conflict Exception the existing API Method was subsequently deleted by a too eager resource identification algorithm.
    • +
+
+ +
+

Templates

+ +
+

1.7.0 (2016-04-22)

@@ -2342,7 +2378,7 @@

1.7.0 (2016-04-22)

Upgrade Notes

If you are upgrading from a previous version, ensure that you increase the ExecutionTimeout parameter in CloudFormation to 180 to ensure that the Lambda doesn't timeout prematurely. - This change will not have any noticable impact on exisiting deploys but if your deploy is very complex + This change will not have any notable impact on existing deploys but if your deploy is very complex the extra time may be needed to ensure that all resources are created.
@@ -2352,7 +2388,7 @@

Templates

  • eu-central-1
  • eu-west-1
  • us-east-1
    • -
  • us-west-2
    • +
  • us-west-2
  • ap-northeast-1
    • @@ -2373,7 +2409,7 @@

    Templates

  • eu-central-1
  • eu-west-1
  • us-east-1
    • -
  • us-west-2
    • +
  • us-west-2
  • ap-northeast-1