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

Add ConntrackDeleteFilters #989

Merged
merged 1 commit into from
Aug 6, 2024

Conversation

aroradaman
Copy link
Contributor

ConntrackDeleteFilters enables users to delete flow entries that match any of the specified filters. This allows users to delete multiple flow entries with a single dump table call.

req2.AddRawData(dataRaw[4:])
req2.Execute(unix.NETLINK_NETFILTER, 0)
matched++
break
Copy link
Contributor

Choose a reason for hiding this comment

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

this may use one comment to indicate the flow was already deleted so no need to process more filters and we can continue in the next flow

ConntrackDeleteFilters enables users to delete flow entries
that match any of the specified filters. This allows users
to delete multiple flow entries with a single dump table call.

Signed-off-by: Daman Arora <aroradaman@gmail.com>
@fasaxc
Copy link
Contributor

fasaxc commented Jul 9, 2024

I was just looking for a function like this; hope it makes it in!

@aroradaman
Copy link
Contributor Author

cc. @aboch

@@ -133,9 +139,9 @@ func (h *Handle) ConntrackUpdate(table ConntrackTableType, family InetFamily, fl
return err
}

// ConntrackDeleteFilter deletes entries on the specified table on the base of the filter using the netlink handle passed
Copy link
Contributor

Choose a reason for hiding this comment

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

question for the maintainers, but they may prefer to leave the singular too for backwards compatibility purposes

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree, and it is taken care of above:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

This is indeed a breaking change; the singular one above only took care of the function, but looks like it missed adding a Handle.ConntrackDeleteFilter (singular) method on Handle; see

73.74 # github.com/docker/docker/libnetwork/iptables
73.74 libnetwork/iptables/conntrack.go:78:28: nlh.ConntrackDeleteFilter undefined (type *netlink.Handle has no field or method ConntrackDeleteFilter)
73.74 libnetwork/iptables/conntrack.go:84:28: nlh.ConntrackDeleteFilter undefined (type *netlink.Handle has no field or method ConntrackDeleteFilter)
73.74 libnetwork/iptables/conntrack.go:105:13: nlh.ConntrackDeleteFilter undefined (type *netlink.Handle has no field or method ConntrackDeleteFilter)

@@ -311,7 +311,7 @@ func TestConntrackTableDelete(t *testing.T) {

// Flush entries of groupB
var deleted uint
if deleted, err = h.ConntrackDeleteFilter(ConntrackTable, unix.AF_INET, filter); err != nil {
if deleted, err = h.ConntrackDeleteFilters(ConntrackTable, unix.AF_INET, filter); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

you may better add a new test or expand this to delete multiple flows with two different filters to exercise the new functionality

Copy link
Contributor Author

@aroradaman aroradaman Jul 16, 2024

Choose a reason for hiding this comment

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

I'm also considering passing the errors back to the caller. Currently, Execute() can return an error, but it is not being captured.
Also returning the error will be difficult in case of multiple filters, we would have to decide if we want to exit immediately in case of error, or try out all the filters and then return the error at the end.

@aboch
Copy link
Collaborator

aboch commented Aug 6, 2024

LGTM

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

Successfully merging this pull request may close these issues.

5 participants