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

Ensure its possible to remove stack with missing bucket #10306

Merged
merged 3 commits into from
Dec 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions lib/plugins/aws/lib/check-if-bucket-exists.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
'use strict';

module.exports = {
async checkIfBucketExists(bucketName) {
try {
await this.provider.request('S3', 'headBucket', {
Bucket: bucketName,
});
return true;
} catch (err) {
if (err.code === 'AWS_S3_HEAD_BUCKET_NOT_FOUND') {
return false;
}

throw err;
}
},
};
4 changes: 3 additions & 1 deletion lib/plugins/aws/remove/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

const validate = require('../lib/validate');
const monitorStack = require('../lib/monitorStack');
const checkIfBucketExists = require('../lib/check-if-bucket-exists');
const emptyS3Bucket = require('./lib/bucket');
const removeStack = require('./lib/stack');
const removeEcrRepository = require('./lib/ecr');
Expand All @@ -23,7 +24,8 @@ class AwsRemove {
removeStack,
monitorStack,
removeEcrRepository,
checkIfEcrRepositoryExists
checkIfEcrRepositoryExists,
checkIfBucketExists
);

this.hooks = {
Expand Down
58 changes: 33 additions & 25 deletions lib/plugins/aws/remove/lib/bucket.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,22 @@
'use strict';

const BbPromise = require('bluebird');
const { legacy } = require('@serverless/utils/log');
const { legacy, log } = require('@serverless/utils/log');

module.exports = {
async setServerlessDeploymentBucketName() {
return this.provider.getServerlessDeploymentBucketName().then((bucketName) => {
try {
const bucketName = await this.provider.getServerlessDeploymentBucketName();
this.bucketName = bucketName;
});
} catch (err) {
// If there is a validation error with expected message, it means that logical resource for
// S3 bucket does not exist and we want to proceed with empty `bucketName`
if (
err.providerError.code !== 'ValidationError' ||
!err.message.includes('does not exist for stack')
) {
throw err;
}
}
},

async listObjectsV2() {
Expand All @@ -16,21 +25,18 @@ module.exports = {
legacy.log('Getting all objects in S3 bucket...');
const serviceStage = `${this.serverless.service.service}/${this.provider.getStage()}`;

return this.provider
.request('S3', 'listObjectsV2', {
Bucket: this.bucketName,
Prefix: `${this.provider.getDeploymentPrefix()}/${serviceStage}`,
})
.then((result) => {
if (result) {
result.Contents.forEach((object) => {
this.objectsInBucket.push({
Key: object.Key,
});
});
}
return BbPromise.resolve();
const result = await this.provider.request('S3', 'listObjectsV2', {
Bucket: this.bucketName,
Prefix: `${this.provider.getDeploymentPrefix()}/${serviceStage}`,
});

if (result) {
result.Contents.forEach((object) => {
this.objectsInBucket.push({
Key: object.Key,
});
});
}
},

async listObjectVersions() {
Expand Down Expand Up @@ -75,21 +81,23 @@ module.exports = {
async deleteObjects() {
legacy.log('Removing objects in S3 bucket...');
if (this.objectsInBucket.length) {
return this.provider.request('S3', 'deleteObjects', {
await this.provider.request('S3', 'deleteObjects', {
Bucket: this.bucketName,
Delete: {
Objects: this.objectsInBucket,
},
});
}

return BbPromise.resolve();
},

async emptyS3Bucket() {
return BbPromise.bind(this)
.then(this.setServerlessDeploymentBucketName)
.then(this.listObjects)
.then(this.deleteObjects);
await this.setServerlessDeploymentBucketName();
if (this.bucketName && (await this.checkIfBucketExists(this.bucketName))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't bucketName be unconditionally set here? (we didn't have that check before)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a scenario in which it won't be set so we need to leave this check this way

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a scenario that was already in master and was not handled? Or is this scenario introduced with this PR?

Also when exactly is that the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, in master it was just throwing an error on SDK call in setServerlessDeploymentBucket, now we want to handle such situation gracefully

await this.listObjects();
await this.deleteObjects();
} else {
legacy.log('S3 bucket not found. Skipping S3 bucket objects removal');
log.info('S3 bucket not found. Skipping S3 bucket objects removal');
}
},
};
178 changes: 178 additions & 0 deletions test/unit/lib/plugins/aws/remove/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ describe('test/unit/lib/plugins/aws/remove/index.test.js', () => {
S3: {
deleteObjects: deleteObjectsStub,
listObjectsV2: { Contents: [{ Key: 'first' }, { Key: 'second' }] },
headBucket: {},
},
CloudFormation: {
describeStackEvents: describeStackEventsStub,
Expand Down Expand Up @@ -105,6 +106,97 @@ describe('test/unit/lib/plugins/aws/remove/index.test.js', () => {
expect(deleteRepositoryStub).not.to.be.called;
});

it('executes expected operations related to files removal when S3 bucket has files', async () => {
await runServerless({
fixture: 'function',
command: 'remove',
awsRequestStubMap: {
...awsRequestStubMap,
S3: {
deleteObjects: deleteObjectsStub,
listObjectsV2: { Contents: [] },
headBucket: {},
},
},
});

expect(deleteObjectsStub).not.to.be.called;
});

it('executes expected operations related to files removal when S3 bucket is empty', async () => {
await runServerless({
fixture: 'function',
command: 'remove',
awsRequestStubMap,
});

expect(deleteObjectsStub).to.be.calledWithExactly({
Bucket: 'resource-id',
Delete: {
Objects: [{ Key: 'first' }, { Key: 'second' }],
},
});
});

it('skips attempts to remove S3 objects if S3 bucket not found', async () => {
const { awsNaming } = await runServerless({
fixture: 'function',
command: 'remove',
awsRequestStubMap: {
...awsRequestStubMap,
S3: {
deleteObjects: deleteObjectsStub,
listObjectsV2: { Contents: [{ Key: 'first' }, { Key: 'second' }] },
headBucket: () => {
const err = new Error('err');
err.code = 'AWS_S3_HEAD_BUCKET_NOT_FOUND';
throw err;
},
},
},
});

expect(deleteObjectsStub).not.to.be.called;
expect(deleteStackStub).to.be.calledWithExactly({ StackName: awsNaming.getStackName() });
expect(describeStackEventsStub).to.be.calledWithExactly({
StackName: awsNaming.getStackName(),
});
expect(describeStackEventsStub.calledAfter(deleteStackStub)).to.be.true;
});

it('skips attempts to remove S3 objects if S3 bucket resource missing from CloudFormation template', async () => {
const headBucketStub = sinon.stub();
const { awsNaming } = await runServerless({
fixture: 'function',
command: 'remove',
awsRequestStubMap: {
...awsRequestStubMap,
S3: {
...awsRequestStubMap.S3,
headBucket: headBucketStub,
},
CloudFormation: {
...awsRequestStubMap.CloudFormation,
describeStackResource: () => {
const err = new Error('does not exist for stack');
err.providerError = {
code: 'ValidationError',
};
throw err;
},
},
},
});

expect(headBucketStub).not.to.be.called;
expect(deleteObjectsStub).not.to.be.called;
expect(deleteStackStub).to.be.calledWithExactly({ StackName: awsNaming.getStackName() });
expect(describeStackEventsStub).to.be.calledWithExactly({
StackName: awsNaming.getStackName(),
});
expect(describeStackEventsStub.calledAfter(deleteStackStub)).to.be.true;
});

it('removes ECR repository if it exists', async () => {
describeRepositoriesStub.resolves();
const { awsNaming } = await runServerless({
Expand Down Expand Up @@ -134,4 +226,90 @@ describe('test/unit/lib/plugins/aws/remove/index.test.js', () => {

expect(deleteRepositoryStub).not.to.be.called;
});

it('should execute expected operations with versioning enabled if no object versions are present', async () => {
const listObjectVersionsStub = sinon.stub().resolves();

const { serverless } = await runServerless({
command: 'remove',
fixture: 'function',
configExt: {
provider: {
deploymentPrefix: 'serverless',
deploymentBucket: {
name: 'bucket',
versioning: true,
},
},
},
awsRequestStubMap: {
...awsRequestStubMap,
S3: {
listObjectVersions: listObjectVersionsStub,
headBucket: {},
},
},
});

expect(listObjectVersionsStub).to.be.calledWithExactly({
Bucket: 'bucket',
Prefix: `serverless/${serverless.service.service}/dev`,
});
});

it('should execute expected operations with versioning enabled if object versions are present', async () => {
const listObjectVersionsStub = sinon.stub().resolves({
Versions: [
{ Key: 'object1', VersionId: null },
{ Key: 'object2', VersionId: 'v1' },
],
DeleteMarkers: [{ Key: 'object3', VersionId: 'v2' }],
});

const innerDeleteObjectsStub = sinon.stub().resolves({
Deleted: [
{ Key: 'object1', VersionId: null },
{ Key: 'object2', VersionId: 'v1' },
{ Key: 'object3', VersionId: 'v2' },
],
});

const { serverless } = await runServerless({
command: 'remove',
fixture: 'function',
configExt: {
provider: {
deploymentPrefix: 'serverless',
deploymentBucket: {
name: 'bucket',
versioning: true,
},
},
},
awsRequestStubMap: {
...awsRequestStubMap,
S3: {
listObjectVersions: listObjectVersionsStub,
deleteObjects: innerDeleteObjectsStub,
headBucket: {},
},
},
});

expect(listObjectVersionsStub).to.be.calledWithExactly({
Bucket: 'bucket',
Prefix: `serverless/${serverless.service.service}/dev`,
});

expect(innerDeleteObjectsStub).to.be.calledWithExactly({
Bucket: 'bucket',
Delete: {
Objects: [
{ Key: 'object1', VersionId: null },
{ Key: 'object2', VersionId: 'v1' },
{ Key: 'object3', VersionId: 'v2' },
],
},
});
});
});
Loading