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

Aws Cloudformation Template Added #434

Merged
merged 1 commit into from
Aug 27, 2024
Merged

Conversation

Honricris
Copy link
Contributor

This CloudFormation template sets up the necessary AWS infrastructure to serve map tiles using Protomaps. The template provisions an S3 bucket for storing .pmtiles files, a Lambda function to serve these tiles, and a CloudFront distribution for global content delivery

Type: 'AWS::CloudFront::ResponseHeadersPolicy'
Properties:
ResponseHeadersPolicyConfig:
Name: 'protomaps-cors'
Copy link
Member

Choose a reason for hiding this comment

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

Can this be given a scoped name like !Sub "${AWS::StackName}-ResponseHeadersPolicy"? otherwise it looks like it will conflict if there is two stacks using this template.

CorsConfig:
AccessControlAllowOrigins:
Items:
- 'https://example.com' # Replace with your allowed origin
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to configure this via parameters?

Export:
Name: !Sub "${AWS::StackName}-S3BucketURL"

LambdaFunctionUrl:
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove this block because the LambdaFunctionUrl is an implementation detail.

Type: String

CodeKey:
Description: 'The S3 key for the Lambda function code (e.g., lambda_function.zip)'
Copy link
Member

Choose a reason for hiding this comment

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

I think we can hardcode this as lambda_function.zip and require that to be the key name, instead of needing user configuration

Choose a reason for hiding this comment

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

Added a similar comment below, but an alternative to the ZipFile would be to internally manage this:

# Example of inline code
Code:
        ZipFile: |
          const { S3Client, ListBucketsCommand } = require("@aws-sdk/client-s3");
          const s3 = new S3Client({ region: "us-east-1" }); // replace "us-east-1" with your AWS region

          exports.handler = async function(event) {
            const command = new ListBucketsCommand({});
            const response = await s3.send(command);
            return response.Buckets;
          };

This would be challenging if there are any need for lambda layers with dependencies which cannot be easily minified in JS.

ProtomapsLambdaFunction:
Type: 'AWS::Lambda::Function'
Properties:
FunctionName: protomaps
Copy link
Member

Choose a reason for hiding this comment

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

same as below - maybe assign it a unique name based on the stack name?

Description: 'The name of the S3 bucket where you will store pmtiles files to be served (must be globally unique)'
Type: String

CodeBucketName:
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about combining the buckets?

That you only need to provision one bucket to hold both the lambda_function.zip and your tilesets

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, we can expect the user to have the bucket created beforehand, with the lambda code inside and then use it for storing the tiles. If you are ok with that i will change it.

Choose a reason for hiding this comment

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

I think separating the bucket for vending tiles and storing the lambda code makes sense. You may want completely different settings on those assets (e.g. tiles are vended via cloudfront, whereas code stays private).

An alternative would be to store the javascript for the lambda inline in the template itself (see documentation for Zipfile at https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-lambda-function-code.html). While this may not be the most readable, it removes any additional work before deploying the template.

Ideally the deployment of the CF is as close to one-click as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't know you could directly store the code in the template, it could be an option too. I mean you can delete the code and the bucket after the setup. I have doubts wether to store the code on the bucket , in a different one or in the template itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would say the code is too large to include it in the template. Using a separate bucket for the configuration is a common approach, but is up to us how to do it. Any suggestion on what to do?

Copy link
Member

@bdon bdon Aug 26, 2024

Choose a reason for hiding this comment

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

You may want completely different settings on those assets (e.g. tiles are vended via cloudfront, whereas code stays private).

the way the Lambda works now is it appends .pmtiles to any archive name, so there is no way to address the .zip file in the bucket by default.

I like the idea of inline if we can slim down the Lambda code significantly. For example, we don't need the polyfill for gzip, which is the only dependency. I may do some checks on the practical limit for inline code size.

Copy link
Member

Choose a reason for hiding this comment

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

Inlining the Lambda code is 1634 lines long, and requires a CommonJS output instead of ESM, but otherwise works perfectly and makes this deployment very close to one-click.

We can save the YAML as a template and then inline the CommonJS bundle programatically. I'm tempted to make this the primary deployment method for the AWS code. Any strong objections?

Copy link
Member

Choose a reason for hiding this comment

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

It turns out also that the zip file approach cannot be updated in-place; you need to give it a new object key since CloudFormation does not detect content changes. So another benefit of inlining.

Copy link

@charliemcgrady charliemcgrady Aug 30, 2024

Choose a reason for hiding this comment

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

Inlining the Lambda code is 1634 lines long, and requires a CommonJS output instead of ESM, but otherwise works perfectly and makes this deployment very close to one-click.

We can save the YAML as a template and then inline the CommonJS bundle programatically. I'm tempted to make this the primary deployment method for the AWS code. Any strong objections?

+1.

CloudFormation templates with inline lambdas are about as one-click as you can get currently on AWS. This will open up deploying PMTiles to more non-technical folks. You also can consume these templates in CDK repositories without needing to manage Typescript versioning, etc. using CfnInclude so seems like a win-win.

@jaanli
Copy link

jaanli commented Aug 24, 2024

Can we get an example for how this would work with @cloudflare? I’m happy to put one together for @onefact if needed!

@bdon
Copy link
Member

bdon commented Aug 24, 2024

This is specific to AWS, for automated Cloudflare deployment use Wrangler: https://docs.protomaps.com/deploy/cloudflare#alternative-use-wrangler

Comment on lines +90 to +91
Cors:
AllowOrigins: ["*"]

Choose a reason for hiding this comment

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

I don't think you need CORS since this lambda is invoked by CloudFront and not directly by the client?

@bdon
Copy link
Member

bdon commented Aug 27, 2024

I'm going to merge this into main as a WIP and then make some followup adjustments. Thanks for your contribution @Honricris !

@bdon bdon merged commit 0fd91fa into protomaps:main Aug 27, 2024
2 checks passed
@Honricris
Copy link
Contributor Author

I'm going to merge this into main as a WIP and then make some followup adjustments. Thanks for your contribution @Honricris !

No problem!! I have the updated YAML with all the changes you proposed and the inline code added if you want me to pull request it i'll be pleased to do so.

@bdon
Copy link
Member

bdon commented Aug 27, 2024

I made a bunch of changes in https://github.com/protomaps/PMTiles/pull/435/files, mostly:

  • letting AllowedOrigins as a parameter
  • making the TileJSON work by default without a PublicHostname set (requires adding a CloudFront function to detect the distribution domain name)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants