feat: implement IPVLAN Subinterface configuration and provision - #241
feat: implement IPVLAN Subinterface configuration and provision#241ngcxy wants to merge 5 commits into
Conversation
✅ Deploy Preview for dranet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hi @ngcxy. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Pull request overview
This draft PR adds IPVLAN-based subinterface support to the DRANET driver to enable sharing a single NIC across multiple pods, including allocation-time naming/IPAM, netns plumbing for subinterface creation/teardown, and GCE-derived subinterface ranges.
Changes:
- Added
SubInterfaceAPI types/defaults/validation and integrated subinterface-aware behavior into DRA + NRI hooks. - Introduced a
LocalIPAMallocator and PodConfigStore tracking to allocate addresses from subinterface ranges. - Implemented GCE-specific subinterface IPRange derivation (IPv6
/80from node/64) plus bare-metal routing/neighbor config.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/driver/subinterfaces.go | Creates/deletes IPVLAN subinterfaces in the container netns and reports NetworkDeviceData. |
| pkg/driver/pod_device_config.go | Adds in-memory allocated-IP tracking to back LocalIPAM allocation decisions. |
| pkg/driver/nri_hooks.go | Switches Run/Stop hooks to create/delete subinterfaces when configured; factors common netns config. |
| pkg/driver/local_ipam.go | New local sequential allocator for IP selection from a CIDR range. |
| pkg/driver/driver.go | Wires LocalIPAM into driver startup. |
| pkg/driver/dra_hooks.go | Adds subinterface-aware naming + optional IP assignment from IPRange + source-based rule insertion. |
| pkg/cloudprovider/gce/gce.go | Adds GCE-derived subinterface IPRange plus bare-metal route/neighbor configuration. |
| pkg/cloudprovider/gce/gce_test.go | Adds tests for the new GCE device-config behavior and IPv6 range derivation. |
| pkg/apis/types.go | Introduces SubInterfaceConfig, SubInterfaceType, and IPVlanConfig API types. |
| pkg/apis/constants.go | Adds the SubInterfaceTypeIPVlan constant. |
| pkg/apis/defaults.go | Defaults SubInterface type/mode/flag. |
| pkg/apis/validation.go | Validates SubInterface and IPVLAN config and IPRange format. |
| pkg/apis/validation_test.go | Adds test cases for subinterface validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e7bc2df to
7dc7879
Compare
|
Does this consider multi nic scenario? There can be multiple shared nics in host.. in that case, it might required to have separate vrf for each nic.. |
Hi @tamilmani1989 , could you elaborate more on the requirement of separate vrf? This feature will cover the multi-NIC scenario as more generic use cases, but within the current change, the vrf part modification isn't considered. Would like to hear your suggestion on this. |
| Type SubInterfaceType `json:"type,omitempty"` | ||
|
|
||
| // IPRange is the range to allocate IP addresses for the subinterface | ||
| IPRange string `json:"ipRange,omitempty"` |
There was a problem hiding this comment.
No way to inherit routes/neighbors from the parent interface. The GCE provider hardcodes a virtual gateway MAC, but bond gateways are dynamic (NDP-learned), not available in cloud metadata. Consider copyRoutesFromParent / copyNeighborsFromParent fields so the driver can read from the parent at runtime.
There was a problem hiding this comment.
I have updated addSourceBasedRouting in dra_hooks to dynamically detects the gateway from the parent, and create routes based on it. The permanent neighbors are directly copied from the host during prepareResourceClaim.
There was a problem hiding this comment.
Thanks for the work!
addSourceBasedRouting clears user-configured routes, which silently discards any routes/rules the user specified in the ResourceClaim. Users can and do write custom static routes.
Should this skip auto source-based routing when the user already configured routes/rules, rather than overwriting them?
There was a problem hiding this comment.
Good catch! Updated to keep user config routes and skip source-based routing in dra hooks.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ngcxy The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
gauravkghildiyal
left a comment
There was a problem hiding this comment.
Thanks @ngcxy. Leaving some feedback for mainly the 2nd commit. Will revisit the others later.
if multiple nic share same address space, then it might need vrf to route traffic via appropriate interface |
* Add SubInterfaceConfig, IPVlanConfig, and IPRangeConfig definitions to the NetworkConfig schema. * Support a list of IP ranges per subinterface, each expressed either as a CIDR block or as an explicit start/end address pair. * Define default configuration parameters and merging rules for subinterfaces, with user-provided values taking precedence and duplicates removed. * Implement validation logic for subinterface properties (IP ranges, CIDR formatting, start/end ordering, and IPVLAN modes/flags) along with unit tests.
* Implement LocalIPAM to allocate addresses from the configured IP ranges (CIDR or explicit start/end), returning one address per IP family chosen at random within the range bounds, while tracking the set of in-use addresses to prevent conflicts. * Roll back partial allocations on error, and add Release to return an address to the pool. * Add PodConfigStore.GetAllocatedIPs to collect the addresses recorded in the stored device configs, and seed LocalIPAM with them at NetworkDriver startup so previously assigned IPs are not reissued. * Add unit tests verifying IP allocation boundaries, randomized selection, conflict prevention, and lifecycle cleanups.
…laim hooks * Extend the prepareResourceClaim hook to process subinterface specs: * Assign interface name with the network type prefix. * Use LocalIPAM to automatically allocate IPs from the configured ranges when no static address is specified. * Implement source-based routing to generate custom-table routes and rules for the subinterface, when no custom routes and rules are configured. * Release allocated subinterface IPs in unprepareResourceClaim so addresses are returned to the pool on claim teardown. * Add unit tests validating IP allocation and source-based routing configuration for routes and rules.
* Plumb subinterface setup (createSubinterfaceInNS) and cleanup (nsDeleteSubinterface) into NRI runPodSandbox and stopPodSandbox lifecycle hooks. * Refactor common network configuration tasks (VRF, routes, neighbors, and rules) into a shared configureNetdevInNS helper. * Implement netlink utilities nsCreateSubinterface and nsDeleteSubinterface with support for IPVLAN creation (L2 mode, bridge flag), setting up link state, and configuring IPs. * Add integration tests (TestSubinterface_IPVlan) to verify end-to-end IPVLAN creation, MAC/MTU inheritance, IP assignment, and cleanup inside test network namespaces.
* Implement GetDeviceConfig to lookup GCE VM network interface details matching the device MAC address. * Add a shared cloudutil.IPRangeFromCIDR helper that derives explicit [start, end] allocation bounds from a CIDR, always excluding the network and broadcast addresses and supporting additional reserved counts at each end. * For IPv4, derive the range from the primary GCE Alias IP Range. * For IPv6, compute the range by appending a 16-bit marker (0xC0DE) to the parent interface's base prefix. * Add unit tests for the range helper, GCE device configuration lookups, and IPv6 prefix derivation.
| if config.NetworkInterfaceConfigInPod.SubInterface != nil { | ||
| subIfName := config.NetworkInterfaceConfigInPod.SubInterface.Name | ||
| if err := nsDeleteSubinterface(ns, subIfName); err != nil { | ||
| klog.Errorf("fail to delete subinterface %s for device %s: %v", subIfName, deviceName, err) |
There was a problem hiding this comment.
Use same logger like line 343.
| return netip.MustParseAddr(cfg.StartIP), netip.MustParseAddr(cfg.EndIP), nil | ||
| } | ||
|
|
||
| // Mode 2: derive boundaries from CIDR. |
There was a problem hiding this comment.
Should this exclusion applies to IPv6 too?
|
LGTM. I prototyped Aliyun's LACP-bond use case (issue #239) on top of this PR, and it only took about 90 lines in our cloud provider — zero changes to |
gauravkghildiyal
left a comment
There was a problem hiding this comment.
Another round focusing mostly on API.
| }{ | ||
| { | ||
| name: "valid subinterface config", | ||
| cfg: &SubInterfaceConfig{Type: "ipvlan", IPRanges: []IPRangeConfig{{CIDR: "10.24.3.0/24"}, {StartIP: "10.24.4.10", EndIP: "10.24.4.20"}}, IPVlan: &IPVlanConfig{Mode: "l2", Flag: "bridge"}}, |
There was a problem hiding this comment.
readability: Please format the config across multiple lines.
| cfg IPRangeConfig | ||
| wantErr bool | ||
| }{ | ||
| {name: "CIDR only", cfg: IPRangeConfig{CIDR: "192.168.1.0/24"}}, |
There was a problem hiding this comment.
readability: It's acceptable to have simpler things be in one line, but I think all these configs together seem to warrant being spread over multiple lines.
| {name: "IPv6 CIDR only", cfg: IPRangeConfig{CIDR: "2001:db8::/64"}}, | ||
| {name: "start and end only", cfg: IPRangeConfig{StartIP: "10.0.0.5", EndIP: "10.0.0.10"}}, | ||
| {name: "start == end", cfg: IPRangeConfig{StartIP: "10.0.0.5", EndIP: "10.0.0.5"}}, | ||
| {name: "all three, within cidr", cfg: IPRangeConfig{CIDR: "10.0.0.0/24", StartIP: "10.0.0.5", EndIP: "10.0.0.10"}}, |
There was a problem hiding this comment.
Why would be permit this use case of allowing both a CIDR and a Start+End together?
| } | ||
| } | ||
|
|
||
| func TestValidateSubInterfaceConfig(t *testing.T) { |
There was a problem hiding this comment.
I think our validation tests should also take defaulting into account.
| // overrides the cloud provider config. For slices, the two configurations are combined, | ||
| // but duplicates are resolved in favor of the user config. | ||
| // but duplicates are resolved in favor of the user config. For subinterface IPRanges, | ||
| // user-provided ranges are ordered before cloud-provided ranges before deduplication. |
There was a problem hiding this comment.
Any specific reason to have this divergence in behaviour for IPRanges? If not, let's have consistent behaviour
|
|
||
| // Name is the desired logical name of the subinterface inside the Pod. | ||
| // If not specified, it will be derived by adding a type prefix to the parent interface name. | ||
| // e.g. for ipvlan type, Name will be "ipvlan-<parent_interface_name>" |
There was a problem hiding this comment.
Interface names have a 15 char limit. Adding something like "-ipvlan" consumes 7 characters itself and it's possible that the "<parent_name>-ipvlan" may easily exceed the 15 char limit.
I would say let's not add any prefix/suffix at all and keep the original name as the default. We can leave the option of choosing a custom name
| // SubInterface defines the properties of the subinterfaces created on the network interface. | ||
| // When specified, new subinterfaces will be created in the pod namespace based | ||
| // on this config, while the original interface stays in the host namespace. | ||
| SubInterface *SubInterfaceConfig `json:"subInterface,omitempty"` |
There was a problem hiding this comment.
I've been giving this some more thought and I think merging this together with the existing Interface should be possible and avoid a lot of duplication.
Any thoughts on roughly something like this?
// 1. Remove SubInterface from NetworkConfig
type NetworkConfig struct {
Profile string `json:"profile,omitempty"`
Interface InterfaceConfig `json:"interface"`
// REMOVED: SubInterface *SubInterfaceConfig `json:"subInterface,omitempty"`
Routes []RouteConfig `json:"routes,omitempty"`
Rules []RuleConfig `json:"rules,omitempty"`
// ... (Neighbors, Ethtool)
}
// 2. Add the 3 new fields to your existing InterfaceConfig
type InterfaceConfig struct {
// ADDED: Type determines how the interface is provided to the Pod.
// "passthrough" (default) or "ipvlan"
Type InterfaceType `json:"type,omitempty"`
// ADDED: IPVlan holds IPVLAN-specific settings; only evaluated when Type is "ipvlan".
IPVlan *IPVlanConfig `json:"ipvlan,omitempty"`
// ... (Keep existing fields: Name, Addresses, MTU, VRF, etc.) ...
// ADDED: IPRanges allows node-local IPAM to generate addresses for this interface.
IPRanges []IPRangeConfig `json:"ipRanges,omitempty"`
}
// 3. Rename SubInterfaceType to InterfaceType
type InterfaceType string
const (
InterfaceTypePassthrough InterfaceType = "passthrough"
InterfaceTypeIPVlan InterfaceType = "ipvlan"
)
// 4. Keep your existing IPVlanConfig (no changes needed here)
type IPVlanConfig struct {
Mode string `json:"mode,omitempty"`
Flag string `json:"flag,omitempty"`
}There's some good general guidance on API design here https://github.com/kubernetes/community/blob/main/contributors/devel/sig-architecture/api-conventions.md#automatic-resource-allocation-and-deallocation, including things like why we should define things like an InterfaceType
| // If not specified, assign an IP address from the configured IP ranges using node local IPAM. | ||
| if len(deviceCfg.NetworkInterfaceConfigInPod.SubInterface.Addresses) == 0 { | ||
| ipRanges := deviceCfg.NetworkInterfaceConfigInPod.SubInterface.IPRanges | ||
| if len(ipRanges) == 0 { | ||
| errorList = append(errorList, fmt.Errorf("can't assign IP for subinterface %s, no IPRanges specified", ifName)) | ||
| continue | ||
| } | ||
| if np.localIPAM == nil { | ||
| errorList = append(errorList, fmt.Errorf("can't assign IP for subinterface %s, IPAM database not initialized", ifName)) | ||
| continue | ||
| } | ||
| // Allocate at most one IP per IP family from the configured ranges. | ||
| addresses, err := np.localIPAM.Allocate(ipRanges) | ||
| if err != nil { |
There was a problem hiding this comment.
PrepareResourceClaims only calls LocalIPAM.Allocate() when SubInterface.Addresses is empty. When a user statically specifies Addresses instead of IPRanges, that address is used as-is and never registered into LocalIPAM.allocatedIPs.
This means IPAM has no visibility into statically-assigned addresses during the current driver session. If another claim's IPRanges happens to overlap with a manually-specified static address, Allocate() has no way to know it's taken and can hand it out to a different pod — a real collision, not just a hypothetical one.
(After a driver restart, GetAllocatedIPs() reseeds LocalIPAM from every persisted SubInterface.Addresses, static or not — pod_device_config.go:337 — so static addresses become "known" post-restart.)
Suggested direction: make address resolution go through one path regardless of source, instead of two paths that only sometimes touch shared state.
- If the config specifies
Addresses, LocalIPAM should still record/reserve them (and can validate they don't collide with something already allocated), instead of skipping IPAM entirely. - If the config specifies
IPRanges, LocalIPAM fills inAddressesfrom the pool, exactly as it does today.
Either way, once Addresses is resolved, everything downstream (addIPVlan, addSourceBasedRouting) is already source-agnostic — it just iterates whatever ended up in Addresses. So this isn't a call to rewrite the consuming side; it's specifically about making the resolution step (static vs. IPAM-derived) go through the same bookkeeping, so LocalIPAM is actually the single source of truth for "what's in use" rather than only knowing about addresses it personally handed out.
There was a problem hiding this comment.
We should not make IPAM part of the interface API, https://github.com/kubernetes-sigs/dranet/pull/241/changes#r3710308724
An Interface or Subinterface define its attributes, IPAM is not an interface attribute, that process has to happen before we get here and populate the existing Addresses field, so each cloud provider can decide how they do IPAM
| // IPRanges is a list of IP address ranges from which the node-local IPAM | ||
| // generates IP addresses for the subinterface. It may be provided by the cloud | ||
| // provider in GetDeviceConfig and/or by the user. Exactly one IP address is | ||
| // generated per IP family present in the list. | ||
| IPRanges []IPRangeConfig `json:"ipRanges,omitempty"` |
There was a problem hiding this comment.
we already have Addresses why do we want to leak IPRanges here? Are we not able to do the IPAM without leaking it into the API? we are already doing it for the ones that are not subinterfaces
| // IPRangeConfig describes an allocatable IP address range for node-local IPAM. | ||
| // | ||
| // A range can be specified in one of two ways: | ||
| // 1. Explicit boundaries: both StartIP and EndIP are set (CIDR may be omitted). | ||
| // The driver validates that both are valid IP addresses of the same family | ||
| // and that StartIP <= EndIP. | ||
| // 2. CIDR: only CIDR is set (StartIP and EndIP omitted). The driver validates | ||
| // the CIDR and derives the allocatable boundaries from it, spanning every | ||
| // address except the network (base) address and the broadcast (last) address. | ||
| // | ||
| // If all three fields are set, StartIP/EndIP take priority (when valid) and CIDR | ||
| // is used only to sanity-check that the boundaries fall within it. | ||
| type IPRangeConfig struct { | ||
| // CIDR is the network range in CIDR notation. | ||
| // It is optional when both StartIP and EndIP are provided. | ||
| CIDR string `json:"cidr,omitempty"` | ||
| // StartIP is the first IP address that can be allocated from the range. | ||
| // If empty, the driver derives it from CIDR as the first address after the network address. | ||
| StartIP string `json:"startIP,omitempty"` | ||
| // EndIP is the last IP address that can be allocated from the range. | ||
| // If empty, the driver derives it from CIDR as the last address before the broadcast address. | ||
| EndIP string `json:"endIP,omitempty"` | ||
| } |
There was a problem hiding this comment.
I really want to avoid leaking IPAM config in the API ... this is about subinterfaces properties, IPAM is network configuration
|
/assign @aojea let's discuss the ipam addition and why it can not be part of the provider logic |
|
/hold Unless there's some additional limitation I can't see from the current diff, I don't see the need for a second way of handling IPAM here that's different from what we already do for the primary interface. Adding
I think all the use cases can be solved just with
config:
- opaque:
driver: dra.net
parameters:
profile: gce-subinterface # any non-empty value triggers GetProfileConfig
interface:
subInterface:
type: ipvlan// pkg/cloudprovider/gce/gce.go
type GCEInstance struct {
// ...
subIPAM *ipam.LocalIPAM // internal allocator; never (de)serialized
}
func (g *GCEInstance) GetProfileConfig(id cloudprovider.DeviceIdentifiers, claimUID types.UID, config *apis.NetworkConfig) (*apis.NetworkConfig, error) {
if config.SubInterface == nil || config.SubInterface.Type == "" {
return nil, nil // not a subinterface request
}
ranges, err := g.subInterfaceRangesFor(id) // derived from instance metadata, stays internal
if err != nil {
return nil, err
}
addrs, err := g.subIPAM.Allocate(ranges)
if err != nil {
return nil, err
}
return &apis.NetworkConfig{SubInterface: &apis.SubInterfaceConfig{Addresses: addrs}}, nil
}
func (g *GCEInstance) ReleaseProfileConfig(id cloudprovider.DeviceIdentifiers, claimUID types.UID, config *apis.NetworkConfig) error {
if config.SubInterface == nil {
return nil
}
for _, a := range config.SubInterface.Addresses {
g.subIPAM.Release(a)
}
return nil
}`pkg/ipam.LocalIPAM (this PR's allocator) is still 100% reusable here as a library.
parameters:
interface:
subInterface:
type: ipvlan
addresses: ["192.168.1.50/24"]No
This is where "should we ship a local-IPAM profile webhook" matters, and the answer is: we already ship this with So I can see two ways to reuse that pattern instead of inventing a new one: (a) Extend existing (b) Create a new / cmd/webhook-local-ipam/main.go (sketch)
type Server struct {
ipam *ipam.LocalIPAM
ranges []ipam.Range // from a flag/ConfigMap, e.g. --ranges=10.24.3.0/24,2001:db8::/64
}
func (s *Server) GetProfileConfig(w http.ResponseWriter, r *http.Request) {
var req webhook.ProfileRequest
json.NewDecoder(r.Body).Decode(&req)
if req.Config == nil || req.Config.SubInterface == nil {
json.NewEncoder(w).Encode(apis.NetworkConfig{})
return
}
addrs, err := s.ipam.Allocate(s.ranges)
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
json.NewEncoder(w).Encode(apis.NetworkConfig{SubInterface: &apis.SubInterfaceConfig{Addresses: addrs}})
}
func (s *Server) ReleaseProfileConfig(w http.ResponseWriter, r *http.Request) {
var req webhook.ProfileRequest
json.NewDecoder(r.Body).Decode(&req)
if req.Config != nil && req.Config.SubInterface != nil {
for _, a := range req.Config.SubInterface.Addresses {
s.ipam.Release(a)
}
}
w.WriteHeader(http.StatusOK)
}
Summary
In every case Can we keep |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Thanks @aojea, this is really helpful and reframes the whole API comprehension for me. I will refactor the API to drop the config-oriented IPRanges and let Cloud Provider to produce the static Addresses, avoiding driver-side configuration logic for the device. I see three ways as the next plan, in increasing scope: Option 1 (minimum change) Option 2 (separate persistent storage) Option 3 (webhook for bare metal) I believe for the current PR option 1 is sufficient to unblock, unless such interaction with persistent storage is not preferred. For option 3, I can follow up in future work for bare metal support. Please let me know if this plan sounds right to you. |
|
Option 1 sounds reasonable to me, once the IPs are allocated we assume they are part of the Interface until they are released |
|
Very excited about this work. This will help us at OCI avoid moving RDMA PFs. I used this PR as the base for OKE tests with Nvidia IPAM. IPv4 worked with IPvlan children. IPv6 also worked, but our fabric uses SLAAC. The Nvidia IPAM address was not used for the data path. Support for an IPvlan child without an explicit address would make the IPv6 configuration much simpler. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR introduces support for subinterface provisioning in the DRANET driver, currently implementing IPVLAN as the subinterface type. The feature addresses two networking requirements:
The key changes include:
SubInterfaceConfigandIPVlanConfigtoNetworkConfig, includingIPRanges— a list of allocation ranges, each expressed as a CIDR or an explicit start/end pair.pkg/ipampackage with a concurrency-safe node-localLocalIPAMthat owns the set of in-use addresses and hands out one address per IP family from the configured ranges; allocated IPs are released on claim teardown.GetDeviceConfig, using a shared, cloud-agnosticcloudutil.IPRangeFromCIDRhelper (network and broadcast always excluded; providers specify any additional reserved counts).Which issue(s) this PR is related to:
Fixes #63, #239
Special notes for your reviewer:
The code was tested by requesting a subinterface in ResourceClaim. The pod runs successfully with the IPVlan interface (connectivity verified via ping test), and the parent interface remains in the host’s namespace.
ResourceClaimTemplateyaml example:Does this PR introduce a user-facing change?