Skip to content
This repository has been archived by the owner on May 14, 2020. It is now read-only.

Implement support for whitelists, default-deny/allow (rebase) #8

Merged
merged 5 commits into from
May 10, 2019

Conversation

bigs
Copy link
Contributor

@bigs bigs commented Jan 8, 2019

Rebasing #1 so that we can progress with it.

@ghost ghost assigned bigs Jan 8, 2019
@ghost ghost added the in progress label Jan 8, 2019
filter.go Outdated Show resolved Hide resolved
filter.go Show resolved Hide resolved
filter.go Outdated Show resolved Hide resolved
filter.go Outdated Show resolved Hide resolved
filter.go Outdated Show resolved Hide resolved
@bigs
Copy link
Contributor Author

bigs commented Jan 31, 2019

@vyzo can we take another pass on this?

@vyzo
Copy link

vyzo commented Jan 31, 2019

@bigs can you look at the comment about the potential bug?

@bigs
Copy link
Contributor Author

bigs commented Jan 31, 2019

@vyzo yeah, that was discussed in the original PR. i guess it's supposed to match the semantics of iptables

filter.go Outdated Show resolved Hide resolved
filter.go Outdated Show resolved Hide resolved
filter.go Outdated Show resolved Hide resolved
@prestonvanloon
Copy link

Is there any update here? We really need this for github.com/prysmaticlabs/prysm

filter.go Outdated Show resolved Hide resolved
filter.go Outdated Show resolved Hide resolved
@raulk
Copy link
Member

raulk commented Mar 28, 2019

@prestonvanloon thanks for raising the voice here. I've added this to my queue for tomorrow.

@ghost ghost self-requested a review April 2, 2019 01:33
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me on a high level, I like that it keeps backward compatibility. 👏

Naming needs a cleanup, there's six names (block, reject, deny, allow, accept, whitelist) where two would be appropriate. Whitelist in particular isn't a good name because it's a metaphor that's easy to misunderstand or not understand at all.

There's also a bit of diff clutter because you renamed f to fs in some cases - it'll be easier to review without that rename. I'm fine with that type of change but it shouldn't be included in PRs that already contain significant changes on their own.

filter.go Outdated Show resolved Hide resolved
filter.go Show resolved Hide resolved
filter.go Outdated Show resolved Hide resolved
filter.go Outdated Show resolved Hide resolved
filter.go Outdated Show resolved Hide resolved
filter.go Outdated Show resolved Hide resolved
@raulk
Copy link
Member

raulk commented Apr 2, 2019

I'll take care of adjusting this PR in accordance with review feedback.

@raulk
Copy link
Member

raulk commented Apr 2, 2019

@lgierth @vyzo @bigs I've pushed some changes to bring consistency and nicer API. Is this something we're more keen to get behind?

Todo:

  • taking account specificity when evaluating blocked addrs

@vyzo
Copy link

vyzo commented Apr 2, 2019

looks much better.

@raulk
Copy link
Member

raulk commented Apr 3, 2019

All done here. I've also reworked the commit history to group all contributions while retaining authorship.

@raulk raulk requested review from raulk and a user April 3, 2019 16:20
@raulk raulk requested a review from vyzo April 3, 2019 16:20
@bigs
Copy link
Contributor Author

bigs commented Apr 3, 2019

LGTM! thanks @raulk!

filter.go Outdated Show resolved Hide resolved
@prestonvanloon
Copy link

Can this be merged?

@raulk
Copy link
Member

raulk commented May 10, 2019

@prestonvanloon yes, let's do that 👍

@raulk raulk merged commit 0be077b into master May 10, 2019
@ghost ghost removed the in progress label May 10, 2019
@raulk raulk deleted the arrdem-master branch May 10, 2019 14:37
@raulk
Copy link
Member

raulk commented May 10, 2019

@prestonvanloon released in v0.0.2; update with gomod!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants