policy: Use 'insertOldIfNotExists() in addDependentOnEntry()

jrajahalme · jrajahalme · commit 137abf7b4b6a · 2024-10-02T16:34:25.000+02:00
Keep the oldest changed value in 'addDependentOnEntry()' by using
'insertOldIfNotExists()' rather than modifying 'changes.Old'
directly. Changes in tests are due to the way 'insertOldIfNotExists()'
filters out new entries (not adding an 'old' entry if the key exists in
'changes.Adds'). Test changes are in the 'deletes' field due to the test
internally setting a 'deletes' key for each key in 'changes.Old'.

Old key values are used when reverting a failed endpoint update, prior to
this fix that revert may have produced incorrect results.

Signed-off-by: Jarno Rajahalme &lt;jarno@isovalent.com&gt;
diff --git a/pkg/policy/mapstate.go b/pkg/policy/mapstate.go
@@ -638,9 +638,7 @@ func (ms *mapState) AddDependent(owner Key, dependent Key, changes ChangeState)
 // addDependentOnEntry adds 'dependent' to the set of dependent keys of 'e'.
 func (ms *mapState) addDependentOnEntry(owner Key, e MapStateEntry, dependent Key, changes ChangeState) {
 	if _, exists := e.dependents[dependent]; !exists {
-		if changes.Old != nil {
-			changes.Old[owner] = e
-		}
+		changes.insertOldIfNotExists(owner, e)
 		e.AddDependent(dependent)
 		ms.insert(owner, e)
 	}
@@ -830,9 +828,7 @@ func (ms *mapState) addKeyWithChanges(key Key, entry MapStateEntry, changes Chan
 		}
 
 		// Save old value before any changes, if desired
-		if changes.Old != nil {
-			changes.insertOldIfNotExists(key, oldEntry)
-		}
+		changes.insertOldIfNotExists(key, oldEntry)
 
 		// Compare for datapath equalness before merging, as the old entry is updated in
 		// place!
@@ -1376,8 +1372,11 @@ var visibilityDerivedFromLabels = labels.LabelArray{
 
 var visibilityDerivedFrom = labels.LabelArrayList{visibilityDerivedFromLabels}
 
-// insertIfNotExists only inserts `key=value` if `key` does not exist in keys already
-// returns 'true' if 'key=entry' was added to 'keys'
+// insertIfNotExists only inserts an entry in 'changes.Old' if 'key' does not exist in there already
+// and 'key' does not already exist in 'changes.Adds'. This prevents recording "old" values for
+// newly added keys. When an entry is updated, we are called before the key is added to
+// 'changes.Adds' so we'll record the old value as expected.
+// Returns 'true' if an old entry was added.
 func (changes *ChangeState) insertOldIfNotExists(key Key, entry MapStateEntry) bool {
 	if changes == nil || changes.Old == nil {
 		return false
diff --git a/pkg/policy/mapstate_test.go b/pkg/policy/mapstate_test.go
@@ -2494,10 +2494,7 @@ func TestMapState_AccumulateMapChangesOnVisibilityKeys(t *testing.T) {
 			ingressL3OnlyKey(236): {},
 			HttpIngressKey(236):   {},
 		},
-		deletes: Keys{
-			ingressL3OnlyKey(235): {}, // changed dependents
-			ingressL3OnlyKey(236): {}, // changed dependents
-		},
+		deletes: Keys{},
 	}, {
 		continued: true,
 		name:      "test-2b - Adding Bar also selecting 235",
@@ -2520,9 +2517,7 @@ func TestMapState_AccumulateMapChangesOnVisibilityKeys(t *testing.T) {
 			ingressL3OnlyKey(237): {},
 			HttpIngressKey(237):   {},
 		},
-		deletes: Keys{
-			ingressL3OnlyKey(237): {}, // changed dependents
-		},
+		deletes: Keys{},
 	}, {
 		continued: true,
 		name:      "test-2c - Deleting 235 from Foo, remains on Bar and no deletes",
