Auto detect container runtime socket path

[dorser]

This is a new version of the stale #2448.

The socket path of the container runtime can vary between different distributions. In order to find the socket path automatically, we query the kubelet configuration from the API Server.

Resolves #2447

Additionally, this PR also removes runtimes if their socket path doesn't exist in the IG CLI

How to use
Use container-collection with WithInitialKubernetesContainers or WithhPodInformer options.

Testing done

• Executed ig-integration.test on a K3s deployment
• Used the example exec tracer program WithInitialKubernetesContainers on a K3s deployment
• GitHub CI tests passing: https://github.com/dorser/inspektor-gadget/actions/runs/8913730765

[eiffel-fl]

Hi!

This look good, but I would need to take a deeper look to have a better understanding of the implications of this change.

Best regards.

[eiffel-fl]

Hi!

Can you please rebase, it seems go.mod was modified meanwhile.

Best regards.

[eiffel-fl]

Hi!

I just have two questions but this is looking good!
I will run the CI.

Best regards.

[eiffel-fl]

The CI complains, can you please run go mod tidy?

[dorser]

I always make sure to run go mod tidy :)
I fixed the lint issue. Not sure why the CRI-O test fails.

[eiffel-fl]

I always make sure to run go mod tidy :) I fixed the lint issue. Not sure why the CRI-O test fails.

I launched the CI again, let's see if everything with regard to linting is fixed.
For CRI-O, this is just flakyness.

[eiffel-fl]

Hi!

I have some questions on the code itself, otherwise it looks good to me.
I would nonetheless like to have another review from someone else.

Best regards.

[eiffel-fl]

Hi!

I have some comments on the new version.

Best regards.

[eiffel-fl]

Shouldn't it return the path obtained from the env variable?

[dorser]

We could, but we do it when we create the runtime client:
https://github.com/inspektor-gadget/inspektor-gadget/blob/main/pkg/container-utils/containerutils.go#L54

[eiffel-fl]

I am then wondering if we should not move all this code in NewContainerRuntimeClient(), particularly as with this commit we do the check for environment variables twice.

[dorser]

I thought about it initially. The detection of the socket path is specific to k8s (Using the /configz endpoint in k8s). It is done using the k8s client which later creates a runtime client. The reasoning I added the env variables here is to make sure the env variables override the auto-detection if present. Do you still think it is better moving it to NewContainerRuntimeClient()?

[eiffel-fl]

I would not go toward moving the code, as I think it is better to keep things separated.
Nonetheless, we should define what is the behavior we are aiming before.
Before your patchset, environment variables were used if the socket path was empty.
With your patch, we are basically giving the priority to environment variable, which is a modification of the current behavior.

[eiffel-fl]

Let's stick to what we have now, by merging this one right as it is we enable socket path to be automatically detected for API usage, which is already a great addition.
We can add this for the CLI in a follow-up PR.
We should nonetheless make things clear than, for now, this only targets the API.

[eiffel-fl]

With regard to the priority of environment variables, we already have a situation where we try to autodetect and only use the environment variable if the detection failed:

inspektor-gadget/pkg/runcfanotify/runcfanotify.go

Line 214 in 370de19

return nil, fmt.Errorf("no runc instance can be monitored with fanotify. The following paths were tested: %s. You can use the RUNC_PATH env variable to specify a custom path. If you are successful doing so, please open a PR to add your custom path to runcPaths", strings.Join(runcPaths, ","))

inspektor-gadget/pkg/container-hook/tracer.go

Line 321 in 370de19

return fmt.Errorf("no container runtime can be monitored with fanotify. The following paths were tested: %s. You can use the RUNTIME_PATH env variable to specify a custom path. If you are successful doing so, please open a PR to add your custom path to runtimePaths", strings.Join(runtimePaths, ","))

This could make sense for really specific runtime or k8s distribution where the autodetection failed.

[mauriciovasquezbernal]

As a user, I would think it's "weird" that I "configured" IG to use a socket path and it ends up ignoring it.

We added the possibility to configure them because there was not an auto-detection mechanism in place, but now that we have this, I'd say the autodetected path should take precedence, in any case, if we're able to detect it, it's the right one to use, isn't it?

(We can discuss all of this in a follow PR if you prefer)

As for removing the values, do you want me to remove them as part of this PR or in a follow-up PR as @eiffel-fl suggested? I think a separate PR makes sense.

For sure we can handle this in another PR, but it's a bit weird to me that most of users (the ones that use the template to deploy it) won't benefit from this change after this PR is merged.

[dorser]

Since the config endpoint isn't officially documented, I cannot guarantee its accuracy. It seems to be working properly in all environments I personally tested (k3s, minikube, microK8s and AKS) and given this endpoint has been around for a while, as discussed in this and the previous stale PR, I think it should be safe to use.

I'll remove the values from the templates as part of this PR.

[dorser]

OK. I think I'm convinced the auto-detect should take precedence over the environment variables. I'll remove that part.

[mauriciovasquezbernal]

Some initial comments. I'm really wondering about the priority of env variables.

[mauriciovasquezbernal]

Do we really need to make the env variable have the highest precedence? If we get the path from Kubernetes, can't we be 100% sure it's the right one to use? Perhaps we can print a warning in that case indicating we're using the one we got from Kubernetes.

Saying this carefully, but I think we can remove these hard-coded values as we fallback to these values anyway in the runtime client code: https://github.com/inspektor-gadget/inspektor-gadget/blob/main/pkg/container-utils/runtime-client/interface.go#L25

I agree. With this autodetection mechanism in place, defining those won't be that important anymore.

[mauriciovasquezbernal]

I'm wondering how you tested it. I tried to make it work with K8s and have to apply the following. I suppose it worked for you because you only tried ig, right?

diff --git a/pkg/container-collection/k8s.go b/pkg/container-collection/k8s.go
index 7bde2accc..979611d0a 100644
--- a/pkg/container-collection/k8s.go
+++ b/pkg/container-collection/k8s.go
@@ -20,6 +20,7 @@ import (
        "fmt"
        "math"
        "os"
+       "path"
        "strings"

        log "github.com/sirupsen/logrus"
@@ -37,6 +38,7 @@ import (
        runtimeclient "github.com/inspektor-gadget/inspektor-gadget/pkg/container-utils/runtime-client"
        containerutilsTypes "github.com/inspektor-gadget/inspektor-gadget/pkg/container-utils/types"
        "github.com/inspektor-gadget/inspektor-gadget/pkg/types"
+       "github.com/inspektor-gadget/inspektor-gadget/pkg/utils/host"
 )

 type K8sClient struct {
@@ -73,8 +75,9 @@ func NewK8sClient(nodeName string) (*K8sClient, error) {
        list := strings.SplitN(node.Status.NodeInfo.ContainerRuntimeVersion, "://", 2)
        runtimeClient, err := containerutils.NewContainerRuntimeClient(
                &containerutilsTypes.RuntimeConfig{
-                       Name:       types.String2RuntimeName(list[0]),
-                       SocketPath: socketPath,
+                       Name:            types.String2RuntimeName(list[0]),
+                       SocketPath:      socketPath,
+                       RuntimeProtocol: containerutilsTypes.RuntimeProtocolCRI,
                })
        if err != nil {
                return nil, err
@@ -216,6 +219,9 @@ func getContainerRuntimeSocketPath(clientset *kubernetes.Clientset, nodeName str
        if !found {
                return "", fmt.Errorf("socket path does not start with unix://")
        }
+
+       socketPath = path.Join(host.HostRoot, socketPath)
+
        log.Infof("using the detected container runtime socket path from Kubelet's config: %s", socketPath)
        return socketPath, nil
 }
@@ -225,7 +231,6 @@ func getContainerRuntimeSocketPath(clientset *kubernetes.Clientset, nodeName str
 func getCurrentKubeletConfig(clientset *kubernetes.Clientset, nodeName string) (*kubeletconfig.KubeletConfiguration, error) {
        resp, err := clientset.CoreV1().RESTClient().Get().Resource("nodes").
                Name(nodeName).Suffix("proxy", "configz").DoRaw(context.TODO())
-
        if err != nil {
                return nil, fmt.Errorf("fetching /configz from %q: %w", nodeName, err)
        }
diff --git a/pkg/container-utils/docker/docker.go b/pkg/container-utils/docker/docker.go
index 9611cff58..0f8faaae0 100644
--- a/pkg/container-utils/docker/docker.go
+++ b/pkg/container-utils/docker/docker.go
@@ -56,7 +56,9 @@ func NewDockerClient(socketPath string, protocol string) (runtimeclient.Containe

        case containerutilsTypes.RuntimeProtocolCRI:
                // TODO: Configurable
-               socketPath = runtimeclient.CriDockerDefaultSocketPath
+               if socketPath == "" {
+                       socketPath = runtimeclient.CriDockerDefaultSocketPath
+               }
                return cri.NewCRIClient(types.RuntimeNameDocker, socketPath, DefaultTimeout)

        default:
diff --git a/pkg/operators/localmanager/localmanager.go b/pkg/operators/localmanager/localmanager.go
index 5a2c6457a..9486db952 100644
--- a/pkg/operators/localmanager/localmanager.go
+++ b/pkg/operators/localmanager/localmanager.go
@@ -24,6 +24,8 @@ import (
        "github.com/containerd/containerd/pkg/cri/constants"
        securejoin "github.com/cyphar/filepath-securejoin"
        "github.com/google/uuid"
+       log "github.com/sirupsen/logrus"
+
        commonutils "github.com/inspektor-gadget/inspektor-gadget/cmd/common/utils"
        containercollection "github.com/inspektor-gadget/inspektor-gadget/pkg/container-collection"
        containerutils "github.com/inspektor-gadget/inspektor-gadget/pkg/container-utils"
@@ -39,7 +41,6 @@ import (
        "github.com/inspektor-gadget/inspektor-gadget/pkg/params"
        "github.com/inspektor-gadget/inspektor-gadget/pkg/types"
        "github.com/inspektor-gadget/inspektor-gadget/pkg/utils/host"
-       log "github.com/sirupsen/logrus"
 )

 const (
diff --git a/pkg/resources/manifests/deploy.yaml b/pkg/resources/manifests/deploy.yaml
index e6530aa2b..660f9436a 100644
--- a/pkg/resources/manifests/deploy.yaml
+++ b/pkg/resources/manifests/deploy.yaml
@@ -165,12 +165,12 @@ spec:
             - name: INSPEKTOR_GADGET_OPTION_FALLBACK_POD_INFORMER
               value: "true"
             # Make sure to keep these settings in sync with pkg/container-utils/runtime-client/interface.go
-            - name: INSPEKTOR_GADGET_CONTAINERD_SOCKETPATH
-              value: "/run/containerd/containerd.sock"
-            - name: INSPEKTOR_GADGET_CRIO_SOCKETPATH
-              value: "/run/crio/crio.sock"
-            - name: INSPEKTOR_GADGET_DOCKER_SOCKETPATH
-              value: "/run/docker.sock"
+            #- name: INSPEKTOR_GADGET_CONTAINERD_SOCKETPATH
+            #  value: "/run/containerd/containerd.sock"
+            #- name: INSPEKTOR_GADGET_CRIO_SOCKETPATH
+            #  value: "/run/crio/crio.sock"
+            #- name: INSPEKTOR_GADGET_DOCKER_SOCKETPATH
+            #  value: "/run/docker.sock"
             - name: HOST_ROOT
               value: "/host"
             - name: IG_EXPERIMENTAL

[dorser]

I'm wondering how you tested it. I tried to make it work with K8s and have to apply the following. I suppose it worked for you because you only tried ig, right?

I tested it using IG as a library. I'll look into this now. I assume you tested it using make minikube-deploy and running your tests, right?

[mauriciovasquezbernal]

I suppose make minikube-deploy will work fine. I actually do this manually:

$ minikube start 

# compile IG and push to my container registry
$ export CONTAINER_REPO=ghcr.io/mauriciovasquezbernal/inspektor-gadget
$ make gadget-container && make push-gadget-container && make kubectl-gadget

# Deploy to the cluster ( --daemon-log-level can be used to enabled debug messages)
$  ./kubectl-gadget deploy

# check the gadget logs...

[mauriciovasquezbernal]

Two minor nits, other than it LGTM. Please rebase to take #2914, so we can have the CI passsing.

Thanks a lot for this contribution.

[dorser]

Fixed the nits and rebased.

Thanks for the review @eiffel-fl and @mauriciovasquezbernal ❤️

[mauriciovasquezbernal]

There are still some issues going with with golang modules, would you mind doing cd examples && go mod tidy, committing, squashing and pushing that again to fix the CI?