Fix #290: Add moderators specific rate limiting settings

[DavidKarlas]

This change removes transfer size rate limiting for moderator role users aka. DWG members.

I tried to make change as small as possible and not refactor much code.

User roles were fetch before to check if user can see redactions or not...

[tomhughes]

I envisaged an ability to set a separate, higher, limit for moderators not to remove it all together.

Also this will need changing on the rails side as well to be consistent.

[DavidKarlas]

Ok, I will update PR, should we go with fixed value like 5x or 10x, or should I introduce --moderatormultiplier?
Looks like Ruby side doesn't have rate limiting at all, do you want me to add it?

[tomhughes]

I would suggest just having a second set of limit and burst values for moderators.

[DavidKarlas]

Pushed changes, added --moderator-maxdebt and --moderator-ratelimit. Open to suggestions for better names.

[DavidKarlas]

While working on this, I realized we are walking near limit which is 2GB(New default for moderators is 1GB, which is 4x of previous default of 250MB). How do we avoid problems? I see three options:

• Switch to long(~doubles size of memcached memory)
• throw exception if option value is bigger than 1.5GB
• Change to uint and throw exception if option value is bigger than 3.5GB

[mmd-osm]

Switch to long(~doubles size of memcached memory)

I see another issue here. Since we're storing values in binary format in memcached, any change to the length would either require to purge the cache, or we need the implementation to deal with the previous 32 bit length, and a new 64 bit length.
Also, I'm not exactly clear what Rails is doing with rate limiting.

Then the rate limiting code is a bit dated, and test coverage isn't exactly perfect. Maybe we should think about how we could add some test for the new code. I'm ok with doing that later, if there's an urgent need to move this code to production.

[tomhughes]

I think I was wrong about rails actually... I don't think we actually implement the rate limiting there.

[DavidKarlas]

I will attempt to mock now() to unit test rate limitting.

[DavidKarlas]

I wonder if I should rename:

global_settings::get_bytes_per_sec(moderator);
into
global_settings::get_ratelimiter_ratelimit(moderator);

and

global_settings::get_max_bytes(moderator);
into
global_settings::get_ratelimiter_maxdebt(moderator);
?

[mmd-osm]

Yes, I think that's a good idea. I was also a bit unhappy with the naming, because it's difficult to see that both are related to rate limiting.

[Zaczero]

BTW, do you accept donations, @DavidKarlas? I really appreciate it when someone goes out of their way to help others.

[DavidKarlas]

BTW, do you accept donations, @DavidKarlas? I really appreciate it when someone goes out of their way to help others.

Go here 😉

I pushed now renamed global settings and actually do parsing of options...
I tested this, checked via debugger that options are correctly applied, observed via JOSM that limits are never reached for moderator user... I'm pretty confident this change is good...

Now question... If we want to add test for this, I need guidance on how to mock time(NULL)
As far as I can see there are 2 ways, would like to hear more...

1. I introduce new class for getting time that similar to the way null_rate_limiter vs. memcached_rate_limiter, this means that in main.cpp this would be created and passed to process_request(req, limiter, generator, route, *factory, update_factory.get(), oauth_store.get())
2. Add time parameter to rate_limiter.check and rate_limiter.update that is epoch seconds and only unit tests rate_limiter and not rest of process_request logic like passing in correct bytes_written value...
3. Third option?

If you want test let me know how to mock, or if you think this can be merged without test please review.
Thank you!

[Zaczero]

Go here 😉

2nd time in a week? Give me a break!

[DavidKarlas]

I wonder what would be implications of using req.get_current_time() which is set here:

openstreetmap-cgimap/src/main.cpp

Lines 206 to 207 in 5b5f18b

	std::chrono::system_clock::time_point now(std::chrono::system_clock::now());
	req.set_current_time(now);

This would allow for easy mocking/calling of process_request similar to

openstreetmap-cgimap/test/test_core.cpp

Line 484 in 5b5f18b

process_request(req, limiter, generator, route, factory, nullptr, store);

[mmd-osm]

If you want test let me know how to mock, or if you think this can be merged without test please review.

I think I would need some time for a good idea how to test this. As mentioned earlier, I would be ok to do this later as a separate PR in this case.