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

Load static assets from S3 directly #86

Merged
merged 1 commit into from
Jul 16, 2024

Conversation

nimalank7
Copy link
Contributor

@nimalank7 nimalank7 commented Jul 5, 2024

Description:

  • Static assets (such as CSS, images and Javascript) are stored in an S3 bucket. Currently to retrieve a static asset Fastly will make a request to an nginx pod running in router. nginx will then retrieve the static asset from S3 before passing it to Fastly.
  • This commit makes Fastly talk directly to S3 without going through nginx and Router by are handled in the if block above which triggers a restart causing Fastly to poll the S3 mirror instead.
  • If the static asset doesn't exist in S3 (e.g. www.gov.uk/assets/non-existent.jpg) then S3 returns an XML document and 403. Adding the condition to display a prettified error page for this would make the VCL code difficult to read as well as a special case to handle integration which has no mirrors. Since this error can only be seen if someone looks in the Chrome dev tools it's agreed that it is permissible to just return the S3 403 error instead.
  • See the diagram in Fast path for Rails assets #73 for the previous method and the proposed new implementation

@nimalank7 nimalank7 force-pushed the load-rails-static-assets-from-S3-directly branch 5 times, most recently from 5aaf085 to 550f2ef Compare July 11, 2024 15:16
@nimalank7 nimalank7 marked this pull request as ready for review July 11, 2024 15:28
@nimalank7 nimalank7 force-pushed the load-rails-static-assets-from-S3-directly branch from 550f2ef to d313e4a Compare July 11, 2024 15:29
set beresp.ttl = 31536000s;
set beresp.http.Cache-Control = "max-age=31536000, public, immutable";
set beresp.http.Access-Control-Allow-Origin = "*";
set beresp.http.Access-Control-Allow-Methods = "GET, OPTIONS";
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need OPTIONS? Usually that's only for CORS preflight requests (when a client checks before sending a large POST/PUT request) but that wouldn't apply here since we're just serving static files and not handling uploads.

Copy link
Contributor Author

@nimalank7 nimalank7 Jul 15, 2024

Choose a reason for hiding this comment

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

Yes you're right. We don't even need the Access-Control-Allow-Methods nor the Access-Control-Allow-Headers as these are only set with CORS preflight requests which we don't do as we're only doing simple GET requests.

@nimalank7 nimalank7 force-pushed the load-rails-static-assets-from-S3-directly branch from d313e4a to d89db1c Compare July 16, 2024 10:52
Description:
- Static assets (such as CSS, images and Javascript) are stored in an S3 bucket. Currently to retrieve a static asset Fastly will make a request to an nginx pod running in router. nginx will then retrieve the static asset from S3 before passing it to Fastly.
- This commit makes Fastly talk directly to S3 without going through nginx and Router by are handled in the if block above which triggers a restart causing Fastly to poll the S3 mirror instead.
- If the static asset doesn't exist in S3 (e.g. www.gov.uk/assets/non-existent.jpg) then S3 returns an XML document and 403. Adding the condition to display a prettified error page for this would make the VCL code difficult to read as well as a special case to handle integration which has no mirrors. Since this error can only be seen if someone looks in the Chrome dev tools it's agreed that it is permissible to just return the S3 403 error instead.
- See the diagram in #73 for the previous method and the proposed new implementation
@nimalank7 nimalank7 merged commit 44a43ca into main Jul 16, 2024
5 checks passed
@nimalank7 nimalank7 deleted the load-rails-static-assets-from-S3-directly branch July 16, 2024 11:26
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.

2 participants