enable rate limit for month and year

[rpahli]

What type of PR is this?

adds the option to set rate limit unit to Month and Year which was introduced with envoy 1.25.

[stefansedich]

Thanks @rpahli just came here to open the same PR myself, good thing I looked first 😆

@collin-lee anything preventing this one from being merged? kicking off a project using the ratelimit service and monthly limits sure would be handy.

[collin-lee]

LGTM though do you expect that your service/redis would not need to be restarted for re-deployments monthly/yearly?
thoughts @arkodg?

[stefansedich]

LGTM though do you expect that your service/redis would not need to be restarted for re-deployments monthly/yearly? thoughts @arkodg?

For our use-case I had just planned to enable persistence in redis, my ultimate goal here was to add support for an additional weekly bucket, however looking at the enums over in https://github.com/tyxia/envoy/blob/b5bc27399d723806bab993895e64f9295c863016/api/envoy/service/ratelimit/v3/rls.proto#L101 it will be a breaking change to sneak WEEK in there now...

[arkodg]

not sure I understand @collin-lee, this functionality didn't exist before, so users will need to update to a newer version with this change to leverage setting month and year limits in the ratelimit service.
did a quick scan of the code and UnitToDivider is used everywhere in the code, so this should hopefully be the only change required

[collin-lee]

@arkodg - my question was more about the use case and @stefansedich mentioned he would persist redis values. @stefansedich - so could you do a multiple of days for weeks/months instead? (i.e 7 days for a week, 30 days for a month)?

[stefansedich]

@arkodg - my question was more about the use case and @stefansedich mentioned he would persist redis values. @stefansedich - so could you do a multiple of days for weeks/months instead? (i.e 7 days for a week, 30 days for a month)?

That would be fine too, it just means something like #552 going in which in the end would be far more flexible and a nice addition.

I have gone ahead and added WEEK for now over in envoyproxy/envoy#37494 as I think it makes sense that it exists given we have month and year already, will open a PR here to add support once it gets merged (if it does).

[stefansedich]

@collin-lee anything remaining to get this one in? I am happy to do some additional testing if that would help.

[collin-lee]

@stefansedich - should the code return 60 * 60 * 24 * 356 use 365 instead?

[rpahli]

@stefansedich - should the code return 60 * 60 * 24 * 356 use 365 instead?

the base unit is seconds so I think the code is correct

[stefansedich]

@stefansedich - should the code return 60 * 60 * 24 * 356 use 365 instead?

@collin-lee looks right to me as the output is seconds, and replicates what was done over on the Envoy side.

[collin-lee]

should this be 365 instead?