Skip to content
This repository was archived by the owner on Mar 9, 2022. It is now read-only.

Commit ac04366

Browse files
Random-LiuRandom-Liu
authored
Merge pull request #952 from Random-Liu/cherrypick-#949-release-1.0
Cherrypick #949 release 1.0
2 parents f117382 + 9af1f21 commit ac04366

15 files changed

Lines changed: 320 additions & 169 deletions

File tree

‎integration/restart_test.go‎

Lines changed: 2 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ package integration
1818

1919
import (
2020
"testing"
21-
"time"
2221

2322
"github.com/containerd/containerd"
2423
"github.com/stretchr/testify/assert"
@@ -128,27 +127,8 @@ func TestContainerdRestart(t *testing.T) {
128127
}
129128
}
130129

131-
t.Logf("Kill containerd")
132-
require.NoError(t, KillProcess("containerd"))
133-
defer func() {
134-
assert.NoError(t, Eventually(func() (bool, error) {
135-
return ConnectDaemons() == nil, nil
136-
}, time.Second, 30*time.Second), "make sure containerd is running before test finish")
137-
}()
138-
139-
t.Logf("Wait until containerd is killed")
140-
require.NoError(t, Eventually(func() (bool, error) {
141-
pid, err := PidOf("containerd")
142-
if err != nil {
143-
return false, err
144-
}
145-
return pid == 0, nil
146-
}, time.Second, 30*time.Second), "wait for containerd to be killed")
147-
148-
t.Logf("Wait until containerd is restarted")
149-
require.NoError(t, Eventually(func() (bool, error) {
150-
return ConnectDaemons() == nil, nil
151-
}, time.Second, 30*time.Second), "wait for containerd to be restarted")
130+
t.Logf("Restart containerd")
131+
RestartContainerd(t)
152132

153133
t.Logf("Check sandbox and container state after restart")
154134
loadedSandboxes, err := runtimeService.ListPodSandbox(&runtime.PodSandboxFilter{})

‎integration/sandbox_clean_remove_test.go‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,14 +17,23 @@ limitations under the License.
1717
package integration
1818

1919
import (
20+
"encoding/json"
21+
"io/ioutil"
22+
"os"
23+
"path/filepath"
24+
"strings"
2025
"testing"
2126
"time"
2227

2328
"github.com/containerd/containerd"
29+
runtimespec "github.com/opencontainers/runtime-spec/specs-go"
2430
"github.com/stretchr/testify/assert"
2531
"github.com/stretchr/testify/require"
2632
"golang.org/x/net/context"
33+
"golang.org/x/sys/unix"
2734
runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2"
35+
36+
"github.com/containerd/cri/pkg/server"
2837
)
2938

3039
func TestSandboxCleanRemove(t *testing.T) {
@@ -66,3 +75,100 @@ func TestSandboxCleanRemove(t *testing.T) {
6675
assert.NoError(t, runtimeService.StopPodSandbox(sb))
6776
assert.NoError(t, runtimeService.RemovePodSandbox(sb))
6877
}
78+
79+
func TestSandboxRemoveWithoutIPLeakage(t *testing.T) {
80+
ctx := context.Background()
81+
const hostLocalCheckpointDir = "/var/lib/cni"
82+
83+
t.Logf("Make sure host-local ipam is in use")
84+
config, err := CRIConfig()
85+
require.NoError(t, err)
86+
fs, err := ioutil.ReadDir(config.NetworkPluginConfDir)
87+
require.NoError(t, err)
88+
require.NotEmpty(t, fs)
89+
f := filepath.Join(config.NetworkPluginConfDir, fs[0].Name())
90+
cniConfig, err := ioutil.ReadFile(f)
91+
require.NoError(t, err)
92+
if !strings.Contains(string(cniConfig), "host-local") {
93+
t.Skip("host-local ipam is not in use")
94+
}
95+
96+
t.Logf("Create a sandbox")
97+
sbConfig := PodSandboxConfig("sandbox", "remove-without-ip-leakage")
98+
sb, err := runtimeService.RunPodSandbox(sbConfig)
99+
require.NoError(t, err)
100+
defer func() {
101+
// Make sure the sandbox is cleaned up in any case.
102+
runtimeService.StopPodSandbox(sb)
103+
runtimeService.RemovePodSandbox(sb)
104+
}()
105+
106+
t.Logf("Get pod information")
107+
client, err := RawRuntimeClient()
108+
require.NoError(t, err)
109+
resp, err := client.PodSandboxStatus(ctx, &runtime.PodSandboxStatusRequest{
110+
PodSandboxId: sb,
111+
Verbose: true,
112+
})
113+
require.NoError(t, err)
114+
status := resp.GetStatus()
115+
info := resp.GetInfo()
116+
ip := status.GetNetwork().GetIp()
117+
require.NotEmpty(t, ip)
118+
var sbInfo server.SandboxInfo
119+
require.NoError(t, json.Unmarshal([]byte(info["info"]), &sbInfo))
120+
require.NotNil(t, sbInfo.RuntimeSpec.Linux)
121+
var netNS string
122+
for _, n := range sbInfo.RuntimeSpec.Linux.Namespaces {
123+
if n.Type == runtimespec.NetworkNamespace {
124+
netNS = n.Path
125+
}
126+
}
127+
require.NotEmpty(t, netNS, "network namespace should be set")
128+
129+
t.Logf("Should be able to find the pod ip in host-local checkpoint")
130+
checkIP := func(ip string) bool {
131+
found := false
132+
filepath.Walk(hostLocalCheckpointDir, func(_ string, info os.FileInfo, _ error) error {
133+
if info != nil && info.Name() == ip {
134+
found = true
135+
}
136+
return nil
137+
})
138+
return found
139+
}
140+
require.True(t, checkIP(ip))
141+
142+
t.Logf("Kill sandbox container")
143+
require.NoError(t, KillPid(int(sbInfo.Pid)))
144+
145+
t.Logf("Unmount network namespace")
146+
// The umount will take effect after containerd is stopped.
147+
require.NoError(t, unix.Unmount(netNS, unix.MNT_DETACH))
148+
149+
t.Logf("Restart containerd")
150+
RestartContainerd(t)
151+
152+
t.Logf("Sandbox state should be NOTREADY")
153+
assert.NoError(t, Eventually(func() (bool, error) {
154+
status, err := runtimeService.PodSandboxStatus(sb)
155+
if err != nil {
156+
return false, err
157+
}
158+
return status.GetState() == runtime.PodSandboxState_SANDBOX_NOTREADY, nil
159+
}, time.Second, 30*time.Second), "sandbox state should become NOTREADY")
160+
161+
t.Logf("Network namespace should have been removed")
162+
_, err = os.Stat(netNS)
163+
assert.True(t, os.IsNotExist(err))
164+
165+
t.Logf("Should still be able to find the pod ip in host-local checkpoint")
166+
assert.True(t, checkIP(ip))
167+
168+
t.Logf("Should be able to remove the sandbox after properly stopped")
169+
assert.NoError(t, runtimeService.StopPodSandbox(sb))
170+
assert.NoError(t, runtimeService.RemovePodSandbox(sb))
171+
172+
t.Logf("Should not be able to find the pod ip in host-local checkpoint")
173+
assert.False(t, checkIP(ip))
174+
}

‎integration/test_utils.go‎

Lines changed: 42 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,14 @@ import (
2424
"os/exec"
2525
"strconv"
2626
"strings"
27+
"testing"
2728
"time"
2829

2930
"github.com/containerd/containerd"
3031
"github.com/pkg/errors"
3132
"github.com/sirupsen/logrus"
33+
"github.com/stretchr/testify/assert"
34+
"github.com/stretchr/testify/require"
3235
"google.golang.org/grpc"
3336
"k8s.io/kubernetes/pkg/kubelet/apis/cri"
3437
runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2"
@@ -265,6 +268,15 @@ func KillProcess(name string) error {
265268
return nil
266269
}
267270

271+
// KillPid kills the process by pid. kill is used.
272+
func KillPid(pid int) error {
273+
output, err := exec.Command("kill", strconv.Itoa(pid)).CombinedOutput()
274+
if err != nil {
275+
return errors.Errorf("failed to kill %d - error: %v, output: %q", pid, err, output)
276+
}
277+
return nil
278+
}
279+
268280
// PidOf returns pid of a process by name.
269281
func PidOf(name string) (int, error) {
270282
b, err := exec.Command("pidof", name).CombinedOutput()
@@ -278,8 +290,8 @@ func PidOf(name string) (int, error) {
278290
return strconv.Atoi(output)
279291
}
280292

281-
// CRIConfig gets current cri config from containerd.
282-
func CRIConfig() (*criconfig.Config, error) {
293+
// RawRuntimeClient returns a raw grpc runtime service client.
294+
func RawRuntimeClient() (runtime.RuntimeServiceClient, error) {
283295
addr, dialer, err := kubeletutil.GetAddressAndDialer(*criEndpoint)
284296
if err != nil {
285297
return nil, errors.Wrap(err, "failed to get dialer")
@@ -290,8 +302,16 @@ func CRIConfig() (*criconfig.Config, error) {
290302
if err != nil {
291303
return nil, errors.Wrap(err, "failed to connect cri endpoint")
292304
}
293-
client := runtime.NewRuntimeServiceClient(conn)
294-
resp, err := client.Status(ctx, &runtime.StatusRequest{Verbose: true})
305+
return runtime.NewRuntimeServiceClient(conn), nil
306+
}
307+
308+
// CRIConfig gets current cri config from containerd.
309+
func CRIConfig() (*criconfig.Config, error) {
310+
client, err := RawRuntimeClient()
311+
if err != nil {
312+
return nil, errors.Wrap(err, "failed to get raw runtime client")
313+
}
314+
resp, err := client.Status(context.Background(), &runtime.StatusRequest{Verbose: true})
295315
if err != nil {
296316
return nil, errors.Wrap(err, "failed to get status")
297317
}
@@ -301,3 +321,21 @@ func CRIConfig() (*criconfig.Config, error) {
301321
}
302322
return config, nil
303323
}
324+
325+
func RestartContainerd(t *testing.T) {
326+
require.NoError(t, KillProcess("containerd"))
327+
328+
// Use assert so that the 3rd wait always runs, this makes sure
329+
// containerd is running before this function returns.
330+
assert.NoError(t, Eventually(func() (bool, error) {
331+
pid, err := PidOf("containerd")
332+
if err != nil {
333+
return false, err
334+
}
335+
return pid == 0, nil
336+
}, time.Second, 30*time.Second), "wait for containerd to be killed")
337+
338+
require.NoError(t, Eventually(func() (bool, error) {
339+
return ConnectDaemons() == nil, nil
340+
}, time.Second, 30*time.Second), "wait for containerd to be restarted")
341+
}

‎pkg/server/container_status.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,8 @@ func toCRIContainerStatus(container containerstore.Container, spec *runtime.Imag
9999
}
100100
}
101101

102-
type containerInfo struct {
102+
// ContainerInfo is extra information for a container.
103+
type ContainerInfo struct {
103104
// TODO(random-liu): Add sandboxID in CRI container status.
104105
SandboxID string `json:"sandboxID"`
105106
Pid uint32 `json:"pid"`
@@ -122,7 +123,7 @@ func toCRIContainerInfo(ctx context.Context, container containerstore.Container,
122123
status := container.Status.Get()
123124

124125
// TODO(random-liu): Change CRI status info to use array instead of map.
125-
ci := &containerInfo{
126+
ci := &ContainerInfo{
126127
SandboxID: container.SandboxID,
127128
Pid: status.Pid,
128129
Removing: status.Removing,

‎pkg/server/sandbox_status.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,8 +100,9 @@ func toCRISandboxStatus(meta sandboxstore.Metadata, status sandboxstore.Status,
100100
}
101101
}
102102

103+
// SandboxInfo is extra information for sandbox.
103104
// TODO (mikebrow): discuss predefining constants structures for some or all of these field names in CRI
104-
type sandboxInfo struct {
105+
type SandboxInfo struct {
105106
Pid uint32 `json:"pid"`
106107
Status string `json:"processStatus"`
107108
NetNSClosed bool `json:"netNamespaceClosed"`
@@ -131,7 +132,7 @@ func toCRISandboxInfo(ctx context.Context, sandbox sandboxstore.Sandbox) (map[st
131132
processStatus = taskStatus.Status
132133
}
133134

134-
si := &sandboxInfo{
135+
si := &SandboxInfo{
135136
Pid: sandbox.Status.Get().Pid,
136137
Status: string(processStatus),
137138
Config: sandbox.Config,

‎pkg/server/sandbox_stop.go‎

Lines changed: 14 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ limitations under the License.
1717
package server
1818

1919
import (
20-
"os"
2120
"time"
2221

2322
"github.com/containerd/containerd"
@@ -60,23 +59,21 @@ func (c *criService) StopPodSandbox(ctx context.Context, r *runtime.StopPodSandb
6059
}
6160

6261
// Teardown network for sandbox.
63-
if sandbox.NetNSPath != "" && sandbox.NetNS != nil {
64-
if _, err := os.Stat(sandbox.NetNSPath); err != nil {
65-
if !os.IsNotExist(err) {
66-
return nil, errors.Wrapf(err, "failed to stat network namespace path %s", sandbox.NetNSPath)
67-
}
68-
} else {
69-
if teardownErr := c.teardownPod(id, sandbox.NetNSPath, sandbox.Config); teardownErr != nil {
70-
return nil, errors.Wrapf(teardownErr, "failed to destroy network for sandbox %q", id)
71-
}
62+
if sandbox.NetNSPath != "" {
63+
netNSPath := sandbox.NetNSPath
64+
if sandbox.NetNS == nil || sandbox.NetNS.Closed() {
65+
// Use empty netns path if netns is not available. This is defined in:
66+
// https://github.com/containernetworking/cni/blob/v0.7.0-alpha1/SPEC.md
67+
netNSPath = ""
7268
}
73-
/*TODO:It is still possible that containerd crashes after we teardown the network, but before we remove the network namespace.
74-
In that case, we'll not be able to remove the sandbox anymore. The chance is slim, but we should be aware of that.
75-
In the future, once TearDownPod is idempotent, this will be fixed.*/
76-
77-
//Close the sandbox network namespace if it was created
78-
if err = sandbox.NetNS.Remove(); err != nil {
79-
return nil, errors.Wrapf(err, "failed to remove network namespace for sandbox %q", id)
69+
if err := c.teardownPod(id, netNSPath, sandbox.Config); err != nil {
70+
return nil, errors.Wrapf(err, "failed to destroy network for sandbox %q", id)
71+
}
72+
// Close the sandbox network namespace if it was created
73+
if sandbox.NetNS != nil {
74+
if err = sandbox.NetNS.Remove(); err != nil {
75+
return nil, errors.Wrapf(err, "failed to remove network namespace for sandbox %q", id)
76+
}
8077
}
8178
}
8279

‎pkg/server/service.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ func NewCRIService(config criconfig.Config, client *containerd.Client) (CRIServi
148148

149149
// Try to load the config if it exists. Just log the error if load fails
150150
// This is not disruptive for containerd to panic
151-
if err := c.netPlugin.Load(cni.WithLoNetwork(), cni.WithDefaultConf()); err != nil {
151+
if err := c.netPlugin.Load(cni.WithLoNetwork, cni.WithDefaultConf); err != nil {
152152
logrus.WithError(err).Error("Failed to load cni during init, please check CRI plugin status before setting up network for pods")
153153
}
154154
// prepare streaming server

‎pkg/server/status.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ func (c *criService) Status(ctx context.Context, r *runtime.StatusRequest) (*run
4444
// Check the status of the cni initialization
4545
if err := c.netPlugin.Status(); err != nil {
4646
// If it is not initialized, then load the config and retry
47-
if err = c.netPlugin.Load(cni.WithLoNetwork(), cni.WithDefaultConf()); err != nil {
47+
if err = c.netPlugin.Load(cni.WithLoNetwork, cni.WithDefaultConf); err != nil {
4848
networkCondition.Status = false
4949
networkCondition.Reason = networkNotReadyReason
5050
networkCondition.Message = fmt.Sprintf("Network plugin returns error: %v", err)

‎pkg/server/testing/fake_cni_plugin.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,6 @@ func (f *FakeCNIPlugin) Status() error {
4747
}
4848

4949
// Load loads the network config.
50-
func (f *FakeCNIPlugin) Load(opts ...cni.LoadOption) error {
50+
func (f *FakeCNIPlugin) Load(opts ...cni.CNIOpt) error {
5151
return f.LoadErr
5252
}

‎pkg/server/update_runtime_config.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ func (c *criService) UpdateRuntimeConfig(ctx context.Context, r *runtime.UpdateR
5252
if err := c.netPlugin.Status(); err == nil {
5353
logrus.Infof("Network plugin is ready, skip generating cni config from template %q", confTemplate)
5454
return &runtime.UpdateRuntimeConfigResponse{}, nil
55-
} else if err := c.netPlugin.Load(cni.WithLoNetwork(), cni.WithDefaultConf()); err == nil {
55+
} else if err := c.netPlugin.Load(cni.WithLoNetwork, cni.WithDefaultConf); err == nil {
5656
logrus.Infof("CNI config is successfully loaded, skip generating cni config from template %q", confTemplate)
5757
return &runtime.UpdateRuntimeConfigResponse{}, nil
5858
}

0 commit comments

Comments
 (0)