search

nscuro / traefik-plugin-geoblock

traefik plugin to whitelist requests based on geolocation
Apache License 2.0
41 stars 5 forks source link

Add whitelisting CIDR Blocks #44

Closed gauth-fr closed 2 years ago

gauth-fr commented 2 years ago

I added a small feature to whitelist CIDRs block, even if they are supposed to be rejected (non-allowed country) The provided example is the google cidr.

gauth-fr commented 2 years ago

and that should resolve https://github.com/nscuro/traefik-plugin-geoblock/issues/22 ?

gauth-fr commented 2 years ago

Sure I agree :) what should I do? (I'm quite new to git and go^^) Change my code and commit again?

nscuro commented 2 years ago

Oh sorry, based on your code I was assuming you're experienced! :)

Yeah, it boils down to changing your code locally according to my comments, committing and pushing your changes. The PR will be updated automatically, and we can mark my comments as "resolved". If you run into issues, I'm happy to assist. Just tell me what tools you use to work with Git, what platform (Windows, Linux, macOS) you're on, and I can guide you.

But you may notice that two of my comments have suggested changes in them. You can apply my suggestions using the "commit suggestion" button:

Screenshot 2022-11-04 at 21 30 42

It will create a commit for you.

gauth-fr commented 2 years ago

Nah I learned go like 4h ago thanks to Google and stackoverflow^^ I'll give a try tomorrow, it starts to be late to geek ^^

gauth-fr commented 2 years ago

Done! Then i was wondering if, in case of CIDR parsing error i should really return an error or just print an error message. At least it wouldn't block everything no?

nscuro commented 2 years ago

in case of CIDR parsing error i should really return an error or just print an error message.

I think that when there is a configuration error, it should be super obvious that something is not right. I doubt many users follow their logs, and even if they do, a warning may be easy to miss.

gauth-fr commented 2 years ago

Yeah! My first approved PR^^