Shorten the S3 Bucket for SLS Deployment information

[brendo]

This is a Bug Report

Description

I believe the default template for creating a serverless deployment bucket is this and therefore the resulting bucket name will be at least 40 characters, serverlessdeploymentbucket-abc123def456g. This leaves projects 23 characters before it hits S3 naming length.

May I suggest the template be revised from serverlessdeploymentbucket to slsdeploy?

• serverless is commonly abbreviated to sls, in docs and even in CLI usage
• deployment is commonly abbreviated to deploy in the IT industry
• bucket is superfluous, we know it's going to be a bucket.

This would mean Serverless uses 23 characters and gives projects 40 characters of freedom :)

For bug reports:

• What went wrong? I was unable to deploy a project due to my bucket name exceeding the limit.
• What did you expect should have happened? I should of been able to deploy the project.

$ serverless deploy --region ap-southeast-2 --profile storage --env brendanabbott -v

Additional Data

• Serverless Framework Version you're using: 1.2.1
• Operating System: MacOS
• Stack Trace:
• Provider Error messages:

Serverless: Packaging service...
Serverless: Uploading CloudFormation file to S3...
 
  Serverless Error ---------------------------------------
 
     Missing required key 'Bucket' in params
 
  Get Support --------------------------------------------
     Docs:          docs.serverless.com
     Bugs:          github.com/serverless/serverless/issues
 
  Your Environment Information -----------------------------
     OS:                 darwin
     Node Version:       4.6.2
     Serverless Version: 1.2.1

[mthenw]

I think it's not a bug. I'm not saying that we shouldn't change that but the problem is that it's a breaking change (correct me if I'm wrong) but it will require existing services to redeploy. 23 chars is pretty long though.

[brendo]

but the problem is that it's a breaking change (correct me if I'm wrong)

True, if there are other plugins that are relying on the bucket being named a particular way this change would break them. For a user of Serverless, it should be rather seamless as the 'next' deploy would create the new bucket to hold the code before it's given to Lambda.

That could be a fault in my understanding, but I saw the S3 bucket as a temporary staging area before Lambda has the code. If that bucket was removed the Lambda would continue to function as the code has been transferred from the bucket.

23 chars is pretty long though.

It's actually 40 right now, this PR would make it 23 😉 It's an improvement, but I agree, it's still long. Perhaps sls is an option?

[thughes]

I'd also vote to shorten it to avoid the issue I hit in dancrumb/generator-serverless-policy#1

[jon-packetized]

@brendo thanks for the excellent write up - was running in circles trying to figure out where I was

Missing required key 'Bucket' in params

Thanks to this post, I realized I needed to shorten my service name from alert-manager to alert-mgr

I too vote to shorten it.

[HyperBrain]

First of all, I'm also for changing the default bucket name to allow for longer service names. Imo there are multiple ways to achieve this (one of them is the shortening of the default name prefix, mentioned above). Here are my thoughts with their resp. pros/cons (and a section about these breaking changes in general):

Shorten name prefix

Shortening the name prefix as above will extend the service name length, but still leaves it restricted. E.g. CF allows for stack-names of 255 characters, which theoretically would allow service names of much bigger length. Even the shortened prefix would not even come near this maximum limit.

Use hash to represent service name

The auto-generated name could create a hash (e.g. MD5) from the service name and use that instead in the bucket name generator. This would extend the service name length to infinite - only limited by the CF stack name restrictions. The obvious con is, that you cannot see the service name anymore when inspecting your buckets in S3.

Keep it as is

We also could keep the auto-generated name as is, and tell users to use the deploymentBucket service property to specify a custom name.

Breaking change - How to handle it

Changing the bucket name is in general a breaking change. There are some caveats that have to be addressed:

1. Bucket: The bucket is created by CF (the serverless bucket resource). Every deployed SLS CF stack contains the bucket resource. As long as there are any old stacks deployed, the old bucket cannot be removed - this will lead to deployment failures.
2. Functions: @brendo mentioned that Lambdas will continue to run, if you delete the bucket. For currently deployed Lambdas this is true, but any ROLLBACK in the corresponding CF stack (triggered by whatever reason), might trigger a recreation of a function resource, which in turn has the ZIP file referenced in its resource definition. So it still needs the old bucket.
   Serverless does not delete old deployed versions by default, and the beforementioned is true for all function versions that exist in AWS Lambda.
3. Rollback: The serverless rollback functionality would not work across the date of change.
4. Plugins: Plugins should use the deploymentBucket property (which is set either by the user or the bucket name generation). Imo they are not affected by any change here. If they rely on specific name layouts, I would treat this as a bug in the plugin - such a procedure is highly discouraged.

Possible solution to overcome breaking changes

The Serverless framework should only apply the new name semantics to newly created projects (i.e. projects that create a new CF stack). Old projects should continue to use their old bucket names and the framework should not enforce any change there.
Nevertheless Serverless would have to be bumped to a new major version, to prohibit any automatic updates, as reverting to a previous version would break the projects.

[brendo]

Sorry, I didn't consider rollbacks with my idea at all, nice pickup.

Also CF might be happy with 255 chars, S3 buckets are strongly recommended to be less than 63.

Using deploymentBucket can work, but there's one massive difference in that Serverless expects that bucket to already exist and it will not attempt to create it. I originally thought this was a way to override the bucket name but in testing it supports a different use case.

We agree though, it'd be nice to implement something for SLS2

[HyperBrain]

S3 buckets are strongly recommended to be less than 63.

That's exactly why I mentioned the hash solution. It will allow to have service names as long as the 255 character limit 😄

[brendo]

Does anyone actually have a reproducible use case for this issue?

I've tried to redo the PR with the feedback given here and in #2801 with a contrived use case, but I've actually found that CloudFormation appears to truncate S3 bucket names now, so I can't get it to exceed 63 characters (eg. this-is-my-really-long-b-serverlessdeploymentbuck-ywaa61zk6bkt).

In fact the error I run into instead is to do with my Role name (which can be solved by custom role statements).

Serverless: Packaging service...
Serverless: Uploading CloudFormation file to S3...
Serverless: Uploading artifacts...
Serverless: Uploading service .zip file to S3 (409 B)...
Serverless: Validating template...
Serverless: Updating Stack...
Serverless: Checking Stack update progress...
.........
Serverless: Deployment failed!
 
  Serverless Error ---------------------------------------
 
  An error occurred while provisioning your stack: IamRoleLambdaExecution - 1 validation error detected: Value 'this-is-my-really-long-bucket-name-that-will-fail-ba-test-us-east-1-lambdaRole' at 'roleName' failed to satisfy constraint: Member must have length less than or equal to 64.

I'm going to start a bisect to see if this is something that's been fixed accidentally along the way, or perhaps it's something that has been fixed in the AWS SDK (or CloudFormation) since Nov 2016?

Any ideas?

[HyperBrain]

The role name has a maximum length of 64 characters too (see http://docs.aws.amazon.com/IAM/latest/APIReference/API_CreateRole.html).

So we run into the same issue there with lengthy service names. Maybe a complete service name length fix would also need the same name generation/transformation when creating the IAM role names?

[brendo]

Maybe a complete service name length fix would also need the same name generation/transformation when creating the IAM role names?

Right now, I haven't been able to determine who is responsible for this behaviour, but I agree, the concept discussed for the bucket name could likely be rolled out for the Role Name as well.

[pmuens]

@brendo thanks for trying to reproduce this 👍 and thanks @HyperBrain for providing feedback 👍

@brendo What if you have a suuuuuper long service name (which itself has like 60 chars or smth. like that)?

(Here's some history on how the bucket naming has evolved over time)

We started by generating the deployment bucket name based on the service name in Serverless core and passed it as the name parameter to the S3 bucket resource which is used in the core CloudFormation template to create the stack.

However we removed this naming functionality later on (because we had different issues to ensure the uniqueness of the bucket name) and left the name property of the S3 bucket untouched / empty. That causes AWS to generate a name automatically.

[brendo]

What if you have a suuuuuper long service name (which itself has like 60 chars or smth. like that)?

I tried, thisisareallylongservicenametotestthebucketlengthandseeiftruncated (66 chars), and the bucket name was truncated to thisisareallylongservice-serverlessdeploymentbuck-1aljxweu3wjjd.

Overall, deployment failed though:

An error occurred while provisioning your stack: IamRoleLambdaExecution - 1 validation error detected: Value 'thisisareallylongservicenametotestthebucketlengthandseeiftruncated-dev-us-east-1-lambdaRole' at 'roleName' failed to satisfy constraint: Member must have length less than or equal to 64.

So I guess the question is, should we truncate the service name to prevent this issue?

[brendo]

RoleName must:

• Minimum length of 1. Maximum length of 64.
• Pattern: [\w+=,.@-]+

PolicyName must:

• Minimum length of 1. Maximum length of 128.
• Pattern: [\w+=,.@-]+

One possible proposal is to check here if the role is going to exceed 64 chars. If so, truncate the service name until it won't fail. Likewise for policyName, truncate service if the policy name is going to exceed 128 chars. I think this would maintain backwards compatibility because if you are already hitting the length limits, you're not able to deploy anyway.

Thoughts?

[HyperBrain]

@brendo I'd favor generating a hash of the service name instead of truncating it. The problem with truncating is, that you might end up with ambiguous names, in case multiple services start with the same prefix or appendix (depending on wich side you truncate).

Sample:

svc-thisisareallylongservicenametotestthebucketlengthandseeiftruncated-team1

vs. 

svc-thisisareallylongservicenametotestthebucketlengthandseeiftruncated-team2

If you would use a MD5 hash of the service name integrated in the resource names (role name, policy name, etc.), it would be completely unambiguous.

[brendo]

Good point. I guess once you hash, even though each name is unambiguous, you absolutely have no context of what you are looking at from the AWS console 😄

Still, that's better than the current state, which is nothing at all.

@oren-sarid is reporting reproducing that CF fails with the S3 bucket name in two versions of SLS, but I haven't been able to replicate yet. I'm going to try a couple of different regions to see if this 'auto truncate S3 name' behaviour is region dependent.

[HyperBrain]

Yes, hashed names are unexpressive. But isn't there a "description" property for nearly all resources or at least for the stack, where the clear-text name could be put into?

[brianneisler]

One thought here would be to truncate to the name to `64 - hashlength`. Then generate a hash from the removed portion of the name and appended it to the remaining portion. This is both backwards compatible as well as relatively certain to be unique.

…

On Sun, Jul 30, 2017 at 7:52 AM Frank Schmid ***@***.***> wrote: Yes, hashed names are unexpressive. But isn't there a "description" property for nearly all resources or at least for the stack, where the clear-text name could be put into? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2791 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZ15Fdhx37vC6JNCS5SPXdnc79fjczvks5sTG6ZgaJpZM4K8FQR> .

[brendo]

I'm afraid this is a bit like pandora's box! Enforcing the RoleName then reveals a new error with FunctionName exceeding the length. FunctionName is a bit tricker to remedy because it is defined outside of the AWS scope, in Service.

From what I tell by looking through the code, Functions have two "names", one acts like a key, which and is defined in the serverless config (eg. hello), the other is the fully expanded name (eg. ${service}-${stage}-hello).

I'm not really sure how to best modify the latter, it's exposed through via Service.getAllFunctionsNames or Service.getFunction and is called multiple times from various contexts. Unlike the other resources, getNormalizedFunctionName is only passed the key of the function, (hello), not the fully expanded name.

[oren-sarid]

My bucket is in Frankfurt (eu-central-1), if it matters.

[oren-sarid]

Seems this missed the 1.19.0 release as well. Any idea on when will the fix reach us?

Thanks

[pmuens]

Seems this missed the 1.19.0 release as well. Any idea on when will the fix reach us?

Hey @oren-sarid thanks for your comment 👍

This feature is currently still in-progress (see current status of #2801). The Serverless team is currently busy with products / features which will be announced at the Emit conference next week (17th of August). However we'll review and merge this feature as soon as it's updated again.

[brendo]

I should add that right now I don't know how to continue the work to have a fix, see this comment.

The original of having a bad bucket name I cannot reproduce. In all the tests I have done through multiple regions, CloudFormation truncates the bucket name and I cannot reproduce the failure.

We're going to need more detail to be able to reproduce. I tried to create a test case, but it passed for me.

[pmuens]

The original of having a bad bucket name I cannot reproduce. In all the tests I have done through multiple regions, CloudFormation truncates the bucket name and I cannot reproduce the failure.

Oh yes, I missed that! Thanks for pointing that out again @brendo 👍

---

So any help to reproduce (and of course fix) this bug is greatly appreciated!

---

I'll update the labels to reflect that.

[brendo]

I think I may of stumbled across a way to reproduce it. It seems to be that if you have the deployment bucket, and then it is removed, but the stack still think the bucket exists, than the "Missing required key 'Bucket' in params" error occurs. More tests to come!

(and yes, still face this on a semi regular basis!)

#4285 seems to hint I might be onto something.

[prumand]

I have the same problem. And as with #4285 the first error was Access denied. No idea yet how to fix it though.

[prumand]

When getting the getServerlessDeploymentBucketName it doesn't check for Resource Status. I receive:

{
  StackResourceDetail: {
     ...
     ResourceStatus: 'DELETE_COMPLETE',
    ...
  }
}

https://github.com/serverless/serverless/blob/master/lib/plugins/aws/provider/awsProvider.js#L248

[brendo]

Interesting! Having a look through the code I can't see any scenarios to handle getServerlessDeploymentBucketName not returning a bucket name.

@pmuens any ideas on how you want to handle this? It looks like getServerlessDeploymentBucketName is largely invoked by setBucketName, which is used in various parts of the codebase (deploy, deployList, rollback).

I guess, what experience do we want? Should the bucket be recreated and things carry on? Or just error with a more obvious message?

[pmuens]

Closing since this is quite and old issue and will result in a potential breaking change. We'll definitely re-visit this when working on v2.