refactor: Update apipa nic as separate entry in podIPInfo #4012
Conversation
This PR updates both CNS and CNI code to construct apipa nic as separate entry in podIpInfo if either of allowhostonc or allownctohost set. This allows CNI to treat this as separate endpoint and align with current cni design/model of 1 nic per endpoint info. CNI then iterates through endpoint info and creates one nic at a time. Signed-off-by: Tamilmani <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the handling of APIPA (Automatic Private IP Addressing) NICs by creating them as separate entries in the podIpInfo structure when either allowhostonc or allownctohost flags are set. This aligns with the CNI design model of having one NIC per endpoint.
- Introduces a new
ApipaNICtype constant for APIPA network interface classification - Adds logic to create separate APIPA NIC entries in CNS when communication flags are enabled
- Updates CNI to handle APIPA NICs with specialized configuration including skipped default routes
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cns/NetworkContainerContract.go | Adds ApipaNIC constant and communication flags to PodIpInfo struct |
| cns/restserver/ipam.go | Implements logic to create separate APIPA NIC entries when communication flags are set |
| cni/network/invoker_cns.go | Adds APIPA NIC handling in CNI with specialized configuration function |
| network/network_windows.go | Updates network creation logic to skip network creation for APIPA NICs |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if resp[i].AllowHostToNCCommunication || resp[i].AllowNCToHostCommunication { | ||
| apipaIndex = i | ||
| } |
There was a problem hiding this comment.
The loop overwrites apipaIndex on each iteration where communication flags are true. If multiple responses have these flags set, only the last one will be used for APIPA NIC creation. Consider using a slice to track all indices or clarify if only one APIPA NIC is expected.
Copilot uses AI. Check for mistakes.
| func configureApipaAddResult(info *IPResultInfo, addResult *IPAMAddResult, podIPConfig *cns.IPSubnet, key string) error { | ||
| ip, ipnet, err := podIPConfig.GetIPNet() | ||
| if ip == nil { | ||
| return errors.Wrap(err, "Unable to parse IP from response: "+info.podIPAddress+" with err %w") |
There was a problem hiding this comment.
The error message references info.podIPAddress but the actual IP being parsed comes from podIPConfig, not info.podIPAddress. This could be misleading during debugging.
Copilot uses AI. Check for mistakes.
|
This pull request is stale because it has been open for 2 weeks with no activity. Remove stale label or comment or this will be closed in 7 days |
This PR updates both CNS and CNI code to construct apipa nic as separate entry in podIpInfo if either of allowhostonc or allownctohost set. This allows CNI to treat this as separate endpoint and align with current cni design/model of 1 nic per endpoint info. CNI then iterates through endpoint info and creates one nic at a time.
Reason for Change:
Issue Fixed:
Requirements:
Notes: