BPF: Allows users to set DSCP on traffic toward hosts/hostendpoints#10997
BPF: Allows users to set DSCP on traffic toward hosts/hostendpoints#10997mazdakn merged 6 commits intoprojectcalico:masterfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR modifies the eBPF dataplane's cluster boundary detection logic to exclude hosts from the cluster boundary for DSCP marking purposes. The change allows DSCP to be set on traffic destined to host endpoints and hosts, aligning the default behavior with NAT outgoing functionality.
Key changes:
- Updates the external address detection function to accept an
exclude_hostsparameter - Modifies cluster external traffic detection logic to use the new parameterized function
- Updates test cases to reflect the new behavior where host-to-host traffic is now considered external
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| felix/bpf-gpl/routes.h | Refactors external address detection functions to support host exclusion parameter |
| felix/bpf-gpl/tc.c | Updates cluster boundary detection logic to use new parameterized external address function |
| felix/bpf/ut/qos_test.go | Updates test cases to reflect new host-to-host traffic behavior and route flags |
felix/bpf-gpl/tc.c
Outdated
| CALI_DEBUG("Source " IP_FMT " not in IP pool", debug_ip(ctx->state->ip_src)); | ||
| if (rt_addr_is_external(&ctx->state->post_nat_ip_dst)) { | ||
| r = cali_rt_lookup(&ctx->state->post_nat_ip_dst); | ||
| if (!r || !(r->flags & (CALI_RT_WORKLOAD | CALI_RT_HOST))) { |
There was a problem hiding this comment.
This is basically reverting the change made in the previous PR here: #10934
felix/bpf-gpl/routes.h
Outdated
| return true; | ||
| } | ||
|
|
||
| static CALI_BPF_INLINE bool rt_addr_is_external(ipv46_addr_t *addr, bool exclude_hosts) |
There was a problem hiding this comment.
this is weird API. Host as not external, right?
| if (rt) { | ||
| flags = rt->flags; | ||
| } | ||
| bool exclude_hosts = (GLOBAL_FLAGS & CALI_GLOBALS_NATOUTGOING_EXCLUDE_HOSTS); |
There was a problem hiding this comment.
The original idea here was that you should do nat outgoing if you are going out of a pool and that includes hosts. But we have a config option now that excludes hosts from nat outgoing.
felix/bpf-gpl/tc.c
Outdated
| ctx->state->flags |= CALI_ST_NAT_OUTGOING; | ||
| } | ||
| if ((r->flags & CALI_RT_NAT_OUT) && | ||
| (rt_addr_is_external(&ctx->state->post_nat_ip_dst, GLOBAL_FLAGS & CALI_GLOBALS_NATOUTGOING_EXCLUDE_HOSTS))) { |
There was a problem hiding this comment.
no it read like ... do nat outgoing of dest it outsife of the cluster ... and btw exclude hosts ... but hold on, they are in the cluster 🤔
felix/bpf-gpl/routes.h
Outdated
| // - packet is destined to local host; or | ||
| // - packet is destined to a host and the CALI_GLOBALS_NATOUTGOING_EXCLUDE_HOSTS global flag is set | ||
| static CALI_BPF_INLINE bool rt_flags_should_perform_nat_outgoing(enum cali_rt_flags flags, bool exclude_hosts) | ||
| static CALI_BPF_INLINE bool rt_flags_external(enum cali_rt_flags flags, bool exclude_hosts) |
There was a problem hiding this comment.
external should return whatever is external
| static CALI_BPF_INLINE bool rt_flags_external(enum cali_rt_flags flags, bool exclude_hosts) | |
| static CALI_BPF_INLINE bool rt_flags_external(enum cali_rt_flags flags) |
felix/bpf-gpl/tc.c
Outdated
| } | ||
| // Check if traffic is leaving cluster. We might need to set DSCP later. | ||
| if (cali_rt_flags_is_in_pool(r->flags) && rt_addr_is_external(&ctx->state->post_nat_ip_dst)) { | ||
| if (cali_rt_flags_is_in_pool(r->flags) && rt_addr_is_external(&ctx->state->post_nat_ip_dst, false)) { |
There was a problem hiding this comment.
Wouldn't this do something similar?
| if (cali_rt_flags_is_in_pool(r->flags) && rt_addr_is_external(&ctx->state->post_nat_ip_dst, false)) { | |
| if (cali_rt_flags_is_in_pool(r->flags) && !cali_rt_workload(&ctx->state->post_nat_ip_dst)) { |
felix/bpf-gpl/tc.c
Outdated
| // If either source or destination is outside cluster, set flag as might need to update DSCP later. | ||
| if ((CALI_F_TO_HEP) && (rt_addr_is_local_host(&ctx->state->ip_src)) && | ||
| (rt_addr_is_external(&ctx->state->post_nat_ip_dst))) { | ||
| (rt_addr_is_external(&ctx->state->post_nat_ip_dst, false))) { |
There was a problem hiding this comment.
testing for workload would do but then the flag name is weird
As we may lookup src and dst route on some path multiple times, better to make the lookup just once once. In some cases we may lookup unnecessarily, but it is the first step to consolidate the route lookups in general.
Description
Allow users to set DSCP on traffic destined host endpoints and hosts. This change makes the default behavior similar NAT outgoing.
This change is made as requested by PM here: https://tigera.atlassian.net/browse/PMREQ-736
Related issues/PRs
Similar change for iptables/Nftables #10995
Todos
Release Note
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.