Policy fix revert by only recording an "old" value if it existed before the changes#29162
Merged
jrajahalme merged 3 commits intocilium:mainfrom Nov 22, 2023
Merged
Policy fix revert by only recording an "old" value if it existed before the changes#29162jrajahalme merged 3 commits intocilium:mainfrom
jrajahalme merged 3 commits intocilium:mainfrom
Conversation
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>
909f73f to
faeb621
Compare
Member
Author
|
/test |
bimmlerd
reviewed
Nov 14, 2023
Member
bimmlerd
left a comment
There was a problem hiding this comment.
drive-by comment - lgtm otherwise
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>
faeb621 to
1ef653e
Compare
Member
Author
|
/test |
nebril
approved these changes
Nov 14, 2023
bimmlerd
approved these changes
Nov 14, 2023
aditighag
approved these changes
Nov 21, 2023
Member
aditighag
left a comment
There was a problem hiding this comment.
Approving pkg/endpoint/bpf.go changes for my codeowners.
This was referenced Nov 29, 2023
Contributor
|
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. |
Member
|
This PR is already part of 1.15 |
1 task
Member
Author
|
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. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.