Allow Exempting IP Ranges #429
Conversation
Use netip.Addr instead of storing addresses as strings. This requires conversions at the database level and in tests, but is more memory efficient otherwise, and facilitates the following. Parse rate limit exemptions as netip.Prefix. This allows storing IP ranges in the exemption list. Regular IP addresses (entered explicitly or resolved from hostnames) are IPV4/32, denoting a range of one address.
|
This approach is perfectly fine. I'll leave some comments. |
cmd/serve.go
Outdated
| if err == nil { | ||
| prefixes = append(prefixes, prefix.Masked()) // masked and canonical for easy of debugging, shouldn't matter | ||
| return prefixes, nil // success | ||
| } |
There was a problem hiding this comment.
What's a masked/canonical address?
There was a problem hiding this comment.
For example, if someone enters 10.1.2.3/8, the canonical address is 10.0.0.0/8. It represents every address from 10.0.0.1 to 10.255.255.255. If the address is ever logged in the future, this would help the output be more consistent.
Should I explain this more in the comment?
karmanyaahm
force-pushed
the
ip-range-exempt
branch
from
2c85707
to
de2ca33
Compare
|
Looks great. What's missing (since it's still in draft)? |
|
I'm working on fixing old tests and adding tests for new functionality. Should be done later today |
Codecov ReportBase: 65.97% // Head: 66.10% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #429 +/- ##
==========================================
+ Coverage 65.97% 66.10% +0.12%
==========================================
Files 35 35
Lines 3689 3720 +31
==========================================
+ Hits 2434 2459 +25
- Misses 906 908 +2
- Partials 349 353 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| func TestIP_Host_Parsing(t *testing.T) { | ||
| cases := map[string]string{ | ||
| "1.1.1.1": "1.1.1.1/32", | ||
| "fd00::1234": "fd00::1234/128", | ||
| "192.168.0.3/24": "192.168.0.0/24", | ||
| "10.1.2.3/8": "10.0.0.0/8", | ||
| "201:be93::4a6/21": "201:b800::/21", | ||
| } | ||
| for q, expectedAnswer := range cases { | ||
| ips, err := parseIPHostPrefix(q) | ||
| require.Nil(t, err) | ||
| assert.Equal(t, 1, len(ips)) | ||
| assert.Equal(t, expectedAnswer, ips[0].String()) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Should I just omit testing hostnames (because I'd have to mock the DNS resolver and all that)? The code is not all that complicated.
|
server/server.go is already very thoroughly tested. I don't think there's anything meaningful I can add for this specific change. |
Refactor visitor IPs and allow exempting IP Ranges
Use netip.Addr instead of storing addresses as strings. This requires
conversions at the database level and in tests, but is more memory
efficient and efficient to compare; mainly it facilitates the following.
Parse rate limit exemptions as netip.Prefix. This allows storing IP
ranges in the exemption list. Regular IP addresses (entered explicitly
or resolved from hostnames) are IPV4/32, denoting a range of just that one
address.
draft still need to add tests and fix bugs.However, is the change from string IPs to netip.Addr and netip.Prefix acceptable?
Closes #423