From 83fe87c5b01c1a013f9f080e60b8aba143a06e27 Mon Sep 17 00:00:00 2001 From: Miguel Duarte Barroso Date: Thu, 23 Mar 2023 13:54:15 +0100 Subject: [PATCH 1/3] build: consume specific tables/chains via go-nft This go-nft version allows its users to only read particular tables/chains when invoking `ReadConfig`, instead of the entire ruleset. This will make deleting rules from a large ruleset faster, thus speeding up CNI DELs. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2175041 Signed-off-by: Miguel Duarte Barroso --- go.mod | 2 +- go.sum | 4 +-- .../networkplumbing/go-nft/nft/config.go | 30 +++++++++++++++++-- .../networkplumbing/go-nft/nft/exec/exec.go | 18 +++++++---- vendor/modules.txt | 2 +- 5 files changed, 43 insertions(+), 13 deletions(-) diff --git a/go.mod b/go.mod index 486d01d8..76c175f0 100644 --- a/go.mod +++ b/go.mod @@ -14,7 +14,7 @@ require ( github.com/d2g/dhcp4server v0.0.0-20181031114812-7d4a0a7f59a5 github.com/godbus/dbus/v5 v5.1.0 github.com/mattn/go-shellwords v1.0.12 - github.com/networkplumbing/go-nft v0.2.0 + github.com/networkplumbing/go-nft v0.3.0 github.com/onsi/ginkgo/v2 v2.9.2 github.com/onsi/gomega v1.27.6 github.com/opencontainers/selinux v1.11.0 diff --git a/go.sum b/go.sum index 3e978577..567e35a9 100644 --- a/go.sum +++ b/go.sum @@ -486,8 +486,8 @@ github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8m github.com/mwitkow/go-conntrack v0.0.0-20161129095857-cc309e4a2223/go.mod h1:qRWi+5nqEBWmkhHvq77mSJWrCKwh8bxhgT7d/eI7P4U= github.com/mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f/go.mod h1:ZdcZmHo+o7JKHSa8/e818NopupXU1YMK5fe1lsApnBw= github.com/ncw/swift v1.0.47/go.mod h1:23YIA4yWVnGwv2dQlN4bB7egfYX6YLn0Yo/S6zZO/ZM= -github.com/networkplumbing/go-nft v0.2.0 h1:eKapmyVUt/3VGfhYaDos5yeprm+LPt881UeksmKKZHY= -github.com/networkplumbing/go-nft v0.2.0/go.mod h1:HnnM+tYvlGAsMU7yoYwXEVLLiDW9gdMmb5HoGcwpuQs= +github.com/networkplumbing/go-nft v0.3.0 h1:IIc6yHjN85KyJx21p3ZEsO0iBMYHNXux22rc9Q8TfFw= +github.com/networkplumbing/go-nft v0.3.0/go.mod h1:HnnM+tYvlGAsMU7yoYwXEVLLiDW9gdMmb5HoGcwpuQs= github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e h1:fD57ERR4JtEqsWbfPhv4DMiApHyliiK5xCTNVSPiaAs= github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e/go.mod h1:zD1mROLANZcx1PVRCS0qkT7pwLkGfwJo4zjcN/Tysno= github.com/nxadm/tail v1.4.4/go.mod h1:kenIhsEOeOJmVchQTgglprH7qJGnHDVpk1VPCcaMI8A= diff --git a/vendor/github.com/networkplumbing/go-nft/nft/config.go b/vendor/github.com/networkplumbing/go-nft/nft/config.go index af20a854..27ad5edb 100644 --- a/vendor/github.com/networkplumbing/go-nft/nft/config.go +++ b/vendor/github.com/networkplumbing/go-nft/nft/config.go @@ -20,12 +20,19 @@ package nft import ( + "context" + "time" + nftconfig "github.com/networkplumbing/go-nft/nft/config" nftexec "github.com/networkplumbing/go-nft/nft/exec" ) type Config = nftconfig.Config +const ( + defaultTimeout = 30 * time.Second +) + // NewConfig returns a new nftables config structure. func NewConfig() *nftconfig.Config { return nftconfig.New() @@ -34,12 +41,29 @@ func NewConfig() *nftconfig.Config { // ReadConfig loads the nftables configuration from the system and // returns it as a nftables config structure. // The system is expected to have the `nft` executable deployed and nftables enabled in the kernel. -func ReadConfig() (*Config, error) { - return nftexec.ReadConfig() +func ReadConfig(filterCommands ...string) (*Config, error) { + ctx, cancel := context.WithTimeout(context.Background(), defaultTimeout) + defer cancel() + return ReadConfigContext(ctx, filterCommands...) +} + +// ReadConfigContext loads the nftables configuration from the system and +// returns it as a nftables config structure. +// The system is expected to have the `nft` executable deployed and nftables enabled in the kernel. +func ReadConfigContext(ctx context.Context, filterCommands ...string) (*Config, error) { + return nftexec.ReadConfig(ctx, filterCommands...) } // ApplyConfig applies the given nftables config on the system. // The system is expected to have the `nft` executable deployed and nftables enabled in the kernel. func ApplyConfig(c *Config) error { - return nftexec.ApplyConfig(c) + ctx, cancel := context.WithTimeout(context.Background(), defaultTimeout) + defer cancel() + return ApplyConfigContext(ctx, c) +} + +// ApplyConfigContext applies the given nftables config on the system. +// The system is expected to have the `nft` executable deployed and nftables enabled in the kernel. +func ApplyConfigContext(ctx context.Context, c *Config) error { + return nftexec.ApplyConfig(ctx, c) } diff --git a/vendor/github.com/networkplumbing/go-nft/nft/exec/exec.go b/vendor/github.com/networkplumbing/go-nft/nft/exec/exec.go index c0531886..9ce3d181 100644 --- a/vendor/github.com/networkplumbing/go-nft/nft/exec/exec.go +++ b/vendor/github.com/networkplumbing/go-nft/nft/exec/exec.go @@ -21,6 +21,7 @@ package exec import ( "bytes" + "context" "fmt" "io/ioutil" "os" @@ -41,8 +42,13 @@ const ( // ReadConfig loads the nftables configuration from the system and // returns it as a nftables config structure. // The system is expected to have the `nft` executable deployed and nftables enabled in the kernel. -func ReadConfig() (*nftconfig.Config, error) { - stdout, err := execCommand(cmdJSON, cmdList, cmdRuleset) +func ReadConfig(ctx context.Context, filterCommands ...string) (*nftconfig.Config, error) { + + whatToList := cmdRuleset + if len(filterCommands) > 0 { + whatToList = strings.Join(filterCommands, " ") + } + stdout, err := execCommand(ctx, cmdJSON, cmdList, whatToList) if err != nil { return nil, err } @@ -57,7 +63,7 @@ func ReadConfig() (*nftconfig.Config, error) { // ApplyConfig applies the given nftables config on the system. // The system is expected to have the `nft` executable deployed and nftables enabled in the kernel. -func ApplyConfig(c *nftconfig.Config) error { +func ApplyConfig(ctx context.Context, c *nftconfig.Config) error { data, err := c.ToJSON() if err != nil { return err @@ -77,15 +83,15 @@ func ApplyConfig(c *nftconfig.Config) error { return fmt.Errorf("failed to close temporary file: %v", err) } - if _, err := execCommand(cmdJSON, cmdFile, tmpFile.Name()); err != nil { + if _, err := execCommand(ctx, cmdJSON, cmdFile, tmpFile.Name()); err != nil { return err } return nil } -func execCommand(args ...string) (*bytes.Buffer, error) { - cmd := exec.Command(cmdBin, args...) +func execCommand(ctx context.Context, args ...string) (*bytes.Buffer, error) { + cmd := exec.CommandContext(ctx, cmdBin, args...) var stdout, stderr bytes.Buffer cmd.Stderr = &stderr diff --git a/vendor/modules.txt b/vendor/modules.txt index 29a139e7..b21c622b 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -103,7 +103,7 @@ github.com/google/pprof/profile # github.com/mattn/go-shellwords v1.0.12 ## explicit; go 1.13 github.com/mattn/go-shellwords -# github.com/networkplumbing/go-nft v0.2.0 +# github.com/networkplumbing/go-nft v0.3.0 ## explicit; go 1.16 github.com/networkplumbing/go-nft/nft github.com/networkplumbing/go-nft/nft/config From 7dcd738d34e68a8ab3229c4e7f9ec5599abd00d6 Mon Sep 17 00:00:00 2001 From: Miguel Duarte Barroso Date: Thu, 23 Mar 2023 17:35:19 +0100 Subject: [PATCH 2/3] bridge, spoofcheck: only read the prerouting chain on CNI delete Signed-off-by: Miguel Duarte Barroso --- pkg/link/spoofcheck.go | 12 ++++++++---- pkg/link/spoofcheck_test.go | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/pkg/link/spoofcheck.go b/pkg/link/spoofcheck.go index 6c1bd535..82487fe9 100644 --- a/pkg/link/spoofcheck.go +++ b/pkg/link/spoofcheck.go @@ -29,7 +29,7 @@ const ( type NftConfigurer interface { Apply(*nft.Config) error - Read() (*nft.Config, error) + Read(filterCommands ...string) (*nft.Config, error) } type SpoofChecker struct { @@ -45,8 +45,8 @@ func (dnc defaultNftConfigurer) Apply(cfg *nft.Config) error { return nft.ApplyConfig(cfg) } -func (dnc defaultNftConfigurer) Read() (*nft.Config, error) { - return nft.ReadConfig() +func (dnc defaultNftConfigurer) Read(filterCommands ...string) (*nft.Config, error) { + return nft.ReadConfig(filterCommands...) } func NewSpoofChecker(iface, macAddress, refID string) *SpoofChecker { @@ -109,7 +109,7 @@ func (sc *SpoofChecker) Setup() error { // interface is removed. func (sc *SpoofChecker) Teardown() error { ifaceChain := sc.ifaceChain() - currentConfig, ifaceMatchRuleErr := sc.configurer.Read() + currentConfig, ifaceMatchRuleErr := sc.configurer.Read(listChainBridgeNatPrerouting()...) if ifaceMatchRuleErr == nil { expectedRuleToFind := sc.matchIfaceJumpToChainRule(preRoutingBaseChainName, ifaceChain.Name) // It is safer to exclude the statement matching, avoiding cases where a current statement includes @@ -241,3 +241,7 @@ func ruleComment(id string) string { const refIDPrefix = "macspoofchk-" return refIDPrefix + id } + +func listChainBridgeNatPrerouting() []string { + return []string{"chain", "bridge", natTableName, preRoutingBaseChainName} +} diff --git a/pkg/link/spoofcheck_test.go b/pkg/link/spoofcheck_test.go index 0ed8dde8..f26aa6a6 100644 --- a/pkg/link/spoofcheck_test.go +++ b/pkg/link/spoofcheck_test.go @@ -288,7 +288,7 @@ func (a *configurerStub) Apply(c *nft.Config) error { return nil } -func (a *configurerStub) Read() (*nft.Config, error) { +func (a *configurerStub) Read(_ ...string) (*nft.Config, error) { if a.failReadConfig { return nil, fmt.Errorf(errorReadText) } From 135292e0509f61f75353cc0c348246dd749f3fef Mon Sep 17 00:00:00 2001 From: Miguel Duarte Barroso Date: Thu, 20 Apr 2023 10:30:00 +0200 Subject: [PATCH 3/3] bridge, del: timeout after 55 secs of trying to list rules Making sure the exec'ed nft command is executed in 55 secs allows for CNI to fail early, thus preventing CRI from sending another CNI DEL while the previous NFT call is still being processed. This fix prevents part of the behavior described in [0], in which: > cnv-bridge and nft comes pile up in a loop, increasing every 60, never completes The timeout had to be less than 60 seconds (otherwise CRI would still trigger CNI DEL again) but large enough for this feature to have a chance of working on older kernels (e.g. centOS 8), where it takes longer to access even a specific chain/table. Signed-off-by: Miguel Duarte Barroso --- pkg/link/spoofcheck.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/pkg/link/spoofcheck.go b/pkg/link/spoofcheck.go index 82487fe9..f22a6753 100644 --- a/pkg/link/spoofcheck.go +++ b/pkg/link/spoofcheck.go @@ -15,8 +15,10 @@ package link import ( + "context" "fmt" "os" + "time" "github.com/networkplumbing/go-nft/nft" "github.com/networkplumbing/go-nft/nft/schema" @@ -46,7 +48,10 @@ func (dnc defaultNftConfigurer) Apply(cfg *nft.Config) error { } func (dnc defaultNftConfigurer) Read(filterCommands ...string) (*nft.Config, error) { - return nft.ReadConfig(filterCommands...) + const timeout = 55 * time.Second + ctxWithTimeout, cancelFunc := context.WithTimeout(context.Background(), timeout) + defer cancelFunc() + return nft.ReadConfigContext(ctxWithTimeout, filterCommands...) } func NewSpoofChecker(iface, macAddress, refID string) *SpoofChecker {