Skip to content

Commit 0c33daa

Browse files
ncodencode
authored
fix logging to match the max size supported by iptables (#15)
finally fixed the space drama with logging and the tests use a constance for the filter table and re-organize tests adds missing case adds missing tests for log drops
1 parent bf0ed7a commit 0c33daa

File tree

4 files changed

+616
-1715
lines changed

4 files changed

+616
-1715
lines changed

‎outbound.go‎

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"encoding/json"
66
"fmt"
7+
"github.com/containernetworking/plugins/pkg/utils"
78
"io"
89
"log/slog"
910
"maps"
@@ -15,7 +16,6 @@ import (
1516
"github.com/containernetworking/cni/pkg/types"
1617
current "github.com/containernetworking/cni/pkg/types/100"
1718
"github.com/containernetworking/cni/pkg/version"
18-
"github.com/containernetworking/plugins/pkg/utils"
1919
bv "github.com/containernetworking/plugins/pkg/utils/buildversion"
2020
"github.com/ncode/cni-outbound/pkg/iptables"
2121
)
@@ -43,8 +43,8 @@ var (
4343
// logger is the structured logger instance used throughout the plugin.
4444
logger = slog.New(slog.NewTextHandler(io.Discard, nil))
4545
// newIPTablesManager is a function pointer for creating an IPTablesManager (for mocking in tests).
46-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
47-
return iptables.NewIPTablesManager(conf.MainChainName, conf.DefaultAction, conf.DryRun, conf.LogDrops)
46+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
47+
return iptables.NewIPTablesManager(conf.MainChainName, conf.DefaultAction, logIdentifier, conf.DryRun, conf.LogDrops)
4848
}
4949
// metadata is used to store logging metadata (e.g., container ID).
5050
metadata = map[string]string{}
@@ -61,11 +61,6 @@ func getLogAttrs() slog.Attr {
6161
return slog.Group("metadata", attrs...)
6262
}
6363

64-
// generateChainName creates a short but unique chain name using a prefix and the container ID.
65-
func generateChainName(netName, containerID string) string {
66-
return utils.MustFormatChainNameWithPrefix(netName, containerID, "OUT-")
67-
}
68-
6964
// parseArgs extracts additional outbound rules and metadata from CNI_ARGS.
7065
func parseArgs(args string) ([]iptables.OutboundRule, map[string]string, error) {
7166
logger.Log(context.Background(), slog.LevelInfo,
@@ -314,7 +309,8 @@ func cmdAdd(args *skel.CmdArgs) error {
314309
getLogAttrs(),
315310
)
316311

317-
iptManager, err := newIPTablesManager(conf)
312+
containerChain := utils.MustFormatChainNameWithPrefix(conf.Name, args.ContainerID, "OUT-")
313+
iptManager, err := newIPTablesManager(conf, strings.Replace(containerChain, "CNI-OUT-", "", -1))
318314
if err != nil {
319315
logger.Log(context.Background(), slog.LevelError,
320316
"Failed to create IPTablesManager",
@@ -343,7 +339,6 @@ func cmdAdd(args *skel.CmdArgs) error {
343339
getLogAttrs(),
344340
)
345341

346-
containerChain := generateChainName(conf.Name, args.ContainerID)
347342
if err := iptManager.CreateContainerChain(containerChain); err != nil {
348343
logger.Log(context.Background(), slog.LevelError,
349344
"Failed to create container chain",
@@ -457,7 +452,8 @@ func cmdDel(args *skel.CmdArgs) error {
457452
getLogAttrs(),
458453
)
459454

460-
iptManager, err := newIPTablesManager(conf)
455+
containerChain := utils.MustFormatChainNameWithPrefix(conf.Name, args.ContainerID, "OUT-")
456+
iptManager, err := newIPTablesManager(conf, strings.Replace(containerChain, "CNI-OUT-", "", -1))
461457
if err != nil {
462458
logger.Log(context.Background(), slog.LevelError,
463459
"Failed to create IPTablesManager",
@@ -472,7 +468,6 @@ func cmdDel(args *skel.CmdArgs) error {
472468
getLogAttrs(),
473469
)
474470

475-
containerChain := generateChainName(conf.Name, args.ContainerID)
476471
if err := iptManager.RemoveJumpRuleByTargetChain(containerChain); err != nil {
477472
logger.Log(context.Background(), slog.LevelWarn,
478473
"Failed to remove jump rule from main chain",
@@ -526,7 +521,8 @@ func cmdCheck(args *skel.CmdArgs) error {
526521
getLogAttrs(),
527522
)
528523

529-
iptManager, err := newIPTablesManager(conf)
524+
containerChain := utils.MustFormatChainNameWithPrefix(conf.Name, args.ContainerID, "OUT-")
525+
iptManager, err := newIPTablesManager(conf, strings.Replace(containerChain, "CNI-OUT-", "", -1))
530526
if err != nil {
531527
logger.Log(context.Background(), slog.LevelError,
532528
"Failed to create IPTablesManager",
@@ -564,7 +560,6 @@ func cmdCheck(args *skel.CmdArgs) error {
564560
getLogAttrs(),
565561
)
566562

567-
containerChain := generateChainName(conf.Name, args.ContainerID)
568563
exists, err = iptManager.ChainExists(containerChain)
569564
if err != nil {
570565
logger.Log(context.Background(), slog.LevelError,

‎outbound_test.go‎

Lines changed: 25 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -497,7 +497,7 @@ func TestCmdAdd(t *testing.T) {
497497
mockManager.On("AddJumpRule", "10.0.0.2", mock.Anything).Return(nil)
498498

499499
origNewIPTablesManager := newIPTablesManager
500-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
500+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
501501
return mockManager, nil
502502
}
503503
defer func() { newIPTablesManager = origNewIPTablesManager }()
@@ -544,7 +544,7 @@ func TestCmdAddIPTablesManagerFailure(t *testing.T) {
544544

545545
// Override newIPTablesManager to return an error
546546
origNewIPTablesManager := newIPTablesManager
547-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
547+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
548548
return nil, fmt.Errorf("failed to create IPTablesManager")
549549
}
550550
defer func() { newIPTablesManager = origNewIPTablesManager }()
@@ -588,7 +588,7 @@ func TestCmdAddNoIPs(t *testing.T) {
588588
mockManager := new(MockIPTablesManager)
589589

590590
origNewIPTablesManager := newIPTablesManager
591-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
591+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
592592
return mockManager, nil
593593
}
594594
defer func() { newIPTablesManager = origNewIPTablesManager }()
@@ -646,7 +646,7 @@ func TestCmdAddEnsureMainChainExistsFailure(t *testing.T) {
646646
mockManager.On("EnsureMainChainExists").Return(fmt.Errorf("failed to create main chain"))
647647

648648
origNewIPTablesManager := newIPTablesManager
649-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
649+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
650650
return mockManager, nil
651651
}
652652
defer func() { newIPTablesManager = origNewIPTablesManager }()
@@ -706,7 +706,7 @@ func TestCmdAddEnsureCreateContainerChainFailure(t *testing.T) {
706706
mockManager.On("CreateContainerChain", mock.Anything).Return(fmt.Errorf("failed to create container chain"))
707707

708708
origNewIPTablesManager := newIPTablesManager
709-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
709+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
710710
return mockManager, nil
711711
}
712712
defer func() { newIPTablesManager = origNewIPTablesManager }()
@@ -773,7 +773,7 @@ func TestCmdAddRuleFailure(t *testing.T) {
773773
})).Return(fmt.Errorf("failed to add rule"))
774774

775775
origNewIPTablesManager := newIPTablesManager
776-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
776+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
777777
return mockManager, nil
778778
}
779779
defer func() { newIPTablesManager = origNewIPTablesManager }()
@@ -812,7 +812,7 @@ func TestCmdAddNoPrevResult(t *testing.T) {
812812

813813
// Override newIPTablesManager
814814
origNewIPTablesManager := newIPTablesManager
815-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
815+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
816816
return mockManager, nil
817817
}
818818
defer func() { newIPTablesManager = origNewIPTablesManager }()
@@ -871,7 +871,7 @@ func TestCmdAddJumpRuleFailure(t *testing.T) {
871871
mockManager.On("AddJumpRule", "10.0.0.2", mock.Anything).Return(fmt.Errorf("failed to add jump rule"))
872872

873873
origNewIPTablesManager := newIPTablesManager
874-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
874+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
875875
return mockManager, nil
876876
}
877877
defer func() { newIPTablesManager = origNewIPTablesManager }()
@@ -909,7 +909,7 @@ func TestCmdDel(t *testing.T) {
909909
mockManager.On("ClearAndDeleteChain", mock.Anything).Return(nil)
910910

911911
origNewIPTablesManager := newIPTablesManager
912-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
912+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
913913
return mockManager, nil
914914
}
915915
defer func() { newIPTablesManager = origNewIPTablesManager }()
@@ -941,7 +941,7 @@ func TestCmdDelParseConfigError(t *testing.T) {
941941
mockManager := new(MockIPTablesManager)
942942

943943
origNewIPTablesManager := newIPTablesManager
944-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
944+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
945945
return mockManager, nil
946946
}
947947
defer func() { newIPTablesManager = origNewIPTablesManager }()
@@ -974,7 +974,7 @@ func TestCmdDelRemoveJumpRuleByTargetChainError(t *testing.T) {
974974
mockManager.On("ClearAndDeleteChain", mock.Anything).Return(nil)
975975

976976
origNewIPTablesManager := newIPTablesManager
977-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
977+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
978978
return mockManager, nil
979979
}
980980
defer func() { newIPTablesManager = origNewIPTablesManager }()
@@ -1021,7 +1021,7 @@ func TestCmdDelIPTablesManagerFailure(t *testing.T) {
10211021

10221022
// Override newIPTablesManager to return an error
10231023
origNewIPTablesManager := newIPTablesManager
1024-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
1024+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
10251025
return nil, fmt.Errorf("failed to create IPTablesManager")
10261026
}
10271027
defer func() { newIPTablesManager = origNewIPTablesManager }()
@@ -1054,7 +1054,7 @@ func TestCmdDelClearAndDeleteChainError(t *testing.T) {
10541054
mockManager.On("ClearAndDeleteChain", mock.Anything).Return(fmt.Errorf("failed to clear and delete chain"))
10551055

10561056
origNewIPTablesManager := newIPTablesManager
1057-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
1057+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
10581058
return mockManager, nil
10591059
}
10601060
defer func() { newIPTablesManager = origNewIPTablesManager }()
@@ -1091,7 +1091,7 @@ func TestCmdCheck(t *testing.T) {
10911091
mockManager.On("VerifyRules", mock.Anything, mock.Anything).Return(nil)
10921092

10931093
origNewIPTablesManager := newIPTablesManager
1094-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
1094+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
10951095
return mockManager, nil
10961096
}
10971097
defer func() { newIPTablesManager = origNewIPTablesManager }()
@@ -1135,7 +1135,7 @@ func TestCmdCheckWithMetadata(t *testing.T) {
11351135
mockManager.On("VerifyRules", mock.Anything, mock.Anything).Return(nil)
11361136

11371137
origNewIPTablesManager := newIPTablesManager
1138-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
1138+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
11391139
return mockManager, nil
11401140
}
11411141
defer func() { newIPTablesManager = origNewIPTablesManager }()
@@ -1166,7 +1166,7 @@ func TestCmdCheckNewIPTablesManagerFailure(t *testing.T) {
11661166
}
11671167

11681168
origNewIPTablesManager := newIPTablesManager
1169-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
1169+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
11701170
return nil, fmt.Errorf("failed to create IPTablesManager")
11711171
}
11721172
defer func() { newIPTablesManager = origNewIPTablesManager }()
@@ -1198,7 +1198,7 @@ func TestCmdCheckChainExistsFailureForMainChain(t *testing.T) {
11981198
mockManager.On("ChainExists", "TEST-OUTBOUND").Return(false, nil)
11991199

12001200
origNewIPTablesManager := newIPTablesManager
1201-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
1201+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
12021202
return mockManager, nil
12031203
}
12041204
defer func() { newIPTablesManager = origNewIPTablesManager }()
@@ -1235,7 +1235,7 @@ func TestCmdCheckChainExistsFailure(t *testing.T) {
12351235

12361236
// Override newIPTablesManager
12371237
origNewIPTablesManager := newIPTablesManager
1238-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
1238+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
12391239
return mockManager, nil
12401240
}
12411241
defer func() { newIPTablesManager = origNewIPTablesManager }()
@@ -1278,7 +1278,7 @@ func TestCmdCheckContainerChainExistsFailure(t *testing.T) {
12781278

12791279
// Override newIPTablesManager
12801280
origNewIPTablesManager := newIPTablesManager
1281-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
1281+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
12821282
return mockManager, nil
12831283
}
12841284
defer func() { newIPTablesManager = origNewIPTablesManager }()
@@ -1314,7 +1314,7 @@ func TestCmdCheckChainExistsFailureForContainerChain(t *testing.T) {
13141314
mockManager.On("ChainExists", mock.Anything).Return(false, nil)
13151315

13161316
origNewIPTablesManager := newIPTablesManager
1317-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
1317+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
13181318
return mockManager, nil
13191319
}
13201320
defer func() { newIPTablesManager = origNewIPTablesManager }()
@@ -1352,7 +1352,7 @@ func TestCmdCheckVerifyRulesFailure(t *testing.T) {
13521352
mockManager.On("VerifyRules", mock.Anything, mock.Anything).Return(fmt.Errorf("rule verification failed"))
13531353

13541354
origNewIPTablesManager := newIPTablesManager
1355-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
1355+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
13561356
return mockManager, nil
13571357
}
13581358
defer func() { newIPTablesManager = origNewIPTablesManager }()
@@ -1400,7 +1400,7 @@ func TestCmdCheckIPTablesManagerFailure(t *testing.T) {
14001400

14011401
// Override newIPTablesManager to return an error
14021402
origNewIPTablesManager := newIPTablesManager
1403-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
1403+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
14041404
return nil, fmt.Errorf("failed to create IPTablesManager")
14051405
}
14061406
defer func() { newIPTablesManager = origNewIPTablesManager }()
@@ -1432,7 +1432,7 @@ func TestCmdCheckParseConfigError(t *testing.T) {
14321432
mockManager := new(MockIPTablesManager)
14331433

14341434
origNewIPTablesManager := newIPTablesManager
1435-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
1435+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
14361436
return mockManager, nil
14371437
}
14381438
defer func() { newIPTablesManager = origNewIPTablesManager }()
@@ -1644,12 +1644,6 @@ func TestParseArgsWithLogging(t *testing.T) {
16441644
}
16451645
}
16461646

1647-
func TestGenerateChainName(t *testing.T) {
1648-
chainName := generateChainName("test-net", "test-container")
1649-
assert.NotEmpty(t, chainName)
1650-
assert.Contains(t, chainName, "OUT-")
1651-
}
1652-
16531647
func TestParseArgs(t *testing.T) {
16541648
testCases := []struct {
16551649
name string
@@ -1981,7 +1975,7 @@ func TestCmdAdd_FailedToParsePrevResult(t *testing.T) {
19811975

19821976
// 3. We mock newIPTablesManager if needed or just let the plugin create a no-op
19831977
originalNewIPTablesManager := newIPTablesManager
1984-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
1978+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
19851979
// Return a mock or a no-op manager so we don't fail on iptables calls
19861980
return &mockNoOpManager{}, nil
19871981
}
@@ -2033,7 +2027,7 @@ func TestCmdAdd_NoIPv4Addresses(t *testing.T) {
20332027
// If we actually create an iptables manager, it won't matter because
20342028
// we'll fail before we call iptables methods. But we can still override:
20352029
originalNewIPTablesManager := newIPTablesManager
2036-
newIPTablesManager = func(conf *PluginConf) (iptables.Manager, error) {
2030+
newIPTablesManager = func(conf *PluginConf, logIdentifier string) (iptables.Manager, error) {
20372031
return &mockNoOpManager{}, nil
20382032
}
20392033
defer func() { newIPTablesManager = originalNewIPTablesManager }()

0 commit comments

Comments
 (0)