Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/net/bpf: add Setter interface #19912

Closed
mdlayher opened this issue Apr 10, 2017 · 9 comments
Closed

x/net/bpf: add Setter interface #19912

mdlayher opened this issue Apr 10, 2017 · 9 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Milestone
Unreleased

Comments

@mdlayher
Copy link
Member

In order to standardize BPF filter attachment in Go, I propose an interface which mimics the most popular signature used in the wild:

package bpf

// A Setter is a type which can attach a compiled BPF filter to itself.
type Setter interface {
    SetBPF(filter []bpf.RawInstruction) error
}

I believe this pattern was first used in x/net/ipv{4,6}:

https://godoc.org/golang.org/x/net/ipv4#PacketConn.SetBPF
https://godoc.org/golang.org/x/net/ipv6#PacketConn.SetBPF

I've used it in a couple of my own repositories as well:

https://godoc.org/github.com/mdlayher/raw#Conn.SetBPF
https://godoc.org/github.com/mdlayher/netlink#Conn.SetBPF

Finally, on at least two occasions, I have defined an unexported interface that contains a method using this proposed interface:

https://github.com/coreos/torus/blob/master/block/aoe/aoe.go#L106-L111
https://github.com/mdlayher/netlink/blob/master/conn.go#L230-L234

For simplicity and standardization, I'd like to add the proposed interface to x/net/bpf's API, in order to ensure consistency between types that can attach a BPF filter to themselves.

/cc @danderson, @mikioh, @breml
@mdlayher mdlayher added this to the Proposal milestone Apr 10, 2017
@rsc
Copy link
Contributor

rsc commented Apr 17, 2017

Given that this interface is already implemented by x/net/ipv4 and x/net/ipv6, it seems fine to add the interface definition too. Any thoughts, @danderson, @mikioh?

@mikioh
Copy link
Contributor

mikioh commented Apr 18, 2017

I'll leave it up to @danderson.

@mikioh
Copy link
Contributor

mikioh commented Jun 2, 2017

ping @danderson

@danderson
Copy link
Contributor

The interface definition seems fine, however: what benefit does it provide? I don't understand what benefit this definition provides, given that the package itself doesn't make use of it. My first instinct is that this is an attempt to define a java-style interface that other packages can implements... Except that this isn't needed in Go. So, I'm just confused :)

What am I missing?

@danderson
Copy link
Contributor

To be clear: if the intent is just to try and steer implementors to standardize on using that method signature to attach BPF programs, sure, I'm happy to go along with it. It just makes me feel a bit strange to have a package define an interface exclusively as a "serving suggestion" for other packages. If I'm alone in feeling weird about that, then definitely let's do it :)

@mdlayher
Copy link
Member Author

mdlayher commented Jun 6, 2017

Hmmm, not sure how I missed this notification, sorry!

To be clear: if the intent is just to try and steer implementors to standardize on using that method signature to attach BPF programs, sure, I'm happy to go along with it.

That's exactly it. Think of it like the interfaces defined in: https://golang.org/pkg/encoding/.

@gopherbot
Copy link

CL https://golang.org/cl/44972 mentions this issue.

@rsc
Copy link
Contributor

rsc commented Jun 12, 2017

@danderson, OK to accept this proposal?

@danderson
Copy link
Contributor

LGTM, ship it.

@bradfitz bradfitz changed the title proposal: x/net/bpf: add Setter interface x/net/bpf: add Setter interface Jun 12, 2017
@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 12, 2017
@bradfitz bradfitz modified the milestones: Unreleased, Proposal Jun 12, 2017
@golang golang locked and limited conversation to collaborators Jun 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@danderson @bradfitz @mikioh @rsc @mdlayher @gopherbot