Policy fix revert by only recording an "old" value if it existed before the changes #29162
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jrajahalme
added
kind/bug
Simplify ChangeState by changing 'Old' back to a 'map[Key]MapStateEntry'. This is used only for reverting and in that case there is no need to optimize scanning deny entries. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
jrajahalme
force-pushed
the
policy-fix-revert
branch
from
909f73f
to
faeb621
Compare
|
/test |
bimmlerd
reviewed
Nov 14, 2023
Add a Diff member function so that test failures can report the difference between the obtained and expected MapState. Takes in an unused '*testing.T' to make sure this is only used in testing. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
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>
jrajahalme
force-pushed
the
policy-fix-revert
branch
from
faeb621
to
1ef653e
Compare
|
/test |
nebril
approved these changes
Nov 14, 2023
bimmlerd
approved these changes
Nov 14, 2023
aditighag
approved these changes
Nov 21, 2023
maintainer-s-little-helper
bot
added
the
ready-to-merge
This was referenced Nov 29, 2023
joamaki
added
the
backport/author
|
Marking with backport/author as the backports to v1.12, v1.13 and v1.14 are non-trivial and likely require #28352 to be backported first. |
jrajahalme
added
needs-backport/1.15
aanm
removed
the
needs-backport/1.15
|
This PR is already part of 1.15 |
1 task
jrajahalme
added
backport-pending/1.14
|
Looked into backporting this to 1.13 and 1.12, and there are a lot of non-backported dependencies that make the backport somewhat risky. Since we don't know of any reports of actual problems cause by the issue fixed here, I'll drop the 1.13 and 1.12 backports for now. |
jrajahalme
removed
needs-backport/1.12
needs-backport/1.13
github-actions
bot
added
backport-done/1.14
julianwiedmann
added
affects/v1.12
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Only record an old entry in
ChangeStateif it existed before this round of changes. We do this by testing if the entry is already inAdds. If not, then we record the old entry key and value. If theAddsentry 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 theAddsentry is created, theOldvalue is stored before adding theAddsentry, so for the firstAddsentry theOldvalue does not yet exist and will be added.This removes extraneous
Oldentries that did not actually originally exist. Before this changeChangeState.Revertcould have restored entries the should not exists, based on these extraneousOldentries.Note that the change in
TestMapState_AccumulateMapChangesOnVisibilityKeyson the 3rd commit shows the effect of this change. Previously the test resulted in an entry inChangeState.Oldthat did not exist before the changes started, but was recoded due to an entry being first added and then modified within the same set of changes.