[v3.31] app-policy: match named ports against the IP+port set key - #13892
Open
dimitri-nicolo wants to merge 1 commit into
Open
Conversation
A policy rule that referenced a named port never matched in the app-policy checker (Dikastes), so an Allow rule fell through to the default deny and a Deny rule did nothing. The main dataplanes allowed the same flow. Felix does not hand out a set of port numbers for a named port. It resolves the name against every endpoint that declares it and emits an IP+port set (IPSetUpdate_IP_AND_PORT) whose members are "<IP>,<protocol>:<port>" - the set means "this port, on these endpoints". iptables/nftables match it with --match-set <set> src,src / dst,dst and the BPF dataplane builds the same (IP, protocol, port) key, both taken from the leg the rule names. The checker instead looked up the bare port number, e.g. "8080", which no member of such a set can equal, so the lookup always missed. Build the same key the other dataplanes use and look that up instead: matchPort and matchNotPort now take the leg's "<IP>,<protocol>:<port>" accessor, and requestCache memoizes the source-side key alongside the destination one it already had. sctp joins tcp and udp in protocolMapL4, since a named port can be declared on any of the three. TestMatchNamedPorts covers the reported case plus the wrong-endpoint, wrong-port, wrong-protocol, negated and source-leg variants. TestMatchPort, added by projectcalico#13408 while this change was open, keyed its named port set on the bare port number; it now uses an IP+port member and gives the flow an IP and protocol, as the checker looks the key up. Envoy's ext_authz adapter can only report tcp or udp, so protocolMap in the adapter stays as it is; protocolMapL4 is different, since the checker builds the key from whatever the Flow reports and Felix emits sctp members. The sctp entry is covered by TestMatchNamedPorts, TestMatchDstIPPortSetIds and ipProtoPortKey's own test. Backport to release-v3.31: this branch has no memoized request cache, so the "<IP>,<protocol>:<port>" key is built per call, as matchDstIPPortSetIds already did here. That function now shares the key builder. (cherry picked from commit 7753bba) Fixes projectcalico#13174
dimitri-nicolo
added
release-note-required
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved review comments remain, and the supplied assessments find the changes ready for approval.
Pull request overview
Fixes Dikastes named-port matching by using endpoint-specific IP/protocol/port keys, including SCTP support.
Changes:
- Adds shared source and destination key generation.
- Updates positive and negated named-port matching.
- Adds regression tests for named ports, protocols, and key formatting.
File summaries
| File | Description |
|---|---|
app-policy/checker/requestcache.go |
Builds source and destination IP-port keys. |
app-policy/checker/requestcache_test.go |
Tests key formatting. |
app-policy/checker/match.go |
Uses endpoint-specific keys for named-port matching. |
app-policy/checker/match_test.go |
Tests named-port, negation, source-leg, and SCTP behavior. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
CI triage — CalicoRecommendation: Re-run CI before merging — PR probably not at fault, but tests did not execute Unrelated failures
workflow_id: 86f8cf40-faf9-4589-8bf2-515d4f576b93 |
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
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.
Cherry-pick history
Backport note (release-v3.31): this branch predates the memoized request cache, so the change is adapted rather than a clean pick. The
<IP>,<protocol>:<port>key is built per call (asmatchDstIPPortSetIdsalready did here) through a sharedipProtoPortKey;matchPort/matchNotPorttake the leg's key accessor exactly as on master. Tests are the same as master minusTestMatchPort, which does not exist on this branch.Fixes #13174.
The problem
A policy rule that references a named port never matches in the app-policy checker (Dikastes). An
Allowrule therefore falls through to the default deny, and aDenyrule silently does nothing. The main dataplanes allow the same flow, so ALP and non-ALP disagree.Why
Felix does not hand out a set of port numbers for a named port. It resolves the name against every endpoint that declares it and emits an IP+port set (
IPSetUpdate_IP_AND_PORT) whose members look like10.0.0.1,tcp:8080— the set means "this port, on these endpoints" (felix/calc/rule_scanner.go,felix/labelindex/ipsetmember/ip_set_member.go).Every other dataplane matches it that way:
--match-set <set> src,src/dst,dst(felix/rules/policy.go)(IP, protocol, port)key from the leg the rule names (felix/bpf/polprog/pol_prog_builder.go)matchPort/matchNotPortinapp-policy/checker/match.goinstead looked up the bare port number, e.g."8080", which no member of such a set can equal. The lookup always missed.The change
matchPort/matchNotPorttake the leg's<IP>,<protocol>:<port>accessor and look that key up, matching what the other dataplanes do.requestCachememoizes the source-side key alongside the destination one it already had; both now share anipProtoPortKeyhelper.protocolMapL4gains132: "sctp". A named port can be declared on sctp, and the key would otherwise carry an empty protocol field. This closes the same gap formatchDstIPPortSetIds, which uses the same key.Testing
TestMatchNamedPortsis the reported case plus the wrong-endpoint, wrong-port, wrong-protocol, negated and source-leg variants, and the numeric-OR-named case. I confirmed it reproduces the bug: with the old bare-port lookup restored, three of its cases fail (dst named port matches…,negated dst named port excludes…,src named port matches on the source leg).TestMatchPort(added by #13408 while this PR was open) keyed its named port set on the bare port number; it now uses an IP+port member and gives the flow an IP and protocol.The sctp entry in
protocolMapL4is covered by an sctp named port inTestMatchNamedPorts, an sctp set inTestMatchDstIPPortSetIds, andTestIPProtoPortKey.go test ./app-policy/...passes.Release note: