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

SirPhuttel · 2023-06-02T14:27:29Z

Store the relevant applied config part for later to extract the rule to delete from there instead of having to list the ruleset. This is much faster especially with large rulesets.

SirPhuttel · 2023-06-02T14:30:25Z

@EdDev here we are, could you please review the commit?

EdDev

Looks very nice, thank you!

I have added a few points inline.

I noticed the new logic is not covered by unit tests, which may be a bit tricky now. I think we can improve on that in a follow up.

go.mod

pkg/link/spoofcheck.go

SirPhuttel · 2023-06-13T08:57:55Z

Next round: I introduced SpoofChecker::findPreroutingRule() which is a wrapper around the LookupRule() method, taking care of gathering the relevant ruleset snippet if not cached already.

Cache dropping happens in Teardown() which knows if it is no longer valid or not.

squeed · 2023-06-26T15:48:52Z

pkg/link/spoofcheck.go

-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


Why does this need a separate timeout?

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?

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.

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.

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.

SirPhuttel · 2023-06-27T19:59:08Z

Two updates:

Dropped a leftover line from the commit message, no functional changes.
Extended spoofcheck_test.go to cover the fallback to Read() if Apply() should not return anything.

@EdDev do you think the testsuite part is sufficient now? Any idea what to test for in addition?

EdDev · 2023-06-28T06:47:23Z

Two updates:
* Dropped a leftover line from the commit message, no functional changes.

* Extended spoofcheck_test.go to cover the fallback to Read() if Apply() should not return anything.
@EdDev do you think the testsuite part is sufficient now? Any idea what to test for in addition?

I am good with the current change, thank you.

I need now to release go-nft to add the new API in a formal release.
There may be another change I want to include, so I will update soon about it.

EdDev · 2023-06-28T12:37:30Z

I need now to release go-nft to add the new API in a formal release.
There may be another change I want to include, so I will update soon about it.

@SirPhuttel , new go-nft release is out: https://github.com/networkplumbing/go-nft/releases/tag/v0.4.0
I suggest you update go.mog and sync the vendoring to use it (instead of the one pointing to a specific commit).

pkg/link/spoofcheck_test.go

squeed

Looks good, just two things

a few minor lint issues
can you pick up the latest go-nft release?

Thanks

EdDev

Thank you very much!

s1061123

/lgtm

squeed · 2023-07-03T15:08:28Z

@SirPhuttel we merged some dep updates, can you rebase? Then we'll merge this.

SirPhuttel · 2023-07-04T11:34:54Z

@SirPhuttel we merged some dep updates, can you rebase? Then we'll merge this.

DONE! Applied with just a context conflict in go.mod.

SirPhuttel · 2023-07-04T12:51:19Z

And a (final?) whitespace fix to make the linter happy.

SirPhuttel · 2023-07-18T11:20:55Z

@squeed Ping! This is stuck somehow. Could you please approve the workflow so we see if it passes CI? Thanks!

squeed · 2023-07-19T19:58:09Z

Hmm, the tests are failing for no clear reason. I'm taking a look.

squeed · 2023-07-19T20:58:10Z

tests are broken; I'm investigating. Huh

squeed · 2023-07-20T09:27:13Z

@SirPhuttel can you rebase on main? There was an issue with CI, sorry.

Store the relevant applied config part for later to extract the rule to delete from there instead of having to list the ruleset. This is much faster especially with large rulesets. Signed-off-by: Phil Sutter <psutter@redhat.com>

SirPhuttel · 2023-07-20T09:33:05Z

@squeed DONE, thanks for investigating.

EdDev reviewed Jun 5, 2023

View reviewed changes

go.mod Outdated Show resolved Hide resolved

pkg/link/spoofcheck.go Outdated Show resolved Hide resolved

pkg/link/spoofcheck.go Outdated Show resolved Hide resolved

SirPhuttel force-pushed the applyconfigecho branch from e759696 to 6c8f8c8 Compare June 13, 2023 08:51

squeed reviewed Jun 26, 2023

View reviewed changes

SirPhuttel force-pushed the applyconfigecho branch 2 times, most recently from 3bd5d12 to 3f48434 Compare June 27, 2023 19:52

EdDev mentioned this pull request Jun 28, 2023

Revert "exec: Use a temp file to pass data to the nft binary" networkplumbing/go-nft#51

Merged

squeed reviewed Jun 28, 2023

View reviewed changes

pkg/link/spoofcheck_test.go Outdated Show resolved Hide resolved

squeed approved these changes Jun 28, 2023

View reviewed changes

SirPhuttel force-pushed the applyconfigecho branch 3 times, most recently from 2674966 to b59274e Compare June 28, 2023 16:08

EdDev approved these changes Jun 28, 2023

View reviewed changes

s1061123 mentioned this pull request Jul 3, 2023

build(deps): bump github.com/networkplumbing/go-nft from 0.3.0 to 0.4.0 #920

Closed

s1061123 reviewed Jul 3, 2023

View reviewed changes

SirPhuttel force-pushed the applyconfigecho branch from b59274e to 6cc3eca Compare July 4, 2023 11:34

SirPhuttel force-pushed the applyconfigecho branch from 6cc3eca to be4c52f Compare July 4, 2023 12:51

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

2ba7f16

Store the relevant applied config part for later to extract the rule to delete from there instead of having to list the ruleset. This is much faster especially with large rulesets. Signed-off-by: Phil Sutter <psutter@redhat.com>

SirPhuttel force-pushed the applyconfigecho branch from be4c52f to 2ba7f16 Compare July 20, 2023 09:32

squeed merged commit 438548a into containernetworking:main Jul 20, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

SirPhuttel commented Jun 2, 2023

SirPhuttel commented Jun 2, 2023

EdDev left a comment

SirPhuttel commented Jun 13, 2023

squeed Jun 26, 2023

SirPhuttel Jun 27, 2023

squeed Jun 28, 2023

SirPhuttel Jun 28, 2023

EdDev Jun 28, 2023

SirPhuttel commented Jun 27, 2023

EdDev commented Jun 28, 2023

EdDev commented Jun 28, 2023

squeed left a comment

EdDev left a comment

s1061123 left a comment

squeed commented Jul 3, 2023

SirPhuttel commented Jul 4, 2023

SirPhuttel commented Jul 4, 2023

SirPhuttel commented Jul 18, 2023

squeed commented Jul 19, 2023

squeed commented Jul 19, 2023

squeed commented Jul 20, 2023

SirPhuttel commented Jul 20, 2023

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

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

Conversation

SirPhuttel commented Jun 2, 2023

SirPhuttel commented Jun 2, 2023

EdDev left a comment

Choose a reason for hiding this comment

SirPhuttel commented Jun 13, 2023

squeed Jun 26, 2023

Choose a reason for hiding this comment

SirPhuttel Jun 27, 2023

Choose a reason for hiding this comment

squeed Jun 28, 2023

Choose a reason for hiding this comment

SirPhuttel Jun 28, 2023

Choose a reason for hiding this comment

EdDev Jun 28, 2023

Choose a reason for hiding this comment

SirPhuttel commented Jun 27, 2023

EdDev commented Jun 28, 2023

EdDev commented Jun 28, 2023

squeed left a comment

Choose a reason for hiding this comment

EdDev left a comment

Choose a reason for hiding this comment

s1061123 left a comment

Choose a reason for hiding this comment

squeed commented Jul 3, 2023

SirPhuttel commented Jul 4, 2023

SirPhuttel commented Jul 4, 2023

SirPhuttel commented Jul 18, 2023

squeed commented Jul 19, 2023

squeed commented Jul 19, 2023

squeed commented Jul 20, 2023

SirPhuttel commented Jul 20, 2023