feat(node): add hostport capability and network setup#855
feat(node): add hostport capability and network setup#855BSWANG merged 1 commit intoAliyunContainerService:mainfrom
Conversation
l1b0k
commented
Jul 24, 2025
l1b0k
commented
- Add setupHostPort function to configure portmap plugin and network rules
- Implement configureNetworkRules function for iptables and ip rule setup- Include new hostport command in node capabilities
- Add setupHostPort function to configure portmap plugin and network rules - Implement configureNetworkRules function for iptables and ip rule setup- Include new hostport command in node capabilities Signed-off-by: l1b0k <libokang.lbk@alibaba-inc.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #855 +/- ##
==========================================
+ Coverage 35.42% 35.53% +0.10%
==========================================
Files 129 130 +1
Lines 18666 18837 +171
==========================================
+ Hits 6612 6693 +81
- Misses 11313 11392 +79
- Partials 741 752 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements hostport capability and network setup functionality for the Terway CNI plugin. The changes add support for configuring portmap plugin with network rules including iptables and IP routing rules to enable hostport functionality in Kubernetes pods.
- Add setupHostPort function to detect portmap plugin configuration and setup network rules
- Implement configureNetworkRules function for iptables CONNMARK rules and custom routing table setup
- Update netlink rule handling to support Mark and Mask filters for improved rule management
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/terway-cli/node.go | Adds hostport task and setupHostPort function to node configuration capabilities |
| cmd/terway-cli/portmap.go | Implements network rules configuration with iptables and routing setup |
| cmd/terway-cli/portmap_test.go | Comprehensive test suite for hostport functionality |
| plugin/driver/utils/utils_linux.go | Enhanced FindIPRule function to support Mark and Mask filtering |
| plugin/datapath/policy_router_linux.go | Refactored to use netlink.NewRule() for better rule initialization |
| plugin/datapath/policy_router_linux_test.go | Updated tests to use netlink.NewRule() pattern |
| go.mod | Moved coreos/go-iptables from indirect to direct dependency |
| // Configure iptables and ip rule | ||
| func configureNetworkRules() error { | ||
| ctx := context.Background() | ||
|
|
||
| var ( | ||
| mark = 0x10 | ||
| mask = 0x10 | ||
| tableID = 100 | ||
| defaultInterface = "eth0" | ||
| comment = "terway-portmap" // Rule comment | ||
| rulePrio = 600 |
There was a problem hiding this comment.
Consider extracting these hardcoded configuration values (mark=0x10, tableID=100, rulePrio=600) into named constants or configuration parameters to improve maintainability and avoid magic numbers.
| // Configure iptables and ip rule | |
| func configureNetworkRules() error { | |
| ctx := context.Background() | |
| var ( | |
| mark = 0x10 | |
| mask = 0x10 | |
| tableID = 100 | |
| defaultInterface = "eth0" | |
| comment = "terway-portmap" // Rule comment | |
| rulePrio = 600 | |
| const ( | |
| markDefault = 0x10 | |
| maskDefault = 0x10 | |
| tableIDDefault = 100 | |
| rulePriority = 600 | |
| ) | |
| // Configure iptables and ip rule | |
| func configureNetworkRules() error { | |
| ctx := context.Background() | |
| var ( | |
| mark = markDefault | |
| mask = maskDefault | |
| tableID = tableIDDefault | |
| defaultInterface = "eth0" | |
| comment = "terway-portmap" // Rule comment |
| mark = 0x10 | ||
| mask = 0x10 | ||
| tableID = 100 | ||
| defaultInterface = "eth0" |
There was a problem hiding this comment.
Hardcoding 'eth0' as the default interface may not work in all environments. Consider making this configurable or dynamically detecting the primary network interface.
| err = ensureNFRules(ipt, testRule) | ||
| assert.NoError(t, err, "ensureNFRules should not error when rule already exists") | ||
|
|
||
| // clean up |
There was a problem hiding this comment.
[nitpick] The comment should be capitalized: 'Clean up' instead of 'clean up'.
| // clean up | |
| // Clean up |
| if rule.Src == nil && rule.Dst == nil && rule.OifName == "" { | ||
| return nil, errors.New("both src and dst is nil") | ||
| if rule.Src == nil && rule.Dst == nil && rule.OifName == "" && rule.Mask == 0 && rule.Mark == 0 { | ||
| return nil, errors.New("rule is empty") |
There was a problem hiding this comment.
The error message 'rule is empty' is not very descriptive. Consider providing more context about what fields are required, such as 'rule validation failed: at least one of Src, Dst, OifName, Mask, or Mark must be specified'.
| return nil, errors.New("rule is empty") | |
| return nil, errors.New("rule validation failed: at least one of Src, Dst, OifName, Mask, or Mark must be specified") |
| } | ||
|
|
||
| if exists { | ||
| fmt.Printf("Rule already exists in %s table %s chain %v\n", rule.Table, rule.Chain, rule.Args) |
There was a problem hiding this comment.
Using fmt.Printf for logging in production code is not recommended. Consider using a proper logging framework or at least fmt.Fprintf to stderr for diagnostic messages.
| if err != nil { | ||
| return fmt.Errorf("failed to add rule: %w", err) | ||
| } | ||
| fmt.Printf("Added rule to %s table %s chain %v\n", rule.Table, rule.Chain, rule.Args) |
There was a problem hiding this comment.
Using fmt.Printf for logging in production code is not recommended. Consider using a proper logging framework or at least fmt.Fprintf to stderr for diagnostic messages.
feat(node): add hostport capability and network setup
