ctmap: make PerClusterCTMap.getClusterMap return error for nonexisten…

…t map

By changing the underlying map abstraction to cilium/ebpf, opening a map from a pin
no longer returns an error chain containing a os.PathError, only a unix.ENOENT.
Return errors from getClusterMap() instead of a vague 'nil, nil'.

Check for ENOENT in getAllClusterMaps() instead of checking the first return value.
Stop checking the first return value in multiple places.

Also removed redundant mapType argument from PerClusterCTMap.newInnerMap().

Update tests to perform error checking first before accessing any other return value.

Signed-off-by: Timo Beckers <timo@isovalent.com>

pkg/maps/ctmap/per_cluster_ctmap.go

@@ -6,7 +6,6 @@ package ctmap
 import (
 	"errors"
 	"fmt"
-	"os"
 	"strconv"
 	"unsafe"
 
@@ -245,25 +244,25 @@ func (gm *perClusterCTMaps) GetClusterCTMaps(clusterID uint32) []*Map {
 	}()
 
 	if gm.ipv4 {
-		if im, err = gm.tcp4.getClusterMap(clusterID); err != nil || im == nil {
+		if im, err = gm.tcp4.getClusterMap(clusterID); err != nil {
 			return []*Map{}
 		} else {
 			ret = append(ret, im)
 		}
-		if im, err = gm.any4.getClusterMap(clusterID); err != nil || im == nil {
+		if im, err = gm.any4.getClusterMap(clusterID); err != nil {
 			return []*Map{}
 		} else {
 			ret = append(ret, im)
 		}
 	}
 
 	if gm.ipv6 {
-		if im, err = gm.tcp6.getClusterMap(clusterID); err != nil || im == nil {
+		if im, err = gm.tcp6.getClusterMap(clusterID); err != nil {
 			return []*Map{}
 		} else {
 			ret = append(ret, im)
 		}
-		if im, err = gm.any6.getClusterMap(clusterID); err != nil || im == nil {
+		if im, err = gm.any6.getClusterMap(clusterID); err != nil {
 			return []*Map{}
 		} else {
 			ret = append(ret, im)
@@ -506,9 +505,9 @@ func newPerClusterCTMap(name string, m mapType) (*PerClusterCTMap, error) {
 	}, nil
 }
 
-func (om *PerClusterCTMap) newInnerMap(clusterID uint32, m mapType) *Map {
+func (om *PerClusterCTMap) newInnerMap(clusterID uint32) *Map {
 	name := om.getInnerMapName(clusterID)
-	im := newMap(name, m)
+	im := newMap(name, om.m)
 	im.clusterID = clusterID
 	return im
 }
@@ -522,7 +521,7 @@ func (om *PerClusterCTMap) updateClusterCTMap(clusterID uint32) error {
 		return fmt.Errorf("invalid clusterID %d, clusterID should be 1 - %d", clusterID, cmtypes.ClusterIDMax)
 	}
 
-	im := om.newInnerMap(clusterID, om.m)
+	im := om.newInnerMap(clusterID)
 
 	if err := im.OpenOrCreate(); err != nil {
 		return err
@@ -548,7 +547,7 @@ func (om *PerClusterCTMap) deleteClusterCTMap(clusterID uint32) error {
 		return fmt.Errorf("invalid clusterID %d, clusterID should be 1 - %d", clusterID, cmtypes.ClusterIDMax)
 	}
 
-	im := om.newInnerMap(clusterID, om.m)
+	im := om.newInnerMap(clusterID)
 
 	if err := im.OpenOrCreate(); err != nil {
 		return err
@@ -573,13 +572,10 @@ func (om *PerClusterCTMap) getClusterMap(clusterID uint32) (*Map, error) {
 		return nil, fmt.Errorf("invalid clusterID %d, clusterID should be 1 - %d", clusterID, cmtypes.ClusterIDMax)
 	}
 
-	im := om.newInnerMap(clusterID, om.m)
+	im := om.newInnerMap(clusterID)
 
 	if err := im.Open(); err != nil {
-		if pathErr, ok := err.(*os.PathError); ok && errors.Is(pathErr.Err, unix.ENOENT) {
-			return nil, nil
-		}
-		return nil, err
+		return nil, fmt.Errorf("open inner map: %w", err)
 	}
 
 	// Callers are responsible for closing returned map
@@ -604,13 +600,12 @@ func (om *PerClusterCTMap) getAllClusterMaps() ([]*Map, error) {
 
 	for i := uint32(1); i < cmtypes.ClusterIDMax; i++ {
 		im, err = om.getClusterMap(i)
+		if errors.Is(err, unix.ENOENT) {
+			continue
+		}
 		if err != nil {
 			return nil, err
 		}
-		if im == nil {
-			// Map didn't exist
-			continue
-		}
 		innerMaps = append(innerMaps, im)
 	}
 

pkg/maps/ctmap/per_cluster_ctmap_test.go

@@ -112,20 +112,19 @@ func (k *PerClusterCTMapPrivilegedTestSuite) TestPerClusterCTMap(c *C) {
 
 	// Basic Get
 	im, err := om.getClusterMap(1)
-	c.Assert(im, NotNil)
 	c.Assert(err, IsNil)
+	c.Assert(im, NotNil)
 
 	im.Close()
 
-	// Getting unexisting entry returns nil, nil
-	im, err = om.getClusterMap(2)
-	c.Assert(im, IsNil)
-	c.Assert(err, IsNil)
+	// Getting nonexistent entry returns an error
+	_, err = om.getClusterMap(2)
+	c.Assert(err, NotNil)
 
 	// Basic all get
 	ims, err := om.getAllClusterMaps()
-	c.Assert(len(ims), Equals, 1)
 	c.Assert(err, IsNil)
+	c.Assert(len(ims), Equals, 1)
 
 	for _, im := range ims {
 		im.Close()

pkg/maps/nat/per_cluster_nat.go

@@ -4,14 +4,10 @@
 package nat
 
 import (
-	"errors"
 	"fmt"
-	"os"
 	"strconv"
 	"unsafe"
 
-	"golang.org/x/sys/unix"
-
 	"github.com/cilium/ebpf"
 
 	"github.com/cilium/cilium/pkg/bpf"
@@ -193,9 +189,7 @@ func (om *PerClusterNATMap) getClusterNATMap(clusterID uint32) (*Map, error) {
 	im := om.newInnerMap(om.getInnerMapName(clusterID))
 
 	if err := im.Open(); err != nil {
-		if pathErr, ok := err.(*os.PathError); ok && errors.Is(pathErr.Err, unix.ENOENT) {
-			return nil, nil
-		}
+		return nil, fmt.Errorf("open inner map: %w", err)
 	}
 
 	return im, nil

pkg/maps/nat/per_cluster_nat_test.go

@@ -83,10 +83,9 @@ func (k *PerClusterNATMapPrivilegedTestSuite) TestPerClusterCtMap(c *C) {
 
 	im.Close()
 
-	// Getting unexisting entry returns nil, nil
-	im, err = om.getClusterNATMap(2)
-	c.Assert(im, IsNil)
-	c.Assert(err, IsNil)
+	// Getting nonexistent entry returns error
+	_, err = om.getClusterNATMap(2)
+	c.Assert(err, NotNil)
 
 	// Basic delete
 	err = om.deleteClusterNATMap(1)