Policy track derived from rules better #34437
Merged
Conversation
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
|
/test |
Add MapStateEntry.DatapathAndDerivedFromEqual to be used in testing via mapState.Equal and mapState.Diff. This will help find bugs in properly tracking the DerivedFromRules field, which has so far been ignored in testing when using mapsState.Equal and mapState.Diff. Fix endpoint/redirect_test.go to expect only the contributing rule labels, as they are now tested for. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
lblWorldIP was not generated properly, it was missing the world label and hence it was not selected by the world selector. Use labels.GetCIDRLabels to get the full CIDR labels. Now that mapState.Equal cares about DerivedFromRules labels we need to expect them. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
0e626b3 to
d27026f
Compare
|
/test |
squeed
reviewed
Aug 21, 2024
squeed
reviewed
Aug 21, 2024
squeed
reviewed
Aug 21, 2024
|
Looks basically good. This makes the logic just a hair simpler, which I always appreciate. AFAICT we have all the cases covered, but a few more comments would be useful. |
Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
MapStateEntry metadata was improperly merged when an allow entry and deny entry were merged. Fix this by only merging entries when they both are either allow or deny entries. Add identical keys also when the entries are different regarding all other fields than just the owners. This way DerivedFromRules labels get merged also if the owning CachedSelectors happen to be the same but are used in rules with different labels. When inserting an allow entry as a deny due to covering deny rule, DerivedFromRules was based on the allow rule, when it should be a combination of all the covering deny rules instead. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
d27026f to
bb3e933
Compare
|
/test |
squeed
approved these changes
Aug 22, 2024
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.
MapStateEntry metadata was improperly merged when an allow entry and deny entry were merged. Fix this by only merging entries when they both are either allow or deny entries.
Add identical keys also when the entries are different regarding all other fields than just the owners. This way DerivedFromRules labels get merged also if the owning CachedSelectors happen to be the same but are used in rules with different labels.
When inserting an allow entry as a deny due to covering deny rule, DerivedFromRules was based on the allow rule, when it should be a combination of all the covering deny rules instead.
Add
MapStateEntry.DatapathAndDerivedFromEqualto be used in testing viamapState.EqualandmapState.Diff. This will help find bugs in properly tracking theDerivedFromRulesfield, which has so far been ignored in testing when usingmapsState.EqualandmapState.Diff.Without these changes the resulting
DerivedFromRuleslabels depended on the insertion order of MapState entries, causing intermittent test flakes due to essentially random iteration order of Go maps when test code was changed to care about the resultingDerivedFromRuleslabels by the 1st commit.