Improves the error logs during the bpf maps updating

[elfadel]

Fixes: #15864

[joestringer]

I appreciate the effort to improve the errors in Cilium, as some of them a can be quite confusing. However, I noticed several issues with this PR as-is:

• Please use a spellchecker.
• Syscall library is deprecated
• ENOENT message looks incorrect
• E2BIG is assuming LB map but this is a generic function, it doesn't make sense to add such specific instructions in this generic code.
• EINVAL, EPERM, ENOMEM adds no new information vs. the canonical string for that error
• The default looks like it will hide the real error, which is worse than having a confusing error.

The current code also embeds the underlying error as a string, but it should wrap the error instead in all new code (%w).

[elfadel]

@joestringer thank you for your review.

• Please use a spellchecker.

Oki.

• Syscall library is deprecated

Which library should we use therefore?

• ENOENT message looks incorrect

I don't think so. Here I make the assumption that the user is trying to update an element that doesn't exist in the map. No?

• E2BIG is assuming LB map but this is a generic function, it doesn't make sense to add such specific instructions in this generic code.

I am aware of that. That is why I added a condition to the specific instruction. Unless we have a way to retrieve the map name from it ID. Do we?

• EINVAL, EPERM, ENOMEM adds no new information vs. the canonical string for that error

I will keep the canonical message in these cases.

• The default looks like it will hide the real error, which is worse than having a confusing error.

Here too, I will keep the canonical message.

[joestringer]

ENOENT message looks incorrect

I don't think so. Here I make the assumption that the user is trying to update an element that doesn't exist in the map. No?

The code says:

case syscall.ENOENT:
			errMsg = "Cannot add/update element to bpf map, the element already exist"

"the element already exist" is not the same as "the element doesn't exist".

Incidentally also, this error can only happen if the caller passes flags=BPF_EXIST which is never the case - all callers in the codebase pass BPF_ANY (see linux/bpf.h from the kernel tree for more details).

E2BIG is assuming LB map but this is a generic function, it doesn't make sense to add such specific instructions in this generic code.

I am aware of that. That is why I added a condition to the specific instruction. Unless we have a way to retrieve the map name from it ID. Do we?

Maybe not today. Depending on how comprehensively you want to solve this problem for various map types, you could either add code higher up the stack where the code is specific to LB map (eg by unwrapping the error and checking whether it matches E2BIG), or try to build out such a mapping layer (though I don't have a good idea about how that would look right now, you'd probably have to experiment a bit).

[elfadel]

@joestringer

I finally chose to pass the map's name as an argument to updateElement() function. And performing a lookup of "lb" substring in the map's name.

Potential limit: I make the assumption that the LB bpf map ALWAYS contains "lb" substring in its name.

[kkourt]

Thanks for the patch! Please find some comments below.

[elfadel]

@kkourt thanks for your review. The PR is updated with your suggestion.

[kkourt]

Thanks! LGTM, assuming all the other concerns are resolved.

[brb]

Thanks!

[aanm]

test-me-please

[aanm]

@elfadel it seems like the unit tests are failing:

 	 # github.com/cilium/cilium/pkg/maps/ctmap [github.com/cilium/cilium/pkg/maps/ctmap.test]
	 pkg/maps/ctmap/ctmap_privileged_test.go:87:27: not enough arguments in call to bpf.UpdateElement
	 	have (int, unsafe.Pointer, unsafe.Pointer, number)
	 	want (int, string, unsafe.Pointer, unsafe.Pointer, uint64)
	 pkg/maps/ctmap/ctmap_privileged_test.go:150:25: not enough arguments in call to bpf.UpdateElement
	 	have (int, unsafe.Pointer, unsafe.Pointer, number)
	 	want (int, string, unsafe.Pointer, unsafe.Pointer, uint64)
	 pkg/maps/ctmap/ctmap_privileged_test.go:172:25: not enough arguments in call to bpf.UpdateElement
	 	have (int, unsafe.Pointer, unsafe.Pointer, number)
	 	want (int, string, unsafe.Pointer, unsafe.Pointer, uint64)
	 pkg/maps/ctmap/ctmap_privileged_test.go:193:25: not enough arguments in call to bpf.UpdateElement
	 	have (int, unsafe.Pointer, unsafe.Pointer, number)
	 	want (int, string, unsafe.Pointer, unsafe.Pointer, uint64)
	 pkg/maps/ctmap/ctmap_privileged_test.go:286:25: not enough arguments in call to bpf.UpdateElement
	 	have (int, unsafe.Pointer, unsafe.Pointer, number)
	 	want (int, string, unsafe.Pointer, unsafe.Pointer, uint64)
	 pkg/maps/ctmap/ctmap_privileged_test.go:308:25: not enough arguments in call to bpf.UpdateElement
	 	have (int, unsafe.Pointer, unsafe.Pointer, number)
	 	want (int, string, unsafe.Pointer, unsafe.Pointer, uint64)
	 pkg/maps/ctmap/ctmap_privileged_test.go:329:25: not enough arguments in call to bpf.UpdateElement
	 	have (int, unsafe.Pointer, unsafe.Pointer, number)
	 	want (int, string, unsafe.Pointer, unsafe.Pointer, uint64)
	 pkg/maps/ctmap/ctmap_privileged_test.go:357:25: not enough arguments in call to bpf.UpdateElement
	 	have (int, unsafe.Pointer, unsafe.Pointer, number)
	 	want (int, string, unsafe.Pointer, unsafe.Pointer, uint64)
	 pkg/maps/ctmap/ctmap_privileged_test.go:412:25: not enough arguments in call to bpf.UpdateElement
	 	have (int, unsafe.Pointer, unsafe.Pointer, number)
	 	want (int, string, unsafe.Pointer, unsafe.Pointer, uint64)
	 make: *** [tests-privileged] Error 1

[elfadel]

@aanm oki. I will fix it as soon as possible and make a new commit

[aanm]

test-me-please

[joestringer]

@elfadel could you please rebase against the latest master? The base commit for this branch is quite old, and this may hint at why the CI is so unhappy with the PR right now.

[elfadel]

@joestringer Oki.

[joestringer]

Sorry, there was another change merged into the tree (#16631) that requires all PRs to be rebased once more, otherwise some of the CI jobs will fail. One more time? Then we can run the tests again.

[elfadel]

Done !

[joestringer]

test-me-please

[joestringer]

@elfadel When I click through to the commit of your PR (https://github.com/cilium/cilium/commits/e335addcc806d01362fcebe951f6217cd03cf64d), it shows as being based still on old code. Can you try pulling in the latest cilium/cilium repo code again and rebasing on top of master?

[elfadel]

Oki @joestringer, I will proceed like that and make a new commit as soon as possible

[joestringer]

test-me-please

[aanm]

Marking for backport since this will help users debug issues more easily.