Hi @danwinship. Thanks for your PR.

I'm waiting for a cri-o member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

1. As currently written, this PR entirely drops iptables support

(meant to imply: I could rewrite it to support both iptables and nftables (though that would obviously be more complicated). It just didn't occur to me until I was filing the PR that we might want that.)

/ok-to-test

Codecov Report

Attention: Patch coverage is 78.47896% with 133 lines in your changes missing coverage. Please review.

Project coverage is 47.24%. Comparing base (d8220db) to head (610ede1).

@@            Coverage Diff             @@
##             main    #8684      +/-   ##
==========================================
+ Coverage   46.96%   47.24%   +0.28%     
==========================================
  Files         154      155       +1     
  Lines       21978    22232     +254     
==========================================
+ Hits        10322    10504     +182     
- Misses      10594    10657      +63     
- Partials     1062     1071       +9     

/test all

@danwinship, we are making an assumption here that all users of CRI-O are going to be using nftables, isn't it?

@kwilczynski it assumes that all users of CRI-O 1.32 will be on distros with new-enough nftables (see the "Special notes for your reviewer" section of the initial comment). It doesn't assume they'll be using nftables for kube-proxy or firewalling or anything else. The hostport rules are independent.

(And I can rewrite it to support either iptables or nftables if necessary. It will just be more complicated...)

So there's only one test of HostPorts in the e2e suite ([It] [sig-network] HostPort validates that there is no conflict between pods with same hostPort but different hostIP and protocol [LinuxOnly] [Conformance]), and it's passing in both ci/prow/e2e-aws-ovn and ci/prow/ci-e2e (which, if I understand correctly, runs against a vanilla upstream k8s environment, not OCP, right?)

if I understand correctly, runs against a vanilla upstream k8s environment, not OCP, right

correct!

Can we optimistically use nftables and if that fails fallback to iptables? I'm fine dropping iptables in our docs and recommending nftables but I Think there should be some transition time so we don't pull the rug out from underneath users who use older OSs (I know of some using debian, not sure which version)

@haircommander done, though probably this needs some hand-testing to make sure it really is supporting both modes (and the linter doesn't like what I did with the unit tests...)

Does it need to do both-iptables-and-nftables-cleanup on Remove? (eg, does it need to support the possibility of upgrading and restarting cri-o without rebooting).

Does it need to do both-iptables-and-nftables-cleanup on Remove? (eg, does it need to support the possibility of upgrading and restarting cri-o without rebooting).

Unfortunately, yes

note: #8757 contains the first half of this (unit test updates) which can be reviewed separately

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: danwinship
Once this PR has been reviewed and has the lgtm label, please assign giuseppe, kwilczynski, littlejawa for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

@danwinship: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Full PR test history. Your PR dashboard.

@danwinship do we need this for 1.32 or is it possible to defer to 1.33?

It can be deferred to 1.33

Moved it to the next milestone, thank you!