Fix delete with prev result logic

Author: jaydeokar

cmd/routed-eni-cni-plugin/cni.go

@@ -170,11 +170,11 @@ func add(args *skel.CmdArgs, cniTypes typeswrapper.CNITYPES, grpcClient grpcwrap
 			requiresMultiNICAttachment = true
 		} else {
 			log.Debugf("Multi-NIC annotation mismatch: found=%q, required=%q. Falling back to default configuration.",
-				multiNICAttachment, conf.RuntimeConfig.PodAnnotations[multiNICPodAnnotation])
+				conf.RuntimeConfig.PodAnnotations[multiNICPodAnnotation], multiNICAttachment)
 		}
 	}
 
-	log.Debugf("pod requires Multi-NIC attachment: %t", requiresMultiNICAttachment)
+	log.Debugf("pod requires multi-nic attachment: %t", requiresMultiNICAttachment)
 
 	// Set up a connection to the ipamD server.
 	conn, err := grpcClient.Dial(ipamdAddress, grpc.WithTransportCredentials(insecure.NewCredentials()))
@@ -245,12 +245,11 @@ func add(args *skel.CmdArgs, cniTypes typeswrapper.CNITYPES, grpcClient grpcwrap
 			HostVethName:      hostVethName,
 			ContainerVethName: containerVethName,
 		})
-
-		// Check if we can use PCIID to store RT-ID ?
-		// The RT here is the host route table Id not the container
+		// CNI stores the route table ID in the MAC field of the interface. The Route table ID is on the host side
+		// and is used during cleanup to remove the ip rules when IPAMD is not reachable.
 		podInterfaces = append(podInterfaces,
 			&current.Interface{Name: hostVethName},
-			&current.Interface{Name: containerVethName, Sandbox: args.Netns, PciID: fmt.Sprint(ip.RouteTableId)},
+			&current.Interface{Name: containerVethName, Sandbox: args.Netns, Mac: fmt.Sprint(ip.RouteTableId)},
 		)
 
 		// This index always points to the container interface so that we get the IP address corresponding to the interface
@@ -276,7 +275,7 @@ func add(args *skel.CmdArgs, cniTypes typeswrapper.CNITYPES, grpcClient grpcwrap
 	} else {
 		err = driverClient.SetupPodNetwork(vethMetadata, args.Netns, mtu, log)
 		// For non-branch ENI, the pod VLAN ID value of 0 is packed in Interface.Mac, while the interface device number is packed in Interface.Sandbox
-		dummyInterface = &current.Interface{Name: dummyInterfaceName, Mac: fmt.Sprint(0), Sandbox: fmt.Sprint(vethMetadata[0].DeviceNumber), SocketPath: fmt.Sprint(len(r.IPAddress))}
+		dummyInterface = &current.Interface{Name: dummyInterfaceName, Mac: fmt.Sprint(0), Sandbox: fmt.Sprint(vethMetadata[0].DeviceNumber)}
 	}
 
 	log.Debugf("Using dummy interface: %v", dummyInterface)
@@ -311,6 +310,8 @@ func add(args *skel.CmdArgs, cniTypes typeswrapper.CNITYPES, grpcClient grpcwrap
 	}
 
 	// dummy interface is appended to PrevResult for use during cleanup
+	// The interfaces field should only include host,container and dummy interfaces in the list.
+	// Revisit the prevResult cleanup logic if this changes
 	result.Interfaces = append(result.Interfaces, dummyInterface)
 
 	// Set up a connection to the network policy agent
@@ -348,7 +349,6 @@ func add(args *skel.CmdArgs, cniTypes typeswrapper.CNITYPES, grpcClient grpcwrap
 	}
 
 	log.Debugf("Network Policy agent for EnforceNpToPod returned Success : %v", npr.Success)
-
 	return cniTypes.PrintResult(result, conf.CNIVersion)
 }
 
@@ -539,8 +539,13 @@ func getContainerNetworkMetadata(prevResult *current.Result, contVethName string
 // returns true if the del request is handled.
 func tryDelWithPrevResult(driverClient driver.NetworkAPIs, conf *NetConf, k8sArgs K8sArgs, contVethName string, netNS string, log logger.Logger) (bool, error) {
 	// prevResult might not be available, if we are still using older cni spec < 0.4.0.
-	prevResult, ok := conf.PrevResult.(*current.Result)
-	if !ok {
+	if conf.PrevResult == nil {
+		return false, nil
+	}
+
+	prevResult, err := current.NewResultFromResult(conf.PrevResult)
+	if err != nil {
+		log.Info("PrevResult not available for pod or parsing failed")
 		return false, nil
 	}
 
@@ -581,8 +586,13 @@ func tryDelWithPrevResult(driverClient driver.NetworkAPIs, conf *NetConf, k8sArg
 // Returns true if pod network is torn down
 func teardownPodNetworkWithPrevResult(driverClient driver.NetworkAPIs, conf *NetConf, k8sArgs K8sArgs, contVethName string, log logger.Logger) bool {
 	// For non-branch ENI, prevResult is only available in v1.12.1+
-	prevResult, ok := conf.PrevResult.(*current.Result)
-	if !ok {
+	if conf.PrevResult == nil {
+		log.Infof("PrevResult not available for pod. Pod may have already been deleted.")
+		return false
+	}
+
+	prevResult, err := current.NewResultFromResult(conf.PrevResult)
+	if err != nil {
 		log.Infof("PrevResult not available for pod. Pod may have already been deleted.")
 		return false
 	}
@@ -607,32 +617,24 @@ func teardownPodNetworkWithPrevResult(driverClient driver.NetworkAPIs, conf *Net
 	// RT ID for NC-0 is also stored in the container interface entry. So we have a path for migration where
 	// getting the device number from dummy interface can be deprecated entirely. This is currently done to keep it backwards compatible
 	routeTableId := deviceNumber + 1
-
-	var interfacesAttached = 1
-	if dummyIface.SocketPath != "" {
-		interfacesAttached, err = strconv.Atoi(dummyIface.SocketPath)
-		if err != nil {
-			log.Errorf("error getting number of interfaces attached to the pod: %s", dummyIface.SocketPath)
-			return false
-		}
-	}
-
+	// The number of interfaces attached to the pod is taken as the length of the interfaces array - 1 (for dummy interface) divided by 2 (for host and container interface)
+	var interfacesAttached = (len(prevResult.Interfaces) - 1) / 2
 	var vethMetadata []driver.VirtualInterfaceMetadata
 	for v := range interfacesAttached {
 		containerInterfaceName := networkutils.GenerateContainerVethName(contVethName, containerVethNamePrefix, v)
 		containerIP, containerInterface, err := getContainerNetworkMetadata(prevResult, containerInterfaceName)
 		if err != nil {
-			log.Errorf("Failed to get container IP: %v", err)
-			return false
+			log.Errorf("container interface name %s does not exist %v", containerInterfaceName, err)
+			continue
 		}
 
 		// If this property is set, that means the container metadata has the route table ID which we can use
 		// If this is not set, it is a pod launched before this change was introduced.
-		// So it is only managing network card 0 at that time and device number + 1 is the route table ID which we have above
-		if dummyIface.SocketPath != "" {
-			routeTableId, err = strconv.Atoi(containerInterface.PciID)
+		// So it is only managing network card 0 at that time and device number + 1 is the route table ID which we calculate from device number
+		if containerInterface.Mac != "" {
+			routeTableId, err = strconv.Atoi(containerInterface.Mac)
 			if err != nil {
-				log.Errorf("error getting route table number of the interface %s", containerInterface.PciID)
+				log.Errorf("error getting route table number of the interface %s", containerInterface.Mac)
 				return false
 			}
 		}

go.sum

@@ -329,10 +329,10 @@ github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8m
 github.com/mwitkow/go-conntrack v0.0.0-20161129095857-cc309e4a2223/go.mod h1:qRWi+5nqEBWmkhHvq77mSJWrCKwh8bxhgT7d/eI7P4U=
 github.com/mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f h1:y5//uYreIhSUg3J1GEMiLbxo1LJaP8RfCpH6pymGZus=
 github.com/mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f/go.mod h1:ZdcZmHo+o7JKHSa8/e818NopupXU1YMK5fe1lsApnBw=
-github.com/onsi/ginkgo/v2 v2.22.1 h1:QW7tbJAUDyVDVOM5dFa7qaybo+CRfR7bemlQUN6Z8aM=
-github.com/onsi/ginkgo/v2 v2.22.1/go.mod h1:S6aTpoRsSq2cZOd+pssHAlKW/Q/jZt6cPrPlnj4a1xM=
-github.com/onsi/gomega v1.36.2 h1:koNYke6TVk6ZmnyHrCXba/T/MoLBXFjeC1PtvYgw0A8=
-github.com/onsi/gomega v1.36.2/go.mod h1:DdwyADRjrc825LhMEkD76cHR5+pUnjhUN8GlHlRPHzY=
+github.com/onsi/ginkgo/v2 v2.22.0 h1:Yed107/8DjTr0lKCNt7Dn8yQ6ybuDRQoMGrNFKzMfHg=
+github.com/onsi/ginkgo/v2 v2.22.0/go.mod h1:7Du3c42kxCUegi0IImZ1wUQzMBVecgIHjR1C+NkhLQo=
+github.com/onsi/gomega v1.36.0 h1:Pb12RlruUtj4XUuPUqeEWc6j5DkVVVA49Uf6YLfC95Y=
+github.com/onsi/gomega v1.36.0/go.mod h1:PvZbdDc8J6XJEpDK4HCuRBm8a6Fzp9/DmhC9C7yFlog=
 github.com/opencontainers/go-digest v1.0.0 h1:apOUWs51W5PlhuyGyz9FCeeBIOUDA/6nW8Oi/yOhh5U=
 github.com/opencontainers/go-digest v1.0.0/go.mod h1:0JzlMkj0TRzQZfJkVvzbP0HBR3IKzErnv2BNG4W4MAM=
 github.com/opencontainers/image-spec v1.1.0 h1:8SG7/vwALn54lVB/0yZ/MMwhFrPYtpEHQb2IpWsCzug=

pkg/awsutils/awsutils.go

@@ -918,12 +918,9 @@ func (cache *EC2InstanceMetadataCache) attachENI(eniID string, networkCard int)
 
 // createENITags creates all the tags required to be added to the ENI
 //
-// Parameters:
-// - eniDescription (string):
-//
 // Returns:
 // - []ec2types.TagSpecification: Returns the tags by converting it into AWS SDK class
-func (cache *EC2InstanceMetadataCache) createENITags(eniDescription string) []ec2types.TagSpecification {
+func (cache *EC2InstanceMetadataCache) createENITags() []ec2types.TagSpecification {
 
 	tags := map[string]string{
 		eniCreatedAtTagKey: time.Now().Format(time.RFC3339),
@@ -966,7 +963,7 @@ func (cache *EC2InstanceMetadataCache) createENIInput(eniDescription string, tag
 // return ENI id, error
 func (cache *EC2InstanceMetadataCache) createENI(sg []*string, eniCfgSubnet string, numIPs int) (string, error) {
 	eniDescription := eniDescriptionPrefix + cache.instanceID
-	tags := cache.createENITags(eniDescription)
+	tags := cache.createENITags()
 
 	var needIPs = numIPs
 

pkg/ipamd/datastore/data_store.go

@@ -281,6 +281,7 @@ type DataStore struct {
 	netLink          netlinkwrapper.NetLink
 	isPDEnabled      bool
 	ipCooldownPeriod time.Duration
+	networkCard      int
 }
 
 // ENIInfos contains ENI IP information
@@ -294,14 +295,15 @@ type ENIInfos struct {
 }
 
 // NewDataStore returns DataStore structure
-func NewDataStore(log logger.Logger, backingStore Checkpointer, isPDEnabled bool) *DataStore {
+func NewDataStore(log logger.Logger, backingStore Checkpointer, isPDEnabled bool, networkCard int) *DataStore {
 	return &DataStore{
 		eniPool:          make(ENIPool),
 		log:              log,
 		backingStore:     backingStore,
 		netLink:          netlinkwrapper.NewNetLink(),
 		isPDEnabled:      isPDEnabled,
 		ipCooldownPeriod: getCooldownPeriod(),
+		networkCard:      networkCard,
 	}
 }
 
@@ -743,7 +745,7 @@ func (ds *DataStore) AssignPodIPv4Address(ipamKey IPAMKey, ipamMetadata IPAMMeta
 
 // assignPodIPAddressUnsafe mark Address as assigned.
 func (ds *DataStore) assignPodIPAddressUnsafe(addr *AddressInfo, ipamKey IPAMKey, ipamMetadata IPAMMetadata, assignedTime time.Time) {
-	ds.log.Infof("assignPodIPAddressUnsafe: Assign IP %v to sandbox %s with metadata %",
+	ds.log.Infof("assignPodIPAddressUnsafe: Assign IP %v to sandbox %s with metadata %+v",
 		addr.Address, ipamKey, ipamMetadata)
 
 	if addr.Assigned() {
@@ -1360,6 +1362,10 @@ func (ds *DataStore) getUnusedIP(availableCidr *CidrInfo) (string, error) {
 	return "", fmt.Errorf("no free IP available in the prefix - %s/%s", availableCidr.Cidr.IP, availableCidr.Cidr.Mask)
 }
 
+func (ds *DataStore) GetNetworkCard() int {
+	return ds.networkCard
+}
+
 func getNextIPAddr(ip net.IP) {
 	for j := len(ip) - 1; j >= 0; j-- {
 		ip[j]++
@@ -1547,34 +1553,39 @@ func (ds *DataStore) DeleteFromContainerRule(entry *CheckpointEntry) {
 }
 
 type DataStoreAccess struct {
-	DataStores map[int]*DataStore
+	DataStores []*DataStore
 }
 
 func InitializeDataStores(skipNetworkCards []bool, defaultDataStorePath string, enablePD bool, log logger.Logger) *DataStoreAccess {
 
-	var dsMap = make(map[int]*DataStore, len(skipNetworkCards))
-	for i, shouldSkip := range skipNetworkCards {
-		if shouldSkip {
-			continue
-		} else {
+	var dsList = make([]*DataStore, 0, len(skipNetworkCards))
+	for networkCard, shouldSkip := range skipNetworkCards {
+
+		if !shouldSkip {
 			dsBackingStorePath := defaultDataStorePath
-			if i > 0 {
+			if networkCard > 0 {
 				baseName := strings.TrimSuffix(defaultDataStorePath, ".json")
-				dsBackingStorePath = fmt.Sprintf("%s-nic-%d.json", baseName, i)
+				dsBackingStorePath = fmt.Sprintf("%s-nic-%d.json", baseName, networkCard)
 			}
 			checkpointer := NewJSONFile(dsBackingStorePath)
-			dsMap[i] = NewDataStore(log, checkpointer, enablePD)
-			log.Infof("initialized datastore for network cards index %d", i)
+			dsList = append(dsList, NewDataStore(log, checkpointer, enablePD, networkCard))
+			log.Infof("initialized datastore for network cards index %d", networkCard)
 		}
 	}
 
 	return &DataStoreAccess{
-		DataStores: dsMap,
+		DataStores: dsList,
 	}
 }
 
-func (ds *DataStoreAccess) GetDataStore(index int) *DataStore {
-	return ds.DataStores[index]
+func (ds *DataStoreAccess) GetDataStore(networkCard int) *DataStore {
+
+	for index, datastore := range ds.DataStores {
+		if datastore.GetNetworkCard() == networkCard {
+			return ds.DataStores[index]
+		}
+	}
+	return nil
 }
 
 func (ds *DataStoreAccess) ReadAllDataStores(enableIPv6 bool) error {

pkg/ipamd/introspect.go

@@ -125,8 +125,8 @@ func (c *IPAMContext) setupIntrospectionServer() *http.Server {
 func eniV1RequestHandler(ipam *IPAMContext) func(http.ResponseWriter, *http.Request) {
 	return func(w http.ResponseWriter, r *http.Request) {
 		eniInfos := make(map[int]*datastore.ENIInfos, len(ipam.dataStoreAccess.DataStores))
-		for networkCard, ds := range ipam.dataStoreAccess.DataStores {
-			eniInfos[networkCard] = ds.GetENIInfos()
+		for _, ds := range ipam.dataStoreAccess.DataStores {
+			eniInfos[ds.GetNetworkCard()] = ds.GetENIInfos()
 		}
 		responseJSON, err := json.Marshal(eniInfos)
 		if err != nil {

pkg/ipamd/ipamd.go

@@ -216,7 +216,6 @@ var (
 // IPAMContext contains node level control information
 type IPAMContext struct {
 	awsClient                 awsutils.APIs
-	dataStore                 []*datastore.DataStore
 	dataStoreAccess           *datastore.DataStoreAccess
 	k8sClient                 client.Client
 	enableIPv4                bool
@@ -964,7 +963,8 @@ func (c *IPAMContext) createSecondaryIPv6ENIs(ctx context.Context) error {
 
 	log.Info("Attaching ENIs allowed")
 
-	for networkCard, ds := range c.dataStoreAccess.DataStores {
+	for _, ds := range c.dataStoreAccess.DataStores {
+		networkCard := ds.GetNetworkCard()
 		if networkCard == DefaultNetworkCardIndex {
 			continue
 		}
@@ -1527,7 +1527,8 @@ func (c *IPAMContext) nodeIPPoolReconcile(ctx context.Context, interval time.Dur
 	allManagedAndAttachedENIs := c.filterUnmanagedENIs(allENIs)
 	attachedENIsByNetworkCard := c.getENIsByNetworkCard(allManagedAndAttachedENIs)
 
-	for networkCard, ds := range c.dataStoreAccess.DataStores {
+	for _, ds := range c.dataStoreAccess.DataStores {
+		networkCard := ds.GetNetworkCard()
 		currentENIs := ds.GetENIInfos().ENIs
 		attachedENIs := attachedENIsByNetworkCard[networkCard]
 		trunkENI := ds.GetTrunkENI()
@@ -2280,7 +2281,7 @@ func (c *IPAMContext) AnnotatePod(podName string, podNamespace string, key strin
 			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)
+					return fmt.Errorf("released IP %s does not match existing annotation. Not patching pod", releasedIP)
 				}
 				newPod.Annotations[key] = ""
 			}
@@ -2300,7 +2301,8 @@ func (c *IPAMContext) AnnotatePod(podName string, podNamespace string, key strin
 func (c *IPAMContext) tryUnassignIPsFromENIs() {
 	log.Debugf("tryUnassignIPsFromENIs")
 	// From all datastores, get ENIInfos and unassign IPs
-	for networkCard, ds := range c.dataStoreAccess.DataStores {
+	for _, ds := range c.dataStoreAccess.DataStores {
+		networkCard := ds.GetNetworkCard()
 		eniInfos := ds.GetENIInfos()
 		for eniID := range eniInfos.ENIs {
 			c.tryUnassignIPFromENI(eniID, networkCard)
@@ -2345,7 +2347,8 @@ func (c *IPAMContext) tryUnassignPrefixesFromENIs() {
 	log.Debugf("tryUnassignPrefixesFromENIs")
 
 	// From all datastores get ENIs and remove prefixes
-	for networkCard, ds := range c.dataStoreAccess.DataStores {
+	for _, ds := range c.dataStoreAccess.DataStores {
+		networkCard := ds.GetNetworkCard()
 		eniInfos := ds.GetENIInfos()
 		for eniID := range eniInfos.ENIs {
 			c.tryUnassignPrefixFromENI(eniID, networkCard)
@@ -2442,8 +2445,8 @@ func (c *IPAMContext) isDatastorePoolTooLow() map[int]Decisions {
 		totalIPs = maxIpsPerPrefix
 	}
 
-	for networkCard, ds := range c.dataStoreAccess.DataStores {
-
+	for _, ds := range c.dataStoreAccess.DataStores {
+		networkCard := ds.GetNetworkCard()
 		stats := ds.GetIPStats(ipV4AddrFamily)
 		// If max pods has been reached, pool is not too low
 		if stats.TotalIPs >= c.maxPods {