Skip to content

[v3.32] app-policy: match named ports against the IP+port set key - #13891

Open
dimitri-nicolo wants to merge 1 commit into
projectcalico:release-v3.32from
dimitri-nicolo:auto-pick-of-#13668-upstream-release-v3.32
Open

dimitri-nicolo wants to merge 1 commit into
projectcalico:release-v3.32from
dimitri-nicolo:auto-pick-of-#13668-upstream-release-v3.32

Conversation

@dimitri-nicolo

Copy link
Copy Markdown
Contributor

Cherry-pick history

Fixes #13174.

The problem

A policy rule that references a named port never matches in the app-policy checker (Dikastes). An Allow rule therefore falls through to the default deny, and a Deny rule 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 like 10.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:

  • iptables/nftables render --match-set <set> src,src / dst,dst (felix/rules/policy.go)
  • BPF builds the same (IP, protocol, port) key from the leg the rule names (felix/bpf/polprog/pol_prog_builder.go)

matchPort/matchNotPort in app-policy/checker/match.go instead 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/matchNotPort take the leg's <IP>,<protocol>:<port> accessor and look that key up, matching what the other dataplanes do.
  • requestCache memoizes the source-side key alongside the destination one it already had; both now share an ipProtoPortKey helper.
  • protocolMapL4 gains 132: "sctp". A named port can be declared on sctp, and the key would otherwise carry an empty protocol field. This closes the same gap for matchDstIPPortSetIds, which uses the same key.

Testing

TestMatchNamedPorts is 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 protocolMapL4 is covered by an sctp named port in TestMatchNamedPorts, an sctp set in TestMatchDstIPPortSetIds, and TestIPProtoPortKey.

go test ./app-policy/... passes.

Release note:

Fixed application layer policy (Dikastes) never matching rules that reference a named port, which caused Allow rules to deny and Deny rules to have no effect.

…ed-ports

app-policy: match named ports against the IP+port set key
(cherry picked from commit 7753bba)
@dimitri-nicolo dimitri-nicolo added the release-note-required Change has user-facing impact (no matter how small) label Sep 11, 2026
Copilot AI lite review requested due to automatic review settings September 11, 2026 18:32
@dimitri-nicolo dimitri-nicolo added the docs-not-required Docs not required for this change label Sep 11, 2026
@dimitri-nicolo
dimitri-nicolo requested a review from a team as a code owner September 11, 2026 18:32
@marvin-tigera marvin-tigera added this to the Calico v3.32.3 milestone Sep 11, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

Implementation and test coverage are complete; the remaining documentation comment is a non-blocking nit.

Pull request overview

Fixes Dikastes named-port matching by using Felix-compatible IP/protocol/port IP-set keys, including SCTP.

Changes:

  • Adds memoized source and destination IP-port keys.
  • Updates named-port and IP-port-set matching.
  • Expands tests for named ports, negation, source legs, and SCTP.
File summaries
File Summary
app-policy/checker/requestcache.go Builds and caches IP-port keys.
app-policy/checker/requestcache_test.go Tests key formatting.
app-policy/checker/match.go Uses IP-port keys and supports SCTP.
app-policy/checker/match_test.go Covers named-port and SCTP scenarios.
Review details

Suppressed comments (1)

app-policy/checker/match.go:42

  • This adds SCTP members to the format accepted by the checker, but the IP-set contract still documents IP_AND_PORT as only tcp|udp in felix/proto/felixbackend.proto:195 and app-policy/policystore/ipset.go:43,50. Please update those contract comments (and regenerated artifacts if required) so consumers are not told that the SCTP members this change relies on are invalid.
	132: "sctp",
  • 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.

@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

CI triage — Calico

Recommendation: Dig deeper before merging

Likely caused by this PR

Test Why
Felix: BPF tests on Ubuntu 26.04 with jitharden=2 DIAGS not found for a failed Felix FV job — fv-guru likely failed and needs attention

Unrelated failures

Type Test Note
infra Felix: BPF tests on Ubuntu 24.04 Host VM kernel panic in l3mdev_fib_table_rcu (Fatal exception in interrupt); SSH lost (rc=66)
infra E2E tests: Conformance (cluster routing: BIRD) (+ 1 similar variant) Job stopped before it started (HTTP 404 / pipeline timeout)

workflow_id: 094d828b-80fc-491b-943a-191dc848dd38

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs-not-required Docs not required for this change release-note-required Change has user-facing impact (no matter how small)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants