Skip to content

Commit c4c0556

Browse files
haircommanderhaircommander
authored
Merge pull request #8797 from kwilczynski/feature/backport-commit-429ef7c36-to-release-1.28
[release-1.28] OCPBUGS-45907: Only restore container if all bind mounts are defined
2 parents 16c5678 + 4e4bf87 commit c4c0556

File tree

3 files changed

+102
-144
lines changed

3 files changed

+102
-144
lines changed

‎server/container_restore.go‎

Lines changed: 80 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,10 @@ package server
22

33
import (
44
"encoding/json"
5+
"errors"
56
"fmt"
67
"os"
8+
"strings"
79

810
metadata "github.com/checkpoint-restore/checkpointctl/lib"
911
istorage "github.com/containers/image/v5/storage"
@@ -62,22 +64,31 @@ func (s *Server) CRImportCheckpoint(
6264
) (ctrID string, retErr error) {
6365
var mountPoint string
6466

65-
input := createConfig.Image.Image
67+
// Ensure that the image to restore the checkpoint from has been provided.
68+
if createConfig.Image == nil || createConfig.Image.Image == "" {
69+
return "", errors.New(`attribute "image" missing from container definition`)
70+
}
71+
72+
if createConfig.Metadata == nil || createConfig.Metadata.Name == "" {
73+
return "", errors.New(`attribute "metadata" missing from container definition`)
74+
}
75+
76+
inputImage := createConfig.Image.Image
6677
createMounts := createConfig.Mounts
6778
createAnnotations := createConfig.Annotations
6879
createLabels := createConfig.Labels
6980

70-
checkpointIsOCIImage, err := s.checkIfCheckpointOCIImage(ctx, input)
81+
checkpointIsOCIImage, err := s.checkIfCheckpointOCIImage(ctx, inputImage)
7182
if err != nil {
7283
return "", err
7384
}
7485

7586
if checkpointIsOCIImage {
76-
log.Debugf(ctx, "Restoring from oci image %s\n", input)
87+
log.Debugf(ctx, "Restoring from oci image %s\n", inputImage)
7788

78-
imageRef, err := istorage.Transport.ParseStoreReference(s.ContainerServer.StorageImageServer().GetStore(), input)
89+
imageRef, err := istorage.Transport.ParseStoreReference(s.ContainerServer.StorageImageServer().GetStore(), inputImage)
7990
if err != nil {
80-
return "", fmt.Errorf("failed to parse image name: %s: %w", input, err)
91+
return "", fmt.Errorf("failed to parse image name: %s: %w", inputImage, err)
8192
}
8293
img, err := istorage.Transport.GetStoreImage(s.ContainerServer.StorageImageServer().GetStore(), imageRef)
8394
if err != nil {
@@ -87,23 +98,24 @@ func (s *Server) CRImportCheckpoint(
8798
if err != nil {
8899
return "", err
89100
}
90-
input = img.ID
101+
inputImage = img.ID
91102

92-
logrus.Debugf("Checkpoint image %s mounted at %v\n", input, mountPoint)
103+
logrus.Debugf("Checkpoint image %s mounted at %v\n", inputImage, mountPoint)
93104

94105
defer func() {
95-
if _, err := s.ContainerServer.StorageImageServer().GetStore().UnmountImage(input, true); err != nil {
96-
logrus.Errorf("Could not unmount checkpoint image %s: %q", input, err)
106+
if _, err := s.ContainerServer.StorageImageServer().GetStore().UnmountImage(inputImage, true); err != nil {
107+
logrus.Errorf("Could not unmount checkpoint image %s: %q", inputImage, err)
97108
}
98109
}()
99110
} else {
100111
// First get the container definition from the
101112
// tarball to a temporary directory
102-
archiveFile, err := os.Open(input)
113+
archiveFile, err := os.Open(inputImage)
103114
if err != nil {
104-
return "", fmt.Errorf("failed to open checkpoint archive %s for import: %w", input, err)
115+
return "", fmt.Errorf("failed to open checkpoint archive %s for import: %w", inputImage, err)
105116
}
106117
defer errorhandling.CloseQuiet(archiveFile)
118+
107119
options := &archive.TarOptions{
108120
// Here we only need the files config.dump and spec.dump
109121
ExcludePatterns: []string{
@@ -151,69 +163,25 @@ func (s *Server) CRImportCheckpoint(
151163
ctrID = ""
152164
}
153165

154-
ctrMetadata := types.ContainerMetadata{}
155166
originalAnnotations := make(map[string]string)
156-
originalLabels := make(map[string]string)
157-
158-
if dumpSpec.Annotations[annotations.ContainerManager] == "libpod" {
159-
// This is an import from Podman
160-
ctrMetadata.Name = config.Name
161-
ctrMetadata.Attempt = 0
162-
} else {
163-
if err := json.Unmarshal([]byte(dumpSpec.Annotations[annotations.Metadata]), &ctrMetadata); err != nil {
164-
return "", fmt.Errorf("failed to read %q: %w", annotations.Metadata, err)
165-
}
166-
if createConfig.Metadata != nil && createConfig.Metadata.Name != "" {
167-
ctrMetadata.Name = createConfig.Metadata.Name
168-
}
169-
if err := json.Unmarshal([]byte(dumpSpec.Annotations[annotations.Annotations]), &originalAnnotations); err != nil {
170-
return "", fmt.Errorf("failed to read %q: %w", annotations.Annotations, err)
171-
}
172-
173-
if err := json.Unmarshal([]byte(dumpSpec.Annotations[annotations.Labels]), &originalLabels); err != nil {
174-
return "", fmt.Errorf("failed to read %q: %w", annotations.Labels, err)
175-
}
176-
if sandboxUID != "" {
177-
if _, ok := originalLabels[kubetypes.KubernetesPodUIDLabel]; ok {
178-
originalLabels[kubetypes.KubernetesPodUIDLabel] = sandboxUID
179-
}
180-
if _, ok := originalAnnotations[kubetypes.KubernetesPodUIDLabel]; ok {
181-
originalAnnotations[kubetypes.KubernetesPodUIDLabel] = sandboxUID
182-
}
183-
}
184-
185-
if createLabels != nil {
186-
fixupLabels := []string{
187-
// Update the container name. It has already been update in metadata.Name.
188-
// It also needs to be updated in the container labels.
189-
kubetypes.KubernetesContainerNameLabel,
190-
// Update pod name in the labels.
191-
kubetypes.KubernetesPodNameLabel,
192-
// Also update namespace.
193-
kubetypes.KubernetesPodNamespaceLabel,
194-
}
195167

196-
for _, annotation := range fixupLabels {
197-
_, ok1 := createLabels[annotation]
198-
_, ok2 := originalLabels[annotation]
168+
if err := json.Unmarshal([]byte(dumpSpec.Annotations[annotations.Annotations]), &originalAnnotations); err != nil {
169+
return "", fmt.Errorf("failed to read %q: %w", annotations.Annotations, err)
170+
}
199171

200-
// If the value is not set in the original container or
201-
// if it is not set in the new container, just skip
202-
// the step of updating metadata.
203-
if ok1 && ok2 {
204-
originalLabels[annotation] = createLabels[annotation]
205-
}
206-
}
172+
if sandboxUID != "" {
173+
if _, ok := originalAnnotations[kubetypes.KubernetesPodUIDLabel]; ok {
174+
originalAnnotations[kubetypes.KubernetesPodUIDLabel] = sandboxUID
207175
}
176+
}
208177

209-
if createAnnotations != nil {
210-
// The hash also needs to be update or Kubernetes thinks the container needs to be restarted
211-
_, ok1 := createAnnotations["io.kubernetes.container.hash"]
212-
_, ok2 := originalAnnotations["io.kubernetes.container.hash"]
178+
if createAnnotations != nil {
179+
// The hash also needs to be update or Kubernetes thinks the container needs to be restarted
180+
_, ok1 := createAnnotations["io.kubernetes.container.hash"]
181+
_, ok2 := originalAnnotations["io.kubernetes.container.hash"]
213182

214-
if ok1 && ok2 {
215-
originalAnnotations["io.kubernetes.container.hash"] = createAnnotations["io.kubernetes.container.hash"]
216-
}
183+
if ok1 && ok2 {
184+
originalAnnotations["io.kubernetes.container.hash"] = createAnnotations["io.kubernetes.container.hash"]
217185
}
218186
}
219187

@@ -239,8 +207,8 @@ func (s *Server) CRImportCheckpoint(
239207

240208
containerConfig := &types.ContainerConfig{
241209
Metadata: &types.ContainerMetadata{
242-
Name: ctrMetadata.Name,
243-
Attempt: ctrMetadata.Attempt,
210+
Name: createConfig.Metadata.Name,
211+
Attempt: createConfig.Metadata.Attempt,
244212
},
245213
Image: &types.ImageSpec{
246214
Image: func() string {
@@ -268,14 +236,19 @@ func (s *Server) CRImportCheckpoint(
268236
SecurityContext: &types.LinuxContainerSecurityContext{},
269237
},
270238
Annotations: originalAnnotations,
271-
Labels: originalLabels,
239+
// The labels are nod changed or adapted. They are just taken from the CRI
240+
// request without any modification (in contrast to the annotations).
241+
Labels: createLabels,
272242
}
273243

274-
if createConfig.Linux.Resources != nil {
275-
containerConfig.Linux.Resources = createConfig.Linux.Resources
276-
}
277-
if createConfig.Linux.SecurityContext != nil {
278-
containerConfig.Linux.SecurityContext = createConfig.Linux.SecurityContext
244+
if createConfig.Linux != nil {
245+
if createConfig.Linux.Resources != nil {
246+
containerConfig.Linux.Resources = createConfig.Linux.Resources
247+
}
248+
249+
if createConfig.Linux.SecurityContext != nil {
250+
containerConfig.Linux.SecurityContext = createConfig.Linux.SecurityContext
251+
}
279252
}
280253

281254
if dumpSpec.Linux != nil {
@@ -302,6 +275,13 @@ func (s *Server) CRImportCheckpoint(
302275
"/run/.containerenv": true,
303276
}
304277

278+
// It is necessary to ensure that all bind mounts in the checkpoint archive are defined
279+
// in the create container requested coming in via the CRI. If this check would not
280+
// be here it would be possible to create a checkpoint archive that mounts some random
281+
// file/directory on the host with the user knowing as it will happen without specifying
282+
// it in the container definition.
283+
missingMount := []string{}
284+
305285
for _, m := range dumpSpec.Mounts {
306286
// Following mounts are ignored as they might point to the
307287
// wrong location and if ignored the mounts will correctly
@@ -311,31 +291,40 @@ func (s *Server) CRImportCheckpoint(
311291
}
312292
mount := &types.Mount{
313293
ContainerPath: m.Destination,
314-
HostPath: m.Source,
315294
}
316295

296+
bindMountFound := false
317297
for _, createMount := range createMounts {
318-
if createMount.ContainerPath == m.Destination {
319-
mount.HostPath = createMount.HostPath
298+
if createMount.ContainerPath != m.Destination {
299+
continue
320300
}
301+
302+
bindMountFound = true
303+
mount.HostPath = createMount.HostPath
304+
mount.Readonly = createMount.Readonly
305+
mount.Propagation = createMount.Propagation
306+
break
321307
}
322308

323-
for _, opt := range m.Options {
324-
switch opt {
325-
case "ro":
326-
mount.Readonly = true
327-
case "rprivate":
328-
mount.Propagation = types.MountPropagation_PROPAGATION_PRIVATE
329-
case "rshared":
330-
mount.Propagation = types.MountPropagation_PROPAGATION_BIDIRECTIONAL
331-
case "rslaved":
332-
mount.Propagation = types.MountPropagation_PROPAGATION_HOST_TO_CONTAINER
333-
}
309+
if !bindMountFound {
310+
missingMount = append(missingMount, m.Destination)
311+
// If one mount is missing we can skip over any further code as we have
312+
// to abort the restore process anyway. Not using break to get all missing
313+
// mountpoints in one error message.
314+
continue
334315
}
335316

336317
log.Debugf(ctx, "Adding mounts %#v", mount)
337318
containerConfig.Mounts = append(containerConfig.Mounts, mount)
338319
}
320+
if len(missingMount) > 0 {
321+
return "", fmt.Errorf(
322+
"restoring %q expects following bind mounts defined (%s)",
323+
inputImage,
324+
strings.Join(missingMount, ","),
325+
)
326+
}
327+
339328
sandboxConfig := &types.PodSandboxConfig{
340329
Metadata: &types.PodSandboxMetadata{
341330
Name: sb.Metadata().Name,
@@ -403,7 +392,7 @@ func (s *Server) CRImportCheckpoint(
403392

404393
newContainer.SetCreated()
405394
newContainer.SetRestore(true)
406-
newContainer.SetRestoreArchive(input)
395+
newContainer.SetRestoreArchive(inputImage)
407396
newContainer.SetRestoreIsOCIImage(checkpointIsOCIImage)
408397
newContainer.SetCheckpointedAt(config.CheckpointedAt)
409398

0 commit comments

Comments
 (0)