Refactor sandbox creation function arguments

- Add a dedicated list of setters for the various pod sandbox fields and use them directly on sandbox creation - Add a new method to convert a runtime spec to a sandbox and use it on restore. - Enable the revive linter and set it to a minimum amount of arguments. Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
cri-o · May 7, 2024 · c6e0fb4 · c6e0fb4
1 parent cdfc5b7
commit c6e0fb4
Show file tree

Hide file tree

Showing 13 changed files with 279 additions and 157 deletions.
diff --git a/.golangci.yml b/.golangci.yml
@@ -62,6 +62,7 @@ linters:
     - perfsprint
     - prealloc
     - reassign
+    - revive
     - rowserrcheck
     - sloglint
     - spancheck
@@ -114,7 +115,6 @@ linters:
     # - predeclared
     # - promlinter
     # - protogetter
-    # - revive
     # - tagliatelle
     # - testifylint
     # - testpackage
@@ -127,6 +127,21 @@ linters-settings:
   errcheck:
     check-type-assertions: true
     check-blank: true
+  revive:
+    rules:
+      - name: argument-limit
+        disabled: false
+        arguments: [20]
+      - name: unused-parameter
+        disabled: true
+      - name: unexported-return
+        disabled: true
+      - name: increment-decrement
+        disabled: true
+      - name: unused-parameter
+        disabled: true
+      - name: dot-imports
+        disabled: true
   gocritic:
     enabled-checks:
       - appendCombine

diff --git a/internal/factory/container/container_test.go b/internal/factory/container/container_test.go
@@ -8,7 +8,6 @@ import (
 	"time"
 
 	"github.com/cri-o/cri-o/internal/config/capabilities"
-	"github.com/cri-o/cri-o/internal/hostport"
 	"github.com/cri-o/cri-o/internal/lib"
 	"github.com/cri-o/cri-o/internal/lib/sandbox"
 	oci "github.com/cri-o/cri-o/internal/oci"
@@ -97,11 +96,8 @@ var _ = t.Describe("Container", func() {
 			mountPoint := "test"
 			configStopSignal := "test"
 
-			sb, err := sandbox.New("sandboxID", "", "", "", "test",
-				make(map[string]string), make(map[string]string), "", "",
-				&types.PodSandboxMetadata{}, "", "", false, "", "", "",
-				[]*hostport.PortMapping{}, false, currentTime, "", nil, nil)
-			Expect(err).ToNot(HaveOccurred())
+			sb := sandbox.New("sandboxID", "")
+			sb.SetLogDir("test")
 
 			image, err := sut.UserRequestedImage()
 			Expect(err).ToNot(HaveOccurred())

diff --git a/internal/hostport/hostport.go b/internal/hostport/hostport.go
@@ -42,10 +42,10 @@ const (
 
 // PortMapping represents a network port in a container
 type PortMapping struct {
-	HostPort      int32
-	ContainerPort int32
-	Protocol      v1.Protocol
-	HostIP        string
+	HostPort      int32       `json:"HostPort,omitempty"`
+	ContainerPort int32       `json:"ContainerPort,omitempty"`
+	Protocol      v1.Protocol `json:"Protocol,omitempty"`
+	HostIP        string      `json:"HostIP,omitempty"`
 }
 
 // PodPortMapping represents a pod's network state and associated container port mappings

diff --git a/internal/lib/container_server.go b/internal/lib/container_server.go
@@ -5,15 +5,13 @@ import (
 	"errors"
 	"fmt"
 	"os"
-	"path/filepath"
 	"sync"
 	"time"
 
 	"github.com/containers/common/pkg/hooks"
 	cstorage "github.com/containers/storage"
 	"github.com/containers/storage/pkg/ioutils"
 	"github.com/containers/storage/pkg/truncindex"
-	"github.com/cri-o/cri-o/internal/hostport"
 	"github.com/cri-o/cri-o/internal/lib/sandbox"
 	statsserver "github.com/cri-o/cri-o/internal/lib/stats"
 	"github.com/cri-o/cri-o/internal/log"
@@ -175,12 +173,8 @@ func (c *ContainerServer) LoadSandbox(ctx context.Context, id string) (sb *sandb
 	if err := json.Unmarshal(config, &m); err != nil {
 		return nil, fmt.Errorf("error unmarshalling sandbox spec: %w", err)
 	}
-	labels := make(map[string]string)
-	if err := json.Unmarshal([]byte(m.Annotations[annotations.Labels]), &labels); err != nil {
-		return nil, fmt.Errorf("error unmarshalling %s annotation: %w", annotations.Labels, err)
-	}
-	name := m.Annotations[annotations.Name]
-	name, err = c.ReservePodName(id, name)
+
+	name, err := c.ReservePodName(id, m.Annotations[annotations.Name])
 	if err != nil {
 		return nil, err
 	}
@@ -189,59 +183,11 @@ func (c *ContainerServer) LoadSandbox(ctx context.Context, id string) (sb *sandb
 			c.ReleasePodName(name)
 		}
 	}()
-	var metadata types.PodSandboxMetadata
-	if err := json.Unmarshal([]byte(m.Annotations[annotations.Metadata]), &metadata); err != nil {
-		return nil, fmt.Errorf("error unmarshalling %s annotation: %w", annotations.Metadata, err)
-	}
-
-	processLabel := m.Process.SelinuxLabel
-	mountLabel := m.Linux.MountLabel
 
-	spp := m.Annotations[annotations.SeccompProfilePath]
-
-	kubeAnnotations := make(map[string]string)
-	if err := json.Unmarshal([]byte(m.Annotations[annotations.Annotations]), &kubeAnnotations); err != nil {
-		return nil, fmt.Errorf("error unmarshalling %s annotation: %w", annotations.Annotations, err)
-	}
-
-	portMappings := []*hostport.PortMapping{}
-	if err := json.Unmarshal([]byte(m.Annotations[annotations.PortMappings]), &portMappings); err != nil {
-		return nil, fmt.Errorf("error unmarshalling %s annotation: %w", annotations.PortMappings, err)
-	}
-
-	privileged := isTrue(m.Annotations[annotations.PrivilegedRuntime])
-	hostNetwork := isTrue(m.Annotations[annotations.HostNetwork])
-	nsOpts := types.NamespaceOption{}
-	if err := json.Unmarshal([]byte(m.Annotations[annotations.NamespaceOptions]), &nsOpts); err != nil {
-		return nil, fmt.Errorf("error unmarshalling %s annotation: %w", annotations.NamespaceOptions, err)
-	}
-
-	created, err := time.Parse(time.RFC3339Nano, m.Annotations[annotations.Created])
-	if err != nil {
-		return nil, fmt.Errorf("parsing created timestamp annotation: %w", err)
-	}
-
-	podLinuxOverhead := types.LinuxContainerResources{}
-	if v, found := m.Annotations[annotations.PodLinuxOverhead]; found {
-		if err := json.Unmarshal([]byte(v), &podLinuxOverhead); err != nil {
-			return nil, fmt.Errorf("error unmarshalling %s annotation: %w", annotations.PodLinuxOverhead, err)
-		}
-	}
-
-	podLinuxResources := types.LinuxContainerResources{}
-	if v, found := m.Annotations[annotations.PodLinuxResources]; found {
-		if err := json.Unmarshal([]byte(v), &podLinuxResources); err != nil {
-			return nil, fmt.Errorf("error unmarshalling %s annotation: %w", annotations.PodLinuxResources, err)
-		}
-	}
-
-	sb, err = sandbox.New(id, m.Annotations[annotations.Namespace], name, m.Annotations[annotations.KubeName], filepath.Dir(m.Annotations[annotations.LogPath]), labels, kubeAnnotations, processLabel, mountLabel, &metadata, m.Annotations[annotations.ShmPath], m.Annotations[annotations.CgroupParent], privileged, m.Annotations[annotations.RuntimeHandler], m.Annotations[annotations.ResolvPath], m.Annotations[annotations.HostName], portMappings, hostNetwork, created, m.Annotations[annotations.UsernsModeAnnotation], &podLinuxOverhead, &podLinuxResources)
+	sb, err = sandbox.FromSpec(id, &m)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("sandbox from spec: %w", err)
 	}
-	sb.AddHostnamePath(m.Annotations[annotations.HostnamePath])
-	sb.SetSeccompProfilePath(spp)
-	sb.SetNamespaceOptions(&nsOpts)
 
 	defer func() {
 		if retErr != nil {
@@ -293,13 +239,14 @@ func (c *ContainerServer) LoadSandbox(ctx context.Context, id string) (sb *sandb
 		wasSpoofed = true
 	}
 
+	created := time.Unix(0, sb.CreatedAt())
 	if !wasSpoofed {
-		scontainer, err = oci.NewContainer(m.Annotations[annotations.ContainerID], cname, sandboxPath, m.Annotations[annotations.LogPath], labels, m.Annotations, kubeAnnotations, m.Annotations[annotations.Image], nil, nil, "", nil, id, false, false, false, sb.RuntimeHandler(), sandboxDir, created, m.Annotations["org.opencontainers.image.stopSignal"])
+		scontainer, err = oci.NewContainer(m.Annotations[annotations.ContainerID], cname, sandboxPath, m.Annotations[annotations.LogPath], sb.Labels(), m.Annotations, sb.Annotations(), m.Annotations[annotations.Image], nil, nil, "", nil, id, false, false, false, sb.RuntimeHandler(), sandboxDir, created, m.Annotations["org.opencontainers.image.stopSignal"])
 		if err != nil {
 			return sb, err
 		}
 	} else {
-		scontainer = oci.NewSpoofedContainer(cID, cname, labels, id, created, sandboxPath)
+		scontainer = oci.NewSpoofedContainer(cID, cname, sb.Labels(), id, created, sandboxPath)
 	}
 	scontainer.SetSpec(&m)
 	scontainer.SetMountPoint(m.Annotations[annotations.MountPoint])
@@ -353,7 +300,7 @@ func (c *ContainerServer) LoadSandbox(ctx context.Context, id string) (sb *sandb
 	}
 
 	sb.SetCreated()
-	if err := label.ReserveLabel(processLabel); err != nil {
+	if err := label.ReserveLabel(m.Process.SelinuxLabel); err != nil {
 		return sb, err
 	}
 

diff --git a/internal/lib/sandbox/history_test.go b/internal/lib/sandbox/history_test.go
@@ -1,13 +1,9 @@
 package sandbox_test
 
 import (
-	"time"
-
-	"github.com/cri-o/cri-o/internal/hostport"
 	"github.com/cri-o/cri-o/internal/lib/sandbox"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
-	types "k8s.io/cri-api/pkg/apis/runtime/v1"
 )
 
 // The actual test suite
@@ -17,11 +13,7 @@ var _ = t.Describe("History", func() {
 	// Prepare the sut
 	BeforeEach(func() {
 		beforeEach()
-		otherTestSandbox, err := sandbox.New("sandboxID", "", "", "", "",
-			make(map[string]string), make(map[string]string), "", "",
-			&types.PodSandboxMetadata{}, "", "", false, "", "", "",
-			[]*hostport.PortMapping{}, false, time.Now(), "", nil, nil)
-		Expect(err).ToNot(HaveOccurred())
+		otherTestSandbox := sandbox.New("sandboxID", "")
 		Expect(testSandbox).NotTo(BeNil())
 		sut = &sandbox.History{testSandbox, otherTestSandbox}
 	})