eclipse / openvsx

An open-source registry for VS Code extensions
https://open-vsx.org/
Eclipse Public License 2.0
1.24k stars 134 forks source link

Rate Limit API Calls issue when building an application #587

Open guellaeq opened 1 year ago

guellaeq commented 1 year ago

I am currently facing an issue when building an Eclipse Theia based application which downloads VScode extensions. I get some 429 http error when downloading those plugins at build time (more than 80 extensions).

This error only started to happen recently without much change on the application source code itself. I'm pretty sure it is related to #509

Is this kind of use cases in the scope of this API call rate limiter ?

amvanbaren commented 1 year ago

cc @kineticsquid

marcdumais-work commented 1 year ago

We are also seeing 429's in GitHub CI builds for Theia-based applications.

Is the API rate limit applicable per client IP address? If so, it might disproportionately impact builds/users behind a proxy, that will all seem to originate from the same IP.

When a similar issue happened with the GitHub API rate limit, we were able to authenticate by using a Personal Access Token during the build, and then the rate limit for that build would be accounted for that specific user, instead of all users behind the same IP address.

kineticsquid commented 1 year ago

@amvanbaren Will the approach outlined by @marcdumais-work work?

amvanbaren commented 1 year ago

@kineticsquid It will work, but there's currently no limit on the amount of active personal access tokens a user can have.

kineticsquid commented 1 year ago

@amvanbaren I think that's fine for now.

@marcdumais-work @guellaeq OK with this approach?

marcdumais-work commented 1 year ago

AFAIK, only "publisher" PAT are available in openvsx? For the above to be a practical solution, I think we would need the ability to obtain a token that does not require signing the Eclipse Foundation publisher agreement (or enable publishing).

A basic "read-only" token would only serve to identify a user and permit applying API rate limits towards that user, instead of using the IP address as a proxy to account usage.

marcdumais-work commented 1 year ago

@guellaeq as a mitigation you can use the following option to force your Theia application's builtin extensions to be downloaded sequentially - it seems to help in our CI:

yarn -s download:plugins --parallel=false

amvanbaren commented 1 year ago

@marcdumais-work You need to sign the Eclipse publisher agreement to be able to create a namespace or publish an extension. https://github.com/eclipse/openvsx/blob/95d53c3f42c3d13e393ba4e6055acfcf700bcffa/server/src/main/java/org/eclipse/openvsx/LocalRegistryService.java#L422-L428 https://github.com/eclipse/openvsx/blob/95d53c3f42c3d13e393ba4e6055acfcf700bcffa/server/src/main/java/org/eclipse/openvsx/LocalRegistryService.java#L467-L474 You can create a basic "read-only" token by using a user that hasn't signed the publisher agreement. https://github.com/eclipse/openvsx/blob/95d53c3f42c3d13e393ba4e6055acfcf700bcffa/server/src/main/java/org/eclipse/openvsx/UserAPI.java#L163-L185

marcdumais-work commented 1 year ago

You can create a basic "read-only" token by using a user that hasn't signed the publisher agreement.

Here's what I see: image

kineticsquid commented 1 year ago

@mmatthewkhouzam FYI...

amvanbaren commented 1 year ago

Ah, oke. There's a check in the webui: https://github.com/eclipse/openvsx/blob/95d53c3f42c3d13e393ba4e6055acfcf700bcffa/webui/src/pages/user/user-settings-tokens.tsx#L120-L131

I think we can remove this. Just have to make sure to check for a valid publisher agreement when a publishing operation is performed.

MatthewKhouzam commented 1 year ago

Hey, I was just mulling an idea: could we have a separate vsx for bots... something with awful (10 minute) timeouts... and a token so only member projects can use it. I think VSX is owned by CDT, it would be interesting to separate the robots from the humans.

guellaeq commented 1 year ago

@amvanbaren I think that's fine for now.

@marcdumais-work @guellaeq OK with this approach?

In the CI context, I'm not sure that a PAT will be sufficient to avoid all errors in case of peak load. At least, it would differentiate calls made by other developers from CI calls.

kineticsquid commented 1 year ago

@MatthewKhouzam Your idea is a special access token for bots (CI piplines) that doesn't have the same rate limits? One would have to be a member of the Cloud DevTools working group to get such a token?

@amvanbaren what'd the work effort be like to implement this kind of special member token? Could there be light weight, even manual, admin process for generating and assigning the tokens?

amvanbaren commented 1 year ago

@kineticsquid I don't know. What you propose is a complete new process.

MatthewKhouzam commented 1 year ago

@MatthewKhouzam Your idea is a special access token for bots (CI piplines) that doesn't have the same rate limits? One would have to be a member of the Cloud DevTools working group to get such a token?

Depends on your decision as you own the product. I would say a tiered approach is not bad. Focus on the paying customers first and all.

kineticsquid commented 1 year ago

Well my desire would be to keep this as simple as possible :). Let's assume we have a manual process for generating these bot keys for work group member companies. This bot access key would have a different set of rate limiting constraints. What would those constraints need to be? Where would this token live?

amvanbaren commented 1 year ago

@kineticsquid We can add the premium role to users. Users with the premium role can make a higher number of requests per second than regular users. This requires a database lookup to find the user that is associated to the token. The database query can slow down the request rate. It might be better to use JWT as token implementation, because it lets you define claims as part of the token.

marcdumais-work commented 1 year ago

Starting in Theia v1.32.0 released yesterday, the download of built-in extensions will be sequential by default. This should mitigate the triggering of openvsx API limit when Theia-based applications are build, for example during CI.

https://github.com/eclipse-theia/theia/pull/11860/files#diff-11e38961bd54fcce86babf5ea40cf9bf4538cb2f519763c8fa7a897e94c35f71

amvanbaren commented 1 year ago

Ok, so this issue can be closed?

guellaeq commented 1 year ago

In case of multiple builds in parallel, I'm not sure the sequentialization of the download will be enough. I work with a multi repo environment, where each new feature will trigger a Theia build. All this behind one single IP/user. Maybe it will be a temporary fix, but in the end, this issue can come back during peak loads on CI.

kineticsquid commented 1 year ago

Let's explore this a bit. @amvanbaren, what are the current rate limits? I see the 429 in the API doc but I couldn't find if/where we document what those limits are.

amvanbaren commented 1 year ago

The rate limits are configured in application.yml https://github.com/EclipseFdn/open-vsx.org/blob/33ab99a88a17665765dad980e1b78833184cbb23/configuration/application.yml#L117-L120

kineticsquid commented 1 year ago

If I'm reading the .yml file correctly,

Once that is exceeded, where does the wait period get specified?

amvanbaren commented 1 year ago

If a client does 15 calls in the first 15 ms, then the client has to wait 985 ms to do the 16th call. There's no additional wait or timeout.