Added tests for the nodeportlocal package

[VaibhavMalik4187]

This commit introduces new tests for the pkg/agent/nodeportlocal/rules package.

Fixes: #5650

[VaibhavMalik4187]

@luolanzone, the netnat_rule.go file is giving the following warning:

This file is within module ".", which is not included in your workspace.
To fix this problem, you can add a go.work file that uses this directory.
See the documentation for more information on setting up your workspace:
https://github.com/golang/tools/blob/master/gopls/doc/workspace.md.go list

The iptables commands in the iptable_rule.go are failing due to the permission denied error when run during the tests.
Could you share some suggestions on how to fix these issues?

[tnqn]

The iptables commands in the iptable_rule.go are failing due to the permission denied error when run during the tests.
Could you share some suggestions on how to fix these issues?

Unit tests shouldn't need to really interact with iptables of the system. Instead, you should replace the iptables instance with a mock implementation and only validate if the expected calls has been made. Check the following test as an example:

antrea/pkg/agent/route/route_linux_test.go

Line 584 in 025c3f6

mockIPTables := iptablestest.NewMockInterface(ctrl)

[VaibhavMalik4187]

The iptables commands in the iptable_rule.go are failing due to the permission denied error when run during the tests.
Could you share some suggestions on how to fix these issues?

Unit tests shouldn't need to really interact with iptables of the system. Instead, you should replace the iptables instance with a mock implementation and only validate if the expected calls has been made. Check the following test as an example:

antrea/pkg/agent/route/route_linux_test.go

Line 584 in 025c3f6

mockIPTables := iptablestest.NewMockInterface(ctrl)

It makes sense, thanks for the suggestion. I've updated the code, I think that this PR is ready for a code review.

[XinShuYang]

hi @tnqn, do you know how to enable the github action in this PR for the UT workflow?

[tnqn]

The iptables commands in the iptable_rule.go are failing due to the permission denied error when run during the tests.
Could you share some suggestions on how to fix these issues?

Unit tests shouldn't need to really interact with iptables of the system. Instead, you should replace the iptables instance with a mock implementation and only validate if the expected calls has been made. Check the following test as an example:

antrea/pkg/agent/route/route_linux_test.go

Line 584 in 025c3f6

mockIPTables := iptablestest.NewMockInterface(ctrl)

It makes sense, thanks for the suggestion. I've updated the code, I think that this PR is ready for a code review.

I don't see the code reflects the change. It still calls the iptables command of the system. As seen from the test code, none of the call can succeed, so the test can't really ensure code work as expected. TestAddAndDeleteRule can get a "Successfully added DNAT rule" only because it uses an empty iptables.Client{}, which does nothing actually.

I think you should at least replace *iptables.Client in iptablesRules with iptables.Interface then you can use a mock implementation and check whether expected calls are made. The link I shared have more examples.

[tnqn]

hi @tnqn, do you know how to enable the github action in this PR for the UT workflow?

First time contributor needs a manual approval before the the tests can run. But since the code is not correct, I haven't clicked it yet.

[VaibhavMalik4187]

The iptables commands in the iptable_rule.go are failing due to the permission denied error when run during the tests.
Could you share some suggestions on how to fix these issues?

Unit tests shouldn't need to really interact with iptables of the system. Instead, you should replace the iptables instance with a mock implementation and only validate if the expected calls has been made. Check the following test as an example:

antrea/pkg/agent/route/route_linux_test.go

Line 584 in 025c3f6

mockIPTables := iptablestest.NewMockInterface(ctrl)

It makes sense, thanks for the suggestion. I've updated the code, I think that this PR is ready for a code review.

I don't see the code reflects the change. It still calls the iptables command of the system. As seen from the test code, none of the call can succeed, so the test can't really ensure code work as expected. TestAddAndDeleteRule can get a "Successfully added DNAT rule" only because it uses an empty iptables.Client{}, which does nothing actually.

I think you should at least replace *iptables.Client in iptablesRules with iptables.Interface then you can use a mock implementation and check whether expected calls are made. The link I shared have more examples.

Thanks for sharing the pointer @tnqn. This time, I was able to write some tests using the mockIPTable interface.

[VaibhavMalik4187]

@tnqn, I've made some updates to the code. Could you please review it again and let me know if the tests are now effective?

[tnqn]

I think this version is on the right direction.

[tnqn]

The field is deprecated, you could just set Protocol.

[VaibhavMalik4187]

Protocols are still being used at:

antrea/pkg/agent/nodeportlocal/rules/iptable_rule.go

Line 106 in f07d2eb

for _, protocol := range nplData.Protocols {

This is why I added this field to the tests. I've removed it in the latest version though.

[tnqn]

You are right. The code shouldn't keep using it. It will be changed by #5943 (review). For this PR, it will have to expect wrong output (note that the iptables rules used by "iptables-restore" are empty)

[tnqn]

/skip-all

[tnqn]

LGTM, thanks @VaibhavMalik4187

[VaibhavMalik4187]

LGTM, thanks @VaibhavMalik4187

Thank you for the pointers and the code review.