policy: Only record an old entry if needed

[ upstream commit 9916824 ]

Only record an old entry in ChangeState if it existed before this round
of changes. We do this by testing if the entry is already in Adds. If
not, then we record the old entry key and value. If the Adds entry
exists, however, this entry may have only been added on this round of
changes and we do not record the old value. This is safe due to the fact
that when the Adds entry is created, the Old value is stored before
adding the Adds entry, so for the first Adds entry the Old value does not
yet exist and will be added.

This removes extraneous Old entries that did not actually originally
exist. Before this ChangeState.Revert did restore an entry the should not
exists based on these extraneous Old entries.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>

pkg/policy/mapstate.go

@@ -229,9 +229,9 @@ func (keys MapState) addDependentOnEntry(owner Key, e MapStateEntry, dependent K
 // RemoveDependent removes 'key' from the list of dependent keys.
 // This is called when a dependent entry is being deleted.
 // If 'old' is not nil, then old value is added there before any modifications.
-func (keys MapState) RemoveDependent(owner Key, dependent Key, old MapState) {
+func (keys MapState) RemoveDependent(owner Key, dependent Key, changes ChangeState) {
 	if e, exists := keys[owner]; exists {
-		old.insertIfNotExists(owner, e)
+		changes.insertOldIfNotExists(owner, e)
 		e.RemoveDependent(dependent)
 		keys[owner] = e
 	}
@@ -379,7 +379,7 @@ func (keys MapState) addKeyWithChanges(key Key, entry MapStateEntry, changes Cha
 
 		// Save old value before any changes, if desired
 		if changes.Old != nil {
-			changes.Old.insertIfNotExists(key, oldEntry)
+			changes.insertOldIfNotExists(key, oldEntry)
 		}
 
 		// Compare for datapath equalness before merging, as the old entry is updated in
@@ -411,15 +411,15 @@ func (keys MapState) addKeyWithChanges(key Key, entry MapStateEntry, changes Cha
 func (keys MapState) deleteKeyWithChanges(key Key, owner MapStateOwner, changes ChangeState) {
 	if entry, exists := keys[key]; exists {
 		// Save old value before any changes, if desired
-		oldAdded := changes.Old.insertIfNotExists(key, entry)
+		oldAdded := changes.insertOldIfNotExists(key, entry)
 
 		if owner != nil {
 			// remove the contribution of the given selector only
 			if _, exists = entry.owners[owner]; exists {
 				// Remove the contribution of this selector from the entry
 				delete(entry.owners, owner)
 				if ownerKey, ok := owner.(Key); ok {
-					keys.RemoveDependent(ownerKey, key, changes.Old)
+					keys.RemoveDependent(ownerKey, key, changes)
 				}
 				// key is not deleted if other owners still need it
 				if len(entry.owners) > 0 {
@@ -441,7 +441,7 @@ func (keys MapState) deleteKeyWithChanges(key Key, owner MapStateOwner, changes
 			for owner := range entry.owners {
 				if owner != nil {
 					if ownerKey, ok := owner.(Key); ok {
-						keys.RemoveDependent(ownerKey, key, changes.Old)
+						keys.RemoveDependent(ownerKey, key, changes)
 					}
 				}
 			}
@@ -874,15 +874,20 @@ 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'
-func (keys MapState) insertIfNotExists(key Key, entry MapStateEntry) bool {
-	if keys != nil {
-		if _, exists := keys[key]; !exists {
+func (changes *ChangeState) insertOldIfNotExists(key Key, entry MapStateEntry) bool {
+	if changes == nil || changes.Old == nil {
+		return false
+	}
+	if _, exists := changes.Old[key]; !exists {
+		// Only insert the old entry if the entry was not first added on this round of
+		// changes.
+		if _, added := changes.Adds[key]; !added {
 			// new containers to keep this entry separate from the one that may remain in 'keys'
 			entry.DerivedFromRules = slices.Clone(entry.DerivedFromRules)
 			entry.owners = maps.Clone(entry.owners)
 			entry.dependents = maps.Clone(entry.dependents)
 
-			keys[key] = entry
+			changes.Old[key] = entry
 			return true
 		}
 	}

pkg/policy/mapstate_test.go

@@ -1845,9 +1845,11 @@ func (ds *PolicyTestSuite) TestMapState_AddVisibilityKeys(c *check.C) {
 		},
 	}
 	for _, tt := range tests {
-		old := make(MapState, len(tt.keys))
+		old := ChangeState{
+			Old: make(MapState, len(tt.keys)),
+		}
 		for k, v := range tt.keys {
-			old.insertIfNotExists(k, v)
+			old.insertOldIfNotExists(k, v)
 		}
 		changes := ChangeState{
 			Adds: make(Keys),
@@ -1859,13 +1861,13 @@ func (ds *PolicyTestSuite) TestMapState_AddVisibilityKeys(c *check.C) {
 		// Find new and updated entries
 		wantAdds := make(Keys)
 		wantOld := make(MapState)
-		for k, v := range old {
+		for k, v := range old.Old {
 			if _, ok := tt.keys[k]; !ok {
 				wantOld[k] = v
 			}
 		}
 		for k, v := range tt.keys {
-			if v2, ok := old[k]; ok {
+			if v2, ok := old.Old[k]; ok {
 				if equals, _ := checker.DeepEqual(v2, v); !equals {
 					if !v.DatapathEqual(&v2) {
 						wantAdds[k] = struct{}{}
@@ -2137,10 +2139,7 @@ func (ds *PolicyTestSuite) TestMapState_AccumulateMapChangesOnVisibilityKeys(c *
 			DNSUDPEgressKey(42): {},
 			DNSTCPEgressKey(42): {},
 		},
-		deletes: Keys{
-			// AddVisibilityKeys() returns overwritten entries in 'deletes'
-			DNSUDPEgressKey(42): {},
-		},
+		deletes: Keys{},
 	}, {
 		continued: true,
 		name:      "test-3b - egress HTTP proxy (incremental update)",