Skip to content

Conversation

@sridhartigera
Copy link
Member

@sridhartigera sridhartigera commented Nov 25, 2025

Description

This PR splits the jump maps used for policy in ebpf dataplane into 2, similar to program maps.

Related issues/PRs

Todos

  • Tests
  • Documentation
  • Release note

Release Note

TBD

Reminder for the reviewer

Make sure that this PR has the correct labels and milestone set.

Every PR needs one docs-* label.

  • docs-pr-required: This change requires a change to the documentation that has not been completed yet.
  • docs-completed: This change has all necessary documentation completed.
  • docs-not-required: This change has no user-facing impact and requires no docs.

Every PR needs one release-note-* label.

  • release-note-required: This PR has user-facing changes. Most PRs should have this label.
  • release-note-not-required: This PR has no user-facing changes.

Other optional labels:

  • cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.
  • needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.

@marvin-tigera marvin-tigera added this to the Calico v3.32.0 milestone Nov 25, 2025
@marvin-tigera marvin-tigera added release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented labels Nov 25, 2025
@CLAassistant
Copy link

CLAassistant commented Nov 25, 2025

CLA assistant check
All committers have signed the CLA.

@sridhartigera sridhartigera added the docs-not-required Docs not required for this change label Nov 27, 2025
@marvin-tigera marvin-tigera removed the docs-pr-required Change is not yet documented label Nov 27, 2025
@sridhartigera sridhartigera added docs-pr-required Change is not yet documented release-note-not-required Change has no user-facing impact labels Nov 27, 2025
@marvin-tigera marvin-tigera removed release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented labels Nov 27, 2025
@sridhartigera sridhartigera marked this pull request as ready for review November 27, 2025 22:24
@sridhartigera sridhartigera requested a review from a team as a code owner November 27, 2025 22:24
Copilot AI review requested due to automatic review settings November 27, 2025 22:24
Copilot finished reviewing on behalf of sridhartigera November 27, 2025 22:27
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR splits the policy jump maps used in the eBPF dataplane from a single shared map into two separate maps - one for ingress (from-host) traffic and one for egress (to-host) traffic. This architectural change aligns the policy jump maps with the existing program maps structure.

Key Changes:

  • Introduced separate cali_jump_fh (from-host/ingress) and cali_jump_th (to-host/egress) jump maps
  • Updated BPF endpoint manager to use separate jump map allocators for ingress and egress
  • Split default policy programs into ingress and egress variants
  • Modified all tests and infrastructure code to work with the dual jump map architecture

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
felix/bpf/jump/map.go Defines new separate IngressMapParameters and EgressMapParameters for the split jump maps
felix/bpf/bpfmap/bpf_maps.go Updates CommonMaps to use JumpMaps slice instead of single JumpMap
felix/dataplane/linux/bpf_ep_mgr.go Implements separate jump map allocators (jumpMapAllocFromHEP/jumpMapAllocToHEP) and updates all map selection logic
felix/dataplane/linux/bpf_ep_mgr_test.go Updates test mocks to use two separate jump maps (jumpMapFH/jumpMapTH) and adjusts test expectations
felix/bpf/ut/pol_prog_test.go Updates policy program tests to use jump.Maps() array and correct hook indexing
felix/bpf/ut/bpf_prog_test.go Updates BPF program tests to handle split jump maps with proper hook selection
felix/bpf/ut/attach_test.go Updates attach tests to check both jump maps separately with correct hook indices
felix/fv/infrastructure/felix.go Updates FV infrastructure to reference new jump map naming
felix/bpf/maps/maps.go Adds map stub configuration for new map naming convention
felix/bpf/bpf_syscall.go Adds LoadBPFProgramFromInsnsWithAttachType to support TCX attach type configuration
felix/bpf/bpf_syscall.h Refactors BPF program loading to use libbpf's bpf_prog_load with attach type support
felix/bpf/bpf_syscall_stub.go Adds stub for new LoadBPFProgramFromInsnsWithAttachType function
felix/bpf-gpl/jump.h Updates BPF C code to select correct jump map based on direction and compilation flags
felix/bpf-gpl/calculate-flags Adds CALI_TC_DEF_POLICY flag for policy_default programs
felix/bpf-gpl/bpf.h Defines CALI_TC_DEF_POLICY flag and CALI_F_DEF_POLICY macro
felix/bpf-gpl/Makefile Splits policy_default.o into separate ingress and egress variants
felix/Makefile Updates build targets to include both policy_default_ingress.o and policy_default_egress.o

Comment on lines 731 to 732
case strings.HasPrefix(b.Name, "cali_progs_map_fh"):
objName = "common_map_stub_ing.o"
objName = "common_map_stub_fh.o"
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

Incomplete map stub configuration. Only cali_progs_map_fh is mapped to common_map_stub_fh.o, but there's no corresponding case for cali_jump_th (the egress jump map) to map to common_map_stub_th.o. This could cause the egress jump map to fail to load from the correct stub object file. You likely need to add another case for maps starting with "cali_jump_th".

Copilot uses AI. Check for mistakes.
out, err := f.ExecOutput(
"bpftool", "map", "lookup",
"pinned", "/sys/fs/bpf/tc/globals/cali_jump3",
"pinned", "/sys/fs/bpf/tc/globals/cali_jump_th2",
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

Hardcoded jump map reference doesn't match the new split architecture. The map path is hardcoded to "cali_jump_th2" but should dynamically select between "cali_jump_fh" (ingress) and "cali_jump_th" (egress) based on the policy direction. This function needs context about whether it's checking ingress or egress policy to select the correct map. Also note the version suffix "2" should match the Version in the map parameters.

Copilot uses AI. Check for mistakes.
Comment on lines 123 to 125
#if CALI_F_HEP || CALI_F_PREAMBLE || CALI_F_DEF_POLICY
#if CALI_F_INGRESS
#define cali_jump_prog_map map_symbol(cali_jump_fh, 2)
Copy link
Member

Choose a reason for hiding this comment

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

Seems backwards: if it's a HEP and it's an ingress program (policy and tc direction are both aligned) then that's the "to host" direction, but looks like you'll use fh

Copy link
Member

Choose a reason for hiding this comment

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

CALI_F_INGRESS is defined to be the policy direction. Perhaps all of this logic would be clearer if we split these flags so that we no longer have any ambiguity:

CALI_F_POLICY_INGRESS / EGRESS
CALI_F_PROG_INGRESS / EGRESS

objName = "common_map_stub.o"
case strings.HasPrefix(b.Name, "cali_progs_map_fh"):
objName = "common_map_stub_ing.o"
objName = "common_map_stub_fh.o"
Copy link
Member

Choose a reason for hiding this comment

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

_fh naming still used here? Is Copilot right about needing something for the other direction?

Copy link
Member Author

Choose a reason for hiding this comment

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

The one for the other direction in common_map_stub.o, which we already have.


func (m *bpfEndpointManager) loadDefaultPolicies() error {
file := path.Join(bpfdefs.ObjectDir, "policy_default.o")
func (m *bpfEndpointManager) loadDefaultPolicies(hk hook.Hook) error {
Copy link
Member

Choose a reason for hiding this comment

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

The name of the default policy obj file is based on the policy direction, but this function takes a hook direction. I've forgotten exactly what these objects are for, are they only used for HEPs or are we deliberately only naming them by hook name?

If we only want them named by hook then you should comment that in the BPF code and here since the way the flags is calculated goes via the policy direction, which adds another layer to the confusion!

Copy link
Member Author

Choose a reason for hiding this comment

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

These are named as per hook direction similar to the preamble programs.

@sridhartigera sridhartigera merged commit 8f0d916 into projectcalico:master Dec 4, 2025
2 of 3 checks passed
@sridhartigera sridhartigera deleted the split-pol-jump branch December 4, 2025 15:48
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-not-required Change has no user-facing impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants