Merge pull request spidernet-io#2902 from Icarus9913/fix/wk/ippools

add validation for IPAM IPPools annotation usage
ty-dc · Dec 15, 2023 · 3e3aede · 3e3aede
2 parents fe936b5 + 8ced784
commit 3e3aede
Show file tree

Hide file tree

Showing 5 changed files with 88 additions and 8 deletions.
diff --git a/cmd/spiderpool/cmd/command_add.go b/cmd/spiderpool/cmd/command_add.go
@@ -182,5 +182,9 @@ func assembleResult(cniVersion, IfName string, ipamResponse *daemonset.PostIpamI
 		}
 	}
 
+	if len(result.IPs) == 0 {
+		return nil, fmt.Errorf("no Interface %s IP allocation found", IfName)
+	}
+
 	return result, nil
 }
diff --git a/pkg/ipam/pool_selections.go b/pkg/ipam/pool_selections.go
@@ -49,7 +49,7 @@ func (i *ipam) getPoolCandidates(ctx context.Context, addArgs *models.IpamAddArg
 
 	// Select IPPool candidates through the Pod annotation "ipam.spidernet.io/ippools".
 	if anno, ok := pod.Annotations[constant.AnnoPodIPPools]; ok {
-		return getPoolFromPodAnnoPools(ctx, anno)
+		return getPoolFromPodAnnoPools(ctx, anno, *addArgs.IfName)
 	}
 
 	// Select IPPool candidates through the Pod annotation "ipam.spidernet.io/ippool".
@@ -305,7 +305,7 @@ func (i *ipam) applyThirdControllerAutoPool(ctx context.Context, subnetName stri
 	return pool, nil
 }
 
-func getPoolFromPodAnnoPools(ctx context.Context, anno string) (ToBeAllocateds, error) {
+func getPoolFromPodAnnoPools(ctx context.Context, anno, currentNIC string) (ToBeAllocateds, error) {
 	logger := logutils.FromContext(ctx)
 	logger.Sugar().Infof("Use IPPools from Pod annotation '%s'", constant.AnnoPodIPPools)
 
@@ -317,7 +317,7 @@ func getPoolFromPodAnnoPools(ctx context.Context, anno string) (ToBeAllocateds,
 	}
 
 	// validate and mutate the IPPools annotation value
-	err = validateAndMutateMultipleNICAnnotations(annoPodIPPools)
+	err = validateAndMutateMultipleNICAnnotations(annoPodIPPools, currentNIC)
 	if nil != err {
 		return nil, fmt.Errorf("%w: %v", errPrefix, err)
 	}

diff --git a/pkg/ipam/utils.go b/pkg/ipam/utils.go
@@ -188,16 +188,16 @@ func IsMultipleNicWithNoName(anno map[string]string) bool {
 	return result
 }
 
-func validateAndMutateMultipleNICAnnotations(annoIPPoolsValue types.AnnoPodIPPoolsValue) error {
+func validateAndMutateMultipleNICAnnotations(annoIPPoolsValue types.AnnoPodIPPoolsValue, currentNIC string) error {
 	if len(annoIPPoolsValue) == 0 {
 		return fmt.Errorf("value requires at least one item")
 	}
 
 	nicSet := map[string]struct{}{}
-	isEmpty := annoIPPoolsValue[0].NIC == ""
+	hasNIC := annoIPPoolsValue[0].NIC != ""
 	for index := range annoIPPoolsValue {
 		// require all item NIC should be same in specified or unspecified.
-		if (annoIPPoolsValue[index].NIC == "") != isEmpty {
+		if (annoIPPoolsValue[index].NIC != "") != hasNIC {
 			return fmt.Errorf("NIC should be either all specified or all unspecified")
 		}
 
@@ -209,10 +209,19 @@ func validateAndMutateMultipleNICAnnotations(annoIPPoolsValue types.AnnoPodIPPoo
 		}
 
 		// require no NIC name duplicated, this works for multiple NIC specified by the users
-		if _, ok := nicSet[annoIPPoolsValue[index].NIC]; ok {
+		if _, ok := nicSet[annoIPPoolsValue[index].NIC]; !ok {
+			nicSet[annoIPPoolsValue[index].NIC] = struct{}{}
+		} else {
 			return fmt.Errorf("duplicate interface %s", annoIPPoolsValue[index].NIC)
 		}
 	}
 
+	if hasNIC {
+		_, ok := nicSet[currentNIC]
+		if !ok {
+			return fmt.Errorf("interface %s must be specified", currentNIC)
+		}
+	}
+
 	return nil
 }
diff --git a/test/doc/annotation.md b/test/doc/annotation.md
@@ -1,7 +1,7 @@
 # E2E Cases for Annotation
 
 | Case ID | Title                                                                                                                       | Priority | Smoke | Status | Other |
-| ------- |-----------------------------------------------------------------------------------------------------------------------------|----------|-------|--------|-------|
+|---------|-----------------------------------------------------------------------------------------------------------------------------|----------|-------|--------|-------|
 | A00001  | It fails to run a pod with different VLANs for IPv4 and IPv6 IPPools                                                        | p3       |       | done   |       |
 | A00002  | Added fields such as `"dist":"1.0.0.0/16"`, `"gw":"1.0.0.1"`, and `nics` and the pod was running successfully               | p2       |       | done   |       |
 | A00003  | Failed to run a pod with invalid annotations                                                                                | p3       |       | done   |       |
@@ -13,3 +13,5 @@
 | A00010  | Modify the annotated IPPool for a pod running on multiple NICs                                                              | p3       |       | done   |       |
 | A00011  | Use the ippool route with `cleanGateway=false` in the pod annotation as a default route                                     | p3       |       | done   |       |
 | A00012  | Specify the default NIC through Pod annotations                                                                             | p2       |       |        |       |
+| A00013  | It's invalid to specify one NIC corresponding IPPool in IPPools annotation with multiple NICs                               | p2       |       | done   |       |
+| A00014  | It's invalid to specify same NIC name for IPPools annotation with multiple NICs                                             | p2       |       | done   |       |
diff --git a/test/e2e/annotation/annotation_test.go b/test/e2e/annotation/annotation_test.go
@@ -740,6 +740,71 @@ var _ = Describe("test annotation", Label("annotation"), func() {
 		})
 
 	})
+
+	Context("wrong IPPools annotation usage", func() {
+		It("It's invalid to specify one NIC corresponding IPPool in IPPools annotation with multiple NICs", Label("A00013"), func() {
+			// set pod annotation for nics
+			podIppoolsAnno := types.AnnoPodIPPoolsValue{
+				{
+					NIC: common.NIC2,
+				},
+			}
+			if frame.Info.IpV4Enabled {
+				podIppoolsAnno[0].IPv4Pools = []string{common.SpiderPoolIPv4SubnetVlan100}
+			}
+			if frame.Info.IpV6Enabled {
+				podIppoolsAnno[0].IPv6Pools = []string{common.SpiderPoolIPv6SubnetVlan100}
+			}
+			podIppoolsAnnoMarshal, err := json.Marshal(podIppoolsAnno)
+			Expect(err).NotTo(HaveOccurred())
+			podYaml := common.GenerateExamplePodYaml(podName, nsName)
+			podYaml.Annotations = map[string]string{
+				pkgconstant.AnnoPodIPPools: string(podIppoolsAnnoMarshal),
+				common.MultusNetworks:      fmt.Sprintf("%s/%s", common.MultusNs, common.MacvlanVlan100),
+			}
+			GinkgoWriter.Printf("succeeded to generate pod yaml with IPPools annotation: %+v. \n", podYaml)
+
+			Expect(frame.CreatePod(podYaml)).To(Succeed())
+			ctx, cancel := context.WithTimeout(context.Background(), time.Minute*1)
+			defer cancel()
+			GinkgoWriter.Printf("wait for one minute that pod %v/%v would not ready. \n", nsName, podName)
+			_, err = frame.WaitPodStarted(podName, nsName, ctx)
+			Expect(err).To(HaveOccurred())
+		})
+
+		It("It's invalid to specify same NIC name for IPPools annotation with multiple NICs", Label("A00014"), func() {
+			// set pod annotation for nics
+			podIppoolsAnno := types.AnnoPodIPPoolsValue{
+				{
+					NIC: common.NIC2,
+				},
+				{
+					NIC: common.NIC2,
+				},
+			}
+			if frame.Info.IpV4Enabled {
+				podIppoolsAnno[0].IPv4Pools = []string{common.SpiderPoolIPv4SubnetVlan100}
+			}
+			if frame.Info.IpV6Enabled {
+				podIppoolsAnno[0].IPv6Pools = []string{common.SpiderPoolIPv6SubnetVlan100}
+			}
+			podIppoolsAnnoMarshal, err := json.Marshal(podIppoolsAnno)
+			Expect(err).NotTo(HaveOccurred())
+			podYaml := common.GenerateExamplePodYaml(podName, nsName)
+			podYaml.Annotations = map[string]string{
+				pkgconstant.AnnoPodIPPools: string(podIppoolsAnnoMarshal),
+				common.MultusNetworks:      fmt.Sprintf("%s/%s", common.MultusNs, common.MacvlanVlan100),
+			}
+			GinkgoWriter.Printf("succeeded to generate pod yaml with same NIC name annotation: %+v. \n", podYaml)
+
+			Expect(frame.CreatePod(podYaml)).To(Succeed())
+			ctx, cancel := context.WithTimeout(context.Background(), time.Minute*1)
+			defer cancel()
+			GinkgoWriter.Printf("wait for one minute that pod %v/%v would not ready. \n", nsName, podName)
+			_, err = frame.WaitPodStarted(podName, nsName, ctx)
+			Expect(err).To(HaveOccurred())
+		})
+	})
 })
 
 func checkAnnotationPriority(podYaml *corev1.Pod, podName, nsName string, v4PoolNameList, v6PoolNameList []string) {