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

[firewall tables] Fix code to capture NAT table #3335

Merged

Conversation

jcastill
Copy link
Member

The code used to check the file /proc/net/ip_tables_names and if it didn't exist or was empty, would load
two tables by default - mangle, and filter. The logic was missing the nat table, and so it was not captured in certain scenarios.
The change in this PR simplifies the code by not
checking /proc/net/ip_tables_names anymore, and
looking directly at the output of nf tables list.

Related: RHBZ#2228642


Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

  • Is the commit message split over multiple lines and hard-wrapped at 72 characters?
  • Is the subject and message clear and concise?
  • Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
  • Does the commit contain a Signed-off-by: First Lastname [email protected]?
  • Are any related Issues or existing PRs properly referenced via a Closes (Issue) or Resolved (PR) line?

@packit-as-a-service
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/sosreport-sos-3335
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@pmoravec
Copy link
Contributor

The change will resolvecases when /proc/net/ip_tables_names is missing. But can't be there counter-examples where nft list is smaller than content of the file? (I really dont know)

I.e. why not 1) try /proc/net/ip_tables_names, if it fails then 2) iterate over nft_list, 3) if that fails, failover to the default 2 tables..?

@jcastill
Copy link
Member Author

That was one of the things I considered originally, the other being adding 'nat' in default_ip_tables and get on with it.
From what I've seen so far, it looks like /proc/net/ip_tables_names is dependent on old iptables, and as we move out of that into nftables, relying on that file may not be the best way.
But I'm happy to just add the 'nat' table into default_ip_tables if that's the better approach.

@TurboTurtle
Copy link
Member

But I'm happy to just add the 'nat' table into default_ip_tables if that's the better approach.

I'm thinking that may be the best solution at the moment.

This PR adds the nat table to default_ip_tables
so when reading /proc/net/ip_tables_names fails,
it captures all the default tables.

Related: RHBZ#2228642

Signed-off-by: Jose Castillo <[email protected]>
@jcastill jcastill force-pushed the jcastillo-fix-capture-of-nat-iptable branch from 63f7ce4 to 2fa55da Compare August 16, 2023 14:37
Copy link
Contributor

@pmoravec pmoravec left a comment

Choose a reason for hiding this comment

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

ACK. That table sounds too generic to collect it every time. The worst is we will call a command against not existing table, sometimes.

Copy link
Member

@TurboTurtle TurboTurtle left a comment

Choose a reason for hiding this comment

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

Ack. Not sure why the CI was cancelled, restarting. Will merge before version cut once the CI passes (no reason it won't).

@TurboTurtle TurboTurtle merged commit 7d5a2d9 into sosreport:main Aug 16, 2023
34 checks passed
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.

3 participants