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

feat(aws-s3-storage): serve tarball files via CDN #250

Closed
favoyang opened this issue Nov 16, 2019 · 11 comments
Closed

feat(aws-s3-storage): serve tarball files via CDN #250

favoyang opened this issue Nov 16, 2019 · 11 comments

Comments

@favoyang
Copy link
Contributor

favoyang commented Nov 16, 2019

Is your feature request related to a problem? Please describe.

The intention is to improve the performance of tarball files distribution by leveraging S3 integrated CDN service, like Amazon CloudFront.

Prerequisites

@favoyang favoyang changed the title aws-s3-storage: Add support for s3 ACL aws-s3-storage: add S3 ACL configuration for tarball Nov 17, 2019
@juanpicado
Copy link
Member

It would be great if you elaborate a bit better your since for those are not familiar with AWS and specific features, provide examples to identify the areas you want to improve and about your PR seems to be incomplete, perhaps specify what's missing. As I commented verdaccio/verdaccio#1580 any specific feature for AWS has to be implemented here.

@favoyang favoyang changed the title aws-s3-storage: add S3 ACL configuration for tarball aws-s3-storage: enable built-in CDN/Edge service for S3 store for serving tarball Nov 19, 2019
@favoyang favoyang changed the title aws-s3-storage: enable built-in CDN/Edge service for S3 store for serving tarball aws-s3-storage: enable S3 built-in CDN/Edge service for tarball serving Nov 19, 2019
@favoyang
Copy link
Contributor Author

favoyang commented Nov 19, 2019

@juanpicado updated issue description for intention, solution and implementation details.

I don't know what's the specific incomplete thing you addressed, only part of unit test is missing (see reason above).

Please also checkout verdaccio/verdaccio#1580 (comment).

@favoyang favoyang changed the title aws-s3-storage: enable S3 built-in CDN/Edge service for tarball serving aws-s3-storage: Use S3 Built-in CDN/Edge to Serve Tarball Files Nov 23, 2019
@favoyang
Copy link
Contributor Author

favoyang commented Nov 23, 2019

@juanpicado #249 and verdaccio/verdaccio#1580 are ready to review.

@favoyang favoyang changed the title aws-s3-storage: Use S3 Built-in CDN/Edge to Serve Tarball Files feat(aws-s3-storage): serve tarball files via CDN Jan 27, 2020
favoyang added a commit to favoyang/monorepo that referenced this issue Jan 29, 2020
@favoyang
Copy link
Contributor Author

@juanpicado the issue is re-implemented in #249 #320 verdaccio/verdaccio#1688.

@juanpicado
Copy link
Member

Thanks 🙏 I will put it on my list and back to you with feedback

@juanpicado
Copy link
Member

@favoyang
Copy link
Contributor Author

@juanpicado,

I have checked these threads. It seems people get confused for the url_prefix behavior - assuming setting it to /foo will turn entire verdaccio services (API endpoints, UI endpoints, static files) served under /foo. Seems url_prefix won't affect static files serving which always served under /-/static/? So people start discussing adding an additional rule for handling the static files in their reverser proxy server.

handle /-/static/* {
        reverse_proxy localhost:4873
}

The documentation didn't explain the details,
image

I think the current behavior is counter-intuitive and can be considered a bug. If I set url_prefix to /foo, then everything should be served under it, including /foo/-/static/. Because in most cases set url_prefix means to deploy verdaccio as a component to a server where other endpoints used by other services.

In v5 VERDACCIO_PUBLIC_URL has been introduced. Does it deprecated url_prefix? I'm not sure if this a good idea. I may prefer to use VERDACCIO_PUBLIC_URL to describe the base URL (https://domain,tld) without any prefix or trailing slash. Together with url_prefix, it can be used for referring to full URLs within the verdaccio UI or in notification emails. But if you already decide the usage for VERDACCIO_PUBLIC_URL, then it just need a good documentation.

Anyway, these are not related to this issue #250.

@juanpicado
Copy link
Member

juanpicado commented Mar 25, 2021

@juanpicado,

I have checked these threads. It seems people get confused for the url_prefix behavior - assuming setting it to /foo will turn entire verdaccio services (API endpoints, UI endpoints, static files) served under /foo. Seems url_prefix won't affect static files serving which always served under /-/static/? So people start discussing adding an additional rule for handling the static files in their reverser proxy server.

handle /-/static/* {
        reverse_proxy localhost:4873
}

The documentation didn't explain the details,
image

Currently does not affect static files as you says, but it will. That documentation will be updated once the PR is being merged and released (we don't have versioning on docs anymore), those additional ngix rules won't be need it anymore and should be removed (it will be on the changelog notes so people is aware). The url_prefix will allow only subdirectory, since the VERDACCIO_PUBLIC_URL can be used to specify an specific url in runtime easily.

I think the current behavior is counter-intuitive and can be considered a bug. If I set url_prefix to /foo, then everything should be served under it, including /foo/-/static/. Because in most cases set url_prefix means to deploy verdaccio as a component to a server where other endpoints used by other services.

That's what the new PR intend to fix. The issue of the static files is not that easy to solve (https://serverfault.com/a/932636). UI files are generated with a public path and once some JS chunks are loaded cannot detect are being loaded behind a proxy unless you use a complete URI, not to mention the harcoded values UI had for svg, css (url:xx) and fonts.

In v5 VERDACCIO_PUBLIC_URL has been introduced.

Yes, but I ported that to v4 as well since I wanna give a solution to the url_prefix confusion and issues.

Does it deprecated url_prefix?

No, VERDACCIO_PUBLIC_URL is useful in cases when verdaccio/verdaccio#2129 the host header cannot be defined. Env var are documented under docs/env.variables.md.

I'm not sure if this a good idea. I may prefer to use VERDACCIO_PUBLIC_URL to describe the base URL (https://domain,tld) without any prefix or trailing slash.

If you tell me exactly why, it is not written in stone yet, PR is draft and v5 can change since was not released yet. I'd love listen your opinion.

Together with url_prefix, it can be used for referring to full URLs within the verdaccio UI or in notification emails. But if you already decide the usage for VERDACCIO_PUBLIC_URL, then it just need a good documentation.

Anyway, these are not related to this issue #250.

Agree, just related, #250 on my queue.

@favoyang
Copy link
Contributor Author

If you tell me exactly why, it is not written in stone yet, PR is a draft and v5 can change since was not released yet. I'd love listen your opinion.

I guess we may talk about the same thing.

  • VERDACCIO_PUBLIC_URL is the host with a format of https://domain.tld (i.e. https://mywebsite.com).
  • url_prefix is still useful (i.e. /npm).
  • VERDACCIO_PUBLIC_URL + url_prefix will get the final URL (i.e. https://mywebsite.com/npm) can be used as the base URL for referring assets in HTML or in notification emails (if verdaccio has any email features now or later).

It just feels consistent, so either you have

  • only one configuration to set as /npm or https://mywebsite.com/npm.
  • or you have two configurations, one for the domain, one for the prefix.

But you probably won't want a conditional configuration.

On this version you can use the env variable VERDACCIO_PUBLIC_URL to define specific public URL instead relay on the host header, also will ignore url_prefix` #2129.

Or you may need more specific examples of how it works with url_prefix.

@juanpicado
Copy link
Member

Not sure to be honest, maybe @verdaccio/collaborators have some extra thoughts on this.

@favoyang
Copy link
Contributor Author

favoyang commented May 4, 2021

Closed by #249 and verdaccio/verdaccio#1688

@favoyang favoyang closed this as completed May 4, 2021
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

No branches or pull requests

2 participants