test: refactor container_kill_linux_test.go to use Tigron

[microness]

related: #4613

[ChengyuZhu6]

@microness I found the refactoring dropped a check "iptables rule removed after kill". Would you mind adding the post-kill validation?

[microness]

Thanks for the comment! Fixed.

[haytok]

IMO, instead of hard-coding the port number, it is better to use the port number acquired via portlock.Acquire(). So, could you rewrite it?

• https://github.com/containerd/nerdctl/blob/main/pkg/testutil/portlock/portlock.go

[microness]

Thanks for the comment! Fixed.

[haytok]

Can we re-write using nerdtest.Rootless ?

• https://github.com/containerd/nerdctl/blob/main/docs/testing/tools.md#requirements

[microness]

Thanks for the comment! Fixed.

[haytok]

Could you add logic to release the acquired port number?

• https://github.com/containerd/nerdctl/blob/main/pkg/testutil/portlock/portlock.go#L24
• https://github.com/containerd/nerdctl/blob/main/pkg/testutil/portlock/portlock.go#L59

		if p := data.Labels().Get("hostPort"); p != "" {
			if port, err := strconv.Atoi(p); err == nil {
				_ = portlock.Release(port)
			}
		}

[haytok]

Testing it within the Setup seems odd—couldn't we implement it using helper.Custom?

[microness]

Thanks for the comments!

I’ve fixed on your feedback and have a few questions.
I’d appreciate a re-review.

Q1. Using helpers.Custom in Setup

I also felt that putting assert.Assert directly in Setup was a bit awkward,
So I refactored it with helpers.Custom as suggested below.

// Description: "iptables forwarding rule should exist before container is killed"

// Setup:
helpers.Custom(
	"iptables",
	"-t", "nat",
	"-S", chain,
).Run(&test.Expected{
	ExitCode: expect.ExitCodeSuccess,
	Output: func(stdout string, t tig.T) {
		assert.Assert(t, strings.Contains(stdout, fmt.Sprintf("--dport %d", hostPort)))
		assert.Assert(t, strings.Contains(stdout, "DNAT"))
		assert.Assert(t, strings.Contains(stdout, containerIP))
	},
})

According to the documentation, helpers can be used in all phases including Setup and Cleanup.

nerdctl/mod/tigron/test/interfaces.go

Lines 70 to 72 in 30bc993

	// Helpers provides a set of helpers to run commands with simple expectations,
	// available at all stages of a test (Setup, Cleanup, etc...)
	type Helpers interface {

Does that mean using helpers.Custom in Setup is acceptable in your view?

Q2. Using iptablesutil.ForwardExists vs helpers.Custom with iptables command

For the same reason, I replaced iptablesutil.ForwardExists with a helpers.Custom with iptables command.
Do you think this is a better fit for Setup, or would you prefer keeping the iptablesutil?

Thanks again for the review, I learned a lot.

[haytok]

Thanks for comments 🙏

Does that mean using helpers.Custom in Setup is acceptable in your view?

I had intended to describe it as follow:

Command: func(data test.Data, helpers test.Helpers) test.TestableCommand {
        return helpers.Custom("iptables", "-t", "nat", "-S", data.Labels().Get("chain"))
},

Because, as can be seen from the Helpers interface, it seems better to test by executing commands.

Q2. Using iptablesutil.ForwardExists vs helpers.Custom with iptables command

While this may overlap with what was mentioned earlier, I believe it's better to execute the iptables command using helpers.Custom.

The iptablesutil.ForwardExists function was implemented to read the output of iptables -t nat -S <chain name>. Therefore, I thought it would be better to determine if a matching rule exists based on the output of the iptables command, like the example below. What do you think?

Details

# container_kill_linux_test.go
{
        Description: "iptables forwarding rule should exist before container is killed",
        NoParallel:  true,
        Command: func(data test.Data, helpers test.Helpers) test.TestableCommand {
                return helpers.Custom("iptables", "-t", "nat", "-S", data.Labels().Get("chain"))
        },
        Expected: func(data test.Data, helpers test.Helpers) *test.Expected {
                return &test.Expected{
                        Output: func(stdout string, t tig.T) {
                                rules := strings.Split(stdout, "\n")
                                containerIP := data.Labels().Get("containerIP")
                                hostPort, err := strconv.Atoi(data.Labels().Get("hostPort"))
                                assert.NilError(t, err)
                                found, err := iptablesutil.ForwardExistsFromRules(rules, containerIP, hostPort)
                                assert.NilError(t, err)
                                assert.Assert(t, found)
                        },
                }
        },
},

# pkg/testutil/iptables/iptables_linux.go
func ForwardExistsFromRules(rules []string, containerIP string, port int) (bool, error) {
	if len(rules) < 1 {
		return false, fmt.Errorf("not enough rules: %d", len(rules))
	}

	// here we check if at least one of the rules in the chain
	// matches the required string to identify that the rule was applied
	found := false
	matchRule := `--dport ` + fmt.Sprintf("%d", port) + ` .+ --to-destination ` + containerIP
	for _, rule := range rules {
		foundInRule, err := regexp.MatchString(matchRule, rule)
		if err != nil {
			return false, fmt.Errorf("error in match string: %q", err)
		}
		if foundInRule {
			found = foundInRule
		}
	}
	return found, nil
}

Thanks!

[microness]

Sorry for the confusion and thanks for pointing that out.

I agree with your suggestion.

Thanks for the review!

[haytok]

LGTM, Thanks!

[haytok]

CC: @ChengyuZhu6 @AkihiroSuda

When you have time, could you retry the CI and review 🙏