init.sh: move socketlb creation into own pkg

[rgo3]

Description taken from the main commit:

With this PR we translate code from bpf/init.sh that compiled and
loaded bpf code for the socketlb feature to a pure go implementation.

The main difference to the init.sh code is that now on a fresh install
of cilium, cilium will leverage bpf links to attach programs to cgroups
if bpf_link is available in the kernel.

Test cases focus on testing scenarios that occur on clean agent
startup, restart and upgrade for both attaching and detaching bpf
programs to/from cgroups.

Fixes: #20739

[ti-mo]

/test

[rgo3]

/test

[ti-mo]

/test

[ti-mo]

/test

[ti-mo]

/test

[ti-mo]

/test

[rgo3]

/test

[ti-mo]

/test

[ti-mo]

/test-1.24-5.4

[ti-mo]

/test-1.25-4.19

[ti-mo]

/test-1.26-net-next

[joestringer]

The logic looks much simpler and more reliable now 👍 (I didn't look super close at the details)

[aditighag]

Thanks for the revisions and detailed comments: the code logic is easier to follow now, and adopting bpf_link approach only on fresh kernels looks like a sound approach.

I was looking at the ebpf library code, and I found a couple of instances of panic. They may or may not directly be in the link or PROG_ATTACH logic, but this could change. Can we recover from panic so that we don't end up crashing the agent?

While the code migration to Go code provides visibility and better error reporting, I've had to resort to cleaning things up (re: bpf_clear_cgroups) out of band a few times in the past. Any thoughts on providing back-up code that could be easily run as an independent binary? Could it be as simple as removing the pinned link for the new approach? This isn't necessarily a blocking comment for this PR, but it would be worthwhile to be considered as a follow-up.

[ti-mo]

I was looking at the ebpf library code, and I found a couple of instances of panic. They may or may not directly be in the link or PROG_ATTACH logic, but this could change. Can we recover from panic so that we don't end up crashing the agent?

I know you're just being cautious, but this feels nitpicky. panic is used very sparingly outside of tests and init(), and the caller is not expected to have to recover(). If there are particular cases that worry you, I'd suggest we improve this in the library, not sprinkle recover()s in the agent. If anything, this is something hive could do when launching modules. (cc @joamaki, @lmb)

The only code path that could potentially panic in a real-life scenario is the one in maskProfilerSignal(). We should probably make that return an error, but it's highly unexpected for Pthreadsigmask() to fail. The panic in unmaskProfilerSignal() is also extremely unlikely, though necessary. We need to restore per-thread state to be able to safely call UnlockOSThread(); failure to do so is unsafe.

While the code migration to Go code provides visibility and better error reporting, I've had to resort to cleaning things up (re: bpf_clear_cgroups) out of band a few times in the past. Any thoughts on providing back-up code that could be easily run as an independent binary?

I think that's just part of development, but that doesn't mean we can't maintain tooling to help. With the various ways of attaching programs, this may come in handy in the future.

Could it be as simple as removing the pinned link for the new approach?

Yep! Simply deleting the bpffs pin does the trick, as long as nothing else holds an open fd to the link.

This isn't necessarily a blocking comment for this PR, but it would be worthwhile to be considered as a follow-up.

Yes, let's not increase the scope of this PR and delay it further. Would this fit in cilium cleanup? I think this is part of a wider topic, but please drop an issue with the behaviour you'd like to see so the foundations team can potentially pick this up.

[lmb]

panic is used very sparingly outside of tests and init(), and the caller is not expected to have to recover(). If there are particular cases that worry you, I'd suggest we improve this in the library, not sprinkle recover()s in the agent.

Agreed, users of the lib are never meant to need to recover.

[aditighag]

Barring couple of follow-ups, LGTM. Nice work!

While the code migration to Go code provides visibility and better error reporting, I've had to resort to cleaning things up (re: bpf_clear_cgroups) out of band a few times in the past. Any thoughts on providing back-up code that could be easily run as an independent binary?

I think that's just part of development, but that doesn't mean we can't maintain tooling to help. With the various ways of attaching programs, this may come in handy in the future.

Could it be as simple as removing the pinned link for the new approach?

Yep! Simply deleting the bpffs pin does the trick, as long as nothing else holds an open fd to the link.

It'll be good to document this.

This isn't necessarily a blocking comment for this PR, but it would be worthwhile to be considered as a follow-up.

Yes, let's not increase the scope of this PR and delay it further. Would this fit in cilium cleanup? I think this is part of a wider topic, but please drop an issue with the behaviour you'd like to see so the foundations team can potentially pick this up.

Here it is - #24585.

[rolinh]

Vendor changes lgtm

[ti-mo]

/test

[ti-mo]

/test

[ti-mo]

Had to add some extra code that removes defunct links. Link.Update() fails with ENOLINK if the link's cgroup went away. This happens in the l4lb tests, since they run under dind, and the agent attaches to the container's cgroup, not the system's root. On every test step, the agent's container (and cgroup) is blown away, but the link persists on the shared /sys/fs/bpf. Something to keep in mind. @brb

[ti-mo]

test-runtime hit #24653, all other required tests are green.

[ti-mo]

/test

Job 'Cilium-PR-K8s-1.24-kernel-5.4' failed:

Click to show.

Test Name

K8sDatapathConfig Host firewall With native routing

Failure Output

FAIL: Error deleting resource /home/jenkins/workspace/Cilium-PR-K8s-1.24-kernel-5.4/src/github.com/cilium/cilium/test/k8s/manifests/host-policies.yaml: Cannot retrieve "cilium-zhkz7"'s policy revision: cannot get policy revision: ""

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.24-kernel-5.4 so I can create one.

[ti-mo]

/test-1.24-5.4

[ti-mo]

/test-1.26-net-next