auth: delete cache-entry on ErrKeyNotExist

In the unlikely event of trying to delete an auth map entry which is no
longer present in the auth map, the entry should also be removed from
the auth map cache. Currently it gets kept in the cache, which would
result in unnecessary retries.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>

pkg/auth/authmap_cache.go

@@ -69,7 +69,13 @@ func (r *authMapCache) Delete(key authKey) error {
 	defer r.cacheEntriesMutex.Unlock()
 
 	if err := r.authmap.Delete(key); err != nil {
-		return err
+		if !errors.Is(err, ebpf.ErrKeyNotExist) {
+			return fmt.Errorf("failed to delete auth entry from map: %w", err)
+		}
+
+		r.logger.
+			WithField("key", key).
+			Warning("Failed to delete already deleted auth entry")
 	}
 
 	delete(r.cacheEntries, key)
@@ -85,13 +91,13 @@ func (r *authMapCache) DeleteIf(predicate func(key authKey, info authInfo) bool)
 		if predicate(k, v) {
 			// delete every entry individually to keep the cache in sync in case of an error
 			if err := r.authmap.Delete(k); err != nil {
-				if errors.Is(err, ebpf.ErrKeyNotExist) {
-					r.logger.
-						WithField("key", k).
-						Debug("Failed to delete already deleted auth entry")
-					continue
+				if !errors.Is(err, ebpf.ErrKeyNotExist) {
+					return fmt.Errorf("failed to delete auth entry from map: %w", err)
 				}
-				return fmt.Errorf("failed to delete auth entry from map: %w", err)
+
+				r.logger.
+					WithField("key", k).
+					Warning("Failed to delete already deleted auth entry")
 			}
 			delete(r.cacheEntries, k)
 		}

pkg/auth/authmap_cache_test.go

@@ -79,3 +79,138 @@ func Test_authMapCache_allReturnsCopy(t *testing.T) {
 	assert.Len(t, all, 2)
 	assert.Len(t, am.cacheEntries, 1)
 }
+
+func Test_authMapCache_Delete(t *testing.T) {
+	fakeMap := &fakeAuthMap{
+		entries: map[authKey]authInfo{
+			{
+				localIdentity:  1,
+				remoteIdentity: 2,
+				remoteNodeID:   10,
+				authType:       policy.AuthTypeDisabled,
+			}: {
+				expiration: time.Now().Add(10 * time.Minute),
+			},
+		},
+	}
+	am := authMapCache{
+		logger:  logrus.New(),
+		authmap: fakeMap,
+		cacheEntries: map[authKey]authInfo{
+			{
+				localIdentity:  1,
+				remoteIdentity: 2,
+				remoteNodeID:   10,
+				authType:       policy.AuthTypeDisabled,
+			}: {
+				expiration: time.Now().Add(10 * time.Minute),
+			},
+			{
+				localIdentity:  3,
+				remoteIdentity: 2,
+				remoteNodeID:   10,
+				authType:       policy.AuthTypeDisabled,
+			}: {
+				expiration: time.Now().Add(10 * time.Minute),
+			},
+			{
+				localIdentity:  4,
+				remoteIdentity: 2,
+				remoteNodeID:   10,
+				authType:       policy.AuthTypeDisabled,
+			}: {
+				expiration: time.Now().Add(10 * time.Minute),
+			},
+		},
+	}
+
+	assert.Len(t, am.cacheEntries, 3)
+
+	err := am.Delete(authKey{
+		localIdentity:  1,
+		remoteIdentity: 2,
+		remoteNodeID:   10,
+		authType:       policy.AuthTypeDisabled,
+	})
+	assert.NoError(t, err)
+	assert.Len(t, am.cacheEntries, 2)
+
+	err = am.Delete(authKey{
+		localIdentity:  3,
+		remoteIdentity: 2,
+		remoteNodeID:   10,
+		authType:       policy.AuthTypeDisabled,
+	})
+	assert.NoError(t, err)
+	assert.Len(t, am.cacheEntries, 1) // Delete from cache
+
+	fakeMap.failDelete = true
+	err = am.Delete(authKey{
+		localIdentity:  4,
+		remoteIdentity: 2,
+		remoteNodeID:   10,
+		authType:       policy.AuthTypeDisabled,
+	})
+	assert.ErrorContains(t, err, "failed to delete auth entry from map: failed to delete entry")
+	assert.Len(t, am.cacheEntries, 1) // Technical error -> keep in cache
+}
+
+func Test_authMapCache_DeleteIf(t *testing.T) {
+	fakeMap := &fakeAuthMap{
+		entries: map[authKey]authInfo{
+			{
+				localIdentity:  1,
+				remoteIdentity: 2,
+				remoteNodeID:   10,
+				authType:       policy.AuthTypeDisabled,
+			}: {
+				expiration: time.Now().Add(10 * time.Minute),
+			},
+		},
+	}
+	am := authMapCache{
+		logger:  logrus.New(),
+		authmap: fakeMap,
+		cacheEntries: map[authKey]authInfo{
+			{
+				localIdentity:  1,
+				remoteIdentity: 2,
+				remoteNodeID:   10,
+				authType:       policy.AuthTypeDisabled,
+			}: {
+				expiration: time.Now().Add(10 * time.Minute),
+			},
+			{
+				localIdentity:  3,
+				remoteIdentity: 2,
+				remoteNodeID:   10,
+				authType:       policy.AuthTypeDisabled,
+			}: {
+				expiration: time.Now().Add(10 * time.Minute),
+			},
+			{
+				localIdentity:  4,
+				remoteIdentity: 2,
+				remoteNodeID:   10,
+				authType:       policy.AuthTypeDisabled,
+			}: {
+				expiration: time.Now().Add(10 * time.Minute),
+			},
+		},
+	}
+
+	assert.Len(t, am.cacheEntries, 3)
+
+	err := am.DeleteIf(func(key authKey, info authInfo) bool {
+		return key.localIdentity == 1 || key.localIdentity == 3
+	})
+	assert.NoError(t, err)
+	assert.Len(t, am.cacheEntries, 1)
+
+	fakeMap.failDelete = true
+	err = am.DeleteIf(func(key authKey, info authInfo) bool {
+		return key.localIdentity == 4
+	})
+	assert.ErrorContains(t, err, "failed to delete auth entry from map: failed to delete entry")
+	assert.Len(t, am.cacheEntries, 1)
+}

pkg/auth/manager_test.go

@@ -10,6 +10,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/cilium/ebpf"
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	"golang.org/x/exp/maps"
@@ -223,6 +224,10 @@ func (r *fakeAuthMap) Delete(key authKey) error {
 		return errors.New("failed to delete entry")
 	}
 
+	if _, ok := r.entries[key]; !ok {
+		return ebpf.ErrKeyNotExist
+	}
+
 	delete(r.entries, key)
 	return nil
 }