adding ip check for annotatePod in ipamd

pkg/ipamd/ipamd.go

@@ -2051,7 +2051,7 @@ func (c *IPAMContext) GetPod(podName, namespace string) (*corev1.Pod, error) {
 }
 
 // AnnotatePod annotates the pod with the provided key and value
-func (c *IPAMContext) AnnotatePod(podName, podNamespace, key, val string) error {
+func (c *IPAMContext) AnnotatePod(podName string, podNamespace string, key string, newVal string, releasedIP string) error {
 	ctx := context.TODO()
 	var err error
 
@@ -2069,12 +2069,33 @@ func (c *IPAMContext) AnnotatePod(podName, podNamespace, key, val string) error
 		if newPod.Annotations == nil {
 			newPod.Annotations = make(map[string]string)
 		}
-		newPod.Annotations[key] = val
+
+		oldVal, ok := newPod.Annotations[key]
+
+		// On CNI ADD, always set new annotation
+		if newVal != "" {
+			// Skip patch operation if new value is the same as existing value
+			if ok && oldVal == newVal {
+				log.Infof("Patch updating not needed")
+				return nil
+			}
+			newPod.Annotations[key] = newVal
+		} else {
+			// On CNI DEL, set annotation to empty string if IP is the one we are releasing
+			if ok {
+				log.Debugf("Existing annotation value: %s", oldVal)
+				if oldVal != releasedIP {
+					return fmt.Errorf("Released IP %s does not match existing annotation. Not patching pod.", releasedIP)
+				}
+				newPod.Annotations[key] = ""
+			}
+		}
+
 		if err = c.rawK8SClient.Patch(ctx, newPod, client.MergeFrom(pod)); err != nil {
-			log.Errorf("Failed to annotate %s the pod with %s, error %v", key, val, err)
+			log.Errorf("Failed to annotate %s the pod with %s, error %v", key, newVal, err)
 			return err
 		}
-		log.Debugf("Annotates pod %s with %s: %s", podName, key, val)
+		log.Debugf("Annotates pod %s with %s: %s", podName, key, newVal)
 		return nil
 	})
 

pkg/ipamd/ipamd_test.go

@@ -2344,3 +2344,79 @@ func TestIsConfigValid(t *testing.T) {
 	}
 
 }
+
+func TestAnnotatePod(t *testing.T) {
+	m := setup(t)
+	defer m.ctrl.Finish()
+	ctx := context.Background()
+
+	// Define the Pod objects to test
+	pod := corev1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-pod",
+			Namespace: "test-namespace",
+		},
+	}
+
+	mockContext := &IPAMContext{
+		awsClient:       m.awsutils,
+		rawK8SClient:    m.rawK8SClient,
+		cachedK8SClient: m.cachedK8SClient,
+		primaryIP:       make(map[string]string),
+		terminating:     int32(0),
+		networkClient:   m.network,
+		dataStore:       testDatastore(),
+		enableIPv4:      true,
+		enableIPv6:      false,
+	}
+
+	mockContext.rawK8SClient.Create(ctx, &pod)
+
+	ipOne := "10.0.0.1"
+	ipTwo := "10.0.0.2"
+
+	// Test basic add operation for new pod
+	err := mockContext.AnnotatePod(pod.Name, pod.Namespace, "ip-address", ipOne, "")
+	assert.NoError(t, err)
+
+	updatedPod, err := mockContext.GetPod(pod.Name, pod.Namespace)
+	assert.NoError(t, err)
+	assert.Equal(t, ipOne, updatedPod.Annotations["ip-address"])
+
+	// Test that add operation is idempotent
+	err = mockContext.AnnotatePod(pod.Name, pod.Namespace, "ip-address", ipOne, "")
+	assert.NoError(t, err)
+
+	updatedPod, err = mockContext.GetPod(pod.Name, pod.Namespace)
+	assert.NoError(t, err)
+	assert.Equal(t, ipOne, updatedPod.Annotations["ip-address"])
+
+	// Test that add operation always overwrites value for existing pod
+	err = mockContext.AnnotatePod(pod.Name, pod.Namespace, "ip-address", ipTwo, "")
+	assert.NoError(t, err)
+
+	updatedPod, err = mockContext.GetPod(pod.Name, pod.Namespace)
+	assert.NoError(t, err)
+	assert.Equal(t, ipTwo, updatedPod.Annotations["ip-address"])
+
+	// Test that delete operation will not overwrite if IP being released does not match existing value
+	err = mockContext.AnnotatePod(pod.Name, pod.Namespace, "ip-address", "", ipOne)
+	assert.Error(t, err)
+	assert.Equal(t, fmt.Errorf("Released IP %s does not match existing annotation. Not patching pod.", ipOne), err)
+
+	updatedPod, err = mockContext.GetPod(pod.Name, pod.Namespace)
+	assert.Equal(t, ipTwo, updatedPod.Annotations["ip-address"])
+
+	// Test that delete operation succeeds when IP being released matches existing value
+	err = mockContext.AnnotatePod(pod.Name, pod.Namespace, "ip-address", "", ipTwo)
+	assert.NoError(t, err)
+
+	updatedPod, err = mockContext.GetPod(pod.Name, pod.Namespace)
+	assert.NoError(t, err)
+	assert.Equal(t, "", updatedPod.Annotations["ip-address"])
+
+	// Test that delete on a non-existant pod fails without crashing
+	err = mockContext.AnnotatePod("no-exist-name", "no-exist-namespace", "ip-address", "", ipTwo)
+	assert.Error(t, err)
+	assert.Equal(t, fmt.Errorf("error while trying to retrieve pod info: pods \"no-exist-name\" not found"), err)
+}

pkg/ipamd/rpc_handler.go

@@ -179,7 +179,11 @@ func (s *server) AddNetwork(ctx context.Context, in *rpc.AddNetworkRequest) (*rp
 	}
 
 	if s.ipamContext.enablePodIPAnnotation {
-		s.ipamContext.AnnotatePod(in.K8S_POD_NAME, in.K8S_POD_NAMESPACE, vpccniPodIPKey, ipv4Addr)
+		// On ADD, we pass empty string as there is no IP being released
+		err = s.ipamContext.AnnotatePod(in.K8S_POD_NAME, in.K8S_POD_NAMESPACE, vpccniPodIPKey, ipv4Addr, "")
+		if err != nil {
+			log.Errorf("Failed to add the pod annotation: %v", err)
+		}
 	}
 	resp := rpc.AddNetworkReply{
 		Success:         err == nil,
@@ -273,7 +277,11 @@ func (s *server) DelNetwork(ctx context.Context, in *rpc.DelNetworkRequest) (*rp
 	}
 
 	if s.ipamContext.enablePodIPAnnotation {
-		s.ipamContext.AnnotatePod(in.K8S_POD_NAME, in.K8S_POD_NAMESPACE, vpccniPodIPKey, "")
+		// On DEL, we pass IP being released
+		err = s.ipamContext.AnnotatePod(in.K8S_POD_NAME, in.K8S_POD_NAMESPACE, vpccniPodIPKey, "", ip)
+		if err != nil {
+			log.Errorf("Failed to delete the pod annotation: %v", err)
+		}
 	}
 
 	log.Infof("Send DelNetworkReply: IPv4Addr %s, DeviceNumber: %d, err: %v", ip, deviceNumber, err)