From 766eb6f0f730937efe4a26f6b58faaf012f09be8 Mon Sep 17 00:00:00 2001 From: Euan Kemp Date: Thu, 26 May 2016 17:52:16 -0700 Subject: [PATCH] kubenet: Fix bug where shaper.Reset wasn't called The error check was inverse what it should have been, causing shaper.Reset to only get called with invalid cidrs. --- pkg/kubelet/network/cni/testing/mock_cni.go | 39 +++++++++++++++ pkg/kubelet/network/kubenet/kubenet_linux.go | 4 +- .../network/kubenet/kubenet_linux_test.go | 48 ++++++++++++++++++- 3 files changed, 87 insertions(+), 4 deletions(-) create mode 100644 pkg/kubelet/network/cni/testing/mock_cni.go diff --git a/pkg/kubelet/network/cni/testing/mock_cni.go b/pkg/kubelet/network/cni/testing/mock_cni.go new file mode 100644 index 00000000000..622edefd577 --- /dev/null +++ b/pkg/kubelet/network/cni/testing/mock_cni.go @@ -0,0 +1,39 @@ +/* +Copyright 2014 The Kubernetes Authors All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// mock_cni is a mock of the `libcni.CNI` interface. It's a handwritten mock +// because there are only two functions to deal with. +package mock_cni + +import ( + "github.com/appc/cni/libcni" + "github.com/appc/cni/pkg/types" + "github.com/stretchr/testify/mock" +) + +type MockCNI struct { + mock.Mock +} + +func (m *MockCNI) AddNetwork(net *libcni.NetworkConfig, rt *libcni.RuntimeConf) (*types.Result, error) { + args := m.Called(net, rt) + return args.Get(0).(*types.Result), args.Error(1) +} + +func (m *MockCNI) DelNetwork(net *libcni.NetworkConfig, rt *libcni.RuntimeConf) error { + args := m.Called(net, rt) + return args.Error(0) +} diff --git a/pkg/kubelet/network/kubenet/kubenet_linux.go b/pkg/kubelet/network/kubenet/kubenet_linux.go index cdc83c54cca..497f770dc58 100644 --- a/pkg/kubelet/network/kubenet/kubenet_linux.go +++ b/pkg/kubelet/network/kubenet/kubenet_linux.go @@ -67,7 +67,7 @@ type kubenetNetworkPlugin struct { host network.Host netConfig *libcni.NetworkConfig loConfig *libcni.NetworkConfig - cniConfig *libcni.CNIConfig + cniConfig libcni.CNI shaper bandwidth.BandwidthShaper podCIDRs map[kubecontainer.ContainerID]string MTU int @@ -374,7 +374,7 @@ func (plugin *kubenetNetworkPlugin) TearDownPod(namespace string, name string, i if hasCIDR { glog.V(5).Infof("Removing pod CIDR %s from shaper", cidr) // shaper wants /32 - if addr, _, err := net.ParseCIDR(cidr); err != nil { + if addr, _, err := net.ParseCIDR(cidr); err == nil { if err = plugin.shaper.Reset(fmt.Sprintf("%s/32", addr.String())); err != nil { glog.Warningf("Failed to remove pod CIDR %s from shaper: %v", cidr, err) } diff --git a/pkg/kubelet/network/kubenet/kubenet_linux_test.go b/pkg/kubelet/network/kubenet/kubenet_linux_test.go index a16e11e7fa7..bd83ad06218 100644 --- a/pkg/kubelet/network/kubenet/kubenet_linux_test.go +++ b/pkg/kubelet/network/kubenet/kubenet_linux_test.go @@ -19,14 +19,24 @@ package kubenet import ( "fmt" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + + "testing" + kubecontainer "k8s.io/kubernetes/pkg/kubelet/container" "k8s.io/kubernetes/pkg/kubelet/network" + "k8s.io/kubernetes/pkg/kubelet/network/cni/testing" nettest "k8s.io/kubernetes/pkg/kubelet/network/testing" + "k8s.io/kubernetes/pkg/util/bandwidth" "k8s.io/kubernetes/pkg/util/exec" - "testing" + ipttest "k8s.io/kubernetes/pkg/util/iptables/testing" ) -func newFakeKubenetPlugin(initMap map[kubecontainer.ContainerID]string, execer exec.Interface, host network.Host) network.NetworkPlugin { +// test it fulfills the NetworkPlugin interface +var _ network.NetworkPlugin = &kubenetNetworkPlugin{} + +func newFakeKubenetPlugin(initMap map[kubecontainer.ContainerID]string, execer exec.Interface, host network.Host) *kubenetNetworkPlugin { return &kubenetNetworkPlugin{ podCIDRs: initMap, execer: execer, @@ -111,4 +121,38 @@ func TestGetPodNetworkStatus(t *testing.T) { } } +// TestTeardownBeforeSetUp tests that a `TearDown` call does call +// `shaper.Reset` +func TestTeardownCallsShaper(t *testing.T) { + fexec := &exec.FakeExec{ + CommandScript: []exec.FakeCommandAction{}, + LookPathFunc: func(file string) (string, error) { + return fmt.Sprintf("/fake-bin/%s", file), nil + }, + } + fhost := nettest.NewFakeHost(nil) + fshaper := &bandwidth.FakeShaper{} + mockcni := &mock_cni.MockCNI{} + kubenet := newFakeKubenetPlugin(map[kubecontainer.ContainerID]string{}, fexec, fhost) + kubenet.cniConfig = mockcni + kubenet.shaper = fshaper + kubenet.iptables = ipttest.NewFake() + + mockcni.On("DelNetwork", mock.AnythingOfType("*libcni.NetworkConfig"), mock.AnythingOfType("*libcni.RuntimeConf")).Return(nil) + + details := make(map[string]interface{}) + details[network.NET_PLUGIN_EVENT_POD_CIDR_CHANGE_DETAIL_CIDR] = "10.0.0.1/24" + kubenet.Event(network.NET_PLUGIN_EVENT_POD_CIDR_CHANGE, details) + + existingContainerID := kubecontainer.BuildContainerID("docker", "123") + kubenet.podCIDRs[existingContainerID] = "10.0.0.1/24" + + if err := kubenet.TearDownPod("namespace", "name", existingContainerID); err != nil { + t.Fatalf("Unexpected error in TearDownPod: %v", err) + } + assert.Equal(t, []string{"10.0.0.1/32"}, fshaper.ResetCIDRs, "shaper.Reset should have been called") + + mockcni.AssertExpectations(t) +} + //TODO: add unit test for each implementation of network plugin interface