RedisClient token bucket argument validation

[rparke]

Previously we didn't do any input validation on the RedisClient.get_remaining_bucket_tokens wrapper method. The lack of validation meant that bugs caused by notifications-admin inadvertently setting the replenish_per_sec argument to 0 were harder to track down. This adds validation on the replenish_per_sec, bucket_max and bucket_min parameters.

[quis]

I think we should allow 0. It’s potentially a useful way of shutting off a service’s API calls without revoking their keys.

Suggested change

	if replenish_per_sec <= 0:
	raise ValueError(f"replenish_per_sec: '{replenish_per_sec}' cannot be <= 0")
	if replenish_per_sec < 0:
	raise ValueError(f"replenish_per_sec: '{replenish_per_sec}' cannot be < 0")

If we do want to disallow 0 we’d also want to update the validation here: https://github.com/alphagov/notifications-admin/blob/b9f5615bdebd9f3e1fad0229f8a40a9b87f00a39/app/main/forms.py#L1364

[rparke]

hmm, the bug last week was caused by us setting 0 when we didn't expect to, but yeh it's a nice way of temporarily "revoking" api access.

Happy to go with <0

[quis]

To prevent that bug the suggestion was to limit last_replenished to be <= now here:

notifications-utils/notifications_utils/clients/redis/redis_client.py

Line 118 in 97be6ad

last_replenished = last_replenished + (replenishment / replenish_per_sec)

[rparke]

so something like:

...
...
last_replenished = last_replenished + (replenishment / replenish_per_sec) 
if last_replenished > now
   last_replenished = now
end
...
...

???

[quis]

Yeah, something like that. Would defo need some tests as well. For example call the script with a replenish of 0, call it with a replenish of 50, make sure it still works.

[rparke]

👍