policy: Fix Deny Precedence Bug #22966
Conversation
30664b1 to
37456cf
Compare
|
/test |
37456cf to
7b9f02f
Compare
There was a problem hiding this comment.
I think that this broad approach should work. There's one main concern I have with the logic to pull the CIDR from the identity labels below. A lot of the comments are a scattering of random typo fixes and proposed comment updates just to try to help the readability in this area of the code. Some of the comments date from the previous iteration of the change, as I didn't realize you were updating the PR concurrently.
We'll also want to delete the limitation from the docs as part of this PR after the following PR is merged: #23095
1925ed6 to
cecfad7
Compare
|
Commit 8ea87df does not contain "Signed-off-by". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
8ea87df to
9cf5315
Compare
|
Commit 9cf5315f7e38d5c36a4f60f34958840e11a283e2 does not contain "Signed-off-by". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
9cf5315 to
e3b2b58
Compare
|
/test |
899a9c6 to
336e5ce
Compare
|
/test |
|
CI is looking good 🎉 |
There was a problem hiding this comment.
The way in which the internal loops in denyPreferredInsertWithChanges() are flipped, while simplifying the logic, make it super hard to track what changed and what did not change. It would have been beneficial to have a refactoring commit flipping the looping without any functional changes first, then have the functional changes in a separate commit.
|
@nathanjsweet Do you wish to address the comment nits from Jarno before we merge? I see that CI is green, so if you only do a comment edit I will be OK to merge as-is without re-running the CI. |
- Add Tests for Deny Precedence Bug: Currently, when a broad "Deny" policy is paired with a specific "Unmanaged CIDR" policy, then the "Unmanaged CIDR" policy will still be inserted into the policy map for an endpoint. This results in "Deny" policies not always taking precedence over "Allow" policies. This test confirms the bugs existence. - Fix Deny Precedence Bug: When the policy map state is created CIDRs are now checked against one another to ensure that deny-rules that supersede allow-rules when they should. `DenyPreferredInsert` has been refactored to use utility methods that make the complex boolean logic of policy precedence more atomic. Add `NetsContainsAny` method to `pkg/ip` to compare cases where one set of networks conatins or is equal to any network in another set. - endpoint: Add policy.Identity Implementation A `policy.Identity` implementation is necessary for the incremental update to the endpoint's policy map that can occur with L7 changes. Valid deny-policy entries may prohibit these L7 changes based on CIDR rules, which are only obtainable by looking up all potentially conflicting policies' labels. Thus `l4.ToMapState` needs access to the identity allocater to lookup "random" identity labels. Signed-off-by: Nate Sweet <nathanjsweet@pm.me>
336e5ce to
c0c815e
Compare
|
FYI, #24322 was merged which was introduced by this PR. |
In L4Filter.ToMapState a reference to the SelectorCache is passed down to denyPreferredInsertWithChanges, in order to get the most specific CIDR for a given identity with GetNetsLocked. GetNetsLocked requires the SelectorCache to be read-locked. denyPreferredInsertWithChanges is also called from EndpointPolicy.ConsumeMapChanges, where the SelectorCache must already be locked to avoid concurrent identity updates. Instead, in the L4Filter.ToMapState, it is possible to lock the SelectorCache just before its usage in GetNetsLocked. Narrowing the critical section reduces the contention on the lock, hopefully improving concurrency. A SelectorCacheWrapper wraps a reference to a SelectorCache and overloads the GetNetsLocked in order to lock the cache for the execution of the method. The new type satisfies the policy.Identities interface and can be used interchangeably with the wrapped SelectorCache. This allows to remove the locking of the SelectorCache for the entire duration of selectorPolicy.DistillPolicy and L4DirectionPolicy.updateRedirects. Related: cilium#24322 Related: cilium#22966 Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
When the policy map state is created CIDRs
are now checked against one another to ensure
that deny-rules that supersede allow-rules
when they should.
Fixes: #15198