Page MenuHomePhabricator

Update indirect dependency on github.com/gwicke/kad.git
Open, NormalPublic

Description

The limitation rate-limiting module pulls in a fork of kadence from a git master located at github.com/gwicke/kad.git. This fork hasn't been updated in a while and has begun throwing deprecation warnings on npm install:

npm WARN deprecated kad-memstore@0.0.1: Please upgrade to @kadenceproject/kadence - See https://kadence.github.io
npm WARN deprecated kad-fs@0.0.4: Please upgrade to @kadenceproject/kadence - See https://kadence.github.io

This should be cleaned up and hopefully brought up to date.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 25 2018, 9:55 PM
Mholloway renamed this task from Update indirect dependency on gwicke/kad.git to Update indirect dependency on github.com/gwicke/kad.git.Jul 25 2018, 9:56 PM
Mholloway updated the task description. (Show Details)
Mholloway updated the task description. (Show Details)Jul 26 2018, 3:27 AM
mobrovac triaged this task as Low priority.Jul 30 2018, 10:31 AM
mobrovac added a project: Services (later).

And transferred to wikimedia group.

I've been looking into this and the task ain't easy - the fork significantly diverged from the master, so rebasing it on top of a current master is challenging.

This is increasingly becoming a problem as npm audit is reporting more issues from the dependencies.

bearND added a subscriber: bearND.Dec 4 2018, 3:43 PM
Pchelolo raised the priority of this task from Low to Normal.

Given that lack of maintenance, the limiter has started causing real issues (see T212631), I'm raising the priority to 'Normal'

A bit more background:
The limitation library is a rate limiter based on DHT implementation called KAD. Historically, it was using Gabriels fork which was then transferred into the wikimedia org, but the fork has very significantly diverged from the upstream. Given that upstream API has changed, even the repos the fork is based on were deprecated, we should abandon the fork completely.

Ideally, we need to figure out the reasoning behind the 10 additional commits in the fork and attempt to use the upstream in the limitation library.
Additionally, we need to first reevaluate existing third-party libs - in the years that have passed since limitation was created the environment might have changed.

Update: Now npm audit shows 2 vulnerabilities from limitation.

If this takes longer I suggest just updating its dependencies.

Can't any necessary rate limiting be handled by Varnish/Apache Traffic Server, at least in the WMF production context?

Can't any necessary rate limiting be handled by Varnish/Apache Traffic Server, at least in the WMF production context?

We (as in the WMF) are trying to minimise the reliance on Varnish for anything that is not related to caching (routing, rate-limiting, etc), so this would go against that effort.

Update: Now npm audit shows 2 vulnerabilities from limitation.

Apologies for the hold-up. The dependencies have been updated, so now if you rm -rf node_modules && npm install, the vulnerabilities should no longer show. If you still use the deploy repo for your service (i.e. you are not on k8s), you should use --force when building the repo next time in order to pull in the updated dependencies.