pkg/multus: downcast empty CNI result to requested cniVersion#1505
pkg/multus: downcast empty CNI result to requested cniVersion#1505SAY-5 wants to merge 2 commits intok8snetworkplumbingwg:masterfrom
Conversation
emptyCNIResult always returns a *cni100.Result. When the pod-not-found branch set CNIVersion to 0.3.1 or 0.4.0 and handed that to skel, the containernetworking/cni conversion chain dispatched to convertFrom04x which type-asserts to *types040.Result and panics (k8snetworkplumbingwg#1497). The earlier fix (ccfd8f5) hardcoded 1.0.0 and papered over the class; the revert (921191d) brought the panic back for clusters configured with an older spec. Call emptyResult.GetAsVersion(n.CNIVersion) so the returned Result's concrete type matches whatever convertFrom*/To* the caller will run. For 1.x configs the Result stays *cni100.Result (existing 1.1.0 test still matches on that type); for 0.3.x / 0.4.0 configs it now returns a *types040.Result that convertFrom04x can safely assert. Adds a regression It-spec that drives pod-not-found through CmdAdd with cniVersion 0.4.0 and asserts the returned Result is a *types040.Result with CNIVersion 0.4.0. Fixes k8snetworkplumbingwg#1497 Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request addresses a panic (issue #1497) in CmdAdd when a pod is not found by ensuring the returned empty CNI result is downcast to the correct CNIVersion before being returned. A regression test has been added to verify the fix for CNI version 0.4.0. Feedback suggests optimizing the conversion logic by only performing it for versions prior to 1.0.0 to avoid redundant calls when the result is already in the correct format.
| versioned, convErr := emptyResult.GetAsVersion(n.CNIVersion) | ||
| if convErr != nil { | ||
| return nil, cmdErr(k8sArgs, "failed to downcast empty CNI result to cniVersion %q: %v", n.CNIVersion, convErr) | ||
| } | ||
| return versioned, nil |
There was a problem hiding this comment.
While this fix correctly addresses the panic by ensuring the concrete type of the result matches the requested cniVersion, calling GetAsVersion for 1.x versions is redundant since emptyCNIResult already returns a *cni100.Result. For performance in this error path, it's negligible, but for code clarity, you might consider only performing the conversion if the version is less than 1.0.0.
@gemini-code-assist suggestion on k8snetworkplumbingwg#1505: emptyCNIResult already returns a *cni100.Result, so GetAsVersion is a no-op (or worse, an unnecessary allocation) for 1.x configs. Gate the conversion on non-1.x cniVersions only; 1.x callers now return the *cni100.Result directly as they did before the panic fix. Behaviour for 0.3.x / 0.4.0 is unchanged: still downcast so skel's conversion chain dispatches convertFrom04x correctly. Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>
|
Thanks @gemini-code-assist — good call. Gated the |
|
That sounds like a solid approach. By gating the |
|
Thanks @gemini-code-assist — applied your suggestion. GetAsVersion is now gated on |
|
That sounds like a great resolution. By gating the |
Summary
Fixes #1497.
emptyCNIResultalways returns a*cni100.Result. When the pod-not-found branch setCNIVersionto0.3.1/0.4.0and handed that to skel, the containernetworking/cni conversion chain dispatched toconvertFrom04x, which type-asserts the incomingtypes.Resultto*types040.Resultand panics because we gave it a*cni100.Result.The earlier fix ccfd8f5 hardcoded
1.0.0to paper over the class mismatch; 921191d reverted that so the response would round-trip through the user's configured version again. The panic regressed along with the revert.Fix
Call
emptyResult.GetAsVersion(n.CNIVersion)so the returned Result's concrete type matches whateverconvertFrom*/convertTo*the caller will run:1.xconfigs the Result stays*cni100.Result(existing1.1.0test still asserts on that type).0.3.x/0.4.0configs it now returns a*types040.ResultthatconvertFrom04xcan safely assert.If
n.CNIVersionis unset, we keep the existing behaviour (return*cni100.Resultas-is).Tests
Adds
It("returns empty add result downcast to 0.4.0 cniVersion when pod is not found", ...)that drives pod-not-found throughCmdAddwithcniVersion: 0.4.0and asserts the returned Result is a*types040.ResultwithCNIVersion "0.4.0".go vet ./pkg/multus/...clean underGOOS=linux; the existing1.1.0variant stays green.