[WIP-Refactor part 1] Move container mounts to internal/factory/container

.gitignore

@@ -13,6 +13,7 @@
 /test/checkseccomp/checkseccomp
 /test/checkcriu/checkcriu
 /test/copyimg/copyimg
+test/testdata/checkpoint.json
 /build
 coverprofile
 .gdb_history

Containerfile

@@ -0,0 +1,58 @@
+FROM fedora:latest
+
+RUN dnf update -y && \
+        dnf install -y jq \
+        vim \
+        systemd \
+        bats \
+        cri-tools \
+        containernetworking-plugins \
+        conmon \
+        containers-common \
+        device-mapper-devel \
+        git \
+        make \
+        glib2-devel \
+        glibc-devel \
+        glibc-static \
+        runc \
+        libassuan \
+        libassuan-devel \
+        libgpg-error \
+        libseccomp-devel \
+        libselinux \
+        pkgconf-pkg-config \
+        gpgme-devel \
+        gcc-go \
+        btrfs-progs-devel \
+        python3 \
+        socat \
+        nftables \
+        iptables-nft \
+        net-tools \
+        procps \
+        wget \
+        bash-completion \
+        buildah \
+        openssl \
+        python \
+        iputils \
+        iproute \
+        podman
+
+WORKDIR /root
+
+RUN  echo "containers:100000:65536" | tee -a /etc/subuid && \
+        echo "containers:100000:65536" | tee -a /etc/subgid && \
+        printf "RateLimitInterval=0\nRateLimitBurst=0\n" | tee /etc/systemd/journald.conf && \
+        mkdir -p /root/go && \
+        mkdir -p /opt/cni/bin && \
+        wget https://go.dev/dl/go1.21.7.linux-amd64.tar.gz && \
+        rm -rf /usr/local/go && tar -C /usr/local -xzf go*.tar.gz && \
+        wget https://github.com/kubernetes-sigs/cri-tools/releases/download/v1.29.0/crictl-v1.29.0-linux-amd64.tar.gz && \
+        rm -rf /usr/local/bin/crictl && tar -C /usr/local/bin/ -xzf crictl-*.tar.gz && \
+        echo "export PATH=/usr/local/go/bin:$PATH" >> /root/.bashrc && \
+        echo "export GOPATH=/root/go" >> /root/.bashrc && \
+        echo "for i in \$(ls /usr/libexec/cni/);do if [ ! -f /opt/cni/bin/\$i ]; then ln -s /usr/libexec/cni/\$i /opt/cni/bin/\$i; fi done" >> /root/.bashrc
+
+CMD ["/sbin/init"]

Makefile

@@ -324,6 +324,13 @@ testunit-bin:
 			--gcflags '-N' -c -o ${TESTBIN_PATH}/$$(basename $$PACKAGE) ;\
 	done
 
+testunit-package:
+	mkdir -p ${TESTBIN_PATH}
+	go test ${PACKAGE} \
+		--tags "test $(BUILDTAGS)" \
+		--gcflags '-N' -c -o ${TESTBIN_PATH}/$$(basename ${PACKAGE})
+
+
 mockgen: \
 	mock-cmdrunner \
 	mock-containerstorage \
@@ -333,7 +340,8 @@ mockgen: \
 	mock-image-types \
 	mock-ocicni-types \
 	mock-seccompociartifact-types \
-	mock-ociartifact-types
+	mock-ociartifact-types \
+	mock-container
 
 mock-containereventserver: ${MOCKGEN}
 	${MOCKGEN} \
@@ -395,6 +403,12 @@ mock-ociartifact-types: ${MOCKGEN}
 		-destination ${MOCK_PATH}/ociartifact/ociartifact.go \
 		github.com/cri-o/cri-o/internal/config/ociartifact Impl
 
+mock-container: ${MOCKGEN}
+	${BUILD_BIN_PATH}/mockgen \
+		-package containermock \
+		-destination ${MOCK_PATH}/container/container.go \
+		github.com/cri-o/cri-o/internal/factory/container Impl
+
 codecov: SHELL := $(shell which bash)
 codecov:
 	bash <(curl -s https://codecov.io/bash) -f ${COVERAGE_PATH}/coverprofile

internal/factory/container/container.go

@@ -13,6 +13,7 @@
 	"time"
 
 	"github.com/containers/podman/v4/pkg/annotations"
+	"github.com/containers/storage/pkg/idtools"
 	"github.com/containers/storage/pkg/stringid"
 	"github.com/cri-o/cri-o/internal/config/capabilities"
 	"github.com/cri-o/cri-o/internal/config/device"
@@ -21,15 +22,16 @@
 	"github.com/cri-o/cri-o/internal/lib/sandbox"
 	"github.com/cri-o/cri-o/internal/log"
 	oci "github.com/cri-o/cri-o/internal/oci"
+	"github.com/cri-o/cri-o/internal/resourcestore"
 	"github.com/cri-o/cri-o/internal/storage"
 	crioann "github.com/cri-o/cri-o/pkg/annotations"
 	"github.com/cri-o/cri-o/pkg/config"
+	sconfig "github.com/cri-o/cri-o/pkg/config"
 	"github.com/cri-o/cri-o/utils"
 	v1 "github.com/opencontainers/image-spec/specs-go/v1"
 	rspec "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/opencontainers/runtime-tools/generate"
 	validate "github.com/opencontainers/runtime-tools/validate/capabilities"
-	"github.com/opencontainers/selinux/go-selinux/label"
 	"github.com/sirupsen/logrus"
 	"github.com/syndtr/gocapability/capability"
 	types "k8s.io/cri-api/pkg/apis/runtime/v1"
@@ -99,10 +101,12 @@
 	// returns the spec
 	Spec() *generate.Generator
 
-	// SpecAddMount adds a mount to the container's spec
-	// it takes the rspec mount object
+	// SpecAddPreOCIMounts add mounts to the container's spec before creating ocicontainer
 	// if there is already a mount at the path specified, it removes it.
-	SpecAddMount(rspec.Mount)
+	SpecAddPreOCIMounts(ctx context.Context, resourceStore *resourcestore.ResourceStore, serverConfig *sconfig.Config, sb *sandbox.Sandbox, containerInfo storage.ContainerInfo, mountPoint string, idMapSupport bool) ([]oci.ContainerVolume, []rspec.Mount, error)
+
+	// SpecAddPostOCIMounts add mounts to the container after creating oci container
+	SpecAddPostOCIMounts(ctx context.Context, serverConfig *sconfig.Config, containerInfo storage.ContainerInfo, ociContainer *oci.Container, mountPoint string, timeZone string, rootPair idtools.IDPair) error
 
 	// SpecAddAnnotations adds annotations to the spec.
 	SpecAddAnnotations(ctx context.Context, sandbox *sandbox.Sandbox, containerVolume []oci.ContainerVolume, mountPoint, configStopSignal string, imageResult *storage.ImageResult, isSystemd bool, seccompRef, platformRuntimePath string) error
@@ -141,6 +145,7 @@
 	restore    bool
 	spec       generate.Generator
 	pidns      nsmgr.Namespace
+	mountInfo  *mountInfo
 }
 
 // New creates a new, empty Sandbox instance
@@ -155,14 +160,6 @@
 	}, nil
 }
 
-// SpecAddMount adds a specified mount to the spec
-//
-//nolint:gocritic // passing the spec mount around here is intentional
-func (c *container) SpecAddMount(r rspec.Mount) {
-	c.spec.RemoveMount(r.Destination)
-	c.spec.AddMount(r)
-}
-
 // SpecAddAnnotation adds all annotations to the spec
 func (c *container) SpecAddAnnotations(ctx context.Context, sb *sandbox.Sandbox, containerVolumes []oci.ContainerVolume, mountPoint, configStopSignal string, imageResult *storage.ImageResult, isSystemd bool, seccompRef, platformRuntimePath string) (err error) {
 	ctx, span := log.StartSpan(ctx)
@@ -491,35 +488,6 @@
 	return serverIsReadOnly
 }
 
-// SelinuxLabel returns the container's SelinuxLabel
-// it takes the sandbox's label, which it falls back upon
-func (c *container) SelinuxLabel(sboxLabel string) ([]string, error) {
-	selinuxConfig := c.config.Linux.SecurityContext.SelinuxOptions
-
-	labels := map[string]string{}
-
-	labelOptions, err := label.DupSecOpt(sboxLabel)
-	if err != nil {
-		return nil, err
-	}
-	for _, r := range labelOptions {
-		k := strings.Split(r, ":")[0]
-		labels[k] = r
-	}
-
-	if selinuxConfig != nil {
-		for _, r := range utils.GetLabelOptions(selinuxConfig) {
-			k := strings.Split(r, ":")[0]
-			labels[k] = r
-		}
-	}
-	ret := []string{}
-	for _, v := range labels {
-		ret = append(ret, v)
-	}
-	return ret, nil
-}
-
 // AddUnifiedResourcesFromAnnotations adds the cgroup-v2 resources specified in the io.kubernetes.cri-o.UnifiedCgroup annotation
 func (c *container) AddUnifiedResourcesFromAnnotations(annotationsMap map[string]string) error {
 	if c.config == nil || c.config.Labels == nil {

internal/factory/container/container_test.go

@@ -35,32 +35,6 @@ var _ = t.Describe("Container", func() {
 		}
 		sboxConfig = &types.PodSandboxConfig{}
 	})
-	t.Describe("SpecAddMount", func() {
-		It("should add the mount to the spec", func() {
-			sut.SpecAddMount(rspec.Mount{
-				Destination: "test",
-				Type:        "test",
-				Source:      "test",
-				Options:     []string{"test"},
-			})
-			Expect(len(sut.Spec().Mounts())).To(Equal(defaultMounts + 1))
-		})
-		It("should add only one copy to the spec", func() {
-			sut.SpecAddMount(rspec.Mount{
-				Destination: "test",
-				Type:        "test",
-				Source:      "test",
-				Options:     []string{"test"},
-			})
-			sut.SpecAddMount(rspec.Mount{
-				Destination: "test",
-				Type:        "test",
-				Source:      "test",
-				Options:     []string{"test"},
-			})
-			Expect(len(sut.Spec().Mounts())).To(Equal(defaultMounts + 1))
-		})
-	})
 	t.Describe("Spec", func() {
 		It("should return the spec", func() {
 			Expect(sut.Spec()).ToNot(Equal(nil))

internal/factory/container/impl.go

@@ -0,0 +1,6 @@
+package container
+
+// Impl is the main implementation interface of this package.
+type Impl interface {
+	SecurityLabel(path, secLabel string, shared, maybeRelabel bool) error
+}

internal/factory/container/label.go

@@ -0,0 +1,17 @@
+package container
+
+type SecLabel struct {
+	impl Impl
+}
+
+type secLabelImpl struct{}
+
+func newSecLabel() *SecLabel {
+	return &SecLabel{
+		impl: &secLabelImpl{},
+	}
+}
+
+func SecurityLabel(path, secLabel string, shared, maybeRelabel bool) error {
+	return newSecLabel().impl.SecurityLabel(path, secLabel, shared, maybeRelabel)
+}

internal/factory/container/label_linux.go

@@ -0,0 +1,66 @@
+//go:build linux
+// +build linux
+
+package container
+
+import (
+	"errors"
+	"fmt"
+	"strings"
+
+	"github.com/cri-o/cri-o/utils"
+	selinux "github.com/opencontainers/selinux/go-selinux"
+	"github.com/opencontainers/selinux/go-selinux/label"
+	"github.com/sirupsen/logrus"
+	"golang.org/x/sys/unix"
+)
+
+func (slabel *secLabelImpl) SecurityLabel(path, secLabel string, shared, maybeRelabel bool) error {
+	if maybeRelabel {
+		canonicalSecLabel, err := selinux.CanonicalizeContext(secLabel)
+		if err != nil {
+			logrus.Errorf("Canonicalize label failed %s: %v", secLabel, err)
+		} else {
+			currentLabel, err := label.FileLabel(path)
+			if err == nil && currentLabel == canonicalSecLabel {
+				logrus.Debugf(
+					"Skipping relabel for %s, as TrySkipVolumeSELinuxLabel is true and the label of the top level of the volume is already correct",
+					path)
+				return nil
+			}
+		}
+	}
+	if err := label.Relabel(path, secLabel, shared); err != nil && !errors.Is(err, unix.ENOTSUP) {
+		return fmt.Errorf("relabel failed %s: %w", path, err)
+	}
+	return nil
+}
+
+// SelinuxLabel returns the container's SelinuxLabel
+// it takes the sandbox's label, which it falls back upon
+func (c *container) SelinuxLabel(sboxLabel string) ([]string, error) {
+	selinuxConfig := c.config.Linux.SecurityContext.SelinuxOptions
+
+	labels := map[string]string{}
+
+	labelOptions, err := label.DupSecOpt(sboxLabel)
+	if err != nil {
+		return nil, err
+	}
+	for _, r := range labelOptions {
+		k := strings.Split(r, ":")[0]
+		labels[k] = r
+	}
+
+	if selinuxConfig != nil {
+		for _, r := range utils.GetLabelOptions(selinuxConfig) {
+			k := strings.Split(r, ":")[0]
+			labels[k] = r
+		}
+	}
+	ret := []string{}
+	for _, v := range labels {
+		ret = append(ret, v)
+	}
+	return ret, nil
+}

internal/factory/container/label_test_inject.go

@@ -0,0 +1,15 @@
+//go:build test
+// +build test
+
+// All *_inject.go files are meant to be used by tests only. Purpose of this
+// files is to provide a way to inject mocked data into the current setup.
+
+package container
+
+func (label *SecLabel) SetImpl(impl Impl) {
+	label.impl = impl
+}
+
+func NewSecLabel() *SecLabel {
+	return newSecLabel()
+}

internal/factory/container/label_unsupported.go

@@ -0,0 +1,14 @@
+//go:build !linux
+// +build !linux
+
+package container
+
+func (slabel *secLabelImpl) SecurityLabel(path string, seclabel string, shared, maybeRelabel bool) error {
+	return nil
+}
+
+// SelinuxLabel returns the container's SelinuxLabel
+// it takes the sandbox's label, which it falls back upon
+func (c *container) SelinuxLabel(sboxLabel string) ([]string, error) {
+	return []string{}, nil
+}