Include nftables information in Agent supportbundle #7547

molegit9 · 2025-11-06T04:03:25Z

Fixes: #7546
Modification location: pkg/support/dump_others.go

I corrected it because the diagnostic file did not contain the information of 'NFTables'.
' 'pkg/support/dump_others.go' has been added with the function 'dumpNFTables' to allow information to be included in the diagnosis

molegit9 · 2025-11-06T05:42:31Z

/test-all
/skip-windows-e2e

antoninbas · 2025-11-06T19:32:09Z

pkg/support/dump_others.go

 }

+func (d *agentDumper) dumpNFTables(basedir string) error {
+	c, err := nftables.New(d.v4Enabled, d.v6Enabled)


If nftables is not supported, we should not fail the suppot bundle collection operation
cc @hongliangl

Yes, I think we should just log the error and return nil.

antoninbas · 2025-11-06T19:32:24Z

pkg/flowaggregator/test_2551044057.config

what are all these files for?

My apologies. Those were temporary files generated by my local tests that were added by mistake.
I have removed them from the commit in the latest push. Thanks for catching that!

hongliangl

I would save the results of table ip antrea and table ip6 antrea to one file nftables, just like file iptables.

Additionally, could you add the unit tests for this?

hongliangl · 2025-11-07T03:49:50Z

pkg/support/dump_others.go

+func (d *agentDumper) dumpNFTables(basedir string) error {
+	c, err := nftables.New(d.v4Enabled, d.v6Enabled)
+	if err != nil {
+		klog.Warningf("Skipping nftables dump, client init failed: %v", err)


We don't use klog.Warningf. Just use klog.ErrorS or klog.InfoS.

hongliangl · 2025-11-07T04:50:21Z

pkg/support/dump_others.go

+	}
+
+	if d.v4Enabled && c.IPv4 != nil {
+		output, err := c.IPv4.DumpTable(context.TODO(), "antrea")


I didn't find the method DumpTable in the interface from nftables lib https://github.com/hongliangl/knftables/blob/e4307300abb50f2d9e7a492c474fe046d777f87f/nftables.go#L30.

Please use the appropriate methods to dump the table.

hongliangl · 2025-11-07T04:51:31Z

pkg/support/dump_others.go

+	if d.v4Enabled && c.IPv4 != nil {
+		output, err := c.IPv4.DumpTable(context.TODO(), "antrea")
+		if err != nil {
+			klog.Warningf("Failed to dump nftables table antrea-v4: %v", err)


We should return an error here since nftables.New succeeded.

hongliangl · 2025-11-07T04:51:44Z

pkg/support/dump_others.go

+			if err := writeFile(d.fs, filepath.Join(basedir, fileName), fileName, output); err != nil {
+				klog.Warningf("Failed to write %s: %v", fileName, err)


Ditto, return the error.

hongliangl · 2025-11-07T04:52:50Z

pkg/support/dump_others.go

+		return nil
+	}
+
+	if d.v4Enabled && c.IPv4 != nil {


Suggested change

if d.v4Enabled && c.IPv4 != nil {

if c.IPv4 != nil {

hongliangl · 2025-11-07T04:53:01Z

pkg/support/dump_others.go

+		}
+	}
+
+	if d.v6Enabled && c.IPv6 != nil {


Suggested change

if d.v6Enabled && c.IPv6 != nil {

if c.IPv6 != nil {

Thanks for the detailed feedback! I've pushed the updates incorporating all your suggestions

hongliangl · 2025-11-11T07:47:26Z

pkg/support/dump_others.go

+func (d *agentDumper) dumpNFTables(basedir string) error {
+	_, err := d.executor.LookPath("nft")
+	if err != nil {
+		klog.InfoS("Skipping nftables dump, 'nft' command not found", "err", err)


Suggested change

klog.InfoS("Skipping nftables dump, 'nft' command not found", "err", err)

klog.ErrorS(err, "'nft' command not found")

pkg/support/dump_others.go

pkg/support/dump_others_test.go

hongliangl

LGTM overall from me, only a nit in PR description:

issue : #7546 -> Fixes: #7546

For git commit and PR titles, how about removing the prefix Fix(#7546):?

Defer to @antoninbas @luolanzone to do the final approval.

hongliangl · 2025-11-12T06:16:45Z

Please resolve the failed checks :

Go/Golangci-lint
Go/Unit test
DCO

molegit9 · 2025-11-12T11:48:29Z

Please resolve the failed checks :

Go/Golangci-lint

Go/Unit test

DCO

Go/Golangci-lint I reordered the import statements using goimports -w -local antrea.io/antrea pkg/support/dump_others.go.

Go/Unit test I modified the code to not write data when there is no output (empty string), like so:

dump_others.go
line 82, 93
if len(output) > 0 {
data.Write(output)
data.WriteString("\n")
}

dump_others_test.go
line 174
Set 'fakeExecutor := &testingexec.FakeExec{}' to ensure it returns the proper fake results.

DCO I corrected the Signed-off-by line from just a name to the full email address.

antoninbas · 2025-11-12T22:51:16Z

pkg/support/dump_others.go

 }

+func (d *agentDumper) dumpNFTables(basedir string) error {
+	_, err := d.executor.LookPath("nft")


@hongliangl shouldn't we use the wrapper library here?

We should prioritize using the library. But unfortunately, the library doesn't provide a method to dump all contents of a table. The existing methods for dumping:

// List returns a list of the names of the objects of objectType ("chain", "set", // "map" or "counter") in the table. If there are no such objects, this will return an empty // list and no error. List(ctx context.Context, objectType string) ([]string, error) // ListRules returns a list of the rules in a chain, in order. If no chain name is // specified, then all rules within the table will be returned. Note that at the // present time, the Rule objects will have their `Comment` and `Handle` fields // filled in, but *not* the actual `Rule` field. So this can only be used to find // the handles of rules if they have unique comments to recognize them by, or if // you know the order of the rules within the chain. If the chain exists but // contains no rules, this will return an empty list and no error. ListRules(ctx context.Context, chain string) ([]*Rule, error) // ListElements returns a list of the elements in a set or map. (objectType should // be "set" or "map".) If the set/map exists but contains no elements, this will // return an empty list and no error. ListElements(ctx context.Context, objectType, name string) ([]*Element, error) // ListCounters returns a list of the counters. ListCounters(ctx context.Context) ([]*Counter, error)

We can use the above methods to dump a table, but it is a little complicated:

Use List to dump all chains, sets, maps

Use ListRules to dump all chains

Use ListElements to dump all sets and maps

antoninbas · 2025-11-13T19:02:26Z

pkg/support/dump_others.go

+	}
+
+	if data.Len() == 0 {
+		klog.InfoS("No Antrea nftables rules found to dump")


I would omit this log message

antoninbas · 2025-11-13T19:09:23Z

pkg/support/dump_others.go

+		klog.ErrorS(err, "'nft' command not found")
+		return nil
+	}
+


nft can be found (in the antrea-agent container image), but that doesn't mean it will run successfully: maybe nf_tables is not available.
We should check for compatibility once during initialization, and skip dumpNFTables if there is no support in the host kernel. @hongliangl what do you think i the best way to do that?

How about this:

Only initialize the wrapper library Client to check if nftables is available. This is because the initialization process does the following checks. See the link

antrea/pkg/agent/util/nftables/nftables.go

Line 67 in b36df94

func newNFTables(ipFamily knftables.Family) (knftables.Interface, error) {

// knftables.New validates: // - nft binary is available // - sufficient permissions // - kernel version compatibility // - "nft destroy" support

If an error is got when initializing, just log something that nftables is not available and skip dumping nftables.

Use the d.executor.Command("nft", "list", "table", "ip", "antrea").CombinedOutput() to dump the table.

The first step might be confusing, and we need to document it well to state that this step is to verify the availability of nftables.

Hi @antoninbas and @hongliangl,

Just a gentle ping regarding the updates based on your previous discussion.

I've refactored the PR according to the approach you mentioned:

Startup Check: Added nftclient.New in agent.go to verify kernel compatibility during initialization.

Dump Execution: Kept d.executor.Command("nft", ...) for the actual dump since the library doesn't support a full dump.

Log Cleanup: Removed the "No Antrea nftables rules found to dump" log message.

I believe this reflects all the points you raised.
When you have a moment, could you please take a look?

Thanks!

molegit9 · 2025-11-15T17:19:04Z

In agent.go, I made it check for nftables support once at agent startup using nftclient.New(), save it to nftablesSupported, and added it as an argument to the functions.

And I changed the existing LookPath check method to check via nftablesSupported.

There were no problems when I ran make golangci and make .linux-test-unit locally, but I don't know if other problems will occur as I lack knowledge.

Please review the code, and if you find it strange, I will re-upload a version with only the log removal that the reviewer requested.

hongliangl · 2025-11-24T11:06:26Z

cmd/antrea-agent/agent.go

+	var nftablesSupported bool
+	if _, err := nftclient.New(networkConfig.IPv4Enabled, networkConfig.IPv6Enabled); err != nil {
+		klog.InfoS("nftables is not supported on this Node, skipping nftables-related features", "err", err)
+		nftablesSupported = false
+	} else {
+		klog.InfoS("nftables is supported on this Node")
+		nftablesSupported = true
+	}
+


We can just move the logic L338-L346 to function dumpNFTables in dump_others.go, and we don't need the changes of introducing extra parameter nftablesSupported, simplifying the PR.

Thanks for the review! I reverted the complex changes and moved the check logic directly into 'dumpNFTable's as you suggested.

Now it uses 'nftclient.New' to check for support at the beginning.

I verified that both 'make golangci' and 'make .linux-test-unit' pass locally.

hongliangl · 2025-11-25T02:52:35Z

pkg/support/dump_others.go


+func (d *agentDumper) dumpNFTables(basedir string) error {
+	if _, err := nftclient.New(d.v4Enabled, d.v6Enabled); err != nil {
+		klog.V(4).InfoS("Skipping nftables dump because it is not supported on this Node", "err", err)


Use klog.ErrorS here.

Thank you for the review!
'klog.V(4).InfoS' -> 'ErrorS'
I modified it.

hongliangl

LGTM, but you need to resolve the DCO failure in github action.

hongliangl

LGTM, defer to @antoninbas and @luolanzone to do final approvals.

hongliangl · 2025-11-25T06:23:20Z

You have failed unit tests, please correct them first.

antoninbas · 2025-11-25T18:55:53Z

pkg/support/dump_others.go

 	"strings"

+	"k8s.io/klog/v2"
+	testingexec "k8s.io/utils/exec/testing"


There is never a reason to import this package in a non-test file.

antoninbas · 2025-11-25T18:58:06Z

pkg/support/dump_others.go

+	if _, isFake := d.executor.(*testingexec.FakeExec); !isFake {
+		if _, err := nftclient.New(d.v4Enabled, d.v6Enabled); err != nil {


This code is confusing and also doesn't match what was discussed in #7547 (comment)

Ideally we should check for nftables only once, not for every call to dumpNFTables. It was also mentioned that we should comment this code well to explain why we are calling nftclient.New, but not using the returned value.

As you advised, we removed the testingexec import. Instead, we separated the kernel inspection called inside dumpNFTables into a variable (var checkNFTablesSupport). And in the test code, we modified this variable to pass the test by mocking it.

antoninbas · 2025-11-26T22:09:22Z

pkg/support/dump_others.go

+var newNFTablesClient = func(v4Enabled, v6Enabled bool) (interface{}, error) {
+	return nftclient.New(v4Enabled, v6Enabled)
+}


could we do something like this:

var nftablesIPv4Supported = sync.OnceValue(func() bool { _, err := newNFTables(knftables.IPv4Family) return err != nil }) var nftablesIPv6Supported = sync.OnceValue(func() bool { _, err := newNFTables(knftables.IPv6Family) return err != nil })

I am using "sigs.k8s.io/knftables" directly here, instead of "antrea.io/antrea/pkg/agent/util/nftables" which is fine.

This way in dumpNFTables you can use nftablesIPv4Supported() / nftablesIPv6Supported(), and the sync.OnceValue wrapper ensures that we don't check the same thing multiple times over.

You can also easily "mock" them in unit tests.

Thanks for the review. I've updated the PR based on your feedback.

Signed-off-by: molegit9 <[email protected]>

molegit9 force-pushed the fix-nftables-supportbundle branch 2 times, most recently from 001ea65 to 6bb8141 Compare November 6, 2025 05:39

molegit9 force-pushed the fix-nftables-supportbundle branch from 6bb8141 to f6801cb Compare November 6, 2025 07:07

antoninbas reviewed Nov 6, 2025

View reviewed changes

antoninbas requested a review from hongliangl November 6, 2025 19:32

molegit9 force-pushed the fix-nftables-supportbundle branch from f6801cb to 95e0ce7 Compare November 7, 2025 03:47

hongliangl reviewed Nov 7, 2025

View reviewed changes

molegit9 force-pushed the fix-nftables-supportbundle branch from 95e0ce7 to f41be5c Compare November 7, 2025 14:38

molegit9 requested a review from hongliangl November 10, 2025 10:34

hongliangl reviewed Nov 11, 2025

View reviewed changes

molegit9 force-pushed the fix-nftables-supportbundle branch from f41be5c to a6a96a3 Compare November 11, 2025 09:39

molegit9 requested review from antoninbas and hongliangl November 12, 2025 02:02

hongliangl reviewed Nov 12, 2025

View reviewed changes

hongliangl requested a review from luolanzone November 12, 2025 05:56

molegit9 force-pushed the fix-nftables-supportbundle branch from a6a96a3 to c8a0ab9 Compare November 12, 2025 10:51

molegit9 changed the title ~~Fix(#7546): Include nftables information in Agent supportbundle~~ Include nftables information in Agent supportbundle Nov 12, 2025

molegit9 force-pushed the fix-nftables-supportbundle branch from c8a0ab9 to e917f2b Compare November 12, 2025 11:14

molegit9 requested a review from hongliangl November 12, 2025 11:55

antoninbas reviewed Nov 12, 2025

View reviewed changes

molegit9 requested a review from antoninbas November 13, 2025 06:30

antoninbas reviewed Nov 13, 2025

View reviewed changes

molegit9 force-pushed the fix-nftables-supportbundle branch from e917f2b to 9008f34 Compare November 15, 2025 17:24

molegit9 requested a review from antoninbas November 17, 2025 12:28

hongliangl reviewed Nov 24, 2025

View reviewed changes

molegit9 force-pushed the fix-nftables-supportbundle branch from 9008f34 to 2c85620 Compare November 24, 2025 12:37

hongliangl reviewed Nov 25, 2025

View reviewed changes

molegit9 force-pushed the fix-nftables-supportbundle branch from 2c85620 to 232b5a3 Compare November 25, 2025 03:01

hongliangl reviewed Nov 25, 2025

View reviewed changes

molegit9 force-pushed the fix-nftables-supportbundle branch from 232b5a3 to 7cd8f1d Compare November 25, 2025 03:25

hongliangl previously approved these changes Nov 25, 2025

View reviewed changes

molegit9 dismissed hongliangl’s stale review via fb3df3c November 25, 2025 06:25

molegit9 force-pushed the fix-nftables-supportbundle branch 5 times, most recently from 828fa93 to 0a3a75f Compare November 25, 2025 16:03

antoninbas reviewed Nov 25, 2025

View reviewed changes

molegit9 force-pushed the fix-nftables-supportbundle branch from 0a3a75f to 63d257d Compare November 26, 2025 00:11

molegit9 requested review from antoninbas and hongliangl November 26, 2025 00:15

antoninbas reviewed Nov 26, 2025

View reviewed changes

Include nftables information in Agent supportbundle

c1e1cda

Signed-off-by: molegit9 <[email protected]>

molegit9 force-pushed the fix-nftables-supportbundle branch from 63d257d to c1e1cda Compare November 27, 2025 04:24

molegit9 requested a review from antoninbas November 27, 2025 04:25

		if err := writeFile(d.fs, filepath.Join(basedir, fileName), fileName, output); err != nil {
		klog.Warningf("Failed to write %s: %v", fileName, err)

	klog.InfoS("Skipping nftables dump, 'nft' command not found", "err", err)
	klog.ErrorS(err, "'nft' command not found")

		if _, isFake := d.executor.(*testingexec.FakeExec); !isFake {
		if _, err := nftclient.New(d.v4Enabled, d.v6Enabled); err != nil {

Include nftables information in Agent supportbundle #7547

Are you sure you want to change the base?

Include nftables information in Agent supportbundle #7547

Uh oh!

Conversation

molegit9 commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

molegit9 commented Nov 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hongliangl Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hongliangl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hongliangl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hongliangl commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

molegit9 commented Nov 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hongliangl Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

molegit9 commented Nov 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hongliangl left a comment

Choose a reason for hiding this comment

Uh oh!

molegit9 commented Nov 6, 2025 •

edited

Loading

hongliangl Nov 7, 2025 •

edited

Loading

hongliangl left a comment •

edited

Loading

hongliangl commented Nov 12, 2025 •

edited

Loading

hongliangl Nov 13, 2025 •

edited

Loading