feat(AWS Deploy): Ensure existence of S3 bucket if possible #10317

pgrzesik · 2021-12-07T10:17:51Z

Follow up to: #10306

Closes: #3964

On a best-effort basis tries to ensure that S3 bucket for deployment artifact exists. Handles the following scenarios:

If Deployment Bucket is missing in CF template, then it injects the config for it and issues additional update
If Deployment Bucket is defined in CF template but has been removed manually, throws a descriptive error
If custom Deployment Bucket is used, it is validated for location and existence, if it doesn't exist then descriptive error is thrown

pgrzesik · 2021-12-07T10:19:04Z

lib/plugins/aws/deploy/index.js

@@ -104,11 +107,6 @@ class AwsDeploy {
      'before:deploy:deploy': async () => {
        await this.serverless.pluginManager.spawn('aws:common:validate');

-        const bucketName = this.serverless.service.provider.deploymentBucket;
-        if (bucketName) {


This logic is now fully handled in ensureValidBucketExists

pgrzesik · 2021-12-07T10:19:50Z

lib/plugins/aws/deploy/lib/ensure-valid-bucket-exists.js

+    // If bucket name is set, it means it's defined as a part of CloudFormation template (custom bucket case was handled by logic above)
+    if (this.bucketName) {
+      if (!(await this.checkIfBucketExists(this.bucketName))) {
+        // It means that bucket was removed manually but is still a part of the CloudFormation stack, we cannot manually fix it


I double checked if we can force CloudFormation deploy with the original template, but as expected, the update is rejected because "nothing changed"

lib/plugins/aws/deploy/lib/ensure-valid-bucket-exists.js

codecov · 2021-12-07T10:21:44Z

Codecov Report

Merging #10317 (8fe8f66) into master (1a85a4a) will decrease coverage by 0.05%.
The diff coverage is 76.56%.

❗ Current head 8fe8f66 differs from pull request most recent head 0b4e5ad. Consider uploading reports for the commit 0b4e5ad to get more accurate results

@@            Coverage Diff             @@
##           master   #10317      +/-   ##
==========================================
- Coverage   85.43%   85.38%   -0.06%     
==========================================
  Files         340      340              
  Lines       13936    13984      +48     
==========================================
+ Hits        11906    11940      +34     
- Misses       2030     2044      +14

Impacted Files	Coverage Δ
...ugins/aws/deploy/lib/ensure-valid-bucket-exists.js	`75.40% <75.40%> (ø)`
lib/plugins/aws/deploy/index.js	`100.00% <100.00%> (+1.21%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a85a4a...0b4e5ad. Read the comment docs.

medikoo

@pgrzesik that looks really great! I have just a few minor suggestions

medikoo · 2021-12-07T11:38:53Z

lib/plugins/aws/deploy/index.js

@@ -55,6 +57,7 @@ class AwsDeploy {
              deploy: {
                lifecycleEvents: [
                  'createStack',
+                  'ensureValidBucketExists',


Do you think we need a new lifecycle event? Why not just do it at L130, before setBucketName ?

I started with it in checkForChanges, but it didn't feel right to have logic that "modifies" something in step called check, which suggests that we're only doing checking here. But I see a point of not adding extra lifecycle events - what do you think is better here - having a little bit "inconsistent" (mutating) operation as a part of "check" logic or having a separate lifecycle event?

Currently logic that resolves bucketName and crashes if there's no bucket is in context of checkForChanges, so I think it's ok to add there recovery logic, which instead of crashing ensures the bucket

I'm also not sure about changing our current lifecycle events set, I'd rather treat it more as part of legacy design that's best if left untouched (and not further explored). To avoid unexpected issues I'd rather introduce new lifecycle events only if there's really hard to go without it. WDYT?

That's a good point - let's treat lifecycle engine as "legacy" and not introduce new hooks unless there's a really good reason for it. I've changed the implementation.

medikoo · 2021-12-07T11:39:51Z

lib/plugins/aws/deploy/index.js

@@ -126,6 +124,10 @@ class AwsDeploy {
        mainProgress.notice('Retrieving CloudFormation stack', { isMainEvent: true }),
      'aws:deploy:deploy:createStack': async () => this.createStack(),

+      'before:aws:deploy:deploy:ensureValidBucketExists': () =>
+        mainProgress.notice('Ensuring that deployment bucket exists', { isMainEvent: true }),


I'm not sure we need a new progress event here.

It feels to me as part of Retrieving CloudFormation stack part, and it's pretty fast, so unlikely to be noticeable

I wasn't sure how to capture that properly, but I like your suggestions around progress 👍

medikoo · 2021-12-07T11:49:55Z

lib/plugins/aws/deploy/lib/ensure-valid-bucket-exists.js

+    const templateBody = _.merge(
+      JSON.parse(getTemplateResult.TemplateBody),
+      this.serverless.service.provider.coreCloudFormationTemplate
+    );


_.merge doesn't feel safe and we overuse it. I'd do:

Object.assign(getTemplateResult.TemplateBody.Resources, this.serverless.service.provider.coreCloudFormationTemplate.Resources); // Same for Outputs Object.assign(..Outputs, ...Outputs);

Sounds good 👍

medikoo · 2021-12-07T11:51:35Z

lib/plugins/aws/deploy/lib/ensure-valid-bucket-exists.js

+    );
+    const stackName = this.provider.naming.getStackName();
+
+    // This is situation where the bucket is not defined in the template at all


Here I would register another main progress with title as: Ensure deployment bucket

Sounds good 👍

medikoo · 2021-12-08T09:13:40Z

lib/plugins/aws/deploy/index.js

@@ -127,6 +124,7 @@ class AwsDeploy {
      'aws:deploy:deploy:createStack': async () => this.createStack(),

      'aws:deploy:deploy:checkForChanges': async () => {
+        await this.ensureValidBucketExists();
        await this.setBucketName();


This line I believe now can be removed

This line is still needed, but potentially could be moved inside ensureValidBucketExists - after the logic of ensureValidBucketExists, in scenario where we create a new bucket via CF update, we don't know the bucket name and we have to retrieve it via SDK and that call handles that. What do you think, would it be better to just call it at the end of processing in ensureValidBucketExists?

I propose to simplify it as:

Remove setBucketName() entirely (I've checked and it's safe against plugins)

In ensureValidBucketExists,ensure following logic is run:

if (!this.bucketName) { this.bucketName = await this.provider.getServerlessDeploymentBucketName(); }

I think we cannot remove setBucketName entirely, because it's also used in rollback and deployList logic. as for the proposed syntax, we can definitely do that but adjust the logic in ensureValidBucketExists a bit more

I've removed the additional setBucketName in checkForChanges hook and adjusted the implementation of ensureValidBucketExists - at the end of the function we should unconditionally call that method because we know the bucketName is not set (because we just ran the update for missing bucket in CF template)

medikoo

Looks great 👍

pgrzesik added enhancement cat/deployment labels Dec 7, 2021

pgrzesik commented Dec 7, 2021

View reviewed changes

lib/plugins/aws/deploy/lib/ensure-valid-bucket-exists.js Show resolved Hide resolved

pgrzesik requested a review from medikoo December 7, 2021 10:26

pgrzesik self-assigned this Dec 7, 2021

medikoo reviewed Dec 7, 2021

View reviewed changes

pgrzesik force-pushed the ensure-existence-of-deployment-bucket branch 2 times, most recently from f1a96db to 8ac2439 Compare December 7, 2021 16:20

pgrzesik requested a review from medikoo December 7, 2021 16:37

medikoo reviewed Dec 8, 2021

View reviewed changes

feat(AWS Deploy): Ensure existence of S3 bucket if possible

0b4e5ad

pgrzesik force-pushed the ensure-existence-of-deployment-bucket branch from 8ac2439 to 0b4e5ad Compare December 8, 2021 12:38

pgrzesik requested a review from medikoo December 8, 2021 12:55

medikoo approved these changes Dec 8, 2021

View reviewed changes

pgrzesik merged commit f358585 into master Dec 8, 2021

pgrzesik deleted the ensure-existence-of-deployment-bucket branch December 8, 2021 13:18

medikoo mentioned this pull request Dec 21, 2021

refactor(AWS Deploy): Use change sets in CF deployments #10390

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(AWS Deploy): Ensure existence of S3 bucket if possible #10317

feat(AWS Deploy): Ensure existence of S3 bucket if possible #10317

pgrzesik commented Dec 7, 2021

pgrzesik Dec 7, 2021

pgrzesik Dec 7, 2021

codecov bot commented Dec 7, 2021 •

edited

medikoo left a comment

medikoo Dec 7, 2021

pgrzesik Dec 7, 2021

medikoo Dec 7, 2021

pgrzesik Dec 7, 2021

medikoo Dec 7, 2021

pgrzesik Dec 7, 2021

medikoo Dec 7, 2021

pgrzesik Dec 7, 2021

medikoo Dec 7, 2021

pgrzesik Dec 7, 2021

medikoo Dec 8, 2021

pgrzesik Dec 8, 2021

medikoo Dec 8, 2021

pgrzesik Dec 8, 2021 •

edited

pgrzesik Dec 8, 2021

medikoo left a comment

feat(AWS Deploy): Ensure existence of S3 bucket if possible #10317

feat(AWS Deploy): Ensure existence of S3 bucket if possible #10317

Conversation

pgrzesik commented Dec 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 7, 2021 • edited

Codecov Report

medikoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgrzesik Dec 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medikoo left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 7, 2021 •

edited

pgrzesik Dec 8, 2021 •

edited