pkg/allocator: Improve 'Key allocation attempt failed' handling for C…

cilium-dbg/cmd/preflight_identity_crd_migrate.go

@@ -132,7 +132,7 @@ func migrateIdentities(ctx hive.HookContext, clientset k8sClient.Clientset, reso
 		})
 
 		ctx, cancel := context.WithTimeout(ctx, opTimeout)
-		err := crdBackend.AllocateID(ctx, id, key)
+		_, err := crdBackend.AllocateID(ctx, id, key)
 		switch {
 		case err != nil && k8serrors.IsAlreadyExists(err):
 			alreadyAllocatedKeys[id] = key

pkg/allocator/allocator.go

@@ -202,11 +202,19 @@ type Backend interface {
 	// error in that case is not expected to be fatal. The actual ID is obtained
 	// by Allocator from the local idPool, which is updated with used-IDs as the
 	// Backend makes calls to the handler in ListAndWatch.
-	AllocateID(ctx context.Context, id idpool.ID, key AllocatorKey) error
+	// The implementation of the backend might return an AllocatorKey that is
+	// a copy of 'key' with an internal reference of the backend key or, if it
+	// doesn't use the internal reference of the backend key it simply returns
+	// 'key'. In case of an error the returned 'AllocatorKey' should be nil.
+	AllocateID(ctx context.Context, id idpool.ID, key AllocatorKey) (AllocatorKey, error)
 
 	// AllocateIDIfLocked behaves like AllocateID but when lock is non-nil the
 	// operation proceeds only if it is still valid.
-	AllocateIDIfLocked(ctx context.Context, id idpool.ID, key AllocatorKey, lock kvstore.KVLocker) error
+	// The implementation of the backend might return an AllocatorKey that is
+	// a copy of 'key' with an internal reference of the backend key or, if it
+	// doesn't use the internal reference of the backend key it simply returns
+	// 'key'. In case of an error the returned 'AllocatorKey' should be nil.
+	AllocateIDIfLocked(ctx context.Context, id idpool.ID, key AllocatorKey, lock kvstore.KVLocker) (AllocatorKey, error)
 
 	// AcquireReference records that this node is using this key->ID mapping.
 	// This is distinct from any reference counting within this agent; only one
@@ -464,6 +472,13 @@ type AllocatorKey interface {
 	// PutKeyFromMap stores the labels in v into the key to be used later. This
 	// is the inverse operation to GetAsMap.
 	PutKeyFromMap(v map[string]string) AllocatorKey
+
+	// PutValue puts metadata inside the global identity for the given 'key' with
+	// the given 'value'.
+	PutValue(key any, value any) AllocatorKey
+
+	// Value returns the value stored in the metadata map.
+	Value(key any) any
 }
 
 func (a *Allocator) encodeKey(key AllocatorKey) string {
@@ -530,7 +545,7 @@ func (a *Allocator) lockedAllocate(ctx context.Context, key AllocatorKey) (idpoo
 
 		if err = a.backend.AcquireReference(ctx, value, key, lock); err != nil {
 			a.localKeys.release(k)
-			return 0, false, false, fmt.Errorf("unable to create slave key '%s': %s", k, err)
+			return 0, false, false, fmt.Errorf("unable to create secondary key '%s': %s", k, err)
 		}
 
 		// mark the key as verified in the local cache
@@ -579,7 +594,7 @@ func (a *Allocator) lockedAllocate(ctx context.Context, key AllocatorKey) (idpoo
 		return 0, false, false, fmt.Errorf("Found master key after proceeding with new allocation for %s", k)
 	}
 
-	err = a.backend.AllocateIDIfLocked(ctx, id, key, lock)
+	key, err = a.backend.AllocateIDIfLocked(ctx, id, key, lock)
 	if err != nil {
 		// Creation failed. Another agent most likely beat us to allocting this
 		// ID, retry.
@@ -595,7 +610,7 @@ func (a *Allocator) lockedAllocate(ctx context.Context, key AllocatorKey) (idpoo
 		// exposed and may be in use by other nodes. The garbage
 		// collector will release it again.
 		releaseKeyAndID()
-		return 0, false, false, fmt.Errorf("slave key creation failed '%s': %s", k, err)
+		return 0, false, false, fmt.Errorf("secondary key creation failed '%s': %s", k, err)
 	}
 
 	// mark the key as verified in the local cache

pkg/allocator/allocator_test.go

@@ -60,12 +60,12 @@ func (d *dummyBackend) DeleteAllKeys(ctx context.Context) {
 	d.identities = map[idpool.ID]AllocatorKey{}
 }
 
-func (d *dummyBackend) AllocateID(ctx context.Context, id idpool.ID, key AllocatorKey) error {
+func (d *dummyBackend) AllocateID(ctx context.Context, id idpool.ID, key AllocatorKey) (AllocatorKey, error) {
 	d.mutex.Lock()
 	defer d.mutex.Unlock()
 
 	if _, ok := d.identities[id]; ok {
-		return fmt.Errorf("identity already exists")
+		return nil, fmt.Errorf("identity already exists")
 	}
 
 	d.identities[id] = key
@@ -74,10 +74,10 @@ func (d *dummyBackend) AllocateID(ctx context.Context, id idpool.ID, key Allocat
 		d.handler.OnAdd(id, key)
 	}
 
-	return nil
+	return key, nil
 }
 
-func (d *dummyBackend) AllocateIDIfLocked(ctx context.Context, id idpool.ID, key AllocatorKey, lock kvstore.KVLocker) error {
+func (d *dummyBackend) AllocateIDIfLocked(ctx context.Context, id idpool.ID, key AllocatorKey, lock kvstore.KVLocker) (AllocatorKey, error) {
 	return d.AllocateID(ctx, id, key)
 }
 
@@ -210,6 +210,14 @@ func (t TestAllocatorKey) PutKeyFromMap(m map[string]string) AllocatorKey {
 	panic("empty map")
 }
 
+func (t TestAllocatorKey) PutValue(key any, value any) AllocatorKey {
+	panic("not implemented")
+}
+
+func (t TestAllocatorKey) Value(any) any {
+	panic("not implemented")
+}
+
 func randomTestName() string {
 	return rand.RandomStringWithPrefix(testPrefix, 12)
 }

pkg/identity/key/global_identity.go

@@ -4,15 +4,24 @@
 package key
 
 import (
+	"maps"
 	"strings"
 
 	"github.com/cilium/cilium/pkg/allocator"
 	"github.com/cilium/cilium/pkg/labels"
 )
 
+const (
+	// MetadataKeyBackendKey is the key used to store the backend key.
+	MetadataKeyBackendKey = iota
+)
+
 // GlobalIdentity is the structure used to store an identity
 type GlobalIdentity struct {
 	labels.LabelArray
+
+	// metadata contains metadata that are stored for example by the backends.
+	metadata map[any]any
 }
 
 // GetKey encodes an Identity as string
@@ -32,13 +41,32 @@ func (gi *GlobalIdentity) GetAsMap() map[string]string {
 
 // PutKey decodes an Identity from its string representation
 func (gi *GlobalIdentity) PutKey(v string) allocator.AllocatorKey {
-	return &GlobalIdentity{labels.NewLabelArrayFromSortedList(v)}
+	return &GlobalIdentity{LabelArray: labels.NewLabelArrayFromSortedList(v)}
 }
 
 // PutKeyFromMap decodes an Identity from a map of key to value. Output
 // from GetAsMap can be parsed.
 // Note: NewLabelArrayFromMap will parse the ':' separated label source from
 // the keys because the source parameter is ""
 func (gi *GlobalIdentity) PutKeyFromMap(v map[string]string) allocator.AllocatorKey {
-	return &GlobalIdentity{labels.Map2Labels(v, "").LabelArray()}
+	return &GlobalIdentity{LabelArray: labels.Map2Labels(v, "").LabelArray()}
+}
+
+// PutValue puts metadata inside the global identity for the given 'key' with
+// the given 'value'.
+func (gi *GlobalIdentity) PutValue(key, value any) allocator.AllocatorKey {
+	newMap := map[any]any{}
+	if gi.metadata != nil {
+		newMap = maps.Clone(gi.metadata)
+	}
+	newMap[key] = value
+	return &GlobalIdentity{
+		LabelArray: gi.LabelArray,
+		metadata:   newMap,
+	}
+}
+
+// Value returns the value stored in the metadata map.
+func (gi *GlobalIdentity) Value(key any) any {
+	return gi.metadata[key]
 }

pkg/k8s/identitybackend/identity.go

@@ -16,6 +16,7 @@ import (
 	"k8s.io/client-go/tools/cache"
 
 	"github.com/cilium/cilium/pkg/allocator"
+	cacheKey "github.com/cilium/cilium/pkg/identity/key"
 	"github.com/cilium/cilium/pkg/idpool"
 	k8sConst "github.com/cilium/cilium/pkg/k8s/apis/cilium.io"
 	v2 "github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2"
@@ -88,7 +89,8 @@ func sanitizeK8sLabels(old map[string]string) (selected, skipped map[string]stri
 // AllocateID will create an identity CRD, thus creating the identity for this
 // key-> ID mapping.
 // Note: the lock field is not supported with the k8s CRD allocator.
-func (c *crdBackend) AllocateID(ctx context.Context, id idpool.ID, key allocator.AllocatorKey) error {
+// Returns an allocator key with the cilium identity stored in it.
+func (c *crdBackend) AllocateID(ctx context.Context, id idpool.ID, key allocator.AllocatorKey) (allocator.AllocatorKey, error) {
 	selectedLabels, skippedLabels := sanitizeK8sLabels(key.GetAsMap())
 	log.WithField(logfields.Labels, skippedLabels).Info("Skipped non-kubernetes labels when labelling ciliumidentity. All labels will still be used in identity determination")
 
@@ -100,11 +102,14 @@ func (c *crdBackend) AllocateID(ctx context.Context, id idpool.ID, key allocator
 		SecurityLabels: key.GetAsMap(),
 	}
 
-	_, err := c.Client.CiliumV2().CiliumIdentities().Create(ctx, identity, metav1.CreateOptions{})
-	return err
+	ci, err := c.Client.CiliumV2().CiliumIdentities().Create(ctx, identity, metav1.CreateOptions{})
+	if err != nil {
+		return nil, err
+	}
+	return key.PutValue(cacheKey.MetadataKeyBackendKey, ci), nil
 }
 
-func (c *crdBackend) AllocateIDIfLocked(ctx context.Context, id idpool.ID, key allocator.AllocatorKey, lock kvstore.KVLocker) error {
+func (c *crdBackend) AllocateIDIfLocked(ctx context.Context, id idpool.ID, key allocator.AllocatorKey, lock kvstore.KVLocker) (allocator.AllocatorKey, error) {
 	return c.AllocateID(ctx, id, key)
 }
 
@@ -136,13 +141,18 @@ func (c *crdBackend) AcquireReference(ctx context.Context, id idpool.ID, key all
 		return err
 	}
 	if !exists {
-		return fmt.Errorf("identity (id:%q,key:%q) does not exist", id, key)
+		// fall back to the key stored in the allocator key. If it's not present
+		// then return the error.
+		ci, ok = key.Value(cacheKey.MetadataKeyBackendKey).(*v2.CiliumIdentity)
+		if !ok {
+			return fmt.Errorf("identity (id:%q,key:%q) does not exist", id, key)
+		}
 	}
-	ci = ci.DeepCopy()
 
 	ts, ok = ci.Annotations[HeartBeatAnnotation]
 	if ok {
 		log.WithField(logfields.Identity, ci).Infof("Identity marked for deletion (at %s); attempting to unmark it", ts)
+		ci = ci.DeepCopy()
 		delete(ci.Annotations, HeartBeatAnnotation)
 		_, err = c.Client.CiliumV2().CiliumIdentities().Update(ctx, ci, metav1.UpdateOptions{})
 		if err != nil {
@@ -183,7 +193,7 @@ func (c *crdBackend) UpdateKey(ctx context.Context, id idpool.ID, key allocator.
 
 	if reliablyMissing {
 		// Recreate a missing master key
-		if err = c.AllocateID(ctx, id, key); err != nil {
+		if _, err = c.AllocateID(ctx, id, key); err != nil {
 			return fmt.Errorf("Unable recreate missing CRD identity %q->%q: %s", key, id, err)
 		}
 		return nil

pkg/kvstore/allocator/allocator.go

@@ -123,29 +123,29 @@ func (k *kvstoreBackend) DeleteAllKeys(ctx context.Context) {
 }
 
 // AllocateID allocates a key->ID mapping in the kvstore.
-func (k *kvstoreBackend) AllocateID(ctx context.Context, id idpool.ID, key allocator.AllocatorKey) error {
+func (k *kvstoreBackend) AllocateID(ctx context.Context, id idpool.ID, key allocator.AllocatorKey) (allocator.AllocatorKey, error) {
 	// create /id/<ID> and fail if it already exists
 	keyPath := path.Join(k.idPrefix, id.String())
 	keyEncoded := []byte(k.backend.Encode([]byte(key.GetKey())))
 	success, err := k.backend.CreateOnly(ctx, keyPath, keyEncoded, false)
 	if err != nil || !success {
-		return fmt.Errorf("unable to create master key '%s': %s", keyPath, err)
+		return nil, fmt.Errorf("unable to create master key '%s': %s", keyPath, err)
 	}
 
-	return nil
+	return key, nil
 }
 
 // AllocateID allocates a key->ID mapping in the kvstore.
-func (k *kvstoreBackend) AllocateIDIfLocked(ctx context.Context, id idpool.ID, key allocator.AllocatorKey, lock kvstore.KVLocker) error {
+func (k *kvstoreBackend) AllocateIDIfLocked(ctx context.Context, id idpool.ID, key allocator.AllocatorKey, lock kvstore.KVLocker) (allocator.AllocatorKey, error) {
 	// create /id/<ID> and fail if it already exists
 	keyPath := path.Join(k.idPrefix, id.String())
 	keyEncoded := []byte(k.backend.Encode([]byte(key.GetKey())))
 	success, err := k.backend.CreateOnlyIfLocked(ctx, keyPath, keyEncoded, false, lock)
 	if err != nil || !success {
-		return fmt.Errorf("unable to create master key '%s': %s", keyPath, err)
+		return nil, fmt.Errorf("unable to create master key '%s': %s", keyPath, err)
 	}
 
-	return nil
+	return key, nil
 }
 
 // AcquireReference marks that this node is using this key->ID mapping in the kvstore.

pkg/kvstore/allocator/allocator_test.go

@@ -87,6 +87,14 @@ func (t TestAllocatorKey) PutKeyFromMap(m map[string]string) allocator.Allocator
 	panic("empty map")
 }
 
+func (t TestAllocatorKey) PutValue(key any, value any) allocator.AllocatorKey {
+	panic("not implemented")
+}
+
+func (t TestAllocatorKey) Value(any) any {
+	panic("not implemented")
+}
+
 func randomTestName() string {
 	return rand.RandomStringWithPrefix(testPrefix, 12)
 }