Skip to content

Conversation

@brandond
Copy link
Contributor

@brandond brandond commented Jul 15, 2022

Properly handle "null" as plugin output. This successfully unmarshals to a nil map, which crashes later when assigning confVersion to the map. This regression was introduced in #896.

Found when attempting to update multus to v1.1.1; its tests use a dummy plugin output of "null" which causes a panic rather than an error.

  Test Panicked
  assignment to entry in nil map
  /usr/lib/go-1.18/src/runtime/map_faststr.go:205

  Full Stack Trace
  github.com/containernetworking/cni/pkg/invoke.fixupResultVersion({0xc000598000?, 0x18?, 0x1a?}, {0xc000430888, 0x4, 0x8})
        /var/lib/repos/go/src/github.com/k8snetworkplumbingwg/multus-cni/vendor/github.com/containernetworking/cni/pkg/invoke/exec.go:65 +0x199
  github.com/containernetworking/cni/pkg/invoke.ExecPluginWithResult({0x18e5bc0, 0xc000138000}, {0xc0003a2228, 0x18}, {0xc000598000, 0xc1, 0xd0}, {0x18cf360?, 0xc0002d8780?}, {0x18db748, ...})
        /var/lib/repos/go/src/github.com/k8snetworkplumbingwg/multus-cni/vendor/github.com/containernetworking/cni/pkg/invoke/exec.go:125 +0x119
  github.com/containernetworking/cni/libcni.(*CNIConfig).addNetwork(0xc0003c6b88, {0x18e5bc0, 0xc000138000}, {0xc0004302f0, 0xe}, {0xc0004302b0, 0x5}, 0xc00038e0e0, {0x0, 0x0}, ...)
        /var/lib/repos/go/src/github.com/k8snetworkplumbingwg/multus-cni/vendor/github.com/containernetworking/cni/libcni/api.go:414 +0x385
  github.com/containernetworking/cni/libcni.(*CNIConfig).AddNetworkList(0xc00058e000?, {0x18e5bc0, 0xc000138000}, 0xc00034e8a0, 0x0?)
        /var/lib/repos/go/src/github.com/k8snetworkplumbingwg/multus-cni/vendor/github.com/containernetworking/cni/libcni/api.go:422 +0x10b
  gopkg.in/k8snetworkplumbingwg/multus-cni.v3/pkg/multus.conflistAdd(0xc00034e5a0, {0xc00058e000, 0x76, 0x80}, 0xc00058a000, {0x18db748, 0xc0003e2060})
        /var/lib/repos/go/src/github.com/k8snetworkplumbingwg/multus-cni/pkg/multus/multus.go:241 +0x30e
  gopkg.in/k8snetworkplumbingwg/multus-cni.v3/pkg/multus.delegateAdd({0x18db748?, 0xc0003e2060}, 0x0, 0x0, 0xc0002b2000, 0xc00034e5a0, 0x1?)
        /var/lib/repos/go/src/github.com/k8snetworkplumbingwg/multus-cni/pkg/multus/multus.go:331 +0x545
  gopkg.in/k8snetworkplumbingwg/multus-cni.v3/pkg/multus.CmdAdd(0xc000450cb0, {0x18db748?, 0xc0003e2060}, 0x0)
        /var/lib/repos/go/src/github.com/k8snetworkplumbingwg/multus-cni/pkg/multus/multus.go:626 +0x8ec
  gopkg.in/k8snetworkplumbingwg/multus-cni.v3/pkg/multus.glob..func1.21()
        /var/lib/repos/go/src/github.com/k8snetworkplumbingwg/multus-cni/pkg/multus/multus_test.go:1283 +0x1a8
  gopkg.in/k8snetworkplumbingwg/multus-cni.v3/pkg/multus.TestMultus(0x0?)
        /var/lib/repos/go/src/github.com/k8snetworkplumbingwg/multus-cni/pkg/multus/multus_test.go:53 +0x90
  testing.tRunner(0xc00038a9c0, 0x178beb0)
        /usr/lib/go-1.18/src/testing/testing.go:1439 +0x102
  created by testing.(*T).Run
        /usr/lib/go-1.18/src/testing/testing.go:1486 +0x35f

Signed-off-by: Brad Davidson [email protected]

@brandond brandond force-pushed the fix_null_output branch 2 times, most recently from fe77fb4 to 1c7c696 Compare July 15, 2022 22:23
Properly handle "null" as plugin output. This successfully unmarshals to a nil map, which crashes later when assigning confVersion to the map.

Signed-off-by: Brad Davidson <[email protected]>
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 72.211% when pulling 1c7c696 on brandond:fix_null_output into 812a8cf on containernetworking:main.

@dcbw
Copy link
Member

dcbw commented Jul 20, 2022

/lgtm

1 similar comment
@MikeZappa87
Copy link
Contributor

/lgtm

@brandond
Copy link
Contributor Author

Anyone have access to merge? Maybe tag a new release?

@MikeZappa87
Copy link
Contributor

@brandond do you need plugins as well? I can get to this later/tomorrow.

@brandond
Copy link
Contributor Author

I'm honestly not sure! This is my first time contributing.

@dcbw dcbw merged commit 3363d14 into containernetworking:main Jul 27, 2022
@dcbw
Copy link
Member

dcbw commented Jul 27, 2022

@brandond released https://github.com/containernetworking/cni/releases/tag/v1.1.2 with your fix. Thanks!

Runtimes should revendor libcni to get the fix.

@s1061123
Copy link
Contributor

@brandond Thank you for the fix! multus will include it in 4.0.

s1061123 added a commit to s1061123/multus-cni that referenced this pull request Jul 29, 2022
This change introduces containernetworking/cni#904
to fix the issue.
s1061123 added a commit to s1061123/multus-cni that referenced this pull request Jul 29, 2022
This change introduces containernetworking/cni#904
to fix the issue.
s1061123 added a commit to s1061123/multus-cni that referenced this pull request Jul 29, 2022
This change introduces containernetworking/cni#904
to fix the issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants