Use pod UID instead of Name/Namespace

Use of Pod UID instead of Name/Namespace to get the podSandBoxId and NetNs with the CRI API. Using Pod/Namespace might not be unique if a pod is deleted/created, UID will be unique.
k8snetworkplumbingwg · Mar 24, 2024 · 0fe7156 · 0fe7156
1 parent 2cc7fb5
commit 0fe7156
Show file tree

Hide file tree

Showing 8 changed files with 50 additions and 57 deletions.
diff --git a/.golangci.yml b/.golangci.yml
@@ -6,8 +6,8 @@ linters-settings:
   dupl:
     threshold: 100
   funlen:
-    lines: 100
-    statements: 50
+    lines: 110
+    statements: 55
   gci:
     local-prefixes: github.com/maiqueb/multus-dynamic-networks-controller
   goconst:

diff --git a/pkg/controller/pod.go b/pkg/controller/pod.go
@@ -51,9 +51,9 @@ type DynamicAttachmentRequest struct {
 // ContainerRuntime interface
 type ContainerRuntime interface {
 	// NetworkNamespace returns the network namespace of the given pod.
-	NetworkNamespace(ctx context.Context, podName string, podNamespace string) (string, error)
+	NetworkNamespace(ctx context.Context, podUID string) (string, error)
 	// PodSandboxID returns the PodSandboxID of the given pod.
-	PodSandboxID(ctx context.Context, podName string, podNamespace string) (string, error)
+	PodSandboxID(ctx context.Context, podUID string) (string, error)
 }
 
 func (dar *DynamicAttachmentRequest) String() string {
@@ -219,13 +219,13 @@ func (pnc *PodNetworksController) processNextWorkItem() bool {
 	indexedNetworkSelectionElements := annotations.IndexNetworkSelectionElements(networkSelectionElements)
 	indexedNetworkStatus := annotations.IndexNetworkStatus(networkStatus)
 
-	netnsPath, err := pnc.containerRuntime.NetworkNamespace(ctx, podName, podNamespace)
+	netnsPath, err := pnc.containerRuntime.NetworkNamespace(ctx, string(pod.UID))
 	if err != nil {
 		klog.Errorf("failed to figure out the pod's network namespace: %v", err)
 		return true
 	}
 
-	podSandboxID, err := pnc.containerRuntime.PodSandboxID(ctx, podName, podNamespace)
+	podSandboxID, err := pnc.containerRuntime.PodSandboxID(ctx, string(pod.UID))
 	if err != nil {
 		klog.Errorf("failed to figure out the PodSandboxID: %v", err)
 		return true
@@ -276,10 +276,11 @@ func (pnc *PodNetworksController) processNextWorkItem() bool {
 	if len(attachmentsToRemove) > 0 {
 		var res []annotations.AttachmentResult
 		res, err = pnc.handleDynamicInterfaceRequest(&DynamicAttachmentRequest{
-			Pod:         pod,
-			Attachments: attachmentsToRemove,
-			Type:        remove,
-			PodNetNS:    netnsPath,
+			Pod:          pod,
+			Attachments:  attachmentsToRemove,
+			Type:         remove,
+			PodNetNS:     netnsPath,
+			PodSandboxID: podSandboxID,
 		})
 		results = append(results, res...)
 		if err != nil {

diff --git a/pkg/controller/pod_test.go b/pkg/controller/pod_test.go
@@ -17,6 +17,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
 	v1coreinformerfactory "k8s.io/client-go/informers"
 	k8sclient "k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/kubernetes/fake"
@@ -71,6 +72,7 @@ var _ = Describe("Dynamic Attachment controller", func() {
 				namespace   = "default"
 				networkName = "tiny-net"
 				podName     = "tiny-winy-pod"
+				podUID      = "abc-def"
 			)
 			cniArgs := &map[string]string{"foo": "bar"}
 			var (
@@ -91,7 +93,7 @@ var _ = Describe("Dynamic Attachment controller", func() {
 			}
 
 			BeforeEach(func() {
-				pod = podSpec(podName, namespace, networkName)
+				pod = podSpec(podName, namespace, podUID, networkName)
 				networkToAdd = fmt.Sprintf("%s-2", networkName)
 
 				var err error
@@ -212,7 +214,7 @@ var _ = Describe("Dynamic Attachment controller", func() {
 
 			When("an attachment is added to a host networked pod", func() {
 				BeforeEach(func() {
-					pod = hostNetworkedPodSpec(podName, namespace, networkName)
+					pod = hostNetworkedPodSpec(podName, namespace, podUID, networkName)
 				})
 
 				JustAfterEach(func() {
@@ -764,12 +766,13 @@ func dummyNetSpec(networkName string, cniVersion string) string {
     }`, cniVersion, networkName)
 }
 
-func podSpec(name string, namespace string, networks ...string) *corev1.Pod {
+func podSpec(name string, namespace string, uid string, networks ...string) *corev1.Pod {
 	return &corev1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:        name,
 			Namespace:   namespace,
 			Annotations: podNetworkConfig(networks...),
+			UID:         types.UID(uid),
 		},
 		Status: corev1.PodStatus{
 			ContainerStatuses: []corev1.ContainerStatus{
@@ -876,8 +879,8 @@ func ifaceStatusForDefaultNamespace(networkName, ifaceName, macAddress string) n
 	}
 }
 
-func hostNetworkedPodSpec(name string, namespace string, networks ...string) *corev1.Pod {
-	pod := podSpec(name, namespace, networks...)
+func hostNetworkedPodSpec(name string, namespace string, uid string, networks ...string) *corev1.Pod {
+	pod := podSpec(name, namespace, uid, networks...)
 	pod.Spec.HostNetwork = true
 	return pod
 }
diff --git a/pkg/cri/fake/criclient.go b/pkg/cri/fake/criclient.go
@@ -14,7 +14,7 @@ import (
 )
 
 type CrioClient struct {
-	cachePodSandboxID map[string]string // Key: podname.podnamespace, value: podSandboxID
+	cachePodSandboxID map[string]string // Key: podUID, value: podSandboxID
 	cacheNetNs        map[string]string // Key: podSandboxID, value: netns
 }
 
@@ -31,9 +31,9 @@ func NewFakeClient(opts ...ClientOpt) *CrioClient {
 	return client
 }
 
-func WithCachedContainer(podName string, podNamespace string, podSandboxID string, netnsPath string) ClientOpt {
+func WithCachedContainer(podUID, podSandboxID string, netnsPath string) ClientOpt {
 	return func(client *CrioClient) {
-		client.cachePodSandboxID[fmt.Sprintf("%s.%s", podName, podNamespace)] = podSandboxID
+		client.cachePodSandboxID[podUID] = podSandboxID
 		client.cacheNetNs[podSandboxID] = netnsPath
 	}
 }
@@ -95,18 +95,12 @@ func (cc CrioClient) ListPodSandbox(
 	res := &crioruntime.ListPodSandboxResponse{
 		Items: []*crioruntime.PodSandbox{},
 	}
-
-	podName, exists := listPodSandboxRequest.Filter.LabelSelector[types.KubernetesPodNameLabel]
-	if !exists {
-		return res, nil
-	}
-
-	podNamespace, exists := listPodSandboxRequest.Filter.LabelSelector[types.KubernetesPodNamespaceLabel]
+	podUID, exists := listPodSandboxRequest.Filter.LabelSelector[types.KubernetesPodUIDLabel]
 	if !exists {
 		return res, nil
 	}
 
-	id, exists := cc.cachePodSandboxID[fmt.Sprintf("%s.%s", podName, podNamespace)]
+	id, exists := cc.cachePodSandboxID[podUID]
 	if !exists {
 		return res, nil
 	}

diff --git a/pkg/cri/fake/runtime.go b/pkg/cri/fake/runtime.go
@@ -18,21 +18,21 @@ func NewFakeRuntime(pods ...v1.Pod) *Runtime {
 
 	for i := range pods {
 		hash := md5.Sum([]byte(pods[i].GetName())) // #nosec
-		runtimeCache[pods[i].GetName()] = hex.EncodeToString(hash[:])
+		runtimeCache[string(pods[i].GetUID())] = hex.EncodeToString(hash[:])
 	}
 	return &Runtime{cache: runtimeCache}
 }
 
-func (r *Runtime) NetworkNamespace(_ context.Context, podName string, podNamespace string) (string, error) {
-	if netnsName, wasFound := r.cache[podName]; wasFound {
+func (r *Runtime) NetworkNamespace(_ context.Context, podUID string) (string, error) {
+	if netnsName, wasFound := r.cache[podUID]; wasFound {
 		return netnsName, nil
 	}
-	return "", fmt.Errorf("could not find a network namespace for container: %s.%s", podName, podNamespace)
+	return "", fmt.Errorf("could not find a network namespace for container: %s", podUID)
 }
 
-func (r *Runtime) PodSandboxID(_ context.Context, podName string, podNamespace string) (string, error) {
-	if netnsName, wasFound := r.cache[podName]; wasFound {
+func (r *Runtime) PodSandboxID(_ context.Context, podUID string) (string, error) {
+	if netnsName, wasFound := r.cache[podUID]; wasFound {
 		return netnsName, nil
 	}
-	return "", fmt.Errorf("could not find a PodSandboxID for pod: %s.%s", podName, podNamespace)
+	return "", fmt.Errorf("could not find a PodSandboxID for pod: %s", podUID)
 }
diff --git a/pkg/cri/runtime.go b/pkg/cri/runtime.go
@@ -34,23 +34,24 @@ func NewRuntime(socketPath string, timeout time.Duration) (*Runtime, error) {
 	}, nil
 }
 
-func (r *Runtime) NetworkNamespace(ctx context.Context, podName string, podNamespace string) (string, error) {
-	podSandboxId, err := r.PodSandboxID(ctx, podName, podNamespace)
+func (r *Runtime) NetworkNamespace(ctx context.Context, podUID string) (string, error) {
+	podSandboxID, err := r.PodSandboxID(ctx, podUID)
 	if err != nil {
 		return "", err
 	}
 
 	podSandboxStatus, err := r.Client.PodSandboxStatus(ctx, &cri.PodSandboxStatusRequest{
-		PodSandboxId: podSandboxId,
+		PodSandboxId: podSandboxID,
 		Verbose:      true,
 	})
 	if err != nil || podSandboxStatus == nil {
-		return "", fmt.Errorf("failed to PodSandboxStatus for PodSandboxId %s: %w", podSandboxId, err)
+		return "", fmt.Errorf("failed to PodSandboxStatus for PodSandboxId %s: %w", podSandboxID, err)
 	}
 
 	sandboxInfo := &PodSandboxStatusInfo{}
 
-	if err := json.Unmarshal([]byte(podSandboxStatus.Info[InfoKey]), sandboxInfo); err != nil {
+	err = json.Unmarshal([]byte(podSandboxStatus.Info[InfoKey]), sandboxInfo)
+	if err != nil {
 		return "", fmt.Errorf("failed to Unmarshal podSandboxStatus.Info['%s']: %w", InfoKey, err)
 	}
 
@@ -66,32 +67,31 @@ func (r *Runtime) NetworkNamespace(ctx context.Context, podName string, podNames
 	}
 
 	if networkNamespace == "" {
-		return "", fmt.Errorf("failed to find network namespace for PodSandboxId %s: %w", podSandboxId, err)
+		return "", fmt.Errorf("failed to find network namespace for PodSandboxId %s: %w", podSandboxID, err)
 	}
 
 	return networkNamespace, nil
 }
 
-func (r *Runtime) PodSandboxID(ctx context.Context, podName string, podNamespace string) (string, error) {
+func (r *Runtime) PodSandboxID(ctx context.Context, podUID string) (string, error) {
 	// Labels used by Kubernetes: https://github.com/kubernetes/kubernetes/blob/v1.29.2/staging/src/k8s.io/kubelet/pkg/types/labels.go#L19
 	ListPodSandboxRequest, err := r.Client.ListPodSandbox(ctx, &cri.ListPodSandboxRequest{
 		Filter: &cri.PodSandboxFilter{
 			LabelSelector: map[string]string{
-				types.KubernetesPodNameLabel:      podName,
-				types.KubernetesPodNamespaceLabel: podNamespace,
+				types.KubernetesPodUIDLabel: podUID,
 			},
 		},
 	})
 	if err != nil {
-		return "", fmt.Errorf("failed to ListPodSandbox for pod %s.%s: %w", podName, podNamespace, err)
+		return "", fmt.Errorf("failed to ListPodSandbox for pod %s: %w", podUID, err)
 	}
 
 	if ListPodSandboxRequest == nil || ListPodSandboxRequest.Items == nil || len(ListPodSandboxRequest.Items) == 0 {
-		return "", fmt.Errorf("ListPodSandbox returned 0 item for pod %s.%s: %w", podName, podNamespace, err)
+		return "", fmt.Errorf("ListPodSandbox returned 0 item for pod %s", podUID)
 	}
 
 	if len(ListPodSandboxRequest.Items) > 1 {
-		return "", fmt.Errorf("ListPodSandbox returned more than 1 item for pod %s.%s: %w", podName, podNamespace, err)
+		return "", fmt.Errorf("ListPodSandbox returned more than 1 item for pod %s", podUID)
 	}
 
 	return ListPodSandboxRequest.Items[0].Id, nil

diff --git a/pkg/cri/runtime_test.go b/pkg/cri/runtime_test.go
@@ -21,42 +21,40 @@ var _ = Describe("CRI runtime", func() {
 
 	When("the runtime *does not* feature any pod", func() {
 		const (
-			podName      = "my-pod"
-			podNamespace = "default"
+			podUID = "abc-def"
 		)
 
 		BeforeEach(func() {
 			runtime = newDummyCrioRuntime()
 		})
 
 		It("cannot extract the network namespace of a pod", func() {
-			_, err := runtime.NetworkNamespace(context.Background(), podName, podNamespace)
+			_, err := runtime.NetworkNamespace(context.Background(), podUID)
 			Expect(err).To(HaveOccurred())
 		})
 
 		It("cannot extract the PodSandboxID of a pod", func() {
-			_, err := runtime.PodSandboxID(context.Background(), podName, podNamespace)
+			_, err := runtime.PodSandboxID(context.Background(), podUID)
 			Expect(err).To(HaveOccurred())
 		})
 	})
 
 	When("a live container is provisioned in the runtime", func() {
 		const (
-			podName      = "my-pod"
-			podNamespace = "default"
+			podUID       = "abc-def"
 			podSandboxID = "1234"
 			netnsPath    = "bottom-drawer"
 		)
 		BeforeEach(func() {
-			runtime = newDummyCrioRuntime(fake.WithCachedContainer(podName, podNamespace, podSandboxID, netnsPath))
+			runtime = newDummyCrioRuntime(fake.WithCachedContainer(podUID, podSandboxID, netnsPath))
 		})
 
 		It("cannot extract the network namespace of a pod", func() {
-			Expect(runtime.NetworkNamespace(context.Background(), podName, podNamespace)).To(Equal(netnsPath))
+			Expect(runtime.NetworkNamespace(context.Background(), podUID)).To(Equal(netnsPath))
 		})
 
 		It("cannot extract the PodSandboxID of a pod", func() {
-			Expect(runtime.PodSandboxID(context.Background(), podName, podNamespace)).To(Equal(podSandboxID))
+			Expect(runtime.PodSandboxID(context.Background(), podUID)).To(Equal(podSandboxID))
 		})
 	})
 })

diff --git a/templates/dynamic-networks-controller.yaml.j2 b/templates/dynamic-networks-controller.yaml.j2
@@ -61,9 +61,6 @@ metadata:
 data:
   dynamic-networks-config.json: |
     {
-        {%- if CRIO_RUNTIME is defined %}
-        "criType": "crio",
-        {%- endif %}
         "criSocketPath": "/host{{ CRI_SOCKET_PATH }}",
         "multusSocketPath": "/host{{ MULTUS_SOCKET_PATH }}"
     }