OCPBUGS-100110: Preserve conflist delegate config so DEL does not leak IPs - #336
OCPBUGS-100110: Preserve conflist delegate config so DEL does not leak IPs#336wizhaoredhat wants to merge 2 commits into
Conversation
A clusterNetwork loaded from a .conflist path was reconstructed via the lossy cnitypes.PluginConf, dropping CNI-specific fields (calico's kubeconfig/datastore_type/policy, etc). ADD survived by using the complete CNINetworkConfigList, but DEL used the stripped Bytes, so calico could not build its datastore client, never released the IP, and leaked one address per teardown. - LoadDelegateNetConfFromConfList: rebuild Bytes losslessly from the libcni raw bytes (per-plugin, covering subdirectory-chain plugins) instead of marshaling the lossy NetConfList; assign Bytes once after deviceID/cni-args injection (fixes deviceID-without-cni-args drop) - collapse the redundant CNINetworkConfigList field; conflistAdd now uses Bytes uniformly, removing the dual source of truth and the TODO - CmdDel: inject cniVersion onto raw Bytes via InjectCNIVersionInConfList instead of re-marshaling the lossy ConfList (second stripping point) - add regression tests for field preservation, subdirectory-chain plugins, and cniVersion injection Signed-off-by: Chen Tang <tangchen1234567@gmail.com>
CmdDel: do not clobber delegate Bytes when cniVersion injection fails. Assign to a temporary and only overwrite on success, so DEL keeps the original config instead of running with nil input, which would risk leaking the IP. Signed-off-by: Chen Tang <tangchen1234567@gmail.com>
openshift-ci-robot
added
jira/severity-important
|
@wizhaoredhat: This pull request references Jira Issue OCPBUGS-100110, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/jira refresh |
|
@wizhaoredhat: This pull request references Jira Issue OCPBUGS-100110, which is invalid:
Comment DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/approve |
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@wizhaoredhat: This pull request references Jira Issue OCPBUGS-100110, which is valid. The bug has been moved to the POST state. 7 validation(s) were run on this bug
DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
removed
the
jira/invalid-bug
|
/cherry-pick release-4.22 |
|
@wizhaoredhat: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions 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. |
|
@wizhaoredhat: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vinnie1110, wizhaoredhat The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
Hold due to #338 |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
5 participants
Cherry pick of 6386911
Summary
This PR fixes an IP leak that occurs when a Multus delegate is a CNI conflist (
plugins: [...]). OnCMD_DEL, the delegate was invoked with a lossy copy of its configuration, so IPAM plugins that need CNI-specific fields (e.g. Calico, which needskubeconfig/datastore_typeto reach its datastore) failed to release the allocated IP. Over time, pod churn exhausts the IP pool.Symptom
In a cluster using Calico as a Multus delegate (conflist form), we observed that IPs allocated on pod creation were never released on pod deletion. The delegate's
DELran against an incomplete config, Calico IPAM could not authenticate to its datastore, and the IP allocation was orphaned. The pool slowly drained until new pods could no longer get an address.Root cause
When loading a conflist delegate,
LoadDelegateNetConfFromConfListstoredDelegateNetConf.Bytesby marshaling the structuredtypes.NetConfList(built fromcnitypes.PluginConf):cnitypes.PluginConfonly models a fixed subset of keys (cniVersion/name/type/capabilities/ipam.type/dns). Any other field — Calico'skubeconfig,datastore_type, and similar plugin-specific options — is silently dropped during that round-trip.The same lossy marshal happened again on the DEL path in
CmdDel, where the code backfilledcniVersionwith:So the config handed to the delegate's
DELwas missing exactly the fields the plugin needs to clean up its IPAM state — hence the leak. (ADDhappened to work in many setups because the on-disk conf was read more directly, masking the asymmetry.)Fix
Preserve the original conflist bytes losslessly.
LoadDelegateNetConfFromConfListnow rebuildsDelegateNetConf.Bytesfrom the libcni raw bytes via a newrawConfListByteshelper: it takes the list-level keys fromconfList.Bytesand rebuilds thepluginsarray from each plugin's own raw bytes. Per-plugin raw bytes are used (rather thanconfList.Bytesas a whole) because libcni appends plugins loaded from a subdirectory chain intoconfList.Pluginswithout updatingconfList.Bytes; relying onconfList.Bytesalone would drop those appended plugins.Backfill
cniVersionwithout data loss on DEL.CmdDelnow uses a newInjectCNIVersionInConfListhelper that unmarshals the raw conflist into a generic map, sets onlycniVersion, and re-marshals — instead of marshaling the lossy structuredNetConfList. DEL now receives the same complete config that ADD did.Remove the now-redundant representation. The
CNINetworkConfigList libcni.NetworkConfigListfield onDelegateNetConfis dropped, andconflistAddis simplified to always parse from the (now-correct) raw bytes. This unifies ADD and DEL on a single source of truth — the losslessBytes— and removes the duplicate config carried on the struct.Testing
Added unit tests in
pkg/types/conf_test.go:LoadDelegateNetConfFromConfList preserves CNI-specific fields so DEL does not leak— verifies plugin-specific fields (e.g.kubeconfig/datastore_type) survive intoDelegateNetConf.Bytes.LoadDelegateNetConfFromConfList keeps plugins appended from a subdirectory chain— verifies plugins appended by libcni (not present inconfList.Bytes) are retained.InjectCNIVersionInConfList sets cniVersion without dropping fields— verifiescniVersionbackfill is lossless.Compatibility
No API or manifest changes. The fix only changes how delegate config bytes are reconstructed internally; delegates now receive the complete config on both ADD and DEL, which is the intended behavior.