Testing:

Create a docker network for your test

docker network create iperf

Create an iperf server

To listen to multicast

docker run --rm -it --network=iperf --name server sk278/iperf -s -u -B 226.94.1.1 -i 1

Run an iperf client

docker run --rm -it --network=iperf --name client sk278/iperf -c 226.94.1.1 -u -T 32 -t 3 -i 1

Watch the output on your server

No packet loss...

[  1] local 226.94.1.1 port 5001 connected with 172.18.0.3 port 56090
[ ID] Interval       Transfer     Bandwidth        Jitter   Lost/Total Datagrams
[  1] 0.00-1.00 sec   131 KBytes  1.07 Mbits/sec   0.014 ms 0/91 (0%)
[  1] 1.00-2.00 sec   128 KBytes  1.05 Mbits/sec   0.029 ms 0/89 (0%)
[  1] 2.00-3.00 sec   128 KBytes  1.05 Mbits/sec   0.017 ms 0/89 (0%)
[  1] 0.00-3.02 sec   389 KBytes  1.06 Mbits/sec   0.016 ms 0/271 (0%)

Start pumba to apply loss on your server

pumba --iptables-image rancher/mirrored-kube-vip-kube-vip-iptables:v0.8.9 -d 1m -p udp --dport 5001 loss --probability 0.2 server

Start you client again

docker run --rm -it --network=iperf --name client sk278/iperf -c 226.94.1.1 -u -T 32 -t 3 -i 1

Magic

[  2] local 226.94.1.1 port 5001 connected with 172.18.0.3 port 36262
[ ID] Interval       Transfer     Bandwidth        Jitter   Lost/Total Datagrams
[  2] 0.00-1.00 sec   115 KBytes   941 Kbits/sec   0.025 ms 11/91 (12%)
[  2] 1.00-2.00 sec   109 KBytes   894 Kbits/sec   0.014 ms 13/89 (15%)
[  2] 2.00-3.00 sec  97.6 KBytes   800 Kbits/sec   0.021 ms 21/89 (24%)
[  2] 0.00-3.02 sec   324 KBytes   880 Kbits/sec   0.020 ms 45/271 (17%)

I may need some help with these failing tests and why they are failing...

In pkg/container/docker_client.go, there's a critical bug in the command construction logic for IP and port filters.

Each of the four for loops (srcIPs, dstIPs, sports, dports) has the same issue:

The second append call should use cmd instead of cmdPrefix:

// Current:
cmd := append(cmdPrefix, "-s", ip.String())
cmd = append(cmdPrefix, cmdSuffix...) // WRONG: using cmdPrefix again

// Should be:
cmd := append(cmdPrefix, "-s", ip.String())
cmd = append(cmd, cmdSuffix...) // RIGHT: continue building on cmd

This bug will cause all filters to be ignored, as each command ends up being just cmdPrefix followed by cmdSuffix without the actual filter parameters.

In pkg/container/mock_Client.go, there's an inconsistency in the mock implementation of StopIPTablesContainer. The parameter list doesn't match the actual implementation in docker_client.go.

The mock is missing the container parameter which will cause test failures:

// Current:
// StopIPTablesContainer provides a mock function with given fields: _a0, _a1, _a2, _a3, _a4, _a5, _a6, _a7, _a8, _a9
func (_m *MockClient) StopIPTablesContainer(_a0 context.Context, _a1 *Container, _a2, _a3 []string, _a4, _a5 []*net.IPNet, _a6 []string, _a7 []string, _a8 string, _a9 bool, _a10 bool) error {
  // ...
}

Please ensure all mock implementations match their interface definitions.

I use Mockery tool to generate mock for interfaces. Check the Makefile and regenerate mocks for updated interfaces.

And thank you for this PR!

Thank you for the thorough review! Good finds on the cmdPrefix / cmdSuffix errors!

Some comments and requests for additional info inline below:

Code Style and Consistency Issues

1. Documentation: Please add more explanatory comments for the public functions, similar to those in the netem package.

Anywhere specific? I copied the netem sources to the iptables implementation and updated all the comments so comments look familiar to the netem package.

2. Error Message Formatting: Standardize on using errors.Wrap/Wrapf pattern consistently:

   // Instead of:
   return errors.New("invalid loss mode: must be either random or nth")
   
   // Prefer:
   return errors.Errorf("invalid loss mode: %s, must be either random or nth", mode)

I updated this in loss.go but in general errors.New is used throughout the original code as well...

3. Logging Consistency: Align logging levels with netem package. Some operations use debug level in iptables where netem uses info level for similar operations.

Can you provide a pointer where you noticed this? Similar to 1 I copied the functions and updated the logging text but did not change the levels.

5. Parameter Ordering: Ensure parameter ordering in interface methods follows the established patterns.

Not sure what you mean by this comment?

Other Issues

1. MockClient Interface: The StopIPTablesContainer mock function signature doesn't match the actual interface, which could cause test failures. Please fix the mock implementation.

Used the mock generator to do this, hopefully this is now fixed.

Sorry but it took me some time to figure out what went wrong with the test case but now it successfully runs on my dev system.

I've also updated the Dockerfile but you may want to look at my changes as they also bump the version of go. I can revert these changes if you prefer.

Thank you for addressing the critical command construction bug and other issues from my previous review.

I noticed one remaining validation issue:

In pkg/chaos/iptables/loss.go, the validation for the 'every' parameter in 'nth' mode allows a value of 0:

if every < 0 {
    return nil, errors.Errorf("invalid loss every value: must be >= 0")
}

For 'nth' mode, an 'every' value of 0 would cause division by zero issues or illogical behavior. Please update the validation to require 'every > 0' specifically when in 'nth' mode:

if mode == "nth" && every <= 0 {
    return nil, errors.Errorf("invalid loss every value for nth mode: must be > 0")
}

Please also add unit tests to verify this validation behavior.

Otherwise, the PR looks good to merge after this fix is applied.

Hi @eelcocramer,

Thanks for your work on this PR! We're getting close - I just wanted to point out a version compatibility issue that's causing CI to fail.

The current error is related to the generated mock files and Go versions:

pkg/container/mock_Client.go:21:12: _m.Called undefined (type *MockClient has no field or method Called)
pkg/container/mock_FilterFunc.go:14:12: _m.Called undefined (type *MockFilterFunc has no field or method Called)
pkg/container/mock_conn.go:20:12: _m.Called undefined (type *mockConn has no field or method Called)

I noticed the PR's go.mod specifies Go 1.23/1.24.1 toolchain, but our CI is likely running on Go 1.21. It seems the mock files were generated with mockery v2.53.2 on a newer Go version (1.24), producing code that's incompatible with Go 1.21.

You have two options to resolve this:

1. Regenerate mocks with Go 1.21:

   • Use Go 1.21 to regenerate mocks with a compatible mockery version
   • Keep the project on Go 1.21 for this PR

2. Upgrade the project to Go 1.24:

   • Update all GitHub workflows to use Go 1.24
   • Verify all dependencies are compatible with Go 1.24
   • May require additional changes to CI configuration

Either approach is fine with me - you can choose what works best. If you decide to upgrade to Go 1.24, it might be cleaner to do that in a separate PR first, before merging this one.

Let me know if you have any questions!

I've updated the repository workflow permissions to allow contributors to run the workflows on their PRs. You should now be able to re-run failed workflows without my approval. Feel free to make the suggested changes and verify your fixes work by running the GitHub Actions workflows.

I've updated the repository workflow permissions to allow contributors to run the workflows on their PRs. You should now be able to re-run failed workflows without my approval. Feel free to make the suggested changes and verify your fixes work by running the GitHub Actions workflows.

Hm, I don't think I'm able to run these workflow yet.

@eelcocramer please run linter with make lint command - there are a few places in the code where you are appending data to a wrong slice.

@eelcocramer please run linter with make lint command - there are a few places in the code where you are appending data to a wrong slice.

Anything to satisfy the linter ;-)

Great, thanks a lot!