Add rate-limiter [#301](https://github.com/snoyberg/keter/issues/301)

[Oleksandr-Zhabenko]

The following pull request adds keter rate limiter using specially created package keter-rate-limiting-plugin. It adds WAI middleware rate limiter with 5 possible algorithms that supports complex configurations. It addresses issue #301

[jappeace]

please find another way of encoding middlewareCache.
I don't think we should introduce global variables anywhere.

[jappeace]

can't you put this in the plugin? I don't want to introduce global variables. Or find another way to do this cache.

[Oleksandr-Zhabenko]

There is a possibility to do the following:

Migration checklist
Types:

• Change middleware fields in the stanza/config types to [MiddlewareSpec].

• Update FromJSON instances to parse via parseMiddlewareSpecs.
  Wiring:

• Build a MiddlewareRegistry in Main (registerCoreBuiltins, registerRateLimiter, etc.).

• Pass the registry to Proxy.makeSettings.
  Plugins:

• Move rate-limiter parsing and construction into its own module exporting registerRateLimiter.

{-# LANGUAGE OverloadedStrings #-}
module Keter.RateLimiter.WAI
  ( registerRateLimiter
  -- other exports there
  ) where

import Data.Aeson (fromJSON, Result(..), Value)
import Data.Text (Text)
import Network.Wai (Middleware)
-- other imports there (actually in the keter-rate-limiting-plugin package)
import Keter.Config.Middleware (MiddlewareRegistry, register)

registerRateLimiter :: MiddlewareRegistry -> MiddlewareRegistry
registerRateLimiter =
  register "rate-limiter" build
  where
    build :: Value -> IO Middleware
    build v = case fromJSON v of
      Success cfg -> buildRateLimiter cfg
      Error e     -> fail ("rate-limiter: invalid config: " <> e)

• Any future middleware adds only a new registerX call in Main; no changes in Keter core.

import Keter.Config.Middleware (MiddlewareRegistry, emptyRegistry, registerCoreBuiltins)
import Keter.RateLimiter.WAI (registerRateLimiter)
-- later plugins can register similarly:
-- import Keter.SomePlugin.WAI.Plugin (registerSomePlugin)

startListening :: HostMan.HostManager -> KeterM KeterConfig ()
startListening hostman = do
    KeterConfig{..} <- ask
    -- Build the middleware registry once
    let registry :: MiddlewareRegistry
        registry = registerRateLimiter
                 $ registerCoreBuiltins
                 $ emptyRegistry
                 -- add more plugin registrations here
    settings <- Proxy.makeSettings hostman registry
    withMappedConfig (const settings) $ withRunInIO $ \rio ->
        liftIO $ runAndBlock kconfigListeners $ \ls ->
            rio $ Proxy.reverseProxy ls

This gives us “more a plugin”: Keter core no longer pattern matches on particular middlewares, and the rate limiter lives entirely as a plugin with a one-line registration.

Is this desirable?

[jappeace]

I don't think we need that middleware cache at all

[jappeace]

@Oleksandr-Zhabenko let me know when I can check again

[Oleksandr-Zhabenko]

@Oleksandr-Zhabenko let me know when I can check again

@jappeace I have made requested changes. Please, check again.

[jezen]

I don't feel great about this change, unfortunately.

This is quite a large change, and there is clearly ChatGPT output everywhere which suggests to me that not enough care was taken in ensuring the changes here are actually the desired changes.

There's also no evidence that this solution actually works. In order to verify that, someone would have to clone the repository with this change, run it manually, and try to observe the expected rate limiting behaviour.

I would expect to see, for example, a NixOS test which runs a minimal example application within keter which is configured with a rate limiter, and is then attacked with siege, or perhaps just a load of HTTP requests sent with cURL in a loop.

[Oleksandr-Zhabenko]

This is quite a large change, and there is clearly ChatGPT output everywhere which suggests to me that not enough care was taken in ensuring the changes here are actually the desired changes.

There's also no evidence that this solution actually works. In order to verify that, someone would have to clone the repository with this change, run it manually, and try to observe the expected rate limiting behaviour.

I would expect to see, for example, a NixOS test which runs a minimal example application within keter which is configured with a rate limiter, and is then attacked with siege, or perhaps just a load of HTTP requests sent with cURL in a loop.

Actually, I found the needed requested changes by Jappie quite large and complicated and I used ChatGPT to provide them. Firstly, I needed to convince ChatGPT to make them (it suggested not to make such changes), then I provided the additional data for the AI to provide the changes safely. Afterwards, I have carefully compared the changes with the original versions using meld, and made mostly additions of the deleted original comments (but left 'Updated:' informatonal messages for easier review). Afterwards, I changed tests according to the new APT. There was a need to add Show instance for ProxyActionRaw, using another AI model. Then I just accurately tested build and test and then after reran because of Mac0S random failure. Afterwards this provided the actual variant.

Actually, there are two nix tests of the rate limiter functionality in the keter-rate-limiting-plugin package itself. They use legacy keter (without integration with the new package within). I will provide another one here, actually using integration layer, should I?

[jezen]

Actually, there are two nix tests of the rate limiter functionality in the keter-rate-litiming-plugin package itself. They use legacy keter (without integration with the new package within). I will provide another one here, actually using integration layer, should I?

If I run keter, but this functionality isn't tested against keter, then I can't really have any confidence that this functionality works in keter.

[jappeace]

does this work or not? @Oleksandr-Zhabenko
I'm fine with merging as is.

also, the fact oleksander has to open a PR to add a plugin is just a major design flaw.

#319

[Oleksandr-Zhabenko]

Sorry, I have injured my eye, so several days I need to cure it. I hope to continue my work afterwards.

[Oleksandr-Zhabenko]

does this work or not? @Oleksandr-Zhabenko I'm fine with merging as is.

also, the fact oleksander has to open a PR to add a plugin is just a major design flaw.

#319

Yes, @jappeace, it works.

[ulidtko]

Seems counterproductive to comment on the actual code parts of the change... So let me review at least the docs.

Please see a few comments inline. All are easy to address.

On the same note, I'm not sure we should make literally 1/3rd of keter's README about the ratelimiting plugin. The keter readme is pretty scattered and hard to navigate already, IMO. @Oleksandr-Zhabenko why can't the README of keter-rate-limiting-plugin specify all the exhaustive tiniest detail? Those config fields specifications, the comparative analysis of algorithms... All that seems unnecessarily detailed and verbose -- for the readme of keter.

Suppose a user of keter is not using the middleware; but now they still have to see, and scroll through, and pay tokens for 1/3rd of keter readme simply becomes irrelevant to them. That way of structuring documentation is no good.

[ulidtko]

Why is this being added under incoming, and not under etc ?

Would make more sense as etc/middlewares.yaml. Better yet: in the ratelimiter plugin's README.

[Oleksandr-Zhabenko]

In the incoming there are other specific examples, therefore, I have decided earlier to provide example configuration there.

[jappeace]

does the main branch fail if you run this test against it?

[Oleksandr-Zhabenko]

does the main branch fail if you run this test against it?

No, it succeeds. It works.

[Oleksandr-Zhabenko]

@jappeace @jezen @ulidtko I have updated the keter-rate-limiting-plugin version to 0.2.0.2, fixed IO issue with integration tests .nix files.

Now it is also available on Stackage (nightly builds).

Besides I have created a separate dedicated repository with 3 zipped textual output files that contain the output of my .nix integration tests while being run (I truncated the OS introductory loading output and preserved the actual tests output). They are evidence of the passed tests (2 for the keter-rate-limiting-plugin itself and 1 for the keter integration).

Could you review the pull request here and merge it, please?

[jappeace]

I guess strictly you're doing as I told, but I wouldn't have mind leaving this in 😅

[Oleksandr-Zhabenko]

I have removed it. There is no such in my repository version.

[jappeace]

thanks 🙏🏽

[Oleksandr-Zhabenko]

🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉