Compile dikastes/collector policies once instead of interpreting per flow (rebase of #13267) - #13898
Draft
dimitri-nicolo wants to merge 3 commits into
Draft
Conversation
The match functions hold the flow's net.IP but IPSet.Contains only takes a string, so NET sets re-parse the address on every rule that references an IP set. Add an IPAddrSet fast path (ContainsIP) implemented by ipNetSet, memoize the parsed per-flow IPs in requestCache, and route address lookups through it. The parsed-IP memoization tracks fetched-ness explicitly rather than using nil as its "not fetched yet" sentinel: nil is also a valid value (non-IP connections, e.g. pipes), and such flows would otherwise call back into the Flow implementation on every access. BenchmarkEvaluateBaselinePolicyScale/MissingSets (-cpu 1), measured before cheapest-first criterion ordering (projectcalico#13408) landed: 3.95ms -> 3.25ms per evaluation, 5247 -> 499 allocs/op, 108KB -> 33KB/op. Squash of the first, fourth and fifth commits of projectcalico#13267 (15f7ca0, 4418393, 9ac0d63), rebased onto master after projectcalico#13408, projectcalico#13410 and projectcalico#13416.
…flow checker.Evaluate walked every criterion of every rule for every flow, re-reading the large heap-scattered proto.Rule structs and recomputing rule-constant values each time (action enum, namespace-match inputs, selector/CIDR/protocol parses, IP set ID lookups). At the scale seen in a large production deployment (294 policies x 68 rules, ~3.7k IP sets) one evaluation cost milliseconds and re-warned 'IPSet not found' per rule per flow, saturating the felix collector. Compile each policy/profile once, when the policy store applies its update: per-rule slices of matcher closures covering only the criteria the rule uses, over pre-resolved values (IP set objects, parsed selectors and CIDRs, flattened port ranges, resolved protocol numbers, precomputed namespace matches, parsed actions). The matchers are emitted in the same cheapest-first order the interpreted match() evaluates its criteria in (projectcalico#13408): protocol, ports, CIDRs, IP sets, identity, HTTP last so that a malformed request path only fails rules that otherwise apply. The store owns the compiled artifacts, so they die with the double-buffered store on resync; a reverse index (IP set ID -> referencing policies) recompiles just the affected policies when a full IPSetUpdate replaces a set's object (deltas mutate the object in place and need nothing). The compiler is injected into the store by the two wiring sites (dikastes and the felix collector) to keep the package dependency checker -> policystore one-way. The interpreted path remains the reference implementation and the fallback: policies with no compiled entry (no compiler configured, compile failure, or the CALICO_DISABLE_POLICY_COMPILATION kill switch) are interpreted per flow as before, and the compiled path defers to it at debug log level so per-criterion debug logging is unchanged. The tier walk is shared by both engines, so the staged-policy scope (projectcalico#13416) applies identically: under EnforcedOnly staged policies are skipped and a staged-only tier contributes no end-of-tier action, and a policy missing from the store fails the evaluation closed whichever engine would have evaluated it. TestCompiledPolicyEquivalence asserts both engines return identical (action, index) across the criterion matrix, and the existing checkStore tests run against both engines. A missing IP set now warns once at compile time instead of once per rule per flow. Endpoints are compiled too: their tiers' policy references and their profile references become slices, index-parallel to the endpoint's own, that evaluation indexes directly instead of hashing a three-string policy ID per policy per flow. A PolicySlot indirection sits between the slice and the compiled policy so that recompiling a policy publishes through the slot and leaves compiled endpoints untouched; without it one update to an all-endpoints policy would rebuild every endpoint on the node. Endpoints are keyed by the identity of the endpoint object, which is what evaluation has to hand; a stale copy misses and falls back to the by-ID lookup. The remaining per-evaluation allocations are removed as well: the log level is tested once per Evaluate rather than per policy (boxing an int above 255 allocates); the tier default RuleID is precomputed per tier and direction and the 'no active profiles' RuleIDs built once; actionFromString and ruleActionFromStr compare with EqualFold instead of building a map and lowercasing; the requestCache is recycled through a sync.Pool (it escapes because the compiled matchers take it through a func value); Evaluate appends the trace to a caller-supplied slice, which the felix collector reuses across flows; and the matched rule's trace entry is memoized on the compiled rule on first use (building one per rule eagerly would cost megabytes for entries that are almost never reached), published atomically since evaluations read it concurrently under the store's read lock. Squash of the second, third and sixth to eleventh commits of projectcalico#13267 (c855bb3, e6b5b7e, e671083, c3fb6ca, 16b8eb8, 26d50b8, 64970c9, 411f5f4), rebased onto master after projectcalico#13408, projectcalico#13410 and projectcalico#13416.
… staged scope The compiled matchers were reconciled with projectcalico#13408's cheapest-first criterion order and projectcalico#13416's staged-policy scope while rebasing projectcalico#13267. Pin both with tests that fail if either drifts again: - TestCompiledPolicyEquivalence gains an ICMP flow (port 0, so the IP+port keys carry "icmp:0"), an IPv6 flow through the NET set's v6 prefix, named port sets keyed on the destination leg's "<IP>,<protocol>:<port>" (projectcalico#13174), port ranges combined with named sets, wide and multi-range negations, a protocol combined with ports and with HTTP criteria, and a rule that uses every criterion class at once. - TestCheckTiersPolicyScope, TestCheckStoreReportsWhichPolicyIsMissing, TestEvaluateRecordsStagedPolicyInPendingTraceOnly and TestMalformedHTTPPathOnlyFailsRulesThatReachIt run both engines and assert they agree, so the scope handling, the fail-closed missing policy and the protocol-before-HTTP ordering are checked on the compiled path too. - BenchmarkEvaluateEgressAllowListCompiled measures the egress allow-list fixture from projectcalico#13408 with compiled policies, next to the interpreted one.
dimitri-nicolo
added
release-note-required
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
3 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.
Description
Rebase of #13267 (Shaun Crampton's "Compile dikastes/collector policies once instead of interpreting per flow") onto current master, opened as a draft for the collector policy-evaluation work in CORE-13316. Shaun's two logical changes keep his authorship; the eleven original commits are squashed into two because seven of them rewrote
check.go, which master had since rewritten twice (#13416 scope and error signature, #13408 cheapest-first), and a commit-by-commit rebase would have resolved the same conflict seven times. The two squashed commits reproduce the #13267 head tree byte for byte before the rebase; the messages name the source SHAs. A third commit adds equivalence tests for the behaviours master gained in the meantime. Shaun: if you would rather pull this into your branch and keep #13267 as the PR, that works just as well; this is here so the epic has something to measure against.What the change does (unchanged from #13267): a parsed-IP fast path for NET IP sets (
IPAddrSet.ContainsIP) with the flow's parsed addresses memoised inrequestCache; and per-policy compilation, in which the policy store compiles each policy and profile when it applies the update, through aPolicyCompilerinjected at the two wiring sites (dikastes and the felix collector). A compiled policy is a per-rule slice of matcher closures covering only the criteria the rule uses, over values resolved once at compile time; a reverse index recompiles exactly the policies affected when an IP set is replaced.What the rebase reconciled (please look at these first):
check.go—Evaluate(scope, dir, store, ep, flow, traceBuf)merges Make the staged-policy scope of ALP policy evaluation explicit #13416's scope and error return with the PR's reusable trace buffer. The tier walk is shared by both engines and keeps everything from Make the staged-policy scope of ALP policy evaluation explicit #13416 above the engine dispatch: theEnforcedOnlystaged skip, thepoliciesInScope > 0gate on the end-of-tier action, and the fail-closedINTERNALfor a policy missing from the store.checkTierPolicyreturns(action, index, found): the compiled slot when present, else the stored policy interpreted, elsefound=falseand the walk fails closed. Confirm you are happy that "compiled entry present" implies "policy present" (the store empties a slot on remove and on a nil-policy update).compile.go—appendRuleMatchersemits matchers in Evaluate policy rule criteria cheapest-first in Dikastes #13408's cheapest-first order (protocol, source ports, destination ports, source net, destination net, source IP sets, destination IP sets, destination IP+port sets, source identity, destination identity, HTTP last so the malformed-path panic is confined identically), and named-port sets are looked up by the"<IP>,<protocol>:<port>"key introduced by Checker can not evaluate named ports #13174. These two are the rebase's reconciliation, not Shaun's code.checker.Evaluatenow takes the trace buffer as a sixth parameter; the only caller isfelix/collector. If the buffer is not wanted it can be dropped without touching the engine. (app-policy/checker: cache pending-policy verdicts per policy store generation #13897 on the same epic also wrapsEvaluate, with a verdict cache; whichever lands second rebases.)tierDefaultRuleIDmemo applies only when the first in-scope policy is the tier's first; a staged policy first underEnforcedOnlyfalls back to building the RuleID. Covered byTestCheckTiersPolicyScopeon both engines.IPSet not found, bad CIDR or selector) use plainlog.Warn, not the rate-limited loggers; bounded per compile, but a decision to make.Numbers —
go test ./app-policy/checker/ -run '^$' -bench BenchmarkEvaluate -benchmem -benchtime 20x -cpu 1, min of three runs on one laptop core. These supersede the figures in #13267's description, which predate cheapest-first.Compiled egress cost is 18 / 33 / 29 ns per rule against 81 / 106 / 109 on master. The interpreted path on this branch is within noise of master.
Testing
The third commit extends
TestCompiledPolicyEquivalence(ICMP flow, IPv6 through a NET set, named-port sets on the destination leg and the wrong leg, ranges plus named sets, wide negations, protocol plus ports, protocol plus HTTP, an all-criteria rule) and runsTestCheckTiersPolicyScope,TestCheckStoreReportsWhichPolicyIsMissing,TestEvaluateRecordsStagedPolicyInPendingTraceOnlyandTestMalformedHTTPPathOnlyFailsRulesThatReachIton both engines, and addsBenchmarkEvaluateEgressAllowListCompiled.Release note:
AI assistance: The rebase, conflict resolution and added tests were done with Claude Code (Claude Fable 5.1); the engine is Shaun's work from #13267. The author reviewed the reconciled files and ran the tests and benchmarks above.
By opening this PR you take responsibility for every line in it, and you agree to explain the change yourself during review rather than routing review comments back through an agent. See AI_POLICY.md.
🤖 Generated with Claude Code