search

coredns / rrl

Response Rate Limiting Plugin for CoreDNS
Apache License 2.0
23 stars 21 forks source link

Is Evict function testing for the wrong type? #37

Closed markdingo closed 1 year ago

markdingo commented 1 year ago

Should this:

https://github.com/coredns/rrl/blob/1ebc54edd477cb399328e57470c0920f47205cc6/plugins/rrl/rrl.go#L100

be

ra, ok := (*el).(*ResponseAccount)

That is, the contents of el are a pointer to a ResponseAccount?

I ran a few tests with some debugging output and I found that the !ok path was always being taken and thus the actual timeout evaluation never occurs. This makes evictions effectively always delete the first account in the shard.

Importantly, it also means that eviction always succeeds even if all accounts are still within the window size. A side effect of this is that unlock issue #36 is never triggered because eviction always succeeds. Point being, if the aforementioned bug is fixed, it'll most likely start triggering #36.

As an aside, I'm a bit surprised EvictFn uses *interface{} as this is very uncommon and rarely needed. Is there any reason why this isn't just the usual interface{}?

markdingo commented 1 year ago

I just noticed:

https://github.com/coredns/rrl/blob/1ebc54edd477cb399328e57470c0920f47205cc6/plugins/rrl/rrl.go#L169

which does deref the contents of el as a pointer to ResponseAccount. Which seems right to me as that code has to work otherwise the whole rrl would never trigger any rate limiting at all!

chrisohaver commented 1 year ago

Thanks for reviewing the code! It hasn't had a large number of eyes on it, so I appreciate the grooming.

I think in this case the root problem is that shard items are map[string]*interface{} - the reference to an interface being superfluous, since an interface is already a reference in Go. They should instead be refactored to map[string]interface{}, which would be less fiddly, and less prone to this type of error.

I doubt I had a valid reason to make the shard items *interface{} - probably just ignorance/confusion on my part at the time.

chrisohaver commented 1 year ago

Disregard that, I realize now that the two things are unrelated. Yes, *interface{} should probably be cleaned/refactored to interface{}... but that's not related to the *ResponseAccount that the interface references.

markdingo commented 1 year ago

Yeah. Sorry for the confusion. The issue is the bug. I just mentioned that *interface{} is a bit unusual but not related to the bug per se. I probably should have left that for another thread.

markdingo commented 1 year ago

Note that when you fix this it will potentially trigger the bug in #36 which this bug has hidden.