-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
backport (v1.13): policy: Fix Deny Precedence Bug #25427
backport (v1.13): policy: Fix Deny Precedence Bug #25427
Conversation
/ci-e2e-1.13 |
k8s-1.24-kernel-4.19 hit a known flake: #25255 |
[ upstream commit c9f0def ] - 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>
[ upstream commit 52ace8e ] Marco Iorio reports that with previous code, Cilium could crash at runtime after importing a network policy, with the following error printed to the logs: fatal error: concurrent map read and map write The path for this issue is printed also in the logs, with the following call stack: pkg/policy.(*SelectorCache).GetLabels(...) pkg/policy.(*MapStateEntry).getNets(...) pkg/policy.entryIdentityIsSupersetOf(...) pkg/policy.MapState.denyPreferredInsertWithChanges(...) pkg/policy.MapState.DenyPreferredInsert(...) pkg/policy.(*EndpointPolicy).computeDirectionL4PolicyMapEntries(...) pkg/policy.(*EndpointPolicy).computeDesiredL4PolicyMapEntries(...) pkg/policy.(*selectorPolicy).DistillPolicy(...) pkg/policy.(*cachedSelectorPolicy).Consume(...) pkg/endpoint.(*Endpoint).regeneratePolicy(...) ... Upon further inspection, this call path is not grabbing the SelectorCache lock at any point. If we check all of the incoming calls to this function, we can see multiple higher level functions calling into this function. The following tree starts from the deepest level of the call stack and increasing indentation represents one level higher in the call stack. INCOMING CALLS - f GetLabels github.com/cilium/cilium/pkg/policy • selectorcache.go - f getNets github.com/cilium/cilium/pkg/policy • mapstate.go - f entryIdentityIsSupersetOf github.com/cilium/cilium/pkg/policy • mapstate.go - f denyPreferredInsertWithChanges github.com/cilium/cilium/pkg/policy • mapstate.go - f DenyPreferredInsert github.com/cilium/cilium/pkg/policy • mapstate.go - f computeDirectionL4PolicyMapEntries github.com/cilium/cilium/pkg/policy • resolve.go - f computeDesiredL4PolicyMapEntries github.com/cilium/cilium/pkg/policy • resolve.go + f DistillPolicy github.com/cilium/cilium/pkg/policy • resolve.go <--- No SelectorCache lock - f DetermineAllowLocalhostIngress github.com/cilium/cilium/pkg/policy • mapstate.go + f DistillPolicy github.com/cilium/cilium/pkg/policy • resolve.go <--- No SelectorCache lock - f consumeMapChanges github.com/cilium/cilium/pkg/policy • mapstate.go + f ConsumeMapChanges github.com/cilium/cilium/pkg/policy • resolve.go <--- Already locks the SelectorCache Read the above tree as "GetLabels() is called by getNets()", "getNets() is called by entryIdentityIsSupersetOf()", and so on. Siblings at the same level of indent represent alternate callers of the function that is one level of indentation less in the tree, ie DenyPreferredInsert() and consumeMapChanges() both call denyPreferredInsertWithChanges(). As annotated above, we see that calls through DistillPolicy() do not grab the SelectorCache lock. Given that ConsumeMapChanges() grabs the SelectorCache lock, we cannot introduce a new lock acquisition in any descendent function, otherwise it would introduce a deadlock in goroutines that follow that call path. This provides us the option to lock at some point from the sibling of consumeMapChanges() or higher in the call stack. Given that the ancestors of DenyPreferredInsert() are all from DistillPolicy(), we can amortize the cost of grabbing the SelectorCache lock by grabbing it once for the policy distillation phase rather than putting the lock into DenyPreferredInsert() where the SelectorCache could be locked and unlocked for each map state entry. Future work could investigate whether these call paths could make use of the IdentityAllocator's cache of local identities for the GetLabels() call rather than relying on the SelectorCache, but for now this patch should address the immediate locking issue that triggers agent crashes. CC: Nate Sweet <nathanjsweet@pm.me> Fixes: c9f0def ("policy: Fix Deny Precedence Bug") Reported-by: Marco Iorio <marco.iorio@isovalent.com> Co-authored-by: Chris Tarazi <chris@isovalent.com> Signed-off-by: Joe Stringer <joe@cilium.io> Signed-off-by: Nate Sweet <nathanjsweet@pm.me>
[ upstream commit bfbe5a2 ] The policyIdentityLabelLookup wrapper for Endpoint implements the GetLabels interface method. This is necessary for the constructing the MapState of the policy engine. This implementation incorrectly did not check if the identity returned by LookupIdentityByID was nil. This fixes this bug, which heretofore has not caused any issues. Signed-off-by: Nate Sweet <nathanjsweet@pm.me>
1e929aa
to
08f4af6
Compare
rebased onto latest v1.13 to apply fix for |
/test-backport-1.13 |
/ci-e2e-1.13 |
/ci-aks-1.13 |
ConformanceAKS - test passed but cleanup failed: #20887
k8s-1.24-kernel-4.19 same as above lots of flakiness from archive.ubuntu.com today |
Thanks @thorn3r for documenting the test failures.
Given that 3 Jenkins jobs didn't even start, I would like a confirmation from the author of the backports PR @nathanjsweet whether it's okay to ignore the failures. There are multiple Jenkins jobs that test different combinations of k8s and kernel versions, and some of the jobs ran successfully, but I don't have full context for the PR changes to make a judgement call around test coverage. |
This is mergeable. This PR cannot be affected by K8s version or Linux version changes. |
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 topkg/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. Thusl4.ToMapState
needs access to the identity allocater to lookup "random" identity labels.Additional commit from @joestringer to fix concurrent access to the SelectorCache, a bug the original work introduced.
Additional commit from @nathanjsweet to prevent a potential nil pointer dereference in the endpoint code.