Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

spoofcheck: Make use of go-nft's ApplyConfigEcho() #902

Merged
merged 1 commit into from
Jul 20, 2023
Merged

spoofcheck: Make use of go-nft's ApplyConfigEcho() #902

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.3.0
github.com/networkplumbing/go-nft v0.4.0
github.com/onsi/ginkgo/v2 v2.11.0
github.com/onsi/gomega v1.27.8
github.com/opencontainers/selinux v1.11.0
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.3.0 h1:IIc6yHjN85KyJx21p3ZEsO0iBMYHNXux22rc9Q8TfFw=
github.com/networkplumbing/go-nft v0.3.0/go.mod h1:HnnM+tYvlGAsMU7yoYwXEVLLiDW9gdMmb5HoGcwpuQs=
github.com/networkplumbing/go-nft v0.4.0 h1:kExVMwXW48DOAukkBwyI16h4uhE5lN9iMvQd52lpTyU=
github.com/networkplumbing/go-nft v0.4.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=
Expand Down
68 changes: 43 additions & 25 deletions pkg/link/spoofcheck.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ const (
)

type NftConfigurer interface {
Apply(*nft.Config) error
Apply(*nft.Config) (*nft.Config, error)
Read(filterCommands ...string) (*nft.Config, error)
}

Expand All @@ -39,12 +39,16 @@ type SpoofChecker struct {
macAddress string
refID string
configurer NftConfigurer
rulestore *nft.Config
}

type defaultNftConfigurer struct{}

func (dnc defaultNftConfigurer) Apply(cfg *nft.Config) error {
return nft.ApplyConfig(cfg)
func (dnc defaultNftConfigurer) Apply(cfg *nft.Config) (*nft.Config, error) {
const timeout = 55 * time.Second
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this need a separate timeout?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since nft.ApplyConfigEcho() requires a context, I had to create one so I just copied the respective code from Read(). Not sure if it is possible to create a context without timeout, but looking at commit 135292e ("bridge, del: timeout after 55 secs of trying to list rules"), which introduced the 55s timeout to Read(), it seems sensible to make ApplyConfigEcho() behave the same.
Or are you suggesting to introduce a common timeout value for use in both methods?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a big problem, but it's certainly not necessary. A CNI plugin binary is executed for every call, so if the runtime (read: containerd / cri-o) decides to cancel the CNI execution, it will just kill the binary. That will certainly stop any nft calls :-). In that case, you can just use context.Background() which will never be cancelled.

That said, 55 seconds seems like more than enough for nft to apply (I hope!), so this is probably academic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, frequent updates to an oversized nftables ruleset have a realistic chance of starving readers. Each time the ruleset's generation ID bumps, the reader discards the already fetched cache and restarts. Older nft versions always fetched the full ruleset irrespective of the actual command (so listing a small chain would still take long if a large other one was present).
I don't think this is possible to happen with echo mode, but I don't see why having the timeout should hurt.
Your explanation sounds like the context with timeout is pointless, though I assume @maiqueb fixed a real issue when introducing the Read() timeout.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A CNI plugin binary is executed for every call, so if the runtime (read: containerd / cri-o) decides to cancel the CNI execution, it will just kill the binary. That will certainly stop any nft calls :-). In that case, you can just use context.Background() which will never be cancelled.

We saw in k8s based systems that there is no timeout that eventually kills the call to the CNI.
We identified thousands of nft instances hanging with no one canceling them.

I think it is not safe to assume someone will cancel the CNI call after a timeout, it depends on the runtime or even the one who calls the runtime in the first place.

ctxWithTimeout, cancelFunc := context.WithTimeout(context.Background(), timeout)
defer cancelFunc()
return nft.ApplyConfigEcho(ctxWithTimeout, cfg)
}

func (dnc defaultNftConfigurer) Read(filterCommands ...string) (*nft.Config, error) {
Expand All @@ -59,7 +63,7 @@ func NewSpoofChecker(iface, macAddress, refID string) *SpoofChecker {
}

func NewSpoofCheckerWithConfigurer(iface, macAddress, refID string, configurer NftConfigurer) *SpoofChecker {
return &SpoofChecker{iface, macAddress, refID, configurer}
return &SpoofChecker{iface, macAddress, refID, configurer, nil}
}

// Setup applies nftables configuration to restrict traffic
Expand Down Expand Up @@ -88,7 +92,7 @@ func (sc *SpoofChecker) Setup() error {
macChain := sc.macChain(ifaceChain.Name)
baseConfig.AddChain(macChain)

if err := sc.configurer.Apply(baseConfig); err != nil {
if _, err := sc.configurer.Apply(baseConfig); err != nil {
return fmt.Errorf("failed to setup spoof-check: %v", err)
}

Expand All @@ -102,45 +106,59 @@ func (sc *SpoofChecker) Setup() error {
rulesConfig.AddRule(sc.matchMacRule(macChain.Name))
rulesConfig.AddRule(sc.dropRule(macChain.Name))

if err := sc.configurer.Apply(rulesConfig); err != nil {
rulestore, err := sc.configurer.Apply(rulesConfig)
if err != nil {
return fmt.Errorf("failed to setup spoof-check: %v", err)
}
sc.rulestore = rulestore

return nil
}

func (sc *SpoofChecker) findPreroutingRule(ruleToFind *schema.Rule) ([]*schema.Rule, error) {
ruleset := sc.rulestore
if ruleset == nil {
chain, err := sc.configurer.Read(listChainBridgeNatPrerouting()...)
if err != nil {
return nil, err
}
ruleset = chain
}
return ruleset.LookupRule(ruleToFind), nil
}

// Teardown removes the interface and mac-address specific chains and their rules.
// The table and base-chain are expected to survive while the base-chain rule that matches the
// interface is removed.
func (sc *SpoofChecker) Teardown() error {
ifaceChain := sc.ifaceChain()
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
// additional default entries (e.g. counters).
ruleToFindExcludingStatements := *expectedRuleToFind
ruleToFindExcludingStatements.Expr = nil
rules := currentConfig.LookupRule(&ruleToFindExcludingStatements)
if len(rules) > 0 {
c := nft.NewConfig()
for _, rule := range rules {
c.DeleteRule(rule)
}
if err := sc.configurer.Apply(c); err != nil {
ifaceMatchRuleErr = fmt.Errorf("failed to delete iface match rule: %v", err)
}
} else {
fmt.Fprintf(os.Stderr, "spoofcheck/teardown: unable to detect iface match rule for deletion: %+v", expectedRuleToFind)
expectedRuleToFind := sc.matchIfaceJumpToChainRule(preRoutingBaseChainName, ifaceChain.Name)
// It is safer to exclude the statement matching, avoiding cases where a current statement includes
// additional default entries (e.g. counters).
ruleToFindExcludingStatements := *expectedRuleToFind
ruleToFindExcludingStatements.Expr = nil

rules, ifaceMatchRuleErr := sc.findPreroutingRule(&ruleToFindExcludingStatements)
if ifaceMatchRuleErr == nil && len(rules) > 0 {
c := nft.NewConfig()
for _, rule := range rules {
c.DeleteRule(rule)
}
if _, err := sc.configurer.Apply(c); err != nil {
ifaceMatchRuleErr = fmt.Errorf("failed to delete iface match rule: %v", err)
}
// Drop the cache, it should contain deleted rule(s) now
sc.rulestore = nil
} else {
fmt.Fprintf(os.Stderr, "spoofcheck/teardown: unable to detect iface match rule for deletion: %+v", expectedRuleToFind)
}

regularChainsConfig := nft.NewConfig()
regularChainsConfig.DeleteChain(ifaceChain)
regularChainsConfig.DeleteChain(sc.macChain(ifaceChain.Name))

var regularChainsErr error
if err := sc.configurer.Apply(regularChainsConfig); err != nil {
if _, err := sc.configurer.Apply(regularChainsConfig); err != nil {
regularChainsErr = fmt.Errorf("failed to delete regular chains: %v", err)
}

Expand Down
34 changes: 30 additions & 4 deletions pkg/link/spoofcheck_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,25 @@ var _ = Describe("spoofcheck", func() {
)))
})
})

Context("echo", func() {
It("succeeds, no read called", func() {
c := configurerStub{}
sc := link.NewSpoofCheckerWithConfigurer(iface, mac, id, &c)
Expect(sc.Setup()).To(Succeed())
Expect(sc.Teardown()).To(Succeed())
Expect(c.readCalled).To(BeFalse())
})

It("succeeds, fall back to config read", func() {
c := configurerStub{applyReturnNil: true}
sc := link.NewSpoofCheckerWithConfigurer(iface, mac, id, &c)
Expect(sc.Setup()).To(Succeed())
c.readConfig = c.applyConfig[0]
Expect(sc.Teardown()).To(Succeed())
Expect(c.readCalled).To(BeTrue())
})
})
})

func assertExpectedRegularChainsDeletionInTeardownConfig(action configurerStub) {
Expand Down Expand Up @@ -274,21 +293,28 @@ type configurerStub struct {
failFirstApplyConfig bool
failSecondApplyConfig bool
failReadConfig bool

applyReturnNil bool
readCalled bool
}

func (a *configurerStub) Apply(c *nft.Config) error {
func (a *configurerStub) Apply(c *nft.Config) (*nft.Config, error) {
a.applyCounter++
if a.failFirstApplyConfig && a.applyCounter == 1 {
return fmt.Errorf(errorFirstApplyText)
return nil, fmt.Errorf(errorFirstApplyText)
}
if a.failSecondApplyConfig && a.applyCounter == 2 {
return fmt.Errorf(errorSecondApplyText)
return nil, fmt.Errorf(errorSecondApplyText)
}
a.applyConfig = append(a.applyConfig, c)
return nil
if a.applyReturnNil {
return nil, nil
}
return c, nil
}

func (a *configurerStub) Read(_ ...string) (*nft.Config, error) {
a.readCalled = true
if a.failReadConfig {
return nil, fmt.Errorf(errorReadText)
}
Expand Down
7 changes: 7 additions & 0 deletions vendor/github.com/networkplumbing/go-nft/nft/config.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

50 changes: 32 additions & 18 deletions vendor/github.com/networkplumbing/go-nft/nft/exec/exec.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion vendor/modules.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.3.0
# github.com/networkplumbing/go-nft v0.4.0
## explicit; go 1.16
github.com/networkplumbing/go-nft/nft
github.com/networkplumbing/go-nft/nft/config
Expand Down