Handle: add ConntrackDeleteFilter alias for backward compat

[thaJeztah]

• relates to Add ConntrackDeleteFilters #989 (comment)
• relates to vendor: github.com/vishvananda/netlink v1.3.0 moby/moby#48368 (comment)

Commit c96b03b changed the signature of this method to accept a list of filters and renamed it to ConntrackDeleteFilters (plural).

This patch adds back ConntrackDeleteFilter as an alias, and marks it as deprecated in favor of the new version.

[thaJeztah]

@aboch @aojea @aroradaman PTAL

[aroradaman]

LGTM

[aojea]

heh #989 (comment)

/lgtm

[antoninbas]

Thanks for taking care of this. I hope @aboch can release v1.2.2 promptly with this change so that people can upgrade to a new release without having to deal with breaking changes.

[antoninbas]

why is this being marked as deprecated, when the top-level ConntrackDeleteFilter function is not?

[thaJeztah]

My thinking here was that the new one effectively replaces the existing one. So deprecation as indication for users to move over.

I'm fine removing though (honestly I think the change in signature probably would've been fine without a rename, but I guess it could break someone trying to match against an interface)

[antoninbas]

My point was that if this method is marked as deprecated, then ConntrackDeleteFilter should also be marked as deprecated, or it's inconsistent

netlink/conntrack_linux.go

Line 72 in 65a253d

func ConntrackDeleteFilter(table ConntrackTableType, family InetFamily, filter CustomConntrackFilter) (uint, error) {

[thaJeztah]

Ah! Sorry, misinterpreted your comment 😅

Yes, that makes sense; I pushed, and deprecated that one as well

While working on that, I also found that there c96b03b did not add stubs for non-supported platforms, so I added those as well.

[thaJeztah]

Thanks for taking care of this. I hope @aboch can release v1.2.2 promptly with this change so that people can upgrade to a new release without having to deal with breaking changes.

We haven't found time yet to bisect/dig into, but I should mention that we also ran into other issues with this update in Moby (docker engine); moby/moby#48368 (comment)

[antoninbas]

Thanks for taking care of this. I hope @aboch can release v1.2.2 promptly with this change so that people can upgrade to a new release without having to deal with breaking changes.

We haven't found time yet to bisect/dig into, but I should mention that we also ran into other issues with this update in Moby (docker engine); moby/moby#48368 (comment)

This is likely because of this change: acdc658
It was made a year ago but I guess there hasn't been an actual release of the netlink library for a while. You will probably need to update the moby code to check if route.Dst is "all zeros" instead of nil.

More generally, there has been quite a lot of changes to the API since the 1.2.0 release. New functions were added but there were also some "breaking" changes, most of them more subtle than just the removal of ConntrackDeleteFilter. The behavior of some API functions was changed (which is the issue you are running into), the type of some fields was changed (#983), etc. It seems that at the very least a new minor release (1.3.0) would have been more appropriate than a patch release (1.2.1).

[thaJeztah]

Thanks @antoninbas ! That sounds indeed like it's related; thanks for that pointer, I think we should be able to work with that 🫶

More generally, there has been quite a lot of changes to the API since the 1.2.0 release. New functions were added but there were also some "breaking" changes

Yes, we stuck to a tagged version (to not force others to update), but the beta was probably tagged a bit too soon (either that, or had been around for too long, and gathered too many changes since). 1.3.0 definitely would've given more flexibility (keeping the option open to tag 1.2.x versions if needed).

[aboch]

Thanks @thaJeztah for adding back the Handle version of the original method.
Why do we need to deprecate the methods which take an individual filter? Do you see a problem in leaving them available?

[thaJeztah]

Mostly to keep the option open to reduce the public API in future, and to reduce maintenance (admitted, for these that's fairly limited). Deprecated doesn't have to mean "remove" immediately. Happy to remove the deprecation though if that's no concern.

From a public API perspective, while looking at this, even started wondering if this change was strictly needed; the existing function accepts a custom filter, which must match an interface; a utility to construct a "compound" filter probably would've given the same (and more), e.g. MultiFilter(filter1, filter2) or even AndFilter(filter1, filter2, ...) 🙈