Merge pull request cri-o#8397 from littlejawa/cleanup/refactor_createSandboxContainer

openshift-merge-bot[bot] · web-flow · commit 7269e0d9ab89 · 2024-11-12T12:30:59.000Z
Cleanup/refactor create sandbox container
diff --git a/internal/config/node/cgroups_unsupported.go b/internal/config/node/cgroups_unsupported.go
@@ -3,6 +3,10 @@
 
 package node
 
+func CgroupIsV2() bool {
+	return false
+}
+
 // CgroupHasMemorySwap returns whether the memory swap controller is present
 func CgroupHasMemorySwap() bool {
 	return false
diff --git a/internal/factory/container/container.go b/internal/factory/container/container.go
@@ -24,7 +24,9 @@ import (
 	kubeletTypes "k8s.io/kubelet/pkg/types"
 
 	"github.com/cri-o/cri-o/internal/config/capabilities"
+	"github.com/cri-o/cri-o/internal/config/cgmgr"
 	"github.com/cri-o/cri-o/internal/config/device"
+	"github.com/cri-o/cri-o/internal/config/node"
 	"github.com/cri-o/cri-o/internal/config/nsmgr"
 	"github.com/cri-o/cri-o/internal/lib/constants"
 	"github.com/cri-o/cri-o/internal/log"
@@ -123,6 +125,12 @@ type Container interface {
 	// SpecSetupCapabilities sets up the container's capabilities
 	SpecSetupCapabilities(*types.Capability, capabilities.Capabilities, bool) error
 
+	// SpecSetPrivileges sets the container's privileges
+	SpecSetPrivileges(ctx context.Context, securityContext *types.LinuxContainerSecurityContext, cfg *config.Config) error
+
+	// SpecSetLinuxContainerResources sets the container resources
+	SpecSetLinuxContainerResources(resources *types.LinuxContainerResources, containerMinMemory int64) error
+
 	// PidNamespace returns the pid namespace created by SpecAddNamespaces.
 	PidNamespace() nsmgr.Namespace
 
@@ -758,3 +766,98 @@ func getOCICapabilitiesList() []string {
 	}
 	return caps
 }
+
+func (c *container) SpecSetPrivileges(ctx context.Context, securityContext *types.LinuxContainerSecurityContext, cfg *config.Config) error {
+	specgen := c.Spec()
+	if c.Privileged() {
+		specgen.SetupPrivileged(true)
+	} else {
+		caps := securityContext.Capabilities
+		if err := c.SpecSetupCapabilities(caps, cfg.DefaultCapabilities, cfg.AddInheritableCapabilities); err != nil {
+			return err
+		}
+	}
+
+	if securityContext.NoNewPrivs {
+		const sysAdminCap = "CAP_SYS_ADMIN"
+		for _, cap := range specgen.Config.Process.Capabilities.Bounding {
+			if cap == sysAdminCap {
+				log.Warnf(ctx, "Setting `noNewPrivileges` flag has no effect because container has %s capability", sysAdminCap)
+			}
+		}
+
+		if c.Privileged() {
+			log.Warnf(ctx, "Setting `noNewPrivileges` flag has no effect because container is privileged")
+		}
+	}
+
+	specgen.SetProcessNoNewPrivileges(securityContext.NoNewPrivs)
+
+	if !c.Privileged() {
+		if securityContext.MaskedPaths != nil {
+			for _, path := range securityContext.MaskedPaths {
+				specgen.AddLinuxMaskedPaths(path)
+			}
+		}
+
+		if securityContext.ReadonlyPaths != nil {
+			for _, path := range securityContext.ReadonlyPaths {
+				specgen.AddLinuxReadonlyPaths(path)
+			}
+		}
+	}
+	return nil
+}
+
+func (c *container) SpecSetLinuxContainerResources(resources *types.LinuxContainerResources, containerMinMemory int64) error {
+	specgen := c.Spec()
+	specgen.SetLinuxResourcesCPUPeriod(uint64(resources.CpuPeriod))
+	specgen.SetLinuxResourcesCPUQuota(resources.CpuQuota)
+	specgen.SetLinuxResourcesCPUShares(uint64(resources.CpuShares))
+
+	memoryLimit := resources.MemoryLimitInBytes
+	if memoryLimit != 0 {
+		if err := cgmgr.VerifyMemoryIsEnough(memoryLimit, containerMinMemory); err != nil {
+			return err
+		}
+		specgen.SetLinuxResourcesMemoryLimit(memoryLimit)
+		if resources.MemorySwapLimitInBytes != 0 {
+			if resources.MemorySwapLimitInBytes > 0 && resources.MemorySwapLimitInBytes < resources.MemoryLimitInBytes {
+				return fmt.Errorf(
+					"container %s create failed because memory swap limit (%d) cannot be lower than memory limit (%d)",
+					c.ID(),
+					resources.MemorySwapLimitInBytes,
+					resources.MemoryLimitInBytes,
+				)
+			}
+			memoryLimit = resources.MemorySwapLimitInBytes
+		}
+		// If node doesn't have memory swap, then skip setting
+		// otherwise the container creation fails.
+		if node.CgroupHasMemorySwap() {
+			specgen.SetLinuxResourcesMemorySwap(memoryLimit)
+		}
+	}
+
+	specgen.SetProcessOOMScoreAdj(int(resources.OomScoreAdj))
+	specgen.SetLinuxResourcesCPUCpus(resources.CpusetCpus)
+	specgen.SetLinuxResourcesCPUMems(resources.CpusetMems)
+
+	// If the kernel has no support for hugetlb, silently ignore the limits
+	if node.CgroupHasHugetlb() {
+		hugepageLimits := resources.HugepageLimits
+		for _, limit := range hugepageLimits {
+			specgen.AddLinuxResourcesHugepageLimit(limit.PageSize, limit.Limit)
+		}
+	}
+
+	if node.CgroupIsV2() && len(resources.Unified) != 0 {
+		if specgen.Config.Linux.Resources.Unified == nil {
+			specgen.Config.Linux.Resources.Unified = make(map[string]string, len(resources.Unified))
+		}
+		for key, value := range resources.Unified {
+			specgen.Config.Linux.Resources.Unified[key] = value
+		}
+	}
+	return nil
+}
diff --git a/internal/factory/container/container_test.go b/internal/factory/container/container_test.go
@@ -12,6 +12,7 @@ import (
 	v1 "github.com/opencontainers/image-spec/specs-go/v1"
 	rspec "github.com/opencontainers/runtime-spec/specs-go"
 	validate "github.com/opencontainers/runtime-tools/validate/capabilities"
+	"github.com/syndtr/gocapability/capability"
 	types "k8s.io/cri-api/pkg/apis/runtime/v1"
 	kubeletTypes "k8s.io/kubelet/pkg/types"
 
@@ -23,6 +24,7 @@ import (
 	"github.com/cri-o/cri-o/internal/storage"
 	"github.com/cri-o/cri-o/internal/storage/references"
 	"github.com/cri-o/cri-o/pkg/annotations"
+	pkgConfig "github.com/cri-o/cri-o/pkg/config"
 )
 
 var _ = t.Describe("Container", func() {
@@ -633,4 +635,213 @@ var _ = t.Describe("Container", func() {
 			Expect(sut.Spec().Config.Process.Capabilities.Inheritable).To(HaveLen(1))
 		})
 	})
+	t.Describe("SpecSetPrivileges", func() {
+		It("Non privileged container should get selected capabilities", func() {
+			// Given
+			sc := &types.LinuxContainerSecurityContext{
+				Capabilities: &types.Capability{
+					AddCapabilities:  []string{"CHOWN"},
+					DropCapabilities: nil,
+				},
+			}
+			cfg := &pkgConfig.Config{}
+
+			// When
+			Expect(sut.SpecSetPrivileges(context.Background(), sc, cfg)).To(Succeed())
+
+			// Then
+			Expect(sut.Spec().Config.Process.Capabilities.Bounding).To(HaveLen(len(cfg.DefaultCapabilities) + 1))
+			Expect(sut.Spec().Config.Process.Capabilities.Effective).To(HaveLen(len(cfg.DefaultCapabilities) + 1))
+			Expect(sut.Spec().Config.Process.Capabilities.Permitted).To(HaveLen(len(cfg.DefaultCapabilities) + 1))
+			Expect(sut.Spec().Config.Process.Capabilities.Inheritable).To(BeEmpty())
+			Expect(sut.Spec().Config.Process.Capabilities.Ambient).To(BeEmpty())
+		})
+		It("Privileged container gets all capabilities", func() {
+			// Given
+			sc := &types.LinuxContainerSecurityContext{}
+			cfg := &pkgConfig.Config{}
+			config := &types.ContainerConfig{
+				Metadata: &types.ContainerMetadata{Name: "name"},
+				Linux: &types.LinuxContainerConfig{
+					SecurityContext: &types.LinuxContainerSecurityContext{
+						Privileged: true,
+					},
+				},
+			}
+			sboxConfig := &types.PodSandboxConfig{
+				Linux: &types.LinuxPodSandboxConfig{
+					SecurityContext: &types.LinuxSandboxSecurityContext{
+						Privileged: true,
+					},
+				},
+			}
+			expectedSize := len(capability.List())
+
+			// When
+			Expect(sut.SetConfig(config, sboxConfig)).To(Succeed())
+			Expect(sut.SetPrivileged()).To(Succeed())
+			Expect(sut.SpecSetPrivileges(context.Background(), sc, cfg)).To(Succeed())
+
+			// Then
+			Expect(sut.Spec().Config.Process.Capabilities.Bounding).To(HaveLen(expectedSize))
+			Expect(sut.Spec().Config.Process.Capabilities.Effective).To(HaveLen(expectedSize))
+			Expect(sut.Spec().Config.Process.Capabilities.Permitted).To(HaveLen(expectedSize))
+			Expect(sut.Spec().Config.Process.Capabilities.Inheritable).To(HaveLen(expectedSize))
+			Expect(sut.Spec().Config.Process.Capabilities.Ambient).To(HaveLen(expectedSize))
+		})
+		It("Should set NoNewPrivs flag if set", func() {
+			// Given
+			sc := &types.LinuxContainerSecurityContext{
+				NoNewPrivs: true,
+			}
+			cfg := &pkgConfig.Config{}
+
+			// When
+			Expect(sut.SpecSetPrivileges(context.Background(), sc, cfg)).To(Succeed())
+
+			// Then
+			Expect(sut.Spec().Config.Process.NoNewPrivileges).To(BeTrue())
+		})
+		It("Should add masked paths if set", func() {
+			// Given
+			sc := &types.LinuxContainerSecurityContext{
+				MaskedPaths: []string{"path1", "path2"},
+			}
+			cfg := &pkgConfig.Config{}
+
+			// When
+			Expect(sut.SpecSetPrivileges(context.Background(), sc, cfg)).To(Succeed())
+
+			// Then
+			Expect(sut.Spec().Config.Linux.MaskedPaths).To(HaveLen(2))
+		})
+		It("Should add readonly paths if set", func() {
+			// Given
+			sc := &types.LinuxContainerSecurityContext{
+				ReadonlyPaths: []string{"path1", "path2"},
+			}
+			cfg := &pkgConfig.Config{}
+
+			// When
+			Expect(sut.SpecSetPrivileges(context.Background(), sc, cfg)).To(Succeed())
+
+			// Then
+			Expect(sut.Spec().Config.Linux.ReadonlyPaths).To(HaveLen(2))
+		})
+	})
+	t.Describe("SpecSetLinuxContainerResources", func() {
+		It("Sets all fields to their expected values", func() {
+			// Given
+			resources := &types.LinuxContainerResources{
+				CpuPeriod:   1,
+				CpuQuota:    2,
+				CpuShares:   3,
+				OomScoreAdj: 4,
+				CpusetCpus:  "5",
+				CpusetMems:  "6",
+			}
+
+			// When
+			Expect(sut.SpecSetLinuxContainerResources(resources, 0)).To(Succeed())
+
+			// Then
+			Expect(*sut.Spec().Config.Linux.Resources.CPU.Period).To(Equal(uint64(resources.CpuPeriod)))
+			Expect(*sut.Spec().Config.Linux.Resources.CPU.Quota).To(Equal(resources.CpuQuota))
+			Expect(*sut.Spec().Config.Linux.Resources.CPU.Shares).To(Equal(uint64(resources.CpuShares)))
+			Expect(*sut.Spec().Config.Process.OOMScoreAdj).To(Equal(int(resources.OomScoreAdj)))
+			Expect(sut.Spec().Config.Linux.Resources.CPU.Cpus).To(Equal(resources.CpusetCpus))
+			Expect(sut.Spec().Config.Linux.Resources.CPU.Mems).To(Equal(resources.CpusetMems))
+		})
+		It("Fails to set memory limit if invalid", func() {
+			// Given
+			minMemory := int64(2048)
+
+			// When
+			resources := &types.LinuxContainerResources{
+				MemoryLimitInBytes: 1024, // must be >= minMemory
+			}
+
+			// Then
+			Expect(sut.SpecSetLinuxContainerResources(resources, minMemory)).NotTo(Succeed())
+		})
+		It("Fails to set memory swap limit if invalid", func() {
+			// Given
+			minMemory := int64(2048)
+
+			// When
+			resources := &types.LinuxContainerResources{
+				MemoryLimitInBytes:     2048,
+				MemorySwapLimitInBytes: 1024, // must be >= MemoryLimitInBytes
+			}
+
+			// Then
+			Expect(sut.SpecSetLinuxContainerResources(resources, minMemory)).NotTo(Succeed())
+		})
+		It("Set memory limit to both swap and RAM when only MemoryLimit is set", func() {
+			// Given
+			resources := &types.LinuxContainerResources{
+				MemoryLimitInBytes: 4096,
+			}
+
+			// When
+			Expect(sut.SpecSetLinuxContainerResources(resources, 2048)).To(Succeed())
+
+			// Then
+			Expect(*sut.Spec().Config.Linux.Resources.Memory.Limit).To(Equal(resources.MemoryLimitInBytes))
+			Expect(*sut.Spec().Config.Linux.Resources.Memory.Swap).To(Equal(resources.MemoryLimitInBytes))
+		})
+		It("Set memory limits appropriately when Limit and SwapLimit are set", func() {
+			// Given
+			resources := &types.LinuxContainerResources{
+				MemoryLimitInBytes:     4096,
+				MemorySwapLimitInBytes: 4096,
+			}
+
+			// When
+			Expect(sut.SpecSetLinuxContainerResources(resources, 0)).To(Succeed())
+
+			// Then
+			Expect(*sut.Spec().Config.Linux.Resources.Memory.Limit).To(Equal(resources.MemoryLimitInBytes))
+			Expect(*sut.Spec().Config.Linux.Resources.Memory.Swap).To(Equal(resources.MemorySwapLimitInBytes))
+		})
+		It("Set hugepage limits", func() {
+			// Given
+			hugepageLimits := []*types.HugepageLimit{
+				{
+					PageSize: "1KB",
+					Limit:    1024,
+				},
+				{
+					PageSize: "2KB",
+					Limit:    2048,
+				},
+			}
+			resources := &types.LinuxContainerResources{
+				HugepageLimits: hugepageLimits,
+			}
+
+			// When
+			Expect(sut.SpecSetLinuxContainerResources(resources, 0)).To(Succeed())
+
+			// Then
+			for i, pageLimit := range sut.Spec().Config.Linux.Resources.HugepageLimits {
+				Expect(pageLimit.Pagesize).To(Equal(hugepageLimits[i].PageSize))
+				Expect(pageLimit.Limit).To(Equal(hugepageLimits[i].Limit))
+			}
+		})
+		It("Set Cgroupv2 resources", func() {
+			// Given
+			resources := &types.LinuxContainerResources{
+				Unified: make(map[string]string, 2),
+			}
+			resources.Unified["memory.high"] = "8000000"
+			resources.Unified["memory.low"] = "100000"
+
+			// When
+			Expect(sut.SpecSetLinuxContainerResources(resources, 2048)).To(Succeed())
+
+			// Then
+			Expect(sut.Spec().Config.Linux.Resources.Unified).To(HaveLen(len(resources.Unified)))
+		})
+	})
 })
diff --git a/server/container_create_linux.go b/server/container_create_linux.go
