Nftables: use verdict map for QoS policies#10827
Nftables: use verdict map for QoS policies#10827mazdakn wants to merge 28 commits intoprojectcalico:masterfrom
Conversation
marvin-tigera
added
release-note-required
There was a problem hiding this comment.
Pull Request Overview
This PR implements support for QoS policies with DSCP marking based on workload endpoint annotations. The implementation adds the ability to configure DSCP (Differentiated Services Code Point) values for egress traffic through Kubernetes pod annotations and translates them into nftables verdict maps for efficient packet classification.
- Adds DSCP annotation support for workload endpoints through
qos.projectcalico.org/dscp - Implements QoS policy manager to handle DSCP-based traffic classification
- Provides iptables and nftables dataplane implementations for DSCP marking
Reviewed Changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| libcalico-go/lib/backend/k8s/conversion/workload_endpoint_default.go | Adds parsing logic for DSCP annotation |
| libcalico-go/lib/backend/k8s/conversion/constants.go | Defines new DSCP annotation constant |
| libcalico-go/lib/apis/v3/workloadendpoint.go | Adds DSCP field to QoSControls struct |
| felix/rules/qos.go | Implements QoS policy chain rendering for DSCP rules |
| felix/dataplane/linux/qos_policy_mgr.go | Manages QoS policies and generates iptables/nftables rules |
| felix/iptables/actions.go | Implements DSCP action for iptables |
| felix/nftables/actions.go | Implements DSCP action for nftables |
| api/pkg/lib/numorstring/dscp.go | Defines DSCP type with validation and string constants |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
felix/rules/qos.go
Outdated
|
|
||
| rules = append(rules, generictables.Rule{ | ||
| Match: r.NewMatch().SourceNetVMAP("some map"), | ||
| }) |
There was a problem hiding this comment.
The hardcoded placeholder string "some map" indicates incomplete implementation. This should use a proper map name or be removed if not yet implemented.
| }) | |
| // TODO: Implement SourceNetVMAP with a proper map name or object. |
felix/rules/qos.go
Outdated
|
|
||
| rules = append(rules, generictables.Rule{ | ||
| Match: r.NewMatch().SourceNetVMAP("some map"), | ||
| }) |
There was a problem hiding this comment.
The nftables implementation appends a rule with incomplete match criteria and no action, which will likely cause runtime errors. Either complete the implementation or remove this placeholder code.
| }) | |
| for _, p := range policies { | |
| rules = append(rules, generictables.Rule{ | |
| Match: r.NewMatch().SourceNetVMAP(p.SrcAddrs), | |
| Action: r.DSCP(p.DSCP, ipVersion), | |
| }) | |
| } |
| dscp := msg.Endpoint.QosPolicies[0].Dscp | ||
|
|
||
| // This situation must be handled earlier. | ||
| if dscp > 63 || dscp < 0 { |
There was a problem hiding this comment.
The condition dscp < 0 will never be true since dscp is declared as int32 and comes from a protobuf field, but DSCP values are inherently non-negative. Consider removing this redundant check or change the comparison to handle the specific case where negative values might indicate an error.
| if dscp > 63 || dscp < 0 { | |
| if dscp > 63 { |
| ipVersion uint8 | ||
| ruleRenderer rules.RuleRenderer | ||
| mangleTable Table | ||
| forNftables bool |
There was a problem hiding this comment.
The forNftables field is declared but never used in the struct. Consider removing it if it's not needed or implement its intended functionality.
| forNftables bool |
|
This PR is stale because it has been open for 60 days with no activity. |
Description
Related issues/PRs
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.